[PATCH 0/3] Read firmware, tplg and machine driver name from dts node

[PATCH 0/3] Read firmware, tplg and machine driver name from dts node

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

This patchseries adds support for reading the firmware name, topology
name and machine driver name from dsp dts node.

Intel side uses ACPI to read this info. We should use DT for i.MX.

First patches should go via sof-dev tree. Also last one could go via the
same tree after we get an Ack from Rob.

Daniel Baluta (3):
  ASoC: SOF: Parse fw/tplg filename from DT
  ASoC: SOF: Introduce machine driver name
  dt-bindings: dsp: fsl: Document newly introduced fsl,properties

 .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++
 include/sound/sof.h                           |  1 +
 sound/soc/sof/pcm.c                           |  5 ++-
 sound/soc/sof/sof-audio.c                     |  2 +-
 sound/soc/sof/sof-of-dev.c                    | 34 +++++++++++++++++++
 5 files changed, 60 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 0/3] Read firmware, tplg and machine driver name from dts node

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

This patchseries adds support for reading the firmware name, topology
name and machine driver name from dsp dts node.

Intel side uses ACPI to read this info. We should use DT for i.MX.

First patches should go via sof-dev tree. Also last one could go via the
same tree after we get an Ack from Rob.

Daniel Baluta (3):
  ASoC: SOF: Parse fw/tplg filename from DT
  ASoC: SOF: Introduce machine driver name
  dt-bindings: dsp: fsl: Document newly introduced fsl,properties

 .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++
 include/sound/sof.h                           |  1 +
 sound/soc/sof/pcm.c                           |  5 ++-
 sound/soc/sof/sof-audio.c                     |  2 +-
 sound/soc/sof/sof-of-dev.c                    | 34 +++++++++++++++++++
 5 files changed, 60 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Introduce two DT properties in dsp node:
	* fw-filename, optional property giving the firmware filename
	(if this is missing fw filename is read from board description)
	* tplg-filename, mandatory giving the topology filename.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/sof/sof-of-dev.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/sound/soc/sof/sof-of-dev.c b/sound/soc/sof/sof-of-dev.c
index d1a21edfa05d..770935191823 100644
--- a/sound/soc/sof/sof-of-dev.c
+++ b/sound/soc/sof/sof-of-dev.c
@@ -65,11 +65,28 @@ static void sof_of_probe_complete(struct device *dev)
 	pm_runtime_put_autosuspend(dev);
 }
 
+int sof_of_parse(struct platform_device *pdev)
+{
+	struct snd_sof_pdata *sof_pdata = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	/* firmware-name is optional in DT */
+	of_property_read_string(np, "firmware-name", &sof_pdata->fw_filename);
+
+	ret = of_property_read_string(np, "tplg-name", &sof_pdata->tplg_filename);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int sof_of_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct sof_dev_desc *desc;
 	struct snd_sof_pdata *sof_pdata;
+	int ret;
 
 	dev_info(&pdev->dev, "DT DSP detected");
 
@@ -77,6 +94,8 @@ static int sof_of_probe(struct platform_device *pdev)
 	if (!sof_pdata)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, sof_pdata);
+
 	desc = device_get_match_data(dev);
 	if (!desc)
 		return -ENODEV;
@@ -94,6 +113,16 @@ static int sof_of_probe(struct platform_device *pdev)
 	sof_pdata->fw_filename_prefix = sof_pdata->desc->default_fw_path;
 	sof_pdata->tplg_filename_prefix = sof_pdata->desc->default_tplg_path;
 
+	ret = sof_of_parse(pdev);
+	if (ret < 0) {
+		dev_err(dev, "Could not parse SOF OF DSP node\n");
+		return ret;
+	}
+
+	/* use default fw filename if none provided in DT */
+	if (!sof_pdata->fw_filename)
+		sof_pdata->fw_filename = desc->default_fw_filename;
+
 	/* set callback to be called on successful device probe to enable runtime_pm */
 	sof_pdata->sof_probe_complete = sof_of_probe_complete;
 
-- 
2.27.0

[PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Introduce two DT properties in dsp node:
	* fw-filename, optional property giving the firmware filename
	(if this is missing fw filename is read from board description)
	* tplg-filename, mandatory giving the topology filename.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/sof/sof-of-dev.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/sound/soc/sof/sof-of-dev.c b/sound/soc/sof/sof-of-dev.c
index d1a21edfa05d..770935191823 100644
--- a/sound/soc/sof/sof-of-dev.c
+++ b/sound/soc/sof/sof-of-dev.c
@@ -65,11 +65,28 @@ static void sof_of_probe_complete(struct device *dev)
 	pm_runtime_put_autosuspend(dev);
 }
 
+int sof_of_parse(struct platform_device *pdev)
+{
+	struct snd_sof_pdata *sof_pdata = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	/* firmware-name is optional in DT */
+	of_property_read_string(np, "firmware-name", &sof_pdata->fw_filename);
+
+	ret = of_property_read_string(np, "tplg-name", &sof_pdata->tplg_filename);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int sof_of_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct sof_dev_desc *desc;
 	struct snd_sof_pdata *sof_pdata;
+	int ret;
 
 	dev_info(&pdev->dev, "DT DSP detected");
 
@@ -77,6 +94,8 @@ static int sof_of_probe(struct platform_device *pdev)
 	if (!sof_pdata)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, sof_pdata);
