[PATCH 0/6] ASoC: SOF: Intel: hda-mlink: fixes and extensions
With additional testing with multiple links and multiple DAI types, we found a couple of mistakes with refcounts, base address, missing initialization.
A new helper was also added due to a change in the SoundWire programming sequences, with the host driver in charge of setting up the DMA channel mapping instead of the firmware.
Pierre-Louis Bossart (6): ASoC: SOF: Intel: hda-mlink: fix sublink refcounting ASoC: SOF: Intel: hda-mlink: add helper to get SoundWire hlink ASoC: SOF: Intel: hda-mlink: fix base_ptr computation ASoC: SOF: Intel: hda-mlink: use 'ml_addr' parameter consistently ASoC: SOF: Intel: hda-mlink: initialize instance_offset member ASoC: SOF: Intel: hda-mlink: add helper to program SoundWire PCMSyCM registers
include/sound/hda-mlink.h | 14 +++++ sound/soc/sof/intel/hda-mlink.c | 96 +++++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 9 deletions(-)
In hindsight it was a very bad idea to use the same refcount for Extended and 'legacy' HDaudio multi-links. The existing solution only powers-up the first sublink, which causes SoundWire and SSP tests to fail when more than one DAI is used concurrently. Solving this problem requires per-sublink refcounting, as suggested in this patch.
The existing refcounting remains for 'legacy' HdAudio links, mainly to avoid changing the obscure programming sequence in snd_hdac_ext_bus_link_put().
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/intel/hda-mlink.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c index 775582ab7494..6d0145c30afe 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -19,6 +19,9 @@
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_MLINK)
+/* worst-case number of sublinks is used for sublink refcount array allocation only */ +#define HDAML_MAX_SUBLINKS (AZX_ML_LCTL_CPA_SHIFT - AZX_ML_LCTL_SPA_SHIFT) + /** * struct hdac_ext2_link - HDAudio extended+alternate link * @@ -33,6 +36,7 @@ * @leptr: extended link pointer * @eml_lock: mutual exclusion to access shared registers e.g. CPA/SPA bits * in LCTL register + * @sublink_ref_count: array of refcounts, required to power-manage sublinks independently * @base_ptr: pointer to shim/ip/shim_vs space * @instance_offset: offset between each of @slcount instances managed by link * @shim_offset: offset to SHIM register base @@ -53,6 +57,7 @@ struct hdac_ext2_link { u32 leptr;
struct mutex eml_lock; /* prevent concurrent access to e.g. CPA/SPA */ + int sublink_ref_count[HDAML_MAX_SUBLINKS];
/* internal values computed from LCAP contents */ void __iomem *base_ptr; @@ -641,8 +646,13 @@ static int hdac_bus_eml_power_up_base(struct hdac_bus *bus, bool alt, int elid, if (eml_lock) mutex_lock(&h2link->eml_lock);
- if (++hlink->ref_count > 1) - goto skip_init; + if (!alt) { + if (++hlink->ref_count > 1) + goto skip_init; + } else { + if (++h2link->sublink_ref_count[sublink] > 1) + goto skip_init; + }
ret = hdaml_link_init(hlink->ml_addr + AZX_REG_ML_LCTL, sublink);
@@ -684,9 +694,13 @@ static int hdac_bus_eml_power_down_base(struct hdac_bus *bus, bool alt, int elid if (eml_lock) mutex_lock(&h2link->eml_lock);
- if (--hlink->ref_count > 0) - goto skip_shutdown; - + if (!alt) { + if (--hlink->ref_count > 0) + goto skip_shutdown; + } else { + if (--h2link->sublink_ref_count[sublink] > 0) + goto skip_shutdown; + } ret = hdaml_link_shutdown(hlink->ml_addr + AZX_REG_ML_LCTL, sublink);
skip_shutdown:
Same functionality as for DMIC/SSP with different ID.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- include/sound/hda-mlink.h | 4 ++++ sound/soc/sof/intel/hda-mlink.c | 12 ++++++++++++ 2 files changed, 16 insertions(+)
diff --git a/include/sound/hda-mlink.h b/include/sound/hda-mlink.h index dbc47af08135..5bfa8ae940e4 100644 --- a/include/sound/hda-mlink.h +++ b/include/sound/hda-mlink.h @@ -51,6 +51,7 @@ int hda_bus_ml_suspend(struct hdac_bus *bus);
struct hdac_ext_link *hdac_bus_eml_ssp_get_hlink(struct hdac_bus *bus); struct hdac_ext_link *hdac_bus_eml_dmic_get_hlink(struct hdac_bus *bus); +struct hdac_ext_link *hdac_bus_eml_sdw_get_hlink(struct hdac_bus *bus);
struct mutex *hdac_bus_eml_get_mutex(struct hdac_bus *bus, bool alt, int elid);
@@ -155,6 +156,9 @@ hdac_bus_eml_ssp_get_hlink(struct hdac_bus *bus) { return NULL; } static inline struct hdac_ext_link * hdac_bus_eml_dmic_get_hlink(struct hdac_bus *bus) { return NULL; }
+static inline struct hdac_ext_link * +hdac_bus_eml_sdw_get_hlink(struct hdac_bus *bus) { return NULL; } + static inline struct mutex * hdac_bus_eml_get_mutex(struct hdac_bus *bus, bool alt, int elid) { return NULL; }
diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c index 6d0145c30afe..cfa43d93cbd0 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -850,6 +850,18 @@ struct hdac_ext_link *hdac_bus_eml_dmic_get_hlink(struct hdac_bus *bus) } EXPORT_SYMBOL_NS(hdac_bus_eml_dmic_get_hlink, SND_SOC_SOF_HDA_MLINK);
+struct hdac_ext_link *hdac_bus_eml_sdw_get_hlink(struct hdac_bus *bus) +{ + struct hdac_ext2_link *h2link; + + h2link = find_ext2_link(bus, true, AZX_REG_ML_LEPTR_ID_SDW); + if (!h2link) + return NULL; + + return &h2link->hext_link; +} +EXPORT_SYMBOL_NS(hdac_bus_eml_sdw_get_hlink, SND_SOC_SOF_HDA_MLINK); + int hdac_bus_eml_enable_offload(struct hdac_bus *bus, bool alt, int elid, bool enable) { struct hdac_ext2_link *h2link;
The base_ptr value needs to be derived from the remap_addr pointer, not the ml_addr. This base_ptr was used only in debug logs that were so far not contributed upstream so the issue was not detected. It needs to be fixed for SoundWire support on LunarLake.
Fixes: 17c9b6ec35c0 ("ASoC: SOF: Intel: hda-mlink: add structures to parse ALT links") Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-mlink.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c index cfa43d93cbd0..7f5d0ea4abeb 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -96,7 +96,7 @@ struct hdac_ext2_link { */
static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link, - void __iomem *ml_addr, int link_idx) + void __iomem *remap_addr, void __iomem *ml_addr, int link_idx) { struct hdac_ext_link *hlink = &h2link->hext_link; u32 base_offset; @@ -136,7 +136,7 @@ static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link, h2link->elid = FIELD_GET(AZX_REG_ML_LEPTR_ID, h2link->leptr);
base_offset = FIELD_GET(AZX_REG_ML_LEPTR_PTR, h2link->leptr); - h2link->base_ptr = hlink->ml_addr + base_offset; + h2link->base_ptr = remap_addr + base_offset;
switch (h2link->elid) { case AZX_REG_ML_LEPTR_ID_SDW: @@ -369,7 +369,7 @@ static int hda_ml_alloc_h2link(struct hdac_bus *bus, int index) hlink->bus = bus; hlink->ml_addr = bus->mlcap + AZX_ML_BASE + (AZX_ML_INTERVAL * index);
- ret = hdaml_lnk_enum(bus->dev, h2link, hlink->ml_addr, index); + ret = hdaml_lnk_enum(bus->dev, h2link, bus->remap_addr, hlink->ml_addr, index); if (ret < 0) { kfree(h2link); return ret;
We mix the use of hlink->ml_addr and the 'ml_addr' parameter. It's the same thing, let's align on using the parameter.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-mlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c index 7f5d0ea4abeb..c540b1d75451 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -131,7 +131,7 @@ static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link, link_idx, h2link->slcount);
/* find IP ID and offsets */ - h2link->leptr = readl(hlink->ml_addr + AZX_REG_ML_LEPTR); + h2link->leptr = readl(ml_addr + AZX_REG_ML_LEPTR);
h2link->elid = FIELD_GET(AZX_REG_ML_LEPTR_ID, h2link->leptr);
We defined the values but never initialized it for SoundWire/SSP, fix this miss.
A Fixes: tag is not provided as instance_offset was not used so far, so nothing was really broken. This patch is only required for the SoundWire support in the following patch.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-mlink.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c index c540b1d75451..2d0c5d5914b1 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -140,6 +140,7 @@ static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link,
switch (h2link->elid) { case AZX_REG_ML_LEPTR_ID_SDW: + h2link->instance_offset = AZX_REG_SDW_INSTANCE_OFFSET; h2link->shim_offset = AZX_REG_SDW_SHIM_OFFSET; h2link->ip_offset = AZX_REG_SDW_IP_OFFSET; h2link->shim_vs_offset = AZX_REG_SDW_VS_SHIM_OFFSET; @@ -154,6 +155,7 @@ static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link, link_idx, base_offset); break; case AZX_REG_ML_LEPTR_ID_INTEL_SSP: + h2link->instance_offset = AZX_REG_INTEL_SSP_INSTANCE_OFFSET; h2link->shim_offset = AZX_REG_INTEL_SSP_SHIM_OFFSET; h2link->ip_offset = AZX_REG_INTEL_SSP_IP_OFFSET; h2link->shim_vs_offset = AZX_REG_INTEL_SSP_VS_SHIM_OFFSET;
These registers enable the HDaudio DMA hardware to split/merge data from different PDIs, possibly on different links.
This capability exists for all types of HDaudio extended links, but for now is only required for SoundWire. In the SSP/DMIC case, the IP is programmed by the DSP firmware.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- include/sound/hda-mlink.h | 10 +++++++ sound/soc/sof/intel/hda-mlink.c | 50 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+)
diff --git a/include/sound/hda-mlink.h b/include/sound/hda-mlink.h index 5bfa8ae940e4..4f44f0bd5388 100644 --- a/include/sound/hda-mlink.h +++ b/include/sound/hda-mlink.h @@ -44,6 +44,9 @@ int hdac_bus_eml_sdw_power_down_unlocked(struct hdac_bus *bus, int sublink);
int hdac_bus_eml_sdw_set_lsdiid(struct hdac_bus *bus, int sublink, int dev_num);
+int hdac_bus_eml_sdw_map_stream_ch(struct hdac_bus *bus, int sublink, int y, + int channel_mask, int stream_id, int dir); + void hda_bus_ml_put_all(struct hdac_bus *bus); void hda_bus_ml_reset_losidv(struct hdac_bus *bus); int hda_bus_ml_resume(struct hdac_bus *bus); @@ -145,6 +148,13 @@ hdac_bus_eml_sdw_power_down_unlocked(struct hdac_bus *bus, int sublink) { return static inline int hdac_bus_eml_sdw_set_lsdiid(struct hdac_bus *bus, int sublink, int dev_num) { return 0; }
+static inline int +hdac_bus_eml_sdw_map_stream_ch(struct hdac_bus *bus, int sublink, int y, + int channel_mask, int stream_id, int dir) +{ + return 0; +} + static inline void hda_bus_ml_put_all(struct hdac_bus *bus) { } static inline void hda_bus_ml_reset_losidv(struct hdac_bus *bus) { } static inline int hda_bus_ml_resume(struct hdac_bus *bus) { return 0; } diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c index 2d0c5d5914b1..b7cbf66badf5 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -73,6 +73,7 @@ struct hdac_ext2_link { #define AZX_REG_SDW_SHIM_OFFSET 0x0 #define AZX_REG_SDW_IP_OFFSET 0x100 #define AZX_REG_SDW_VS_SHIM_OFFSET 0x6000 +#define AZX_REG_SDW_SHIM_PCMSyCM(y) (0x16 + 0x4 * (y))
/* only one instance supported */ #define AZX_REG_INTEL_DMIC_SHIM_OFFSET 0x0 @@ -340,6 +341,21 @@ static void hdaml_link_set_lsdiid(u32 __iomem *lsdiid, int dev_num) writel(val, lsdiid); }
+static void hdaml_shim_map_stream_ch(u16 __iomem *pcmsycm, int lchan, int hchan, + int stream_id, int dir) +{ + u16 val; + + val = readw(pcmsycm); + + u16p_replace_bits(&val, lchan, GENMASK(3, 0)); + u16p_replace_bits(&val, hchan, GENMASK(7, 4)); + u16p_replace_bits(&val, stream_id, GENMASK(13, 8)); + u16p_replace_bits(&val, dir, BIT(15)); + + writew(val, pcmsycm); +} + static void hdaml_lctl_offload_enable(u32 __iomem *lctl, bool enable) { u32 val = readl(lctl); @@ -756,6 +772,40 @@ int hdac_bus_eml_sdw_set_lsdiid(struct hdac_bus *bus, int sublink, int dev_num) return 0; } EXPORT_SYMBOL_NS(hdac_bus_eml_sdw_set_lsdiid, SND_SOC_SOF_HDA_MLINK);
+/* + * the 'y' parameter comes from the PCMSyCM hardware register naming. 'y' refers to the + * PDI index, i.e. the FIFO used for RX or TX + */ +int hdac_bus_eml_sdw_map_stream_ch(struct hdac_bus *bus, int sublink, int y, + int channel_mask, int stream_id, int dir) +{ + struct hdac_ext2_link *h2link; + u16 __iomem *pcmsycm; + u16 val; + + h2link = find_ext2_link(bus, true, AZX_REG_ML_LEPTR_ID_SDW); + if (!h2link) + return -ENODEV; + + pcmsycm = h2link->base_ptr + h2link->shim_offset + + h2link->instance_offset * sublink + + AZX_REG_SDW_SHIM_PCMSyCM(y); + + mutex_lock(&h2link->eml_lock); + + hdaml_shim_map_stream_ch(pcmsycm, 0, hweight32(channel_mask), + stream_id, dir); + + mutex_unlock(&h2link->eml_lock); + + val = readw(pcmsycm); + + dev_dbg(bus->dev, "channel_mask %#x stream_id %d dir %d pcmscm %#x\n", + channel_mask, stream_id, dir, val); + + return 0; +} EXPORT_SYMBOL_NS(hdac_bus_eml_sdw_map_stream_ch, SND_SOC_SOF_HDA_MLINK); + void hda_bus_ml_put_all(struct hdac_bus *bus) { struct hdac_ext_link *hlink;
On Fri, 12 May 2023 12:46:05 -0500, Pierre-Louis Bossart wrote:
With additional testing with multiple links and multiple DAI types, we found a couple of mistakes with refcounts, base address, missing initialization.
A new helper was also added due to a change in the SoundWire programming sequences, with the host driver in charge of setting up the DMA channel mapping instead of the firmware.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: SOF: Intel: hda-mlink: fix sublink refcounting commit: 7430dea49410de3d154fb87f931d079a0a643b1a [2/6] ASoC: SOF: Intel: hda-mlink: add helper to get SoundWire hlink commit: dcb88fc47d0e79fd54a19a63a4c8a7594ba0838e [3/6] ASoC: SOF: Intel: hda-mlink: fix base_ptr computation commit: af8c32b1a3d55f9b42294aee7e7c7eca85ee3bd2 [4/6] ASoC: SOF: Intel: hda-mlink: use 'ml_addr' parameter consistently commit: 7dfd1ccdb71e5b819c2898b59c58b89f26038292 [5/6] ASoC: SOF: Intel: hda-mlink: initialize instance_offset member commit: 9643456ec3c48adfe535c56f659ab705365f4572 [6/6] ASoC: SOF: Intel: hda-mlink: add helper to program SoundWire PCMSyCM registers commit: ccc2f0c1b6b613cd0014c3dcd465a4b57856b0fe
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Pierre-Louis Bossart