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

[Damien Le Moal]

On Sat, 2020-12-05 at 08:43 -0500, Sean Anderson wrote:
> On 12/5/20 2:43 AM, Damien Le Moal wrote:
> > Hi Stephen,
> > 
> > Thank you for the review. I will address all your comments.
> > I just have a few questions below.
> > 
> > On Fri, 2020-12-04 at 22:19 -0800, Stephen Boyd wrote:
> > > Quoting Damien Le Moal (2020-12-01 19:24:50)
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 2daa6ee673f7..3da9a7a02f61 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -3822,6 +3822,22 @@ W:       https://github.com/Cascoda/ca8210-linux.git
> > > >   F:     Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
> > > >   F:     drivers/net/ieee802154/ca8210.c
> > > >   
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > +CANAAN/KENDRYTE K210 SOC CLOCK DRIVER
> > > > +M:     Damien Le Moal <damien.lemoal at wdc.com>
> > > > +L:     linux-riscv at lists.infradead.org
> > > > +L:     linux-clk at vger.kernel.org (clock driver)
> > > 
> > > Is this needed? I think we cover all of drivers/clk/ and bindings/clock
> > > already.
> > 
> > I was not sure about that so I added the entry. Will remove it.
> > 
> > > 
> > > > +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 do not understand here... Just selecting the drivers needed for the SoC here.
> > Is there any other way of doing this ?
> > 
> > [...]
> > > > +
> > > > +       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?
> > 
> > Oh. Yes, it is. I did not know about this function. Will change the code to use
> > it.
> 
> FWIW the timeout could be incorrect since we might be configuring a
> parent of ACLK. And realistically the only way this fails is if a user
> has edited this file and put in invalid PLL parameters. I don't think
> you gain much by adding a timeout.

readl_poll_timeout() allows a timeout of 0 for "no timeout". It is not easy to
use this macro due to the stop condition interface, which is not through a
callback. This makes the code very ugly to get the writel() call added in the
stop condition for each iteration of the poll loop. So I left the code as is.

> 
> > > 
> > > > +       }
> > > > +}
> > > > +
> > > > +static bool k210_pll_hw_is_enabled(struct k210_pll *pll)
> > > > +{
> > > > +       u32 reg = readl(pll->reg);
> > > > +       u32 mask = K210_PLL_PWRD | K210_PLL_EN;
> > > > +
> > > > +       if (reg & K210_PLL_RESET)
> > > > +               return false;
> > > > +
> > > > +       return (reg & mask) == mask;
> > > > +}
> > > > +
> > > > +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.
> > 
> > No clue... They are "magic" nops that are present in the K210 SDK from
> > Kendryte. I copied that, but do not actually know if they are really needed. I
> > am working without any specs for the hardware: the Kendryte SDK is my main
> > source of information here. I will try to remove them or just replace this with
> > a delay() call a nd see what happens.
> 
> Basically any delay should work as long as it takes more than 2
> instructions ;) Of course, anything longer than that just delays startup
> for no reason.

Removing the nop() does work. Not sure if that is solid though.
Any other xxdelay() call fails, including __delay() (CPU cycles). I guess
because at this early stage, there is no information yet on the CPU
frequency/timers and k210_clk_early_init() hangs. So I think I will keep the
nop(). This driver being only for this SoC, I do not think it is a big issue in
terms of portability, for now at least.


-- 
Damien Le Moal
Western Digital