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@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 ? > > 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
