Hi Prabhakar,
On Thu, Jan 2, 2025 at 7:18 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
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