+
 	desc = device_get_match_data(dev);
 	if (!desc)
 		return -ENODEV;
@@ -94,6 +113,16 @@ static int sof_of_probe(struct platform_device *pdev)
 	sof_pdata->fw_filename_prefix = sof_pdata->desc->default_fw_path;
 	sof_pdata->tplg_filename_prefix = sof_pdata->desc->default_tplg_path;
 
+	ret = sof_of_parse(pdev);
+	if (ret < 0) {
+		dev_err(dev, "Could not parse SOF OF DSP node\n");
+		return ret;
+	}
+
+	/* use default fw filename if none provided in DT */
+	if (!sof_pdata->fw_filename)
+		sof_pdata->fw_filename = desc->default_fw_filename;
+
 	/* set callback to be called on successful device probe to enable runtime_pm */
 	sof_pdata->sof_probe_complete = sof_of_probe_complete;
 
-- 
2.27.0

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Thu, Jul 15, 2021 at 05:18:00PM +0300, Daniel Baluta wrote:

> Introduce two DT properties in dsp node:
> 	* fw-filename, optional property giving the firmware filename
> 	(if this is missing fw filename is read from board description)
> 	* tplg-filename, mandatory giving the topology filename.

These sound entirely like operating system configuration which I'd
expect to be inferred from the machine identification.  What happens if
a system has multiple options for firmware files, or if the OS ships the
topology and firmware bundled up in a single image to avoid them getting
out of sync?  What's the benefit of putting them in the DT?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Thu, Jul 15, 2021 at 05:18:00PM +0300, Daniel Baluta wrote:

> Introduce two DT properties in dsp node:
> 	* fw-filename, optional property giving the firmware filename
> 	(if this is missing fw filename is read from board description)
> 	* tplg-filename, mandatory giving the topology filename.

These sound entirely like operating system configuration which I'd
expect to be inferred from the machine identification.  What happens if
a system has multiple options for firmware files, or if the OS ships the
topology and firmware bundled up in a single image to avoid them getting
out of sync?  What's the benefit of putting them in the DT?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Daniel Baluta]

On Thu, Jul 15, 2021 at 5:39 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 15, 2021 at 05:18:00PM +0300, Daniel Baluta wrote:
>
> > Introduce two DT properties in dsp node:
> >       * fw-filename, optional property giving the firmware filename
> >       (if this is missing fw filename is read from board description)
> >       * tplg-filename, mandatory giving the topology filename.
>
> These sound entirely like operating system configuration which I'd
> expect to be inferred from the machine identification.  What happens if
> a system has multiple options for firmware files, or if the OS ships the
> topology and firmware bundled up in a single image to avoid them getting
> out of sync?  What's the benefit of putting them in the DT?

We thought that if a system has multiple options for firmware files
we could use different Device Tree files. But indeed this doesn't scale.

It would be awkward to create a new dts just to change the firmware name.

Similarly for topology files. We might have:

- different audio scenarios (e.g different audio pipeline with
different components, e.g Post Processing Components, etc)
- different hardware attached to a board (e.g i.MX8 can have a
baseboard attached which brings in more codecs).

I think the best way to specify the audio firmware is via the board
description structure which is already
used to provide a default value for firmware file name.

Then for the topology used we could make that as a module parameter.

For us it is important to be able to use different topologies without
recompiling the kernel. So, far we just
used a simbolic link to the default topology file and change the
symbolic link to the desired topology and then reboot.

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Daniel Baluta]

On Thu, Jul 15, 2021 at 5:39 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 15, 2021 at 05:18:00PM +0300, Daniel Baluta wrote:
>
> > Introduce two DT properties in dsp node:
> >       * fw-filename, optional property giving the firmware filename
> >       (if this is missing fw filename is read from board description)
> >       * tplg-filename, mandatory giving the topology filename.
>
> These sound entirely like operating system configuration which I'd
> expect to be inferred from the machine identification.  What happens if
> a system has multiple options for firmware files, or if the OS ships the
> topology and firmware bundled up in a single image to avoid them getting
> out of sync?  What's the benefit of putting them in the DT?

We thought that if a system has multiple options for firmware files
we could use different Device Tree files. But indeed this doesn't scale.

It would be awkward to create a new dts just to change the firmware name.

Similarly for topology files. We might have:

- different audio scenarios (e.g different audio pipeline with
different components, e.g Post Processing Components, etc)
- different hardware attached to a board (e.g i.MX8 can have a
baseboard attached which brings in more codecs).

I think the best way to specify the audio firmware is via the board
description structure which is already
used to provide a default value for firmware file name.

Then for the topology used we could make that as a module parameter.

For us it is important to be able to use different topologies without
recompiling the kernel. So, far we just
used a simbolic link to the default topology file and change the
symbolic link to the desired topology and then reboot.

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Daniel Baluta]

Hi Pierre, Liam, Mark,

On Fri, Jul 16, 2021 at 5:31 PM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> On Thu, Jul 15, 2021 at 5:39 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Thu, Jul 15, 2021 at 05:18:00PM +0300, Daniel Baluta wrote:
> >
> > > Introduce two DT properties in dsp node:
> > >       * fw-filename, optional property giving the firmware filename
> > >       (if this is missing fw filename is read from board description)
> > >       * tplg-filename, mandatory giving the topology filename.
> >
> > These sound entirely like operating system configuration which I'd
> > expect to be inferred from the machine identification.  What happens if
> > a system has multiple options for firmware files, or if the OS ships the
> > topology and firmware bundled up in a single image to avoid them getting
> > out of sync?  What's the benefit of putting them in the DT?

