Re: [PATCH v11 2/4] clk: cix: add sky1 audss clock controller

Hi Joakim,

On Fri, Jul 24, 2026 at 06:14:29AM +0000, Joakim  Zhang wrote:
> > -----Original Message-----
> > From: Brian Masney <[email protected]>
> > Sent: Friday, July 24, 2026 12:43 AM
> > To: Joakim Zhang <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; cix-kernel-
> > upstream <[email protected]>; [email protected];
> > [email protected]; [email protected]; linux-arm-
> > [email protected]
> > Subject: Re: [PATCH v11 2/4] clk: cix: add sky1 audss clock controller
> > 
> > EXTERNAL EMAIL
> > 
> > Hi Joakim,
> > 
> > There's one question from Sashiko that I don't see where you answered that seems
> > to be legit.
> > 
> > On Thu, Jul 23, 2026 at 05:08:06PM +0800, [email protected] wrote:
> > > From: Joakim Zhang <[email protected]>
> > >
> > > Add a platform driver for the Cix Sky1 AUDSS CRU. The driver maps the
> > > CRU registers and registers mux, divider and gate clocks for DSP,
> > > SRAM, HDA, DMAC, I2S, mailbox, watchdog and timer blocks.
> > >
> > > Four SoC-level audio reference clocks are enabled as inputs to the
> > > internal clock tree. The driver releases the AUDSS NOC reset, enables
> > > runtime PM and instantiates the auxiliary reset device.
> > >
> > > Signed-off-by: Joakim Zhang <[email protected]>
> > 
> > [snip]
> > 
> > > +static int __maybe_unused sky1_audss_clk_runtime_resume(struct device
> > > +*dev) {
> > > +     struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
> > > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> > >devtype_data;
> > > +     unsigned long flags;
> > > +     int i, ret;
> > > +
> > > +     ret = sky1_audss_clks_enable(priv);
> > 
> > From Sashiko:
> > 
> >     [Severity: High]
> >     Could this result in a deadlock? When a consumer calls clk_prepare on an
> >     audss clock, CCF acquires the global prepare_lock mutex and may call
> >     pm_runtime_resume_and_get. This triggers sky1_audss_clk_runtime_resume,
> >     which calls sky1_audss_clks_enable. Since sky1_audss_clks_enable executes
> >     clk_prepare_enable, it will attempt to acquire the non-recursive prepare_lock
> >     again.
> 
> prepare_lock is re-entrant for the same task (see clk_prepare_lock(): if prepare_owner == current it just bumps prepare_refcnt). So calling clk_prepare_enable() on the parent SCMI clocks from runtime_resume(), while already in clk_prepare() for an AUDSS clock, does not deadlock. This matches how CCF recursively prepares parents under the same lock.
> 
> That safety assumes resume of the AUDSS CRU is driven from CCF while the same task already owns prepare_lock. If another device device-links to the AUDSS CRU (or otherwise runtime resumes it outside clk_prepare), a concurrent clk_prepare() can deadlock:
>   Thread A: holds prepare_lock, waits in pm_runtime for our resume
>   Thread B: in our runtime_resume, clk_prepare_enable() waits for prepare_lock
> 
> We do not have such a device-link usage today. Would you prefer we add a short comment near sky1_audss_clk_runtime_resume() / sky1_audss_clks_enable() documenting that consumers should use these clocks via CCF and should not PM-resume this device through a device link (or other non-CCF path) that can race with clk_prepare? Happy to add that in the next version if useful.
> 
> > I found drivers/clk/samsung/clk-exynos-audss.c that is similar to your driver and it
> > calls clk_prepare_enable() in probe.
> 
> Exynos AUDSS is only partially similar. It clk_prepare_enable()s EPLL in probe and keeps it enabled for the lifetime of the driver; its runtime PM callbacks only save/restore AUDSS registers and never disable that parent.
> 
> We cannot follow that pattern. Our parent clocks come from SCMI and also gate the audio subsystem power domain. If we clk_prepare_enable() them in probe and leave them enabled, their enable counts stay non-zero, so after system suspend (and on runtime idle) those SCMI clocks cannot be disabled and the audio SS power domain stays on.
> 
> That is why we enable the parent SCMI clocks in runtime_resume() and disable them in runtime_suspend(), instead of keeping a permanent prepare/enable from probe like Exynos.

That makes sense and thanks for the clarification.

Reviewed-by: Brian Masney <[email protected]>