Re: [PATCH v2] clk: renesas: r9a06g032: Add clock domain support

From: Geert Uytterhoeven
Date: Wed May 22 2019 - 08:04:48 EST

Next message: Vidya Sagar: "Re: [PATCH V7 14/15] PCI: tegra: Add Tegra194 PCIe support"
Previous message: Jagan Teki: "Re: [linux-sunxi] [PATCH 3/6] drm/sun4i: dsi: Add bridge support"
In reply to: Gareth Williams: "[PATCH v2] clk: renesas: r9a06g032: Add clock domain support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

> 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.

> + 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?).

> + 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@xxxxxxxxxxxxxx

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

Next message: Vidya Sagar: "Re: [PATCH V7 14/15] PCI: tegra: Add Tegra194 PCIe support"
Previous message: Jagan Teki: "Re: [linux-sunxi] [PATCH 3/6] drm/sun4i: dsi: Add bridge support"
In reply to: Gareth Williams: "[PATCH v2] clk: renesas: r9a06g032: Add clock domain support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]