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@xxxxxxxxx>
> > + * 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
