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

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/12/08 3:35, Stephen Boyd wrote:
> Quoting Damien Le Moal (2020-12-04 23:43:14)
>> 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 ?
> 
> Could use the 'default CONFIG_FOO' on the CLK_K210 symbol instead.

Got it. I made this change for the clk driver and for other drivers too.

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

As mentioned in previous email, I kept the nop() calls as using delay()
functions does not work due to the early execution of this code.

> 
>>>> +        */
>>>> +       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() ?
> 
> Use struct clk_parent_data please.

I ended up using parent_hws instead of parent_names in the init structure. Using
parent_hws was simpler than using parent_data. That nicely cleaned up the code I
think.

>>>> +
>>>> +       /* 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).
> 
> Ok it seems like these could just be enabled once at probe and then
> ignored? I prefer that as it's simpler.

I removed the CLINT clock as it is completely unused. I kept the CPU clock as it
is referenced by the uarths device node. The 3 SRAM clocks are not referenced,
but I kept them as is as switching to special code for these instead of using
the clock infrastructure would have just generated more code for not much gains
(beside a tiny memory saving maybe).

>>>> +       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). 
> 
> It would be visible in sysfs if there was a cpufreq driver. Can we use
> that?

I checked and there is no cpufreq information is sysfs. So unless you insist, I
would like to keep this message for information to the user.

> 
>>
>> [...]
>>>> +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.
>>
> 
> Ok. Thanks!

I tried hard with this one, but as mentioned in a previous email, there is a
circular dependency with the sysctl driver which prevents anything from being
initialized if I use a platform driver. So I switched to declaring the clock
driver using CLK_OF_DECLARE().

Note: I posted v5 but the clk driver patch 11/21 is "held until the list
moderator can review it for approval". The reason is the patch size:
    Message body is too big: 41706 bytes with a limit of 40 KB

Palmer: Who is the list moderator ? Is it you ?

Thanks for all your comments !


-- 
Damien Le Moal
Western Digital Research