[PATCH 0/4] clk: add Siflower SF21 topcrm support

[PATCH 0/4] clk: add Siflower SF21 topcrm support

[Chuanhong Guo]

Siflower SF21A6826 and SF21H8898 are RISC-V chips with quad-core
T-Head C908 for home routers and gateways.
This series adds the initial RISC-V Kconfig entry for Siflower SoCs and
support for the toplevel clock and reset module on Siflower SF21 socs.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
Chuanhong Guo (4):
      riscv: add Siflower RISC-V SoC family Kconfig support
      dt-bindings: clock: add binding header for sf21-topcrm
      dt-bindings: clock: add doc for Siflower sf21-topcrm
      clk: add support for siflower sf21-topcrm

 .../bindings/clock/siflower,sf21-topcrm.yaml       |   69 ++
 arch/riscv/Kconfig.socs                            |    7 +
 drivers/clk/Kconfig                                |    1 +
 drivers/clk/Makefile                               |    1 +
 drivers/clk/siflower/Kconfig                       |   22 +
 drivers/clk/siflower/Makefile                      |    1 +
 drivers/clk/siflower/clk-sf21-topcrm.c             | 1053 ++++++++++++++++++++
 include/dt-bindings/clock/siflower,sf21-topcrm.h   |   63 ++
 8 files changed, 1217 insertions(+)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260515-sf21-topcrm-1856c05b6138

Best regards,
-- 
Chuanhong Guo <gch981213@gmail.com>

[PATCH 1/4] riscv: add Siflower RISC-V SoC family Kconfig support

[Chuanhong Guo]

Siflower RISC-V SoCs, including SF21A6826 and SF21H8898, are RISC-V
chips with T-Head C908 cores for home routers and gateways. Add a
Kconfig entry named ARCH_SIFLOWER for them.
Notably these chips uses ARM PL011 for UART. ARM_AMBA is selected
for its driver.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 arch/riscv/Kconfig.socs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index c174ac0ec46b..9996591cd9db 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -37,6 +37,13 @@ config ARCH_SIFIVE
 	help
 	  This enables support for SiFive SoC platform hardware.
 
+config ARCH_SIFLOWER
+	bool "Siflower RISC-V SoCs"
+	select ARM_AMBA if TTY
+	select ERRATA_THEAD
+	help
+	  This enables support for Siflower RISC-V SoC platform hardware.
+
 config ARCH_SOPHGO
 	bool "Sophgo SoCs"
 	help

-- 
2.54.0

Re: [PATCH 1/4] riscv: add Siflower RISC-V SoC family Kconfig support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Conditionally selecting the `ARM_AMBA` hardware bus based on the `TTY` software subsystem.
--

commit 49824e33ae15603df7f899c7df0a4ab9b5f20eed
Author: Chuanhong Guo <gch981213@gmail.com>

riscv: add Siflower RISC-V SoC family Kconfig support

This adds Kconfig support for the Siflower RISC-V SoCs, including the
SF21A6826 and SF21H8898 chips. It defines ARCH_SIFLOWER, selects the
relevant T-Head errata, and conditionally selects ARM_AMBA for the UART.

> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index c174ac0ec46b4..9996591cd9dbf 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
[ ... ]
> @@ -37,6 +37,13 @@ config ARCH_SIFIVE
>  	help
>  	  This enables support for SiFive SoC platform hardware.
>  
> +config ARCH_SIFLOWER
> +	bool "Siflower RISC-V SoCs"
> +	select ARM_AMBA if TTY

Should ARM_AMBA be selected conditionally based on TTY?

ARM_AMBA models the presence of a physical hardware bus, which exists
regardless of whether the TTY software subsystem is enabled. Even if the
PL011 UART is currently the only AMBA device on this platform, tying the
hardware bus infrastructure to a software abstraction might cause issues if
other AMBA devices are added later and TTY is disabled.

Would it be better to select ARM_AMBA unconditionally, similar to how other
architectures handle it?

> +	select ERRATA_THEAD
> +	help
> +	  This enables support for Siflower RISC-V SoC platform hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-sf21-topcrm-v1-0-438f2e0513ff@gmail.com?part=1

Re: [PATCH 1/4] riscv: add Siflower RISC-V SoC family Kconfig support

[Conor Dooley]

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

On Sun, May 17, 2026 at 02:31:52PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Conditionally selecting the `ARM_AMBA` hardware bus based on the `TTY` software subsystem.
> --
> 
> commit 49824e33ae15603df7f899c7df0a4ab9b5f20eed
> Author: Chuanhong Guo <gch981213@gmail.com>
> 
> riscv: add Siflower RISC-V SoC family Kconfig support
> 
> This adds Kconfig support for the Siflower RISC-V SoCs, including the
> SF21A6826 and SF21H8898 chips. It defines ARCH_SIFLOWER, selects the
> relevant T-Head errata, and conditionally selects ARM_AMBA for the UART.
> 
> > diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> > index c174ac0ec46b4..9996591cd9dbf 100644
> > --- a/arch/riscv/Kconfig.socs
> > +++ b/arch/riscv/Kconfig.socs
> [ ... ]
> > @@ -37,6 +37,13 @@ config ARCH_SIFIVE
> >  	help
> >  	  This enables support for SiFive SoC platform hardware.
> >  
> > +config ARCH_SIFLOWER
> > +	bool "Siflower RISC-V SoCs"
> > +	select ARM_AMBA if TTY
> 
> Should ARM_AMBA be selected conditionally based on TTY?
> 
> ARM_AMBA models the presence of a physical hardware bus, which exists
> regardless of whether the TTY software subsystem is enabled. Even if the

There's no point having the driver built if there's no driver built for
devices on the bus, I think is the angle here.

> PL011 UART is currently the only AMBA device on this platform, tying the
> hardware bus infrastructure to a software abstraction might cause issues if
> other AMBA devices are added later and TTY is disabled.
> 
> Would it be better to select ARM_AMBA unconditionally, similar to how other
> architectures handle it?
> 
> > +	select ERRATA_THEAD
> > +	help
> > +	  This enables support for Siflower RISC-V SoC platform hardware.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260517-sf21-topcrm-v1-0-438f2e0513ff@gmail.com?part=1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] riscv: add Siflower RISC-V SoC family Kconfig support

[Chuanhong Guo]

Hi!

On Mon, May 18, 2026 at 4:46 AM Conor Dooley <conor@kernel.org> wrote:
> [...]
> > >       This enables support for SiFive SoC platform hardware.
> > >
> > > +config ARCH_SIFLOWER
> > > +   bool "Siflower RISC-V SoCs"
> > > +   select ARM_AMBA if TTY
> >
> > Should ARM_AMBA be selected conditionally based on TTY?
> >
> > ARM_AMBA models the presence of a physical hardware bus, which exists
> > regardless of whether the TTY software subsystem is enabled. Even if the
>
> There's no point having the driver built if there's no driver built for
> devices on the bus, I think is the angle here.
>
> > PL011 UART is currently the only AMBA device on this platform, tying the
> > hardware bus infrastructure to a software abstraction might cause issues if
> > other AMBA devices are added later and TTY is disabled.

I copied this from the vendor tree. PL011 is the only peripheral on this chip
requiring this flag, and there's no more AMBA-driver compatible devices
on this SoC.
However I'll change this one in v2 since I don't want to add the only flag
in the kernel which conditionally select ARM_AMBA :P

-- 
Regards,
Chuanhong Guo

[PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[Chuanhong Guo]

Add the device tree binding header for Siflower SF21A6826/SF21H8898
toplevel clock and reset module. The header covers both clock and
reset IDs provided by the block.
CLK_ETH_REF_P is a clock name that exists in the vendor datasheet.
This clock connects directly to CLK_PCIEPLL_FOUT2 and there's no
clock gate/mux in between. An alias is created for this clock
to make available clock names align with the datasheet.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 include/dt-bindings/clock/siflower,sf21-topcrm.h | 63 ++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/include/dt-bindings/clock/siflower,sf21-topcrm.h b/include/dt-bindings/clock/siflower,sf21-topcrm.h
new file mode 100644
index 000000000000..3690b3452501
--- /dev/null
+++ b/include/dt-bindings/clock/siflower,sf21-topcrm.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
+
+#ifndef _DT_BINDINGS_CLK_SIFLOWER_SF21_TOPCRM_H
+#define _DT_BINDINGS_CLK_SIFLOWER_SF21_TOPCRM_H
+
+#define SF21_CLK_CMNPLL_VCO		0
+#define SF21_CLK_CMNPLL_POSTDIV		1
+
+#define SF21_CLK_DDRPLL_POSTDIV		2
+
+#define SF21_CLK_PCIEPLL_VCO		3
+#define SF21_CLK_PCIEPLL_FOUT0		4
+#define SF21_CLK_PCIEPLL_FOUT1		5
+#define SF21_CLK_PCIEPLL_FOUT2		6
+#define SF21_CLK_ETH_REF_P		SF21_CLK_PCIEPLL_FOUT2
+#define SF21_CLK_PCIEPLL_FOUT3		7
+
+#define SF21_CLK_CPU			8
+#define SF21_CLK_PIC			9
+#define SF21_CLK_AXI			10
+#define SF21_CLK_AHB			11
+#define SF21_CLK_APB			12
+#define SF21_CLK_UART			13
+#define SF21_CLK_IRAM			14
+#define SF21_CLK_NPU			15
+#define SF21_CLK_DDRPHY_REF		16
+#define SF21_CLK_DDR_BYPASS		17
+#define SF21_CLK_ETHTSU			18
+#define SF21_CLK_GMAC_BYP_REF		19
+#define SF21_CLK_USB			20
+#define SF21_CLK_USBPHY			21
+#define SF21_CLK_SERDES_CSR		22
+#define SF21_CLK_CRYPT_CSR		23
+#define SF21_CLK_CRYPT_APP		24
+#define SF21_CLK_IROM			25
+#define SF21_CLK_BOOT			26
+#define SF21_CLK_PVT			27
+#define SF21_CLK_PLL_TEST		28
+#define SF21_CLK_PCIE_REFN		29
+#define SF21_CLK_PCIE_REFP		30
+#define SF21_CLK_MAX			31
+
+#define SF21_RESET_GIC			0
+#define SF21_RESET_AXI			1
+#define SF21_RESET_AHB			2
+#define SF21_RESET_APB			3
+#define SF21_RESET_IRAM			4
+#define SF21_RESET_NPU			5
+#define SF21_RESET_DDR_CTL		6
+#define SF21_RESET_DDR_PHY		7
+#define SF21_RESET_DDR_PWR_OK_IN	8
+#define SF21_RESET_DDR_CTL_APB		9
+#define SF21_RESET_DDR_PHY_APB		10
+#define SF21_RESET_USB			11
+#define SF21_RESET_PVT			12
+#define SF21_RESET_SERDES_CSR		13
+#define SF21_RESET_CRYPT_CSR		14
+#define SF21_RESET_CRYPT_APP		15
+#define SF21_RESET_NPU2DDR_ASYNCBRIDGE	16
+#define SF21_RESET_IROM			17
+#define SF21_RESET_MAX			18
+
+#endif

-- 
2.54.0

Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The dt-bindings header file exposes software-specific `MAX` macros (`SF21_CLK_MAX` and `SF21_RESET_MAX`).
--

commit b39e2fdfa1953205f67f3e13ae23a09e43917c11
Author: Chuanhong Guo <gch981213@gmail.com>

dt-bindings: clock: add binding header for sf21-topcrm

Add the device tree binding header for Siflower SF21A6826/SF21H8898
toplevel clock and reset module. The header covers both clock and
reset IDs provided by the block.

> diff --git a/include/dt-bindings/clock/siflower,sf21-topcrm.h b/include/dt-bindings/clock/siflower,sf21-topcrm.h
> new file mode 100644
> index 0000000000000..3690b3452501e
> --- /dev/null
> +++ b/include/dt-bindings/clock/siflower,sf21-topcrm.h
[ ... ]
> +#define SF21_CLK_PCIE_REFN		29
> +#define SF21_CLK_PCIE_REFP		30
> +#define SF21_CLK_MAX			31

Should this MAX macro be exposed in the dt-bindings header?

Device tree binding headers define the hardware ABI, and the total number
of clocks is an internal software implementation detail. Changing this
value when new clocks are added in the future breaks ABI compatibility for
components compiled against the older header.

Could this macro be defined internally within the driver source code instead?

> +
> +#define SF21_RESET_GIC			0
[ ... ]
> +#define SF21_RESET_NPU2DDR_ASYNCBRIDGE	16
> +#define SF21_RESET_IROM			17
> +#define SF21_RESET_MAX			18

Similarly, should SF21_RESET_MAX be moved to the driver source code to
avoid breaking ABI compatibility if new resets are added?

> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-sf21-topcrm-v1-0-438f2e0513ff@gmail.com?part=2

Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[Conor Dooley]

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

On Sun, May 17, 2026 at 10:12:56PM +0800, Chuanhong Guo wrote:
> +#define SF21_CLK_PCIE_REFP		30
> +#define SF21_CLK_MAX			31

> +#define SF21_RESET_IROM			17
> +#define SF21_RESET_MAX			18


Having _MAX is not permitted, but is also really confusing that "max"
appears to be max+1, so actually represents the count not the max?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[Conor Dooley]

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

On Sun, May 17, 2026 at 09:47:26PM +0100, Conor Dooley wrote:
> On Sun, May 17, 2026 at 10:12:56PM +0800, Chuanhong Guo wrote:
> > +#define SF21_CLK_PCIE_REFP		30
> > +#define SF21_CLK_MAX			31
> 
> > +#define SF21_RESET_IROM			17
> > +#define SF21_RESET_MAX			18
> 
> 
> Having _MAX is not permitted, but is also really confusing that "max"
> appears to be max+1, so actually represents the count not the max?

To be clear, if you need a define like this, put it in the driver.
Bindings having a "_MAX" or "NUM_CLKS" doesn't make sense, and half the
time this number ends up changing anyway.

Also, squash this with the patch adding the clock binding.

Cheers,
Conor.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[Chuanhong Guo]

On Mon, May 18, 2026 at 4:52 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sun, May 17, 2026 at 09:47:26PM +0100, Conor Dooley wrote:
> > On Sun, May 17, 2026 at 10:12:56PM +0800, Chuanhong Guo wrote:
> > > +#define SF21_CLK_PCIE_REFP         30
> > > +#define SF21_CLK_MAX                       31
> >
> > > +#define SF21_RESET_IROM                    17
> > > +#define SF21_RESET_MAX                     18
> >
> >
> > Having _MAX is not permitted, but is also really confusing that "max"
> > appears to be max+1, so actually represents the count not the max?
>
> To be clear, if you need a define like this, put it in the driver.
> Bindings having a "_MAX" or "NUM_CLKS" doesn't make sense,

Sure. I'll move it to the driver and rename it in v2.

> and half the
> time this number ends up changing anyway.

This one will definitely change later for the two clocks I can't test yet.

> Also, squash this with the patch adding the clock binding.

OK. I'll do so in v2.

-- 
Regards,
Chuanhong Guo

Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[Yao Zi]

On Sun, May 17, 2026 at 10:12:56PM +0800, Chuanhong Guo wrote:
> Add the device tree binding header for Siflower SF21A6826/SF21H8898
> toplevel clock and reset module. The header covers both clock and
> reset IDs provided by the block.

Would it be better to split reset stuff into a separate header in
dt-bindings/reset, so it ends up with a clearer structure? This
shouldn't cost a lot.

Regards,
Yao Zi

> CLK_ETH_REF_P is a clock name that exists in the vendor datasheet.
> This clock connects directly to CLK_PCIEPLL_FOUT2 and there's no
> clock gate/mux in between. An alias is created for this clock
> to make available clock names align with the datasheet.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  include/dt-bindings/clock/siflower,sf21-topcrm.h | 63 ++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)

