1. Patchew
  2. linux
  3. [PATCH 00/37] Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK
  4. View patch

[PATCH 12/37] clk: renesas: rzg2l: reduce the critical area

Claudiu posted 37 patches 2 years, 3 months ago
There is a newer version of this series
[PATCH 12/37] clk: renesas: rzg2l: reduce the critical area
Posted by Claudiu 2 years, 3 months ago 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses
to hardware register. There is no need to protect the instructions that set
temporary variable which will be then written to register. Thus limit the
spinlock only to the hardware register access.

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

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 6c289223a4e2..d8801f88df8e 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 
 	dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk,
 		enable ? "ON" : "OFF");
-	spin_lock_irqsave(&priv->rmw_lock, flags);
 
 	value = bitmask << 16;
 	if (enable)
 		value |= bitmask;
-	writel(value, priv->base + CLK_ON_R(reg));
 
+	spin_lock_irqsave(&priv->rmw_lock, flags);
+	writel(value, priv->base + CLK_ON_R(reg));
 	spin_unlock_irqrestore(&priv->rmw_lock, flags);
 
 	if (!enable)
-- 
2.39.2
Re: [PATCH 12/37] clk: renesas: rzg2l: reduce the critical area
Posted by Geert Uytterhoeven 2 years, 3 months ago 
Hi Claudiu,

On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses
> to hardware register. There is no need to protect the instructions that set
> temporary variable which will be then written to register. Thus limit the
> spinlock only to the hardware register access.
>
> 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
> @@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
>
>         dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk,
>                 enable ? "ON" : "OFF");
> -       spin_lock_irqsave(&priv->rmw_lock, flags);
>
>         value = bitmask << 16;
>         if (enable)
>                 value |= bitmask;
> -       writel(value, priv->base + CLK_ON_R(reg));
>
> +       spin_lock_irqsave(&priv->rmw_lock, flags);
> +       writel(value, priv->base + CLK_ON_R(reg));
>         spin_unlock_irqrestore(&priv->rmw_lock, flags);

After this, it becomes obvious there is nothing to protect at all,
so the locking can just be removed from this function?

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 12/37] clk: renesas: rzg2l: reduce the critical area
Posted by claudiu beznea 2 years, 3 months ago 

On 14.09.2023 16:12, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses
>> to hardware register. There is no need to protect the instructions that set
>> temporary variable which will be then written to register. Thus limit the
>> spinlock only to the hardware register access.
>>
>> 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
>> @@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
>>
>>         dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk,
>>                 enable ? "ON" : "OFF");
>> -       spin_lock_irqsave(&priv->rmw_lock, flags);
>>
>>         value = bitmask << 16;
>>         if (enable)
>>                 value |= bitmask;
>> -       writel(value, priv->base + CLK_ON_R(reg));
>>
>> +       spin_lock_irqsave(&priv->rmw_lock, flags);
>> +       writel(value, priv->base + CLK_ON_R(reg));
>>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> 
> After this, it becomes obvious there is nothing to protect at all,
> so the locking can just be removed from this function?

I tend to be paranoid when writing to hardware resources thus I kept it.
Would you prefer to remove it at all?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Re: [PATCH 12/37] clk: renesas: rzg2l: reduce the critical area
Posted by Geert Uytterhoeven 2 years, 3 months ago 
Hi Claudiu,

On Fri, Sep 15, 2023 at 7:51 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
> On 14.09.2023 16:12, Geert Uytterhoeven wrote:
> > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> spinlock in rzg2l_mod_clock_endisable() is intended to protect the accesses
> >> to hardware register. There is no need to protect the instructions that set
> >> temporary variable which will be then written to register. Thus limit the
> >> spinlock only to the hardware register access.
> >>
> >> 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
> >> @@ -912,13 +912,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
> >>
> >>         dev_dbg(dev, "CLK_ON %u/%pC %s\n", CLK_ON_R(reg), hw->clk,
> >>                 enable ? "ON" : "OFF");
> >> -       spin_lock_irqsave(&priv->rmw_lock, flags);
> >>
> >>         value = bitmask << 16;
> >>         if (enable)
> >>                 value |= bitmask;
> >> -       writel(value, priv->base + CLK_ON_R(reg));
> >>
> >> +       spin_lock_irqsave(&priv->rmw_lock, flags);
> >> +       writel(value, priv->base + CLK_ON_R(reg));
> >>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >
> > After this, it becomes obvious there is nothing to protect at all,
> > so the locking can just be removed from this function?
>
> I tend to be paranoid when writing to hardware resources thus I kept it.
> Would you prefer to remove it at all?

Yes please. I guess this was copied from R-Car and friends, where
there is a RMW operation on an MSTPCR register.

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 - 2025 Red Hat, Inc.