Add basic support for Mediatek mt8195 platform #3160

yaochunhung · 2021-09-16T13:36:40Z

Hi,
This PR is for basic 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 paths) on MT8195 Audio Hardware(which we call AFE, the Abbreviation of Audio Front End). This PR includes basic skeleton hardware support on MT8195, Mediatek AFE DAIs for toplolgy, fw loader ops, and dai driver defintions.
Thanks!

sound/soc/sof/mediatek/mt8195/mt8195.c

dbaluta · 2021-09-16T14:00:30Z

sound/soc/sof/mediatek/mt8195/mt8195.h

+
+/* DSP memories */
+#define MBOX_OFFSET	0x800000 // DRAM
+#define MBOX_SIZE	0x1000 // memory.h in sof fw


Please do not use C++ style comments. Use /* */

ok. I will modify it. thanks

sound/soc/sof/mediatek/mt8195/mt8195.c

dbaluta · 2021-09-16T14:05:32Z

@yaochunhung thanks. This PR is now much easier to review.

Few general comments:

Please check the order of patches. Each patch individually should compile fine, to help with bisection. First patch should be
ASoC: SOF: mediatek: Add mt8195 hardware support but maybe github shows the patches in a different order than you have in your tree.
Check sparse warnings displayed by github:

https://github.com/thesofproject/linux/pull/3160/checks?check_run_id=3621871638

   CHECK   sound/soc/sof/mediatek/mt8195/mt8195.c
sound/soc/sof/mediatek/mt8195/mt8195.c:167:22: error: incorrect type in assignment (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:167:22:    expected void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:167:22:    got void [noderef] __iomem *
sound/soc/sof/mediatek/mt8195/mt8195.c:176:30: error: incorrect type in argument 1 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:176:30:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:176:30:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:176:70: error: incorrect type in argument 2 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:176:70:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:176:70:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:178:30: error: incorrect type in argument 1 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:178:30:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:178:30:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:178:69: error: incorrect type in argument 2 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:178:69:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:178:69:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:374:24: error: symbol 'sof_mt8195_ops' was not declared. Should it be static?

yaochunhung · 2021-09-17T03:40:32Z

@yaochunhung thanks. This PR is now much easier to review.

Few general comments:

Please check the order of patches. Each patch individually should compile fine, to help with bisection. First patch should be
ASoC: SOF: mediatek: Add mt8195 hardware support but maybe github shows the patches in a different order than you have in your tree.
Check sparse warnings displayed by github:

https://github.com/thesofproject/linux/pull/3160/checks?check_run_id=3621871638

   CHECK   sound/soc/sof/mediatek/mt8195/mt8195.c
sound/soc/sof/mediatek/mt8195/mt8195.c:167:22: error: incorrect type in assignment (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:167:22:    expected void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:167:22:    got void [noderef] __iomem *
sound/soc/sof/mediatek/mt8195/mt8195.c:176:30: error: incorrect type in argument 1 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:176:30:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:176:30:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:176:70: error: incorrect type in argument 2 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:176:70:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:176:70:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:178:30: error: incorrect type in argument 1 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:178:30:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:178:30:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:178:69: error: incorrect type in argument 2 (different address spaces)
sound/soc/sof/mediatek/mt8195/mt8195.c:178:69:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/mediatek/mt8195/mt8195.c:178:69:    got void [noderef] __iomem **va_dspsysreg
sound/soc/sof/mediatek/mt8195/mt8195.c:374:24: error: symbol 'sof_mt8195_ops' was not declared. Should it be static?

@dbaluta Thanks. I reorder the patches but there is still sparse warning shown as below, The symbol 'sof_mt8195_ops' will be used in mediatek/mediatek-ops.h and cannot be set as static. Do you have any suggestion to fix it? Thanks.
CHECK sound/soc/sof/mediatek/mt8195/mt8195.c
sound/soc/sof/mediatek/mt8195/mt8195.c:371:24: error: symbol 'sof_mt8195_ops' was not declared. Should it be static?
make[5]: *** [scripts/Makefile.build:278: sound/soc/sof/mediatek/mt8195/mt8195.o] Error 1
make[5]: *** Deleting file 'sound/soc/sof/mediatek/mt8195/mt8195.o'

lyakh

nothing critical, all should be easy to address, but maybe good to do so at least for some of the comments before merging

lyakh · 2021-09-20T10:00:38Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+	}
+
+	adsp->pa_sram = (phys_addr_t)res->start;
+	adsp->srammsize = (u32)resource_size(res);


why does .srammsize have two "m"s in it?

It is typo and I will modify it. thanks

lyakh · 2021-09-20T10:04:42Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+	struct adsp_chip_info *adsp = data;
+	int i, ret;
+
+	res = &resource;


optional / cosmetic: you reuse res for different resources below. Maybe it would be better to just use &resource for the part, where that's what's needed to avoid confusion

OK. I will use struct resource res for of_address_to_resource and struct resource *mmio for platform_get_resource to avoid confusion. thanks.

lyakh · 2021-09-20T10:16:41Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+	adsp->dram_offset = offset;
+	offset >>= DRAM_REMAP_SHIFT;
+	dev_dbg(dev, "adsp->pa_dram %llx, offset 0x%x\n", adsp->pa_dram, offset);
+	WARN_ON(offset < 0);


first of all, it would be better to check for this apparent error immediately after calculating offset or maybe even before that by chacking adsp->pa_dram < DRAM_PHYS_BASE_FROM_DSP_VIEW perhaps where it's obtained from res->start. Secondly what does a WARN_ON() bring? Is this a valid value? If not, perhaps it shouldn't be used.

Thanks for suggestion but I think WARN_ON could be removed since offset can be negative value for mt8195 hardware design. thanks

lyakh · 2021-09-20T10:32:34Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+#include "../../ops.h"
+#include "../../sof-audio.h"
+
+static struct adsp_chip_info *adsp_info;


is this global variable really needed? I don't see any other platforms doing this.

ok. I will remove this global variable and use priv->adsp to replace it. thanks.

lyakh · 2021-09-20T10:34:14Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+		goto err_adsp_sram_power_off;
+	}
+	adsp_info->va_dram = sdev->bar[SOF_FW_BLK_TYPE_SRAM];
+	mailbox_type = SOF_FW_BLK_TYPE_SRAM;


