Skip to content

Conversation

@yaochunhung
Copy link

Hi,
This PR is for support mt8195 platform on SOF.
MT8195 include Cadence HiFi-4 single core, currently we support 4 DAI interfaces, SOF_DL2/DL3/UL4/UL5(Downlink/Uplink path) on MT8195 Audio Hardware(which we call AFE, the Abbreviation of Audio Front End). Some ASoC codes(the committer is Trevor) in the PR is accepted by ASoC reviewer but not in the sof-linux branch, please ignore them then help to review on SOF part.
Thanks!

Update mediatek common driver to support MT8195

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
Reported-by: kernel test robot <lkp@intel.com>
This patch adds mt8195 audio cg control.
Audio clock gates are registered to CCF for reference count and
clock parent management.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
This patch adds mt8195 tdm/i2s dai driver.

MCLK clock tree is as follows.
PLL -> MUX -> DIVIDER -> MCLK

For PLL source of MCLK, driver only supports APLL1 and APLL2 now.
APLL3 and APLL4 are used to track external clock source, so they are
only used when slave input is connected.

For example,
case 1: (HDMI RX connected)
DL memif (a1sys) -> etdm out2 (clk from apll1/apll2) -> codec
case 2: (HDMI RX disconnected)
HDMI RX -> a3sys -> UL memif (a3sys) -> DL memif (a3sys) -> .... ->
etdm out2 (clk from apll3) -> codec

We keep all modules in the pipeline working on the same clock domain.
MCLK is expected to output the clock generated from the same clock
source as the pipeline, so dynamic reparenting is required for MCLK
configuration.

As a result, clk_set_parent() is used to select PLL source,
and clk_set_rate() is used to configure divider to get MCLK output rate.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
This patch adds mt8195 adda dai driver.

audio_h clock is used by ADSP bus and ADDA module.
When ADDA requires audio_h clock, it is switched to APLL1, otherwise
it is switched to Xtal_26m so that APLL1 can be turned off when audio
feature is not used.
ADSP bus only requires that the clock is on, so dynamic reparenting
is used for the purpose of lowering power consumption.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
This patch adds mt8195 pcm dai driver.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
This patch adds mt8195 platform and affiliated driver.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
Reported-by: kernel test robot <lkp@intel.com>
This patch adds mt8195 audio afe document.

In order to support dynamic clock reparenting for ADDA and ETDM, PLL
and MUX clocks are requested even though they are not consumed by afe
directly.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
…5682

This patch adds support for mt8195 board with mt6359, rt1019 and rt5682.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
Reported-by: kernel test robot <lkp@intel.com>
This patch adds DPTX audio support on mt8195-mt6359-rt1019-rt5682 board.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
This patch adds HDMITX audio support on mt8195-mt6359-rt1019-rt5682 board.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
This patch adds document for mt8195 board with mt6359, rt1019 and rt5682

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
pd->pcm_construct = sof_pcm_new;
pd->ignore_machine = drv_name;
if (!plat_data->desc->no_ignore_machine)
pd->ignore_machine = drv_name;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't look backward compatible (default for new fields is supposed to be a 0-like value, in this case false).

Copy link
Author

@yaochunhung yaochunhung Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is zero and set as true if want to reuse original platform driver. we set it as true in the sof_of_mt8195_desc. Is there anything for my misunderstanding? Please advice.Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with @dbaluta's suggestion. Call the flag the same in your descriptor as in the ASoC driver definition, this will avoid logical inversions that are unfortunately oh so common in engineering circles.

@plbossart
Copy link
Member

Some ASoC codes(the committer is Trevor) in the PR is accepted by ASoC reviewer but not in the sof-linux branch, please ignore them then help to review on SOF part.

What patches are you referring to? We merge upstream code weekly (that includes Mark Brown and Takashi Iwai for-next branches), so everything needed should be there?

@yaochunhung
Copy link
Author

What patches are you referring to? We merge upstream code weekly (that includes Mark Brown and Takashi Iwai for-next branches), so everything needed should be there?
Below are mt8195 ASoC commits in the linux kernel.
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?qt=author&q=Trevor+Wu
Because I add the commit ea950f5 is related with machine driver. So I also add them into the PR. Thanks!

int ret = 0;

