| Message ID | 20230912045157.177966-13-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK | expand |
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 >
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 --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)