[PATCH v2 00/11] clk: k210: Rewrite K210 clock without CCF

[Sean Anderson]

On 6/4/21 12:28 AM, Damien Le Moal wrote:
> On 2021/06/04 12:58, Sean Anderson wrote:
>> This is something I've been meaning to do for a while but only just got around
>> to. The CCF has been quite unwieldy in a few ways:
>>
>> * It is very rigid, and there are not easy ways to hook into it without
>>    rewriting many things. See e.g. things like the bypass clock and all the _half
>>    clocks which were created because CCF didn't support the dividers used on the
>>    k210. While preparing this series, I encountered several edge cases which I
>>    had initially overlooked (or which were not supported in the initial release).
>>    These would have been very difficult to fix with CCF, but were much easier to
>>    address because each funcion is implemented in one place.
>> * There is a lot of magic going on under the curtains because of all the CCF
>>    code which lives in many different files. Some things live in drivers, but
>>    many things live in e.g. clk-uclass.c. So many things live in so many files
>>    and it can be very difficult to get a handle on what exactly happens.
>>    Complicating this is that there is a conflation of struct clk as a handle and
>>    struct clk as a device. In this regard, refcounting is completely broken. IMO
>>    we should just do away with refcounts and only disable clocks when explicitly
>>    asked for.
>> * It is very dependent on runtime initialization. Typically, everything is
>>    initialized by calling into various register() functions, usually with several
>>    wrappers to avoid specifying all the arguments. This balloons the runtime
>>    memory usage since there are so many devices created. It also makes it hard to
>>    debug, since if you do it the "typical" way it is easy to accidentally assign
>>    a clock to the wrong register.
>> * It inflates code size by pulling in not just some dead code (e.g. this driver
>>    does not use divider tables but they are in clk-divider anyway) but also
>>    pulling in numerous imx-specific clocks. This could be fixed, but I don't want
>>    to due to the other reasons listed.
>>
>> I am very happy to have completely excised it from my driver. IMO there should
>> be big warning signs on the CCF warning not to use it for new code. This would
>> hopefully dissuade those like myself who had no idea that CCF was *not* in fact
>> a good way to write a clock driver.
>>
>> Overall there is a total savings of 12k from this series.
>>     text	   data	    bss	    dec	    hex	filename
>>   292485	  32672	  12624	 337781	  52775	before/u-boot
>>   283125	  29856	  12624	 325605	  4f7e5	after/u-boot
>>
>> This series depends on
>> https://patchwork.ozlabs.org/project/uboot/list/?series=238211
>>
>> Changes in v2:
>> - Convert stage to enum
>> - Only force probe clocks pre-reloc
>> - Rebase on u-boot/master
>>
>> Sean Anderson (11):
>>    clk: Allow force setting clock defaults before relocation
>>    clk: k210: Rewrite to remove CCF
>>    clk: k210: Move pll into the rest of the driver
>>    clk: k210: Implement soc_clk_dump
>>    clk: k210: Re-add support for setting rate
>>    clk: k210: Don't set PLL rates if we are already at the correct rate
>>    clk: k210: Remove bypass driver
>>    clk: k210: Move k210 clock out of its own subdirectory
>>    k210: dts: Set PLL1 to the same rate as PLL0
>>    k210: Don't imply CCF
>>    test: Add K210 PLL tests to sandbox defconfigs
>>
>>   MAINTAINERS                             |    4 +-
>>   arch/riscv/dts/k210.dtsi                |    2 +
>>   board/sipeed/maix/Kconfig               |    2 -
>>   configs/sandbox64_defconfig             |    2 +
>>   configs/sandbox_defconfig               |    2 +
>>   configs/sandbox_flattree_defconfig      |    2 +
>>   configs/sipeed_maix_bitm_defconfig      |    1 -
>>   drivers/clk/Kconfig                     |   14 +-
>>   drivers/clk/Makefile                    |    2 +-
>>   drivers/clk/clk-uclass.c                |   27 +-
>>   drivers/clk/clk_kendryte.c              | 1320 +++++++++++++++++++++++
>>   drivers/clk/kendryte/Kconfig            |   12 -
>>   drivers/clk/kendryte/Makefile           |    1 -
>>   drivers/clk/kendryte/bypass.c           |  273 -----
>>   drivers/clk/kendryte/clk.c              |  668 ------------
>>   drivers/clk/kendryte/pll.c              |  585 ----------
>>   drivers/clk/rockchip/clk_rk3308.c       |    2 +-
>>   drivers/core/device.c                   |    2 +-
>>   drivers/net/gmac_rockchip.c             |    2 +-
>>   include/clk.h                           |   30 +-
>>   include/dt-bindings/clock/k210-sysctl.h |   94 +-
>>   include/kendryte/bypass.h               |   31 -
>>   include/kendryte/clk.h                  |   35 -
>>   include/kendryte/pll.h                  |   34 -
>>   24 files changed, 1436 insertions(+), 1711 deletions(-)
> 
> Awesome cleanup !
> 
> I will try to revisit the clock rate setting in Linux and re-check where we diverge.

I don't believe there are any divergences other than the fix mentioned in 02/11.

> 
> FYI: I have a draft series adding K210 builds to buildroot here:
> 
> https://github.com/damien-lemoal/buildroot (k210-v2 branch)

Ok, I will try and check it out.

(you have a typo in board/canaan/k210/README.txt: Sipedd -> Sipeed)

--Sean

> 
> I cannot get userspace to run with kernel 5.12, even adding the binfmt_flat
> change in 5.13 (commit 04d82a6d0881 "binfmt_flat: allow not offsetting data
> start"). Not sure what is going on yet. 5.13-rc4 kernel works perfectly.
> 
> 
>>   create mode 100644 drivers/clk/clk_kendryte.c
>>   delete mode 100644 drivers/clk/kendryte/Kconfig
>>   delete mode 100644 drivers/clk/kendryte/Makefile
>>   delete mode 100644 drivers/clk/kendryte/bypass.c
>>   delete mode 100644 drivers/clk/kendryte/clk.c
>>   delete mode 100644 drivers/clk/kendryte/pll.c
>>   delete mode 100644 include/kendryte/bypass.h
>>   delete mode 100644 include/kendryte/clk.h
>>
> 
>