Re: [PATCH v4 11/21] riscv: Add Canaan Kendryte K210 clock driver

[Damien Le Moal <Damien.LeMoal@xxxxxxx>]

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