Can you help me with this, specifically for selecting topology name.

I think I'm fine selecting a default value for SOF firmware name. It
looks like even
for Intel platforms there is no way of changing the firmware name.

But how about selecting topology name? We have lots of audio scenarios
that can run on the exact same hardware:
- e.g
   - Audio PCM playback + Post Processing
   - Audio Compress playback
   - Keyword detection


So, we need to use different topologies to select the scenario we want
to demonstrate.

Would it be acceptable to add tplg_name as a module parameter?

thanks,
Daniel.

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Daniel Baluta]

Hi Pierre, Liam, Mark,

On Fri, Jul 16, 2021 at 5:31 PM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> On Thu, Jul 15, 2021 at 5:39 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Thu, Jul 15, 2021 at 05:18:00PM +0300, Daniel Baluta wrote:
> >
> > > Introduce two DT properties in dsp node:
> > >       * fw-filename, optional property giving the firmware filename
> > >       (if this is missing fw filename is read from board description)
> > >       * tplg-filename, mandatory giving the topology filename.
> >
> > These sound entirely like operating system configuration which I'd
> > expect to be inferred from the machine identification.  What happens if
> > a system has multiple options for firmware files, or if the OS ships the
> > topology and firmware bundled up in a single image to avoid them getting
> > out of sync?  What's the benefit of putting them in the DT?

Can you help me with this, specifically for selecting topology name.

I think I'm fine selecting a default value for SOF firmware name. It
looks like even
for Intel platforms there is no way of changing the firmware name.

But how about selecting topology name? We have lots of audio scenarios
that can run on the exact same hardware:
- e.g
   - Audio PCM playback + Post Processing
   - Audio Compress playback
   - Keyword detection


So, we need to use different topologies to select the scenario we want
to demonstrate.

Would it be acceptable to add tplg_name as a module parameter?

thanks,
Daniel.

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Pierre-Louis Bossart]

>>>> Introduce two DT properties in dsp node:
>>>>       * fw-filename, optional property giving the firmware filename
>>>>       (if this is missing fw filename is read from board description)
>>>>       * tplg-filename, mandatory giving the topology filename.
>>>
>>> These sound entirely like operating system configuration which I'd
>>> expect to be inferred from the machine identification.  What happens if
>>> a system has multiple options for firmware files, or if the OS ships the
>>> topology and firmware bundled up in a single image to avoid them getting
>>> out of sync?  What's the benefit of putting them in the DT?
> 
> Can you help me with this, specifically for selecting topology name.
> 
> I think I'm fine selecting a default value for SOF firmware name. It
> looks like even
> for Intel platforms there is no way of changing the firmware name.
> 
> But how about selecting topology name? We have lots of audio scenarios
> that can run on the exact same hardware:
> - e.g
>    - Audio PCM playback + Post Processing
>    - Audio Compress playback
>    - Keyword detection
> 
> 
> So, we need to use different topologies to select the scenario we want
> to demonstrate.
> 
> Would it be acceptable to add tplg_name as a module parameter?

we already have a "tplg_path" module parameter which was intended to differentiate between product skews/versions using the same hardware and firmware version. A typical example would be an OEM using 'public' firmware + topology for basic audio support, distributed through sof-bin and packaged by distros, and 3rd-party/closed sources firmware modules in more advanced packages distributed separately by the OEM. In the latter case you do want the same path for firmware and topology, otherwise you'd have a risk of using a topology making references to a library not bundled in the firmware.

There was an initial ask from Curtis to have the ability to override the firmware/topology names, but they've been able to work with the path parameters - set with udev rules for specific models.

If you wanted to demonstrate 'scenarios', you could use the same approach?

Two other points to reply to Mark:

- we currently don't support 'shipping the topology and firmware bundled up in a single image to avoid them getting out of sync'. No idea how that might work.

- if the machine driver is specified in DeviceTree, then the topology used is *required* to be aligned with the machine driver. The rules are that a topology may not make references to a BE dailink exposed in the machine driver, but conversely if the topology makes a reference to a BE dailink that is not exposed in the machine driver the topology parsing will fail. It's one of the current weaknesses of topology-based solutions, we have non-configurable hardware-related things that are described in topology but should really be described in platform firmware, be it ACPI or DT, and provided to the topology.

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Pierre-Louis Bossart]

>>>> Introduce two DT properties in dsp node:
>>>>       * fw-filename, optional property giving the firmware filename
>>>>       (if this is missing fw filename is read from board description)
>>>>       * tplg-filename, mandatory giving the topology filename.
>>>
>>> These sound entirely like operating system configuration which I'd
>>> expect to be inferred from the machine identification.  What happens if
>>> a system has multiple options for firmware files, or if the OS ships the
>>> topology and firmware bundled up in a single image to avoid them getting
>>> out of sync?  What's the benefit of putting them in the DT?
> 
> Can you help me with this, specifically for selecting topology name.
> 
> I think I'm fine selecting a default value for SOF firmware name. It
> looks like even
> for Intel platforms there is no way of changing the firmware name.
> 
> But how about selecting topology name? We have lots of audio scenarios
> that can run on the exact same hardware:
> - e.g
>    - Audio PCM playback + Post Processing
>    - Audio Compress playback
>    - Keyword detection
> 
> 
> So, we need to use different topologies to select the scenario we want
> to demonstrate.
> 
> Would it be acceptable to add tplg_name as a module parameter?