/* power on DSP */
pm_runtime_get_sync(&pdev->dev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, why do you need the get_sync() here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely wrong, sorry.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it first then check how to add PM later. Thanks.

ret = adsp_clock_on(&pdev->dev);
if (ret) {
dev_err(sdev->dev, "adsp_clock_on fail!\n");
return -EINVAL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return ret please

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. thanks!

ret = adsp_sram_power_on(sdev->dev, true);
if (ret) {
dev_err(sdev->dev, "[ADSP] adsp_sram_power_on fail!\n");
return -EINVAL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this. return ret at the end will handle it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will remove it. thanks!

return ret;
}

// init value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use /* */ fr comments please

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will fix it. Thanks!

if (ret < 0) {
dev_err(sdev->dev, "error: failed to init adsp ipi device:%d\n",
ret);
return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will remove it. Thanks!

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this series and welcome aboard.

My main objection is wrt to pm_runtime, it doesn't look like the solution allows for any suspend/resume to happen. There are also tons of style issues, please revisit.

bool use_acpi_target_states;

/*set to 1 to not use ignore_machine and still use the original platform driver*/
bool no_ignore_machine;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear why you need this for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we need to reuse original platform driver in our design, we don't want to overwrite it in the function soc_check_tplg_fes to still use original platform. Then we can route sof link to original(normal sink),
e.g. sof_conn_stream : { "ETDM2_OUT_BE", "AFE_SOF_DL2", SOF_DMA_DL2, SNDRV_PCM_STREAM_PLAYBACK}, to route sof_link : SOF_DMA_DL2 to normal link ETDM2_OUT_BE, then we can use normal link setting for audio hardware.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think going with ignore_machine name here works better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaochunhung I totally missed this. ignore_machine has nothing to do with ignoring the machine driver selected in SOF. It is more to do with how the core deals with the FE DAI links in the legacy machine driver. In your case, setting this should have no effect at all because there's no legacy driver to deal with right? @plbossart is my understanding correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 We still use legacy driver because some pcm nodes are still controlled by non-sof driver, not move to DSP. And We also want to keep to legacy BE DAI driver because the power and clock of our audio hardware are controlled by AP side. e.g ETDM2_OUT_BE. If no_ignore_machine is set as true, the legacy driver can be keep and reuse. Thanks!

Copy link
Author

@yaochunhung yaochunhung Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart In the machine driver sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c. we define g_sof_conn_streams about the connection among ETDM2_OUT_BE,SOF_DMA_DL2 and AFE_SOF_DL2.
static const struct sof_conn_stream g_sof_conn_streams[] = {
{ "ETDM2_OUT_BE", "AFE_SOF_DL2", SOF_DMA_DL2, SNDRV_PCM_STREAM_PLAYBACK},
{ "ETDM1_OUT_BE", "AFE_SOF_DL3", SOF_DMA_DL3, SNDRV_PCM_STREAM_PLAYBACK},
{ "UL_SRC1_BE", "AFE_SOF_UL4", SOF_DMA_UL4, SNDRV_PCM_STREAM_CAPTURE},
{ "ETDM2_IN_BE", "AFE_SOF_UL5", SOF_DMA_UL5, SNDRV_PCM_STREAM_CAPTURE},
};
Then we implement .late_probe = mt8195_mt6359_rt1019_rt5682_card_late_probe.
In this function, we search pcm_runtime by sof_link and normal_link then get sof's pcm runtime : sof_rtd and normal pcm runtime : normal_rtd. After that, we search the sof's runtime cpu_dai then check the widget in its path then set dapm route's source and sink then add routes.

for example, ETDM2_OUT_BE is normal_link, AFE_SOF_DL2 is sof_link,SOF_DMA_DL2 is sof_dma, and stream_dir is SNDRV_PCM_STREAM_PLAYBACK.
We will use snd_soc_dapm_widget_for_each_source_path to get the playback_widget is BUFn.x in the diagram above.
then set sink of dpam route as conn->sof_dma(SOF_DMA_DL2 in this case). After that, BUFn.x will be connected to SOF_DMA_DL2.
For SOF_DMA_DL2 to ETDM2_OUT_BE , we define the dapm route path in machine driver.
/* SOF Downlink */
{"I070", NULL, SOF_DMA_DL2},
{"I071", NULL, SOF_DMA_DL2},
And also define dapm route path in mt8195-dai-etdm.c
{"O048", "I070 Switch", "I070"},
{"O049", "I071 Switch", "I071"},
{"ETDM2 Playback", NULL, "O048"},
{"ETDM2 Playback", NULL, "O049"},
I hope this explanation is more clear. Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this explanation is more clear. Thanks.

Humm, no :-(
I am missing the big picture, you just hammered us with details that are probably correct but don't really provide a sense of what the problem statement is, if you considered alternatives and what led you to pick this solution you're asking us to review.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart
We set no_ignore_machine as true to prevent ignoring normal FE in soc_check_tplg_fes.

/* ignore this FE */
if (dai_link->dynamic) {
dai_link->ignore = true;
continue;
})

The reason is that we still need to use normal FE for the paths which don't gothrough DSP. The benifit is that we can offload the process of certain audio data(e.g. speaker/headphone) to dsp and we can reuse normal alsa driver for those audio data which don't need dsp offloading.(e.g. Display port). Thanks.

Copy link
Collaborator

@ranj063 ranj063 Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaochunhung It makes a bit more sense now. but a naive question. Is there a way for you define the Normal FE-> Normal BE in topology?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 For normal FE/BE, I think it need to communicate with DSP if defining them in topology(please correct me if my misunderstanding). It takes lower effort to reuse normal FE/BE without topology. Thanks.

#define __ADSP_HELPER_H__

#include <linux/of_irq.h>
#include <linux/platform_device.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move those to where you include this file? I don't see any dependency on platform_device below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, they are unnecessary and I will remove it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will remove them. Thanks!

}

clk_handle[CLK_TOP_AUDIO_LOCAL_BUS_SEL] = devm_clk_get(dev,
"audio_local_bus");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one line if it fits on 100 chars?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I will modify it

}

clk_handle[CLK_SCP_ADSP_AUDIODSP] = devm_clk_get(dev,
"scp_adsp_audiodsp");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify it. thanks


static int mt8195_mt6359_rt1019_rt5682_card_late_probe(struct snd_soc_card *card)
{
int i = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialization not needed, and move two lines below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify it.

{
int i = 0;
struct snd_soc_pcm_runtime *runtime = NULL;
struct snd_soc_component *sof_comp = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initializations not needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify them.

const struct sof_conn_stream *conn = &g_sof_conn_streams[i];
struct snd_soc_pcm_runtime *sof_rtd = NULL;
struct snd_soc_pcm_runtime *normal_rtd = NULL;
struct snd_soc_pcm_runtime *rtd = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revisit all initializations please.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify it.

for_each_card_prelinks(card, i, dai_link) {
if (dai_link->no_pcm && !dai_link->stream_name && dai_link->name)
dai_link->stream_name = dai_link->name;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this doing? This looks like ASoC code that's duplicated in the machine driver?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-core.c#L1851 here skip assign stream_name in soc_check_tplg_fes function when no_ignore_machineis true, so we assign them in the mt8195_mt6359_rt1019_rt5682_card_probe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not understanding the explanation, sorry.

#else
platform_node = of_parse_phandle(pdev->dev.of_node,
"mediatek,platform", 0);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't look good to me, if there is a single build where you want deal with two platforms, you need to auto-detect the platform then make a selection at run time. Compile-time selections are really problematic for distributions. @cujomalainey please comment here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry missed this comment. I am just trying to make sure I understand this code correctly. Its a compile time switch to select which driver services the platform correct? @yaochunhung does MTK plan to make SOF the default for this platform? Or do you plan to use the other driver still? @plbossart this seems like a reasonable switch for during the transition period, much like we had on GLK for a while no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cujomalainey Yes, we plan to make SOF the default for this platform. thanks!

@plbossart
Copy link
Member

@yaochunhung You will need to work with @dbaluta to update the ARM64 Kconfig so that we can compile your code in GitHub CI. See https://github.com/thesofproject/kconfig/blob/master/kconfig-sof-arm64.sh

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 27, 2021

@yaochunhung I will provide you more feedback on Monday. Care to share parts of your dts file? Also, do you reuse your non-SOF machine drivers right?

Copy link
Author

@yaochunhung yaochunhung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaochunhung I will provide you more feedback on Monday. Care to share parts of your dts file? Also, do you reuse your non-SOF machine drivers right?

@dbaluta yes, we reuse non-SOF machine driver and I can share dts file to you. Please tell me your email. Thanks!

static struct adsp_chip_info *adsp_info;

static struct snd_soc_acpi_mach sof_mt8195_mach = {
.id = "819501",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we use acpi mach for mt8195, this id is mt'8195' with version '01'

struct platform_device *pdev =
container_of(sdev->dev, struct platform_device, dev);
struct adsp_priv *priv;
int ret = 0, mailbox_type;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will modify it.

@yaochunhung
Copy link
Author

@yaochunhung I've gone your patch series and added some comments. There is still work to be done. I would suggest you take it slower with simple patches first and then go for more delicate ones.

You can run git log --oneline sound/soc/sof/imx/ and see how we added our initial support for i.MX8 boards. It is not complete, we are now sending our clocks patches, then following by PM support, then the rest of the stuff.

If you want and consider it necessary we could also setup a meeting to discuss more.

@yaochunhung yaochunhung closed this Sep 2, 2021
@yaochunhung yaochunhung reopened this Sep 2, 2021
if (ret) {
dev_err(dev, "%s clk_prepare_enable(scp_adsp_audiodsp) fail %d\n",
__func__, ret);
return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If clk_prepare_enable fails here we should call clk_disable_unprepare for the rest of the clocks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify it. thanks!

When selected, the probe is handled in two steps, for example to
avoid lockdeps if request_module is used in the probe.

source "sound/soc/sof/imx/Kconfig"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaochunhung would love to see some more details in the commit message. E.g How is the hardware configuration for your platform, what kind of architecture / cores is on application processor, what kind of DSP do you have.
Also some information about the way these two entities communicate, how are the resources managed (clocks, power domains, pinmuxing, etc).
This is useful for review.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will add more information. Thanks!

u32 (*ap2adsp_addr)(u32 addr);
u32 (*adsp2ap_addr)(u32 addr);

void *private_data;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can you provide a short memory map from AP view and DSP view. I suspect there is no 1 to 1 mapping. We have the same scenarios on one of our boards and we've implemented similar functions with ap2adsp_addr but on the firmware side not on the kernel. Not sure which is better. I dont suggest to change this if it fits your needs, but it must be explained in some comments or in the commit message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a memory remapping for DRAM between AP view and DSP view. In DSP view, the start address of EMI is 0x50000000 and in AP view is 0x40000000. In current design, I set dram of SOF between AP and DSP are the same. Both are 0x60000000. the memory remapping is zero, Could you please tell me what's the keyword for your implementation? I will check it. Thanks!

adsp = get_adsp_chip_data();
if (!adsp)
return NULL;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't delete code that was previously added in a patch of the same Pull Request. Just do not add it at all from the beginning.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify it.


/* get machine node and save it to mach->pdata */
mach->pdata = of_get_child_by_name(sdev->dev->of_node, "sound");
if (!mach->pdata)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely this needs more explanations. How to you plan to describe your machine. Maybe create finer grained patches with proper patches explanations. This should also go to review to DT maintainer.

#endif
return type;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function here is not related with this commit. Maybe add it in the initial commit with hardware support. Also, need to explain the use of iffdef.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will move it in the initial commit. Not ADSP_USE_DRAM is only used in early development stage for DSP SRAM only case, But DSP SRAM is only 256KB, I think it won't be used without DRAM. I think the definition can be removed. Thanks.

@dbaluta
Copy link
Collaborator

dbaluta commented Sep 8, 2021

@yaochunhung my suggestion for this is to submit only the obvious correct patches and then add the rest later. The amount of patches right now will make the code hard the review.
I would suggest on going with:

  • adding sound/soc/sof/mtk/ -> stuff (MTK HW support only)
  • sound-of-dev.c changes

Add PM support later.

@yaochunhung
Copy link
Author

yaochunhung commented Sep 9, 2021

@yaochunhung my suggestion for this is to submit only the obvious correct patches and then add the rest later. The amount of patches right now will make the code hard the review.
I would suggest on going with:

  • adding sound/soc/sof/mtk/ -> stuff (MTK HW support only)
  • sound-of-dev.c changes

Add PM support later.
@dbaluta Thanks for suggestion. Do you mean that I should separate my patches and only summit correct patches? Because PM runtime is also handled in probe function, should I remove it first? Thanks.

@yaochunhung yaochunhung closed this Sep 9, 2021
@yaochunhung yaochunhung reopened this Sep 9, 2021
@yaochunhung yaochunhung requested a review from ranj063 September 9, 2021 06:21
} else { /* Power down ADSP SRAM */
writel(readl(va_dspsysreg) | DSP_SRAM_POOL_PD_MASK,
va_dspsysreg);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

braces not needed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will remove them. thanks

dev_err(dev, "failed to adsp_enable_clock: %d\n", ret);
goto TAIL;
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what I meant was having a return inside the if(enable) so you wont need the else. but its only minor

return ret;
}

dev_info(dev, "[ADSP][DMA] pbase=0x%llx, size=0x%llx\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really meant to be info? or can it be dbg?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I think it can be dbg. thanks

return -EINVAL;
}

dev_info(dev, "[ADSP] dram pbase=%pa, dramsize=0x%x\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, dbg level maybe?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will modify it. thanks

if (IS_ERR(adsp->va_mboxreg[i]))
return PTR_ERR(adsp->va_mboxreg[i]);

dev_info(dev, "[ADSP]MBOX%d pa_mboxreg:%pa, va_mboxreg:%p\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please go through all th dev_info's in your series to make sure they need to be info and not dbg?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I think they could be dbg level. thanks

offset);
WARN_ON(offset < 0);
writel(offset, vaddr_emi_map);
dev_info(dev, "After vaddr_emi_map 0x%x\n", readl(vaddr_emi_map));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right im not asking to remove. just wondering if they all need to be info level or can they be dev_dbg()

static int mt8195_get_bar_index(struct snd_sof_dev *sdev, u32 type)
{
return type;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a TODO? why return an argument as is?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is one-to-one match between type and Bar index so it just return type. thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a comment to say this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will add comment for it. thanks

ch_info->ipi_op_val = op;
ret = mbox_send_message(priv->ipi[idx].chan, NULL);
if (ret < 0)
dev_err(sdev->dev, "failed to send message via mbox: %d\n", ret);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this can fail, should this function return the error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok . I will modify it. thanks.

sof_mailbox_write(sdev, sdev->host_box.offset, msg->msg_data,
msg->msg_size);

adsp_ipi_send(sdev, ADSP_IPI_MBOX_REQ, ADSP_IPI_OP_REQ);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can file, so ideally you should return the error to indicate if the msg TX failed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I will add it. thanks.

Add the definition for Mediatek audio front end(AFE) tokens,include
AFE sampling rate, channels, and format.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add new sof dai and config to pass topology file configuration
to SOF firmware running on Mediatek platform DSP core.
Add mediatek audio front end(AFE) to the list of supported sof_dais

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add no_ignore_machine to check if ignore machine, set to 1 to not use
ignore_machine and still use the original platform driver.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
This patch initialize to support SOF on Mediatek mt8195 platform.
MT8195 has four Coretex A78 cores paired with four Coretex A55 cores.
It also has Cadence HiFi-4 DSP single core. There are shared DRAM and
mailbox interrupt between AP and DSP to use for IPC communication.
The clocks of bus and dsp core are managed by CCF module. The power
domain of DSP will be handled by mtk-scpsys.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add dsp ops callback to select and register machine driver.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add dsp ops callback to register AFE DL2/DL3/UL4/UL5 sof dai's with ALSA

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add mt8195-loader module with ops callback to load and run firmware
on mt8195 platform.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Support pcm stream callback for hw_params and hw_free

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add DSP PM callback for supsend and resume

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add IPC module for mt8195 and exposed ops callback for
to synchronize SOF IPC message between host and DSP

Signed-off-by: YC Hung <yc.hung@mediatek.com>
This patch adds document for mt8195 DSP

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add SOF device and DT descriptor for Mediatek mt8195 platform.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
Add SOF platform driver and use connection table to connect sof platform
driver and normal platform driver. Use no_ignore_machine in sof device
descriptor and set as 1 to keep and reuse original platform driver.

Signed-off-by: YC Hung <yc.hung@mediatek.com>
@yaochunhung
Copy link
Author

Per discussion with @dbaluta , I will close this pull and keep it for reference. Then create new PR for basic skeleton for MTK hardware support first. Thanks.

@cujomalainey
Copy link

@yaochunhung please link the followup PRs here

@yaochunhung
Copy link
Author

yaochunhung commented Sep 15, 2021

@yaochunhung please link the followup PRs here

@cujomalainey ok. I will link them. thanks!

Basic hardware support for mt8195 : #3160 (merged)
Add PCM stream callback for mt8195 : #3190
ASoC: SOF: pcm: add sof_keep_normal_fes to keep normal fes : #3217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants