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

[Jonathan Neuschäfer]

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