we already have a "tplg_path" module parameter which was intended to differentiate between product skews/versions using the same hardware and firmware version. A typical example would be an OEM using 'public' firmware + topology for basic audio support, distributed through sof-bin and packaged by distros, and 3rd-party/closed sources firmware modules in more advanced packages distributed separately by the OEM. In the latter case you do want the same path for firmware and topology, otherwise you'd have a risk of using a topology making references to a library not bundled in the firmware.

There was an initial ask from Curtis to have the ability to override the firmware/topology names, but they've been able to work with the path parameters - set with udev rules for specific models.

If you wanted to demonstrate 'scenarios', you could use the same approach?

Two other points to reply to Mark:

- we currently don't support 'shipping the topology and firmware bundled up in a single image to avoid them getting out of sync'. No idea how that might work.

- if the machine driver is specified in DeviceTree, then the topology used is *required* to be aligned with the machine driver. The rules are that a topology may not make references to a BE dailink exposed in the machine driver, but conversely if the topology makes a reference to a BE dailink that is not exposed in the machine driver the topology parsing will fail. It's one of the current weaknesses of topology-based solutions, we have non-configurable hardware-related things that are described in topology but should really be described in platform firmware, be it ACPI or DT, and provided to the topology.

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Tue, Jul 20, 2021 at 10:28:57AM -0500, Pierre-Louis Bossart wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> - we currently don't support 'shipping the topology and firmware
> bundled up in a single image to avoid them getting out of sync'. No
> idea how that might work.

Seems like it'd be trivial to arrange in the kernel, or with userspace
firmware loading the loader could do the unpacking.

> - if the machine driver is specified in DeviceTree, then the topology
> used is *required* to be aligned with the machine driver. The rules
> are that a topology may not make references to a BE dailink exposed in
> the machine driver, but conversely if the topology makes a reference
> to a BE dailink that is not exposed in the machine driver the topology
> parsing will fail. It's one of the current weaknesses of
> topology-based solutions, we have non-configurable hardware-related
> things that are described in topology but should really be described
> in platform firmware, be it ACPI or DT, and provided to the topology.

That seems like an orthogonal issue here?  The requirement for a
firmware that's joined up with the hardware (and system description)
that it's being used with exists regardless of how we rename things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Tue, Jul 20, 2021 at 10:28:57AM -0500, Pierre-Louis Bossart wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> - we currently don't support 'shipping the topology and firmware
> bundled up in a single image to avoid them getting out of sync'. No
> idea how that might work.

Seems like it'd be trivial to arrange in the kernel, or with userspace
firmware loading the loader could do the unpacking.

> - if the machine driver is specified in DeviceTree, then the topology
> used is *required* to be aligned with the machine driver. The rules
> are that a topology may not make references to a BE dailink exposed in
> the machine driver, but conversely if the topology makes a reference
> to a BE dailink that is not exposed in the machine driver the topology
> parsing will fail. It's one of the current weaknesses of
> topology-based solutions, we have non-configurable hardware-related
> things that are described in topology but should really be described
> in platform firmware, be it ACPI or DT, and provided to the topology.

That seems like an orthogonal issue here?  The requirement for a
firmware that's joined up with the hardware (and system description)
that it's being used with exists regardless of how we rename things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Pierre-Louis Bossart]

> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Oops.

>> - we currently don't support 'shipping the topology and firmware
>> bundled up in a single image to avoid them getting out of sync'. No
>> idea how that might work.
> 
> Seems like it'd be trivial to arrange in the kernel, or with userspace
> firmware loading the loader could do the unpacking.

I think we can bundle the firmware inside of the kernel image itself,
but we've never tried so it doesn't work by default.
I don't know what userspace loading means, we rely on request_firmware
and don't assume any specific support from userspace.

>> - if the machine driver is specified in DeviceTree, then the topology
>> used is *required* to be aligned with the machine driver. The rules
>> are that a topology may not make references to a BE dailink exposed in
>> the machine driver, but conversely if the topology makes a reference
>> to a BE dailink that is not exposed in the machine driver the topology
>> parsing will fail. It's one of the current weaknesses of
>> topology-based solutions, we have non-configurable hardware-related
>> things that are described in topology but should really be described
>> in platform firmware, be it ACPI or DT, and provided to the topology.
> 
> That seems like an orthogonal issue here?  The requirement for a
> firmware that's joined up with the hardware (and system description)
> that it's being used with exists regardless of how we rename things.

It's not completely orthogonal. The topology currently defines e.g. the
I2S interface index, Mclk, bclk, fsync, etc, and my point is that these
bits of information are completely related to the hardware and should
probably come from platform firmware/ACPI.

The topology framework currently provides too much freedom to
developers, it's fine to add new pipelines, PCM devices and new
processing, but when it comes to the hardware interfaces the topology is
completely constrained. I've been arguing for a while now that the
dailink descriptions and configurations should be treated as an input to
the topology, not something that the topology can configure. I don't
know how many issues we had to deal with because the topology settings
were not supported by the hardware, or mismatches between topology and
machine drivers (missing dailinks, bad dailink index, etc).

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Pierre-Louis Bossart]

> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Oops.

>> - we currently don't support 'shipping the topology and firmware
>> bundled up in a single image to avoid them getting out of sync'. No
>> idea how that might work.
> 
> Seems like it'd be trivial to arrange in the kernel, or with userspace
> firmware loading the loader could do the unpacking.

I think we can bundle the firmware inside of the kernel image itself,
but we've never tried so it doesn't work by default.
I don't know what userspace loading means, we rely on request_firmware
and don't assume any specific support from userspace.

>> - if the machine driver is specified in DeviceTree, then the topology
>> used is *required* to be aligned with the machine driver. The rules
>> are that a topology may not make references to a BE dailink exposed in
>> the machine driver, but conversely if the topology makes a reference
>> to a BE dailink that is not exposed in the machine driver the topology
>> parsing will fail. It's one of the current weaknesses of
>> topology-based solutions, we have non-configurable hardware-related
>> things that are described in topology but should really be described
>> in platform firmware, be it ACPI or DT, and provided to the topology.
> 
> That seems like an orthogonal issue here?  The requirement for a
> firmware that's joined up with the hardware (and system description)
> that it's being used with exists regardless of how we rename things.

It's not completely orthogonal. The topology currently defines e.g. the
I2S interface index, Mclk, bclk, fsync, etc, and my point is that these
bits of information are completely related to the hardware and should
probably come from platform firmware/ACPI.

The topology framework currently provides too much freedom to
developers, it's fine to add new pipelines, PCM devices and new
processing, but when it comes to the hardware interfaces the topology is
completely constrained. I've been arguing for a while now that the
dailink descriptions and configurations should be treated as an input to
the topology, not something that the topology can configure. I don't
know how many issues we had to deal with because the topology settings
were not supported by the hardware, or mismatches between topology and
machine drivers (missing dailinks, bad dailink index, etc).

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2843 bytes --]

On Wed, Jul 21, 2021 at 08:28:17AM -0500, Pierre-Louis Bossart wrote:

> > Seems like it'd be trivial to arrange in the kernel, or with userspace
> > firmware loading the loader could do the unpacking.

> I think we can bundle the firmware inside of the kernel image itself,
> but we've never tried so it doesn't work by default.
> I don't know what userspace loading means, we rely on request_firmware
> and don't assume any specific support from userspace.

If you have a userspace handler that implements loading firmware into
the kernel (rather than having the kernel just try with a given path
prefix) then that program can do anything it likes to get the firmware,
including unpacking it out of another image.

> > That seems like an orthogonal issue here?  The requirement for a
> > firmware that's joined up with the hardware (and system description)
> > that it's being used with exists regardless of how we rename things.

> It's not completely orthogonal. The topology currently defines e.g. the
> I2S interface index, Mclk, bclk, fsync, etc, and my point is that these
> bits of information are completely related to the hardware and should
> probably come from platform firmware/ACPI.

If only ACPI based platforms offered a standard way to do this like DT
does and didn't rely on all these platform specific hacks!  In any case
my point is more that use case dependent selection of the firmware is a
separate issue to having firmware that matches a specific board and
there seemed to be some conflation of the two.  For having a completely
board specific firmware we already have system level identification in
both DT and ACPI which can be used.

> The topology framework currently provides too much freedom to
> developers, it's fine to add new pipelines, PCM devices and new
> processing, but when it comes to the hardware interfaces the topology is
> completely constrained. I've been arguing for a while now that the
> dailink descriptions and configurations should be treated as an input to
> the topology, not something that the topology can configure. I don't
> know how many issues we had to deal with because the topology settings
> were not supported by the hardware, or mismatches between topology and
> machine drivers (missing dailinks, bad dailink index, etc).

I think it'd definitely help to at least have some strong diagnostics
for detecting mismatches between the topology and the hardware and
machine driver it's being applied to, including what configurations the
machine driver is willing to have on the links (which could be just a
single configuration if that's what makes sense for the platform).  I
can see that the topology might want to select different configurations
for the various hardware links depending on how it wants to use them in
a given application, especially in more embedded contexts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2843 bytes --]

On Wed, Jul 21, 2021 at 08:28:17AM -0500, Pierre-Louis Bossart wrote:

> > Seems like it'd be trivial to arrange in the kernel, or with userspace
> > firmware loading the loader could do the unpacking.

> I think we can bundle the firmware inside of the kernel image itself,
> but we've never tried so it doesn't work by default.
> I don't know what userspace loading means, we rely on request_firmware
> and don't assume any specific support from userspace.

If you have a userspace handler that implements loading firmware into
the kernel (rather than having the kernel just try with a given path
prefix) then that program can do anything it likes to get the firmware,
including unpacking it out of another image.

> > That seems like an orthogonal issue here?  The requirement for a
> > firmware that's joined up with the hardware (and system description)
> > that it's being used with exists regardless of how we rename things.

> It's not completely orthogonal. The topology currently defines e.g. the
> I2S interface index, Mclk, bclk, fsync, etc, and my point is that these
> bits of information are completely related to the hardware and should
> probably come from platform firmware/ACPI.

If only ACPI based platforms offered a standard way to do this like DT
does and didn't rely on all these platform specific hacks!  In any case
my point is more that use case dependent selection of the firmware is a
separate issue to having firmware that matches a specific board and
there seemed to be some conflation of the two.  For having a completely
board specific firmware we already have system level identification in
both DT and ACPI which can be used.

