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

[Damien Le Moal]

On 2021/06/11 22:57, Sean Anderson wrote:
> On 6/11/21 4:21 AM, Lukasz Majewski wrote:
>> On Fri, 11 Jun 2021 00:16:06 -0400
>> Sean Anderson <seanga2 at gmail.com> 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.
>>
>> You mean for U-Boot or for Linux ?
> 
> For U-Boot. I wasn't aware that the CCF was mainly designed for porting
> drivers from Linux.
> 
>>
>>>
>>> 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
>>>
>>
>> I'm not going to defend CCF to the last breath, I know their issues.
>>
>> However, the goal for CCF was to have:
>>
>> 1. Ported code from Linux (with some code simplification)
>> 2. Get the standard approach to the clock subsystem.
>>
>> I'm just wondering if we (as a community) want to have such diversity -
>> I mean each architecture would have different clock driver (under the
>> device model)
> 
> If there is already a CCF driver for that arch for Linux, then I think
> reusing it is OK. However, I think it generally tends toward larger
> drivers, larger runtime memory usage, and awkward design. So if you are
> writing a new driver for U-Boot, I think it is better to not use CCF
> drivers.
> 
>>
>> As fair as I can see - the K210 already has support for CCF in Linux:
>> drivers/clk/clk-k210.c
> 
> Yes, but it does not really "use" the CCF. For example, things like
> composite clocks are absent; all calculations of rate are internal to
> the driver. In fact, I took inspiration from Damien's handling of this
> driver in Linux.

Correct. The initial development versions of the driver were inspired (hmmm...
copied) from Sean CCF based driver, but the composite clocks and some others
were a real pain due to the weird divisors that the K210 clocks use. In the end,
I decided to not use Linux CCF, or rather, to limit its use to simple clocks
only and to implement internally all divisors for the clocks. This ended up
simplifying the code a lot. That is what went upstream.

> 
>> Reading the above comment - it looks like you couldn't simplyfy this
>> Linux driver to be smaller and fit into U-Boot?
> 
> Well, this driver takes major inspiration from the Linux driver, which
> was itself based on this code. So in a sense, this is already a
> simplification of the Linux driver.
> 
>>
>> Sean, do you think that other archs can benefit from your work?
> 
> Hm, I don't know. I don't think there is must code which could be
> reused. Some of the major savings in disabling CCF were from disabling
> imx-specific clocks which are always enabled, even though they are not
> designed to be used outside of that arch.
> 
> In terms of general improvements, I think that get_parent() should be
> part of clk_ops. That would allow me to use the existing clock dump
> code. Additionally, I think that enable_count has no business living in
> struct clk, as that struct is a "thick pointer" and multiple ones can
> exist for the same clock.
> 
> --Sean
> 
>>
>>> This series depends on
>>> https://patchwork.ozlabs.org/project/uboot/list/?series=238211
>>>
>>> Changes in v3:
>>> - Always define clk_defaults_stage, even if clk_set_defaults is a
>>> dummy
>>> - Fix inverted condition for setting defaults
>>> - Fix val not being set for K210_DIV_POWER
>>> - Add CLK_K210_SET_RATE to defconfig so these changes apply
>>>
>>> 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      |    2 +-
>>>   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, 1437 insertions(+), 1711 deletions(-)
>>>   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
>>>
>>
>>
>>
>> Best regards,
>>
>> Lukasz Majewski
>>
>> --
>>
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
>>
> 
> 


-- 
Damien Le Moal
Western Digital Research