December 2018 - Sound-open-firmware - mailman.alsa-project.org

[Sound-open-firmware] [PATCH v3 00/14] Sound Open Firmware (SOF) core
by Pierre-Louis Bossart 30 Jan '19

30 Jan '19

Sound Open Firmware (SOF) is a host and DSP architecture agnostic audio DSP firmware. SOF is not tied to any specific host architecture or any specific physical IO communication type (it will work with on SoC DSPs, or DSP connected via SPI/I2C). SOF is also not coupled to any particular DSP architecture and has abstraction similar to Linux to allow porting to other DSP architectures. https://www.sofproject.org/ This patch series introduces the SOF core and utilities. Support for Intel devices is provided as a follow-up series. The SOF core manages all the core DSP services and ALSA/ASoC IO. The core is responsible for loading firmware, parsing topology, exposing PCMs and kcontrols, providing debug and trace mechanisms and performing IPC between host and DSP. The SOF core also has logic to allow reuse of existing machine drivers for other platforms/machines without any code modification. i.e. DAI links can be modified at runtime to bind with SOF and SOF topologies instead of existing hard coded DAI links and topology. Main changes since v2 (end of August): There are honestly too many changes to be listed... The biggest changes are the introduction of an ABI check and runtime_pm, as well as countless improvements coming from product validation. We will provide a revision history starting with the next update. Precisions: Since I've been maintaining the SOF kernel parts for most of 2018, Liam and I mutually agreed that it would be more efficient for me to send the patches upstream to avoid any disconnects. The code in this patchset is directly squashed from the SOF development branch [1], which tracks Mark Brown's for-next branch on a weekly basis and the configurations used for testing are based on the defconfigs at [2] Full disclosure on known limitation and issues (full list at [3]) a) the interaction with ASoC is not always perfect, there are a couple of points where a better solution is desired (use of private data, support for link DMA, etc). The known points are explicitly documented in the code and will be updated. To be clearer, it's our belief that SOF does not cripple ASoC in any way, and that merging this SOF core does not harm others. It's just complicated to get things right, especially on a couple of cases where the topology and DPCM frameworks generate complex flows that very few people understand. b) The get/put methods for controls generate an IPC and possibly a wake-up. This is not optimal and is being fixed. c) The debugfs parts will be updated for runtime_pm parts d) The topology free, firmware release and Hdaudio exit sequences will be updated. It is our intention to fully support module load/unload and free all resources. e) runtime_pm is being hardened and the use of SMART_SUSPEND was suggested. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre [1] https://github.com/thesofproject/linux [2] https://github.com/thesofproject/kconfig [3] https://github.com/thesofproject/linux/issues Liam Girdwood (12): ASoC: SOF: Add Sound Open Firmware driver core ASoC: SOF: Add Sound Open Firmware KControl support ASoC: SOF: Add driver debug support. ASoC: SOF: Add support for IPC IO between DSP and Host ASoC: SOF: Add PCM operations support ASoC: SOF: Add support for loading topologies ASoC: SOF: Add DSP firmware logger support ASoC: SOF: Add DSP HW abstraction operations ASoC: SOF: Add firmware loader support ASoC: SOF: Add userspace ABI support ASoC: SOF: Add PM support ASoC: SOF: Add Nocodec machine driver support Pierre-Louis Bossart (2): ASoC: SOF: Add xtensa support ASoC: SOF: Add utils include/sound/soc.h | 3 + include/sound/sof.h | 91 + include/sound/sof/control.h | 125 ++ include/sound/sof/dai-intel.h | 175 ++ include/sound/sof/dai.h | 75 + include/sound/sof/header.h | 158 ++ include/sound/sof/info.h | 118 ++ include/sound/sof/pm.h | 48 + include/sound/sof/stream.h | 149 ++ include/sound/sof/topology.h | 256 +++ include/sound/sof/trace.h | 66 + include/sound/sof/xtensa.h | 44 + include/uapi/sound/sof/abi.h | 63 + include/uapi/sound/sof/eq.h | 164 ++ include/uapi/sound/sof/fw.h | 67 + include/uapi/sound/sof/header.h | 27 + include/uapi/sound/sof/manifest.h | 188 ++ include/uapi/sound/sof/tokens.h | 95 + include/uapi/sound/sof/tone.h | 21 + include/uapi/sound/sof/trace.h | 93 + sound/soc/sof/control.c | 421 +++++ sound/soc/sof/core.c | 398 +++++ sound/soc/sof/debug.c | 176 ++ sound/soc/sof/ipc.c | 813 +++++++++ sound/soc/sof/loader.c | 322 ++++ sound/soc/sof/nocodec.c | 81 + sound/soc/sof/ops.c | 200 +++ sound/soc/sof/ops.h | 379 ++++ sound/soc/sof/pcm.c | 755 ++++++++ sound/soc/sof/pm.c | 403 +++++ sound/soc/sof/sof-priv.h | 574 ++++++ sound/soc/sof/topology.c | 2775 +++++++++++++++++++++++++++++ sound/soc/sof/trace.c | 295 +++ sound/soc/sof/utils.c | 173 ++ sound/soc/sof/xtensa/Kconfig | 3 + sound/soc/sof/xtensa/Makefile | 5 + sound/soc/sof/xtensa/core.c | 152 ++ 37 files changed, 9951 insertions(+) create mode 100644 include/sound/sof.h create mode 100644 include/sound/sof/control.h create mode 100644 include/sound/sof/dai-intel.h create mode 100644 include/sound/sof/dai.h create mode 100644 include/sound/sof/header.h create mode 100644 include/sound/sof/info.h create mode 100644 include/sound/sof/pm.h create mode 100644 include/sound/sof/stream.h create mode 100644 include/sound/sof/topology.h create mode 100644 include/sound/sof/trace.h create mode 100644 include/sound/sof/xtensa.h create mode 100644 include/uapi/sound/sof/abi.h create mode 100644 include/uapi/sound/sof/eq.h create mode 100644 include/uapi/sound/sof/fw.h create mode 100644 include/uapi/sound/sof/header.h create mode 100644 include/uapi/sound/sof/manifest.h create mode 100644 include/uapi/sound/sof/tokens.h create mode 100644 include/uapi/sound/sof/tone.h create mode 100644 include/uapi/sound/sof/trace.h create mode 100644 sound/soc/sof/control.c create mode 100644 sound/soc/sof/core.c create mode 100644 sound/soc/sof/debug.c create mode 100644 sound/soc/sof/ipc.c create mode 100644 sound/soc/sof/loader.c create mode 100644 sound/soc/sof/nocodec.c create mode 100644 sound/soc/sof/ops.c create mode 100644 sound/soc/sof/ops.h create mode 100644 sound/soc/sof/pcm.c create mode 100644 sound/soc/sof/pm.c create mode 100644 sound/soc/sof/sof-priv.h create mode 100644 sound/soc/sof/topology.c create mode 100644 sound/soc/sof/trace.c create mode 100644 sound/soc/sof/utils.c create mode 100644 sound/soc/sof/xtensa/Kconfig create mode 100644 sound/soc/sof/xtensa/Makefile create mode 100644 sound/soc/sof/xtensa/core.c -- 2.17.1

