ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

From: bui duc phuc <[email protected]>

Hi,

This series contains a few improvements for the sun4i-spdif driver, including guard() conversions and ensuring the device is resumed via runtime PM before kcontrol register accesses.

Link v1:
https://lore.kernel.org/all/[email protected]/

Change in v2:
- Switched from using guard() to scoped_guard()
- Added runtime PM handling for kcontrol register accesses.

Best Regards,
Phuc

bui duc phuc (3):
ASoC: sunxi: sun4i-spdif: Use guard() for spin locks
ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register
access
ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

 sound/soc/sunxi/sun4i-spdif.c | 76 +++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 34 deletions(-)

2.43.0

From: bui duc phuc <[email protected]>

Clean up the code using guard() for spin locks.
Merely code refactoring, and no behavior change.

---

Change in v2:
- Switched from using guard() to scoped_guard()

 sound/soc/sunxi/sun4i-spdif.c | 62 ++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

index c2ec19437cd7..89eccc83a130 100644
@@ -427,24 +427,21 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
-	unsigned long flags;
 	unsigned int reg;
 
-	spin_lock_irqsave(&host->lock, flags);
+	scoped_guard(spinlock_irqsave, &host->lock) {
+		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
 
-	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
+		status[0] = reg & 0xff;
+		status[1] = (reg >> 8) & 0xff;
+		status[2] = (reg >> 16) & 0xff;
+		status[3] = (reg >> 24) & 0xff;
 
-	status[0] = reg & 0xff;
-	status[1] = (reg >> 8) & 0xff;
-	status[2] = (reg >> 16) & 0xff;
-	status[3] = (reg >> 24) & 0xff;
+		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA1, &reg);
 
-	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA1, &reg);
-
-	status[4] = reg & 0xff;
-	status[5] = (reg >> 8) & 0x3;
-
-	spin_unlock_irqrestore(&host->lock, flags);
+		status[4] = reg & 0xff;
+		status[5] = (reg >> 8) & 0x3;
+	}
 
 	return 0;
 }

@@ -455,35 +452,32 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
-	unsigned long flags;
 	unsigned int reg;
 	bool chg0, chg1;
 
-	spin_lock_irqsave(&host->lock, flags);
-
-	reg = (u32)status[3] << 24;
-	reg |= (u32)status[2] << 16;
-	reg |= (u32)status[1] << 8;
-	reg |= (u32)status[0];
+	scoped_guard(spinlock_irqsave, &host->lock) {
+		reg = (u32)status[3] << 24;
+		reg |= (u32)status[2] << 16;
+		reg |= (u32)status[1] << 8;
+		reg |= (u32)status[0];
 
-	regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA0,
-				 GENMASK(31,0), reg, &chg0);
+		regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA0,
+					 GENMASK(31, 0), reg, &chg0);
 
-	reg = (u32)status[5] << 8;
-	reg |= (u32)status[4];
+		reg = (u32)status[5] << 8;
+		reg |= (u32)status[4];
 
-	regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA1,
-				 GENMASK(9,0), reg, &chg1);
+		regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA1,
+					 GENMASK(9, 0), reg, &chg1);
 
-	reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
-	if (status[0] & IEC958_AES0_NONAUDIO)
-		reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
+		reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
+		if (status[0] & IEC958_AES0_NONAUDIO)
+			reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
 
-	regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
-			   SUN4I_SPDIF_TXCFG_CHSTMODE |
-			   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
-
-	spin_unlock_irqrestore(&host->lock, flags);
+		regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
+				   SUN4I_SPDIF_TXCFG_CHSTMODE |
+				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
+	}
 
 	return chg0 || chg1;
 }

2.43.0

From: bui duc phuc <[email protected]>

Accessing registers while the device is runtime-suspended may lead to invalid hardware accesses on systems where the APB bus clock is gated during runtime suspend.

Ensure the device is resumed before accessing registers.

---
 sound/soc/sunxi/sun4i-spdif.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 89eccc83a130..f54eb14c9ed8 100644
