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. FYI: I have a draft series adding K210 builds to buildroot here: https://github.com/damien-lemoal/buildroot (k210-v2 branch) 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 > -- Damien Le Moal Western Digital Research