5 47

[Sound-open-firmware] [PATCH 00/21] Sound Open Firmware (SOF) - Intel support
by Pierre-Louis Bossart 22 Jan '19

22 Jan '19

This patch series introduces the support for Intel devices (Baytrail, CherryTrail, Broadwell/Haswell, ApolloLake, GeminiLake, CannonLake, IceLake). Support for Skylake and KabyLake is incomplete at the moment but the basic building blocks required are already present. The series starts with the 'legacy' devices, then all the more recent ones and concludes with ACPI/PCI hooks. SKL+ platforms can be compiled without HDaudio link/codec support, in which case the hdac library is not used. When support for HDMI or HDaudio codecs is selected, parts of this library are used. With Takashi's latest patches, it is understood some fixes will be needed before merge, they will be provided as an update when merged by Mark. Patch 19/21 ('ALSA-HDA-export-process_unsol_events') was submitted as a dependency last week but not yet merged. It is provided for completeness should anyone try to compile the code. The machine drivers supported by SOF are not provided as part of this series, mainly because of likely conflicts with other patches in flight to support HDaudio fallback and additional work required for upstream. The build will only work in 'nocodec' mode for now. Support for SPI-based devices, compressed streams is also not provided for now since it's not ready for upstream just yet. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre Keyon Jie (3): ASoC: SOF: Intel: Add hda-bus support and initialization ASoC: SOF: Intel: SKL, CNL, APL platform DAIs ALSA: HDA: export process_unsol_events() Liam Girdwood (15): ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support. ASoC: SOF: Intel: Add HSW HW DSP support ASoC: SOF: Intel: Add support for BDW HW DSP support ASoC: SOF: Intel: Add APL/CNL HW DSP support ASoC: SOF: Intel: Add HDA controller for Intel DSP ASoC: SOF: Intel: Add Intel specific HDA DSP HW operations ASoC: SOF: Intel: Add Intel specific HDA IPC mechanisms. ASoC: SOF: Intel: Add Intel specific HDA firmware loader ASoC: SOF: Intel: Add Intel specific HDA PCM operations ASoC: SOF: Intel: Add Intel specific HDA stream operations ASoC: SOF: Intel: Add Intel specific HDA trace operations ASoC: SOF: Intel: Add platform differentiation for SKL, APL and CNL ASoC: SOF: Add ACPI device support ASoC: SOF: Add PCI device support ASoC: SOF: Add Build support for SOF core and Intel drivers Pierre-Louis Bossart (3): ASoC: SOF: Intel: Add support for HDAudio codecs ASoC: SOF: Intel: Add SKL-specific code loader ASoC: Intel: Kconfig: expose common option between SST and SOF drivers include/sound/hdaudio.h | 1 + sound/hda/hdac_bus.c | 7 +- sound/soc/Kconfig | 1 + sound/soc/Makefile | 1 + sound/soc/intel/Kconfig | 7 +- sound/soc/sof/Kconfig | 82 +++ sound/soc/sof/Makefile | 17 + sound/soc/sof/intel/Kconfig | 136 +++++ sound/soc/sof/intel/Makefile | 17 + sound/soc/sof/intel/apl.c | 99 ++++ sound/soc/sof/intel/bdw.c | 700 +++++++++++++++++++++++ sound/soc/sof/intel/byt.c | 805 +++++++++++++++++++++++++++ sound/soc/sof/intel/cnl.c | 242 ++++++++ sound/soc/sof/intel/hda-bus.c | 107 ++++ sound/soc/sof/intel/hda-codec.c | 185 ++++++ sound/soc/sof/intel/hda-ctrl.c | 166 ++++++ sound/soc/sof/intel/hda-dai.c | 368 ++++++++++++ sound/soc/sof/intel/hda-dsp.c | 417 ++++++++++++++ sound/soc/sof/intel/hda-ipc.c | 387 +++++++++++++ sound/soc/sof/intel/hda-loader-skl.c | 531 ++++++++++++++++++ sound/soc/sof/intel/hda-loader.c | 369 ++++++++++++ sound/soc/sof/intel/hda-pcm.c | 232 ++++++++ sound/soc/sof/intel/hda-stream.c | 661 ++++++++++++++++++++++ sound/soc/sof/intel/hda-trace.c | 79 +++ sound/soc/sof/intel/hda.c | 704 +++++++++++++++++++++++ sound/soc/sof/intel/hda.h | 537 ++++++++++++++++++ sound/soc/sof/intel/hsw.c | 701 +++++++++++++++++++++++ sound/soc/sof/intel/shim.h | 159 ++++++ sound/soc/sof/intel/skl.c | 92 +++ sound/soc/sof/sof-acpi-dev.c | 269 +++++++++ sound/soc/sof/sof-pci-dev.c | 361 ++++++++++++ 31 files changed, 8435 insertions(+), 5 deletions(-) create mode 100644 sound/soc/sof/Kconfig create mode 100644 sound/soc/sof/Makefile create mode 100644 sound/soc/sof/intel/Kconfig create mode 100644 sound/soc/sof/intel/Makefile create mode 100644 sound/soc/sof/intel/apl.c create mode 100644 sound/soc/sof/intel/bdw.c create mode 100644 sound/soc/sof/intel/byt.c create mode 100644 sound/soc/sof/intel/cnl.c create mode 100644 sound/soc/sof/intel/hda-bus.c create mode 100644 sound/soc/sof/intel/hda-codec.c create mode 100644 sound/soc/sof/intel/hda-ctrl.c create mode 100644 sound/soc/sof/intel/hda-dai.c create mode 100644 sound/soc/sof/intel/hda-dsp.c create mode 100644 sound/soc/sof/intel/hda-ipc.c create mode 100644 sound/soc/sof/intel/hda-loader-skl.c create mode 100644 sound/soc/sof/intel/hda-loader.c create mode 100644 sound/soc/sof/intel/hda-pcm.c create mode 100644 sound/soc/sof/intel/hda-stream.c create mode 100644 sound/soc/sof/intel/hda-trace.c create mode 100644 sound/soc/sof/intel/hda.c create mode 100644 sound/soc/sof/intel/hda.h create mode 100644 sound/soc/sof/intel/hsw.c create mode 100644 sound/soc/sof/intel/shim.h create mode 100644 sound/soc/sof/intel/skl.c create mode 100644 sound/soc/sof/sof-acpi-dev.c create mode 100644 sound/soc/sof/sof-pci-dev.c -- 2.17.1