Re: [PATCH 2/4] dt-bindings: clock: add binding header for sf21-topcrm

[Chuanhong Guo]

On Mon, May 18, 2026 at 7:05 PM Yao Zi <me@ziyao.cc> wrote:
>
> On Sun, May 17, 2026 at 10:12:56PM +0800, Chuanhong Guo wrote:
> > Add the device tree binding header for Siflower SF21A6826/SF21H8898
> > toplevel clock and reset module. The header covers both clock and
> > reset IDs provided by the block.
>
> Would it be better to split reset stuff into a separate header in
> dt-bindings/reset, so it ends up with a clearer structure? This
> shouldn't cost a lot.
>

Sure. I'll split it in v2.

-- 
Regards,
Chuanhong Guo

[PATCH 3/4] dt-bindings: clock: add doc for Siflower sf21-topcrm

[Chuanhong Guo]

Add a binding doc for the top clock and reset module found on Siflower
SF21 SoCs. This block provides the main PLLs, high-level clock
controls, and some reset lines.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 .../bindings/clock/siflower,sf21-topcrm.yaml       | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.yaml b/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.yaml
new file mode 100644
index 000000000000..a013d48841f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/siflower,sf21-topcrm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Siflower SF21 toplevel clock and reset module
+
+maintainers:
+  - Chuanhong Guo <gch981213@gmail.com>
+
+description: |
+  The toplevel clock and reset module on Siflower SF21 SoCs manages
+  the main PLLs, high-level clock muxes/dividers/gates, and some
+  reset lines.
+  Available clocks and resets are defined in:
+  include/dt-bindings/clock/siflower,sf21-topcrm.h
+
+properties:
+  compatible:
+    const: siflower,sf21-topcrm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: xin25m
+
+  "#clock-cells":
+    const: 1
+
+  "#reset-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#clock-cells"
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/siflower,sf21-topcrm.h>
+    / {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        xin25m: clock-25000000 {
+            compatible = "fixed-clock";
+            #clock-cells = <0>;
+            clock-frequency = <25000000>;
+        };
+
+        clock-controller@ce00400 {
+            compatible = "siflower,sf21-topcrm";
+            reg = <0x0ce00400 0x400>;
+            clocks = <&xin25m>;
+            clock-names = "xin25m";
+            #clock-cells = <1>;
+            #reset-cells = <1>;
+        };
+    };

-- 
2.54.0

Re: [PATCH 3/4] dt-bindings: clock: add doc for Siflower sf21-topcrm

[Rob Herring (Arm)]

On Sun, 17 May 2026 22:12:57 +0800, Chuanhong Guo wrote:
> Add a binding doc for the top clock and reset module found on Siflower
> SF21 SoCs. This block provides the main PLLs, high-level clock
> controls, and some reset lines.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  .../bindings/clock/siflower,sf21-topcrm.yaml       | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.example.dtb: /: 'compatible' is a required property
	from schema $id: http://devicetree.org/schemas/root-node.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.example.dtb: /: 'model' is a required property
	from schema $id: http://devicetree.org/schemas/root-node.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260517-sf21-topcrm-v1-3-438f2e0513ff@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 3/4] dt-bindings: clock: add doc for Siflower sf21-topcrm

[Conor Dooley]

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

On Sun, May 17, 2026 at 10:12:57PM +0800, Chuanhong Guo wrote:
> Add a binding doc for the top clock and reset module found on Siflower
> SF21 SoCs. This block provides the main PLLs, high-level clock
> controls, and some reset lines.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  .../bindings/clock/siflower,sf21-topcrm.yaml       | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.yaml b/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.yaml
> new file mode 100644
> index 000000000000..a013d48841f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/siflower,sf21-topcrm.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/siflower,sf21-topcrm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Siflower SF21 toplevel clock and reset module
> +
> +maintainers:
> +  - Chuanhong Guo <gch981213@gmail.com>
> +
> +description: |
> +  The toplevel clock and reset module on Siflower SF21 SoCs manages
> +  the main PLLs, high-level clock muxes/dividers/gates, and some
> +  reset lines.
> +  Available clocks and resets are defined in:
> +  include/dt-bindings/clock/siflower,sf21-topcrm.h
> +
> +properties:
> +  compatible:
> +    const: siflower,sf21-topcrm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: xin25m

Is a 25 MHz reference required on this SoC? If not, name here is
obviously problematic if it can be something else.
Not much value in clock-names anyway when you only have 1.

> +
> +  "#clock-cells":
> +    const: 1
> +
> +  "#reset-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - "#clock-cells"
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/siflower,sf21-topcrm.h>
> +    / {

Replace this / with "soc".

> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        xin25m: clock-25000000 {
> +            compatible = "fixed-clock";
> +            #clock-cells = <0>;
> +            clock-frequency = <25000000>;
> +        };

Delete this node, it's not needed in the example. The tooling will fill
it in.

Also, please test your bindings since this doesn't pass.

pw-bot: changes-requested

Thanks,
Conor.

> +
> +        clock-controller@ce00400 {
> +            compatible = "siflower,sf21-topcrm";
> +            reg = <0x0ce00400 0x400>;
> +            clocks = <&xin25m>;
> +            clock-names = "xin25m";
> +            #clock-cells = <1>;
> +            #reset-cells = <1>;
> +        };
> +    };
> 
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] dt-bindings: clock: add doc for Siflower sf21-topcrm

[Chuanhong Guo]

Hi!

On Mon, May 18, 2026 at 4:50 AM Conor Dooley <conor@kernel.org> wrote:
>
> [...]
> > +
> > +  clock-names:
> > +    const: xin25m
>
> Is a 25 MHz reference required on this SoC? If not, name here is
> obviously problematic if it can be something else.
> Not much value in clock-names anyway when you only have 1.

You are right. I'll drop clock-names and use index to grab the
parent I needed in the driver in v2.

