RE: [PATCH v2] clk: renesas: r9a06g032: Add clock domain support
From: Gareth Williams
Date: Fri May 24 2019 - 08:31:07 EST
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@xxxxxxxxxxxxxx> wrote:
> Hi Gareth,
>
> On Tue, May 21, 2019 at 2:35 PM Gareth Williams
> <gareth.williams.jx@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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@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
Kind Regards,
Gareth Williams