Message-ID: <CAMuHMdX1gPNCFddg_DyK7Bv0BeFLOLi=5eteT_HhMH=Ph2wVvA@mail.gmail.com>
Date: Fri, 3 Jan 2025 18:06:05 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to
 per-bit basis

Hi Prabhakar,

On Thu, Jan 2, 2025 at 7:18 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Switch MSTOP handling from group-based to per-bit configuration to
> address issues with shared dependencies between module clocks. In the
> current group-based configuration, multiple module clocks may rely on
> a single MSTOP bit. When both clocks are turned ON and one is
> subsequently turned OFF, the shared MSTOP bit will still be set, which
> is incorrect since the other dependent module clock remains ON. By
> switching to a per-bit configuration, we ensure precise control over
> individual MSTOP bits, preventing such conflicts.
>
> Replace the refcount API with atomic operations for managing MSTOP bit
> counters. The refcount API requires explicitly setting the counter to
> `1` before calling `refcount_inc()`, which introduces potential edge
> cases and unnecessary complexity. Using atomic operations simplifies
> the logic and avoids such issues, resulting in cleaner and more
> maintainable code.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> v2->v3
> - Dropped unnecessary parentheses
> - Switched using to devm_kcalloc() instead of devm_kmalloc_array()
> - Optimized check in rzv2h_mod_clock_endisable() if the states are same
> - Dropped GET_MSTOP_IDX() macro and handled indexing in the code
> - Made mstop_mask to unsigned long to avoid casting

Thanks for the update!

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -446,38 +443,70 @@ rzv2h_cpg_register_core_clk(const struct cpg_core_clk *core,
>  }
>
>  static void rzv2h_mod_clock_mstop_enable(struct rzv2h_cpg_priv *priv,
> -                                        struct mod_clock *clock)
> +                                        u32 mstop_data)
>  {
> +       unsigned long mstop_mask = FIELD_GET(BUS_MSTOP_BITS_MASK, mstop_data);
> +       u16 mstop_index = FIELD_GET(BUS_MSTOP_IDX_MASK, mstop_data);
> +       unsigned int index = (mstop_index - 1) * 16;

Probably I didn't explain it well, but you could avoid the "- 1"
here and in all functions accessing priv->mstop_count, by adjusting
priv->mstop_count in rzv2h_cpg_probe()[1].

> +       atomic_t *mstop = &priv->mstop_count[index];
>         unsigned long flags;
> -       u32 val;
> +       unsigned int i;
> +       u32 val = 0;
>
>         spin_lock_irqsave(&priv->rmw_lock, flags);
> -       if (!refcount_read(&clock->mstop->ref_cnt)) {
> -               val = clock->mstop->mask << 16;
> -               writel(val, priv->base + CPG_BUS_MSTOP(clock->mstop->idx));
> -               refcount_set(&clock->mstop->ref_cnt, 1);
> -       } else {
> -               refcount_inc(&clock->mstop->ref_cnt);
> +       for_each_set_bit(i, &mstop_mask, 16) {
> +               if (!atomic_read(&mstop[i]))
> +                       val |= BIT(i) << 16;
> +               atomic_inc(&mstop[i]);
>         }
> +       if (val)
> +               writel(val, priv->base + CPG_BUS_MSTOP(mstop_index));
>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
>  }

> @@ -647,12 +619,39 @@ rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
>
>         priv->clks[id] = clock->hw.clk;
>
> -       if (mod->mstop_data != BUS_MSTOP_NONE) {
> -               clock->mstop = rzv2h_cpg_get_mstop(priv, clock, mod->mstop_data);
> -               if (!clock->mstop) {
> -                       clk = ERR_PTR(-ENOMEM);
> -                       goto fail;
> +       /*
> +        * Ensure the module clocks and MSTOP bits are synchronized when they are
> +        * turned ON by the bootloader. Enable MSTOP bits for module clocks that were
> +        * turned ON in an earlier boot stage.
> +        */
> +       if (clock->mstop_data != BUS_MSTOP_NONE &&
> +           !mod->critical && rzv2h_mod_clock_is_enabled(&clock->hw)) {
> +               rzv2h_mod_clock_mstop_enable(priv, clock->mstop_data);
> +       } else if (clock->mstop_data != BUS_MSTOP_NONE && mod->critical) {
> +               unsigned long mstop_mask = FIELD_GET(BUS_MSTOP_BITS_MASK, clock->mstop_data);
> +               u16 mstop_index = FIELD_GET(BUS_MSTOP_IDX_MASK, clock->mstop_data);
> +               unsigned int index = (mstop_index - 1) * 16;
> +               atomic_t *mstop = &priv->mstop_count[index];
> +               unsigned long flags;
> +               unsigned int i;
> +               u32 val = 0;
> +
> +               /*
> +                * Critical clocks are turned ON immediately upon registration, and the
> +                * MSTOP counter is updated through the rzv2h_mod_clock_enable() path.
> +                * However, if the critical clocks were already turned ON by the initial
> +                * bootloader, synchronize the atomic counter here and clear the MSTOP bit.
> +                */
> +               spin_lock_irqsave(&priv->rmw_lock, flags);
> +               for_each_set_bit(i, &mstop_mask, 16) {
> +                       if (atomic_read(&mstop[i]))
> +                               continue;
> +                       val |= BIT(i) << 16;
> +                       atomic_inc(&mstop[i]);
>                 }
> +               if (val)
> +                       writel(val, priv->base + CPG_BUS_MSTOP(mstop_index));
> +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
>         }

This logic could be simplified to:

    if (clock->mstop_data == BUS_MSTOP_NONE)
            return;

    if (mod->critical) {
            unsigned long mstop_mask = FIELD_GET(BUS_MSTOP_BITS_MASK,
clock->mstop_data);
            ...
    } else if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
            rzv2h_mod_clock_mstop_enable(priv, clock->mstop_data);
    }

>
>         return;
> @@ -922,6 +921,11 @@ static int __init rzv2h_cpg_probe(struct platform_device *pdev)
>         if (!clks)
>                 return -ENOMEM;
>
> +       priv->mstop_count = devm_kcalloc(dev, info->num_mstop_bits,
> +                                        sizeof(*priv->mstop_count), GFP_KERNEL);
> +       if (!priv->mstop_count)
> +               return -ENOMEM;

[1]

    /* Adjust for CPG_BUS_m_MSTOP starting from m = 1 */
    priv->mstop_count -= 16;

Anyway, it's getting late in the cycle, so I am queuing this tentatively
in renesas-clk for v6.14, to allow the bots to give it a run in this
Monday's linux-next. I will still have to squash the fixes
(+ whatever minor updates?) into the original bad commit (adding
Co-Developed-by-tags for you and Biju), but that can be done later,
just before sending my final pull requests for the v6.14 merge window...

Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

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

Powered by blists - more mailing lists