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:

quoted

-----Original Message-----
From: Brian Masney <bmasney@redhat.com>
Sent: Friday, July 24, 2026 12:43 AM
To: Joakim Zhang <redacted>
Cc: mturquette@baylibre.com; sboyd@kernel.org; robh@kernel.org;
krzk+dt@kernel.org; conor+dt@kernel.org; p.zabel@pengutronix.de; cix-kernel-
upstream [off-list ref]; linux-clk@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org
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, joakim.zhang@cixtech.com wrote:

quoted

From: Joakim Zhang <redacted>

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 <redacted>

[snip]

quoted

+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.

quoted

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 <bmasney@redhat.com>