| Message ID | 20220222103437.194779-4-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | RZN1 DMA support | expand |
Hi Miquel, On Tue, Feb 22, 2022 at 11:35 AM 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 > @@ -922,6 +947,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev) > clocks->reg = of_iomap(np, 0); > if (WARN_ON(!clocks->reg)) > return -ENOMEM; > + > + if (sysctrl_priv) > + return -EBUSY; Can this actually happen, or can the check be removed? > + > + sysctrl_priv = clocks; Oh yes, it can happen, if any of the clock registrations below fail due to -EPROBE_DEFER. So I think the assignment to sysctrl_priv should be postponed until we're sure that can no longer happen. Then the check above can be removed, too. > + > for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) { > const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i]; > const char *parent_name = d->source ? The rest LGTM. 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
Hi Geert, geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:28:35 +0100: > Hi Miquel, > > On Tue, Feb 22, 2022 at 11:35 AM 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 > > @@ -922,6 +947,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev) > > clocks->reg = of_iomap(np, 0); > > if (WARN_ON(!clocks->reg)) > > return -ENOMEM; > > + > > + if (sysctrl_priv) > > + return -EBUSY; > > Can this actually happen, or can the check be removed? I mostly wanted to prevent the case where a SoC would need the clock driver to probe twice (sanity check let's say). > > + > > + sysctrl_priv = clocks; > > Oh yes, it can happen, if any of the clock registrations below fail > due to -EPROBE_DEFER. So I think the assignment to sysctrl_priv > should be postponed until we're sure that can no longer happen. > Then the check above can be removed, too. Ok, no problem. > > + > > for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) { > > const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i]; > > const char *parent_name = d->source ? > > The rest LGTM. Great, thanks! Miquèl
diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c index c99942f0e4d4..370c89490be9 100644 --- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -20,9 +20,12 @@ #include <linux/pm_clock.h> #include <linux/pm_domain.h> #include <linux/slab.h> +#include <linux/soc/renesas/r9a06g032-sysctrl.h> #include <linux/spinlock.h> #include <dt-bindings/clock/r9a06g032-sysctrl.h> +#define R9A06G032_SYSCTRL_DMAMUX 0xA0 + struct r9a06g032_gate { u16 gate, reset, ready, midle, scon, mirack, mistat; @@ -315,6 +318,28 @@ struct r9a06g032_priv { void __iomem *reg; }; +/* Exported helper to access the DMAMUX register */ +static struct r9a06g032_priv *sysctrl_priv; +int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val) +{ + unsigned long flags; + u32 dmamux; + + if (!sysctrl_priv) + return -EPROBE_DEFER; + + spin_lock_irqsave(&sysctrl_priv->lock, flags); + + dmamux = readl(sysctrl_priv->reg + R9A06G032_SYSCTRL_DMAMUX); + dmamux &= ~mask; + dmamux |= val & mask; + writel(dmamux, sysctrl_priv->reg + R9A06G032_SYSCTRL_DMAMUX); + + spin_unlock_irqrestore(&sysctrl_priv->lock, flags); + + return 0; +} + /* register/bit pairs are encoded as an uint16_t */ static void clk_rdesc_set(struct r9a06g032_priv *clocks, @@ -922,6 +947,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev) clocks->reg = of_iomap(np, 0); if (WARN_ON(!clocks->reg)) return -ENOMEM; + + if (sysctrl_priv) + return -EBUSY; + + sysctrl_priv = clocks; + for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) { const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i]; const char *parent_name = d->source ? diff --git a/include/linux/soc/renesas/r9a06g032-sysctrl.h b/include/linux/soc/renesas/r9a06g032-sysctrl.h new file mode 100644 index 000000000000..066dfb15cbdd --- /dev/null +++ b/include/linux/soc/renesas/r9a06g032-sysctrl.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __LINUX_SOC_RENESAS_R9A06G032_SYSCTRL_H__ +#define __LINUX_SOC_RENESAS_R9A06G032_SYSCTRL_H__ + +#ifdef CONFIG_CLK_R9A06G032 +int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val); +#else +static inline int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val) { return -ENODEV; } +#endif + +#endif /* __LINUX_SOC_RENESAS_R9A06G032_SYSCTRL_H__ */
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> --- drivers/clk/renesas/r9a06g032-clocks.c | 31 +++++++++++++++++++ include/linux/soc/renesas/r9a06g032-sysctrl.h | 11 +++++++ 2 files changed, 42 insertions(+) create mode 100644 include/linux/soc/renesas/r9a06g032-sysctrl.h