@@ -428,6 +428,11 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
 	unsigned int reg;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
 
 	scoped_guard(spinlock_irqsave, &host->lock) {
 		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

@@ -443,6 +448,8 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 		status[5] = (reg >> 8) & 0x3;
 	}
 
+	pm_runtime_put(cpu_dai->dev);
+
 	return 0;
 }
 

@@ -453,8 +460,13 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
 	unsigned int reg;
+	int ret;
 	bool chg0, chg1;
 
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
+
 	scoped_guard(spinlock_irqsave, &host->lock) {
 		reg = (u32)status[3] << 24;
 		reg |= (u32)status[2] << 16;

@@ -479,6 +491,8 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
 	}
 
+	pm_runtime_put(cpu_dai->dev);
+
 	return chg0 || chg1;
 }
 

2.43.0

Did you actually reproduce the issue, or did you add the patch simply because Sashiko mentioned it?

On sunxi, either it will hang the system because the bus transaction got ignored, or it won't as something else enabled the clock.

And when you do add patches due to Sashiko raising an issue, please do mention it in the commit message.

Hi Chen-Yu,

As mentioned in the v1 discussion , this issue was originally reported by Sashiko.
I'll add the Reported-by tag in the next revision.
v1 links:
https://lore.kernel.org/all/[email protected]/

Since I lack Sunxi hardware, I couldn't reproduce it or perform runtime testing. But I did compile-test the patch.
The patch aims to fix unsafe register accesses that occur before ensuring the device is runtime-resumed.

If Sunxi's PM design already guarantees safe access here, feel free to reject the patch.

Best Regards,
Phuc

When you submit a patch, it is expected that you already tested it. If you only compile tested it, please remember to say so in the footer (or mark the patch as RFT) so that others can test for you and the maintainer knows the status.

And if possible, provide a scheme to test it.

I can't say that it does. But since the only control that SPDIF gives is the IEC958 status, and that doesn't appear in the standard mixer apps, it's unlikely that a _user_ will trigger it. Plus the control was added after the basic structure of the driver was done, so there is definitely some possibility of a crash.

But what you wrote in the commit message doesn't match the actual hardware behavior, like I wrote.

ChenYu

Hi Chenyu,

Thanks for the guidance.
I’ll clearly mention the test status next time.

Thanks for the clarification.
I'll update the commit message to something like:
"The kcontrols may access hardware registers while the device is runtime-suspended.
Ensure the device is resumed before touching the registers."

Best Regards,
Phuc

Hi ,

I did some additional investigation into the regmap core functions currently used by this driver, such as _regmap_update_bits():

https://elixir.bootlin.com/linux/v7.1-rc5/source/drivers/base/regmap/regmap.c#L3249

and _regmap_read():
https://elixir.bootlin.com/linux/v7.1-rc5/source/drivers/base/regmap/regmap.c#L2822

as well as the current sun4i_spdif_regmap_config definition:
https://elixir.bootlin.com/linux/v7.1-rc5/source/sound/soc/sunxi/sun4i-spdif.c#L526

Based on the current regmap core implementation, I think it might be cleaner to leverage the existing regmap suspend protection mechanism:

if (map->cache_only)
return -EBUSY;

This would inherently prevent direct hardware register access and help avoid potential crashes during suspend. To support this properly, we would likely also need to enable a basic cache type (e.g. REGCACHE_FLAT) in sun4i_spdif_regmap_config, since the driver currently
does not define a .cache_type.

A patch in this direction would look roughly like this:

static int sun4i_spdif_runtime_suspend(struct device *dev) {
struct sun4i_spdif_dev *host = dev_get_drvdata(dev);

clk_disable_unprepare(host->spdif_clk);
+regcache_cache_only(host->regmap, true);
clk_disable_unprepare(host->apb_clk);

}

static int sun4i_spdif_runtime_resume(struct device *dev) {
struct sun4i_spdif_dev *host = dev_get_drvdata(dev);

clk_prepare_enable(host->apb_clk);
+ regcache_cache_only(host->regmap, false);

clk_prepare_enable(host->spdif_clk);
}

