Message ID | 20210604035834.625772-1-seanga2@gmail.com |
---|---|
Headers | show |
Series | clk: k210: Rewrite K210 clock without CCF | expand |
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 >
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 >> > >
On Fri, Jun 04, 2021 at 11:58:23AM +0800, Sean Anderson wrote: > 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(-) > 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 > > -- > 2.31.0 > Hi Sean, This serie of patches seems to fail CI, could you fix these errors and send it again? Thanks! CI result: https://dev.azure.com/ycliang-tw/u-boot-riscv/_build/results?buildId=37&view=results Best regards, Leo