From: Claudiu Beznea <[email protected]>
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 <[email protected]>
---
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
Hi Claudiu,
On Tue, Sep 12, 2023 at 6:52 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> 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 <[email protected]>
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 -- [email protected]
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
Hi Claudiu,
On Fri, Sep 15, 2023 at 7:51 AM claudiu beznea <[email protected]> wrote:
> On 14.09.2023 16:12, Geert Uytterhoeven wrote:
> > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <[email protected]> wrote:
> >> From: Claudiu Beznea <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > 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 -- [email protected]
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
On 14.09.2023 16:12, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Tue, Sep 12, 2023 at 6:52 AM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> 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 <[email protected]>
>
> 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
>