| Message ID | 20250102181839.352599-5-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix MSTOP handling and add SYS/GIC clock entries for RZ/V2H(P) SoC | expand |
Hi Prabhakar, On Thu, Jan 2, 2025 at 7:18 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > 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@bp.renesas.com> > --- > 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@glider.be> Gr{oetje,eeting}s, Geert
Hi Prabhakar, Thanks for the patch. > -----Original Message----- > From: Prabhakar <prabhakar.csengg@gmail.com> > Sent: 02 January 2025 18:19 > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > 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. I guess this statement is incorrect. Still in group-based, mstop bit is controlled by usage count(ref_cnt). The real advantage with per-bit configuration is, we can drop index manipulation. Cheers, Biju
Hi Biju, On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > Thanks for the patch. > > > -----Original Message----- > > From: Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 02 January 2025 18:19 > > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > 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. > > I guess this statement is incorrect. Still in group-based, mstop bit is controlled by usage count(ref_cnt). > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 | 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same MSTOP register say MSTOP1). Now this will be seperate groups having separate count(ref_cnt). Say you turn ON IP-A module clock and correspondingly clear the MSTOP bits and similarly now lets turn ON module clocks for IP-B and clear the MSTOP bits. Now let's say you want to turn OFF IP-A so you turn OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case you will now see issues with IP-B as MSTOP BIT(10) has been set when we turned OFF IP-A block. This case is handled by switching refcount on per mstop bit by this patch. Cheers, Prabhakar
Hi Prabhakar, > -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: 07 January 2025 11:46 > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > Hi Biju, > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > Thanks for the patch. > > > > > -----Original Message----- > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 02 January 2025 18:19 > > > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > configuration to per-bit basis > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > 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. > > > > I guess this statement is incorrect. Still in group-based, mstop bit is controlled by usage > count(ref_cnt). > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 | > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same MSTOP register say MSTOP1). Now > this will be seperate groups having separate count(ref_cnt). Say you turn ON IP-A module clock and > correspondingly clear the MSTOP bits and similarly now lets turn ON module clocks for IP-B and clear > the MSTOP bits. Now let's say you want to turn OFF IP-A so you turn OFF module clock and set the MSTOP > bits 8 | 9 | 10. In this case you will now see issues with IP-B as MSTOP BIT(10) has been set when we > turned OFF IP-A block. This case is handled by switching refcount on per mstop bit by this patch. I agree, Do we have such use case? Consider another use case, index 0, bit 8| index 0, bit9| index0, bit10 and index 0, bit8 | index1, bit 0 | index1 10 is addressed in current patch series? Cheers, Biju
Hi Biju, On Tue, Jan 7, 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > > -----Original Message----- > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 07 January 2025 11:46 > > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > > > Hi Biju, > > > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > Hi Prabhakar, > > > > > > Thanks for the patch. > > > > > > > -----Original Message----- > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > Sent: 02 January 2025 18:19 > > > > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > > configuration to per-bit basis > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > 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. > > > > > > I guess this statement is incorrect. Still in group-based, mstop bit is controlled by usage > > count(ref_cnt). > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 | > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same MSTOP register say MSTOP1). Now > > this will be seperate groups having separate count(ref_cnt). Say you turn ON IP-A module clock and > > correspondingly clear the MSTOP bits and similarly now lets turn ON module clocks for IP-B and clear > > the MSTOP bits. Now let's say you want to turn OFF IP-A so you turn OFF module clock and set the MSTOP > > bits 8 | 9 | 10. In this case you will now see issues with IP-B as MSTOP BIT(10) has been set when we > > turned OFF IP-A block. This case is handled by switching refcount on per mstop bit by this patch. > > I agree, Do we have such use case? > Yes, for USB2.0 on RZ/V2H. > Consider another use case, index 0, bit 8| index 0, bit9| index0, bit10 and index 0, bit8 | index1, bit 0 | index1 10 is addressed in current patch series? > Can you please elaborate, the above isn't clear to me. Cheers, Prabhakar
Hi Prabhakar, > -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: 07 January 2025 12:31 > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > Hi Biju, > > On Tue, Jan 7, 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > > -----Original Message----- > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 07 January 2025 11:46 > > > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > configuration to per-bit basis > > > > > > Hi Biju, > > > > > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > Hi Prabhakar, > > > > > > > > Thanks for the patch. > > > > > > > > > -----Original Message----- > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > Sent: 02 January 2025 18:19 > > > > > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > > > configuration to per-bit basis > > > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > 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. > > > > > > > > I guess this statement is incorrect. Still in group-based, mstop > > > > bit is controlled by usage > > > count(ref_cnt). > > > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 > > > | > > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same > > > MSTOP register say MSTOP1). Now this will be seperate groups having > > > separate count(ref_cnt). Say you turn ON IP-A module clock and > > > correspondingly clear the MSTOP bits and similarly now lets turn ON > > > module clocks for IP-B and clear the MSTOP bits. Now let's say you > > > want to turn OFF IP-A so you turn OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case > you will now see issues with IP-B as MSTOP BIT(10) has been set when we turned OFF IP-A block. This > case is handled by switching refcount on per mstop bit by this patch. > > > > I agree, Do we have such use case? > > > Yes, for USB2.0 on RZ/V2H. OK, then it make sense for per-bit configuration. > > > Consider another use case, index 0, bit 8| index 0, bit9| index0, bit10 and index 0, bit8 | index1, > bit 0 | index1 10 is addressed in current patch series? > > > Can you please elaborate, the above isn't clear to me. I just provide a random example for a future IP, where IP_A requires mstop1 {8,9,10} And IP_B requires mstop1 {8} and mstop2 {9, 10} Note: I haven't seen this scenario in hardware manual. Cheers, Biju
Hi Biju, On Tue, Jan 7, 2025 at 12:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > > -----Original Message----- > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 07 January 2025 12:31 > > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > > > Hi Biju, > > > > On Tue, Jan 7, 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > Hi Prabhakar, > > > > > > > -----Original Message----- > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > > Sent: 07 January 2025 11:46 > > > > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > > configuration to per-bit basis > > > > > > > > Hi Biju, > > > > > > > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > > > Hi Prabhakar, > > > > > > > > > > Thanks for the patch. > > > > > > > > > > > -----Original Message----- > > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > > Sent: 02 January 2025 18:19 > > > > > > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > > > > configuration to per-bit basis > > > > > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > 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. > > > > > > > > > > I guess this statement is incorrect. Still in group-based, mstop > > > > > bit is controlled by usage > > > > count(ref_cnt). > > > > > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 > > > > | > > > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same > > > > MSTOP register say MSTOP1). Now this will be seperate groups having > > > > separate count(ref_cnt). Say you turn ON IP-A module clock and > > > > correspondingly clear the MSTOP bits and similarly now lets turn ON > > > > module clocks for IP-B and clear the MSTOP bits. Now let's say you > > > > want to turn OFF IP-A so you turn OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case > > you will now see issues with IP-B as MSTOP BIT(10) has been set when we turned OFF IP-A block. This > > case is handled by switching refcount on per mstop bit by this patch. > > > > > > I agree, Do we have such use case? > > > > > Yes, for USB2.0 on RZ/V2H. > > OK, then it make sense for per-bit configuration. > > > > > > Consider another use case, index 0, bit 8| index 0, bit9| index0, bit10 and index 0, bit8 | index1, > > bit 0 | index1 10 is addressed in current patch series? > > > > > Can you please elaborate, the above isn't clear to me. > > > I just provide a random example for a future IP, where > > IP_A requires mstop1 {8,9,10} > And > IP_B requires mstop1 {8} and mstop2 {9, 10} > No, this case is not handled by the patch series. > Note: I haven't seen this scenario in hardware manual. > Yes, neither do I. For this case we will have to re-work the BUS_MSTOP() macro. Let me know if we want this case to be handled. I'll create a new patch on top of this series. Cheers. Prabhakar
Hi Biju, On Tue, Jan 7, 2025 at 1:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > On Tue, Jan 7, 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > 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. > > > > > > > > > > I guess this statement is incorrect. Still in group-based, mstop > > > > > bit is controlled by usage > > > > count(ref_cnt). > > > > > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 > > > > | > > > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same > > > > MSTOP register say MSTOP1). Now this will be seperate groups having > > > > separate count(ref_cnt). Say you turn ON IP-A module clock and > > > > correspondingly clear the MSTOP bits and similarly now lets turn ON > > > > module clocks for IP-B and clear the MSTOP bits. Now let's say you > > > > want to turn OFF IP-A so you turn OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case > > you will now see issues with IP-B as MSTOP BIT(10) has been set when we turned OFF IP-A block. This > > case is handled by switching refcount on per mstop bit by this patch. > > > > > > I agree, Do we have such use case? > > > > > Yes, for USB2.0 on RZ/V2H. > > OK, then it make sense for per-bit configuration. > > > > Consider another use case, index 0, bit 8| index 0, bit9| index0, bit10 and index 0, bit8 | index1, > > bit 0 | index1 10 is addressed in current patch series? > > > > > Can you please elaborate, the above isn't clear to me. > > I just provide a random example for a future IP, where > > IP_A requires mstop1 {8,9,10} > And > IP_B requires mstop1 {8} and mstop2 {9, 10} > > Note: I haven't seen this scenario in hardware manual. That case is indeed not handled, and I had already checked before it is not needed for the current SoCs (until we discover e.g. a dependency between different GTM channels ;-) If it is ever needed for future SoCs, the system has to be adapted... Gr{oetje,eeting}s, Geert
Hi Prabhakar, > -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: 07 January 2025 12:44 > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > Hi Biju, > > On Tue, Jan 7, 2025 at 12:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > > -----Original Message----- > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: 07 January 2025 12:31 > > > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > configuration to per-bit basis > > > > > > Hi Biju, > > > > > > On Tue, Jan 7, 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > Hi Prabhakar, > > > > > > > > > -----Original Message----- > > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > > > Sent: 07 January 2025 11:46 > > > > > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > > > configuration to per-bit basis > > > > > > > > > > Hi Biju, > > > > > > > > > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > > > > > Hi Prabhakar, > > > > > > > > > > > > Thanks for the patch. > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > > > Sent: 02 January 2025 18:19 > > > > > > > Subject: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP > > > > > > > configuration to per-bit basis > > > > > > > > > > > > > > From: Lad Prabhakar > > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > > > 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. > > > > > > > > > > > > I guess this statement is incorrect. Still in group-based, > > > > > > mstop bit is controlled by usage > > > > > count(ref_cnt). > > > > > > > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 > > > > > | 9 > > > > > | > > > > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the > > > > > same MSTOP register say MSTOP1). Now this will be seperate > > > > > groups having separate count(ref_cnt). Say you turn ON IP-A > > > > > module clock and correspondingly clear the MSTOP bits and > > > > > similarly now lets turn ON module clocks for IP-B and clear the > > > > > MSTOP bits. Now let's say you want to turn OFF IP-A so you turn > > > > > OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case > > > you will now see issues with IP-B as MSTOP BIT(10) has been set when > > > we turned OFF IP-A block. This case is handled by switching refcount on per mstop bit by this > patch. > > > > > > > > I agree, Do we have such use case? > > > > > > > Yes, for USB2.0 on RZ/V2H. > > > > OK, then it make sense for per-bit configuration. > > > > > > > > > Consider another use case, index 0, bit 8| index 0, bit9| index0, > > > > bit10 and index 0, bit8 | index1, > > > bit 0 | index1 10 is addressed in current patch series? > > > > > > > Can you please elaborate, the above isn't clear to me. > > > > > > I just provide a random example for a future IP, where > > > > IP_A requires mstop1 {8,9,10} > > And > > IP_B requires mstop1 {8} and mstop2 {9, 10} > > > No, this case is not handled by the patch series. > > > Note: I haven't seen this scenario in hardware manual. > > > Yes, neither do I. For this case we will have to re-work the > BUS_MSTOP() macro. Let me know if we want this case to be handled. > I'll create a new patch on top of this series. I guess we can address this later when a real use case like USB2.0 arises. Cheers, Biju
Hi Prabhakar, On Tue, Jan 7, 2025 at 1:44 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Tue, Jan 7, 2025 at 12:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > On Tue, Jan 7, 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > > > On Tue, Jan 7, 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > > > 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. > > > > > > > > > > > > I guess this statement is incorrect. Still in group-based, mstop > > > > > > bit is controlled by usage > > > > > count(ref_cnt). > > > > > > > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 | 9 > > > > > | > > > > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the same > > > > > MSTOP register say MSTOP1). Now this will be seperate groups having > > > > > separate count(ref_cnt). Say you turn ON IP-A module clock and > > > > > correspondingly clear the MSTOP bits and similarly now lets turn ON > > > > > module clocks for IP-B and clear the MSTOP bits. Now let's say you > > > > > want to turn OFF IP-A so you turn OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case > > > you will now see issues with IP-B as MSTOP BIT(10) has been set when we turned OFF IP-A block. This > > > case is handled by switching refcount on per mstop bit by this patch. > > > > > > > > Consider another use case, index 0, bit 8| index 0, bit9| index0, bit10 and index 0, bit8 | index1, > > > bit 0 | index1 10 is addressed in current patch series? > > > > > > > Can you please elaborate, the above isn't clear to me. > > > > I just provide a random example for a future IP, where > > > > IP_A requires mstop1 {8,9,10} > > And > > IP_B requires mstop1 {8} and mstop2 {9, 10} > > > No, this case is not handled by the patch series. > > > Note: I haven't seen this scenario in hardware manual. > > > Yes, neither do I. For this case we will have to re-work the > BUS_MSTOP() macro. Let me know if we want this case to be handled. > I'll create a new patch on top of this series. -EPROBE_DEFER. I.e. fix it when the need arises (if ever)... Gr{oetje,eeting}s, Geert
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 07 January 2025 12:50 > Subject: Re: [PATCH v3 4/6] clk: renesas: rzv2h: Switch MSTOP configuration to per-bit basis > > Hi Biju, > > On Tue, Jan 7, 2025 at 1:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> On Tue, Jan 7, > > > 2025 at 12:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> On Tue, Jan 7, > > > > > 2025 at 11:24 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > From: Lad Prabhakar > > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > > > 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. > > > > > > > > > > > > I guess this statement is incorrect. Still in group-based, > > > > > > mstop bit is controlled by usage > > > > > count(ref_cnt). > > > > > > > > > > > It is valid, consider an example say IP-A reuiqres MSTOP bits 8 > > > > > | 9 > > > > > | > > > > > 10 and consider IP-B requires MSTOP bits 10 | 11 | 12 (of the > > > > > same MSTOP register say MSTOP1). Now this will be seperate > > > > > groups having separate count(ref_cnt). Say you turn ON IP-A > > > > > module clock and correspondingly clear the MSTOP bits and > > > > > similarly now lets turn ON module clocks for IP-B and clear the > > > > > MSTOP bits. Now let's say you want to turn OFF IP-A so you turn > > > > > OFF module clock and set the MSTOP bits 8 | 9 | 10. In this case > > > you will now see issues with IP-B as MSTOP BIT(10) has been set when > > > we turned OFF IP-A block. This case is handled by switching refcount on per mstop bit by this > patch. > > > > > > > > I agree, Do we have such use case? > > > > > > > Yes, for USB2.0 on RZ/V2H. > > > > OK, then it make sense for per-bit configuration. > > > > > > Consider another use case, index 0, bit 8| index 0, bit9| index0, > > > > bit10 and index 0, bit8 | index1, > > > bit 0 | index1 10 is addressed in current patch series? > > > > > > > Can you please elaborate, the above isn't clear to me. > > > > I just provide a random example for a future IP, where > > > > IP_A requires mstop1 {8,9,10} > > And > > IP_B requires mstop1 {8} and mstop2 {9, 10} > > > > Note: I haven't seen this scenario in hardware manual. > > That case is indeed not handled, and I had already checked before it is not needed for the current > SoCs (until we discover e.g. a dependency between different GTM channels ;-) If it is ever needed for > future SoCs, the system has to be adapted... I agree. Cheers, Biju
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 23fb209d3232..a4c1e92e1fd7 100644 --- a/drivers/clk/renesas/rzv2h-cpg.c +++ b/drivers/clk/renesas/rzv2h-cpg.c @@ -68,6 +68,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 values * @rcdev: Reset controller entity */ struct rzv2h_cpg_priv { @@ -82,17 +83,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 +104,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 +114,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,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; + 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); } static void rzv2h_mod_clock_mstop_disable(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; + 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_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, &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); } +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) { + bool enabled = rzv2h_mod_clock_is_enabled(hw); struct mod_clock *clock = to_mod_clock(hw); unsigned int reg = GET_CLK_ON_OFFSET(clock->on_index); struct rzv2h_cpg_priv *priv = clock->priv; @@ -489,15 +518,18 @@ 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 (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); } @@ -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,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); } 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; + 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;