On 6/11/21 4:21 AM, Lukasz Majewski wrote:
On Fri, 11 Jun 2021 00:16:06 -0400
Sean Anderson <sean...@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
drivers.
As fair as I can see - the K210 already has support for CCF in Linux:
drivers/clk/clk-k210.c
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.
--Sean
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: lu...@denx.de
