diff mbox series

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

Message ID 20230912045157.177966-13-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State New, archived
Headers show
Series Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK | expand

Commit Message

Claudiu Beznea Sept. 12, 2023, 4:51 a.m. UTC 
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(-)

Comments

Geert Uytterhoeven Sept. 14, 2023, 1:12 p.m. UTC | #1 
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
Claudiu Beznea Sept. 15, 2023, 5:51 a.m. UTC | #2 
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
>
Geert Uytterhoeven Sept. 15, 2023, 7:05 a.m. UTC | #3 
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
diff mbox series

Patch

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)