not really needed, you can use SOF_FW_BLK_TYPE_SRAM directly in assignments below

ok. I will remove it. thanks

lyakh · 2021-09-20T10:35:18Z

sound/soc/sof/mediatek/mt8195/mt8195.h

+#define DSP_RESERVED_1       (0x03F4)
+
+/* dsp wdt */
+#define DSP_WDT_MODE         (0x0400)


parentheses around individual constants aren't needed

ok. I will modify it. thanks

lyakh · 2021-09-20T10:36:15Z

sound/soc/sof/mediatek/mt8195/mt8195.h

+	(SIZE_SHARED_DRAM_DL + SIZE_SHARED_DRAM_UL)
+
+#define SRAM_PHYS_BASE_FROM_DSP_VIEW  (0x40000000) /* MT8195 DSP view */
+#define DRAM_PHYS_BASE_FROM_DSP_VIEW  (0x60000000) /* MT8195 DSP view */


ditto: parentheses unneeded

ok. I will modify them. thanks

lyakh · 2021-09-20T10:40:21Z

sound/soc/sof/sof-of-dev.c

 };
 #endif
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_MT8195)
+static struct sof_dev_desc sof_of_mt8195_desc = {


ok. I will add it. thanks

lyakh · 2021-09-20T10:43:33Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+}
+
+/* mt8195 ops */
+struct snd_sof_dsp_ops sof_mt8195_ops = {


ok. I will add it. thanks

sound/soc/sof/mediatek/mt8195/mt8195.h

sound/soc/sof/mediatek/mt8195/mt8195.c

yaochunhung · 2021-09-23T03:41:31Z

@dbaluta There are conflicts in dai.h and topology.c. It seems caused by AMD's patch. Could you please help advice how to resolve it? Should I fetch the latest codes and create another PR? I tried to resolve conflicts but I don't have permission to commit merge after resolving conflicts. thanks.

dbaluta · 2021-09-23T07:26:18Z

@dbaluta There are conflicts in dai.h and topology.c. It seems caused by AMD's patch. Could you please help advice how to resolve it? Should I fetch the latest codes and create another PR? I tried to resolve conflicts but I don't have permission to commit merge after resolving conflicts. thanks.

You don't need to create another Pull Request. Just do something like this on your branch that this PR was created from:

$ git fetch origin
$ git rebase origin/topic/sof-dev
# resolve conflicts
$ git push -f -u <your_github_remote>

That should be it.

yaochunhung · 2021-09-23T09:35:12Z

@dbaluta I will try it.Thanks

plbossart

minor nit-picks below, the main issue is really that we need the ability to compile this code with GitHub actions so that we can see any compilation-level issues on this PR and any changes to the SOF tree. Thanks!

sound/soc/sof/mediatek/mt8195/mt8195.c

sound/soc/sof/Kconfig

sound/soc/sof/mediatek/Makefile

sound/soc/sof/mediatek/mt8195/mt8195.c

yaochunhung · 2021-09-30T10:00:51Z

@ujfalusi yes, IPC will be next PR. I also modify the PR by your comments. Please help to check if there is still anything I need to modify. thanks.

ujfalusi · 2021-09-30T10:14:07Z

sound/soc/sof/mediatek/mt8195/mt8195-loader.c

Just forward declare the sdev struct in the header, no need to include anything else:

struct snd_sof_dev;

ujfalusi · 2021-09-30T10:15:55Z

sound/soc/sof/mediatek/mt8195/mt8195-loader.c

I think you should have

void hifixdsp_boot_sequence(struct snd_sof_dev *sdev, u32 boot_addr)

ujfalusi · 2021-09-30T10:17:08Z

sound/soc/sof/mediatek/mt8195/mt8195-loader.c

Here you want to have

void hifixdsp_shutdown(struct snd_sof_dev *sdev)

I would probably prefix these functions with sof_

sound/soc/sof/mediatek/mt8195/mt8195.c

ujfalusi · 2021-09-30T10:39:17Z

sound/soc/sof/mediatek/mt8195/mt8195.c

Is it a good idea to do devm_ioremap() every time this function is called?

I will replace it with ioremap and iounmap. thanks.

ujfalusi · 2021-09-30T10:40:57Z

sound/soc/sof/mediatek/mt8195/mt8195.c

I might have used different name instead of val, on perhaps is better?

ok. I will modify it. thanks

ujfalusi · 2021-09-30T10:44:17Z

sound/soc/sof/mediatek/mt8195/mt8195.c

Why void *data and not directly struct mtk_adsp_chip_info *adsp?

If you need, just forward declare it in the header.

ok. I will modify it.thanks.

ujfalusi · 2021-09-30T10:47:44Z

sound/soc/sof/mediatek/mt8195/mt8195.c

This is not much of a help if it happens, it might be better to
dev_err(dev, "ioremap failed for shared DRAM\n");

ok I will modify it. thanks

ujfalusi · 2021-09-30T10:49:28Z

sound/soc/sof/mediatek/mt8195/mt8195.c

Is there real information comes out from the [ADSP] prefix, you will have the device name in prints.

ok. I will remove it. thanks

ujfalusi

@yaochunhung, thanks for the quick updates, it really looks fine, if you could align the numbers in the header I can approve it right away.

sound/soc/sof/Makefile

sound/soc/sof/mediatek/mt8195/mt8195.c

ujfalusi · 2021-10-01T12:12:38Z

sound/soc/sof/mediatek/mt8195/mt8195.h

Can you align the numbers, there is one tab line jump here.

ok. I will align them. thanks!

yaochunhung

@ujfalusi Thanks for comments. I update the patch. Please help to check it. thanks.

yaochunhung · 2021-10-01T13:33:36Z

sound/soc/sof/mediatek/mt8195/mt8195.h

ok. I will align them. thanks!

sound/soc/sof/mediatek/mt8195/mt8195.c

ujfalusi

@yaochunhung, one comment needs to be moved up one line.

sound/soc/sof/mediatek/mt8195/mt8195.h

This patch initialize to support SOF on Mediatek mt8195 platform. MT8195 has four Cortex A78 cores paired with four Cortex 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. Signed-off-by: YC Hung <yc.hung@mediatek.com>

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 mt8195-loader module with ops callback to load and run firmware on mt8195 platform. 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 dsp ops callback to register AFE DL2/DL3/UL4/UL5 sof dai's with ALSA Signed-off-by: YC Hung <yc.hung@mediatek.com>

yaochunhung

@ujfalusi I will modify it. thanks.

sound/soc/sof/mediatek/mt8195/mt8195.h

ujfalusi

@yaochunhung, thank you for the prompt updates!

The PR looks fine from my side.

yaochunhung · 2021-10-01T14:23:52Z

@ujfalusi thanks for your comments and approval. :)

