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

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 12 Dec '18

12 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

[Sound-open-firmware] MIC record support on Apollo lake
by Muhammed, Wasim 16 Nov '18

16 Nov '18

Hi , I am trying to add MIC as an record device in the SOF firmware on the Apollo lake MRB platform. Is this supported by the SOF architecture already? Your response is highly appreciated. Thanks BR Wasim

2 2

[Sound-open-firmware] FW: GLK v0.5 release for Google - SoF Tools Build Error
by M, Naveen 09 Oct '18

09 Oct '18

JFYI, (re-sent due to access issues) Can you please guide me with below issue. Thanks, Naveen From: M, Naveen Sent: Monday, October 1, 2018 10:44 AM To: 'sound-open-firmware(a)alsa-project.org' <sound-open-firmware(a)alsa-project.org> Subject: GLK v0.5 release for Google - SoF Tools Build Error Hello All, Tried building SoFT as per the release document(SoF commits as below) but observe following issue. SOF FW source https://github.com/thesofproject/sof/tree/glk-003-drop-stable Commit: 0ead9e762b36b87cd59afe44e90f912978a79fad (marked as: https://github.com/thesofproject/sof/releases/tag/0ead9e7 ) SOF topology https://github.com/thesofproject/soft/tree/master Commit: 918e4013191f67600dd11eb2d40bc7535b7aa3c9 SOF Drivers source https://github.com/thesofproject/linux/tree/topic/cros-sof-v4.14-rebase 3d04ca232eebd739cbdabeeb0ec9ea705fdbafec Build Error Log: Making all in logger make[2]: Entering directory '/home/nm/google/sof/soft/logger' gcc -DHAVE_CONFIG_H -I. -I.. -g -O2 -MT logger.o -MD -MP -MF .deps/logger.Tpo -c -o logger.o logger.c logger.c:7:10: fatal error: /usr/local/include/sof/uapi/logging.h: No such file or directory #include "/usr/local/include/sof/uapi/logging.h" ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. Makefile:353: recipe for target 'logger.o' failed make[2]: *** [logger.o] Error 1 make[2]: Leaving directory '/home/nm/google/sof/soft/logger' Makefile:365: recipe for target 'all-recursive' failed make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory '/home/nm/google/sof/soft' Makefile:306: recipe for target 'all' failed make: *** [all] Error 2 NOTE: make from path soft/topology, generates the topology binary & .conf fine Did I miss anything ? Please guide me. Thanks, Naveen

3 3

Re: [Sound-open-firmware] Any SOF pcm hw parameters change that can disable PAUSE on GLK?
by Lin, Mengdong 09 Oct '18

09 Oct '18

Hi Marcin/Wojtek, Could you run "dmesg" and share the output? As well as the output of "aplay -l" and "lsmod"? Maybe there is something wrong when SOF opens the PCM. It seems driver will always enable PAUSE if sof_pcm_open() succeeds by setting SNDRV_PCM_INFO_PAUSE. Please see https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/pcm… static int sof_pcm_open(struct snd_pcm_substream *substream) { ... /* set runtime config */ runtime->hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME | SNDRV_PCM_INFO_NO_PERIOD_WAKEUP; ... } Thanks Mengdong > -----Original Message----- > From: Maka, Marcin > Sent: Tuesday, October 9, 2018 6:45 PM > To: Jie, Yang <yang.jie(a)intel.com>; Lin, Mengdong > <mengdong.lin(a)intel.com>; OTC Linux Audio <otc.linux.audio(a)intel.com>; > Wittbrodt, WojciechX <wojciechx.wittbrodt(a)intel.com> > Cc: sound-open-firmware(a)alsa-project.org > Subject: RE: Any SOF pcm hw parameters change that can disable PAUSE on > GLK? > > I am using SOF stack built by Wojtek and installed on Yorp. > > > -----Original Message----- > > From: Jie, Yang > > Sent: Tuesday, October 9, 2018 11:45 AM > > To: Lin, Mengdong <mengdong.lin(a)intel.com>; OTC Linux Audio > > <otc.linux.audio(a)intel.com>; Maka, Marcin <marcin.maka(a)intel.com> > > Cc: sound-open-firmware(a)alsa-project.org > > Subject: RE: Any SOF pcm hw parameters change that can disable PAUSE > on GLK? > > > > > > I bet you are not using SOF driver, Marcin? > > > > We need to configure pcm info with SNDRV_PCM_INFO_PAUSE if you > want > > your PCM supporting that, please refer to sof/pcm.c:404 for that. > > > > Thanks, > > ~Keyon > > > > > > >-----Original Message----- > > >From: Lin, Mengdong > > >Sent: Tuesday, October 9, 2018 5:27 PM > > >To: OTC Linux Audio <otc.linux.audio(a)intel.com>; Maka, Marcin > > ><marcin.maka(a)intel.com> > > >Cc: sound-open-firmware(a)alsa-project.org > > >Subject: Any SOF pcm hw parameters change that can disable PAUSE on > GLK? > > > > > >Could someone share some insight here? > > > > > >This is an issue observed by Marcin on GNL platform. > > >When he runs 'aplay Ring09.wav -vv -I' and press SPACE to > > >pause/resume, aplay will complain: PAUSE command ignored (no hw > > >support) > > > > > >aplay.c will call snd_pcm_hw_params_can_pause() and decide whethter > > >HW supports pause. It seems hw parameter indicates that PAUSE cannot > > >be supported. > > > > > >Here is snd_pcm_hw_params_can_pause defined in alsa-lib pcm.c > > >/** > > >3487 * \brief Check if hardware supports pause > > >3488 * \param params Configuration space > > >3489 * \retval 0 Hardware doesn't support pause > > >3490 * \retval 1 Hardware supports pause > > >3491 * > > >3492 * This function should only be called when the configuration > > >space > > >3493 * contains a single configuration. Call #snd_pcm_hw_params to > > >choose > > >3494 * a single configuration from the configuration space. > > >3495 */ > > >3496 int snd_pcm_hw_params_can_pause(const > snd_pcm_hw_params_t > > >*params) > > >3497 { > > >3498 assert(params); > > >3499 if (CHECK_SANITY(params->info == ~0U)) { > > >3500 SNDMSG("invalid PCM info field"); > > >3501 return 0; /* FIXME: should be a negative error? */ > > >3502 } > > >3503 return !!(params->info & SNDRV_PCM_INFO_PAUSE); > > >3504 } > > > > > >Thanks > > >Mengdong > > >

1 0