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@xxxxxxx> > > > > +L: linux-riscv@xxxxxxxxxxxxxxxxxxx > > > > +L: linux-clk@xxxxxxxxxxxxxxx (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
