Mt8186 pcm #3733

chunxu-li · 2022-06-30T13:32:52Z

Hi ,
This series introduce the support of MT8186 SOF audio driver.

sound/soc/sof/mediatek/mt8186/mt8186.c

Add dsp ops callback to register AFE DL1/DL2/UL1/UL2 SOF dai's with ALSA Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

Use generic IPC stream and mailbox ops for mt8186 Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

Use generic sof_ipc_msg_data instead of specific implementation as they do the same things Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

ranj063 · 2022-07-01T03:51:52Z

include/sound/sof.h

@dbaluta out of curiosity how do you handle for the IMX platforms?

@ranj063, in our internal tree we read this info from device tree but in a very non standard way.

https://www.spinics.net/lists/devicetree/msg433908.html

We got lots of advice from community on how to handle this, but after several iterations i got busy with other stuff. I think @chunxu-li is the way to go. Will update with similar patches for i.MX

Thanks @dbaluta this is what I wanted to know i fthis will be used in other platforms as well.

ranj063 · 2022-07-01T16:32:37Z

include/sound/sof.h

Thanks @dbaluta this is what I wanted to know i fthis will be used in other platforms as well.

sound/soc/sof/mediatek/mt8186/mt8186.c

ujfalusi · 2022-07-04T08:45:22Z

sound/soc/sof/pcm.c

Should somehow the pd->ignore_machine initialized to something not NULL for non ACPI platforms?

Hi @ujfalusi , current patch can resolve this pull request as well.

The 'ignore_machine' field is currently used to ignore all FE dailinks statically added by the machine drivers, as well as override the fixups for the BE dailinks. The motivation for this field was primarily to reuse the same machine driver on Intel devices, both with legacy and SOF-based platform drivers.
The other platforms, where the same card may uses SOF-based dailinks to deal with DSP-managed streams, as well as 'regular' dailinks. The 'ignore_machine' field set by the core SOF platform driver is too strong, with dailinks not managed by SOF being modified.

update as

if (plat_data->machine) drv_name = plat_data->machine->drv_name; else if (plat_data->of_machine) drv_name = plat_data->of_machine->drv_name; else drv_name = NULL;

sound/soc/sof/mediatek/mt8186/mt8186.c

sound/soc/sof/sof-priv.h

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

From current design in sof_machine_check, the SOF can only support ACPI based machine, We introduce snd_sof_of_machine_select to give a chance for the machine which do not based on ACPI, this callback is optional, each machine that do not based on ACPI can implement their own specific routine. Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

As the mt8186 do not use ACPI based machine, the of_machine_select is used to distinguish different machines in sof_machine_check. Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

plbossart · 2022-07-05T14:58:52Z

sound/soc/sof/ops.h