>
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  "#reset-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - "#clock-cells"
> > +  - "#reset-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/siflower,sf21-topcrm.h>
> > +    / {
>
> Replace this / with "soc".

Will do so in v2.

>
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        xin25m: clock-25000000 {
> > +            compatible = "fixed-clock";
> > +            #clock-cells = <0>;
> > +            clock-frequency = <25000000>;
> > +        };
>
> Delete this node, it's not needed in the example. The tooling will fill
> it in.

Oh, I didn't know that. I'll drop it in v2.

>
> Also, please test your bindings since this doesn't pass.
>
> pw-bot: changes-requested

It's failing on the example as root node missing "model" and "compatible".
and it will be fixed after changing "/" to "soc".
I'll remember to run the full dt check instead of using DT_SCHEMA_FILES
for my single file next time.

-- 
Regards,
Chuanhong Guo

Re: [PATCH 3/4] dt-bindings: clock: add doc for Siflower sf21-topcrm

[Yao Zi]

On Mon, May 18, 2026 at 08:12:14PM +0800, Chuanhong Guo wrote:
> Hi!
> 
> On Mon, May 18, 2026 at 4:50 AM Conor Dooley <conor@kernel.org> wrote:

...

> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/siflower,sf21-topcrm.h>

Though it isn't a big problem, the include is unnecessary, either, since
you don't make use of any constants from the binding header in the
example.

> > > +    / {
> >
> > Replace this / with "soc".
> 
> Will do so in v2.
> 
> >
> > > +        #address-cells = <1>;
> > > +        #size-cells = <1>;
> > > +
> > > +        xin25m: clock-25000000 {
> > > +            compatible = "fixed-clock";
> > > +            #clock-cells = <0>;
> > > +            clock-frequency = <25000000>;
> > > +        };
> >
> > Delete this node, it's not needed in the example. The tooling will fill
> > it in.
> 
> Oh, I didn't know that. I'll drop it in v2.
> 
> >
> > Also, please test your bindings since this doesn't pass.
> >
> > pw-bot: changes-requested
> 
> It's failing on the example as root node missing "model" and "compatible".
> and it will be fixed after changing "/" to "soc".
> I'll remember to run the full dt check instead of using DT_SCHEMA_FILES
> for my single file next time.

Alternatively you could choose to drop the outer node and keep the
clock-controller node only.

> 
> -- 
> Regards,
> Chuanhong Guo

Best regards,
Yao Zi

[PATCH 4/4] clk: add support for siflower sf21-topcrm

[Chuanhong Guo]

This commit adds a driver for the Toplevel clock and reset controller
found on Siflower SF21A6826/SF21H8898 SoCs.
This block contains control for 3 PLLs, several clock mux/gate/divider
blocks, and a reset register for on-chip peripherals.

There are also two registers for enabling PCIE clock output in this
block. They aren't covered by this patch because I can't test those
without a PCIE driver. These will be added with the PCIE driver
patchset later after I get that working.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 drivers/clk/Kconfig                    |    1 +
 drivers/clk/Makefile                   |    1 +
 drivers/clk/siflower/Kconfig           |   22 +
 drivers/clk/siflower/Makefile          |    1 +
 drivers/clk/siflower/clk-sf21-topcrm.c | 1053 ++++++++++++++++++++++++++++++++
 5 files changed, 1078 insertions(+)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index b2efbe9f6acb..8098e38d5f59 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -523,6 +523,7 @@ source "drivers/clk/renesas/Kconfig"
 source "drivers/clk/rockchip/Kconfig"
 source "drivers/clk/samsung/Kconfig"
 source "drivers/clk/sifive/Kconfig"
+source "drivers/clk/siflower/Kconfig"
 source "drivers/clk/socfpga/Kconfig"
 source "drivers/clk/sophgo/Kconfig"
 source "drivers/clk/spacemit/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a3e2862ebd7e..7492942b7fad 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -144,6 +144,7 @@ obj-y					+= renesas/
 obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
 obj-$(CONFIG_CLK_SIFIVE)		+= sifive/
+obj-$(CONFIG_CLK_SIFLOWER)		+= siflower/
 obj-y					+= socfpga/
 obj-y					+= sophgo/
 obj-y					+= spacemit/
diff --git a/drivers/clk/siflower/Kconfig b/drivers/clk/siflower/Kconfig
new file mode 100644
index 000000000000..03cbfbdbdb8d
--- /dev/null
+++ b/drivers/clk/siflower/Kconfig
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menuconfig CLK_SIFLOWER
+	bool "Clock driver for Siflower SoCs"
+	depends on ARCH_SIFLOWER || COMPILE_TEST
+	default ARCH_SIFLOWER
+	help
+	  Clock drivers for Siflower Linux-capable SoCs.
+
+if CLK_SIFLOWER
+
+config CLK_SF21_TOPCRM
+	tristate "Clock driver for Siflower SF21 toplevel clock & reset module"
+	depends on ARCH_SIFLOWER || COMPILE_TEST
+	default ARCH_SIFLOWER
+	select RESET_CONTROLLER
+	help
+	  Supports the toplevel clock and reset module in Siflower SF21 SoCs.
+	  If this kernel is meant to run on Siflower SF21A6826 or SF21H8898,
+	  enable this driver.
+
+endif
diff --git a/drivers/clk/siflower/Makefile b/drivers/clk/siflower/Makefile
new file mode 100644
index 000000000000..952a470a4308
--- /dev/null
+++ b/drivers/clk/siflower/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_CLK_SF21_TOPCRM) += clk-sf21-topcrm.o
diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
new file mode 100644
index 000000000000..7d4c5e370d6d
--- /dev/null
+++ b/drivers/clk/siflower/clk-sf21-topcrm.c
@@ -0,0 +1,1053 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/clk-provider.h>
+#include <linux/bitfield.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/rational.h>
+#include <linux/module.h>
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/siflower,sf21-topcrm.h>
+
+struct sf_clk_common {
+	void __iomem	*base;
+	/* Serializes register RMW sequences shared by clocks and resets. */
+	spinlock_t	*lock;
+	struct clk_hw	hw;
+};
+
+static inline struct sf_clk_common *hw_to_sf_clk_common(struct clk_hw *hw)
+{
+	return container_of(hw, struct sf_clk_common, hw);
+}
+
+static inline u32 sf_readl(struct sf_clk_common *priv, u32 reg)
+{
+	return readl(priv->base + reg);
+}
+
+static inline void sf_writel(struct sf_clk_common *priv, u32 reg, u32 val)
+{
+	return writel(val, priv->base + reg);
+}
+
+static inline void sf_rmw(struct sf_clk_common *priv, u32 reg, u32 clr, u32 set)
+{
+	u32 val;
+
+	val = sf_readl(priv, reg);
+	val &= ~clr;
+	val |= set;
+	sf_writel(priv, reg, val);
+}
+
+#define PLL_CMN_CFG1		0x0
+#define  PLL_CMN_BYPASS		BIT(27)
+#define  PLL_CMN_PD		BIT(26)
+#define  PLL_CMN_FBDIV		GENMASK(25, 14)
+#define  PLL_CMN_FBDIV_BITS	(25 - 14 + 1)
+#define  PLL_CMN_POSTDIV_PD	BIT(13)
+#define  PLL_CMN_VCO_PD		BIT(12)
+#define  PLL_CMN_POSTDIV1	GENMASK(11, 9)
+#define  PLL_CMN_POSTDIV2	GENMASK(8, 6)
+#define  PLL_CMN_REFDIV		GENMASK(5, 0)
+#define  PLL_CMN_REFDIV_BITS	6
+
+#define PLL_CMN_LOCK		0xc8
+#define PLL_DDR_LOCK		0xcc
+#define PLL_PCIE_LOCK		0xd4
+
+#define CFG_LOAD		0x100
+#define  CFG_LOAD_PCIE_PLL	BIT(4)
+#define  CFG_LOAD_DDR_PLL	BIT(2)
+#define  CFG_LOAD_CMN_PLL	BIT(1)
+#define  CFG_LOAD_DIV		BIT(0)
+
+#define PLL_LOCK_TIMEOUT_US	1000
+
+static unsigned long sf21_cmnpll_vco_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	u32 cfg = sf_readl(priv, PLL_CMN_CFG1);
+	unsigned long refdiv = FIELD_GET(PLL_CMN_REFDIV, cfg);
+	unsigned long fbdiv = FIELD_GET(PLL_CMN_FBDIV, cfg);
+
+	if (!refdiv || !fbdiv)
+		return 0;
+
+	return (parent_rate / refdiv) * fbdiv;
+}
+
+static int sf21_cmnpll_vco_determine_rate(struct clk_hw *hw,
+					  struct clk_rate_request *req)
+{
+	unsigned long fbdiv, refdiv;
+
+	rational_best_approximation(req->rate, req->best_parent_rate,
+				    BIT(PLL_CMN_FBDIV_BITS) - 1,
+				    BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
+				    &refdiv);
+	if (!refdiv || !fbdiv)
+		return -EINVAL;
+
+	req->rate = (req->best_parent_rate / refdiv) * fbdiv;
+
+	return 0;
+}
+
+static int sf21_cmnpll_vco_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long parent_rate)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	unsigned long flags;
+	unsigned long fbdiv, refdiv;
+	u32 val;
+	int ret;
+
+	rational_best_approximation(rate, parent_rate,
+				    BIT(PLL_CMN_FBDIV_BITS) - 1,
+				    BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
+				    &refdiv);
+	if (!refdiv || !fbdiv)
+		return -EINVAL;
+
+	spin_lock_irqsave(priv->lock, flags);
+
+	sf_rmw(priv, PLL_CMN_CFG1, PLL_CMN_REFDIV | PLL_CMN_FBDIV | PLL_CMN_PD,
+	       FIELD_PREP(PLL_CMN_REFDIV, refdiv) |
+		       FIELD_PREP(PLL_CMN_FBDIV, fbdiv));
+	sf_writel(priv, CFG_LOAD, CFG_LOAD_CMN_PLL);
+	sf_writel(priv, CFG_LOAD, 0);
+
+	ret = readl_poll_timeout_atomic(priv->base + PLL_CMN_LOCK, val, val & 1,
+					0, PLL_LOCK_TIMEOUT_US);
+	if (ret)
+		goto out_unlock;
+
+out_unlock:
+	spin_unlock_irqrestore(priv->lock, flags);
+	return ret;
+}
+
+static const struct clk_ops sf21_cmnpll_vco_ops = {
+	.recalc_rate = sf21_cmnpll_vco_recalc_rate,
+	.determine_rate = sf21_cmnpll_vco_determine_rate,
+	.set_rate = sf21_cmnpll_vco_set_rate,
+};
+
+static struct sf_clk_common cmnpll_vco = {
+	.hw.init = CLK_HW_INIT_FW_NAME("cmnpll_vco", "xin25m",
+				       &sf21_cmnpll_vco_ops, 0),
+};
+
+static unsigned long sf21_dualdiv_round_rate(unsigned long rate,
+					     unsigned long parent_rate,
+					     unsigned int range,
+					     unsigned int *diva,
+					     unsigned int *divb)
+{
+	unsigned int div = DIV_ROUND_CLOSEST(parent_rate, rate);
+	unsigned int best_diff, da, db, cur_div, cur_diff;
+
+	if (div <= 1) {
+		*diva = 1;
+		*divb = 1;
+		return parent_rate;
+	}
+
+	best_diff = div - 1;
+	*diva = 1;
+	*divb = 1;
+
+	for (da = 1; da <= range; da++) {
+		db = DIV_ROUND_CLOSEST(div, da);
+		if (db > da)
+			db = da;
+
+		cur_div = da * db;
+		if (div > cur_div)
+			cur_diff = div - cur_div;
+		else
+			cur_diff = cur_div - div;
+
+		if (cur_diff < best_diff) {
+			best_diff = cur_diff;
+			*diva = da;
+			*divb = db;
+		}
+		if (cur_diff == 0)
+			break;
+	}
+
+	return parent_rate / *diva / *divb;
+}
+
+static int sf21_cmnpll_postdiv_determine_rate(struct clk_hw *hw,
+					      struct clk_rate_request *req)
+{
+	unsigned int diva, divb;
+
+	if (!req->rate)
+		return -EINVAL;
+
+	req->rate = sf21_dualdiv_round_rate(req->rate, req->best_parent_rate,
+					    7, &diva, &divb);
+
+	return 0;
+}
+
+static int sf21_cmnpll_postdiv_set_rate(struct clk_hw *hw,
+					unsigned long rate,
+					unsigned long parent_rate)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	unsigned int diva, divb;
+	unsigned long flags;
+
+	if (!rate)
+		return -EINVAL;
+
+	sf21_dualdiv_round_rate(rate, parent_rate, 7, &diva, &divb);
+
+	spin_lock_irqsave(priv->lock, flags);
+	sf_rmw(priv, PLL_CMN_CFG1, PLL_CMN_POSTDIV1 | PLL_CMN_POSTDIV2,
+	       FIELD_PREP(PLL_CMN_POSTDIV1, diva) |
+		       FIELD_PREP(PLL_CMN_POSTDIV2, divb));
+	sf_writel(priv, CFG_LOAD, CFG_LOAD_CMN_PLL);
+	sf_writel(priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(priv->lock, flags);
+	return 0;
+}
+
+static unsigned long
+sf21_cmnpll_postdiv_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	u32 cfg = sf_readl(priv, PLL_CMN_CFG1);
+	unsigned long div1 = FIELD_GET(PLL_CMN_POSTDIV1, cfg);
+	unsigned long div2 = FIELD_GET(PLL_CMN_POSTDIV2, cfg);
+
+	if (!div1 || !div2)
+		return 0;
+
+	return parent_rate / div1 / div2;
+}
+
+static const struct clk_ops sf21_cmnpll_postdiv_ops = {
+	.recalc_rate = sf21_cmnpll_postdiv_recalc_rate,
+	.determine_rate = sf21_cmnpll_postdiv_determine_rate,
+	.set_rate = sf21_cmnpll_postdiv_set_rate,
+};
+
+static struct sf_clk_common cmnpll_postdiv = {
+	.hw.init = CLK_HW_INIT_HW("cmnpll_postdiv", &cmnpll_vco.hw,
+				  &sf21_cmnpll_postdiv_ops, 0),
+};
+
+#define PLL_DDR_CFG1		0x18
+#define  PLL_DDR_BYPASS		BIT(23)
+#define  PLL_DDR_PLLEN		BIT(22)
+#define  PLL_DDR_4PHASEEN	BIT(21)
+#define  PLL_DDR_POSTDIVEN	BIT(20)
+#define  PLL_DDR_DSMEN		BIT(19)
+#define  PLL_DDR_DACEN		BIT(18)
+#define  PLL_DDR_DSKEWCALBYP	BIT(17)
+#define  PLL_DDR_DSKEWCALCNT	GENMASK(16, 14)
+#define  PLL_DDR_DSKEWCALEN	BIT(13)
+#define  PLL_DDR_DSKEWCALIN	GENMASK(12, 1)
+#define  PLL_DDR_DSKEWFASTCAL	BIT(0)
+
+#define PLL_DDR_CFG2		0x1c
+#define  PLL_DDR_POSTDIV1	GENMASK(29, 27)
+#define  PLL_DDR_POSTDIV2	GENMASK(26, 24)
+#define  PLL_DDR_FRAC		GENMASK(23, 0)
+
+#define PLL_DDR_CFG3		0x20
+#define  PLL_DDR_FBDIV		GENMASK(17, 6)
+#define  PLL_DDR_REFDIV		GENMASK(5, 0)
+
+static unsigned long
+sf21_ddrpll_postdiv_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	u32 cfg2 = sf_readl(priv, PLL_DDR_CFG2);
+	u32 postdiv1 = FIELD_GET(PLL_DDR_POSTDIV1, cfg2);
+	u32 postdiv2 = FIELD_GET(PLL_DDR_POSTDIV2, cfg2);
+	u32 cfg3 = sf_readl(priv, PLL_DDR_CFG3);
+	u32 fbdiv = FIELD_GET(PLL_DDR_FBDIV, cfg3);
+	u32 refdiv = FIELD_GET(PLL_DDR_REFDIV, cfg3);
+
+	if (!refdiv || !fbdiv || !postdiv1 || !postdiv2)
+		return 0;
+
+	return (parent_rate / refdiv) * fbdiv / postdiv1 / postdiv2;
+}
+
+static const struct clk_ops sf21_ddrpll_postdiv_ops = {
+	.recalc_rate = sf21_ddrpll_postdiv_recalc_rate,
+};
+
+static struct sf_clk_common ddrpll_postdiv = {
+	.hw.init = CLK_HW_INIT_FW_NAME("ddrpll_postdiv", "xin25m",
+				       &sf21_ddrpll_postdiv_ops, 0),
+};
+
+#define PLL_PCIE_CFG1		0x4c
+#define  PLL_PCIE_PLLEN		BIT(31)
+#define  PLL_PCIE_POSTDIV0PRE	BIT(30)
+#define  PLL_PCIE_REFDIV	GENMASK(29, 24)
+#define  PLL_PCIE_FRAC		GENMASK(23, 0)
+
+#define PLL_PCIE_CFG2		0x50
+#define  PLL_PCIE_FOUTEN(i)	BIT(28 + (i))
+#define  PLL_PCIE_BYPASS(i)	BIT(24 + (i))
+#define  PLL_PCIE_PDIVA_OFFS(i)	(21 - 6 * (i))
+#define  PLL_PCIE_PDIVB_OFFS(i)	(18 - 6 * (i))
+#define  PLL_PCIE_PDIV_MASK	GENMASK(2, 0)
+
+#define PLL_PCIE_CFG3		0x54
+#define  PLL_PCIE_DSKEWFASTCAL	BIT(31)
+#define  PLL_PCIE_DACEN		BIT(30)
+#define  PLL_PCIE_DSMEN		BIT(29)
+#define  PLL_PCIE_DSKEWCALEN	BIT(28)
+#define  PLL_PCIE_DSKEWCALBYP	BIT(27)
+#define  PLL_PCIE_DSKEWCALCNT	GENMASK(26, 24)
+#define  PLL_PCIE_DSKEWCALIN	GENMASK(23, 12)
+#define  PLL_PCIE_FBDIV		GENMASK(11, 0)
+
+static unsigned long
+sf21_pciepll_vco_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	u32 cfg1 = sf_readl(priv, PLL_PCIE_CFG1);
+	unsigned long refdiv = FIELD_GET(PLL_PCIE_REFDIV, cfg1);
+	u32 cfg3 = sf_readl(priv, PLL_PCIE_CFG3);
+	unsigned long fbdiv = FIELD_GET(PLL_PCIE_FBDIV, cfg3);
+
+	if (!refdiv || !fbdiv)
+		return 0;
+
+	return (parent_rate / refdiv) * fbdiv / 4;
+}
+
+static int sf21_pciepll_vco_enable(struct clk_hw *hw)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	unsigned long flags;
+	u32 val;
+	int ret;
+
+	spin_lock_irqsave(priv->lock, flags);
+	sf_rmw(priv, PLL_PCIE_CFG1, 0, PLL_PCIE_PLLEN);
+	sf_writel(priv, CFG_LOAD, CFG_LOAD_PCIE_PLL);
+	sf_writel(priv, CFG_LOAD, 0);
+	ret = readl_poll_timeout_atomic(priv->base + PLL_PCIE_LOCK, val, val & 1,
+					0, PLL_LOCK_TIMEOUT_US);
+	spin_unlock_irqrestore(priv->lock, flags);
+	return ret;
+}
+
+static void sf21_pciepll_vco_disable(struct clk_hw *hw)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+	unsigned long flags;
+
+	spin_lock_irqsave(priv->lock, flags);
+	sf_rmw(priv, PLL_PCIE_CFG1, PLL_PCIE_PLLEN, 0);
+	sf_writel(priv, CFG_LOAD, CFG_LOAD_PCIE_PLL);
+	sf_writel(priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(priv->lock, flags);
+}
+
+static int sf21_pciepll_vco_is_enabled(struct clk_hw *hw)
+{
+	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
+
+	return !!(sf_readl(priv, PLL_PCIE_CFG1) & PLL_PCIE_PLLEN);
+}
+
+static const struct clk_ops sf21_pciepll_vco_ops = {
+	.enable = sf21_pciepll_vco_enable,
+	.disable = sf21_pciepll_vco_disable,
+	.is_enabled = sf21_pciepll_vco_is_enabled,
+	.recalc_rate = sf21_pciepll_vco_recalc_rate,
+};
+
+static struct sf_clk_common pciepll_vco = {
+	.hw.init = CLK_HW_INIT_FW_NAME("pciepll_vco", "xin25m",
+				       &sf21_pciepll_vco_ops,
+				       CLK_SET_RATE_GATE),
+};
+
+struct sf21_pciepll_fout {
+	struct sf_clk_common common;
+	u8 index;
+};
+
+static int sf21_pciepll_fout_enable(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_pciepll_fout *priv =
+		container_of(cmn_priv, struct sf21_pciepll_fout, common);
+	unsigned long flags;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, PLL_PCIE_CFG2, 0, PLL_PCIE_FOUTEN(priv->index));
+	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_PCIE_PLL);
+	sf_writel(cmn_priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+	return 0;
+}
+
+static void sf21_pciepll_fout_disable(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_pciepll_fout *priv =
+		container_of(cmn_priv, struct sf21_pciepll_fout, common);
+	unsigned long flags;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, PLL_PCIE_CFG2, PLL_PCIE_FOUTEN(priv->index), 0);
+	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_PCIE_PLL);
+	sf_writel(cmn_priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+}
+
+static int sf21_pciepll_fout_is_enabled(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_pciepll_fout *priv =
+		container_of(cmn_priv, struct sf21_pciepll_fout, common);
+
+	return !!(sf_readl(cmn_priv, PLL_PCIE_CFG2) &
+		  PLL_PCIE_FOUTEN(priv->index));
+}
+
+static int sf21_pciepll_fout_determine_rate(struct clk_hw *hw,
+					    struct clk_rate_request *req)
+{
+	unsigned int diva, divb;
+
+	if (!req->rate)
+		return -EINVAL;
+
+	req->rate = sf21_dualdiv_round_rate(req->rate, req->best_parent_rate,
+					    8, &diva, &divb);
+
+	return 0;
+}
+
+static int sf21_pciepll_fout_set_rate(struct clk_hw *hw,
+				      unsigned long rate,
+				      unsigned long parent_rate)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_pciepll_fout *priv =
+		container_of(cmn_priv, struct sf21_pciepll_fout, common);
+	unsigned int diva, divb;
+	unsigned long flags;
+
+	if (!rate)
+		return -EINVAL;
+
+	sf21_dualdiv_round_rate(rate, parent_rate, 8, &diva, &divb);
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, PLL_PCIE_CFG2,
+	       (PLL_PCIE_PDIV_MASK << PLL_PCIE_PDIVA_OFFS(priv->index)) |
+		       (PLL_PCIE_PDIV_MASK << PLL_PCIE_PDIVB_OFFS(priv->index)),
+	       ((diva - 1) << PLL_PCIE_PDIVA_OFFS(priv->index)) |
+		       ((divb - 1) << PLL_PCIE_PDIVB_OFFS(priv->index)));
+	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_PCIE_PLL);
+	sf_writel(cmn_priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+	return 0;
+}
+
+static unsigned long
+sf21_pciepll_fout_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_pciepll_fout *priv =
+		container_of(cmn_priv, struct sf21_pciepll_fout, common);
+	int idx = priv->index;
+	u32 cfg2 = sf_readl(cmn_priv, PLL_PCIE_CFG2);
+	ulong pdiva = (cfg2 >> PLL_PCIE_PDIVA_OFFS(idx)) & PLL_PCIE_PDIV_MASK;
+	ulong pdivb = (cfg2 >> PLL_PCIE_PDIVB_OFFS(idx)) & PLL_PCIE_PDIV_MASK;
+
+	return parent_rate / (pdiva + 1) / (pdivb + 1);
+}
+
+static const struct clk_ops sf21_pciepll_fout_ops = {
+	.enable = sf21_pciepll_fout_enable,
+	.disable = sf21_pciepll_fout_disable,
+	.is_enabled = sf21_pciepll_fout_is_enabled,
+	.recalc_rate = sf21_pciepll_fout_recalc_rate,
+	.determine_rate = sf21_pciepll_fout_determine_rate,
+	.set_rate = sf21_pciepll_fout_set_rate,
+};
+
+#define SF21_PCIEPLL_FOUT(_name, _idx, _flags)			\
+	struct sf21_pciepll_fout _name = {				\
+		.common.hw.init = CLK_HW_INIT_HW(#_name,		\
+						 &pciepll_vco.hw,	\
+						 &sf21_pciepll_fout_ops,\
+						 _flags),		\
+		.index = _idx,						\
+	}
+
+static SF21_PCIEPLL_FOUT(pciepll_fout0, 0, 0);
+static SF21_PCIEPLL_FOUT(pciepll_fout1, 1, 0);
+static SF21_PCIEPLL_FOUT(pciepll_fout2, 2, 0);
+static SF21_PCIEPLL_FOUT(pciepll_fout3, 3, 0);
+
+struct sf21_clk_muxdiv {
+	struct sf_clk_common common;
+	u16 en;
+	u8 mux_reg;
+	u8 mux_offs;
+	u8 div_reg;
+	u8 div_offs;
+};
+
+#define CRM_CLK_SEL(_x)		((_x) * 4 + 0x80)
+#define  CLK_SEL1_PLL_TEST	GENMASK(6, 4)
+#define CRM_CLK_EN		0x8c
+#define CRM_CLK_DIV(_x)		((_x) * 4 + 0x94)
+#define  CRM_CLK_DIV_MASK	GENMASK(7, 0)
+
+static unsigned long sf21_muxdiv_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	ulong div_reg = CRM_CLK_DIV(priv->div_reg);
+	u16 div_offs = priv->div_offs;
+	u16 div_val = (sf_readl(cmn_priv, div_reg) >> div_offs) &
+		      CRM_CLK_DIV_MASK;
+	div_val += 1;
+	return parent_rate / div_val;
+}
+
+static int sf21_muxdiv_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	unsigned int div;
+
+	if (!req->rate)
+		return -EINVAL;
+
+	div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
+	if (!div)
+		div = 1;
+	else if (div > CRM_CLK_DIV_MASK + 1)
+		div = CRM_CLK_DIV_MASK + 1;
+
+	req->rate = req->best_parent_rate / div;
+	return 0;
+}
+
+static int sf21_muxdiv_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	ulong div_reg = CRM_CLK_DIV(priv->div_reg);
+	u16 div_offs = priv->div_offs;
+	unsigned long flags;
+	unsigned int div;
+
+	if (!rate)
+		return -EINVAL;
+
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
+	if (div < 1)
+		div = 1;
+	else if (div > CRM_CLK_DIV_MASK + 1)
+		div = CRM_CLK_DIV_MASK + 1;
+	div -= 1;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, div_reg, CRM_CLK_DIV_MASK << div_offs,
+	       div << div_offs);
+	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_DIV);
+	sf_writel(cmn_priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+	return 0;
+}
+
+static int sf21_muxdiv_enable(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	unsigned long flags;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, CRM_CLK_EN, 0, BIT(priv->en));
+	/*
+	 * Clock divider value load only happens when the clock is running.
+	 * Pulse the CFG_LOAD_DIV so that set_rate() which happened
+	 * before enable() is applied.
+	 */
+	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_DIV);
+	sf_writel(cmn_priv, CFG_LOAD, 0);
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+	return 0;
+}
+
+static void sf21_muxdiv_disable(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	unsigned long flags;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, CRM_CLK_EN, BIT(priv->en), 0);
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+}
+
+static int sf21_muxdiv_is_enabled(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	u32 reg_val = sf_readl(cmn_priv, CRM_CLK_EN);
+
+	return reg_val & (BIT(priv->en)) ? 1 : 0;
+}
+
+static u8 sf21_muxdiv_get_parent(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	ulong mux_reg = CRM_CLK_SEL(priv->mux_reg);
+	u16 mux_offs = priv->mux_offs;
+	u32 reg_val = sf_readl(cmn_priv, mux_reg);
+
+	return reg_val & BIT(mux_offs) ? 1 : 0;
+}
+
+static int sf21_muxdiv_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	struct sf21_clk_muxdiv *priv =
+		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
+	ulong mux_reg = CRM_CLK_SEL(priv->mux_reg);
+	u16 mux_offs = priv->mux_offs;
+	unsigned long flags;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	if (index)
+		sf_rmw(cmn_priv, mux_reg, 0, BIT(mux_offs));
+	else
+		sf_rmw(cmn_priv, mux_reg, BIT(mux_offs), 0);
+
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+	return 0;
+}
+
+static const struct clk_ops sf21_clk_muxdiv_ops = {
+	.enable = sf21_muxdiv_enable,
+	.disable = sf21_muxdiv_disable,
+	.is_enabled = sf21_muxdiv_is_enabled,
+	.recalc_rate = sf21_muxdiv_recalc_rate,
+	.determine_rate = sf21_muxdiv_determine_rate,
+	.set_rate = sf21_muxdiv_set_rate,
+	.get_parent = sf21_muxdiv_get_parent,
+	.set_parent = sf21_muxdiv_set_parent,
+};
+
+#define SF21_MUXDIV(_name, _parents, _mux_reg, _mux_offs, _div_reg,	\
+		    _div_offs, _en, _flags)				\
+	struct sf21_clk_muxdiv _name = {				\
+		.common.hw.init = CLK_HW_INIT_PARENTS_HW(		\
+		#_name, _parents, &sf21_clk_muxdiv_ops, _flags),	\
+		.en = _en,						\
+		.mux_reg = _mux_reg,					\
+		.mux_offs = _mux_offs,					\
+		.div_reg = _div_reg,					\
+		.div_offs = _div_offs,					\
+	}
+
+static const struct clk_hw *clk_periph_parents[] = {
+	&cmnpll_postdiv.hw,
+	&ddrpll_postdiv.hw,
+};
+
+static const struct clk_hw *clk_ddr_parents[] = {
+	&ddrpll_postdiv.hw,
+	&cmnpll_postdiv.hw,
+};
+
+static const struct clk_hw *clk_gmac_usb_parents[] = {
+	&cmnpll_vco.hw,
+	&ddrpll_postdiv.hw,
+};
+
+static SF21_MUXDIV(muxdiv_cpu, clk_periph_parents, 0, 1, 0, 0, 0,
+		   CLK_IGNORE_UNUSED);
+static SF21_MUXDIV(muxdiv_pic, clk_periph_parents, 0, 3, 3, 16, 1,
+		   CLK_IGNORE_UNUSED);
+static SF21_MUXDIV(muxdiv_axi, clk_periph_parents, 0, 5, 0, 8, 2,
+		   CLK_IS_CRITICAL);
+static SF21_MUXDIV(muxdiv_ahb, clk_periph_parents, 0, 7, 0, 16, 3,
+		   CLK_IS_CRITICAL);
+static SF21_MUXDIV(muxdiv_apb, clk_periph_parents, 0, 9, 0, 24, 4,
+		   CLK_IS_CRITICAL);
+static SF21_MUXDIV(muxdiv_uart, clk_periph_parents, 0, 11, 1, 0, 5, 0);
+static SF21_MUXDIV(muxdiv_iram, clk_periph_parents, 0, 13, 1, 8, 6, 0);
+static SF21_MUXDIV(muxdiv_npu, clk_periph_parents, 0, 17, 1, 24, 8, 0);
+static SF21_MUXDIV(muxdiv_ddrphy, clk_ddr_parents, 0, 19, 2, 0, 9,
+		   CLK_IS_CRITICAL);
+static SF21_MUXDIV(muxdiv_ddr_bypass, clk_ddr_parents, 0, 21, 3, 0, 10,
+		   CLK_IS_CRITICAL);
+static SF21_MUXDIV(muxdiv_ethtsu, clk_periph_parents, 0, 25, 2, 16, 12,
+		   0);
+static SF21_MUXDIV(muxdiv_gmac_byp_ref, clk_gmac_usb_parents, 0, 27, 2,
+		   24, 13, 0);
+static SF21_MUXDIV(muxdiv_usb, clk_gmac_usb_parents, 1, 1, 1, 16, 24, 0);
+static SF21_MUXDIV(muxdiv_usbphy, clk_gmac_usb_parents, 1, 3, 2, 8, 25,
+		   0);
+static SF21_MUXDIV(muxdiv_serdes_csr, clk_periph_parents, 1, 15, 5, 0,
+		   20, 0);
+static SF21_MUXDIV(muxdiv_crypt_csr, clk_periph_parents, 1, 17, 5, 8,
+		   21, 0);
+static SF21_MUXDIV(muxdiv_crypt_app, clk_periph_parents, 1, 19, 5, 16,
+		   22, 0);
+static SF21_MUXDIV(muxdiv_irom, clk_periph_parents, 1, 21, 5, 24, 23,
+		   CLK_IS_CRITICAL);
+
+static int sf21_mux_determine_rate(struct clk_hw *hw,
+				   struct clk_rate_request *req)
+{
+	req->rate = req->best_parent_rate;
+	return 0;
+}
+
+static const struct clk_ops sf21_clk_mux_ops = {
+	.get_parent = sf21_muxdiv_get_parent,
+	.set_parent = sf21_muxdiv_set_parent,
+	.determine_rate = sf21_mux_determine_rate,
+};
+
+#define SF21_MUX(_name, _parents, _mux_reg, _mux_offs, _flags)	\
+	struct sf21_clk_muxdiv _name = {				\
+		.common.hw.init = CLK_HW_INIT_PARENTS_DATA(		\
+			#_name, _parents, &sf21_clk_mux_ops, _flags),	\
+		.en = 0,						\
+		.mux_reg = _mux_reg,					\
+		.mux_offs = _mux_offs,					\
+		.div_reg = 0,						\
+		.div_offs = 0,						\
+	}
+
+static const struct clk_parent_data clk_boot_parents[] = {
+	{ .hw = &muxdiv_irom.common.hw },
+	{ .fw_name = "xin25m" },
+};
+
+static SF21_MUX(mux_boot, clk_boot_parents, 0, 30, CLK_IS_CRITICAL);
+
+static const struct clk_ops sf21_clk_div_ops = {
+	.recalc_rate = sf21_muxdiv_recalc_rate,
+	.determine_rate = sf21_muxdiv_determine_rate,
+	.set_rate = sf21_muxdiv_set_rate,
+};
+
+#define SF21_DIV(_name, _parent, _div_reg, _div_offs, _flags)		\
+	struct sf21_clk_muxdiv _name = {				\
+		.common.hw.init = CLK_HW_INIT_FW_NAME(			\
+			#_name, _parent, &sf21_clk_div_ops, _flags),	\
+		.en = 0,						\
+		.mux_reg = 0,						\
+		.mux_offs = 0,						\
+		.div_reg = _div_reg,					\
+		.div_offs = _div_offs,					\
+	}
+
+static SF21_DIV(div_pvt, "xin25m", 3, 8, 0);
+
+static const struct clk_hw *clk_pll_test_parents[] = {
+	&cmnpll_postdiv.hw,
+	&ddrpll_postdiv.hw,
+	&pciepll_fout3.common.hw,
+};
+
+static u8 sf21_pll_test_get_parent(struct clk_hw *hw)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	u32 reg_val = sf_readl(cmn_priv, CRM_CLK_SEL(1));
+	u8 parent = FIELD_GET(CLK_SEL1_PLL_TEST, reg_val);
+
+	if (parent >= ARRAY_SIZE(clk_pll_test_parents))
+		return 0;
+
+	return parent;
+}
+
+static int sf21_pll_test_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
+	unsigned long flags;
+
+	if (index >= ARRAY_SIZE(clk_pll_test_parents))
+		return -EINVAL;
+
+	spin_lock_irqsave(cmn_priv->lock, flags);
+	sf_rmw(cmn_priv, CRM_CLK_SEL(1), CLK_SEL1_PLL_TEST,
+	       FIELD_PREP(CLK_SEL1_PLL_TEST, index));
+	spin_unlock_irqrestore(cmn_priv->lock, flags);
+	return 0;
+}
+
+static const struct clk_ops sf21_clk_pll_test_ops = {
+	.recalc_rate = sf21_muxdiv_recalc_rate,
+	.determine_rate = sf21_muxdiv_determine_rate,
+	.set_rate = sf21_muxdiv_set_rate,
+	.get_parent = sf21_pll_test_get_parent,
+	.set_parent = sf21_pll_test_set_parent,
+};
+
+static struct sf21_clk_muxdiv muxdiv_pll_test = {
+	.common.hw.init = CLK_HW_INIT_PARENTS_HW("muxdiv_pll_test",
+						 clk_pll_test_parents,
+						 &sf21_clk_pll_test_ops, 0),
+	.en = 0,
+	.mux_reg = 0,
+	.mux_offs = 0,
+	.div_reg = 3,
+	.div_offs = 24,
+};
+
+static const struct clk_ops sf21_clk_gate_ops = {
+	.enable = sf21_muxdiv_enable,
+	.disable = sf21_muxdiv_disable,
+	.is_enabled = sf21_muxdiv_is_enabled,
+};
+
+#define SF21_GATE(_name, _parents, _en, _flags)				\
+	struct sf21_clk_muxdiv _name = {				\
+		.common.hw.init = CLK_HW_INIT_PARENTS_HW(		\
+			#_name, _parents, &sf21_clk_gate_ops, _flags),	\
+		.en = _en,						\
+		.mux_reg = 0,						\
+		.mux_offs = 0,						\
+		.div_reg = 0,						\
+		.div_offs = 0,						\
+	}
+
+static const struct clk_hw *clk_pcie_parents[] = {
+	&pciepll_fout1.common.hw,
+};
+
+static SF21_GATE(pcie_refclk_n, clk_pcie_parents, 15, 0);
+static SF21_GATE(pcie_refclk_p, clk_pcie_parents, 16, 0);
+
+static struct clk_hw_onecell_data sf21_hw_clks = {
+	.num = SF21_CLK_MAX,
+	.hws = {
+		[SF21_CLK_CMNPLL_VCO] = &cmnpll_vco.hw,
+		[SF21_CLK_CMNPLL_POSTDIV] = &cmnpll_postdiv.hw,
+		[SF21_CLK_DDRPLL_POSTDIV] = &ddrpll_postdiv.hw,
+		[SF21_CLK_PCIEPLL_VCO] = &pciepll_vco.hw,
+		[SF21_CLK_PCIEPLL_FOUT0] = &pciepll_fout0.common.hw,
+		[SF21_CLK_PCIEPLL_FOUT1] = &pciepll_fout1.common.hw,
+		[SF21_CLK_PCIEPLL_FOUT2] = &pciepll_fout2.common.hw,
+		[SF21_CLK_PCIEPLL_FOUT3] = &pciepll_fout3.common.hw,
+		[SF21_CLK_CPU] = &muxdiv_cpu.common.hw,
+		[SF21_CLK_PIC] = &muxdiv_pic.common.hw,
+		[SF21_CLK_AXI] = &muxdiv_axi.common.hw,
+		[SF21_CLK_AHB] = &muxdiv_ahb.common.hw,
+		[SF21_CLK_APB] = &muxdiv_apb.common.hw,
+		[SF21_CLK_UART] = &muxdiv_uart.common.hw,
+		[SF21_CLK_IRAM] = &muxdiv_iram.common.hw,
+		[SF21_CLK_NPU] = &muxdiv_npu.common.hw,
+		[SF21_CLK_DDRPHY_REF] = &muxdiv_ddrphy.common.hw,
+		[SF21_CLK_DDR_BYPASS] = &muxdiv_ddr_bypass.common.hw,
+		[SF21_CLK_ETHTSU] = &muxdiv_ethtsu.common.hw,
+		[SF21_CLK_GMAC_BYP_REF] = &muxdiv_gmac_byp_ref.common.hw,
+		[SF21_CLK_USB] = &muxdiv_usb.common.hw,
+		[SF21_CLK_USBPHY] = &muxdiv_usbphy.common.hw,
+		[SF21_CLK_SERDES_CSR] = &muxdiv_serdes_csr.common.hw,
+		[SF21_CLK_CRYPT_CSR] = &muxdiv_crypt_csr.common.hw,
+		[SF21_CLK_CRYPT_APP] = &muxdiv_crypt_app.common.hw,
+		[SF21_CLK_IROM] = &muxdiv_irom.common.hw,
+		[SF21_CLK_BOOT] = &mux_boot.common.hw,
+		[SF21_CLK_PVT] = &div_pvt.common.hw,
+		[SF21_CLK_PLL_TEST] = &muxdiv_pll_test.common.hw,
+		[SF21_CLK_PCIE_REFN] = &pcie_refclk_n.common.hw,
+		[SF21_CLK_PCIE_REFP] = &pcie_refclk_p.common.hw,
+	}
+};
+
+struct sf21_clk_ctrl {
+	void __iomem *base;
+	/* Serializes register RMW sequences shared by clocks and resets. */
+	spinlock_t lock;
+	struct reset_controller_dev rcdev;
+	const u32 *reset_bits;
+	unsigned int nr_resets;
+};
+
+#define SF21_SOFT_RESET		0xc0
+
+static const u32 sf21_topcrm_reset_bits[] = {
+	[SF21_RESET_GIC]			= BIT(1),
+	[SF21_RESET_AXI]			= BIT(2),
+	[SF21_RESET_AHB]			= BIT(3),
+	[SF21_RESET_APB]			= BIT(4),
+	[SF21_RESET_IRAM]			= BIT(5),
+	[SF21_RESET_NPU]			= BIT(7),
+	[SF21_RESET_DDR_CTL]			= BIT(8),
+	[SF21_RESET_DDR_PHY]			= BIT(9),
+	[SF21_RESET_DDR_PWR_OK_IN]		= BIT(10),
+	[SF21_RESET_DDR_CTL_APB]		= BIT(11),
+	[SF21_RESET_DDR_PHY_APB]		= BIT(12),
+	[SF21_RESET_USB]			= BIT(19),
+	[SF21_RESET_PVT]			= BIT(23),
+	[SF21_RESET_SERDES_CSR]			= BIT(24),
+	[SF21_RESET_CRYPT_CSR]			= BIT(28),
+	[SF21_RESET_CRYPT_APP]			= BIT(29),
+	[SF21_RESET_NPU2DDR_ASYNCBRIDGE]	= BIT(30),
+	[SF21_RESET_IROM]			= BIT(31),
+};
+
+static inline struct sf21_clk_ctrl *
+rcdev_to_sf21_topcrm(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct sf21_clk_ctrl, rcdev);
+}
+
+static int sf21_topcrm_reset_update(struct reset_controller_dev *rcdev,
+				    unsigned long id, bool assert)
+{
+	struct sf21_clk_ctrl *ctrl = rcdev_to_sf21_topcrm(rcdev);
+	u32 bit = ctrl->reset_bits[id];
+	unsigned long flags;
+	u32 reg;
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+	reg = readl(ctrl->base + SF21_SOFT_RESET);
+	if (assert)
+		reg &= ~bit;
+	else
+		reg |= bit;
+	writel(reg, ctrl->base + SF21_SOFT_RESET);
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static int sf21_topcrm_reset_assert(struct reset_controller_dev *rcdev,
+				    unsigned long id)
+{
+	return sf21_topcrm_reset_update(rcdev, id, true);
+}
+
+static int sf21_topcrm_reset_deassert(struct reset_controller_dev *rcdev,
+				      unsigned long id)
+{
+	return sf21_topcrm_reset_update(rcdev, id, false);
+}
+
+static int sf21_topcrm_reset_status(struct reset_controller_dev *rcdev,
+				    unsigned long id)
+{
+	struct sf21_clk_ctrl *ctrl = rcdev_to_sf21_topcrm(rcdev);
+	u32 bit = ctrl->reset_bits[id];
+
+	return !(readl(ctrl->base + SF21_SOFT_RESET) & bit);
+}
+
+static const struct reset_control_ops sf21_topcrm_reset_ops = {
+	.assert = sf21_topcrm_reset_assert,
+	.deassert = sf21_topcrm_reset_deassert,
+	.status = sf21_topcrm_reset_status,
+};
+
+static int sf21_topcrm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sf21_clk_ctrl *ctrl;
+	int i, ret;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return dev_err_probe(dev, PTR_ERR(ctrl->base),
+				     "failed to map resources\n");
+
+	spin_lock_init(&ctrl->lock);
+
+	for (i = 0; i < sf21_hw_clks.num; i++) {
+		struct clk_hw *hw = sf21_hw_clks.hws[i];
+		struct sf_clk_common *common;
+
+		if (!hw)
+			continue;
+		common = hw_to_sf_clk_common(hw);
+		common->base = ctrl->base;
+		common->lock = &ctrl->lock;
+		ret = devm_clk_hw_register(dev, hw);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to register clock %d\n",
+					     i);
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					  &sf21_hw_clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add hw provider\n");
+
+	ctrl->reset_bits = sf21_topcrm_reset_bits;
+	ctrl->nr_resets = ARRAY_SIZE(sf21_topcrm_reset_bits);
+	ctrl->rcdev.owner = THIS_MODULE;
+	ctrl->rcdev.nr_resets = ctrl->nr_resets;
+	ctrl->rcdev.ops = &sf21_topcrm_reset_ops;
+	ctrl->rcdev.of_node = dev->of_node;
+	ctrl->rcdev.of_reset_n_cells = 1;
+
+	ret = devm_reset_controller_register(dev, &ctrl->rcdev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register reset controller\n");
+
+	return 0;
+}
+
+static const struct of_device_id sf21_topcrm_dt_ids[] = {
+	{ .compatible = "siflower,sf21-topcrm" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, sf21_topcrm_dt_ids);
+
+static struct platform_driver sf21_topcrm_driver = {
+	.probe	= sf21_topcrm_probe,
+	.driver	= {
+		.name			= "sf21-topcrm",
+		.suppress_bind_attrs	= true,
+		.of_match_table		= sf21_topcrm_dt_ids,
+	},
+};
+module_platform_driver(sf21_topcrm_driver);
+
+MODULE_AUTHOR("Chuanhong Guo <gch981213@gmail.com>");
+MODULE_DESCRIPTION("driver for Siflower SF21 top clock and reset module");
+MODULE_LICENSE("GPL");

-- 
2.54.0

Re: [PATCH 4/4] clk: add support for siflower sf21-topcrm

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Static initialization of the flexible array in `sf21_hw_clks` creates a critical out-of-bounds memory access vulnerability.
- [High] Lock nesting violation on PREEMPT_RT due to `spinlock_t` usage inside `.enable`/`.disable` callbacks.
- [High] Global static clock definition structures are modified with per-device dynamically allocated pointers.
- [High] Missing `select RATIONAL` in Kconfig leads to linker errors.
- [Medium] Integer truncation in PLL rate calculations causes precision loss.
- [Medium] Silent acceptance of invalid or unsupported soft reset IDs.
--

commit 83e3e7ec9410a85a7d5912f8b3485089ca10bdce
Author: Chuanhong Guo <gch981213@gmail.com>

clk: add support for siflower sf21-topcrm

This commit adds a driver for the toplevel clock and reset controller
found on Siflower SF21A6826/SF21H8898 SoCs. It includes support for
multiple PLLs, mux/gate/divider blocks, and a reset register.

> diff --git a/drivers/clk/siflower/Kconfig b/drivers/clk/siflower/Kconfig
> new file mode 100644
> index 0000000000000..03cbfbdbdb8db
> --- /dev/null
> +++ b/drivers/clk/siflower/Kconfig
> @@ -0,0 +1,22 @@
[ ... ]
> +config CLK_SF21_TOPCRM
> +	tristate "Clock driver for Siflower SF21 toplevel clock & reset module"
> +	depends on ARCH_SIFLOWER || COMPILE_TEST
> +	default ARCH_SIFLOWER
> +	select RESET_CONTROLLER

Should this select RATIONAL? The driver calls rational_best_approximation()
in rate calculations, which could lead to linker errors if RATIONAL is not
enabled during a COMPILE_TEST build.

[ ... ]
> diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> new file mode 100644
> index 0000000000000..7d4c5e370d6d2
> --- /dev/null
> +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
> @@ -0,0 +1,1053 @@
[ ... ]
> +struct sf_clk_common {
> +	void __iomem	*base;
> +	/* Serializes register RMW sequences shared by clocks and resets. */
> +	spinlock_t	*lock;

Could this lock nesting cause issues on PREEMPT_RT?

The Common Clock Framework invokes .enable and .disable callbacks while
holding its own enable_lock, which is a raw_spinlock_t. Acquiring a
spinlock_t inside a raw spinlock critical section is a lock nesting
violation and can lead to a scheduling-while-atomic panic on PREEMPT_RT
kernels.

Does this hardware lock need to be a raw_spinlock_t?

> +	struct clk_hw	hw;
> +};
[ ... ]
> +static unsigned long sf21_cmnpll_vco_recalc_rate(struct clk_hw *hw,
> +						 unsigned long parent_rate)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	u32 cfg = sf_readl(priv, PLL_CMN_CFG1);
> +	unsigned long refdiv = FIELD_GET(PLL_CMN_REFDIV, cfg);
> +	unsigned long fbdiv = FIELD_GET(PLL_CMN_FBDIV, cfg);
> +
> +	if (!refdiv || !fbdiv)
> +		return 0;
> +
> +	return (parent_rate / refdiv) * fbdiv;

Could this integer division truncate the remainder and cause precision
loss in the reported frequency?

This pattern also appears in other rate calculation functions like
sf21_pciepll_vco_recalc_rate and sf21_ddrpll_postdiv_recalc_rate. Would it
be better to use 64-bit math such as div_u64((u64)parent_rate * fbdiv,
refdiv) or mult_frac()?

> +}
[ ... ]
> +static struct clk_hw_onecell_data sf21_hw_clks = {
> +	.num = SF21_CLK_MAX,
> +	.hws = {
> +		[SF21_CLK_CMNPLL_VCO] = &cmnpll_vco.hw,
[ ... ]
> +		[SF21_CLK_PCIE_REFN] = &pcie_refclk_n.common.hw,
> +		[SF21_CLK_PCIE_REFP] = &pcie_refclk_p.common.hw,
> +	}
> +};

If SF21_CLK_MAX is larger than the number of items explicitly initialized
in this array, could this lead to an out-of-bounds memory read?

When statically initialized, the compiler only allocates space up to the
highest explicitly initialized index (SF21_CLK_PCIE_REFP). If the loop in
the probe function reads up to sf21_hw_clks.num, it could read past the end
of the allocated structure.

[ ... ]
> +struct sf21_clk_ctrl {
> +	void __iomem *base;
> +	/* Serializes register RMW sequences shared by clocks and resets. */
> +	spinlock_t lock;
> +	struct reset_controller_dev rcdev;
> +	const u32 *reset_bits;
> +	unsigned int nr_resets;
> +};
[ ... ]
> +static int sf21_topcrm_reset_update(struct reset_controller_dev *rcdev,
> +				    unsigned long id, bool assert)
> +{
> +	struct sf21_clk_ctrl *ctrl = rcdev_to_sf21_topcrm(rcdev);
> +	u32 bit = ctrl->reset_bits[id];

Can this accept invalid or unsupported reset IDs? Since the
sf21_topcrm_reset_bits array has unpopulated gaps, bit will evaluate to 0
for unsupported IDs.

If this happens, the code blindly performs reg &= ~0 or reg |= 0 and
returns success without throwing an error. Should it verify that the bit
is valid and return -EINVAL if not?

> +	unsigned long flags;
> +	u32 reg;
> +
> +	spin_lock_irqsave(&ctrl->lock, flags);
> +	reg = readl(ctrl->base + SF21_SOFT_RESET);
> +	if (assert)
> +		reg &= ~bit;
> +	else
> +		reg |= bit;
> +	writel(reg, ctrl->base + SF21_SOFT_RESET);
> +	spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +	return 0;
> +}
[ ... ]
> +static int sf21_topcrm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sf21_clk_ctrl *ctrl;
> +	int i, ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ctrl->base))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->base),
> +				     "failed to map resources\n");
> +
> +	spin_lock_init(&ctrl->lock);
> +
> +	for (i = 0; i < sf21_hw_clks.num; i++) {
> +		struct clk_hw *hw = sf21_hw_clks.hws[i];
> +		struct sf_clk_common *common;
> +
> +		if (!hw)
> +			continue;
> +		common = hw_to_sf_clk_common(hw);
> +		common->base = ctrl->base;
> +		common->lock = &ctrl->lock;

Could modifying global static clock definitions with per-device pointers
lead to a use-after-free?

If probe defers or fails later, ctrl is freed by devres, leaving the
global clock objects pointing to dangling memory. Subsequent probes could
then encounter corrupted pointers.

> +		ret = devm_clk_hw_register(dev, hw);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to register clock %d\n",
> +					     i);
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-sf21-topcrm-v1-0-438f2e0513ff@gmail.com?part=4

