[v11,3/4] clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver

This driver implements the following features w.r.t. the clock and reset
controller in the WPCM450 SoC:

- It calculates the rates for all clocks managed by the clock controller
- It leaves the clock tree mostly unchanged, except that it enables/
  disables clock gates based on usage.
- It exposes the reset lines managed by the controller using the
  Generic Reset Controller subsystem

NOTE: If the driver and the corresponding devicetree node are present,
      the driver will disable "unused" clocks. This is problem until
      the clock relations are properly declared in the devicetree (in a
      later patch). Until then, the clk_ignore_unused kernel parameter
      can be used as a workaround.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---

I have considered converting this driver to a platform driver instead of
using CLK_OF_DECLARE, because platform drivers are generally the way
forward. However, the timer-npcm7xx driver used on the same platform
requires is initialized with TIMER_OF_DECLARE and thus requires the
clocks to be available earlier than a platform driver can provide them.

v11:
- no changes

v10:
- select RESET_{CONTROLLER,SIMPLE} from CLK_WPCM450 instead of messing with the 'default' statement

v9:
- Apply comments made by Stephen Boyd
- Move to drivers/clk/nuvoton/ directory
- Update SPDX license identifier from GPL-2.0 to GPL-2.0-only
- Rename clk_np variable to np
- Use of_clk_hw_register
- Refer to clock parents by .fw_name

v8:
- https://lore.kernel.org/lkml/20230428190226.1304326-3-j.neuschaefer@gmx.net/
- Use %pe format specifier throughout the driver, as suggested by Philipp Zabel
- Add Joel's R-b

v7:
- https://lore.kernel.org/lkml/20230422220240.322572-3-j.neuschaefer@gmx.net/
- Simplify error handling by not deallocating resources

v6:
- Enable RESET_SIMPLE based on ARCH_WPCM450, not ARCH_NPCM, as suggested by Tomer Maimon

v5:
- https://lore.kernel.org/lkml/20221104161850.2889894-6-j.neuschaefer@gmx.net/
- Switch to using clk_parent_data

v4:
- Fix reset controller initialization

v3:
- Change reference clock name from "refclk" to "ref"
- Remove unused variable in return path of wpcm450_clk_register_pll
- Remove unused divisor tables

v2:
- no changes
---
 drivers/clk/Makefile              |   2 +-
 drivers/clk/nuvoton/Kconfig       |  10 +-
 drivers/clk/nuvoton/Makefile      |   1 +
 drivers/clk/nuvoton/clk-wpcm450.c | 372 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 383 insertions(+), 2 deletions(-)


--
2.43.0

Quoting Jonathan Neuschäfer (2024-04-01 07:06:32)
> This driver implements the following features w.r.t. the clock and reset
> controller in the WPCM450 SoC:
> 
> - It calculates the rates for all clocks managed by the clock controller
> - It leaves the clock tree mostly unchanged, except that it enables/
>   disables clock gates based on usage.
> - It exposes the reset lines managed by the controller using the
>   Generic Reset Controller subsystem
> 
> NOTE: If the driver and the corresponding devicetree node are present,
>       the driver will disable "unused" clocks. This is problem until
>       the clock relations are properly declared in the devicetree (in a
>       later patch). Until then, the clk_ignore_unused kernel parameter
>       can be used as a workaround.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> 
> I have considered converting this driver to a platform driver instead of
> using CLK_OF_DECLARE, because platform drivers are generally the way
> forward. However, the timer-npcm7xx driver used on the same platform
> requires is initialized with TIMER_OF_DECLARE and thus requires the
> clocks to be available earlier than a platform driver can provide them.

In that case you can use CLK_OF_DECLARE_DRIVER(), register the clks
needed for the timer driver to probe, and then put the rest of the clk
registration in a normal platform driver.

