Message-ID: <CA+V-a8tWK+3ybxKYJguWpShYeiXF2MtJgoCA-o75KUrm1y8Atw@mail.gmail.com>
Date: Tue, 31 Dec 2024 12:52:47 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
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 v2 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to
 per-bit basis

Hi Geert,

Thank you for the review.

On Fri, Dec 27, 2024 at 3:49 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Dec 23, 2024 at 6:37 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > 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@...renesas.com>
>
> 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)...
>
As agreed below, I'll get rid of this macro.

> >
> >  #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?
>
OK.

> >   * @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.
>
OK.

> > +       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.
>
OK.

> > +               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.
>
Something like below?

static void rzv2h_mod_clock_mstop_enable(struct rzv2h_cpg_priv *priv,
                     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;
    atomic_t *mstop = &priv->mstop_count[index];
    unsigned long flags;
    unsigned int i;
    u32 val = 0;

    spin_lock_irqsave(&priv->rmw_lock, flags);
    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);
}

static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
                      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;
    atomic_t *mstop = &priv->mstop_count[index];
    unsigned long flags;
    unsigned int i;
    u32 val = 0;

    spin_lock_irqsave(&priv->rmw_lock, flags);
    for_each_set_bit(i, &mstop_mask, 16) {
        if (!atomic_read(&mstop[i]) ||
            atomic_dec_and_test(&mstop[i]))
            val |= BIT(i) << 16 | BIT(i);
    }
    if (val)
        writel(val, priv->base + CPG_BUS_MSTOP(mstop_index));
    spin_unlock_irqrestore(&priv->rmw_lock, flags);
}


>
> >         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()?
>
no, that's to avoid underflow.

> > +                       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;
>
OK.

> > +
> >         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);
> >
Ive updated this code, to handle a case where critical clocks are
turned ON by bootloader. Now updated code looks like below:

    /*
     * 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);
    }

> >         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.
>
OK.

Cheers,
Prabhakar

Powered by blists - more mailing lists