Skip to content

Mt8186 pcm #3733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 5 commits into from
Aug 2, 2022
Merged

Mt8186 pcm #3733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d6725f
ASoC: SOF: mediatek: Add dai driver for mt8186
chunxu-li Jun 30, 2022
b5f90df
ASoC: SOF: mediatek: add snd_sof_dsp_ops callbacks for pcm and mail box
chunxu-li Jul 1, 2022
86fef5b
ASoC: SOF: mediatek: Use generic implementation for .ipc_msg_data field
chunxu-li Jul 1, 2022
fe92905
ASoC: SOF: Introduce optional callback snd_sof_of_machine_select
chunxu-li Jun 30, 2022
74ecbb6
ASoC: SOF: mediatek: Add .of_machine_select field for mt8186
chunxu-li Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/sound/sof.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ struct snd_sof_pdata {
/* machine */
struct platform_device *pdev_mach;
const struct snd_soc_acpi_mach *machine;
const struct snd_sof_of_mach *of_machine;
Copy link
Member

@plbossart plbossart Jul 5, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@ranj063 ranj063 Jul 5, 2022

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@plbossart plbossart Jul 5, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Author

@chunxu-li chunxu-li Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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; 
		}
// ...
}

Copy link
Collaborator

@ranj063 ranj063 Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Author

@chunxu-li chunxu-li Jul 8, 2022

Choose a reason for hiding this comment

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

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

  1. we add a condition before machine_select?
    or
  2. 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; 
		}
// ...
}
  1. 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; 
		}
// ...
}

Copy link

@kuanhsuncheng kuanhsuncheng Jul 27, 2022

Choose a reason for hiding this comment

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

@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.


void *hw_pdata;

Expand All @@ -102,6 +103,7 @@ struct snd_sof_pdata {
struct sof_dev_desc {
/* list of machines using this configuration */
struct snd_soc_acpi_mach *machines;
struct snd_sof_of_mach *of_machines;

/* alternate list of machines using this configuration */
struct snd_soc_acpi_mach *alt_machines;
Expand Down
69 changes: 62 additions & 7 deletions sound/soc/sof/mediatek/mt8186/mt8186.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,13 +460,36 @@ static int mt8186_get_bar_index(struct snd_sof_dev *sdev, u32 type)
return type;
}

static int mt8186_ipc_msg_data(struct snd_sof_dev *sdev,
struct snd_pcm_substream *substream,
void *p, size_t sz)
static struct snd_soc_dai_driver mt8186_dai[] = {
{
sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
return 0;
}
.name = "SOF_DL1",
.playback = {
.channels_min = 1,
.channels_max = 2,
},
},
{
.name = "SOF_DL2",
.playback = {
.channels_min = 1,
.channels_max = 2,
},
},
{
.name = "SOF_UL1",
.capture = {
.channels_min = 1,
.channels_max = 2,
},
},
{
.name = "SOF_UL2",
.capture = {
.channels_min = 1,
.channels_max = 2,
},
},
};

