| Message ID | 20230327112931.23411-9-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: SOF: Intel: hda-mlink: HDaudio multi-link extension update | expand |
On 3/27/2023 1:29 PM, Peter Ujfalusi wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Add helpers to program SPA/CPA bits, using a mutex to access the > shared LCTL register if required. > > All links are managed with the same LCTLx.SPA bits. However there are > quite a few implementation details to be aware of: > > Legacy HDaudio multi-links are powered-up when exiting reset, which > requires the ref_count to be manually set to one when initializing the > link. > > Alternate links for SoundWire/DMIC/SSP need to be explicitly > powered-up before accessing the SHIM/IP/Vendor-Specific SHIM space for > each sublink. DMIC/SSP/SoundWire are all different cases with a > different device/dai/hlink relationship. > > SoundWire will handle power management with the auxiliary device > resume/suspend routine. The ref_count is not necessary in this case. > > The DMIC/SSP will by contrast handle the power management from DAI > .startup and .shutdown callbacks. > > The SSP has a 1:1 mapping between sublink and DAI, but it's > bidirectional so the ref_count will help avoid turning off the sublink > when one of the two directions is still in use. > > The DMIC has a single link but two DAIs for data generated at > different sampling frequencies, again the ref_count will make sure the > two DAIs can be used concurrently. > > And last the SoundWire Intel require power-up/down and bank switch to > be handled with a lock already taken, so the 'eml_lock' is made > optional with the _unlocked versions of the helpers. > > Note that the _check_power_active() implementation is similar to > previous helpers in sound/hda/ext, with sleep duration and timeout > aligned with hardware recommendations. If desired, this helper could > be modified in a second step with .e.g. readl_poll_timeout() > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Rander Wang <rander.wang@intel.com> > Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> > --- > include/sound/hda-mlink.h | 32 +++++++ > sound/soc/sof/intel/hda-mlink.c | 163 ++++++++++++++++++++++++++++++++ > 2 files changed, 195 insertions(+) > ... > diff --git a/sound/soc/sof/intel/hda-mlink.c b/sound/soc/sof/intel/hda-mlink.c > index 90b68ae2564c..4cfef4007d0c 100644 > --- a/sound/soc/sof/intel/hda-mlink.c > +++ b/sound/soc/sof/intel/hda-mlink.c > @@ -170,6 +170,68 @@ static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link, > return 0; > } > > +/* > + * Hardware recommendations are to wait ~10us before checking any hardware transition > + * reported by bits changing status. > + * This value does not need to be super-precise, a slack of 5us is perfectly acceptable. > + * The worst-case is about 1ms before reporting an issue > + */ > +#define HDAML_POLL_DELAY_MIN_US 10 > +#define HDAML_POLL_DELAY_SLACK_US 5 > +#define HDAML_POLL_DELAY_RETRY 100 > + > +static int check_power_active(u32 __iomem *lctl, int sublink, bool enable) Should last argument be named 'active' instead of 'enable'? It would make more sense to me. > +{ > + int mask = BIT(sublink) << AZX_ML_LCTL_CPA_SHIFT; > + int retry = HDAML_POLL_DELAY_RETRY; > + u32 val; > + > + usleep_range(HDAML_POLL_DELAY_MIN_US, > + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); > + do { > + val = readl(lctl); > + if (enable) { > + if (val & mask) > + return 0; > + } else { > + if (!(val & mask)) > + return 0; > + } > + usleep_range(HDAML_POLL_DELAY_MIN_US, > + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); > + > + } while (--retry); > + > + return -EIO; > +} > + ...
>> +static int check_power_active(u32 __iomem *lctl, int sublink, bool >> enable) > > Should last argument be named 'active' instead of 'enable'? It would > make more sense to me. Naming is the hardest part, eh? I am not super happy with 'active', since the 'not-active' part is not very clear: it's not suspended, it's really powered-off/disabled. I also didn't want to introduce a power state, since again it's on or off and we don't want to introduce the Dx concepts here. If I had to revisit this, my preference would be: static int check_sublink_power(u32 __iomem *lctl, int sublink, bool enabled)
On 3/28/2023 3:24 PM, Pierre-Louis Bossart wrote: > > > >>> +static int check_power_active(u32 __iomem *lctl, int sublink, bool >>> enable) >> >> Should last argument be named 'active' instead of 'enable'? It would >> make more sense to me. > > Naming is the hardest part, eh? > I don't disagree ;) > I am not super happy with 'active', since the 'not-active' part is not > very clear: it's not suspended, it's really powered-off/disabled. > > I also didn't want to introduce a power state, since again it's on or > off and we don't want to introduce the Dx concepts here. > > If I had to revisit this, my preference would be: > > static int check_sublink_power(u32 __iomem *lctl, int sublink, bool enabled) Yes that reads better to me.
On Mon, 27 Mar 2023 13:29:21 +0200, Peter Ujfalusi wrote: > > +/* > + * Hardware recommendations are to wait ~10us before checking any hardware transition > + * reported by bits changing status. > + * This value does not need to be super-precise, a slack of 5us is perfectly acceptable. > + * The worst-case is about 1ms before reporting an issue > + */ > +#define HDAML_POLL_DELAY_MIN_US 10 > +#define HDAML_POLL_DELAY_SLACK_US 5 > +#define HDAML_POLL_DELAY_RETRY 100 > + > +static int check_power_active(u32 __iomem *lctl, int sublink, bool enable) > +{ > + int mask = BIT(sublink) << AZX_ML_LCTL_CPA_SHIFT; > + int retry = HDAML_POLL_DELAY_RETRY; > + u32 val; > + > + usleep_range(HDAML_POLL_DELAY_MIN_US, > + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); > + do { > + val = readl(lctl); > + if (enable) { > + if (val & mask) > + return 0; > + } else { > + if (!(val & mask)) > + return 0; > + } > + usleep_range(HDAML_POLL_DELAY_MIN_US, > + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); > + > + } while (--retry); > + > + return -EIO; > +} Can read_poll_timeout() and co be alternative? thanks, Takashi
>> +/* >> + * Hardware recommendations are to wait ~10us before checking any hardware transition >> + * reported by bits changing status. >> + * This value does not need to be super-precise, a slack of 5us is perfectly acceptable. >> + * The worst-case is about 1ms before reporting an issue >> + */ >> +#define HDAML_POLL_DELAY_MIN_US 10 >> +#define HDAML_POLL_DELAY_SLACK_US 5 >> +#define HDAML_POLL_DELAY_RETRY 100 >> + >> +static int check_power_active(u32 __iomem *lctl, int sublink, bool enable) >> +{ >> + int mask = BIT(sublink) << AZX_ML_LCTL_CPA_SHIFT; >> + int retry = HDAML_POLL_DELAY_RETRY; >> + u32 val; >> + >> + usleep_range(HDAML_POLL_DELAY_MIN_US, >> + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); >> + do { >> + val = readl(lctl); >> + if (enable) { >> + if (val & mask) >> + return 0; >> + } else { >> + if (!(val & mask)) >> + return 0; >> + } >> + usleep_range(HDAML_POLL_DELAY_MIN_US, >> + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); >> + >> + } while (--retry); >> + >> + return -EIO; >> +} > > Can read_poll_timeout() and co be alternative? Yes they can. I chose the simple route because I find it clearer, and because we modified the sleep/retries compared to previous implementations. here's what I wrote in the commit message: " Note that the _check_power_active() implementation is similar to previous helpers in sound/hda/ext, with sleep duration and timeout aligned with hardware recommendations. If desired, this helper could be modified in a second step with .e.g. readl_poll_timeout() " If you want to jump directly to the next step that'd be fine. Peter had the same comment, so I'll go with the flow.
diff --git a/include/sound/hda-mlink.h b/include/sound/hda-mlink.h index beef5f509e47..6908af849dd5 100644 --- a/include/sound/hda-mlink.h +++ b/include/sound/hda-mlink.h @@ -12,6 +12,13 @@ struct hdac_bus; int hda_bus_ml_get_capabilities(struct hdac_bus *bus); void hda_bus_ml_free(struct hdac_bus *bus); + +int hdac_bus_eml_power_up(struct hdac_bus *bus, bool alt, int elid, int sublink); +int hdac_bus_eml_power_up_unlocked(struct hdac_bus *bus, bool alt, int elid, int sublink); + +int hdac_bus_eml_power_down(struct hdac_bus *bus, bool alt, int elid, int sublink); +int hdac_bus_eml_power_down_unlocked(struct hdac_bus *bus, bool alt, int elid, int sublink); + 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); @@ -23,6 +30,31 @@ static inline int hda_bus_ml_get_capabilities(struct hdac_bus *bus) { return 0; } static inline void hda_bus_ml_free(struct hdac_bus *bus) { } + +static inline int +hdac_bus_eml_power_up(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return 0; +} + +static inline int +hdac_bus_eml_power_up_unlocked(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return 0; +} + +static inline int +hdac_bus_eml_power_down(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return 0; +} + +static inline int +hdac_bus_eml_power_down_unlocked(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + 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 90b68ae2564c..4cfef4007d0c 100644 --- a/sound/soc/sof/intel/hda-mlink.c +++ b/sound/soc/sof/intel/hda-mlink.c @@ -170,6 +170,68 @@ static int hdaml_lnk_enum(struct device *dev, struct hdac_ext2_link *h2link, return 0; } +/* + * Hardware recommendations are to wait ~10us before checking any hardware transition + * reported by bits changing status. + * This value does not need to be super-precise, a slack of 5us is perfectly acceptable. + * The worst-case is about 1ms before reporting an issue + */ +#define HDAML_POLL_DELAY_MIN_US 10 +#define HDAML_POLL_DELAY_SLACK_US 5 +#define HDAML_POLL_DELAY_RETRY 100 + +static int check_power_active(u32 __iomem *lctl, int sublink, bool enable) +{ + int mask = BIT(sublink) << AZX_ML_LCTL_CPA_SHIFT; + int retry = HDAML_POLL_DELAY_RETRY; + u32 val; + + usleep_range(HDAML_POLL_DELAY_MIN_US, + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); + do { + val = readl(lctl); + if (enable) { + if (val & mask) + return 0; + } else { + if (!(val & mask)) + return 0; + } + usleep_range(HDAML_POLL_DELAY_MIN_US, + HDAML_POLL_DELAY_MIN_US + HDAML_POLL_DELAY_SLACK_US); + + } while (--retry); + + return -EIO; +} + +static int hdaml_link_init(u32 __iomem *lctl, int sublink) +{ + u32 val; + u32 mask = BIT(sublink) << AZX_ML_LCTL_SPA_SHIFT; + + val = readl(lctl); + val |= mask; + + writel(val, lctl); + + return check_power_active(lctl, sublink, true); +} + +static int hdaml_link_shutdown(u32 __iomem *lctl, int sublink) +{ + u32 val; + u32 mask; + + val = readl(lctl); + mask = BIT(sublink) << AZX_ML_LCTL_SPA_SHIFT; + val &= ~mask; + + writel(val, lctl); + + return check_power_active(lctl, sublink, false); +} + /* END HDAML section */ static int hda_ml_alloc_h2link(struct hdac_bus *bus, int index) @@ -251,6 +313,107 @@ void hda_bus_ml_free(struct hdac_bus *bus) } EXPORT_SYMBOL_NS(hda_bus_ml_free, SND_SOC_SOF_HDA_MLINK); +static struct hdac_ext2_link * +find_ext2_link(struct hdac_bus *bus, bool alt, int elid) +{ + struct hdac_ext_link *hlink; + + list_for_each_entry(hlink, &bus->hlink_list, list) { + struct hdac_ext2_link *h2link = hdac_ext_link_to_ext2(hlink); + + if (h2link->alt == alt && h2link->elid == elid) + return h2link; + } + + return NULL; +} + +static int hdac_bus_eml_power_up_base(struct hdac_bus *bus, bool alt, int elid, int sublink, + bool eml_lock) +{ + struct hdac_ext2_link *h2link; + struct hdac_ext_link *hlink; + int ret = 0; + + h2link = find_ext2_link(bus, alt, elid); + if (!h2link) + return -ENODEV; + + if (sublink >= h2link->slcount) + return -EINVAL; + + hlink = &h2link->hext_link; + + if (eml_lock) + mutex_lock(&h2link->eml_lock); + + if (++hlink->ref_count > 1) + goto skip_init; + + ret = hdaml_link_init(hlink->ml_addr + AZX_REG_ML_LCTL, sublink); + +skip_init: + if (eml_lock) + mutex_unlock(&h2link->eml_lock); + + return ret; +} + +int hdac_bus_eml_power_up(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return hdac_bus_eml_power_up_base(bus, alt, elid, sublink, true); +} +EXPORT_SYMBOL_NS(hdac_bus_eml_power_up, SND_SOC_SOF_HDA_MLINK); + +int hdac_bus_eml_power_up_unlocked(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return hdac_bus_eml_power_up_base(bus, alt, elid, sublink, false); +} +EXPORT_SYMBOL_NS(hdac_bus_eml_power_up_unlocked, SND_SOC_SOF_HDA_MLINK); + +static int hdac_bus_eml_power_down_base(struct hdac_bus *bus, bool alt, int elid, int sublink, + bool eml_lock) +{ + struct hdac_ext2_link *h2link; + struct hdac_ext_link *hlink; + int ret = 0; + + h2link = find_ext2_link(bus, alt, elid); + if (!h2link) + return -ENODEV; + + if (sublink >= h2link->slcount) + return -EINVAL; + + hlink = &h2link->hext_link; + + if (eml_lock) + mutex_lock(&h2link->eml_lock); + + if (--hlink->ref_count > 0) + goto skip_shutdown; + + ret = hdaml_link_shutdown(hlink->ml_addr + AZX_REG_ML_LCTL, sublink); + +skip_shutdown: + if (eml_lock) + mutex_unlock(&h2link->eml_lock); + + return ret; +} + +int hdac_bus_eml_power_down(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return hdac_bus_eml_power_down_base(bus, alt, elid, sublink, true); +} +EXPORT_SYMBOL_NS(hdac_bus_eml_power_down, SND_SOC_SOF_HDA_MLINK); + +int hdac_bus_eml_power_down_unlocked(struct hdac_bus *bus, bool alt, int elid, int sublink) +{ + return hdac_bus_eml_power_down_base(bus, alt, elid, sublink, false); +} +EXPORT_SYMBOL_NS(hdac_bus_eml_power_down_unlocked, SND_SOC_SOF_HDA_MLINK); + void hda_bus_ml_put_all(struct hdac_bus *bus) { struct hdac_ext_link *hlink;