From: Sean Anderson <seanga2@gmail.com>
To: Damien Le Moal <Damien.LeMoal@wdc.com>,
"u-boot@lists.denx.de" <u-boot@lists.denx.de>
Cc: Lukasz Majewski <lukma@denx.de>,
Leo Liang <ycliang@andestech.com>,
Andreas Dannenberg <dannenberg@ti.com>,
Lokesh Vutla <lokeshvutla@ti.com>,
Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Subject: Re: [PATCH v2 00/11] clk: k210: Rewrite K210 clock without CCF
Date: Fri, 4 Jun 2021 00:40:44 -0400 [thread overview]
Message-ID: <b4697619-bac0-93df-6c83-b2e5052c421e@gmail.com> (raw)
In-Reply-To: <DM6PR04MB7081A7F1F70F6C9DB0B9E560E73B9@DM6PR04MB7081.namprd04.prod.outlook.com>
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
>>
>
>
next prev parent reply other threads:[~2021-06-04 4:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-04 3:58 [PATCH v2 00/11] clk: k210: Rewrite K210 clock without CCF Sean Anderson
2021-06-04 3:58 ` [PATCH v2 01/11] clk: Allow force setting clock defaults before relocation Sean Anderson
2021-06-04 3:58 ` [PATCH v2 02/11] clk: k210: Rewrite to remove CCF Sean Anderson
2021-06-04 3:58 ` [PATCH v2 03/11] clk: k210: Move pll into the rest of the driver Sean Anderson
2021-06-04 3:58 ` [PATCH v2 04/11] clk: k210: Implement soc_clk_dump Sean Anderson
2021-06-04 3:58 ` [PATCH v2 05/11] clk: k210: Re-add support for setting rate Sean Anderson
2021-06-04 4:17 ` Sean Anderson
2021-06-04 3:58 ` [PATCH v2 06/11] clk: k210: Don't set PLL rates if we are already at the correct rate Sean Anderson
2021-06-04 3:58 ` [PATCH v2 07/11] clk: k210: Remove bypass driver Sean Anderson
2021-06-04 3:58 ` [PATCH v2 08/11] clk: k210: Move k210 clock out of its own subdirectory Sean Anderson
2021-06-04 3:58 ` [PATCH v2 09/11] k210: dts: Set PLL1 to the same rate as PLL0 Sean Anderson
2021-06-04 3:58 ` [PATCH v2 10/11] k210: Don't imply CCF Sean Anderson
2021-06-04 3:58 ` [PATCH v2 11/11] test: Add K210 PLL tests to sandbox defconfigs Sean Anderson
2021-06-04 4:28 ` [PATCH v2 00/11] clk: k210: Rewrite K210 clock without CCF Damien Le Moal
2021-06-04 4:40 ` Sean Anderson [this message]
2021-06-10 18:00 ` Leo Liang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b4697619-bac0-93df-6c83-b2e5052c421e@gmail.com \
--to=seanga2@gmail.com \
--cc=Damien.LeMoal@wdc.com \
--cc=dannenberg@ti.com \
--cc=lokeshvutla@ti.com \
--cc=lukma@denx.de \
--cc=philipp.tomsich@theobroma-systems.com \
--cc=u-boot@lists.denx.de \
--cc=ycliang@andestech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).