Date:   Fri, 24 May 2019 12:28:01 +0000
From:   Gareth Williams <gareth.williams.jx@...esas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     Geert Uytterhoeven <geert+renesas@...der.be>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] clk: renesas: r9a06g032: Add clock domain support

Hi Geert,

Thanks for the feedback. I added some additional information about the 
logic I used for the USB clocks inline below and will send V3 shortly.

On Tue, May 22, 2019 at 1:02 PM Gareth Williams
<geert@...ux-m68k.org> wrote:
> Hi Gareth,
> 
> On Tue, May 21, 2019 at 2:35 PM Gareth Williams
> <gareth.williams.jx@...esas.com> wrote:
> > There are several clocks on the r9ag032 which are currently not
> > enabled in their drivers that can be delegated to clock domain system
> > for power management. Therefore add support for clock domain
> > functionality to the
> > r9a06g032 clock driver.
> >
> > Signed-off-by: Gareth Williams <gareth.williams.jx@...esas.com>
> > ---
> > v2:
> >  - Rebased onto kernel/git/geert/renesas-drivers.git
> 
> Thanks for the update!
> 
> >  drivers/clk/renesas/r9a06g032-clocks.c | 243
> > ++++++++++++++++++++++++---------
> >  1 file changed, 176 insertions(+), 67 deletions(-)
> 
> Please also update
> Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt,
> to describe #power-domain-cells (must be 0), and to update the provider
> and consumer examples.
> 
> > --- a/drivers/clk/renesas/r9a06g032-clocks.c
> > +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> 
> > @@ -28,6 +30,7 @@ struct r9a06g032_gate {
> >  /* This is used to describe a clock for instantiation */  struct
> > r9a06g032_clkdesc {
> >         const char *name;
> > +       bool managed;
> 
> This flag can be integrated into the bit field below, avoiding a size increase.
> 
> >         uint32_t type: 3;
> >         uint32_t index: 8;
> >         uint32_t source : 8; /* source index + 1 (0 == none) */ @@
> > -60,7 +63,11 @@ struct r9a06g032_clkdesc {  #define D_GATE(_idx, _n,
> > _src, ...) \
> >         { .type = K_GATE, .index = R9A06G032_##_idx, \
> >                 .source = 1 + R9A06G032_##_src, .name = _n, \
> > -               .gate = I_GATE(__VA_ARGS__), }
> > +               .managed = 0, .gate = I_GATE(__VA_ARGS__) }
> 
> No need to initialize static variable fields to zero.
> 
> > +#define D_MODULE(_idx, _n, _src, ...) \
> > +       { .type = K_GATE, .index = R9A06G032_##_idx, \
> > +               .source = 1 + R9A06G032_##_src, .name = _n, \
> > +               .managed = 1, .gate = I_GATE(__VA_ARGS__) }
> >  #define D_ROOT(_idx, _n, _mul, _div) \
> >         { .type = K_FFC, .index = R9A06G032_##_idx, .name = _n, \
> >                 .div = _div, .mul = _mul } @@ -170,7 +177,7 @@ static
> > const struct r9a06g032_clkdesc r9a06g032_clocks[] __initconst = {
> >         D_GATE(CLK_P6_PG2, "clk_p6_pg2", DIV_P6_PG, 0x8a3, 0x8a4, 0x8a5,
> 0, 0xb61, 0, 0),
> >         D_GATE(CLK_P6_PG3, "clk_p6_pg3", DIV_P6_PG, 0x8a6, 0x8a7, 0x8a8,
> 0, 0xb62, 0, 0),
> >         D_GATE(CLK_P6_PG4, "clk_p6_pg4", DIV_P6_PG, 0x8a9, 0x8aa, 0x8ab,
> 0, 0xb63, 0, 0),
> > -       D_GATE(CLK_PCI_USB, "clk_pci_usb", CLKOUT_D40, 0xe6, 0, 0, 0, 0, 0,
> 0),
> > +       D_MODULE(CLK_PCI_USB, "clk_pci_usb", CLKOUT_D40, 0xe6, 0, 0,
> > + 0, 0, 0, 0),
> 
> Is this correct? All other module clocks are HCLK_*?
> You do have separate module clocks for USB host/function/power
> management below?
The naming is different here to keep consistency with the manuals, 
however the USB PCI clock was added as a D_MODULE because it
does not need to be enabled at all times. CLK_PCI_USB only needs
to be enabled when the USB host is active, based on Figure 10.13
(Host Controller Power-Down Flow) in the RZN1 System Manual.
> 
> >         D_GATE(CLK_QSPI0, "clk_qspi0", DIV_QSPI0, 0x2a4, 0x2a5, 0, 0, 0, 0, 0),
> >         D_GATE(CLK_QSPI1, "clk_qspi1", DIV_QSPI1, 0x484, 0x485, 0, 0, 0, 0, 0),
> >         D_GATE(CLK_RGMII_REF, "clk_rgmii_ref", CLKOUT_D8, 0x340, 0, 0,
> > 0, 0, 0, 0), @@ -187,17 +194,17 @@ static const struct r9a06g032_clkdesc
> r9a06g032_clocks[] __initconst = {
> >         D_GATE(CLK_SPI5, "clk_spi5", DIV_P4_PG, 0x822, 0x823, 0, 0, 0, 0, 0),
> >         D_GATE(CLK_SWITCH, "clk_switch", DIV_SWITCH, 0x982, 0x983, 0, 0, 0,
> 0, 0),
> >         D_DIV(DIV_MOTOR, "div_motor", CLKOUT_D5, 84, 2, 8),
> > -       D_GATE(HCLK_ECAT125, "hclk_ecat125", CLKOUT_D8, 0x400, 0x401, 0,
> 0x402, 0, 0x440, 0x441),
> > -       D_GATE(HCLK_PINCONFIG, "hclk_pinconfig", CLKOUT_D40, 0x740,
> 0x741, 0x742, 0, 0xae0, 0, 0),
> > -       D_GATE(HCLK_SERCOS, "hclk_sercos", CLKOUT_D10, 0x420, 0x422, 0,
> 0x421, 0, 0x460, 0x461),
> > -       D_GATE(HCLK_SGPIO2, "hclk_sgpio2", DIV_P5_PG, 0x8c3, 0x8c4,
> 0x8c5, 0, 0xb41, 0, 0),
> > -       D_GATE(HCLK_SGPIO3, "hclk_sgpio3", DIV_P5_PG, 0x8c6, 0x8c7,
> 0x8c8, 0, 0xb42, 0, 0),
> > -       D_GATE(HCLK_SGPIO4, "hclk_sgpio4", DIV_P5_PG, 0x8c9, 0x8ca,
> 0x8cb, 0, 0xb43, 0, 0),
> > -       D_GATE(HCLK_TIMER0, "hclk_timer0", CLKOUT_D40, 0x743, 0x744,
> 0x745, 0, 0xae1, 0, 0),
> > -       D_GATE(HCLK_TIMER1, "hclk_timer1", CLKOUT_D40, 0x746, 0x747,
> 0x748, 0, 0xae2, 0, 0),
> > -       D_GATE(HCLK_USBF, "hclk_usbf", CLKOUT_D8, 0xe3, 0, 0, 0xe4, 0,
> 0x102, 0x103),
> > -       D_GATE(HCLK_USBH, "hclk_usbh", CLKOUT_D8, 0xe0, 0xe1, 0, 0xe2, 0,
> 0x100, 0x101),
> > -       D_GATE(HCLK_USBPM, "hclk_usbpm", CLKOUT_D8, 0xe5, 0, 0, 0, 0, 0,
> 0),
> 
> ... here.
HCLK_USBPM needs to be enabled at all times because it is used for VBUS detection
and wakeup for both the USB host and function. Therefore D_GATE was used.

USB_HCLKF is a bus clock used by the USB function, although it can technically be
turned off whilst waiting for VBUS detection (system manual:
"10.6.6.2 VBUS Detection"), our USB function driver does not currently implement
PM. I will update this to a module in the next version as I see no issue doing
so conceptually or through testing.

> 
> > +       D_MODULE(HCLK_ECAT125, "hclk_ecat125", CLKOUT_D8, 0x400,
> 0x401, 0, 0x402, 0, 0x440, 0x441),
> > +       D_MODULE(HCLK_PINCONFIG, "hclk_pinconfig", CLKOUT_D40, 0x740,
> 0x741, 0x742, 0, 0xae0, 0, 0),
> > +       D_MODULE(HCLK_SERCOS, "hclk_sercos", CLKOUT_D10, 0x420, 0x422,
> 0, 0x421, 0, 0x460, 0x461),
> > +       D_MODULE(HCLK_SGPIO2, "hclk_sgpio2", DIV_P5_PG, 0x8c3, 0x8c4,
> 0x8c5, 0, 0xb41, 0, 0),
> > +       D_MODULE(HCLK_SGPIO3, "hclk_sgpio3", DIV_P5_PG, 0x8c6, 0x8c7,
> 0x8c8, 0, 0xb42, 0, 0),
> > +       D_MODULE(HCLK_SGPIO4, "hclk_sgpio4", DIV_P5_PG, 0x8c9, 0x8ca,
> 0x8cb, 0, 0xb43, 0, 0),
> > +       D_MODULE(HCLK_TIMER0, "hclk_timer0", CLKOUT_D40, 0x743, 0x744,
> 0x745, 0, 0xae1, 0, 0),
> > +       D_MODULE(HCLK_TIMER1, "hclk_timer1", CLKOUT_D40, 0x746, 0x747,
> 0x748, 0, 0xae2, 0, 0),
> > +       D_MODULE(HCLK_USBF, "hclk_usbf", CLKOUT_D8, 0xe3, 0, 0, 0xe4, 0,
> 0x102, 0x103),
> > +       D_MODULE(HCLK_USBH, "hclk_usbh", CLKOUT_D8, 0xe0, 0xe1, 0,
> 0xe2, 0, 0x100, 0x101),
> > +       D_MODULE(HCLK_USBPM, "hclk_usbpm", CLKOUT_D8, 0xe5, 0, 0, 0,
> > + 0, 0, 0),
> >         D_GATE(CLK_48_PG_F, "clk_48_pg_f", CLK_48, 0x78c, 0x78d, 0, 0x78e,
> 0, 0xb04, 0xb05),
> >         D_GATE(CLK_48_PG4, "clk_48_pg4", CLK_48, 0x789, 0x78a, 0x78b, 0,
> 0xb03, 0, 0),
> >         D_FFC(CLK_DDRPHY_PLLCLK_D4, "clk_ddrphy_pllclk_d4",
> > CLK_DDRPHY_PLLCLK, 4),
> 
> > @@ -344,6 +351,104 @@ struct r9a06g032_clk_gate {
> >
> >  #define to_r9a06g032_gate(_hw) container_of(_hw, struct
> > r9a06g032_clk_gate, hw)
> >
> > +struct r9a06g032_clk_domain {
> > +       struct generic_pm_domain genpd;
> > +       struct device_node *np;
> 
> np is unused (but see below), and unneeded, as nowadays you can use
> generic_pm_domain.dev.of_node instead.  Hence you can just use struct
> generic_pm_domain directly, instead of wrapping it.
> 
> > +};
> > +
> > +static struct r9a06g032_clk_domain *r9a06g032_clk_domain;
> 
> I don't think this singleton is needed, as there's no interaction with a second
> PM Domain driver (renesas-cpg-mssr interacts with rcar-sysc).
> 
> > +static int create_add_module_clock(struct of_phandle_args *clkspec,
> > +                                  struct device *dev) {
> > +       struct clk *clk;
> > +       int error = 0;
> > +
> > +       clk = of_clk_get_from_provider(clkspec);
> > +
> 
> Please drop this blank line.
> 
> > +       if (IS_ERR(clk))
> > +               return PTR_ERR(clk);
> > +
> > +       error = pm_clk_create(dev);
> > +       if (error) {
> > +               dev_err(dev, "pm_clk_create failed %d\n", error);
> 
> This can only happen when running out of memory, so no need to print a
> message.
> 
> > +               clk_put(clk);
> > +               return error;
> > +       }
> > +
> > +       error = pm_clk_add_clk(dev, clk);
> > +       if (error) {
> > +               dev_err(dev, "pm_clk_add_clk %pC failed %d\n", clk,
> > + error);
> 
> Likewise, I think.
> 
> > +               pm_clk_destroy(dev);
> > +               clk_put(clk);
> > +       }
> > +
> > +       return error;
> > +}
> > +
> > +int __init r9a06g032_attach_dev(struct generic_pm_domain *unused,
> 
> Missing static.
> Please drop the __init, as devices can be attached anytime (no section
> mismatch warnings?).
Because the clock array used __initconst, I only got a section mismatch warning 
without __init in the attach function. I will remove both so it compiles cleanly 
without expecting devices to be attached at one point.
> 
> > +                               struct device *dev) {
> > +       struct r9a06g032_clk_domain *pd = r9a06g032_clk_domain;
> 
> You can just use the passed generic_pm_domain instead...
> 
> > +       struct device_node *np = dev->of_node;
> > +       struct of_phandle_args clkspec;
> > +       int i = 0;
> > +       int error;
> > +
> > +       if (!pd) {
> 
> ... and this check is no longer needed.
> 
> > +               dev_dbg(dev, "RZN1 clock domain not yet available\n");
> > +               return -EPROBE_DEFER;
> 
> 
> > +       }
> > +
> > +       while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
> > +                                          &clkspec)) {
> 
> As an entry in a clocks property may refer to a different type of clock (e.g. an
> external crystal), you need to add a check here, to make sure the clkspec
> refers to a clock handled by this driver:
> 
>     if (clkspec->np != pd->genpd.dev.of_node)
>             continue
> 
> > +               int index = clkspec.args[0];
> > +
> > +               if (index < R9A06G032_CLOCK_COUNT &&
> > +                   r9a06g032_clocks[index].managed) {
> > +                       error = create_add_module_clock(&clkspec,
> > + dev);
> > +
> > +                       if (error)
> > +                               return error;
> > +
> > +                       of_node_put(clkspec.np);
> 
> Please move this line up, before the error check, to avoid a reference leak.
> 
> > +               }
> > +               i++;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +void r9a06g032_detach_dev(struct generic_pm_domain *unused, struct
> > +device *dev)
> 
> Missing static.
> 
> > +{
> > +       if (!pm_clk_no_clocks(dev))
> > +               pm_clk_destroy(dev);
> > +}
> > +
> > +static int __init r9a06g032_add_clk_domain(struct device *dev) {
> > +       struct device_node *np = dev->of_node;
> > +       struct generic_pm_domain *genpd;
> > +       struct r9a06g032_clk_domain *pd;
> 
> Just struct generic_pm_domain *pd, and s/genpd/pd/ below.
> 
> > +
> > +       pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > +       if (!pd)
> > +               return -ENOMEM;
> > +
> > +       pd->np = np;
> 
> Not needed.
> 
> > +
> > +       genpd = &pd->genpd;
> 
> Not needed.
> 
> > +       genpd->name = np->name;
> > +       genpd->flags = GENPD_FLAG_PM_CLK |
> GENPD_FLAG_ACTIVE_WAKEUP;
> > +       genpd->attach_dev = r9a06g032_attach_dev;
> > +       genpd->detach_dev = r9a06g032_detach_dev;
> > +       pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
> > +       r9a06g032_clk_domain = pd;
> 
> Not needed.
> 
> > +
> > +       of_genpd_add_provider_simple(np, genpd);
> > +       return 0;
> > +}
> > +
> >  static void
> >  r9a06g032_clk_gate_set(struct r9a06g032_priv *clocks,
> >                        struct r9a06g032_gate *g, int on) @@ -870,6
> > +975,10 @@ static int __init r9a06g032_clocks_probe(struct
> platform_device *pdev)
> >         if (error)
> >                 return error;
> >
> > +       error = r9a06g032_add_clk_domain(dev);
> 
> I would do this after the call to devm_add_action_or_reset() below.
> 
> > +       if (error)
> > +               return error;
> > +
> >         return devm_add_action_or_reset(dev,
> >
> > r9a06g032_clocks_del_clk_provider, np);  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-
> 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

Kind Regards,

Gareth Williams

Powered by blists - more mailing lists