1. Patchew
  2. linux
  3. [PATCH 0/7] clk: renesas: rzg2l-cpg: Drop PM domain abstraction for MSTOP
  4. View patch

[PATCH 1/7] clk: renesas: rzg2l-cpg: Skip lookup of clock when searching for a sibling

Claudiu posted 7 patches 10 months ago
There is a newer version of this series
[PATCH 1/7] clk: renesas: rzg2l-cpg: Skip lookup of clock when searching for a sibling
Posted by Claudiu 10 months ago 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Since the sibling data is filled after the priv->clks[] array entry is
populated, the first clock that is probed and has a sibling will
temporarily behave as its own sibling until its actual sibling is
populated. To avoid any issues, skip this clock when searching for a
sibling.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/clk/renesas/rzg2l-cpg.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index b91dfbfb01e3..2ae36d94fbfa 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -1324,6 +1324,9 @@ static struct mstp_clock
 
 		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
 		clk = to_mod_clock(hw);
+		if (clk == clock)
+			continue;
+
 		if (clock->off == clk->off && clock->bit == clk->bit)
 			return clk;
 	}
-- 
2.43.0
Re: [PATCH 1/7] clk: renesas: rzg2l-cpg: Skip lookup of clock when searching for a sibling
Posted by Geert Uytterhoeven 9 months, 1 week ago 
Hi Claudiu,

On Thu, 10 Apr 2025 at 16:06, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Since the sibling data is filled after the priv->clks[] array entry is
> populated, the first clock that is probed and has a sibling will
> temporarily behave as its own sibling until its actual sibling is
> populated. To avoid any issues, skip this clock when searching for a
> sibling.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -1324,6 +1324,9 @@ static struct mstp_clock
>
>                 hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
>                 clk = to_mod_clock(hw);
> +               if (clk == clock)
> +                       continue;
> +
>                 if (clock->off == clk->off && clock->bit == clk->bit)
>                         return clk;
>         }

Why not move the whole block around the call to
rzg2l_mod_clock_get_sibling() up instead?

            ret = devm_clk_hw_register(dev, &clock->hw);
            if (ret) {
                    clk = ERR_PTR(ret);
                    goto fail;
            }

    -       clk = clock->hw.clk;
    -       dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
clk_get_rate(clk));
    -       priv->clks[id] = clk;
    -
            if (mod->is_coupled) {
                    struct mstp_clock *sibling;

                    clock->enabled = rzg2l_mod_clock_is_enabled(&clock->hw);
                    sibling = rzg2l_mod_clock_get_sibling(clock, priv);
                    if (sibling) {
                            clock->sibling = sibling;
                            sibling->sibling = clock;
                    }
            }

    +       clk = clock->hw.clk;
    +       dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
clk_get_rate(clk));
    +       priv->clks[id] = clk;
    +
            return;

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Re: [PATCH 1/7] clk: renesas: rzg2l-cpg: Skip lookup of clock when searching for a sibling
Posted by Claudiu Beznea 9 months, 1 week ago 
Hi, Geert,

On 05.05.2025 18:52, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, 10 Apr 2025 at 16:06, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Since the sibling data is filled after the priv->clks[] array entry is
>> populated, the first clock that is probed and has a sibling will
>> temporarily behave as its own sibling until its actual sibling is
>> populated. To avoid any issues, skip this clock when searching for a
>> sibling.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>> @@ -1324,6 +1324,9 @@ static struct mstp_clock
>>
>>                 hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
>>                 clk = to_mod_clock(hw);
>> +               if (clk == clock)
>> +                       continue;
>> +
>>                 if (clock->off == clk->off && clock->bit == clk->bit)
>>                         return clk;
>>         }
> 
> Why not move the whole block around the call to
> rzg2l_mod_clock_get_sibling() up instead?
> 
>             ret = devm_clk_hw_register(dev, &clock->hw);
>             if (ret) {
>                     clk = ERR_PTR(ret);
>                     goto fail;
>             }
> 
>     -       clk = clock->hw.clk;
>     -       dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> clk_get_rate(clk));
>     -       priv->clks[id] = clk;
>     -
>             if (mod->is_coupled) {
>                     struct mstp_clock *sibling;
> 
>                     clock->enabled = rzg2l_mod_clock_is_enabled(&clock->hw);
>                     sibling = rzg2l_mod_clock_get_sibling(clock, priv);
>                     if (sibling) {
>                             clock->sibling = sibling;
>                             sibling->sibling = clock;
>                     }
>             }
> 
>     +       clk = clock->hw.clk;
>     +       dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> clk_get_rate(clk));
>     +       priv->clks[id] = clk;
>     +
>             return;

This should work as well. I considered the proposed patch generates less
diff. Please let me know if you prefer it addressed as you proposed.

Thank you for your review,
Claudiu

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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
Re: [PATCH 1/7] clk: renesas: rzg2l-cpg: Skip lookup of clock when searching for a sibling
Posted by Geert Uytterhoeven 9 months, 1 week ago 
Hi Claudiu,

On Wed, 7 May 2025 at 14:12, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 05.05.2025 18:52, Geert Uytterhoeven wrote:
> > On Thu, 10 Apr 2025 at 16:06, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Since the sibling data is filled after the priv->clks[] array entry is
> >> populated, the first clock that is probed and has a sibling will
> >> temporarily behave as its own sibling until its actual sibling is
> >> populated. To avoid any issues, skip this clock when searching for a
> >> sibling.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/clk/renesas/rzg2l-cpg.c
> >> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> >> @@ -1324,6 +1324,9 @@ static struct mstp_clock
> >>
> >>                 hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> >>                 clk = to_mod_clock(hw);
> >> +               if (clk == clock)
> >> +                       continue;
> >> +
> >>                 if (clock->off == clk->off && clock->bit == clk->bit)
> >>                         return clk;
> >>         }
> >
> > Why not move the whole block around the call to
> > rzg2l_mod_clock_get_sibling() up instead?
> >
> >             ret = devm_clk_hw_register(dev, &clock->hw);
> >             if (ret) {
> >                     clk = ERR_PTR(ret);
> >                     goto fail;
> >             }
> >
> >     -       clk = clock->hw.clk;
> >     -       dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> > clk_get_rate(clk));
> >     -       priv->clks[id] = clk;
> >     -
> >             if (mod->is_coupled) {
> >                     struct mstp_clock *sibling;
> >
> >                     clock->enabled = rzg2l_mod_clock_is_enabled(&clock->hw);
> >                     sibling = rzg2l_mod_clock_get_sibling(clock, priv);
> >                     if (sibling) {
> >                             clock->sibling = sibling;
> >                             sibling->sibling = clock;
> >                     }
> >             }
> >
> >     +       clk = clock->hw.clk;
> >     +       dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> > clk_get_rate(clk));
> >     +       priv->clks[id] = clk;
> >     +
> >             return;
>
> This should work as well. I considered the proposed patch generates less
> diff. Please let me know if you prefer it addressed as you proposed.

Given you have a later patch that contains a similar check, postponing
setting priv->clks[id] looks like the better solution to me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.