[PATCH v2 0/5] ASoC: rockchip: Reorder clock enable sequence

[PATCH v2 0/5] ASoC: rockchip: Reorder clock enable sequence

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all,

This series reorders the runtime resume clock enable sequence in the 
Rockchip SPDIF and PDM drivers to enable the bus clock before the 
functional controller clock.

It also updates the SPDIF DT binding clock descriptions to match the 
actual clock usage in the driver.

Additionally, this v2 adds two new patches addressing issues reported 
by the Sashiko AI Review tool regarding regcache sync failure handling 
and runtime PM resume status validation.

Testing:
  - Patch 1: Verified (dt_binding_check passed).
  - Patches 2 to 5: Compile tested only. Please help test if you have 
    the relevant Rockchip hardware.

Changes in v2:
  - Include two new patches to handle runtime PM resume and regcache sync 
    failures based on Sashiko AI Review.
  - Update commit message based on Krzysztof's review
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.


Best Regards,
Phuc

bui duc phuc (5):
  ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions
  ASoC: rockchip: spdif: Reorder clock enable sequence
  ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence
  ASoC: rockchip: spdif: Restore regcache cache-only mode on sync
    failure
  ASoC: rockchip: rockchip_pdm: Handle runtime PM resume failures in
    set_fmt

 .../bindings/sound/rockchip-spdif.yaml           |  2 +-
 sound/soc/rockchip/rockchip_pdm.c                | 16 ++++++++++------
 sound/soc/rockchip/rockchip_spdif.c              | 11 ++++++-----
 3 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Update the binding descriptions to match the actual clock usage, where
'mclk' is the controller clock and 'hclk' is the bus clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v2:
  - Update commit message based on Krzysztof's review

 Documentation/devicetree/bindings/sound/rockchip-spdif.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
index 502907dd28b3..b174d7498029 100644
--- a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
+++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
@@ -45,8 +45,8 @@ properties:
 
   clocks:
     items:
-      - description: clock for SPDIF bus
       - description: clock for SPDIF controller
+      - description: clock for SPDIF bus
 
   clock-names:
     items:
-- 
2.43.0

[PATCH v2 2/5] ASoC: rockchip: spdif: Reorder clock enable sequence

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Enable the 'hclk' bus clock before the 'mclk' controller clock during
runtime resume.
The bus clock provides the register access interface, so enable it before
the controller clock. This also makes the resume sequence the reverse of
the suspend sequence, which keeps the clock ordering consistent.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.
Changes in v2:
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.

 sound/soc/rockchip/rockchip_spdif.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
index 581624f2682e..8de5b76cfe79 100644
--- a/sound/soc/rockchip/rockchip_spdif.c
+++ b/sound/soc/rockchip/rockchip_spdif.c
@@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
 	struct rk_spdif_dev *spdif = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(spdif->mclk);
