Hi Morimoto-san,
On Tue, Dec 17, 2024 at 3:38 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> > ------------[ cut here ]------------
> > clk_multiplier already disabled
> (snip)
> > ------------[ cut here ]------------
> > clk_multiplier already unprepared
> (snip)
> > Unfortunately I cannot reproduce it at will.
> > The above is from today's renesas-devel release, but my logs indicate
> > it happens every few months since at least v6.1.
> > So far I have seen it on all Salvator-X(S) variants, but not on any other
> > SoCs or boards.
>
> Hmm... I'm not sure why, but according to the log, it seems it calls
> clk_disable_unprepare() without calling clk_prepare_enable().
> I think "clk_multiplier" means "cs2000" on Salvator-X(S).
>
> Basically, I don't think it can be happen. But current rsnd driver doesn't
> check return value of clk_prepare_enable(). So if cs2000 failed clk_enable()
> for some reasons, indeed clk_disable_unprepare() might be called without
> enabled (It is another issue, though...)
That sounds plausible...
> I'm not tesed, but can this patch improve situation ?
>
> If above assumption was correct, the clk WARNING issue itself can be solved,
> but sound driver itself will fail to probe...
I'm adding your patch to my local tree.
Let's see what happens during the next few months...
> ------------------
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Date: Tue, 17 Dec 2024 11:30:46 +0900
> Subject: [PATCH] ASoC: rsnd: check rsnd_adg_clk_enable() return value
>
> rsnd_adg_clk_enable() might be failed for some reasons. In such case,
> we will get WARNING from clk.c when suspend was used that it try to
> disable clk without enabled. Check rsnd_adg_clk_enable() return value.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> sound/soc/renesas/rcar/adg.c | 30 ++++++++++++++++++++++++------
> sound/soc/renesas/rcar/core.c | 4 +---
> sound/soc/renesas/rcar/rsnd.h | 2 +-
> 3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/renesas/rcar/adg.c b/sound/soc/renesas/rcar/adg.c
> index 0f190abf00e75..723dcc88af306 100644
> --- a/sound/soc/renesas/rcar/adg.c
> +++ b/sound/soc/renesas/rcar/adg.c
> @@ -389,18 +389,33 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
>
> for_each_rsnd_clkin(clk, adg, i) {
> if (enable) {
> - clk_prepare_enable(clk);
> + ret = clk_prepare_enable(clk);
>
> /*
> * We shouldn't use clk_get_rate() under
> * atomic context. Let's keep it when
> * rsnd_adg_clk_enable() was called
> */
> - adg->clkin_rate[i] = clk_get_rate(clk);
> + if (ret < 0)
> + break;
> + else
No need for the else.
> + adg->clkin_rate[i] = clk_get_rate(clk);
> } else {
> - clk_disable_unprepare(clk);
> + if (adg->clkin_rate[i])
> + clk_disable_unprepare(clk);
> +
> + adg->clkin_rate[i] = 0;
> }
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
