On Fri, 11 Jun 2021 00:16:06 -0400 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. You mean for U-Boot or for 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) As fair as I can see - the K210 already has support for CCF in Linux: drivers/clk/clk-k210.c Reading the above comment - it looks like you couldn't simplyfy this Linux driver to be smaller and fit into U-Boot? Sean, do you think that other archs can benefit from your work? > 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@denx.de