> The topology framework currently provides too much freedom to
> developers, it's fine to add new pipelines, PCM devices and new
> processing, but when it comes to the hardware interfaces the topology is
> completely constrained. I've been arguing for a while now that the
> dailink descriptions and configurations should be treated as an input to
> the topology, not something that the topology can configure. I don't
> know how many issues we had to deal with because the topology settings
> were not supported by the hardware, or mismatches between topology and
> machine drivers (missing dailinks, bad dailink index, etc).

I think it'd definitely help to at least have some strong diagnostics
for detecting mismatches between the topology and the hardware and
machine driver it's being applied to, including what configurations the
machine driver is willing to have on the links (which could be just a
single configuration if that's what makes sense for the platform).  I
can see that the topology might want to select different configurations
for the various hardware links depending on how it wants to use them in
a given application, especially in more embedded contexts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: SOF: Parse fw/tplg filename from DT

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on sound/for-next robh/for-next v5.14-rc1 next-20210715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Baluta/Read-firmware-tplg-and-machine-driver-name-from-dts-node/20210715-221927
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: m68k-buildonly-randconfig-r006-20210715 (attached as .config)
compiler: m68k-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f334d71a2e2f944458fcdfa43566fb298ea5411e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Baluta/Read-firmware-tplg-and-machine-driver-name-from-dts-node/20210715-221927
        git checkout f334d71a2e2f944458fcdfa43566fb298ea5411e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> sound/soc/sof/sof-of-dev.c:68:5: warning: no previous prototype for 'sof_of_parse' [-Wmissing-prototypes]
      68 | int sof_of_parse(struct platform_device *pdev)
         |     ^~~~~~~~~~~~


vim +/sof_of_parse +68 sound/soc/sof/sof-of-dev.c

    67	
  > 68	int sof_of_parse(struct platform_device *pdev)
    69	{
    70		struct snd_sof_pdata *sof_pdata = platform_get_drvdata(pdev);
    71		struct device_node *np = pdev->dev.of_node;
    72		int ret;
    73	
    74		/* firmware-name is optional in DT */
    75		of_property_read_string(np, "firmware-name", &sof_pdata->fw_filename);
    76	
    77		ret = of_property_read_string(np, "tplg-name", &sof_pdata->tplg_filename);
    78		if (ret < 0)
    79			return ret;
    80	
    81		return 0;
    82	}
    83	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28695 bytes --]

[PATCH 2/3] ASoC: SOF: Introduce machine driver name

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

ACPI creates tables with information about the machine driver.
With DT there is no need for such tables because we can directly
get all the information needed from DT file.

This patch introduces machine driver property inside dsp node.

Notice that sof_pdata->machine_drv_name is the OF equivalent of
sof_pdata->machine (snd_soc_acpi_mach). We don't need more
information than machine_drv_name for OF platforms (for now).

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 include/sound/sof.h        | 1 +
 sound/soc/sof/pcm.c        | 5 ++++-
 sound/soc/sof/sof-audio.c  | 2 +-
 sound/soc/sof/sof-of-dev.c | 5 +++++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/sound/sof.h b/include/sound/sof.h
index 23b374311d16..191607945432 100644
--- a/include/sound/sof.h
+++ b/include/sound/sof.h
@@ -51,6 +51,7 @@ struct snd_sof_pdata {
 	/* machine */
 	struct platform_device *pdev_mach;
 	const struct snd_soc_acpi_mach *machine;
+	const char *machine_drv_name; /* machine driver name, set only for OF case */
 
 	void *hw_pdata;
 };
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 27244dc043ce..47def9240e7c 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -885,7 +885,10 @@ 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
+		drv_name = plat_data->machine_drv_name;
 
 	pd->name = "sof-audio-component";
 	pd->probe = sof_pcm_probe;
diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c
index 10aa0d8ea186..aebfa5150fa1 100644
--- a/sound/soc/sof/sof-audio.c
+++ b/sound/soc/sof/sof-audio.c
@@ -884,7 +884,7 @@ int sof_machine_check(struct snd_sof_dev *sdev)
 
 		/* find machine */
 		snd_sof_machine_select(sdev);
