[PATCH v3 00/11] clk: k210: Rewrite K210 clock without CCF
seanga2 at gmail.com
Fri Jun 11 15:57:15 CEST 2021
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
> As fair as I can see - the K210 already has support for CCF in Linux:
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.
> 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.
>> This series depends on
>> Changes in v3:
>> - Always define clk_defaults_stage, even if clk_set_defaults is a
>> - 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
More information about the U-Boot