plbossart · 2021-10-01T18:04:26Z

@dbaluta @lyakh @bardliao last call for reviews!

dbaluta

LGTM, sorry for the late answer I had a day off on Friday.

yaochunhung · 2021-10-04T07:58:52Z

@dbaluta Thanks a lot for your suggestion and comments.

yaochunhung requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, lyakh, plbossart, ranj063 and singalsu as code owners September 16, 2021 13:36

yaochunhung mentioned this pull request Sep 16, 2021

Add support for Mediatek mt8195 platform #3122

Closed

dbaluta reviewed Sep 16, 2021

View reviewed changes

yaochunhung force-pushed the mt8195-basic branch 2 times, most recently from ef81334 to 617aaae Compare September 17, 2021 03:17

lyakh reviewed Sep 20, 2021

View reviewed changes

dbaluta reviewed Sep 20, 2021

View reviewed changes

sound/soc/sof/mediatek/mt8195/mt8195.h Outdated Show resolved Hide resolved

sound/soc/sof/mediatek/mt8195/mt8195.c Show resolved Hide resolved

yaochunhung force-pushed the mt8195-basic branch from 617aaae to e76689b Compare September 22, 2021 13:27

yaochunhung requested review from dbaluta and lyakh September 22, 2021 13:27

yaochunhung force-pushed the mt8195-basic branch from e76689b to 88017e2 Compare September 23, 2021 09:34

