| Message ID | 20241223173708.384108-5-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add SYS and GIC clock entries for RZ/V2H(P) SoC | expand |
Hi Prabhakar, On Mon, Dec 23, 2024 at 6:37 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.renesas.com> > --- > v1->v2 > - New patch Thanks for your patch! Early review comment: the big missing part in the patch description is the answer to the "Why?"-question. So please elaborate. No need to resend (for now). Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On Tue, Dec 24, 2024 at 12:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Dec 23, 2024 at 6:37 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.renesas.com> > > --- > > v1->v2 > > - New patch > > Thanks for your patch! > > Early review comment: the big missing part in the patch description > is the answer to the "Why?"-question. So please elaborate. > Ahh, I was under the impression patch 1-4 will be squashed into the original patch series so I didn't elaborate it much. If that's not the case I'll update the commit message as below: clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis 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. > No need to resend (for now). OK. Cheers, Prabhakar
Hi Prabhakar, On Mon, Dec 23, 2024 at 6:37 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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)... > > #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
Hi Geert, Thank you for the review. On Fri, Dec 27, 2024 at 3:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Dec 23, 2024 at 6:37 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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
diff --git a/drivers/clk/renesas/r9a09g047-cpg.c b/drivers/clk/renesas/r9a09g047-cpg.c index 7945b9f95b95..536d922bed70 100644 --- 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, }; 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, }; diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c index 29b1ce003370..154ad0db6dca 100644 --- 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 */ +#define GET_MSTOP_IDX(mask) ((FIELD_GET(BUS_MSTOP_IDX_MASK, (mask))) - 1) #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 * @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; }; #define rcdev_to_priv(x) container_of(x, struct rzv2h_cpg_priv, rcdev) -struct rzv2h_mstop { - u16 idx; - u16 mask; - refcount_t ref_cnt; -}; - struct pll_clk { struct rzv2h_cpg_priv *priv; void __iomem *base; @@ -107,7 +106,7 @@ struct pll_clk { * struct mod_clock - Module clock * * @priv: CPG private data - * @mstop: handle to cpg bus mstop data + * @mstop_data: mstop data relating to module clock * @hw: handle between common and hardware-specific interfaces * @no_pm: flag to indicate PM is not supported * @on_index: register offset @@ -117,7 +116,7 @@ struct pll_clk { */ struct mod_clock { struct rzv2h_cpg_priv *priv; - struct rzv2h_mstop *mstop; + unsigned int mstop_data; struct clk_hw hw; bool no_pm; u8 on_index; @@ -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)); + 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_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) { + 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)); 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])) + 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; + 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); } @@ -525,73 +557,12 @@ static void rzv2h_mod_clock_disable(struct clk_hw *hw) rzv2h_mod_clock_endisable(hw, false); } -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 const struct clk_ops rzv2h_mod_clock_ops = { .enable = rzv2h_mod_clock_enable, .disable = rzv2h_mod_clock_disable, .is_enabled = rzv2h_mod_clock_is_enabled, }; -static struct rzv2h_mstop -*rzv2h_cpg_get_mstop(struct rzv2h_cpg_priv *priv, struct mod_clock *clock, u32 mstop_data) -{ - struct rzv2h_mstop *mstop; - unsigned int i; - - for (i = 0; i < priv->num_mod_clks; i++) { - struct mod_clock *clk; - struct clk_hw *hw; - - if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT)) - continue; - - hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]); - clk = to_mod_clock(hw); - if (!clk->mstop) - continue; - - if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) { - if (rzv2h_mod_clock_is_enabled(&clock->hw)) { - if (refcount_read(&clk->mstop->ref_cnt)) - refcount_inc(&clk->mstop->ref_cnt); - else - refcount_set(&clk->mstop->ref_cnt, 1); - } - return clk->mstop; - } - } - - mstop = devm_kzalloc(priv->dev, sizeof(*mstop), GFP_KERNEL); - if (!mstop) - return NULL; - - mstop->idx = FIELD_GET(BUS_MSTOP_IDX_MASK, (mstop_data)); - mstop->mask = FIELD_GET(BUS_MSTOP_BITS_MASK, (mstop_data)); - if (rzv2h_mod_clock_is_enabled(&clock->hw)) - refcount_set(&mstop->ref_cnt, 1); - else - refcount_set(&mstop->ref_cnt, 0); - - return mstop; -} - static void __init rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod, struct rzv2h_cpg_priv *priv) @@ -638,6 +609,7 @@ rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod, clock->no_pm = mod->no_pm; clock->priv = priv; clock->hw.init = &init; + clock->mstop_data = mod->mstop_data; ret = devm_clk_hw_register(dev, &clock->hw); if (ret) { @@ -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); + if (!priv->mstop_count) + return -ENOMEM; + for (i = 0; i < info->num_mstop_bits; i++) + atomic_set(&priv->mstop_count[i], 0); + priv->resets = devm_kmemdup(dev, info->resets, sizeof(*info->resets) * info->num_resets, GFP_KERNEL); if (!priv->resets) diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h index f918620c4650..a772304f9057 100644 --- a/drivers/clk/renesas/rzv2h-cpg.h +++ b/drivers/clk/renesas/rzv2h-cpg.h @@ -193,6 +193,9 @@ struct rzv2h_reset { * * @resets: Array of Module Reset definitions * @num_resets: Number of entries in resets[] + * + * @num_mstop_bits: Maximum number of MSTOP bits supported, equivalent to the + * number of CPG_BUS_m_MSTOP registers multiplied by 16. */ struct rzv2h_cpg_info { /* Core Clocks */ @@ -209,6 +212,8 @@ struct rzv2h_cpg_info { /* Resets */ const struct rzv2h_reset *resets; unsigned int num_resets; + + unsigned int num_mstop_bits; }; extern const struct rzv2h_cpg_info r9a09g047_cpg_info;