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@xxxxxxx>
> > Reviewed-by: Joel Stanley <joel@xxxxxxxxx>
> > ---
> >
> > 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@xxxxxxx>
> > + */
> > +
> > +#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
Attachment:
signature.asc
Description: PGP signature