> diff --git a/drivers/clk/nuvoton/clk-wpcm450.c b/drivers/clk/nuvoton/clk-wpcm450.c
> new file mode 100644
> index 00000000000000..9100c4b8a56483
> --- /dev/null
> +++ b/drivers/clk/nuvoton/clk-wpcm450.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Nuvoton WPCM450 clock and reset controller driver.
> + *
> + * Copyright (C) 2022 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Isn't KBUILD_MODNAME an option already for dynamic debug?

> +
> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset/reset-simple.h>
> +#include <linux/slab.h>
> +
[...]
> +
> +static const struct clk_parent_data default_parents[] = {
> +       { .name = "pll0" },
> +       { .name = "pll1" },
> +       { .name = "ref" },
> +};
> +
> +static const struct clk_parent_data huart_parents[] = {
> +       { .fw_name = "ref" },
> +       { .name = "refdiv2" },

Please remove all .name elements and use indexes or direct pointers.

> +};
> +
> +static const struct wpcm450_clksel_data clksel_data[] = {
> +       { "cpusel", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 0, 2, -1, CLK_IS_CRITICAL },
> +       { "clkout", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 2, 2, -1, 0 },
> +       { "usbphy", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 6, 2, -1, 0 },
> +       { "uartsel", default_parents, ARRAY_SIZE(default_parents),
> +               parent_table, 8, 2, WPCM450_CLK_USBPHY, 0 },
> +       { "huartsel", huart_parents, ARRAY_SIZE(huart_parents),
> +               parent_table, 10, 1, -1, 0 },
> +};
> +
> +static const struct clk_div_table div_fixed2[] = {
> +       { .val = 0, .div = 2 },
> +       { }
> +};
> +
> +struct wpcm450_clkdiv_data {
> +       const char *name;
> +       struct clk_parent_data parent;
> +       int div_flags;
> +       const struct clk_div_table *table;
> +       int shift;
> +       int width;
> +       unsigned long flags;
> +};
> +
> +static struct wpcm450_clkdiv_data clkdiv_data_early[] = {
> +       { "refdiv2", { .name = "ref" }, 0, div_fixed2, 0, 0 },
> +};
> +
> +static const struct wpcm450_clkdiv_data clkdiv_data[] = {
> +       { "cpu", { .name = "cpusel" }, 0, div_fixed2, 0, 0, CLK_IS_CRITICAL },
> +       { "adcdiv", { .name = "ref" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 28, 2, 0 },
> +       { "apb", { .name = "ahb" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 26, 2, 0 },
> +       { "ahb", { .name = "cpu" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 24, 2, 0 },
> +       { "uart", { .name = "uartsel" }, 0, NULL, 16, 4, 0 },
> +       { "ahb3", { .name = "ahb" }, CLK_DIVIDER_POWER_OF_TWO, NULL, 8, 2, 0 },
> +};
> +
> +struct wpcm450_clken_data {
> +       const char *name;
> +       struct clk_parent_data parent;
> +       int bitnum;
> +       unsigned long flags;
> +};
> +
> +static const struct wpcm450_clken_data clken_data[] = {
> +       { "fiu", { .name = "ahb3" }, WPCM450_CLK_FIU, 0 },

This actually is  { .index = 0, .name = "ahb3" } and that is a bad
combination. struct clk_parent_data should only have .name as a fallback
when there's an old binding out there that doesn't have the 'clocks'
property for the clk provider node. There shouldn't be any .name
property because this is new code and a new binding.

> +       { "xbus", { .name = "ahb3" }, WPCM450_CLK_XBUS, 0 },
> +       { "kcs", { .name = "apb" }, WPCM450_CLK_KCS, 0 },
> +       { "shm", { .name = "ahb3" }, WPCM450_CLK_SHM, 0 },
> +       { "usb1", { .name = "ahb" }, WPCM450_CLK_USB1, 0 },
> +       { "emc0", { .name = "ahb" }, WPCM450_CLK_EMC0, 0 },
> +       { "emc1", { .name = "ahb" }, WPCM450_CLK_EMC1, 0 },
> +       { "usb0", { .name = "ahb" }, WPCM450_CLK_USB0, 0 },
> +       { "peci", { .name = "apb" }, WPCM450_CLK_PECI, 0 },
> +       { "aes", { .name = "apb" }, WPCM450_CLK_AES, 0 },
> +       { "uart0", { .name = "uart" }, WPCM450_CLK_UART0, 0 },
> +       { "uart1", { .name = "uart" }, WPCM450_CLK_UART1, 0 },
> +       { "smb2", { .name = "apb" }, WPCM450_CLK_SMB2, 0 },
> +       { "smb3", { .name = "apb" }, WPCM450_CLK_SMB3, 0 },
> +       { "smb4", { .name = "apb" }, WPCM450_CLK_SMB4, 0 },
> +       { "smb5", { .name = "apb" }, WPCM450_CLK_SMB5, 0 },
> +       { "huart", { .name = "huartsel" }, WPCM450_CLK_HUART, 0 },
> +       { "pwm", { .name = "apb" }, WPCM450_CLK_PWM, 0 },
> +       { "timer0", { .name = "refdiv2" }, WPCM450_CLK_TIMER0, 0 },
> +       { "timer1", { .name = "refdiv2" }, WPCM450_CLK_TIMER1, 0 },
> +       { "timer2", { .name = "refdiv2" }, WPCM450_CLK_TIMER2, 0 },
> +       { "timer3", { .name = "refdiv2" }, WPCM450_CLK_TIMER3, 0 },
> +       { "timer4", { .name = "refdiv2" }, WPCM450_CLK_TIMER4, 0 },
> +       { "mft0", { .name = "apb" }, WPCM450_CLK_MFT0, 0 },
> +       { "mft1", { .name = "apb" }, WPCM450_CLK_MFT1, 0 },
> +       { "wdt", { .name = "refdiv2" }, WPCM450_CLK_WDT, 0 },
> +       { "adc", { .name = "adcdiv" }, WPCM450_CLK_ADC, 0 },
> +       { "sdio", { .name = "ahb" }, WPCM450_CLK_SDIO, 0 },
> +       { "sspi", { .name = "apb" }, WPCM450_CLK_SSPI, 0 },
> +       { "smb0", { .name = "apb" }, WPCM450_CLK_SMB0, 0 },
> +       { "smb1", { .name = "apb" }, WPCM450_CLK_SMB1, 0 },
> +};
> +
> +static DEFINE_SPINLOCK(wpcm450_clk_lock);
> +
> +/*
> + * NOTE: Error handling is very rudimentary here. If the clock driver initial-
> + * ization fails, the system is probably in bigger trouble than what is caused

Don't break words across lines with hyphens.

> + * by a few leaked resources.
> + */
> +
> +static void __init wpcm450_clk_init(struct device_node *np)
> +{
> +       struct clk_hw_onecell_data *clk_data;
> +       static struct clk_hw **hws;
> +       static struct clk_hw *hw;
> +       void __iomem *clk_base;
> +       int i, ret;
> +       struct reset_simple_data *reset;
> +
> +       clk_base = of_iomap(np, 0);
> +       if (!clk_base) {
> +               pr_err("%pOFP: failed to map registers\n", np);
> +               of_node_put(np);
> +               return;
> +       }
> +       of_node_put(np);

The 'np' is used later when registering PLLs. You can only put the node
after it's no longer used. Also, you never got the node with
of_node_get(), so putting it here actually causes an underflow on the
refcount. Just remove all the get/puts instead.

> +
> +       clk_data = kzalloc(struct_size(clk_data, hws, WPCM450_NUM_CLKS), GFP_KERNEL);
> +       if (!clk_data)
> +               return;
> +
> +       clk_data->num = WPCM450_NUM_CLKS;
[...]
> +       /* Reset controller */
> +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> +       if (!reset)
> +               return;
> +       reset->rcdev.owner = THIS_MODULE;
> +       reset->rcdev.nr_resets = WPCM450_NUM_RESETS;
> +       reset->rcdev.ops = &reset_simple_ops;
> +       reset->rcdev.of_node = np;
> +       reset->membase = clk_base + REG_IPSRST;
> +       ret = reset_controller_register(&reset->rcdev);
> +       if (ret)
> +               pr_err("Failed to register reset controller: %pe\n", ERR_PTR(ret));

It would be nicer to register this device as an auxiliary device with a
single API call and then have all the resets exist in that file
instead of this file. The driver would be put in drivers/reset/ as well.

> +
> +       of_node_put(np);

Drop this of_node_put()

Hi,

On Fri, Apr 12, 2024 at 12:25:05AM -0700, Stephen Boyd wrote:
> Quoting Jonathan Neuschäfer (2024-04-01 07:06:32)
> > This driver implements the following features w.r.t. the clock and reset
> > controller in the WPCM450 SoC:
> > 
> > - It calculates the rates for all clocks managed by the clock controller
> > - It leaves the clock tree mostly unchanged, except that it enables/
> >   disables clock gates based on usage.
> > - It exposes the reset lines managed by the controller using the
> >   Generic Reset Controller subsystem
> > 
> > NOTE: If the driver and the corresponding devicetree node are present,
> >       the driver will disable "unused" clocks. This is problem until
> >       the clock relations are properly declared in the devicetree (in a
> >       later patch). Until then, the clk_ignore_unused kernel parameter
> >       can be used as a workaround.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > ---
> > 
> > I have considered converting this driver to a platform driver instead of
> > using CLK_OF_DECLARE, because platform drivers are generally the way
> > forward. However, the timer-npcm7xx driver used on the same platform
> > requires is initialized with TIMER_OF_DECLARE and thus requires the
> > clocks to be available earlier than a platform driver can provide them.
> 
> In that case you can use CLK_OF_DECLARE_DRIVER(), register the clks
> needed for the timer driver to probe, and then put the rest of the clk
> registration in a normal platform driver.

I'll give it a try. I'm not sure how to make it work correctly without
calling (devm_)of_clk_add_hw_provider twice, though (once for the early
clock, timer0; once for the rest).

Another (probably simpler) approach seems be to declare a fixed-clock or
fixed-factor-clock in the DT, and use that in the timer:

	refclk_div2: clock-div2 {
		compatible = "fixed-factor-clock";
		clocks = <&refclk>;
		#clock-cells = <0>;
		clock-mult = <1>;
		clock-div = <2>;
	};

	timer0: timer@b8001000 {
		compatible = "nuvoton,wpcm450-timer";
		interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
		reg = <0xb8001000 0x1c>;
		clocks = <&refclk_div2>;
	};

... and additionally to mark the timer clocks as critical in
clk-wpcm450.c, so they don't get disabled for being "unused".


> > diff --git a/drivers/clk/nuvoton/clk-wpcm450.c b/drivers/clk/nuvoton/clk-wpcm450.c
> > new file mode 100644
> > index 00000000000000..9100c4b8a56483
> > --- /dev/null
> > +++ b/drivers/clk/nuvoton/clk-wpcm450.c
> > @@ -0,0 +1,372 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Nuvoton WPCM450 clock and reset controller driver.
> > + *
> > + * Copyright (C) 2022 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> Isn't KBUILD_MODNAME an option already for dynamic debug?

Indeed, it's the +m option.

My motivation for setting pr_fmt in the first place should become
obsolete with the move towards a platform driver anyway, because then I
can use dev_err() etc. I'll remove the #define.

> 
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/reset/reset-simple.h>
> > +#include <linux/slab.h>
> > +
> [...]
> > +
> > +static const struct clk_parent_data default_parents[] = {
> > +       { .name = "pll0" },
> > +       { .name = "pll1" },
> > +       { .name = "ref" },
> > +};
> > +
> > +static const struct clk_parent_data huart_parents[] = {
> > +       { .fw_name = "ref" },
> > +       { .name = "refdiv2" },
> 
> Please remove all .name elements and use indexes or direct pointers.

Will do.

What I'm not yet sure about, is which of these is better:

 1. Having two kinds of indexes, 1. for internal use in the driver,
    identifying all clocks, 2. public as part of the devicetree binding
    ABI (defined in include/dt-bindings/clock/nuvoton,wpcm450-clk.h)
 2. Unifying the two and giving every clock a public index
 3. Using the same number space, but only providing public definitions
    (in the binding) for clocks that can be used outside the clock
    controller.

Option 3 sounds fairly reasonable.

> > +static const struct wpcm450_clken_data clken_data[] = {
> > +       { "fiu", { .name = "ahb3" }, WPCM450_CLK_FIU, 0 },
> 
> This actually is  { .index = 0, .name = "ahb3" } and that is a bad
> combination. struct clk_parent_data should only have .name as a fallback
> when there's an old binding out there that doesn't have the 'clocks'
> property for the clk provider node. There shouldn't be any .name
> property because this is new code and a new binding.

I'll try switching to .index or .hw instead for the references to
internal clocks.


[...]
> > +/*
> > + * NOTE: Error handling is very rudimentary here. If the clock driver initial-
> > + * ization fails, the system is probably in bigger trouble than what is caused
> 
> Don't break words across lines with hyphens.

Good point.

(Due to the switch to a platform driver, this comment will probably
become obsolete anyway.)

> > + * by a few leaked resources.
> > + */
> > +
> > +static void __init wpcm450_clk_init(struct device_node *np)
> > +{
> > +       struct clk_hw_onecell_data *clk_data;
> > +       static struct clk_hw **hws;
> > +       static struct clk_hw *hw;
> > +       void __iomem *clk_base;
> > +       int i, ret;
> > +       struct reset_simple_data *reset;
> > +
> > +       clk_base = of_iomap(np, 0);
> > +       if (!clk_base) {
> > +               pr_err("%pOFP: failed to map registers\n", np);
> > +               of_node_put(np);
> > +               return;
> > +       }
> > +       of_node_put(np);
> 
> The 'np' is used later when registering PLLs. You can only put the node
> after it's no longer used. Also, you never got the node with
> of_node_get(), so putting it here actually causes an underflow on the
> refcount. Just remove all the get/puts instead.

That simplifies it, thanks for the hint!

> > +
> > +       clk_data = kzalloc(struct_size(clk_data, hws, WPCM450_NUM_CLKS), GFP_KERNEL);
> > +       if (!clk_data)
> > +               return;
> > +
> > +       clk_data->num = WPCM450_NUM_CLKS;
> [...]
> > +       /* Reset controller */
> > +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> > +       if (!reset)
> > +               return;
> > +       reset->rcdev.owner = THIS_MODULE;
> > +       reset->rcdev.nr_resets = WPCM450_NUM_RESETS;
> > +       reset->rcdev.ops = &reset_simple_ops;
> > +       reset->rcdev.of_node = np;
> > +       reset->membase = clk_base + REG_IPSRST;
> > +       ret = reset_controller_register(&reset->rcdev);
> > +       if (ret)
> > +               pr_err("Failed to register reset controller: %pe\n", ERR_PTR(ret));
> 
> It would be nicer to register this device as an auxiliary device with a
> single API call and then have all the resets exist in that file
> instead of this file. The driver would be put in drivers/reset/ as well.

Not sure I'd move ten lines to a whole new file, but moving them to a
separate function definitely makes sense, I'll do that.

> 
> > +
> > +       of_node_put(np);
> 
> Drop this of_node_put()

Ok.


Thanks for your detailed review!

I'll send the next revision after testing my changes on the relevant
hardware (which I currently don't have with me).

-jn