-		if (sof_pdata->machine) {
+		if (sof_pdata->machine || sof_pdata->machine_drv_name) {
 			snd_sof_set_mach_params(sof_pdata->machine, sdev);
 			return 0;
 		}
diff --git a/sound/soc/sof/sof-of-dev.c b/sound/soc/sof/sof-of-dev.c
index 770935191823..18bfe5b78966 100644
--- a/sound/soc/sof/sof-of-dev.c
+++ b/sound/soc/sof/sof-of-dev.c
@@ -78,6 +78,11 @@ int sof_of_parse(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	ret = of_property_read_string(np, "machine-drv-name",
+				      &sof_pdata->machine_drv_name);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 }
 
-- 
2.27.0

[PATCH 2/3] ASoC: SOF: Introduce machine driver name

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

ACPI creates tables with information about the machine driver.
With DT there is no need for such tables because we can directly
get all the information needed from DT file.

This patch introduces machine driver property inside dsp node.

Notice that sof_pdata->machine_drv_name is the OF equivalent of
sof_pdata->machine (snd_soc_acpi_mach). We don't need more
information than machine_drv_name for OF platforms (for now).

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 include/sound/sof.h        | 1 +
 sound/soc/sof/pcm.c        | 5 ++++-
 sound/soc/sof/sof-audio.c  | 2 +-
 sound/soc/sof/sof-of-dev.c | 5 +++++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/sound/sof.h b/include/sound/sof.h
index 23b374311d16..191607945432 100644
--- a/include/sound/sof.h
+++ b/include/sound/sof.h
@@ -51,6 +51,7 @@ struct snd_sof_pdata {
 	/* machine */
 	struct platform_device *pdev_mach;
 	const struct snd_soc_acpi_mach *machine;
+	const char *machine_drv_name; /* machine driver name, set only for OF case */
 
 	void *hw_pdata;
 };
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 27244dc043ce..47def9240e7c 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -885,7 +885,10 @@ 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
+		drv_name = plat_data->machine_drv_name;
 
 	pd->name = "sof-audio-component";
 	pd->probe = sof_pcm_probe;
diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c
index 10aa0d8ea186..aebfa5150fa1 100644
--- a/sound/soc/sof/sof-audio.c
+++ b/sound/soc/sof/sof-audio.c
@@ -884,7 +884,7 @@ int sof_machine_check(struct snd_sof_dev *sdev)
 
 		/* find machine */
 		snd_sof_machine_select(sdev);
-		if (sof_pdata->machine) {
+		if (sof_pdata->machine || sof_pdata->machine_drv_name) {
 			snd_sof_set_mach_params(sof_pdata->machine, sdev);
 			return 0;
 		}
diff --git a/sound/soc/sof/sof-of-dev.c b/sound/soc/sof/sof-of-dev.c
index 770935191823..18bfe5b78966 100644
--- a/sound/soc/sof/sof-of-dev.c
+++ b/sound/soc/sof/sof-of-dev.c
@@ -78,6 +78,11 @@ int sof_of_parse(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	ret = of_property_read_string(np, "machine-drv-name",
+				      &sof_pdata->machine_drv_name);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 }
 
-- 
2.27.0

[PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl,properties

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Document firmware-name, tplg-name and machine-drv-name properties.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
index 7afc9f2be13a..8095aa178e7c 100644
--- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
+++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
@@ -60,6 +60,22 @@ properties:
       used by DSP (see bindings/reserved-memory/reserved-memory.txt)
     maxItems: 1
 
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      If present, name of the file within the firmware search path containing
+      the DSP firmware loaded by SOF at DSP boot time.
+
+  tplg-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Should contain the audio topology file name loaded by SOF driver.
+
+  machine-drv-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Contains the ASoC machine driver name used by SOF to handle DSP audio scenario.
+
 required:
   - compatible
   - reg
@@ -69,6 +85,8 @@ required:
   - mboxes
   - mbox-names
   - memory-region
+  - tplg-name
+  - machine-drv-name
 
 additionalProperties: false
 
@@ -90,4 +108,6 @@ examples:
         mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
         mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
         memory-region = <&dsp_reserved>;
+        tplg-name = "sof-imx8-wm8960.tplg";
+        machine-drv-name = "asoc-simple-card";
     };
-- 
2.27.0

[PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl, properties

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Document firmware-name, tplg-name and machine-drv-name properties.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
index 7afc9f2be13a..8095aa178e7c 100644
--- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
+++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
@@ -60,6 +60,22 @@ properties:
       used by DSP (see bindings/reserved-memory/reserved-memory.txt)
     maxItems: 1
 
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      If present, name of the file within the firmware search path containing
+      the DSP firmware loaded by SOF at DSP boot time.
+
+  tplg-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Should contain the audio topology file name loaded by SOF driver.
+
+  machine-drv-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Contains the ASoC machine driver name used by SOF to handle DSP audio scenario.
+
 required:
   - compatible
   - reg
@@ -69,6 +85,8 @@ required:
   - mboxes
   - mbox-names
   - memory-region
+  - tplg-name
+  - machine-drv-name
 
 additionalProperties: false
 
@@ -90,4 +108,6 @@ examples:
         mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
         mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
         memory-region = <&dsp_reserved>;
+        tplg-name = "sof-imx8-wm8960.tplg";
+        machine-drv-name = "asoc-simple-card";
     };
-- 
2.27.0

Re: [PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl,properties

[Rob Herring]

On Thu, Jul 15, 2021 at 8:18 AM Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
>
> From: Daniel Baluta <daniel.baluta@nxp.com>
>
> Document firmware-name, tplg-name and machine-drv-name properties.

That's obvious from the diff.

Why do you need these?

>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> index 7afc9f2be13a..8095aa178e7c 100644
> --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -60,6 +60,22 @@ properties:
>        used by DSP (see bindings/reserved-memory/reserved-memory.txt)
>      maxItems: 1
>
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      If present, name of the file within the firmware search path containing
> +      the DSP firmware loaded by SOF at DSP boot time.
> +
> +  tplg-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      Should contain the audio topology file name loaded by SOF driver.

Is this some format the DSP requires? Why do we need a separate file?
This is defined by the board or user config?

> +
> +  machine-drv-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      Contains the ASoC machine driver name used by SOF to handle DSP audio scenario.

Umm, no.

> +
>  required:
>    - compatible
>    - reg
> @@ -69,6 +85,8 @@ required:
>    - mboxes
>    - mbox-names
>    - memory-region
> +  - tplg-name
> +  - machine-drv-name
>
>  additionalProperties: false
>
> @@ -90,4 +108,6 @@ examples:
>          mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
>          mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
>          memory-region = <&dsp_reserved>;
> +        tplg-name = "sof-imx8-wm8960.tplg";
> +        machine-drv-name = "asoc-simple-card";
>      };
> --
> 2.27.0
>

Re: [PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl, properties

[Rob Herring]

On Thu, Jul 15, 2021 at 8:18 AM Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
>
> From: Daniel Baluta <daniel.baluta@nxp.com>
>
> Document firmware-name, tplg-name and machine-drv-name properties.

That's obvious from the diff.

Why do you need these?

>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> index 7afc9f2be13a..8095aa178e7c 100644
> --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -60,6 +60,22 @@ properties:
>        used by DSP (see bindings/reserved-memory/reserved-memory.txt)
>      maxItems: 1
>
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      If present, name of the file within the firmware search path containing
> +      the DSP firmware loaded by SOF at DSP boot time.
> +
> +  tplg-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      Should contain the audio topology file name loaded by SOF driver.

Is this some format the DSP requires? Why do we need a separate file?
This is defined by the board or user config?

> +
> +  machine-drv-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      Contains the ASoC machine driver name used by SOF to handle DSP audio scenario.

Umm, no.

> +
>  required:
>    - compatible
>    - reg
> @@ -69,6 +85,8 @@ required:
>    - mboxes
>    - mbox-names
>    - memory-region
> +  - tplg-name
> +  - machine-drv-name
>
>  additionalProperties: false
>
> @@ -90,4 +108,6 @@ examples:
>          mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
>          mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
>          memory-region = <&dsp_reserved>;
> +        tplg-name = "sof-imx8-wm8960.tplg";
> +        machine-drv-name = "asoc-simple-card";
>      };
> --
> 2.27.0
>

Re: [PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl,properties

[Daniel Baluta]

On Thu, Jul 15, 2021 at 5:59 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Jul 15, 2021 at 8:18 AM Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
> >
> > From: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Document firmware-name, tplg-name and machine-drv-name properties.
>
> That's obvious from the diff.
>
> Why do you need these?
>
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > index 7afc9f2be13a..8095aa178e7c 100644
> > --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > @@ -60,6 +60,22 @@ properties:
> >        used by DSP (see bindings/reserved-memory/reserved-memory.txt)
> >      maxItems: 1
> >
> > +  firmware-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      If present, name of the file within the firmware search path containing
> > +      the DSP firmware loaded by SOF at DSP boot time.
> > +
> > +  tplg-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      Should contain the audio topology file name loaded by SOF driver.
>
> Is this some format the DSP requires? Why do we need a separate file?
> This is defined by the board or user config?

This is not specific to DSP but to ALSA (See ALSA topology [1]).

We need the .tplg file in order to describe the support Audio scenario
by our board.

This could be defined both by:

board:
- e.g our CPU board can have a baseboard attached (so the audio
scenario changes).
user config:
- e.g user wants to enable post processing or any audio component.

I couldnt find a good way to specify this except via DTS. Intel folks
derive this information from ACPI tables.

[1] https://www.alsa-project.org/wiki/ALSA_topology

Re: [PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl, properties

[Daniel Baluta]

On Thu, Jul 15, 2021 at 5:59 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Jul 15, 2021 at 8:18 AM Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
> >
> > From: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Document firmware-name, tplg-name and machine-drv-name properties.
>
> That's obvious from the diff.
>
> Why do you need these?
>
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > index 7afc9f2be13a..8095aa178e7c 100644
> > --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > @@ -60,6 +60,22 @@ properties:
> >        used by DSP (see bindings/reserved-memory/reserved-memory.txt)
> >      maxItems: 1
> >
> > +  firmware-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      If present, name of the file within the firmware search path containing
> > +      the DSP firmware loaded by SOF at DSP boot time.
> > +
> > +  tplg-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      Should contain the audio topology file name loaded by SOF driver.
>
> Is this some format the DSP requires? Why do we need a separate file?
> This is defined by the board or user config?

This is not specific to DSP but to ALSA (See ALSA topology [1]).

We need the .tplg file in order to describe the support Audio scenario
by our board.

This could be defined both by:

board:
- e.g our CPU board can have a baseboard attached (so the audio
scenario changes).
user config:
- e.g user wants to enable post processing or any audio component.

I couldnt find a good way to specify this except via DTS. Intel folks
derive this information from ACPI tables.

[1] https://www.alsa-project.org/wiki/ALSA_topology

Re: [PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl,properties

[Rob Herring]

On Thu, 15 Jul 2021 17:18:02 +0300, Daniel Baluta wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Document firmware-name, tplg-name and machine-drv-name properties.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mailbox/arm,mhuv2.example.dt.yaml: dsp@596e8000: 'tplg-name' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mailbox/arm,mhuv2.example.dt.yaml: dsp@596e8000: 'machine-drv-name' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1505740

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 3/3] dt-bindings: dsp: fsl: Document newly introduced fsl, properties

[Rob Herring]

On Thu, 15 Jul 2021 17:18:02 +0300, Daniel Baluta wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Document firmware-name, tplg-name and machine-drv-name properties.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mailbox/arm,mhuv2.example.dt.yaml: dsp@596e8000: 'tplg-name' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mailbox/arm,mhuv2.example.dt.yaml: dsp@596e8000: 'machine-drv-name' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1505740

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.