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