Re: [PATCH 4/4] clk: add support for siflower sf21-topcrm

[Chuanhong Guo]

On Sun, May 17, 2026 at 11:09 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [Critical] Static initialization of the flexible array in `sf21_hw_clks` creates a critical out-of-bounds memory access vulnerability.
> - [High] Lock nesting violation on PREEMPT_RT due to `spinlock_t` usage inside `.enable`/`.disable` callbacks.
> - [High] Global static clock definition structures are modified with per-device dynamically allocated pointers.
> - [High] Missing `select RATIONAL` in Kconfig leads to linker errors.
> - [Medium] Integer truncation in PLL rate calculations causes precision loss.
> - [Medium] Silent acceptance of invalid or unsupported soft reset IDs.
> --
>
> commit 83e3e7ec9410a85a7d5912f8b3485089ca10bdce
> Author: Chuanhong Guo <gch981213@gmail.com>
>
> clk: add support for siflower sf21-topcrm
>
> This commit adds a driver for the toplevel clock and reset controller
> found on Siflower SF21A6826/SF21H8898 SoCs. It includes support for
> multiple PLLs, mux/gate/divider blocks, and a reset register.
>
> > diff --git a/drivers/clk/siflower/Kconfig b/drivers/clk/siflower/Kconfig
> > new file mode 100644
> > index 0000000000000..03cbfbdbdb8db
> > --- /dev/null
> > +++ b/drivers/clk/siflower/Kconfig
> > @@ -0,0 +1,22 @@
> [ ... ]
> > +config CLK_SF21_TOPCRM
> > +     tristate "Clock driver for Siflower SF21 toplevel clock & reset module"
> > +     depends on ARCH_SIFLOWER || COMPILE_TEST
> > +     default ARCH_SIFLOWER
> > +     select RESET_CONTROLLER
>
> Should this select RATIONAL? The driver calls rational_best_approximation()
> in rate calculations, which could lead to linker errors if RATIONAL is not
> enabled during a COMPILE_TEST build.

