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

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

09 Jan '19

On 1/9/19 2:51 PM, Mark Brown wrote: > On Tue, Dec 11, 2018 at 03:23:12PM -0600, Pierre-Louis Bossart wrote: > >> +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 */ > This looks a lot like you want to write regmap_pci_config... I think you made that note for the v2 a long time ago, not sure if I replied at the time. We only use this for 4 cases (power/clock gating on/off, traffic class, etc) and only during the hardware initialization. This is similar to the legacy and Skylake driver, I don't see a significant benefit with a regmap? intel/hda-ctrl.c: snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_MISCBDCGE_MASK, val); intel/hda-ctrl.c: snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_ADSPDCGE, val); intel/hda-ctrl.c: snd_sof_pci_update_bits(sdev, PCI_PGCTL, PCI_PGCTL_ADSPPGD, val); intel/hda-dsp.c: snd_sof_pci_update_bits(sdev, PCI_PGCTL, intel/hda-dsp.c: snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0); > >> +/* control */ >> +static inline int snd_sof_dsp_run(struct snd_sof_dev *sdev) >> +{ >> + if (sdev->ops->run) >> + return sdev->ops->run(sdev); >> + >> + return 0; >> +} > Do we really want to return 0 for all these ops if they're not > implemented? For some that seems sensible but there's others where it > seems like the caller might want to know they got ignored and an error > code like -ENOTSUPP might be better. Good point indeed. There are a set of ops that are really mandatory, we should rework this instead. Thanks for the comment.

1 0

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

09 Jan '19

Thanks for the reviews Mark, much appreciated. +int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev) >> +{ >> + struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev); >> + const char *fw_filename; >> + int ret; > This never actually calls the load_firmware() operation for the DSP > AFAICT? it's actually the implementation of the load_firmware callback, see e.g. for Baytrail/Broadwell /*Firmware loading */ .load_firmware = snd_sof_load_firmware_memcpy, > >> + ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev); >> + >> + if (ret < 0) { >> + dev_err(sdev->dev, "error: request firmware failed err: %d\n", >> + ret); >> + return ret; >> + } > I'd suggest logging the name of the firmware we tried to load, users > will thank you. yes indeed, thanks for the suggestion, will add this right away. > >> + /* create fw_version debugfs to store boot version info */ >> + if (sdev->first_boot) { >> + ret = snd_sof_debugfs_buf_create_item(sdev, &sdev->fw_version, >> + sizeof(sdev->fw_version), >> + "fw_version"); >> + >> + if (ret < 0) { >> + dev_err(sdev->dev, "error: cannot create debugfs for fw_version\n"); >> + return ret; >> + } >> + } > As Andy said elsewhere debugfs stuff like that probably shouldn't be > fatal. yes, we've fixed this already.

1 0

[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