Hi Geert,
Thank you for the review.
On Fri, Dec 27, 2024 at 3:49 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> 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)...
>
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