3 32

[Sound-open-firmware] sof_text_start address in baytrail.x.in
by Daniel Baluta 07 Jan '19

07 Jan '19

Hi, In src/platform/baytrail/baytrail.x.in we have: vector_double_text : org = XCHAL_DOUBLEEXC_VECTOR_PADDR, len = SOF_MEM_VECT_TEXT_SIZE sof_text_start : org = XCHAL_DOUBLEEXC_VECTOR_PADDR + SOF_MEM_VECT_SIZE, Is the address of sof_text_start correct here? vector_double_text ends at: XCHAL_DOUBLEEXC_VECTOR_PADDR + SOF_MEM_VECT_TEXT_SIZE but sof_text_start (which is the next segment in memory) doesn't start immediately after vector_double_text because SOF_MEM_VECT_SIZE = SOF_MEM_VECT_TEXT_SIZE + SOF_MEM_VECT_LIT_SIZE so there is a gap of SOF_MEM_VECT_LIT_SIZE (4 bytes) between vector_double_text and sof_text_start. Is this on purpose? thanks, Daniel.

2 1

[Sound-open-firmware] Linker scripts
by Daniel Baluta 21 Dec '18

21 Dec '18

Hi Liam, Do you have any guide on how to generate the linker scripts for sof/? For example sof/src/platform/suecreek/suecreek.x.in. Did you use Xtensa Xplorer? Any public docs or hints would be excellent. thanks, Daniel.

2 1

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support
by Bard liao 21 Dec '18

21 Dec '18

> Date: Wed, 12 Dec 2018 12:23:33 +0100 > From: Takashi Iwai <tiwai(a)suse.de> > To: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> > Cc: alsa-devel(a)alsa-project.org, andriy.shevchenko(a)intel.com, Daniel > Baluta <daniel.baluta(a)gmail.com>, liam.r.girdwood(a)linux.intel.com, > vkoul(a)kernel.org, broonie(a)kernel.org, Alan Cox <alan(a)linux.intel.com>, > sound-open-firmware(a)alsa-project.org > Subject: Re: [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware > loader support > Message-ID: <s5htvjjj8ei.wl-tiwai(a)suse.de> > Content-Type: text/plain; charset=US-ASCII > > On Tue, 11 Dec 2018 22:23:13 +0100, > Pierre-Louis Bossart wrote: > >> + >> + snd_sof_dsp_block_write(sdev, offset, >> + (void *)block + sizeof(*block), >> + block->size); >> + >> + /* next block */ >> + block = (void *)block + sizeof(*block) + block->size; > This may lead to an unaligned access. Did you mean we should double check the block->size to prevent access to an invalid address? > Also how is the endianess guaranteed? Did you mean we should guarantee the driver can work no matter what kernel's endianess is? ie. Use le32_to_cpu() to handle it? > > > thanks, > > Takashi > >

4 5

[Sound-open-firmware] Loading coefficients for Audio IP
by Daniel Baluta 19 Dec '18

19 Dec '18

Hello Pierre, Liam, Mark, What is the standard ALSA way of loading tables of coefficients for various audio IPs? We have IPs like PDM or ASRC which need large table of coefficients depending on configuration. This tables can reach up to 120K in size. Creating this kind of tables as arrays of integers in kernel doesn't seem to be a common used solution and we will end up with large files that contains only numbers. Another approach that we are thinking of is to create and compile this tables in userspace as raw binary files and then loading them in kernel using the firmware interface. Would this be an acceptable approach? thanks, Daniel.

3 2

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 13/14] ASoC: SOF: Add xtensa support
by Pierre-Louis Bossart 17 Dec '18

17 Dec '18

On 12/11/18 5:08 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:17PM -0600, Pierre-Louis Bossart wrote: >> Add common directory for xtensa architecture >> + for (i = 0; i <= stack_words - 4; i += 4) { >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x 0x%8.8x 0x%8.8x 0x%8.8x\n", >> + stack_ptr + i, stack[i], stack[i + 1], stack[i + 2], >> + stack[i + 3]); >> + } >> + >> + /* deal with any remaining words */ >> + switch (stack_words - i) { >> + case 0: >> + break; >> + case 1: >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x\n", >> + stack_ptr + stack_words - 1, stack[stack_words - 1]); >> + break; >> + case 2: >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x 0x%8.8x\n", >> + stack_ptr + stack_words - 2, stack[stack_words - 2], >> + stack[stack_words - 1]); >> + break; >> + case 3: >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x 0x%8.8x 0x%8.8x\n", >> + stack_ptr + stack_words - 3, stack[stack_words - 3], >> + stack[stack_words - 2], stack[stack_words - 1]); >> + break; >> + default: >> + break; >> + } > hex_dump_to_buffer(). I vaguely recall there was a reason not to do it, we'll relook at this.