+snd_sof_of_machine_select(struct snd_sof_dev *sdev)
+{
+	if (sof_ops(sdev) && sof_ops(sdev)->of_machine_select)
+		return sof_ops(sdev)->of_machine_select(sdev);


I don't understand the definitions at all. In the DT world, the machine is already described with a device, which will be matched with a driver already. What does 'of_machine_select' mean in this context?

do you mean 'a machine is already described with a device using device tree, right?'. Device tree describes the hardware, while we need information like machine driver name, tplg name, firmware name which according to Mark cannot be placed inside a DT node.

https://www.spinics.net/lists/devicetree/msg433913.html

plbossart · 2022-07-05T14:59:56Z

include/sound/sof.h

 	/* machine */
 	struct platform_device *pdev_mach;
 	const struct snd_soc_acpi_mach *machine;
+	const struct snd_sof_of_mach *of_machine;


grammar in commit message

a chance for the machines which are not based on ACPI, this callback
is optional, each machine that is not based on ACPI can implement

@plbossart do you think it makes better sense to make the machine field const void * in snd_sof_pdata and resolve it to either snd_soc_acpi_mach or and_sof_of_mach in the platform dependent code?

possibly, but it's not clear to me by the DT/OF machines need something in the kernel when everything is already described in platform firmware. That's the big difference between DT/OF and ACPI, in ACPI we do not have a device entry for the machine driver, so we do all this machine driver selection. It's not needed for DT/OF, so it's really questionable why we would build on something that was a work-around ACPI limitations.

@plbossart @ranj063 , Sorry for confusion.
If we do not implement the machine_select callback the sof_machine_check will return -ENODEV. what we actually want is do not return error since we don't need machine_select with DT/OF machines. What's more the sof_machine_select not just checking ACPI machine, but also assign sof_tplg_filename at the same time. Do you have any ideal how to handle this?

int sof_machine_check(struct snd_sof_dev *sdev) { //... if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) { /* return ACPI machine, this will return NULL with DT/OF machines */ mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; snd_sof_set_mach_params(mach, sdev); return 0; } /* this will lead to machine check fail. */ if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) { dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n"); return -ENODEV; } // ... }

@plbossart that is indeed a valid question. We need to find the ACPI machine and register it in sof_machine_register(). Without the need to explicitly register the machine driver, I cant explain what the need for change for OF devices would be for. @dbaluta any ideas?

Hi @ranj063 @plbossart @dbaluta The 'ACPI machine' should be selected before register by sof_machine_register, then it looks like the machine_select and machine_register should use at the same time when using ACPI machine.

How about

we add a condition before machine_select?
or

add a flag to check whether the current machine is ACPI or not?

1.check whether the machine_register is implement or not, if the machine_register is implemented then the machine must be checked before register.

int sof_machine_check(struct snd_sof_dev *sdev) { //... if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) { /* acpi machine should implement machine register */ if (sof_ops(dev) && sof_ops(dev)->machine_register) { /* return ACPI machine */ mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; snd_sof_set_mach_params(mach, sdev); return 0; } } else { /* of machine device return directly */ return 0; } /* this will lead to machine check fail. */ if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) { dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n"); return -ENODEV; } // ... }

add a flag in sof_dev_desc to check whether the current machine is ACPI or not?

int sof_machine_check(struct snd_sof_dev *sdev) { //... const struct sof_dev_desc *desc = sof_pdata->desc; if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) { /* acpi machine should select machine before register */ if (desc->is_acpi_machine) { /* return ACPI machine */ mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; snd_sof_set_mach_params(mach, sdev); return 0; } } else { /* of machine device return directly */ return 0; } /* this will lead to machine check fail. */ if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) { dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n"); return -ENODEV; } // ... }

@plbossart,
From devicetree people comment, it's not ok we put fw/tplg filenames in DT. I think ACPI and DT are different specifications and it's not a work-around.

We need find a way for sof_machine_check func.

Please let me know if you have any comments. thanks.

chunxu-li · 2022-07-04T13:23:49Z

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

Hi @dbaluta I checked this link https://www.spinics.net/lists/devicetree/msg433908.html
the following members should meet your requirement, do you have any other advice?
drv_name
tplg_filename
fw_filename

@chunxu-li what I don't understand from your patch is where do we get for example drv_name.

Mark, here https://www.spinics.net/lists/devicetree/msg433913.html advised that this should be taken from something like sof_dev_desc.

Hi @dbaluta :

this should be taken from something like sof_dev_desc.
Yes, We add this mach info into sof_of_mt8186_desc.of_machines field, and assign drv_name in function snd_sof_new_platform_drv at sof/pcm.c
code example

static struct snd_sof_of_mach sof_mt8186_machs[] = { { .board = "google,kingler", .sof_tplg_filename = "sof-mt8186-mt6366-da7219-max98357.tplg", .drv_name = "mt8186-mt6366-da7219-max98357" }, { .board = "google,krabby", .sof_tplg_filename = "sof-mt8186-mt6366-rt1019-rt5682s.tplg", .drv_name = "mt8186-mt6366-rt1019-rt5682s" }, {} }; static const struct sof_dev_desc sof_of_mt8186_desc = { .of_machines = sof_mt8186_machs,

// sof/pcm.c // void snd_sof_new_platform_drv(struct snd_sof_dev *sdev) if (plat_data->machine) drv_name = plat_data->machine->drv_name; else if (plat_data->of_machine) drv_name = plat_data->of_machine->drv_name;

I see. This works for me.

sound/soc/sof/sof-priv.h

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

chunxu-li · 2022-07-06T03:53:30Z

include/sound/sof.h

 	/* machine */
 	struct platform_device *pdev_mach;
 	const struct snd_soc_acpi_mach *machine;
+	const struct snd_sof_of_mach *of_machine;


@plbossart @ranj063 , Sorry for confusion.
If we do not implement the machine_select callback the sof_machine_check will return -ENODEV. what we actually want is do not return error since we don't need machine_select with DT/OF machines. What's more the sof_machine_select not just checking ACPI machine, but also assign sof_tplg_filename at the same time. Do you have any ideal how to handle this?

int sof_machine_check(struct snd_sof_dev *sdev) { //... if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) { /* return ACPI machine, this will return NULL with DT/OF machines */ mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; snd_sof_set_mach_params(mach, sdev); return 0; } /* this will lead to machine check fail. */ if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) { dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n"); return -ENODEV; } // ... }

chunxu-li · 2022-07-08T06:36:44Z

include/sound/sof.h

 	/* machine */
 	struct platform_device *pdev_mach;
 	const struct snd_soc_acpi_mach *machine;
+	const struct snd_sof_of_mach *of_machine;


Hi @ranj063 @plbossart @dbaluta The 'ACPI machine' should be selected before register by sof_machine_register, then it looks like the machine_select and machine_register should use at the same time when using ACPI machine.

How about

we add a condition before machine_select?
or

add a flag to check whether the current machine is ACPI or not?

1.check whether the machine_register is implement or not, if the machine_register is implemented then the machine must be checked before register.

int sof_machine_check(struct snd_sof_dev *sdev) { //... if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) { /* acpi machine should implement machine register */ if (sof_ops(dev) && sof_ops(dev)->machine_register) { /* return ACPI machine */ mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; snd_sof_set_mach_params(mach, sdev); return 0; } } else { /* of machine device return directly */ return 0; } /* this will lead to machine check fail. */ if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) { dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n"); return -ENODEV; } // ... }

add a flag in sof_dev_desc to check whether the current machine is ACPI or not?

int sof_machine_check(struct snd_sof_dev *sdev) { //... const struct sof_dev_desc *desc = sof_pdata->desc; if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) { /* acpi machine should select machine before register */ if (desc->is_acpi_machine) { /* return ACPI machine */ mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; snd_sof_set_mach_params(mach, sdev); return 0; } } else { /* of machine device return directly */ return 0; } /* this will lead to machine check fail. */ if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) { dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n"); return -ENODEV; } // ... }

Zames-Chang · 2022-07-29T03:51:40Z

Ping. We think we addressed all comments, is there anything else that needs to change?

chunxu-li requested review from ajitkupandey, bardliao, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, lyakh, plbossart, ranj063, ujfalusi and yaochunhung as code owners June 30, 2022 13:32

chunxu-li requested a review from tinghan-shen June 30, 2022 13:33

dbaluta reviewed Jun 30, 2022

View reviewed changes

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

yaochunhung reviewed Jul 1, 2022

View reviewed changes

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

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

chunxu-li added 3 commits July 1, 2022 10:31

ASoC: SOF: mediatek: Add dai driver for mt8186

2d6725f

Add dsp ops callback to register AFE DL1/DL2/UL1/UL2 SOF dai's with ALSA Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

ASoC: SOF: mediatek: add snd_sof_dsp_ops callbacks for pcm and mail box

b5f90df

Use generic IPC stream and mailbox ops for mt8186 Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

ASoC: SOF: mediatek: Use generic implementation for .ipc_msg_data field

86fef5b

Use generic sof_ipc_msg_data instead of specific implementation as they do the same things Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

chunxu-li requested review from afq984 and cujomalainey July 1, 2022 03:09

ranj063 reviewed Jul 1, 2022

View reviewed changes

kuanhsuncheng previously approved these changes Jul 1, 2022

View reviewed changes

yaochunhung previously approved these changes Jul 1, 2022

View reviewed changes

ranj063 reviewed Jul 1, 2022

View reviewed changes

ujfalusi reviewed Jul 4, 2022

View reviewed changes

chunxu-li dismissed stale reviews from yaochunhung and kuanhsuncheng via 93b241a July 4, 2022 13:18

ujfalusi reviewed Jul 4, 2022

View reviewed changes

sound/soc/sof/sof-priv.h Outdated Show resolved Hide resolved

tinghan-shen reviewed Jul 5, 2022

View reviewed changes

sound/soc/sof/sof-of-dev.c Outdated Show resolved Hide resolved

sound/soc/sof/sof-of-dev.c Outdated Show resolved Hide resolved

ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

74ecbb6

As the mt8186 do not use ACPI based machine, the of_machine_select is used to distinguish different machines in sof_machine_check. Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>

plbossart reviewed Jul 5, 2022

View reviewed changes

chunxu-li commented Jul 11, 2022

View reviewed changes

chunxu-li requested review from dbaluta, ranj063 and yaochunhung and removed request for tinghan-shen July 15, 2022 07:12

dbaluta approved these changes Jul 18, 2022

View reviewed changes

kuanhsuncheng approved these changes Jul 25, 2022

View reviewed changes

yaochunhung approved these changes Jul 26, 2022

View reviewed changes

plbossart merged commit 655b48f into thesofproject:topic/sof-dev Aug 2, 2022

Mt8186 pcm #3733

Mt8186 pcm #3733

Uh oh!

Conversation

chunxu-li commented Jun 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaluta Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

chunxu-li Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

chunxu-li Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chunxu-li Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chunxu-li Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zames-Chang commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dbaluta Jul 1, 2022 •

edited

Loading

chunxu-li Jul 4, 2022 •

edited

Loading

chunxu-li Jul 6, 2022 •

edited

Loading

ranj063 Jul 6, 2022 •

edited

Loading

chunxu-li Jul 18, 2022 •

edited

Loading

chunxu-li Jul 6, 2022 •

edited

Loading