Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Geert,

geert@linux-m68k.org wrote on Mon, 21 Feb 2022 10:16:24 +0100:

> Hi Miquel,
> 
> On Fri, Feb 18, 2022 at 7:12 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > The dmamux register is located within the system controller.
> >
> > Without syscon, we need an extra helper in order to give write access to
> > this register to a dmamux driver.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a06g032-clocks.c
> > +++ b/drivers/clk/renesas/r9a06g032-clocks.c  
> 
> Missing #include <linux/soc/renesas/r9a06g032-syscon.h>
> 
> > @@ -315,6 +315,27 @@ struct r9a06g032_priv {
> >         void __iomem *reg;
> >  };
> >
> > +/* Exported helper to access the DMAMUX register */
> > +static struct r9a06g032_priv *syscon_priv;  
> 
> I'd call this sysctrl_priv, as that matches the bindings and
> binding header file name.

Ok.

> 
> > +int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
> > +{
> > +       u32 dmamux;
> > +
> > +       if (!syscon_priv)
> > +               return -EPROBE_DEFER;
> > +
> > +       spin_lock(&syscon_priv->lock);  
> 
> This needs propection against interrupts => spin_lock_irqsave().

Yes.

> 
> > +
> > +       dmamux = readl(syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
> > +       dmamux &= ~mask;
> > +       dmamux |= val & mask;
> > +       writel(dmamux, syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
> > +
> > +       spin_unlock(&syscon_priv->lock);
> > +
> > +       return 0;
> > +}
> > +
> >  /* register/bit pairs are encoded as an uint16_t */
> >  static void
> >  clk_rdesc_set(struct r9a06g032_priv *clocks,  
> 
> > --- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > +++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > @@ -145,4 +145,6 @@
> >  #define R9A06G032_CLK_UART6            152
> >  #define R9A06G032_CLK_UART7            153
> >
> > +#define R9A06G032_SYSCON_DMAMUX                0xA0  
> 
> I don't think this needs to be part of the bindings, so please move
> it to the driver source file.

I've moved it to the top of the file. There definitions are a bit mixed
with the code, I don't like this, so I kept it at the top.

> 
> > --- /dev/null
> > +++ b/include/linux/soc/renesas/r9a06g032-syscon.h  
> 
> r9a06g032-sysctrl.h etc.

Done.

> 
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
> > +#define __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
> > +
> > +#ifdef CONFIG_CLK_R9A06G032
> > +int r9a06g032_syscon_set_dmamux(u32 mask, u32 val);
> > +#else
> > +static inline int r9a06g032_syscon_set_dmamux(u32 mask, u32 val) { return -ENODEV; }
> > +#endif
> > +
> > +#endif /* __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__ */
> > --
> > 2.27.0  
> 
> 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


Thanks,
Miquèl