Correct. I'll fix this in v2.

>
> [ ... ]
> > diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> > new file mode 100644
> > index 0000000000000..7d4c5e370d6d2
> > --- /dev/null
> > +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
> > @@ -0,0 +1,1053 @@
> [ ... ]
> > +struct sf_clk_common {
> > +     void __iomem    *base;
> > +     /* Serializes register RMW sequences shared by clocks and resets. */
> > +     spinlock_t      *lock;
>
> Could this lock nesting cause issues on PREEMPT_RT?
>
> The Common Clock Framework invokes .enable and .disable callbacks while
> holding its own enable_lock, which is a raw_spinlock_t. Acquiring a
> spinlock_t inside a raw spinlock critical section is a lock nesting
> violation and can lead to a scheduling-while-atomic panic on PREEMPT_RT
> kernels.
>
> Does this hardware lock need to be a raw_spinlock_t?

I'll change this in v2.
I'll probably also split this into clock_lock and reset_lock since the latter
doesn't need raw_spinlock_t.

>
> > +     struct clk_hw   hw;
> > +};
> [ ... ]
> > +static unsigned long sf21_cmnpll_vco_recalc_rate(struct clk_hw *hw,
> > +                                              unsigned long parent_rate)
> > +{
> > +     struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> > +     u32 cfg = sf_readl(priv, PLL_CMN_CFG1);
> > +     unsigned long refdiv = FIELD_GET(PLL_CMN_REFDIV, cfg);
> > +     unsigned long fbdiv = FIELD_GET(PLL_CMN_FBDIV, cfg);
> > +
> > +     if (!refdiv || !fbdiv)
> > +             return 0;
> > +
> > +     return (parent_rate / refdiv) * fbdiv;
>
> Could this integer division truncate the remainder and cause precision
> loss in the reported frequency?
>
> This pattern also appears in other rate calculation functions like
> sf21_pciepll_vco_recalc_rate and sf21_ddrpll_postdiv_recalc_rate. Would it
> be better to use 64-bit math such as div_u64((u64)parent_rate * fbdiv,
> refdiv) or mult_frac()?

Sure. I'll use div_u64 in v2.

>
> > +}
> [ ... ]
> > +static struct clk_hw_onecell_data sf21_hw_clks = {
> > +     .num = SF21_CLK_MAX,
> > +     .hws = {
> > +             [SF21_CLK_CMNPLL_VCO] = &cmnpll_vco.hw,
> [ ... ]
> > +             [SF21_CLK_PCIE_REFN] = &pcie_refclk_n.common.hw,
> > +             [SF21_CLK_PCIE_REFP] = &pcie_refclk_p.common.hw,
> > +     }
> > +};
>
> If SF21_CLK_MAX is larger than the number of items explicitly initialized
> in this array, could this lead to an out-of-bounds memory read?
>
> When statically initialized, the compiler only allocates space up to the
> highest explicitly initialized index (SF21_CLK_PCIE_REFP). If the loop in
> the probe function reads up to sf21_hw_clks.num, it could read past the end
> of the allocated structure.

