| Message ID | 20210715141802.880911-2-daniel.baluta@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] ASoC: SOF: Parse fw/tplg filename from DT | expand |
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.
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.
> 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).
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.
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;