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

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 6376 bytes --]

On Fri, 11 Jun 2021 00:16:06 -0400
Sean Anderson <seanga2@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 ?

> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]