SF21_CLK_MAX = SF21_CLK_PCIE_REFP + 1 so this won't happen.

>
> [ ... ]
> > +struct sf21_clk_ctrl {
> > +     void __iomem *base;
> > +     /* Serializes register RMW sequences shared by clocks and resets. */
> > +     spinlock_t lock;
> > +     struct reset_controller_dev rcdev;
> > +     const u32 *reset_bits;
> > +     unsigned int nr_resets;
> > +};
> [ ... ]
> > +static int sf21_topcrm_reset_update(struct reset_controller_dev *rcdev,
> > +                                 unsigned long id, bool assert)
> > +{
> > +     struct sf21_clk_ctrl *ctrl = rcdev_to_sf21_topcrm(rcdev);
> > +     u32 bit = ctrl->reset_bits[id];
>
> Can this accept invalid or unsupported reset IDs? Since the
> sf21_topcrm_reset_bits array has unpopulated gaps, bit will evaluate to 0
> for unsupported IDs.
>
> If this happens, the code blindly performs reg &= ~0 or reg |= 0 and
> returns success without throwing an error. Should it verify that the bit
> is valid and return -EINVAL if not?

This should have been guarded by fwnode_reset_simple_xlate check
on nr_resets.

>
> > +     unsigned long flags;
> > +     u32 reg;
> > +
> > +     spin_lock_irqsave(&ctrl->lock, flags);
> > +     reg = readl(ctrl->base + SF21_SOFT_RESET);
> > +     if (assert)
> > +             reg &= ~bit;
> > +     else
> > +             reg |= bit;
> > +     writel(reg, ctrl->base + SF21_SOFT_RESET);
> > +     spin_unlock_irqrestore(&ctrl->lock, flags);
> > +
> > +     return 0;
> > +}
> [ ... ]
> > +static int sf21_topcrm_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct sf21_clk_ctrl *ctrl;
> > +     int i, ret;
> > +
> > +     ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> > +     if (!ctrl)
> > +             return -ENOMEM;
> > +
> > +     ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(ctrl->base))
> > +             return dev_err_probe(dev, PTR_ERR(ctrl->base),
> > +                                  "failed to map resources\n");
> > +
> > +     spin_lock_init(&ctrl->lock);
> > +
> > +     for (i = 0; i < sf21_hw_clks.num; i++) {
> > +             struct clk_hw *hw = sf21_hw_clks.hws[i];
> > +             struct sf_clk_common *common;
> > +
> > +             if (!hw)
> > +                     continue;
> > +             common = hw_to_sf_clk_common(hw);
> > +             common->base = ctrl->base;
> > +             common->lock = &ctrl->lock;
>
> Could modifying global static clock definitions with per-device pointers
> lead to a use-after-free?
>
> If probe defers or fails later, ctrl is freed by devres, leaving the
> global clock objects pointing to dangling memory. Subsequent probes could
> then encounter corrupted pointers.

No. All the clock registers are called with devm_ variant. Before
freeing ctrl, all the
clocks should already have been unregistered and shouldn't be touched
by any code until it's registered next time.

-- 
Regards,
Chuanhong Guo

Re: [PATCH 4/4] clk: add support for siflower sf21-topcrm

[Yao Zi]

On Sun, May 17, 2026 at 10:12:58PM +0800, Chuanhong Guo wrote:
> This commit adds a driver for the Toplevel clock and reset controller
> found on Siflower SF21A6826/SF21H8898 SoCs.
> This block contains control for 3 PLLs, several clock mux/gate/divider
> blocks, and a reset register for on-chip peripherals.

It would be better if you could split out the reset code into
drivers/reset, and initialize the reset controller as an auxiliary
device, like what has been done for SpacemiT platform
(drivers/{clock,reset}/spacemit) and AMLogic platform
(drivers/clock/meson/axg-audio.c and
drivers/reset/amlogic/reset-meson-aux.c).

I am neither clock nor reset maintainer, thus this only serves as
a suggestion, with which it ends up in better code structure.

> There are also two registers for enabling PCIE clock output in this
> block. They aren't covered by this patch because I can't test those
> without a PCIE driver. These will be added with the PCIE driver

s/PCIE/PCIe/g which is the formal spelling.

> patchset later after I get that working.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  drivers/clk/Kconfig                    |    1 +
>  drivers/clk/Makefile                   |    1 +
>  drivers/clk/siflower/Kconfig           |   22 +
>  drivers/clk/siflower/Makefile          |    1 +
>  drivers/clk/siflower/clk-sf21-topcrm.c | 1053 ++++++++++++++++++++++++++++++++
>  5 files changed, 1078 insertions(+)

...

> diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> new file mode 100644
> index 000000000000..7d4c5e370d6d
> --- /dev/null
> +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
> @@ -0,0 +1,1053 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/clk-provider.h>
> +#include <linux/bitfield.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/rational.h>
> +#include <linux/module.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/siflower,sf21-topcrm.h>

Consider sorting the headers?

...

> +static int sf21_cmnpll_vco_determine_rate(struct clk_hw *hw,
> +					  struct clk_rate_request *req)
> +{
> +	unsigned long fbdiv, refdiv;
> +
> +	rational_best_approximation(req->rate, req->best_parent_rate,
> +				    BIT(PLL_CMN_FBDIV_BITS) - 1,
> +				    BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,

FIELD_MAX(PLL_CMN_{FBDIV,REFDIV}_BITS}) would be simpler, with which it
should be possible to get avoid of PLL_CMN_*_BITS.

> +				    &refdiv);
> +	if (!refdiv || !fbdiv)
> +		return -EINVAL;
> +
> +	req->rate = (req->best_parent_rate / refdiv) * fbdiv;
> +
> +	return 0;
> +}
> +
> +static int sf21_cmnpll_vco_set_rate(struct clk_hw *hw, unsigned long rate,
> +				    unsigned long parent_rate)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	unsigned long flags;
> +	unsigned long fbdiv, refdiv;
> +	u32 val;
> +	int ret;
> +
> +	rational_best_approximation(rate, parent_rate,
> +				    BIT(PLL_CMN_FBDIV_BITS) - 1,
> +				    BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
> +				    &refdiv);
> +	if (!refdiv || !fbdiv)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(priv->lock, flags);

	guard(spinlock_irqsave)(priv->lock)

