From: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>
To: Stephen Boyd <sboyd@kernel.org>
Cc: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
openbmc@lists.ozlabs.org,
"Michael Turquette" <mturquette@baylibre.com>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Joel Stanley" <joel@jms.id.au>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v11 3/4] clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver
Date: Tue, 16 Apr 2024 11:54:59 +0200 [thread overview]
Message-ID: <Zh5K8_FuUtPq7Pqj@probook> (raw)
In-Reply-To: <9be144291cda6d9714252c9cd83649c2.sboyd@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 7732 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-04-16 9:55 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-01 14:06 [PATCH v11 0/4] Nuvoton WPCM450 clock and reset driver Jonathan Neuschäfer
2024-04-01 14:06 ` [PATCH v11 1/4] dt-bindings: clock: Add Nuvoton WPCM450 clock/reset controller Jonathan Neuschäfer
2024-04-01 14:06 ` [PATCH v11 2/4] ARM: dts: wpcm450: Remove clock-output-names from reference clock node Jonathan Neuschäfer
2024-04-01 14:06 ` [PATCH v11 3/4] clk: wpcm450: Add Nuvoton WPCM450 clock/reset controller driver Jonathan Neuschäfer
2024-04-12 7:25 ` Stephen Boyd
2024-04-16 9:54 ` Jonathan Neuschäfer [this message]
2024-04-01 14:06 ` [PATCH v11 4/4] ARM: dts: wpcm450: Switch clocks to clock controller Jonathan Neuschäfer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zh5K8_FuUtPq7Pqj@probook \
--to=j.neuschaefer@gmx.net \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=joel@jms.id.au \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=openbmc@lists.ozlabs.org \
--cc=p.zabel@pengutronix.de \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).