3 3

Re: [Sound-open-firmware] [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support.
by Yang, Libin 17 Dec '18

17 Dec '18

Hi Sakamoto, > >-------- Forwarded Message -------- >Subject: Re: [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and >BSW DSP HW support. >Date: Wed, 12 Dec 2018 13:08:48 +0900 >From: Takashi Sakamoto <o-takashi(a)sakamocchi.jp> >To: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com>, >alsa-devel(a)alsa-project.org >CC: Daniel Baluta <daniel.baluta(a)gmail.com>, andriy.shevchenko(a)intel.com, >tiwai(a)suse.de, Pan Xiuli <xiuli.pan(a)linux.intel.com>, >liam.r.girdwood(a)linux.intel.com, vkoul(a)kernel.org, broonie(a)kernel.org, >sound-open-firmware(a)alsa-project.org, Rander Wang ><rander.wang(a)linux.intel.com>, Alan Cox <alan(a)linux.intel.com> > >Hi, > >I have small nitpicking. > >On 2018/12/12 6:30, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> Add support for the audio DSP hardware found on Intel Baytrail, >> Cherrytrail and Braswell based devices. >> >> Signed-off-by: Rander Wang <rander.wang(a)linux.intel.com> >> Signed-off-by: Pan Xiuli <xiuli.pan(a)linux.intel.com> >> Signed-off-by: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> Signed-off-by: Pierre-Louis Bossart >> <pierre-louis.bossart(a)linux.intel.com> >> --- >> sound/soc/sof/intel/byt.c | 805 >+++++++++++++++++++++++++++++++++++++ >> sound/soc/sof/intel/shim.h | 159 ++++++++ >> 2 files changed, 964 insertions(+) >> create mode 100644 sound/soc/sof/intel/byt.c >> create mode 100644 sound/soc/sof/intel/shim.h .. >> diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h >> new file mode 100644 index 000000000000..2fdcb2d262f4 >> --- /dev/null >> +++ b/sound/soc/sof/intel/shim.h >> @@ -0,0 +1,159 @@ >> ... >> +extern struct snd_sof_dsp_ops sof_byt_ops; extern struct >> +snd_sof_dsp_ops sof_cht_ops; > >These two symbols are found in this patch. > >> +extern struct snd_sof_dsp_ops sof_hsw_ops; > >This symbol is added in second patch and this first patch has no reference to it. > >> +extern struct snd_sof_dsp_ops sof_bdw_ops; > >This symbol is added in third patch and this first patch has no reference to it. > >> + >> +#endif > >I think it better to add these extern declarations when adding corresponding >symbols (Of course no issues as is). > >Furthermore, if you have no plan to change contents of these symbols in >kernel run time, it's better to have 'const' qualifier to locate the symbol to >readonly section. However, user of these symbols is machine driver and you >have a plan to implement it later. They can get 'const' >when you work for arrangement of existent/new codes. > >In this meaning, it might be good to add a new member to 'struct >snd_soc_acpi_mach' for new drivers to refer to the const ops, then 'pdata' >member is used from the existent drivers. I made a patch based on your suggestion. Could you please help review to see if my understanding is right? https://github.com/thesofproject/linux/pull/457 Regards, Libin > > >Regards > >Takashi Sakamoto >_______________________________________________ >Alsa-devel mailing list >Alsa-devel(a)alsa-project.org >http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

1 0

Re: [Sound-open-firmware] [PATCH v3 02/14] ASoC: SOF: Add Sound Open Firmware KControl support
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

>> + max_size = (be->max < max_size) ? be->max : max_size; > min() / max() ? > The use of min() and min3() seems to generate tons of sparse warnings (see below), so I'll leave the code with an explicit test and a comment stating why. sparse helped us find quite a few issues with endianess, const, iomem qualifiers, I'd like to keep it quiet on non-issues. CHECK sound/soc/sof/control.c sound/soc/sof/control.c:300:20: warning: expression using sizeof(void) sound/soc/sof/control.c:300:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void)

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support.
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

Thanks for the review. >> +++ b/sound/soc/sof/intel/shim.h >> @@ -0,0 +1,159 @@ >> ... >> +extern struct snd_sof_dsp_ops sof_byt_ops; >> +extern struct snd_sof_dsp_ops sof_cht_ops; > > These two symbols are found in this patch. > >> +extern struct snd_sof_dsp_ops sof_hsw_ops; > > This symbol is added in second patch and this first patch has > no reference to it. > >> +extern struct snd_sof_dsp_ops sof_bdw_ops; > > This symbol is added in third patch and this first patch has > no reference to it. > >> + >> +#endif > > I think it better to add these extern declarations when adding > corresponding symbols (Of course no issues as is). Yes, that's no big deal. It won't break as is though since the build is added in the last patch. > > Furthermore, if you have no plan to change contents of these symbols in > kernel run time, it's better to have 'const' qualifier to locate the > symbol to readonly section. However, user of these symbols is machine > driver and you have a plan to implement it later. They can get 'const' > when you work for arrangement of existent/new codes. Good point, I don't think the ops are changed, ever. I'll double-check and change if indeed this is the case, thanks for the suggestion. > > In this meaning, it might be good to add a new member to 'struct > snd_soc_acpi_mach' for new drivers to refer to the const ops, then > 'pdata' member is used from the existent drivers. Sorry, I don't get your point. the ops structure is included in the sof_ops_table, not the snd_soc_acpi_mach.

1 1

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 07/14] ASoC: SOF: Add DSP firmware logger support
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 5:21 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:11PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> This patch adds support for real-time DSP logging (timestamped events >> and bespoke binary data) for firmware debug. The current solution >> relies on DMA transfers to system memory that is then accessed by >> userspace tools such as sof-logger. For Intel platforms, two types of >> DMAs are currently used (GP-DMA for Baytrail/CherryTrail and HDaudio >> DMA for SKL+) >> >> Due to historical reasons, the driver code follows the DSP firmware >> conventions and refers to 'traces', but it is currently unrelated to >> the Linux trace subsystem. Future solutions will include support for >> more advanced hardware (e.g. MIPI Sys-T), additional formats and the >> ability to enable/disable specific traces dynamically. >> + if (count > buffer_size - lpos) >> + count = buffer_size - lpos; > min() / max() ? > >> + /* make sure count is <= avail */ >> + count = avail > count ? count : avail; > ditto. > Check entire series for such. yep. > >> + dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root, >> + dfse, &sof_dfs_trace_fops); >> + if (!dfse->dfsentry) { >> + dev_err(sdev->dev, >> + "error: cannot create debugfs entry for trace\n"); >> + kfree(dfse); >> + return -ENODEV; > Should not return an error. We must be fine w/o debugfs files. Not sure I fully agree with the 'must'. yes execution should proceed, but without debugfs we cannot, well, debug, so this is a real show-stopper > >> + } >> + >> + return 0; >> +}