might simplify the code (especially the error handling path in this
function), this applies as well for other places where
spin_lock_irqsave() involves.

> +
> +	sf_rmw(priv, PLL_CMN_CFG1, PLL_CMN_REFDIV | PLL_CMN_FBDIV | PLL_CMN_PD,
> +	       FIELD_PREP(PLL_CMN_REFDIV, refdiv) |
> +		       FIELD_PREP(PLL_CMN_FBDIV, fbdiv));
> +	sf_writel(priv, CFG_LOAD, CFG_LOAD_CMN_PLL);
> +	sf_writel(priv, CFG_LOAD, 0);
> +
> +	ret = readl_poll_timeout_atomic(priv->base + PLL_CMN_LOCK, val, val & 1,
> +					0, PLL_LOCK_TIMEOUT_US);
> +	if (ret)
> +		goto out_unlock;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(priv->lock, flags);
> +	return ret;
> +}

...

> +static unsigned long sf21_dualdiv_round_rate(unsigned long rate,
> +					     unsigned long parent_rate,
> +					     unsigned int range,
> +					     unsigned int *diva,
> +					     unsigned int *divb)
> +{
> +	unsigned int div = DIV_ROUND_CLOSEST(parent_rate, rate);
> +	unsigned int best_diff, da, db, cur_div, cur_diff;
> +
> +	if (div <= 1) {
> +		*diva = 1;
> +		*divb = 1;
> +		return parent_rate;
> +	}
> +
> +	best_diff = div - 1;
> +	*diva = 1;
> +	*divb = 1;
> +
> +	for (da = 1; da <= range; da++) {
> +		db = DIV_ROUND_CLOSEST(div, da);
> +		if (db > da)
> +			db = da;
> +
> +		cur_div = da * db;
> +		if (div > cur_div)
> +			cur_diff = div - cur_div;
> +		else
> +			cur_diff = cur_div - div;
> +
> +		if (cur_diff < best_diff) {
> +			best_diff = cur_diff;
> +			*diva = da;
> +			*divb = db;
> +		}
> +		if (cur_diff == 0)
> +			break;
> +	}
> +
> +	return parent_rate / *diva / *divb;
> +}
> +
> +static int sf21_cmnpll_postdiv_determine_rate(struct clk_hw *hw,
> +					      struct clk_rate_request *req)
> +{
> +	unsigned int diva, divb;
> +
> +	if (!req->rate)
> +		return -EINVAL;
> +
> +	req->rate = sf21_dualdiv_round_rate(req->rate, req->best_parent_rate,
> +					    7, &diva, &divb);
> +
> +	return 0;
> +}
> +
> +static int sf21_cmnpll_postdiv_set_rate(struct clk_hw *hw,
> +					unsigned long rate,
> +					unsigned long parent_rate)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	unsigned int diva, divb;
> +	unsigned long flags;
> +
> +	if (!rate)
> +		return -EINVAL;
> +
> +	sf21_dualdiv_round_rate(rate, parent_rate, 7, &diva, &divb);
> +
> +	spin_lock_irqsave(priv->lock, flags);
> +	sf_rmw(priv, PLL_CMN_CFG1, PLL_CMN_POSTDIV1 | PLL_CMN_POSTDIV2,
> +	       FIELD_PREP(PLL_CMN_POSTDIV1, diva) |
> +		       FIELD_PREP(PLL_CMN_POSTDIV2, divb));
> +	sf_writel(priv, CFG_LOAD, CFG_LOAD_CMN_PLL);
> +	sf_writel(priv, CFG_LOAD, 0);
> +	spin_unlock_irqrestore(priv->lock, flags);
> +	return 0;
> +}
> +
> +static unsigned long
> +sf21_cmnpll_postdiv_recalc_rate(struct clk_hw *hw,
> +				unsigned long parent_rate)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	u32 cfg = sf_readl(priv, PLL_CMN_CFG1);
> +	unsigned long div1 = FIELD_GET(PLL_CMN_POSTDIV1, cfg);
> +	unsigned long div2 = FIELD_GET(PLL_CMN_POSTDIV2, cfg);
> +
> +	if (!div1 || !div2)
> +		return 0;
> +
> +	return parent_rate / div1 / div2;
> +}
> +
> +static const struct clk_ops sf21_cmnpll_postdiv_ops = {
> +	.recalc_rate = sf21_cmnpll_postdiv_recalc_rate,
> +	.determine_rate = sf21_cmnpll_postdiv_determine_rate,
> +	.set_rate = sf21_cmnpll_postdiv_set_rate,
> +};
> +
> +static struct sf_clk_common cmnpll_postdiv = {
> +	.hw.init = CLK_HW_INIT_HW("cmnpll_postdiv", &cmnpll_vco.hw,
> +				  &sf21_cmnpll_postdiv_ops, 0),
> +};

...

> +static int sf21_pciepll_fout_determine_rate(struct clk_hw *hw,
> +					    struct clk_rate_request *req)
> +{
> +	unsigned int diva, divb;
> +
> +	if (!req->rate)
> +		return -EINVAL;
> +
> +	req->rate = sf21_dualdiv_round_rate(req->rate, req->best_parent_rate,
> +					    8, &diva, &divb);

cmnpll_postdiv works in a very similar way to pciepll_fout. As you could
see in the code, they both contain two divisors to configure, and their
rates could all be calculated through sf21_dualdiv_round_rate(),

> +	return 0;
> +}

...

> +static const struct clk_ops sf21_pciepll_fout_ops = {
> +	.enable = sf21_pciepll_fout_enable,
> +	.disable = sf21_pciepll_fout_disable,
> +	.is_enabled = sf21_pciepll_fout_is_enabled,

Besides field/register offsets, the only difference I could tell between
cmnpll_postdiv and pciepll_fout is that pciepll_fout clocks could be
gated.

Would it be a good idea to describe the gating function separately as a
clock, and merge the common part of pciepll_fout and cmnpll_postdiv? In
which way you could save a lot of mostly duplicated code.

> +	.recalc_rate = sf21_pciepll_fout_recalc_rate,
> +	.determine_rate = sf21_pciepll_fout_determine_rate,
> +	.set_rate = sf21_pciepll_fout_set_rate,
> +};

...

