From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: "linux-riscv@lists.infradead.org"
<linux-riscv@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
"palmer@dabbelt.com" <palmer@dabbelt.com>,
"sboyd@kernel.org" <sboyd@kernel.org>,
"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Cc: "seanga2@gmail.com" <seanga2@gmail.com>
Subject: Re: [PATCH v4 11/21] riscv: Add Canaan Kendryte K210 clock driver
Date: Mon, 7 Dec 2020 03:50:54 +0000 [thread overview]
Message-ID: <b5eb9c289fe58119185550bff7228501d95b730e.camel@wdc.com> (raw)
In-Reply-To: <160714919628.1580929.1456162330322523777@swboyd.mtv.corp.google.com>
Hi Stephen,
I prepared a v5 series addressing your comments (and other comments).
I will post that later today after some more tests.
Below are some comments on how I addressed some of your remarks.
On Fri, 2020-12-04 at 22:19 -0800, Stephen Boyd wrote:
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/clock/canaan,k210-clk.yaml
> > +F: drivers/clk/clk-k210.c
> > diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> > index 88ac0d1a5da4..f2f9633087d1 100644
> > --- a/arch/riscv/Kconfig.socs
> > +++ b/arch/riscv/Kconfig.socs
> > @@ -29,6 +29,8 @@ config SOC_CANAAN
> > select SERIAL_SIFIVE if TTY
> > select SERIAL_SIFIVE_CONSOLE if TTY
> > select SIFIVE_PLIC
> > + select SOC_K210_SYSCTL
> > + select CLK_K210
>
> Any reason to do this vs. just make it the default?
I understood what you meant and added the change.
> > diff --git a/drivers/clk/clk-k210.c b/drivers/clk/clk-k210.c
> > new file mode 100644
> > index 000000000000..95d830a38911
> > --- /dev/null
> > +++ b/drivers/clk/clk-k210.c
> > @@ -0,0 +1,959 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019-20 Sean Anderson <seanga2@gmail.com>
> > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > + */
> > +#define pr_fmt(fmt) "k210-clk: " fmt
> > +
> > +#include <linux/io.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/clk.h>
>
> Preferably this include is dropped.
Fixed (see comment below about "in0" handling).
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
>
> Is this used? Hopefully no.
Removed it, it was not needed.
> > +
> > + while (true) {
> > + reg = readl(pll->lock);
> > + if ((reg & mask) == mask)
> > + break;
> > +
> > + reg |= BIT(pll->lock_shift + K210_PLL_CLEAR_SLIP);
> > + writel(reg, pll->lock);
>
> Is this readl_poll_timeout?
As mentioned in a previous email, using readl_poll_timeout() make the code
harder since for each poll loop the CLEAR SLIP bit needs to be set. So I kept
this as is.
> > +static void k210_pll_enable_hw(struct k210_pll *pll)
> > +{
> > + struct k210_pll_cfg *pll_cfg = &k210_plls_cfg[pll->id];
> > + unsigned long flags;
> > + u32 reg;
> > +
> > + spin_lock_irqsave(&kcl->clk_lock, flags);
> > +
> > + if (k210_pll_hw_is_enabled(pll))
> > + goto unlock;
> > +
> > + if (pll->id == K210_PLL0) {
> > + /* Re-parent aclk to IN0 to keep the CPUs running */
> > + k210_aclk_set_selector(0);
> > + }
> > +
> > + /* Set factors */
> > + reg = readl(pll->reg);
> > + reg &= ~GENMASK(19, 0);
> > + reg |= FIELD_PREP(K210_PLL_CLKR, pll_cfg->r);
> > + reg |= FIELD_PREP(K210_PLL_CLKF, pll_cfg->f);
> > + reg |= FIELD_PREP(K210_PLL_CLKOD, pll_cfg->od);
> > + reg |= FIELD_PREP(K210_PLL_BWADJ, pll_cfg->bwadj);
> > + reg |= K210_PLL_PWRD;
> > + writel(reg, pll->reg);
> > +
> > + /* Ensure reset is low before asserting it */
> > + reg &= ~K210_PLL_RESET;
> > + writel(reg, pll->reg);
> > + reg |= K210_PLL_RESET;
> > + writel(reg, pll->reg);
> > + nop();
> > + nop();
>
> Are these nops needed for some reason? Any comment to add here? It's
> basically non-portable code and hopefully nothing is inserted into that
> writel function that shouldn't be there.
I kept this as is since this function is called from k210_clk_early_init() (SoC
early initialization, before the DTB is parsed). From that context, delay()
functions cannot be used. Since this driver is very specific to this RISC-V
SoC, I do not think there is any portability issue.
> > +static u32 k210_clk_get_div_val(struct k210_clk_cfg *kclk)
> > +{
> > + u32 reg = readl(kcl->regs + kclk->div_reg);
> > +
> > + return (reg >> kclk->div_shift) & GENMASK(kclk->div_width - 1, 0);
>
> Use FIELD_GET()?
Unfortunately, FIELD_GET() requires a mask that is a constant, which is not the
case here.
> > + in0_clk = of_clk_get(np, 0);
> > + if (IS_ERR(in0_clk)) {
> > + pr_warn("%pOFP: in0 oscillator not found\n", np);
> > + hws[K210_CLK_IN0] =
> > + clk_hw_register_fixed_rate(NULL, "in0", NULL,
> > + 0, K210_IN0_RATE);
> > + } else {
> > + hws[K210_CLK_IN0] = __clk_get_hw(in0_clk);
> > + }
> > + if (IS_ERR(hws[K210_CLK_IN0])) {
> > + pr_err("%pOFP: failed to get base oscillator\n", np);
> > + goto err;
> > + }
> > +
> > + in0 = clk_hw_get_name(hws[K210_CLK_IN0]);
> > + aclk_parents[0] = in0;
> > + pll_parents[0] = in0;
> > + mux_parents[0] = in0;
>
> Can we use the new way of specifying clk parents so that we don't have
> to use __clk_get_hw(), of_clk_get(), and clk_hw_get_name()? Hopefully
> the core can handl that all instead of this driver.
I removed all this by adding:
clock-output-names = "in0";
to the DT fixed-rate oscillator clock node (and documented that too). Doing so,
clk_hw_get_name(), __clk_get_hw() and of_clk_get() are not needed anymore and
the parents clock names arrays do not need run-time update.
>
> > +
> > + /* PLLs */
> > + hws[K210_CLK_PLL0] =
> > + k210_register_pll(K210_PLL0, "pll0", pll_parents, 1, 0);
> > + hws[K210_CLK_PLL1] =
> > + k210_register_pll(K210_PLL1, "pll1", pll_parents, 1, 0);
> > + hws[K210_CLK_PLL2] =
> > + k210_register_pll(K210_PLL2, "pll2", pll_parents, 3, 0);
> > +
> > + /* aclk: muxed of in0 and pll0_d, no gate */
> > + hws[K210_CLK_ACLK] = k210_register_aclk();
> > +
> > + /*
> > + * Clocks with aclk as source: the CPU clock is obviously critical.
> > + * So is the CLINT clock as the scheduler clocksource.
> > + */
> > + hws[K210_CLK_CPU] =
> > + k210_register_clk(K210_CLK_CPU, "cpu", "aclk", CLK_IS_CRITICAL);
> > + hws[K210_CLK_CLINT] =
> > + clk_hw_register_fixed_factor(NULL, "clint", "aclk",
> > + CLK_IS_CRITICAL, 1, 50);
>
> Is anyone getting these clks? It's nice and all to model things in the
> clk framework but if they never have a consumer then it is sort of
> useless and just wastes memory and causes more overhead.
I dropped the CLINT clock as it is not needed by the clint driver and a clock
property is not documented for it. I kept the CPU clock as it is referenced by
the uarths (serial console) driver.
I also removed IN0, the PLLs and ACLK from the clock data array since these are
not referenced in the device tree.
> > +CLK_OF_DECLARE_DRIVER(k210_clk, "canaan,k210-clk", k210_clk_init);
>
> Is this needed or can this just be a plain platform driver? If something
> is needed early for a clocksource or clockevent then the driver can be
> split to register those few clks early from this hook and then register
> the rest later when the platform device probes. That's what
> CLK_OF_DECLARE_DRIVER is for. A DECLARE_DRIVER without a platform driver
> is incorrect.
I tried to split the driver into the early init and platform driver parts but
there is a circular dependency with the sysctl driver: sysctl defines the k210-
clk node (for the regmap) but sysctl has a reference to its advanced power bus
clock too. This dependencies cannot be resolved elegantly. So I changed the
driver declaration to use CLK_OF_DECLARE() instead of CLK_OF_DECLARE_DRIVER().
Thanks !
--
Damien Le Moal
Western Digital
next prev parent reply other threads:[~2020-12-07 3:52 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 3:24 [PATCH v4 00/21] RISC-V Kendryte K210 support improvements Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 01/21] riscv: Fix kernel time_init() Damien Le Moal
2020-12-05 5:41 ` Stephen Boyd
2020-12-02 3:24 ` [PATCH v4 02/21] riscv: Fix sifive serial driver Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 03/21] riscv: Enable interrupts during syscalls with M-Mode Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 04/21] riscv: Fix builtin DTB handling Damien Le Moal
2020-12-04 15:11 ` Geert Uytterhoeven
2020-12-02 3:24 ` [PATCH v4 05/21] riscv: Use vendor name for K210 SoC support Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 06/21] dt-bindings: Add Canaan vendor prefix Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 07/21] dt-binding: clock: Document canaan,k210-clk bindings Damien Le Moal
2020-12-05 5:46 ` Stephen Boyd
2020-12-02 3:24 ` [PATCH v4 08/21] dt-bindings: reset: Document canaan,k210-rst bindings Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 09/21] dt-bindings: pinctrl: Document canaan,k210-fpioa bindings Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 10/21] dt-binding: mfd: Document canaan,k210-sysctl bindings Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 11/21] riscv: Add Canaan Kendryte K210 clock driver Damien Le Moal
2020-12-05 6:19 ` Stephen Boyd
2020-12-05 7:43 ` Damien Le Moal
2020-12-05 13:43 ` Sean Anderson
2020-12-05 14:13 ` Damien Le Moal
[not found] ` <160736612827.1580929.7802371235304556600@swboyd.mtv.corp.google.com>
2020-12-08 7:48 ` Damien Le Moal
2020-12-07 3:50 ` Damien Le Moal [this message]
2020-12-07 8:43 ` Geert Uytterhoeven
2020-12-07 9:55 ` Damien Le Moal
2020-12-07 10:06 ` Geert Uytterhoeven
2020-12-07 10:14 ` Damien Le Moal
2020-12-07 22:58 ` Sean Anderson
2020-12-07 11:34 ` Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 12/21] riscv: Add Canaan Kendryte K210 FPIOA driver Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 13/21] riscv: Add Canaan Kendryte K210 reset controller Damien Le Moal
2020-12-04 10:49 ` Philipp Zabel
2020-12-04 11:16 ` Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 14/21] riscv: Update Canaan Kendryte K210 device tree Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 15/21] riscv: Add SiPeed MAIX BiT board " Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 16/21] riscv: Add SiPeed MAIX DOCK " Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 17/21] riscv: Add SiPeed MAIX GO " Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 18/21] riscv: Add SiPeed MAIXDUINO " Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 19/21] riscv: Add Kendryte KD233 " Damien Le Moal
2020-12-02 3:24 ` [PATCH v4 20/21] riscv: Update Canaan Kendryte K210 defconfig Damien Le Moal
2020-12-02 3:25 ` [PATCH v4 21/21] riscv: Add Canaan Kendryte K210 SD card defconfig Damien Le Moal
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=b5eb9c289fe58119185550bff7228501d95b730e.camel@wdc.com \
--to=damien.lemoal@wdc.com \
--cc=devicetree@vger.kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=p.zabel@pengutronix.de \
--cc=palmer@dabbelt.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=seanga2@gmail.com \
/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 NNTP newsgroup(s).