[v2,3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

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

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

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the devres cleanup order for ALSA components triggers a kernel crash during module unload or driver unbind.
- [High] This problem wasn't introduced by this patch, but the driver incorrectly handles the 3-byte audio format `SNDRV_PCM_FORMAT_S20_3LE`, causing severe audio corruption.
--

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

ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

This commit reorders the clock enable sequence during runtime resume.
It enables the APB bus clock before the SPDIF module clock to ensure
register accesses do not fail.

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index f54eb14c9ed86..102db1a2afbb8 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but the devres cleanup order for
ALSA components appears to trigger a kernel crash during module unload or
driver unbind.

In sun4i_spdif_probe(), the ALSA component and DMA engine are registered via
devm_snd_soc_register_component() and devm_snd_dmaengine_pcm_register().
The devres teardown runs after the driver's sun4i_spdif_remove() function
returns.

However, sun4i_spdif_remove() explicitly calls pm_runtime_disable() and
sun4i_spdif_runtime_suspend(), which disable the apb_clk bus clock.

When the devres teardown later unregisters the ASoC component, it forces any
active PCM streams to stop, invoking sun4i_spdif_trigger() with
SNDRV_PCM_TRIGGER_STOP. This calls sun4i_snd_txctrl_off(), which writes to
hardware registers via regmap.

Because the APB clock has already been disabled by the remove function, do
these regmap accesses hit unclocked hardware and result in a synchronous
external abort (kernel panic) on sunxi platforms?

[Severity: High]
This is a pre-existing issue, but does the driver incorrectly handle the
3-byte audio format SNDRV_PCM_FORMAT_S20_3LE, causing severe audio
corruption?

The driver advertises support for SNDRV_PCM_FMTBIT_S20_3LE in SUN4I_FORMATS,
which means 20-bit audio samples are tightly packed into exactly 3 bytes in
memory.

However, in sun4i_spdif_hw_params(), the DMA slave bus width
(dma_params_tx.addr_width) is left at DMA_SLAVE_BUSWIDTH_4_BYTES for this
format. This configures the dst_addr_width of the DMA controller to 4 bytes.

In the ALSA generic DMA engine code and sunxi-dma driver, the undefined
src_addr_width defaults to match the 4-byte destination width.

Because the memory buffer contains tightly packed 3-byte samples, does
commanding the DMA controller to perform 4-byte memory reads misalign the
stream immediately (reading part of the next sample on every fetch), leading
to complete data corruption and unusable audio output?

>  
>  	return ret;
>  }
  

On Fri, May 22, 2026 at 12:54 PM <phucduc.bui@gmail.com> wrote:
>
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Enable the APB bus clock before the SPDIF module clock
> during runtime resume, as register accesses depend on the
> bus clock being enabled first.

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

> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index f54eb14c9ed8..102db1a2afbb 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -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
>
  

Hi Chen-yu,

Thanks for your feedback

On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > Enable the APB bus clock before the SPDIF module clock
> > during runtime resume, as register accesses depend on the
> > bus clock being enabled first.
>
> That does not even matter here. Access will only happen once the runtime
> PM callbacks return.
>

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
  

On Sat, May 23, 2026 at 3:11 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> Hi Chen-yu,
>
> Thanks for your feedback
>
> On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > Enable the APB bus clock before the SPDIF module clock
> > > during runtime resume, as register accesses depend on the
> > > bus clock being enabled first.
> >
> > That does not even matter here. Access will only happen once the runtime
> > PM callbacks return.
> >
>
> 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?

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
  

On Sun, May 24, 2026 at 9:41 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Sat, May 23, 2026 at 3:11 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
> >
> > Hi Chen-yu,
> >
> > Thanks for your feedback
> >
> > On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > > Enable the APB bus clock before the SPDIF module clock
> > > > during runtime resume, as register accesses depend on the
> > > > bus clock being enabled first.
> > >
> > > That does not even matter here. Access will only happen once the runtime
> > > PM callbacks return.
> > >
> >
> > 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?
>
> 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.

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

ChenYu
  

Hi Chen-yu,

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

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