> +struct sf21_clk_muxdiv {
> +	struct sf_clk_common common;
> +	u16 en;
> +	u8 mux_reg;
> +	u8 mux_offs;
> +	u8 div_reg;
> +	u8 div_offs;
> +};
> +
> +#define CRM_CLK_SEL(_x)		((_x) * 4 + 0x80)
> +#define  CLK_SEL1_PLL_TEST	GENMASK(6, 4)
> +#define CRM_CLK_EN		0x8c
> +#define CRM_CLK_DIV(_x)		((_x) * 4 + 0x94)
> +#define  CRM_CLK_DIV_MASK	GENMASK(7, 0)
> +
> +static unsigned long sf21_muxdiv_recalc_rate(struct clk_hw *hw,
> +					     unsigned long parent_rate)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	ulong div_reg = CRM_CLK_DIV(priv->div_reg);
> +	u16 div_offs = priv->div_offs;
> +	u16 div_val = (sf_readl(cmn_priv, div_reg) >> div_offs) &
> +		      CRM_CLK_DIV_MASK;
> +	div_val += 1;
> +	return parent_rate / div_val;
> +}
> +
> +static int sf21_muxdiv_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	unsigned int div;
> +
> +	if (!req->rate)
> +		return -EINVAL;
> +
> +	div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
> +	if (!div)
> +		div = 1;
> +	else if (div > CRM_CLK_DIV_MASK + 1)
> +		div = CRM_CLK_DIV_MASK + 1;
> +
> +	req->rate = req->best_parent_rate / div;
> +	return 0;
> +}
> +
> +static int sf21_muxdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	ulong div_reg = CRM_CLK_DIV(priv->div_reg);
> +	u16 div_offs = priv->div_offs;
> +	unsigned long flags;
> +	unsigned int div;
> +
> +	if (!rate)
> +		return -EINVAL;
> +
> +	div = DIV_ROUND_CLOSEST(parent_rate, rate);
> +	if (div < 1)
> +		div = 1;
> +	else if (div > CRM_CLK_DIV_MASK + 1)
> +		div = CRM_CLK_DIV_MASK + 1;
> +	div -= 1;
> +
> +	spin_lock_irqsave(cmn_priv->lock, flags);
> +	sf_rmw(cmn_priv, div_reg, CRM_CLK_DIV_MASK << div_offs,
> +	       div << div_offs);
> +	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_DIV);
> +	sf_writel(cmn_priv, CFG_LOAD, 0);
> +	spin_unlock_irqrestore(cmn_priv->lock, flags);
> +	return 0;
> +}
> +
> +static int sf21_muxdiv_enable(struct clk_hw *hw)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(cmn_priv->lock, flags);
> +	sf_rmw(cmn_priv, CRM_CLK_EN, 0, BIT(priv->en));
> +	/*
> +	 * Clock divider value load only happens when the clock is running.
> +	 * Pulse the CFG_LOAD_DIV so that set_rate() which happened
> +	 * before enable() is applied.
> +	 */
> +	sf_writel(cmn_priv, CFG_LOAD, CFG_LOAD_DIV);
> +	sf_writel(cmn_priv, CFG_LOAD, 0);
> +	spin_unlock_irqrestore(cmn_priv->lock, flags);
> +	return 0;
> +}
> +
> +static void sf21_muxdiv_disable(struct clk_hw *hw)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(cmn_priv->lock, flags);
> +	sf_rmw(cmn_priv, CRM_CLK_EN, BIT(priv->en), 0);
> +	spin_unlock_irqrestore(cmn_priv->lock, flags);
> +}
> +
> +static int sf21_muxdiv_is_enabled(struct clk_hw *hw)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	u32 reg_val = sf_readl(cmn_priv, CRM_CLK_EN);
> +
> +	return reg_val & (BIT(priv->en)) ? 1 : 0;
> +}
> +
> +static u8 sf21_muxdiv_get_parent(struct clk_hw *hw)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	ulong mux_reg = CRM_CLK_SEL(priv->mux_reg);
> +	u16 mux_offs = priv->mux_offs;
> +	u32 reg_val = sf_readl(cmn_priv, mux_reg);
> +
> +	return reg_val & BIT(mux_offs) ? 1 : 0;
> +}
> +
> +static int sf21_muxdiv_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf21_clk_muxdiv *priv =
> +		container_of(cmn_priv, struct sf21_clk_muxdiv, common);
> +	ulong mux_reg = CRM_CLK_SEL(priv->mux_reg);
> +	u16 mux_offs = priv->mux_offs;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(cmn_priv->lock, flags);
> +	if (index)
> +		sf_rmw(cmn_priv, mux_reg, 0, BIT(mux_offs));
> +	else
> +		sf_rmw(cmn_priv, mux_reg, BIT(mux_offs), 0);
> +
> +	spin_unlock_irqrestore(cmn_priv->lock, flags);
> +	return 0;
> +}

I believe besides the divider reloading part, clk_mux_ops,
clk_divider_ops, and clk_gate_ops have already provided the logic
you implemented here. So it might be a better option to composite them
together to implement your clocks instead of building from scratch.

> +static const struct clk_ops sf21_clk_muxdiv_ops = {
> +	.enable = sf21_muxdiv_enable,
> +	.disable = sf21_muxdiv_disable,
> +	.is_enabled = sf21_muxdiv_is_enabled,
> +	.recalc_rate = sf21_muxdiv_recalc_rate,
> +	.determine_rate = sf21_muxdiv_determine_rate,
> +	.set_rate = sf21_muxdiv_set_rate,
> +	.get_parent = sf21_muxdiv_get_parent,
> +	.set_parent = sf21_muxdiv_set_parent,
> +};

...

> +static SF21_MUXDIV(muxdiv_cpu, clk_periph_parents, 0, 1, 0, 0, 0,
> +		   CLK_IGNORE_UNUSED);

If it supplies the CPU core, shouldn't it be CLK_IS_CRITICAL instead?

> +static SF21_MUXDIV(muxdiv_pic, clk_periph_parents, 0, 3, 3, 16, 1,
> +		   CLK_IGNORE_UNUSED);

Do you have any information about purpose of the clock and why it's
marked as CLK_IGNORE_UNUSED? It's better to have a comment explaining
this since it's not very obvious...

Best regards,
Yao Zi

Re: [PATCH 4/4] clk: add support for siflower sf21-topcrm

[Chuanhong Guo]

Hi!

On Mon, May 18, 2026 at 8:22 PM Yao Zi <me@ziyao.cc> wrote:
>
> On Sun, May 17, 2026 at 10:12:58PM +0800, Chuanhong Guo wrote:
> > This commit adds a driver for the Toplevel clock and reset controller
> > found on Siflower SF21A6826/SF21H8898 SoCs.
> > This block contains control for 3 PLLs, several clock mux/gate/divider
> > blocks, and a reset register for on-chip peripherals.
>
> It would be better if you could split out the reset code into
> drivers/reset, and initialize the reset controller as an auxiliary
> device, like what has been done for SpacemiT platform
> (drivers/{clock,reset}/spacemit) and AMLogic platform
> (drivers/clock/meson/axg-audio.c and
> drivers/reset/amlogic/reset-meson-aux.c).

It seems pretty common for these kinds of combined clock and reset
blocks to have a single driver. (There are 32 "select RESET_CONTROLLER"
in drivers/clk.) I deliberately chose to merge the clock and reset
together since it's named "clock and reset module" in the datasheet
and is a single block for both clock and reset on this chip.
I think syscon is usually used for those registers with a bunch
of miscellaneous control fields instead.

>
> I am neither clock nor reset maintainer, thus this only serves as
> a suggestion, with which it ends up in better code structure.
>
> > There are also two registers for enabling PCIE clock output in this
> > block. They aren't covered by this patch because I can't test those
> > without a PCIE driver. These will be added with the PCIE driver
>
> s/PCIE/PCIe/g which is the formal spelling.

Sure. I'll fix this in v2.

>
> > patchset later after I get that working.
> >
> > Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> > ---
> >  drivers/clk/Kconfig                    |    1 +
> >  drivers/clk/Makefile                   |    1 +
> >  drivers/clk/siflower/Kconfig           |   22 +
> >  drivers/clk/siflower/Makefile          |    1 +
> >  drivers/clk/siflower/clk-sf21-topcrm.c | 1053 ++++++++++++++++++++++++++++++++
> >  5 files changed, 1078 insertions(+)
>
> ...
>
> > diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> > new file mode 100644
> > index 000000000000..7d4c5e370d6d
> > --- /dev/null
> > +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
> > @@ -0,0 +1,1053 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/rational.h>
> > +#include <linux/module.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/platform_device.h>
> > +#include <dt-bindings/clock/siflower,sf21-topcrm.h>
>
> Consider sorting the headers?

OK.

>
> ...
>
> > +static int sf21_cmnpll_vco_determine_rate(struct clk_hw *hw,
> > +                                       struct clk_rate_request *req)
> > +{
> > +     unsigned long fbdiv, refdiv;
> > +
> > +     rational_best_approximation(req->rate, req->best_parent_rate,
> > +                                 BIT(PLL_CMN_FBDIV_BITS) - 1,
> > +                                 BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
>
> FIELD_MAX(PLL_CMN_{FBDIV,REFDIV}_BITS}) would be simpler, with which it
> should be possible to get avoid of PLL_CMN_*_BITS.

Oh, I didn't know this is a thing. I'll change it in v2.

>
> > +                                 &refdiv);
> > +     if (!refdiv || !fbdiv)
> > +             return -EINVAL;
> > +
> > +     req->rate = (req->best_parent_rate / refdiv) * fbdiv;
> > +
> > +     return 0;
> > +}
> > +
> > +static int sf21_cmnpll_vco_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                                 unsigned long parent_rate)
> > +{
> > +     struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> > +     unsigned long flags;
> > +     unsigned long fbdiv, refdiv;
> > +     u32 val;
> > +     int ret;
> > +
> > +     rational_best_approximation(rate, parent_rate,
> > +                                 BIT(PLL_CMN_FBDIV_BITS) - 1,
> > +                                 BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
> > +                                 &refdiv);
> > +     if (!refdiv || !fbdiv)
> > +             return -EINVAL;
> > +
> > +     spin_lock_irqsave(priv->lock, flags);
>
>         guard(spinlock_irqsave)(priv->lock)
>
> might simplify the code (especially the error handling path in this
> function), this applies as well for other places where
> spin_lock_irqsave() involves.

Oh, that macro is cool. I'll use it in v2.

>
> [...]
>
> Besides field/register offsets, the only difference I could tell between
> cmnpll_postdiv and pciepll_fout is that pciepll_fout clocks could be
> gated.
>
> Would it be a good idea to describe the gating function separately as a
> clock, and merge the common part of pciepll_fout and cmnpll_postdiv? In
> which way you could save a lot of mostly duplicated code.

I don't think so. Since the majority of the existing code are register
operations, and the register bit layout are completely different,
there isn't much to share between these two PLLs.
Writing a struct to describe both register layouts together seems
unnecessarily complicated and harder to read.

BTW the CMNPLL and PCIEPLL are actually different hardware.
The former is an integer PLL while the latter actually supports
fractional operations. I didn't add support for the fractional part
due to the lack of use cases and documentation. PCIE and GMAC
clocks are fed by this PLL and require exact clock frequencies
which can be achieved using only the integer mode.

>
> > +     .recalc_rate = sf21_pciepll_fout_recalc_rate,
> > +     .determine_rate = sf21_pciepll_fout_determine_rate,
> > +     .set_rate = sf21_pciepll_fout_set_rate,
> > +};
> [...]
> > +
> > +     spin_lock_irqsave(cmn_priv->lock, flags);
> > +     if (index)
> > +             sf_rmw(cmn_priv, mux_reg, 0, BIT(mux_offs));
> > +     else
> > +             sf_rmw(cmn_priv, mux_reg, BIT(mux_offs), 0);
> > +
> > +     spin_unlock_irqrestore(cmn_priv->lock, flags);
> > +     return 0;
> > +}
>
> I believe besides the divider reloading part, clk_mux_ops,
> clk_divider_ops, and clk_gate_ops have already provided the logic
> you implemented here. So it might be a better option to composite them
> together to implement your clocks instead of building from scratch.

The divider reloading is the exact reason I chose to not compose them
with the function you mentioned. The reloading bit need to be set on
both clock divider change and clock enabling, because the clock divider
loading only happens when the clock is running. Since I already have
to write two of the three parts you mentioned, trying to reuse
clk_mux_ops doesn't seem to reduce the code complexity here.
It's just trading get_parent and set_parent call with one more level
in the clock tree for every clock and more code to wire them together
in the probe function.

>
> > +static const struct clk_ops sf21_clk_muxdiv_ops = {
> > +     .enable = sf21_muxdiv_enable,
> > +     .disable = sf21_muxdiv_disable,
> > +     .is_enabled = sf21_muxdiv_is_enabled,
> > +     .recalc_rate = sf21_muxdiv_recalc_rate,
> > +     .determine_rate = sf21_muxdiv_determine_rate,
> > +     .set_rate = sf21_muxdiv_set_rate,
> > +     .get_parent = sf21_muxdiv_get_parent,
> > +     .set_parent = sf21_muxdiv_set_parent,
> > +};
>
> ...
>
> > +static SF21_MUXDIV(muxdiv_cpu, clk_periph_parents, 0, 1, 0, 0, 0,
> > +                CLK_IGNORE_UNUSED);
>
> If it supplies the CPU core, shouldn't it be CLK_IS_CRITICAL instead?
>
> > +static SF21_MUXDIV(muxdiv_pic, clk_periph_parents, 0, 3, 3, 16, 1,
> > +                CLK_IGNORE_UNUSED);
>
> Do you have any information about purpose of the clock and why it's
> marked as CLK_IGNORE_UNUSED? It's better to have a comment explaining
> this since it's not very obvious...

This one is Platform Interrupt Controller and it feeds PLIC + CLINT in the
C908 core complex.

I copied these from the vendor driver.
I'll make them CLK_IS_CRITICAL in v2.

-- 
Regards,
Chuanhong Guo