This approach would not only address the current kcontrol-related issue, but could also provide a more generic safeguard against other runtime suspend race conditions that we may not have considered yet.

However, one downside is that this solution depends on the current internal behavior of the regmap core. In addition, if this driver starts using .volatile_reg in the future, the current regmap implementation may no longer provide sufficient protection during suspend.

For example, in the first branch of _regmap_update_bits(), volatile register accesses bypass the cache_only check entirely and can still hit the hardware bus directly, which could still lead to system crashes.

Because of that, the explicit pm_runtime resume handling proposed in the current patch may still be the more robust solution in the long term.

Given our previous discussion about making the pm_runtime handling more consistent between I2S and SPDIF:

https://lore.kernel.org/all/CAABR9nEFGOX5GnQ9qpJY-T-92dA_kDcVS+qBz1ior590G_x6gw@mail.gmail.com/

What are your thoughts on this direction?
Or would it be safer and simpler to keep the current patch as-is?

Best Regards,
Phuc

From: bui duc phuc <[email protected]>

Enable the APB bus clock before the SPDIF module clock during runtime resume, as register accesses depend on the bus clock being enabled first.

---
 sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index f54eb14c9ed8..102db1a2afbb 100644
@@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
 
 static int sun4i_spdif_runtime_resume(struct device *dev)
 {
-	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
+	struct sun4i_spdif_dev *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(host->spdif_clk);
+	ret = clk_prepare_enable(host->apb_clk);
 	if (ret)
 		return ret;
-	ret = clk_prepare_enable(host->apb_clk);
+	ret = clk_prepare_enable(host->spdif_clk);
 	if (ret)
-		clk_disable_unprepare(host->spdif_clk);
+		clk_disable_unprepare(host->apb_clk);
 
 	return ret;
 }

2.43.0

That does not even matter here. Access will only happen once the runtime PM callbacks return.

Hi Chen-yu,

Thanks for your feedback

I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access registers within the current ⁠runtime_resume⁠ path, so the order might not trigger a failure right now.

However, if we look at the peer driver for the same Sunxi SoC family, ⁠sun4i-i2s.c⁠:
Links:
https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:

1. Enable bus clock
2. Access and restore/sync I2S registers
3. Enable module clock

Since both IP blocks belong to the same Sunxi platform and share similar bus/module clock relationships, shouldn't we maintain architectural consistency across these drivers?

Enforcing the "bus clock before module clock" order keeps the dependency ordering aligned with the actual hardware roles, where the bus clock is required for register access while the module clock drives the functional audio path.

Wouldn't keeping this order also make the runtime PM behavior more consistent and easier to follow across the Sunxi audio drivers?

Best Regards,
Phuc

This should be your primary motivation for the patch, i.e. what you put in the commit message as the reason for this patch. What you currently have doesn't make sense, as I already mentioned.

Some background though, sunxi is done mostly by volunteers, so we're not overly concerned with rigidness or aligning different drivers, unless they share a common library, such as all the clk or pinctrl drivers.

ChenYu

Oh, and you could also add that the resume order should (normally) be the reverse of the suspend order.

ChenYu

Hi Chen-yu,

Thank you for the feedback and clarification.

I can certainly rewrite the commit message around the idea that the resume ordering should normally mirror the suspend ordering.

However, I am still slightly unsure about which ordering should be considered the preferred one in this case. If the reasoning is purely based on suspend/resume symmetry, I wonder if changing the suspend path itself to disable the APB clock before the SPDIF module clock would be an option, which would then naturally lead to enabling the SPDIF module clock first during resume.

From the hardware perspective, I originally assumed the APB clock acts as the prerequisite for register access while the module clock mainly drives the functional audio logic, which is why the "bus before module" ordering initially seemed more intuitive to me.

So I would like to better understand which ordering is considered the preferred model for the Sunxi audio blocks here: preserving the existing suspend/resume symmetry, or enforcing the bus/module dependency ordering.

Thanks,
Phuc