| Message ID | 20240401-wpcm-clk-v11-0-379472961244@gmx.net |
|---|---|
| Headers | show |
| Series | Nuvoton WPCM450 clock and reset driver | expand |
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
This series adds support for the clock and reset controller in the Nuvoton WPCM450 SoC. This means that the clock rates for peripherals will be calculated automatically based on the clock tree as it was preconfigured by the bootloader. The 24 MHz dummy clock, that is currently in the devicetree, is no longer needed. Somewhat unfortunately, this also means that there is a breaking change once the devicetree starts relying on the clock driver, but I find it acceptable in this case, because WPCM450 is still at a somewhat early stage. v11: - Improved description in "ARM: dts: wpcm450: Remove clock-output-names from reference clock node" - some minor format differences due to switching to B4 v10: - A small tweak (using selected instead of extending an already-long default line) in Kconfig, for better robustness v9: - Various improvements to the driver - No longer use global clock names (and the clock-output-names property) to refer to the reference clock, but instead rely on a phandle reference v8: - https://lore.kernel.org/lkml/20230428190226.1304326-1-j.neuschaefer@gmx.net/ - Use %pe throughout the driver v7: - Simplified the error handling, by largely removing resource deallocation, which: - was already incomplete - would only happen in a case when the system is in pretty bad state because the clock driver didn't initialize correctly (in other words, the clock driver isn't optional enough that complex error handling really pays off) v6: - Dropped all patches except the clock binding and the clock driver, because they have mostly been merged - Minor correction to how RESET_SIMPLE is selected v5: - Dropped patch 2 (watchdog: npcm: Enable clock if provided), which was since merged upstream - Added patch 2 (clocksource: timer-npcm7xx: Enable timer 1 clock before use) again, because I wasn't able to find it in linux-next - Switched the driver to using struct clk_parent_data - Rebased on 6.1-rc3 v4: - Leave WDT clock running during after restart handler - Fix reset controller initialization - Dropped patch 2/7 (clocksource: timer-npcm7xx: Enable timer 1 clock before use), as it was applied by Daniel Lezcano v3: - https://lore.kernel.org/lkml/20220508194333.2170161-1-j.neuschaefer@gmx.net/ - Changed "refclk" string to "ref" - Fixed some dead code in the driver - Added clk_prepare_enable call to the watchdog restart handler - Added a few review tags v2: - https://lore.kernel.org/lkml/20220429172030.398011-1-j.neuschaefer@gmx.net/ - various small improvements v1: - https://lore.kernel.org/lkml/20220422183012.444674-1-j.neuschaefer@gmx.net/ Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> --- Jonathan Neuschäfer (4): dt-bindings: clock: Add Nuvoton WPCM450 clock/reset controller ARM: dts: wpcm450: Remove clock-output-names from reference clock node clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver ARM: dts: wpcm450: Switch clocks to clock controller .../bindings/clock/nuvoton,wpcm450-clk.yaml | 65 ++++ arch/arm/boot/dts/nuvoton/nuvoton-wpcm450.dtsi | 23 +- drivers/clk/Makefile | 2 +- drivers/clk/nuvoton/Kconfig | 10 +- drivers/clk/nuvoton/Makefile | 1 + drivers/clk/nuvoton/clk-wpcm450.c | 372 +++++++++++++++++++++ include/dt-bindings/clock/nuvoton,wpcm450-clk.h | 67 ++++ 7 files changed, 525 insertions(+), 15 deletions(-) --- base-commit: 4cece764965020c22cff7665b18a012006359095 change-id: 20240330-wpcm-clk-222a37f59cfb Best regards, -- Jonathan Neuschäfer <j.neuschaefer@gmx.net>