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

[Sean Anderson <seanga2@gmail.com>]

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@wdc.com>
>>> +L:     linux-riscv@lists.infradead.org
>>> +L:     linux-clk@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.

>>
>>> +       }
>>> +}
>>> +
>>> +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.

--Sean

> 
> [...]
>>> +static int k210_aclk_set_parent(struct clk_hw *hw, u8 index)
>>> +{
>>> +       if (WARN_ON(index > 1))
>>
>> Is this possible? What am I going to do as a user if this happens?
> 
> No, it is not possible. Will remove this. I could put a BUG_ON(), but I am not
> a fan this extreme.
> 
> [...]
>>> +       /*
>>> +        * in0 is the system base fixed-rate 26MHz oscillator which
>>> +        * should already be defined by the device tree. If it is not,
>>> +        * create it here.
>>
>> Are there old DTBs that don't have this? Sadface.
> 
> No, not any old DTB. Will remove that.
> 
>>
>>> +        */
>>> +       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.
> 
> Not sure what new way of specifying the parent you are referring to here.
> clk_hw_set_parent() ?
> 
>>
>>> +
>>> +       /* 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.
> 
> The CPU and SRAM clocks do not have any consumer, so I could remove them (just
> enable the HW but not represent them as clocks in the driver). There is no
> direct consumer of ACLK but it is the parent of multiple clocks, including the
> SRAM clocks. So it needs to be represented as a clock and kept alive even if
> all the peripheral drivers needing it are disabled. Otherwise, the system just
> stops (SRAM accesses hang).
> 
> [...]
>>> +       ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, kcl->clk_data);
>>> +       if (ret)
>>> +               pr_err("%pOFP: add clock provider failed %d\n", np, ret);
>>> +       else
>>> +               pr_info("%pOFP: CPU running at %lu MHz\n",
>>
>> Is this important? Is there a CPUfreq driver that runs and tells us the
>> boot CPU frequency instead? Doesn't feel like we care in the clk driver
>> about this.
> 
> There is no CPU freq driver that gives this frequency that I know of. That is
> why I added the message since the driver basically just comes up using the
> default HW settings for the SoC. CPU freq speed can be changed though by
> increasing the PLL freq. Just not supporting this for now as it is tricky to
> do: the SRAM clocks depend on aclk and PLL1 and if these are not the same
> value, the system hangs (most likely because we end up with the sram banks
> running at different speeds, which the SoC cache does not like).
> 
> [...]
>>> +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 think I can clean this up: aclk and clint clocks are needed early but others
> can likely be deferred. Will fix this up.
> 
> Thanks !
>