/* mt8186 ops */
static struct snd_sof_dsp_ops sof_mt8186_ops = {
Expand All @@ -481,6 +504,10 @@ static struct snd_sof_dsp_ops sof_mt8186_ops = {
.block_read = sof_block_read,
.block_write = sof_block_write,

/* Mailbox IO */
.mailbox_read = sof_mailbox_read,
.mailbox_write = sof_mailbox_write,

/* Register IO */
.write = sof_io_write,
.read = sof_io_read,
Expand All @@ -491,18 +518,29 @@ static struct snd_sof_dsp_ops sof_mt8186_ops = {
.send_msg = mt8186_send_msg,
.get_mailbox_offset = mt8186_get_mailbox_offset,
.get_window_offset = mt8186_get_window_offset,
.ipc_msg_data = mt8186_ipc_msg_data,
.ipc_msg_data = sof_ipc_msg_data,
.set_stream_data_offset = sof_set_stream_data_offset,

/* misc */
.get_bar_index = mt8186_get_bar_index,

/* machine driver */
.of_machine_select = sof_of_machine_select,

/* stream callbacks */
.pcm_open = sof_stream_pcm_open,
.pcm_close = sof_stream_pcm_close,

/* firmware loading */
.load_firmware = snd_sof_load_firmware_memcpy,

/* Firmware ops */
.dsp_arch_ops = &sof_xtensa_arch_ops,

/* DAI drivers */
.drv = mt8186_dai,
.num_drv = ARRAY_SIZE(mt8186_dai),

/* PM */
.suspend = mt8186_dsp_suspend,
.resume = mt8186_dsp_resume,
Expand All @@ -515,7 +553,24 @@ static struct snd_sof_dsp_ops sof_mt8186_ops = {
SNDRV_PCM_INFO_NO_PERIOD_WAKEUP,
};

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

static const struct sof_dev_desc sof_of_mt8186_desc = {
.of_machines = sof_mt8186_machs,
.ipc_supported_mask = BIT(SOF_IPC),
.ipc_default = SOF_IPC,
.default_fw_path = {
Expand Down
9 changes: 9 additions & 0 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,15 @@ snd_sof_set_mach_params(struct snd_soc_acpi_mach *mach,
sof_ops(sdev)->set_mach_params(mach, sdev);
}

static inline struct snd_sof_of_mach *
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);
Copy link
Member

@plbossart plbossart Jul 5, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@dbaluta dbaluta Jul 18, 2022

Choose a reason for hiding this comment

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

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


return NULL;
}

/**
* snd_sof_dsp_register_poll_timeout - Periodically poll an address
* until a condition is met or a timeout occurs
Expand Down
8 changes: 7 additions & 1 deletion sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sound/pcm_params.h>
#include <sound/sof.h>
#include <trace/events/sof.h>
#include "sof-of-dev.h"
#include "sof-priv.h"
#include "sof-audio.h"
#include "sof-utils.h"
Expand Down Expand Up @@ -654,7 +655,12 @@ void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
struct snd_sof_pdata *plat_data = sdev->pdata;
const char *drv_name;

drv_name = plat_data->machine->drv_name;
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;
Copy link
Collaborator

@ujfalusi ujfalusi Jul 4, 2022

Choose a reason for hiding this comment

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

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

Copy link
Author

@chunxu-li chunxu-li Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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;


pd->name = "sof-audio-component";
pd->probe = sof_pcm_probe;
Expand Down
7 changes: 7 additions & 0 deletions sound/soc/sof/sof-audio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,7 @@ int sof_machine_check(struct snd_sof_dev *sdev)
struct snd_soc_acpi_mach *mach;

if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) {
const struct snd_sof_of_mach *of_mach;

/* find machine */
mach = snd_sof_machine_select(sdev);
Expand All @@ -808,6 +809,12 @@ int sof_machine_check(struct snd_sof_dev *sdev)
return 0;
}

of_mach = snd_sof_of_machine_select(sdev);
if (of_mach) {
sof_pdata->of_machine = of_mach;
return 0;
}

if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) {
dev_err(sdev->dev, "error: no matching ASoC machine driver found - aborting probe\n");
return -ENODEV;
Expand Down
23 changes: 23 additions & 0 deletions sound/soc/sof/sof-of-dev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,29 @@ const struct dev_pm_ops sof_of_pm = {
};
EXPORT_SYMBOL(sof_of_pm);

struct snd_sof_of_mach *sof_of_machine_select(struct snd_sof_dev *sdev)
{
struct snd_sof_pdata *sof_pdata = sdev->pdata;
const struct sof_dev_desc *desc = sof_pdata->desc;
struct snd_sof_of_mach *mach = desc->of_machines;

if (!mach)
return NULL;

for (; mach->board; mach++) {
if (of_machine_is_compatible(mach->board)) {
sof_pdata->tplg_filename = mach->sof_tplg_filename;
if (mach->fw_filename)
sof_pdata->fw_filename = mach->fw_filename;

return mach;
}
}

return NULL;
}
EXPORT_SYMBOL(sof_of_machine_select);

static void sof_of_probe_complete(struct device *dev)
{
/* allow runtime_pm */
Expand Down
8 changes: 8 additions & 0 deletions sound/soc/sof/sof-of-dev.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,16 @@
#ifndef __SOUND_SOC_SOF_OF_H
#define __SOUND_SOC_SOF_OF_H

struct snd_sof_of_mach {
const char *board;
const char *drv_name;
const char *fw_filename;
const char *sof_tplg_filename;
Copy link
Author

@chunxu-li chunxu-li Jul 4, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@dbaluta dbaluta Jul 18, 2022

Choose a reason for hiding this comment

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

@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.

Copy link
Author

@chunxu-li chunxu-li Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link
Collaborator

@dbaluta dbaluta Jul 18, 2022

Choose a reason for hiding this comment

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

I see. This works for me.

};

extern const struct dev_pm_ops sof_of_pm;

struct snd_sof_of_mach *sof_of_machine_select(struct snd_sof_dev *sdev);
int sof_of_probe(struct platform_device *pdev);
int sof_of_remove(struct platform_device *pdev);
void sof_of_shutdown(struct platform_device *pdev);
Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ struct snd_sof_dsp_ops {
void (*machine_unregister)(struct snd_sof_dev *sdev,
void *pdata); /* optional */
struct snd_soc_acpi_mach * (*machine_select)(struct snd_sof_dev *sdev); /* optional */
struct snd_sof_of_mach * (*of_machine_select)(struct snd_sof_dev *sdev); /* optional */
void (*set_mach_params)(struct snd_soc_acpi_mach *mach,
struct snd_sof_dev *sdev); /* optional */

Expand Down