+	ret = clk_prepare_enable(spdif->hclk);
 	if (ret) {
-		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
+		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(spdif->hclk);
+	ret = clk_prepare_enable(spdif->mclk);
 	if (ret) {
-		clk_disable_unprepare(spdif->mclk);
-		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
+		clk_disable_unprepare(spdif->hclk);
+		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0

[PATCH v2 3/5] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Enable the 'hclk' bus clock before the 'clk' controller clock during
runtime resume.
The bus clock provides the register access interface, so enable it before
the controller clock. This also makes the resume sequence the reverse of
the suspend sequence, which keeps the clock ordering consistent.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.
Changes in v2:
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.

 sound/soc/rockchip/rockchip_pdm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
index c69cdd6f2499..8f78f7bc1806 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -422,16 +422,16 @@ static int rockchip_pdm_runtime_resume(struct device *dev)
 	struct rk_pdm_dev *pdm = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(pdm->clk);
+	ret = clk_prepare_enable(pdm->hclk);
 	if (ret) {
-		dev_err(pdm->dev, "clock enable failed %d\n", ret);
+		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pdm->hclk);
+	ret = clk_prepare_enable(pdm->clk);
 	if (ret) {
-		clk_disable_unprepare(pdm->clk);
-		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
+		clk_disable_unprepare(pdm->hclk);
+		dev_err(pdm->dev, "clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0

[PATCH v2 4/5] ASoC: rockchip: spdif: Restore regcache cache-only mode on sync failure

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

If regcache_sync() fails during runtime resume, the driver disables the
clocks and returns an error. However, the regmap cache-only mode is left
disabled.
Restore cache-only mode in the error path so subsequent register accesses
continue to use the cache while the device is inactive.

Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260522103713.6C09D1F000E9@smtp.kernel.org/
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.

 sound/soc/rockchip/rockchip_spdif.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
index 8de5b76cfe79..7f15bc7f8f35 100644
--- a/sound/soc/rockchip/rockchip_spdif.c
+++ b/sound/soc/rockchip/rockchip_spdif.c
@@ -94,6 +94,7 @@ static int rk_spdif_runtime_resume(struct device *dev)
 
 	ret = regcache_sync(spdif->regmap);
 	if (ret) {
+		regcache_cache_only(spdif->regmap, true);
 		clk_disable_unprepare(spdif->mclk);
 		clk_disable_unprepare(spdif->hclk);
 	}
-- 
2.43.0

[PATCH v2 5/5] ASoC: rockchip: rockchip_pdm: Handle runtime PM resume failures in set_fmt

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

rockchip_pdm_set_fmt() calls pm_runtime_get_sync() before accessing
hardware registers, but ignores its return value.
If the runtime resume fails, the function continues to perform register
accesses while the device state is undefined.
Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() and
return early on failure to avoid unpowered register accesses.

Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260522110302.349421F000E9@smtp.kernel.org/
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.

 sound/soc/rockchip/rockchip_pdm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
index 8f78f7bc1806..115e90d3bbfe 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -321,6 +321,7 @@ static int rockchip_pdm_set_fmt(struct snd_soc_dai *cpu_dai,
 {
 	struct rk_pdm_dev *pdm = to_info(cpu_dai);
 	unsigned int mask = 0, val = 0;
+	int ret;
 
 	mask = PDM_CKP_MSK;
 	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
@@ -334,7 +335,10 @@ static int rockchip_pdm_set_fmt(struct snd_soc_dai *cpu_dai,
 		return -EINVAL;
 	}
 
-	pm_runtime_get_sync(cpu_dai->dev);
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
+
 	regmap_update_bits(pdm->regmap, PDM_CLK_CTRL, mask, val);
 	pm_runtime_put(cpu_dai->dev);
 
-- 
2.43.0

Re: [PATCH v2 2/5] ASoC: rockchip: spdif: Reorder clock enable sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Reading channel status bytes via a u16 pointer causes unaligned memory accesses and endianness bugs.
- [High] The MCLK domain reset bit is asserted but never de-asserted, which poisons the regmap cache and causes unintended resets on resume.
--

commit d98bc7bb278e4f820ed1d93795c2185544cad12b
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: rockchip: spdif: Reorder clock enable sequence

This commit reorders the clock enable sequence during runtime resume for the
rockchip spdif driver. It enables the hclk bus clock before the mclk
controller clock, keeping the clock ordering consistent with the suspend
sequence.

> diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
> index 581624f2682ef..8de5b76cfe791 100644
> --- a/sound/soc/rockchip/rockchip_spdif.c
> +++ b/sound/soc/rockchip/rockchip_spdif.c
> @@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
[ ... ]
> -	ret = clk_prepare_enable(spdif->hclk);
> +	ret = clk_prepare_enable(spdif->mclk);
>  	if (ret) {
> -		clk_disable_unprepare(spdif->mclk);
> -		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
> +		clk_disable_unprepare(spdif->hclk);
> +		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
>  		return ret;
>  	}
>  
>  	regcache_cache_only(spdif->regmap, false);
>  	regcache_mark_dirty(spdif->regmap);
>  
>  	ret = regcache_sync(spdif->regmap);

[Severity: High]
This is a pre-existing issue, but does this regcache_sync() call cause
unintended resets due to a poisoned regmap cache?

In rk_spdif_hw_params(), the driver sets the SPDIF_CFGR_CLR_EN bit to clear
the MCLK domain logic:

sound/soc/rockchip/rockchip_spdif.c:rk_spdif_hw_params()
	...
	regmap_update_bits(spdif->regmap, SPDIF_CFGR, SPDIF_CFGR_CLR_MASK,
			   SPDIF_CFGR_CLR_EN);
	udelay(1);

	ret = regmap_update_bits(spdif->regmap, SPDIF_CFGR,
	...

This bit is never explicitly de-asserted.

Because SPDIF_CFGR is a cached register, the cache permanently stores the
value with the clear bit set.

During pm_runtime_resume(), does writing this cached value back to the
hardware unintentionally re-trigger the MCLK domain reset and corrupt the
hardware state?


Also, looking elsewhere in the same file, there is another pre-existing issue
in rk_spdif_hw_params():

sound/soc/rockchip/rockchip_spdif.c:rk_spdif_hw_params()
	...
	ret = snd_pcm_create_iec958_consumer_hw_params(params, cs, sizeof(cs));
	if (ret < 0)
		return ret;

	fc = (u16 *)cs;
	for (i = 0; i < CS_BYTE / 2; i++)
		regmap_write(spdif->regmap, SPDIF_CHNSRn(i), CS_FRAME(fc[i]));
	...

[Severity: High]
This isn't a bug introduced by this patch, but does casting the u8 array to a
u16 pointer cause unaligned memory accesses and endianness bugs?

The 6-byte cs array is cast to a u16 pointer and directly dereferenced.

Can this trigger alignment faults or undefined behavior depending on the
architecture and compiler optimizations?

Additionally, since it performs a host-endian read of the byte stream, does
this cause the IEC958 channel status bits to be incorrectly swapped when
running on big-endian architectures?

Could get_unaligned_le16() be used here instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602101608.45137-1-phucduc.bui@gmail.com?part=2

Re: [PATCH v2 3/5] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If `get_pdm_clk()` falls back to the signoff clock rate, it leaves `clk_out` at 0, resulting in a division by zero panic in `rockchip_pdm_hw_params()`.
--

commit d4ba65106b95a91e9e92f74421222069efac6183
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

Enable the 'hclk' bus clock before the 'clk' controller clock during
runtime resume. This ensures the bus clock is available for register access
before the controller is clocked, keeping the resume sequence symmetrical
with suspend.

> diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
> index c69cdd6f24994..8f78f7bc1806c 100644
> --- a/sound/soc/rockchip/rockchip_pdm.c
> +++ b/sound/soc/rockchip/rockchip_pdm.c

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential division by zero
in this file when the clock tree cannot synthesize an exact frequency match.

In get_pdm_clk(), if the code falls back to the signoff clock rate, it
updates *clk_src but appears to leave *clk_out at 0:

sound/soc/rockchip/rockchip_pdm.c:get_pdm_clk() {
...
	if (!clk) {
		clk = clk_round_rate(pdm->clk, PDM_SIGNOFF_CLK_RATE);
		*clk_src = clk;
	}
...
}

This 0 value is then passed into rational_best_approximation() as the
given_numerator, which evaluates it and sets m = 0. Later in
rockchip_pdm_hw_params(), the driver divides by m:

sound/soc/rockchip/rockchip_pdm.c:rockchip_pdm_hw_params() {
...
	rational_best_approximation(clk_out, clk_src,
				    GENMASK(16 - 1, 0),
				    GENMASK(16 - 1, 0),
				    &m, &n);
...
	clk_div = n / m;
...
}

Could an unprivileged userspace application requesting a standard audio
format trigger this division by zero and panic the kernel? Should *clk_out
be updated in the fallback path, or should rockchip_pdm_hw_params() check
for m == 0 before dividing?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602101608.45137-1-phucduc.bui@gmail.com?part=3