dbaluta previously approved these changes Sep 23, 2021

View reviewed changes

plbossart requested changes Sep 23, 2021

View reviewed changes

sound/soc/sof/mediatek/mt8195/mt8195.c Outdated Show resolved Hide resolved

sound/soc/sof/Kconfig Outdated Show resolved Hide resolved

sound/soc/sof/mediatek/Makefile Outdated Show resolved Hide resolved

sound/soc/sof/mediatek/mt8195/mt8195.c Outdated Show resolved Hide resolved

yaochunhung dismissed dbaluta’s stale review via 263987d September 23, 2021 13:31

yaochunhung force-pushed the mt8195-basic branch 2 times, most recently from 263987d to 1d5fe57 Compare September 23, 2021 14:51

yaochunhung dismissed stale reviews from kv2019i and ranj063 via c7433a7 September 30, 2021 09:33

yaochunhung force-pushed the mt8195-basic branch 2 times, most recently from c7433a7 to 3dc11be Compare September 30, 2021 09:57

ujfalusi requested changes Sep 30, 2021

View reviewed changes

yaochunhung force-pushed the mt8195-basic branch 2 times, most recently from 86e7df1 to d5d22d8 Compare September 30, 2021 14:47

ujfalusi requested changes Oct 1, 2021

View reviewed changes

yaochunhung force-pushed the mt8195-basic branch from d5d22d8 to a325da3 Compare October 1, 2021 13:31

yaochunhung commented Oct 1, 2021

View reviewed changes

ujfalusi requested changes Oct 1, 2021

View reviewed changes

sound/soc/sof/mediatek/mt8195/mt8195.h Outdated Show resolved Hide resolved

yaochunhung added 6 commits October 1, 2021 22:07

ASoC: SOF: tokens: add token for Mediatek AFE

e5b38d2

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>

ASoC: SOF: topology: Add support for Mediatek AFE DAI

c8ffb42

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>

ASoC: SOF: mediatek: Add fw loader and mt8195 dsp ops to load firmware

30f2a1c

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

ASoC: SOF: Add mt8195 device descriptor

5cd9421

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

ASoC: SOF: mediatek: Add dai driver dsp ops callback for mt8195

124404c

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>

yaochunhung force-pushed the mt8195-basic branch from a325da3 to 124404c Compare October 1, 2021 14:09

yaochunhung commented Oct 1, 2021

View reviewed changes

sound/soc/sof/mediatek/mt8195/mt8195.h Outdated Show resolved Hide resolved

ujfalusi approved these changes Oct 1, 2021

View reviewed changes

plbossart approved these changes Oct 1, 2021

View reviewed changes

ranj063 approved these changes Oct 1, 2021

View reviewed changes

kv2019i approved these changes Oct 1, 2021

View reviewed changes

lyakh approved these changes Oct 4, 2021

View reviewed changes

dbaluta approved these changes Oct 4, 2021

View reviewed changes

dbaluta merged commit 786e304 into thesofproject:topic/sof-dev Oct 4, 2021

Add basic support for Mediatek mt8195 platform #3160

Add basic support for Mediatek mt8195 platform #3160

Uh oh!

Conversation

yaochunhung commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbaluta commented Sep 16, 2021

Uh oh!

yaochunhung commented Sep 17, 2021

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yaochunhung commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbaluta commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaochunhung commented Sep 23, 2021

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yaochunhung commented Sep 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yaochunhung commented Sep 23, 2021 •

edited

Loading

dbaluta commented Sep 23, 2021 •

edited

Loading