2 1

[Sound-open-firmware] How do you provide the firmware binary, e.g sof-byt.ri file?
by Daniel Baluta 12 Dec '18

12 Dec '18

Hello all, I am having this problem where SOF is compiled as built in the kernel and the firmware binary is on the rootfs. When request_firmware is called in sof_probe the rootfs is not yet mounted so the call to request_firmware fails. Just curious, how do you provide the firmware binary? thanks, Daniel.

5 8

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 14/14] ASoC: SOF: Add utils
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 5:06 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:18PM -0600, Pierre-Louis Bossart wrote: >> Helpers to set-up back-ends, create platform devices and common >> IO/block read/write operations >> +int sof_bes_setup(struct device *dev, struct snd_sof_dsp_ops *ops, >> + struct snd_soc_dai_link *links, int link_num, >> + struct snd_soc_card *card) >> +{ >> + char name[32]; >> + int i; >> + >> + if (!ops || !links || !card) >> + return -EINVAL; >> + >> + /* set up BE dai_links */ >> + for (i = 0; i < link_num; i++) { >> + snprintf(name, 32, "NoCodec-%d", i); > sizeof(name) ? yes > >> + links[i].name = devm_kstrdup(dev, name, GFP_KERNEL); >> + if (!links[i].name) >> + return -ENOMEM; > ...or better devm_kasprintf(). and yes > >> + >> + links[i].id = i; >> + links[i].no_pcm = 1; >> + links[i].cpu_dai_name = ops->drv[i].name; >> + links[i].platform_name = "sof-audio"; >> + links[i].codec_dai_name = "snd-soc-dummy-dai"; >> + links[i].codec_name = "snd-soc-dummy"; >> + links[i].dpcm_playback = 1; >> + links[i].dpcm_capture = 1; >> + } >> + >> + card->dai_link = links; >> + card->num_links = link_num; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(sof_bes_setup); >> + >> +/* register sof platform device */ >> +int sof_create_platform_device(struct sof_platform_priv *priv) >> +{ >> + struct snd_sof_pdata *sof_pdata = priv->sof_pdata; >> + struct device *dev = sof_pdata->dev; >> + >> + priv->pdev_pcm = >> + platform_device_register_data(dev, "sof-audio", -1, >> + sof_pdata, sizeof(*sof_pdata)); >> + if (IS_ERR(priv->pdev_pcm)) { >> + dev_err(dev, "error: cannot register device sof-audio. Error %d\n", >> + (int)PTR_ERR(priv->pdev_pcm)); > Instead of casting perhaps use %ld? yes > >> + return PTR_ERR(priv->pdev_pcm); >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(sof_create_platform_device); >> +void sof_io_write64(struct snd_sof_dev *sdev, void __iomem *addr, u64 value) >> +{ >> +#ifdef CONFIG_64BIT >> + writeq(value, addr); >> +#else >> + memcpy_toio(addr, &value, sizeof(value)); >> +#endif > This ifdef sounds strange. > Just include io64-nonatomic-lo-hi.h and use readq() w/o limitations. ah yes. We added readq/writeq following your initial feedback, which broke ARCH=i386 and that was added as a workaround. > >> +} >> +EXPORT_SYMBOL(sof_io_write64); >> + >> +u64 sof_io_read64(struct snd_sof_dev *sdev, void __iomem *addr) >> +{ >> +#ifdef CONFIG_64BIT >> + return readq(addr); >> +#else >> + u64 val; >> + >> + memcpy_fromio(&val, addr, sizeof(val)); >> + return val; >> +#endif > Ditto. > >> +} >> +EXPORT_SYMBOL(sof_io_read64); >> + affected_mask = (1 << (8 * n)) - 1; >> + >> + /* first read the 32bit data of dest, then change affected >> + * bytes, and write back to dest. For unaffected bytes, it >> + * should not be changed >> + */ >> + __ioread32_copy(&tmp, dest + m * 4, 1); >> + tmp &= ~affected_mask; >> + >> + tmp |= *(u32 *)(src_byte + m * 4) & affected_mask; >> + __iowrite32_copy(dest + m * 4, &tmp, 1); > Ain't these equivalents to simple ioread32() / iowrite32() ? They are. We removed a loop but the this can be further simplified indeed. Thanks for finding this. >

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware loader support
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 4:38 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> The firmware loader exports APIs that can be called by core to load and >> process multiple different file formats. >> +static int get_ext_windows(struct snd_sof_dev *sdev, >> + struct sof_ipc_ext_data_hdr *ext_hdr) >> +{ >> + struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr; >> + >> + int ret = 0; > I don't see how it's used. Perhaps you need to check code with `make W=1`. Darn, we missed this one. I thought we were using W=1 on github but we aren't, this will be fixed. Though W=1 doesn't report this one, so need to re-inspect this. Thanks for the sighting. > >> + size_t size; >> + >> + if (w->num_windows == 0 || w->num_windows > SOF_IPC_MAX_ELEMS) >> + return -EINVAL; >> + >> + size = sizeof(*w) + sizeof(struct sof_ipc_window_elem) * w->num_windows; >> + >> + /* keep a local copy of the data */ >> + sdev->info_window = kmemdup(w, size, GFP_KERNEL); >> + if (!sdev->info_window) >> + return -ENOMEM; >> + >> + return ret; >> +} >> + dev_warn(sdev->dev, >> + "warning: block %d size zero\n", count); >> + dev_warn(sdev->dev, " type 0x%x offset 0x%x\n", >> + block->type, block->offset); > Hmm... Why do we need a kernel level duplication in words? Not sure I get this one, it's just a warning message where you don't copy a block of size zero into the target memory. > >> +int snd_sof_load_firmware(struct snd_sof_dev *sdev) >> +{ >> + dev_dbg(sdev->dev, "loading firmware\n"); > Noise. > Better to introduce a trace points and drop all these kind of messages. it's not that bad, most people understand what dmesg is, it happens once and only if you have dynamic debug. At some point we also thought about something like drm_debug where you can specify which parts you are interested in, in addition to the verbosity level. > >> + >> + if (sdev->ops->load_firmware) >> + return sdev->ops->load_firmware(sdev); >> + return 0; >> +} >> +EXPORT_SYMBOL(snd_sof_load_firmware);

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 08/14] ASoC: SOF: Add DSP HW abstraction operations
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 5:16 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:12PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> Add operation pointers that can be called by core to control a wide >> variety of DSP targets. The DSP HW drivers will fill in these >> operations. >> +int snd_sof_pci_update_bits_unlocked(struct snd_sof_dev *sdev, u32 offset, >> + u32 mask, u32 value) >> +{ >> + bool change; >> + unsigned int old, new; >> + u32 ret = ~0; /* explicit init to remove uninitialized use warnings */ >> + >> + pci_read_config_dword(sdev->pci, offset, &ret); >> + dev_dbg(sdev->dev, "Debug PCIR: %8.8x at %8.8x\n", >> + pci_read_config_dword(sdev->pci, offset, &ret), offset); >> + >> + old = ret; >> + new = (old & (~mask)) | (value & mask); >> + >> + change = (old != new); >> + if (change) { >> + pci_write_config_dword(sdev->pci, offset, new); >> + dev_dbg(sdev->dev, "Debug PCIW: %8.8x at %8.8x\n", value, >> + offset); >> + } >> + >> + return change; > What about dropping change completely? > > if (old == new) > return false; > > ... > return true; ok for all cases > > >> +} >> +EXPORT_SYMBOL(snd_sof_pci_update_bits_unlocked); >> +int snd_sof_dsp_update_bits_unlocked(struct snd_sof_dev *sdev, u32 bar, >> + u32 offset, u32 mask, u32 value) >> +{ >> + bool change; >> + unsigned int old, new; >> + u32 ret; >> + >> + ret = snd_sof_dsp_read(sdev, bar, offset); >> + >> + old = ret; >> + new = (old & (~mask)) | (value & mask); >> + >> + change = (old != new); >> + if (change) >> + snd_sof_dsp_write(sdev, bar, offset, new); >> + >> + return change; > Ditto. > Everywhere in similar places. > >> +} >> +EXPORT_SYMBOL(snd_sof_dsp_update_bits_unlocked); >> + /* check if set state successful */ >> + for (time = 5; time > 0; time--) { >> + if ((snd_sof_dsp_read(sdev, bar, offset) & mask) == target) >> + break; >> + msleep(20); >> + } >> + >> + if (!time) { >> + /* sleeping in 10ms steps so adjust timeout value */ >> + timeout /= 10; >> + >> + for (time = timeout; time > 0; time--) { >> + if ((snd_sof_dsp_read(sdev, bar, offset) & mask) == >> + target) > I would leave it on one line. ok > >> + break; >> + >> + usleep_range(5000, 10000); >> + } >> + } >> + >> + ret = time ? 0 : -ETIME; >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(snd_sof_dsp_register_poll); >> +static inline void snd_sof_dsp_write64(struct snd_sof_dev *sdev, u32 bar, >> + u32 offset, u64 value) >> +{ >> + if (sdev->ops->write64) >> + sdev->ops->write64(sdev, >> + sdev->bar[bar] + offset, value); > Why not one line? ok > >> +} >

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 4:57 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:08PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> Define an IPC ABI for all host <--> DSP communication. This ABI should >> be transport agnostic. i.e. it should work on MMIO and SPI/I2C style >> interfaces. >> + /* ssc1: TINTE */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_TINTE (1 << 0) >> + /* ssc1: PINTE */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_PINTE (1 << 1) >> + /* ssc2: SMTATF */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_SMTATF (1 << 2) >> + /* ssc2: MMRATF */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_MMRATF (1 << 3) >> + /* ssc2: PSPSTWFDFD */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_PSPSTWFDFD (1 << 4) >> + /* ssc2: PSPSRWFDFD */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_PSPSRWFDFD (1 << 5) > Style mismatch. Looks like a candidate for BIT() Yes but no. This comes from firmware definitions, so it's easier to leave them alone. Changing the kernel side of the IPC for style reasons just adds more work and chances of divergence. > >> +#define SOF_DAI_FMT_FORMAT_MASK 0x000f >> +#define SOF_DAI_FMT_CLOCK_MASK 0x00f0 >> +#define SOF_DAI_FMT_INV_MASK 0x0f00 >> +#define SOF_DAI_FMT_MASTER_MASK 0xf000 > GENMASK() ? > >> +/* flags indicating which time stamps are in sync with each other */ >> +#define SOF_TIME_HOST_SYNC (1 << 0) >> +#define SOF_TIME_DAI_SYNC (1 << 1) >> +#define SOF_TIME_WALL_SYNC (1 << 2) >> +#define SOF_TIME_STAMP_SYNC (1 << 3) >> + >> +/* flags indicating which time stamps are valid */ >> +#define SOF_TIME_HOST_VALID (1 << 8) >> +#define SOF_TIME_DAI_VALID (1 << 9) >> +#define SOF_TIME_WALL_VALID (1 << 10) >> +#define SOF_TIME_STAMP_VALID (1 << 11) >> + >> +/* flags indicating time stamps are 64bit else 3use low 32bit */ >> +#define SOF_TIME_HOST_64 (1 << 16) >> +#define SOF_TIME_DAI_64 (1 << 17) >> +#define SOF_TIME_WALL_64 (1 << 18) >> +#define SOF_TIME_STAMP_64 (1 << 19) > BIT() ? > > >> +#define SOF_IPC_PANIC_MAGIC_MASK 0x0ffff000 >> +#define SOF_IPC_PANIC_CODE_MASK 0x00000fff > GENMASK() ? same for all, when the definitions are shared with firmware it's better to keep the values as is. > >> +#define IPC_TIMEOUT_MSECS 300 > Simple _MS ? yes. > >> +/* find original TX message from DSP reply */ >> +static struct snd_sof_ipc_msg *sof_ipc_reply_find_msg(struct snd_sof_ipc *ipc, >> + u32 header) >> +{ >> + struct snd_sof_dev *sdev = ipc->sdev; >> + struct snd_sof_ipc_msg *msg; >> + >> + header = SOF_IPC_MESSAGE_ID(header); >> + >> + if (list_empty(&ipc->reply_list)) >> + goto err; > Redundant. list_for_each_*() have this check already. ok, nice catch. > >> + >> + list_for_each_entry(msg, &ipc->reply_list, list) { >> + if (SOF_IPC_MESSAGE_ID(msg->header) == header) >> + return msg; >> + } >> + >> +err: >> + dev_err(sdev->dev, "error: rx list empty but received 0x%x\n", >> + header); >> + return NULL; >> +} >> +int snd_sof_ipc_valid(struct snd_sof_dev *sdev) >> +{ >> + struct sof_ipc_fw_ready *ready = &sdev->fw_ready; >> + struct sof_ipc_fw_version *v = &ready->version; >> + >> + dev_info(sdev->dev, >> + " Firmware info: version %d:%d:%d-%s\n", v->major, v->minor, > Indentation issues. I believe they are intentional, but that's Liam's part. > >> + v->micro, v->tag); >> + dev_info(sdev->dev, >> + " Firmware: ABI %d:%d:%d Kernel ABI %d:%d:%d\n", >> + SOF_ABI_VERSION_MAJOR(v->abi_version), >> + SOF_ABI_VERSION_MINOR(v->abi_version), >> + SOF_ABI_VERSION_PATCH(v->abi_version), >> + SOF_ABI_MAJOR, SOF_ABI_MINOR, SOF_ABI_PATCH); >> + >> + if (SOF_ABI_VERSION_INCOMPATIBLE(SOF_ABI_VERSION, v->abi_version)) { >> + dev_err(sdev->dev, "error: incompatible FW ABI version\n"); >> + return -EINVAL; >> + } >> + >> + if (ready->debug.bits.build) { >> + dev_info(sdev->dev, >> + " Firmware debug build %d on %s-%s - options:\n" >> + " GDB: %s\n" >> + " lock debug: %s\n" >> + " lock vdebug: %s\n", >> + v->build, v->date, v->time, >> + ready->debug.bits.gdb ? "enabled" : "disabled", >> + ready->debug.bits.locks ? "enabled" : "disabled", >> + ready->debug.bits.locks_verbose ? >> + "enabled" : "disabled"); > I would leave it on one line (only 3 characters more, but better readability). > > ready->debug.bits.locks_verbose ? "enabled" : "disabled"); That must be a result of me asking folks to run checkpatch.pl --strict... > >> + } >> + >> + /* copy the fw_version into debugfs at first boot */ >> + memcpy(&sdev->fw_version, v, sizeof(*v)); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(snd_sof_ipc_valid); >> + dev_dbg(sdev->dev, "pre-allocate %d IPC messages\n", >> + IPC_EMPTY_LIST_SIZE); > Noise. ok > >> + >> + /* pre-allocate message data */ >> + for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) { >> + msg->msg_data = devm_kzalloc(sdev->dev, PAGE_SIZE, GFP_KERNEL); >> + if (!msg->msg_data) >> + return NULL; >> + >> + msg->reply_data = devm_kzalloc(sdev->dev, PAGE_SIZE, >> + GFP_KERNEL); >> + if (!msg->reply_data) >> + return NULL; > Can't you just get enough (non-continuous?) pages at once via get_free_pages interface? > I didn't know that one, so, consider rather a way to look into. Dunno either, maybe something for Liam to look at? > >> + >> + init_waitqueue_head(&msg->waitq); >> + list_add(&msg->list, &ipc->empty_list); >> + msg++; >> + } >> + >> + return ipc; >> +} >> +EXPORT_SYMBOL(snd_sof_ipc_init);

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 03/14] ASoC: SOF: Add driver debug support.
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

>> + if (count > size - pos) >> + count = size - pos; > max() / min() ok > >> + /* intermediate buffer size must be u32 multiple */ >> + size = (count + 3) & ~3; > Hmm, don't we have something like round_up()? yes, round_up (count, 4) would work. > >> + dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root, >> + dfse, &sof_dfs_fops); >> + if (!dfse->dfsentry) { >> + dev_err(sdev->dev, "error: cannot create debugfs entry.\n"); >> + return -ENODEV; >> + } > We shouldn't rely on debugfs files. > Thus, error checking needs to be removed. > > Something like > > void () > { > struct dentry *d; > > d = debugfs_...(); > if (!d) > dev_err(); > } You lost me on this one. The main purpose of this return -ENODEV is to avoid adding more entries if one item failed. It's not different from what our estimeed colleagues did in skl-debug.c > > >> + dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root, >> + dfse, &sof_dfs_fops); >> + if (!dfse->dfsentry) { >> + dev_err(sdev->dev, "error: cannot create debugfs entry.\n"); >> + return -ENODEV; >> + } > Ditto. > >> + sdev->debugfs_root = debugfs_create_dir("sof", NULL); >> + if (IS_ERR_OR_NULL(sdev->debugfs_root)) { >> + dev_err(sdev->dev, "error: failed to create debugfs directory\n"); >> + return -EINVAL; >> + } >> + >> + /* create debugFS files for platform specific MMIO/DSP memories */ >> + for (i = 0; i < ops->debug_map_count; i++) { >> + map = &ops->debug_map[i]; >> + >> + err = snd_sof_debugfs_io_create_item(sdev, sdev->bar[map->bar] + >> + map->offset, map->size, >> + map->name); >> + if (err < 0) >> + dev_err(sdev->dev, "error: cannot create debugfs for %s\n", >> + map->name); > Ditto (error message dups above). > >> + } >> + >> + return err; or maybe that's the issue, we shouldn't return an error on the init proper, but I see nothing wrong in stopping when things go south. >> +} >> +EXPORT_SYMBOL(snd_sof_dbg_init);

1 0

Re: [Sound-open-firmware] [PATCH v3 01/14] ASoC: SOF: Add Sound Open Firmware driver core
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

Thanks for the late night review Andy :-) On 12/11/18 4:20 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:05PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> The Sound Open Firmware driver core is a generic architecture >> independent layer that allows SOF to be used on many different different >> architectures and platforms. It abstracts DSP operations and IO methods >> so that the target DSP can be an internal memory mapped or external SPI >> or I2C based device. This abstraction also allows SOF to be run on >> many different VMs on the same physical HW. >> >> SOF also requires some data in ASoC PCM runtime data for looking up >> SOF data during ASoC PCM operations. >> +/* SOF defaults if not provided by the platform in ms */ >> +#define TIMEOUT_DEFAULT_IPC 5 >> +#define TIMEOUT_DEFAULT_BOOT 100 > Perhaps _MS at the end? ok > >> +struct snd_sof_dai *snd_sof_find_dai(struct snd_sof_dev *sdev, >> + char *name) >> +{ >> + struct snd_sof_dai *dai = NULL; >> + >> + list_for_each_entry(dai, &sdev->dai_list, list) { >> + if (!dai->name) >> + continue; >> + >> + if (strcmp(name, dai->name) == 0) >> + return dai; > Perhaps > > if (dai->name && strcmp(...)) ok > >> + } >> + >> + return NULL; >> +} >> +int snd_sof_create_page_table(struct snd_sof_dev *sdev, >> + struct snd_dma_buffer *dmab, >> + unsigned char *page_table, size_t size) >> +{ >> + int i, pages; >> + >> + pages = snd_sgbuf_aligned_pages(size); >> + >> + dev_dbg(sdev->dev, "generating page table for %p size 0x%zx pages %d\n", >> + dmab->area, size, pages); >> + >> + for (i = 0; i < pages; i++) { >> + u32 idx = (((i << 2) + i)) >> 1; > Looks like > > u32 idx = (5 * i) >> 1; Yes, but we'd also want an explanation of what we try to multiply by 2.5... Liam or Keyon, can you chime in? > >> + u32 pfn = snd_sgbuf_get_addr(dmab, i * PAGE_SIZE) >> PAGE_SHIFT; >> + u32 *pg_table; >> + >> + dev_vdbg(sdev->dev, "pfn i %i idx %d pfn %x\n", i, idx, pfn); >> + >> + pg_table = (u32 *)(page_table + idx); >> + >> + if (i & 1) >> + *pg_table |= (pfn << 4); >> + else >> + *pg_table |= pfn; >> + } >> + >> + return pages; >> +} >> + if (plat_data->type == SOF_DEVICE_PCI) >> + sdev->pci = container_of(plat_data->dev, struct pci_dev, dev); > Why not to use generic functions? > dev_is_pci() > to_pci_dev() ok > >> + /* register machine driver, pass machine info as pdata */ >> + plat_data->pdev_mach = >> + platform_device_register_data(sdev->dev, drv_name, >> + -1, mach, size); > PLATFORM_DEVID_AUTO (IIRC the name)? did you mean replace -1 by PLATFORM_DEVID_NONE? > >> +static int sof_remove(struct platform_device *pdev) >> +{ >> + struct snd_sof_dev *sdev = dev_get_drvdata(&pdev->dev); >> + struct snd_sof_pdata *pdata = sdev->pdata; >> + >> + if (pdata && !IS_ERR(pdata->pdev_mach)) >> + platform_device_unregister(pdata->pdev_mach); > I'm wondering if pdata could be ever NULL here. I didn't find an error flow that yields NULL but Liam reported some issues that made no sense so might be safer with NULL > Also, as I mentioned internally the patch to accept error pointers would be in > v4.21. Indeed, but I can't rely on this just yet. > >> + >> + snd_soc_unregister_component(&pdev->dev); >> + snd_sof_fw_unload(sdev); >> + snd_sof_ipc_free(sdev); >> + snd_sof_free_debug(sdev); >> + snd_sof_free_trace(sdev); >> + snd_sof_remove(sdev); >> + return 0; >> +} >> + >> +void snd_sof_shutdown(struct device *dev) >> +{ >> +} >> +EXPORT_SYMBOL(snd_sof_shutdown); > No need to provide an empty stub. Why not to add when it would have something useful? ok > >> +/* max BARs mmaped devices can use */ >> +#define SND_SOF_BARS 8 >> + >> +/* time in ms for runtime suspend delay */ >> +#define SND_SOF_SUSPEND_DELAY 2000 > _MS ? ok > >> + struct mutex mutex; /* access mutex */ >> + struct list_head list; /* list in sdev pcm list */ >> + struct snd_pcm_hw_params params[2]; >> + int restore_stream[2]; /* restore hw_params for paused stream */ >> + u32 readback_offset; /* offset to mmaped data if used */ >> + struct sof_ipc_ctrl_data *control_data; >> + u32 size; /* cdata size */ >> + enum sof_ipc_ctrl_cmd cmd; >> + u32 *volume_table; /* volume table computed from tlv data*/ >> + >> + struct mutex mutex; /* access mutex */ >> + struct list_head list; /* list in sdev control list */

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 02/14] ASoC: SOF: Add Sound Open Firmware KControl support
by Pierre-Louis Bossart 11 Dec '18

11 Dec '18

>> + max_size = (be->max < max_size) ? be->max : max_size; > min() / max() ? Good catch, this can be simplified. Not sure I like max_size = min (be->max, max_size), it's confusing. Maybe if (be->max < max_size) max_size = be->max;

1 0