Re: [PATCH v2 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Dec 23, 2024 at 6:37 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Refactor MSTOP handling to switch from group-based to per-bit
> configuration. Introduce atomic counters for each MSTOP bit and update
> enable/disable logic.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a09g047-cpg.c
> +++ b/drivers/clk/renesas/r9a09g047-cpg.c
> @@ -145,4 +145,6 @@ const struct rzv2h_cpg_info r9a09g047_cpg_info __initconst = {
>         /* Resets */
>         .resets = r9a09g047_resets,
>         .num_resets = ARRAY_SIZE(r9a09g047_resets),
> +
> +       .num_mstop_bits = 208,

Note to self: to be folded into commit 61302a455696728c ("clk: renesas:
rzv2h: Add support for RZ/G3E SoC") in renesas-clk.

>  };
> diff --git a/drivers/clk/renesas/r9a09g057-cpg.c b/drivers/clk/renesas/r9a09g057-cpg.c
> index 59dadedb2217..a45b4020996b 100644
> --- a/drivers/clk/renesas/r9a09g057-cpg.c
> +++ b/drivers/clk/renesas/r9a09g057-cpg.c
> @@ -275,4 +275,6 @@ const struct rzv2h_cpg_info r9a09g057_cpg_info __initconst = {
>         /* Resets */
>         .resets = r9a09g057_resets,
>         .num_resets = ARRAY_SIZE(r9a09g057_resets),
> +
> +       .num_mstop_bits = 192,

Note to self: to be folded into commit 7bd4cb3d6b7c43f0 ("clk:
renesas: rzv2h: Add MSTOP support") in renesas-clk, just like the
rest below.

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -43,6 +43,8 @@
>
>  #define CPG_BUS_1_MSTOP                (0xd00)
>  #define CPG_BUS_MSTOP(m)       (CPG_BUS_1_MSTOP + ((m) - 1) * 4)
> +/* On RZ/V2H(P) and RZ/G3E CPG_BUS_m_MSTOP starts from m = 1 */

If you think you need this comment, please move it two lines up,
as it also applies to those lines.

> +#define GET_MSTOP_IDX(mask)    ((FIELD_GET(BUS_MSTOP_IDX_MASK, (mask))) - 1)

I think subtracting one here is the wrong abstraction (see below)...

>
>  #define KDIV(val)              ((s16)FIELD_GET(GENMASK(31, 16), (val)))
>  #define MDIV(val)              FIELD_GET(GENMASK(15, 6), (val))
> @@ -68,6 +70,7 @@
>   * @resets: Array of resets
>   * @num_resets: Number of Module Resets in info->resets[]
>   * @last_dt_core_clk: ID of the last Core Clock exported to DT
> + * @mstop_count: Array of mstop

Array of mstop values?

>   * @rcdev: Reset controller entity
>   */
>  struct rzv2h_cpg_priv {
> @@ -82,17 +85,13 @@ struct rzv2h_cpg_priv {
>         unsigned int num_resets;
>         unsigned int last_dt_core_clk;
>
> +       atomic_t *mstop_count;
> +
>         struct reset_controller_dev rcdev;
>  };

> @@ -446,36 +445,65 @@ 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)
>  {
> +       u16 mstop_mask = FIELD_GET(BUS_MSTOP_BITS_MASK, (mstop_data));

No need for parentheses around mstop_data.

> +       u16 mstop_index = GET_MSTOP_IDX(mstop_data);
> +       unsigned int index = mstop_index * 16;

mstop_index already has one subtracted inside GET_MSTOP_IDX(),
because you need that for indexing priv->mstop_count[]...

>         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, (unsigned long *)&mstop_mask, 16) {

Please make mstop_mask unsigned long instead of using a
non-portable cast.

> +               if (!atomic_read(&priv->mstop_count[index + i]))
> +                       val |= BIT(i) << 16;
> +               atomic_inc(&priv->mstop_count[index + i]);
>         }
> +       if (val)
> +               writel(val, priv->base + CPG_BUS_MSTOP(mstop_index + 1));

... hence you have to re-add one here, which will be subtracted again
inside CPG_BUS_MSTOP().

So what about:
  1. Dropping macro GET_MSTOP_IDX(),
  2. Using mstop_index = FIELD_GET(BUS_MSTOP_BITS_MASK, mstop_data),
     so you can call CPG_BUS_MSTOP(mstop_index) directly,
  3. Letting priv->mstop_count point 16 entries before the allocated
     array, so you can index it by the logical mstop number directly.


>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
>  }
>
>  static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
> -                                         struct mod_clock *clock)
> +                                         u32 mstop_data)
>  {
> +       u16 mstop_mask = FIELD_GET(BUS_MSTOP_BITS_MASK, (mstop_data));
> +       u16 mstop_index = GET_MSTOP_IDX(mstop_data);
> +       unsigned int index = mstop_index * 16;
>         unsigned long flags;
> -       u32 val;
> +       unsigned int i;
> +       u32 val = 0;
>
>         spin_lock_irqsave(&priv->rmw_lock, flags);
> -       if (refcount_dec_and_test(&clock->mstop->ref_cnt)) {
> -               val = clock->mstop->mask << 16 | clock->mstop->mask;
> -               writel(val, priv->base + CPG_BUS_MSTOP(clock->mstop->idx));
> +       for_each_set_bit(i, (unsigned long *)&mstop_mask, 16) {
> +               if (!atomic_read(&priv->mstop_count[index + i]) ||
> +                   atomic_dec_and_test(&priv->mstop_count[index + i]))

Why the first part of the check?
Because you only enable, and never disable, mstop bits in the initial
synchronization in rzv2h_cpg_register_mod_clk()?

> +                       val |= BIT(i) << 16 | BIT(i);
>         }
> +       if (val)
> +               writel(val, priv->base + CPG_BUS_MSTOP(mstop_index + 1));
>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
>  }
>
> +static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
> +{
> +       struct mod_clock *clock = to_mod_clock(hw);
> +       struct rzv2h_cpg_priv *priv = clock->priv;
> +       u32 bitmask;
> +       u32 offset;
> +
> +       if (clock->mon_index >= 0) {
> +               offset = GET_CLK_MON_OFFSET(clock->mon_index);
> +               bitmask = BIT(clock->mon_bit);
> +       } else {
> +               offset = GET_CLK_ON_OFFSET(clock->on_index);
> +               bitmask = BIT(clock->on_bit);
> +       }
> +
> +       return readl(priv->base + offset) & bitmask;
> +}
> +
>  static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable)
>  {
>         struct mod_clock *clock = to_mod_clock(hw);
> @@ -489,15 +517,19 @@ static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable)
>         dev_dbg(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk,
>                 enable ? "ON" : "OFF");
>
> +       if ((rzv2h_mod_clock_is_enabled(hw) && enable) ||
> +           (!rzv2h_mod_clock_is_enabled(hw) && !enable))
> +               return 0;

This may call rzv2h_mod_clock_is_enabled() twice, as readl() is a
compiler barrier.  You can avoid that using:

    bool enabled = rzv2h_mod_clock_is_enabled(hw);
    if (enabled == enable)
            return 0;

> +
>         value = bitmask << 16;
>         if (enable) {
>                 value |= bitmask;
>                 writel(value, priv->base + reg);
> -               if (clock->mstop)
> -                       rzv2h_mod_clock_mstop_enable(priv, clock);
> +               if (clock->mstop_data != BUS_MSTOP_NONE)
> +                       rzv2h_mod_clock_mstop_enable(priv, clock->mstop_data);
>         } else {
> -               if (clock->mstop)
> -                       rzv2h_mod_clock_mstop_disable(priv, clock);
> +               if (clock->mstop_data != BUS_MSTOP_NONE)
> +                       rzv2h_mod_clock_mstop_disable(priv, clock->mstop_data);
>                 writel(value, priv->base + reg);
>         }
>

> @@ -647,13 +619,16 @@ 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. Skip critical clocks, as they will be
> +        * turned ON immediately upon registration, and the MSTOP counter will be
> +        * updated through the rzv2h_mod_clock_enable() path.
> +        */
> +       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);
>
>         return;
>
> @@ -922,6 +897,13 @@ static int __init rzv2h_cpg_probe(struct platform_device *pdev)
>         if (!clks)
>                 return -ENOMEM;
>
> +       priv->mstop_count = devm_kmalloc_array(dev, info->num_mstop_bits,
> +                                              sizeof(*priv->mstop_count), GFP_KERNEL);

devm_kcalloc() ...

> +       if (!priv->mstop_count)
> +               return -ENOMEM;
> +       for (i = 0; i < info->num_mstop_bits; i++)
> +               atomic_set(&priv->mstop_count[i], 0);

... so you don't need to zero them.

> +
>         priv->resets = devm_kmemdup(dev, info->resets, sizeof(*info->resets) *
>                                     info->num_resets, GFP_KERNEL);
>         if (!priv->resets)

The general idea behind it LGTM.

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