linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC
@ 2023-06-13 12:58 Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator Xingyu Wu
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

This patch serises are to add PLL clocks driver and providers by writing
and reading syscon registers for the StarFive JH7110 RISC-V SoC. And add 
documentation and nodes to describe StarFive System Controller(syscon)
Registers. This patch serises are based on Linux 6.4-rc1.

PLL are high speed, low jitter frequency synthesizers in JH7110.
Each PLL clocks work in integer mode or fraction mode by some dividers,
and the dividers are set in several syscon registers.
The formula for calculating frequency is: 
Fvco = Fref * (NI + NF) / M / Q1

The first patch adds docunmentation to describe PLL clock bindings,
and the second patch adds documentation to decribe syscon registers.
The patch 3 modifies the SYSCRG dibindings and adds PLL clock inputs.
The patch 4 adds driver to support PLL clocks for JH7110.
The patch 5 modifies the system clock driver and can select the PLL clock
source from PLL clocks driver. And the patch 6 adds the 
stg/sys/aon syscon nodes for JH7110 SoC. The last patch modifies the
syscrg node in JH7110 dts file.

Changes since v4:
- Rebased on Linux 6.4-rc6.
- Patch 2 dropped the example node about sys-syscon.
- Patch 3 used PLL clocks as one of optional items in SYSCRG bindings.
- Patch 4 used the patch instead about PLL clocks driver from Emil.
- Patch 5 retained the fixed factor PLL clocks as the optional source
  about PLL clocks in SYSCRG clock driver.
- Patch 6 added the child node clock-controller as the complete
  sys-syscon node and patch 7 dropped this part.

v4: https://lore.kernel.org/all/20230512022036.97987-1-xingyu.wu@starfivetech.com/

Changes since v3: 
- Rebased on Linux 6.4-rc1.
- Dropped the 'power-controller' property and used 'power-domain-cells'
  instead in syscon binding.
- Used the data by of_device_id to get the syscon registers'
  configuration include offset, mask and shift.

v3: https://lore.kernel.org/all/20230414024157.53203-1-xingyu.wu@starfivetech.com/

Changes since v2: 
- Rebased on latest JH7110 basic clock drivers.
- Added the complete documentation to describe syscon register.
- Added syscon node in JH7110 dts file.
- Modified the clock rate selection to match the closest rate in
  PLL driver when setting rate.

v2: https://lore.kernel.org/all/20230316030514.137427-1-xingyu.wu@starfivetech.com/

Changes since v1: 
- Changed PLL clock node to be child of syscon node in dts.
- Modifed the definitions and names of function in PLL clock driver.
- Added commit to update syscon and syscrg dt-bindings.

v1: https://lore.kernel.org/all/20230221141147.303642-1-xingyu.wu@starfivetech.com/

William Qiu (2):
  dt-bindings: soc: starfive: Add StarFive syscon module
  riscv: dts: starfive: jh7110: Add syscon nodes

Xingyu Wu (5):
  dt-bindings: clock: Add StarFive JH7110 PLL clock generator
  dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
  clk: starfive: Add StarFive JH7110 PLL clock driver
  clk: starfive: jh7110-sys: Add PLL clocks source from DTS
  riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node

 .../bindings/clock/starfive,jh7110-pll.yaml   |  46 ++
 .../clock/starfive,jh7110-syscrg.yaml         |  56 ++
 .../soc/starfive/starfive,jh7110-syscon.yaml  |  62 +++
 MAINTAINERS                                   |  13 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  30 +-
 drivers/clk/starfive/Kconfig                  |   9 +
 drivers/clk/starfive/Makefile                 |   1 +
 .../clk/starfive/clk-starfive-jh7110-pll.c    | 507 ++++++++++++++++++
 .../clk/starfive/clk-starfive-jh7110-sys.c    |  45 +-
 .../dt-bindings/clock/starfive,jh7110-crg.h   |   6 +
 10 files changed, 755 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
 create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c

-- 
2.25.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 19:06   ` Conor Dooley
  2023-06-13 12:58 ` [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module Xingyu Wu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

Add bindings for the PLL clock generator on the JH7110 RISC-V SoC.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../bindings/clock/starfive,jh7110-pll.yaml   | 46 +++++++++++++++++++
 .../dt-bindings/clock/starfive,jh7110-crg.h   |  6 +++
 2 files changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml

diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
new file mode 100644
index 000000000000..8aa8c7b8e42f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/starfive,jh7110-pll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 PLL Clock Generator
+
+description:
+  This PLL are high speed, low jitter frequency synthesizers in JH7110.
+  Each PLL clocks work in integer mode or fraction mode by some dividers,
+  and the configuration registers and dividers are set in several syscon
+  registers. So pll node should be a child of SYS-SYSCON node.
+  The formula for calculating frequency is that,
+  Fvco = Fref * (NI + NF) / M / Q1
+
+maintainers:
+  - Xingyu Wu <xingyu.wu@starfivetech.com>
+
+properties:
+  compatible:
+    const: starfive,jh7110-pll
+
+  clocks:
+    maxItems: 1
+    description: Main Oscillator (24 MHz)
+
+  '#clock-cells':
+    const: 1
+    description:
+      See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
+
+required:
+  - compatible
+  - clocks
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    pll-clock-controller {
+      compatible = "starfive,jh7110-pll";
+      clocks = <&osc>;
+      #clock-cells = <1>;
+    };
diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
index 06257bfd9ac1..086a6ddcf380 100644
--- a/include/dt-bindings/clock/starfive,jh7110-crg.h
+++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
@@ -6,6 +6,12 @@
 #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
 #define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
 
+/* PLL clocks */
+#define JH7110_CLK_PLL0_OUT			0
+#define JH7110_CLK_PLL1_OUT			1
+#define JH7110_CLK_PLL2_OUT			2
+#define JH7110_PLLCLK_END			3
+
 /* SYSCRG clocks */
 #define JH7110_SYSCLK_CPU_ROOT			0
 #define JH7110_SYSCLK_CPU_CORE			1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 18:31   ` Krzysztof Kozlowski
  2023-06-13 18:32   ` Krzysztof Kozlowski
  2023-06-13 12:58 ` [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs Xingyu Wu
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

From: William Qiu <william.qiu@starfivetech.com>

Add documentation to describe StarFive System Controller Registers.

Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml

diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
new file mode 100644
index 000000000000..a81190f8a54d
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC system controller
+
+maintainers:
+  - William Qiu <william.qiu@starfivetech.com>
+
+description: |
+  The StarFive JH7110 SoC system controller provides register information such
+  as offset, mask and shift to configure related modules such as MMC and PCIe.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: starfive,jh7110-sys-syscon
+          - const: syscon
+          - const: simple-mfd
+      - items:
+          - enum:
+              - starfive,jh7110-aon-syscon
+              - starfive,jh7110-stg-syscon
+          - const: syscon
+
+  reg:
+    maxItems: 1
+
+  clock-controller:
+    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
+    type: object
+
+  "#power-domain-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-aon-syscon
+    then:
+      required:
+        - "#power-domain-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    syscon@10240000 {
+        compatible = "starfive,jh7110-stg-syscon", "syscon";
+        reg = <0x10240000 0x1000>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f794002a192e..ae037ba7fc47 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20122,6 +20122,12 @@ S:	Supported
 F:	Documentation/devicetree/bindings/mmc/starfive*
 F:	drivers/mmc/host/dw_mmc-starfive.c
 
+STARFIVE JH7110 SYSCON
+M:	William Qiu <william.qiu@starfivetech.com>
+M:	Xingyu Wu <xingyu.wu@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+
 STARFIVE JH71X0 CLOCK DRIVERS
 M:	Emil Renner Berthing <kernel@esmil.dk>
 M:	Hal Feng <hal.feng@starfivetech.com>
@@ -20159,6 +20165,7 @@ STARFIVE SOC DRIVERS
 M:	Conor Dooley <conor@kernel.org>
 S:	Maintained
 T:	git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
+F:	Documentation/devicetree/bindings/soc/starfive/
 F:	drivers/soc/starfive/
 
 STARFIVE TRNG DRIVER
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 18:34   ` Krzysztof Kozlowski
  2023-06-13 12:58 ` [PATCH v5 4/7] clk: starfive: Add StarFive JH7110 PLL clock driver Xingyu Wu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

Add optional PLL clock inputs from PLL clock generator.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../clock/starfive,jh7110-syscrg.yaml         | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
index 84373ae31644..5536e5f9e20b 100644
--- a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
+++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
@@ -39,6 +39,33 @@ properties:
           - description: External TDM clock
           - description: External audio master clock
 
+      - items:
+          - description: Main Oscillator (24 MHz)
+          - description: GMAC1 RMII reference or GMAC1 RGMII RX
+          - description: External I2S TX bit clock
+          - description: External I2S TX left/right channel clock
+          - description: External I2S RX bit clock
+          - description: External I2S RX left/right channel clock
+          - description: External TDM clock
+          - description: External audio master clock
+          - description: PLL0
+          - description: PLL1
+          - description: PLL2
+
+      - items:
+          - description: Main Oscillator (24 MHz)
+          - description: GMAC1 RMII reference
+          - description: GMAC1 RGMII RX
+          - description: External I2S TX bit clock
+          - description: External I2S TX left/right channel clock
+          - description: External I2S RX bit clock
+          - description: External I2S RX left/right channel clock
+          - description: External TDM clock
+          - description: External audio master clock
+          - description: PLL0
+          - description: PLL1
+          - description: PLL2
+
   clock-names:
     oneOf:
       - items:
@@ -64,6 +91,35 @@ properties:
           - const: tdm_ext
           - const: mclk_ext
 
+      - items:
+          - const: osc
+          - enum:
+              - gmac1_rmii_refin
+              - gmac1_rgmii_rxin
+          - const: i2stx_bclk_ext
+          - const: i2stx_lrck_ext
+          - const: i2srx_bclk_ext
+          - const: i2srx_lrck_ext
+          - const: tdm_ext
+          - const: mclk_ext
+          - const: pll0_out
+          - const: pll1_out
+          - const: pll2_out
+
+      - items:
+          - const: osc
+          - const: gmac1_rmii_refin
+          - const: gmac1_rgmii_rxin
+          - const: i2stx_bclk_ext
+          - const: i2stx_lrck_ext
+          - const: i2srx_bclk_ext
+          - const: i2srx_lrck_ext
+          - const: tdm_ext
+          - const: mclk_ext
+          - const: pll0_out
+          - const: pll1_out
+          - const: pll2_out
+
   '#clock-cells':
     const: 1
     description:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 4/7] clk: starfive: Add StarFive JH7110 PLL clock driver
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
                   ` (2 preceding siblings ...)
  2023-06-13 12:58 ` [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS Xingyu Wu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

Add driver for the StarFive JH7110 PLL clock controller
and they work by reading and setting syscon registers.

Co-developed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 MAINTAINERS                                   |   6 +
 drivers/clk/starfive/Kconfig                  |   8 +
 drivers/clk/starfive/Makefile                 |   1 +
 .../clk/starfive/clk-starfive-jh7110-pll.c    | 507 ++++++++++++++++++
 4 files changed, 522 insertions(+)
 create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ae037ba7fc47..e2ef606f5315 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20122,6 +20122,12 @@ S:	Supported
 F:	Documentation/devicetree/bindings/mmc/starfive*
 F:	drivers/mmc/host/dw_mmc-starfive.c
 
+STARFIVE JH7110 PLL CLOCK DRIVER
+M:	Xingyu Wu <xingyu.wu@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
+F:	drivers/clk/starfive/clk-starfive-jh7110-pll.*
+
 STARFIVE JH7110 SYSCON
 M:	William Qiu <william.qiu@starfivetech.com>
 M:	Xingyu Wu <xingyu.wu@starfivetech.com>
diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
index 5d2333106f13..5195f7be5213 100644
--- a/drivers/clk/starfive/Kconfig
+++ b/drivers/clk/starfive/Kconfig
@@ -21,6 +21,14 @@ config CLK_STARFIVE_JH7100_AUDIO
 	  Say Y or M here to support the audio clocks on the StarFive JH7100
 	  SoC.
 
+config CLK_STARFIVE_JH7110_PLL
+	bool "StarFive JH7110 PLL clock support"
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	default ARCH_STARFIVE
+	help
+	  Say yes here to support the PLL clock controller on the
+	  StarFive JH7110 SoC.
+
 config CLK_STARFIVE_JH7110_SYS
 	bool "StarFive JH7110 system clock support"
 	depends on ARCH_STARFIVE || COMPILE_TEST
diff --git a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile
index f3df7d957b1e..b48e539e52b0 100644
--- a/drivers/clk/starfive/Makefile
+++ b/drivers/clk/starfive/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_CLK_STARFIVE_JH71X0)	+= clk-starfive-jh71x0.o
 obj-$(CONFIG_CLK_STARFIVE_JH7100)	+= clk-starfive-jh7100.o
 obj-$(CONFIG_CLK_STARFIVE_JH7100_AUDIO)	+= clk-starfive-jh7100-audio.o
 
+obj-$(CONFIG_CLK_STARFIVE_JH7110_PLL)	+= clk-starfive-jh7110-pll.o
 obj-$(CONFIG_CLK_STARFIVE_JH7110_SYS)	+= clk-starfive-jh7110-sys.o
 obj-$(CONFIG_CLK_STARFIVE_JH7110_AON)	+= clk-starfive-jh7110-aon.o
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-pll.c b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
new file mode 100644
index 000000000000..b10c142d456d
--- /dev/null
+++ b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
@@ -0,0 +1,507 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StarFive JH7110 PLL Clock Generator Driver
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ * Copyright (C) 2023 Emil Renner Berthing <emil.renner.berthing@canonical.com>
+ *
+ * This driver is about to register JH7110 PLL clock generator and support ops.
+ * The JH7110 have three PLL clock, PLL0, PLL1 and PLL2.
+ * Each PLL clocks work in integer mode or fraction mode by some dividers,
+ * and the configuration registers and dividers are set in several syscon registers.
+ * The formula for calculating frequency is:
+ * Fvco = Fref * (NI + NF) / M / Q1
+ * Fref: OSC source clock rate
+ * NI: integer frequency dividing ratio of feedback divider, set by fbdiv[11:0].
+ * NF: fractional frequency dividing ratio, set by frac[23:0]. NF = frac[23:0] / 2^24 = 0 ~ 0.999.
+ * M: frequency dividing ratio of pre-divider, set by prediv[5:0].
+ * Q1: frequency dividing ratio of post divider, set by 2^postdiv1[1:0], eg. 1, 2, 4 or 8.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk-provider.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/starfive,jh7110-crg.h>
+
+/* this driver expects a 24MHz input frequency from the oscillator */
+#define JH7110_PLL_OSC_RATE		24000000UL
+
+#define JH7110_PLL0_PD_OFFSET		0x18
+#define JH7110_PLL0_DACPD_SHIFT		24
+#define JH7110_PLL0_DACPD_MASK		BIT(24)
+#define JH7110_PLL0_DSMPD_SHIFT		25
+#define JH7110_PLL0_DSMPD_MASK		BIT(25)
+#define JH7110_PLL0_FBDIV_OFFSET	0x1c
+#define JH7110_PLL0_FBDIV_SHIFT		0
+#define JH7110_PLL0_FBDIV_MASK		GENMASK(11, 0)
+#define JH7110_PLL0_FRAC_OFFSET		0x20
+#define JH7110_PLL0_PREDIV_OFFSET	0x24
+
+#define JH7110_PLL1_PD_OFFSET		0x24
+#define JH7110_PLL1_DACPD_SHIFT		15
+#define JH7110_PLL1_DACPD_MASK		BIT(15)
+#define JH7110_PLL1_DSMPD_SHIFT		16
+#define JH7110_PLL1_DSMPD_MASK		BIT(16)
+#define JH7110_PLL1_FBDIV_OFFSET	0x24
+#define JH7110_PLL1_FBDIV_SHIFT		17
+#define JH7110_PLL1_FBDIV_MASK		GENMASK(28, 17)
+#define JH7110_PLL1_FRAC_OFFSET		0x28
+#define JH7110_PLL1_PREDIV_OFFSET	0x2c
+
+#define JH7110_PLL2_PD_OFFSET		0x2c
+#define JH7110_PLL2_DACPD_SHIFT		15
+#define JH7110_PLL2_DACPD_MASK		BIT(15)
+#define JH7110_PLL2_DSMPD_SHIFT		16
+#define JH7110_PLL2_DSMPD_MASK		BIT(16)
+#define JH7110_PLL2_FBDIV_OFFSET	0x2c
+#define JH7110_PLL2_FBDIV_SHIFT		17
+#define JH7110_PLL2_FBDIV_MASK		GENMASK(28, 17)
+#define JH7110_PLL2_FRAC_OFFSET		0x30
+#define JH7110_PLL2_PREDIV_OFFSET	0x34
+
+#define JH7110_PLL_FRAC_SHIFT		0
+#define JH7110_PLL_FRAC_MASK		GENMASK(23, 0)
+#define JH7110_PLL_POSTDIV1_SHIFT	28
+#define JH7110_PLL_POSTDIV1_MASK	GENMASK(29, 28)
+#define JH7110_PLL_PREDIV_SHIFT		0
+#define JH7110_PLL_PREDIV_MASK		GENMASK(5, 0)
+
+enum jh7110_pll_mode {
+	JH7110_PLL_MODE_FRACTION,
+	JH7110_PLL_MODE_INTEGER,
+};
+
+struct jh7110_pll_preset {
+	unsigned long freq;
+	u32 frac;		/* frac value should be decimals multiplied by 2^24 */
+	unsigned fbdiv    : 12;	/* fbdiv value should be 8 to 4095 */
+	unsigned prediv   :  6;
+	unsigned postdiv1 :  2;
+	unsigned mode     :  1;
+};
+
+struct jh7110_pll_info {
+	char *name;
+	const struct jh7110_pll_preset *presets;
+	unsigned int npresets;
+	struct {
+		unsigned int pd;
+		unsigned int fbdiv;
+		unsigned int frac;
+		unsigned int prediv;
+	} offsets;
+	struct {
+		u32 dacpd;
+		u32 dsmpd;
+		u32 fbdiv;
+	} masks;
+	struct {
+		char dacpd;
+		char dsmpd;
+		char fbdiv;
+	} shifts;
+};
+
+#define _JH7110_PLL(_idx, _name, _presets)				\
+	[_idx] = {							\
+		.name = _name,						\
+		.presets = _presets,					\
+		.npresets = ARRAY_SIZE(_presets),			\
+		.offsets = {						\
+			.pd = JH7110_PLL##_idx##_PD_OFFSET,		\
+			.fbdiv = JH7110_PLL##_idx##_FBDIV_OFFSET,	\
+			.frac = JH7110_PLL##_idx##_FRAC_OFFSET,		\
+			.prediv = JH7110_PLL##_idx##_PREDIV_OFFSET,	\
+		},							\
+		.masks = {						\
+			.dacpd = JH7110_PLL##_idx##_DACPD_MASK,		\
+			.dsmpd = JH7110_PLL##_idx##_DSMPD_MASK,		\
+			.fbdiv = JH7110_PLL##_idx##_FBDIV_MASK,		\
+		},							\
+		.shifts = {						\
+			.dacpd = JH7110_PLL##_idx##_DACPD_SHIFT,	\
+			.dsmpd = JH7110_PLL##_idx##_DSMPD_SHIFT,	\
+			.fbdiv = JH7110_PLL##_idx##_FBDIV_SHIFT,	\
+		},							\
+	}
+#define JH7110_PLL(idx, name, presets) _JH7110_PLL(idx, name, presets)
+
+struct jh7110_pll_data {
+	struct clk_hw hw;
+	unsigned int idx;
+};
+
+struct jh7110_pll_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct jh7110_pll_data pll[JH7110_PLLCLK_END];
+};
+
+struct jh7110_pll_regvals {
+	u32 dacpd;
+	u32 dsmpd;
+	u32 fbdiv;
+	u32 frac;
+	u32 postdiv1;
+	u32 prediv;
+};
+
+/*
+ * Because the pll frequency is relatively fixed,
+ * it cannot be set arbitrarily, so it needs a specific configuration.
+ * PLL0 frequency should be multiple of 125MHz (USB frequency).
+ */
+static const struct jh7110_pll_preset jh7110_pll0_presets[] = {
+	{
+		.freq = 375000000,
+		.fbdiv = 125,
+		.prediv = 8,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 500000000,
+		.fbdiv = 125,
+		.prediv = 6,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 625000000,
+		.fbdiv = 625,
+		.prediv = 24,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 750000000,
+		.fbdiv = 125,
+		.prediv = 4,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 875000000,
+		.fbdiv = 875,
+		.prediv = 24,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1000000000,
+		.fbdiv = 125,
+		.prediv = 3,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1250000000,
+		.fbdiv = 625,
+		.prediv = 12,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1375000000,
+		.fbdiv = 1375,
+		.prediv = 24,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1500000000,
+		.fbdiv = 125,
+		.prediv = 2,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	},
+};
+
+static const struct jh7110_pll_preset jh7110_pll1_presets[] = {
+	{
+		.freq = 1066000000,
+		.fbdiv = 533,
+		.prediv = 12,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1200000000,
+		.fbdiv = 50,
+		.prediv = 1,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1400000000,
+		.fbdiv = 350,
+		.prediv = 6,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1600000000,
+		.fbdiv = 200,
+		.prediv = 3,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	},
+};
+
+static const struct jh7110_pll_preset jh7110_pll2_presets[] = {
+	{
+		.freq = 1188000000,
+		.fbdiv = 99,
+		.prediv = 2,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	}, {
+		.freq = 1228800000,
+		.fbdiv = 256,
+		.prediv = 5,
+		.postdiv1 = 0,
+		.mode = JH7110_PLL_MODE_INTEGER,
+	},
+};
+
+static const struct jh7110_pll_info jh7110_plls[JH7110_PLLCLK_END] = {
+	JH7110_PLL(JH7110_CLK_PLL0_OUT, "pll0_out", jh7110_pll0_presets),
+	JH7110_PLL(JH7110_CLK_PLL1_OUT, "pll1_out", jh7110_pll1_presets),
+	JH7110_PLL(JH7110_CLK_PLL2_OUT, "pll2_out", jh7110_pll2_presets),
+};
+
+static struct jh7110_pll_data *jh7110_pll_data_from(struct clk_hw *hw)
+{
+	return container_of(hw, struct jh7110_pll_data, hw);
+}
+
+static struct jh7110_pll_priv *jh7110_pll_priv_from(struct jh7110_pll_data *pll)
+{
+	return container_of(pll, struct jh7110_pll_priv, pll[pll->idx]);
+}
+
+static void jh7110_pll_regvals_get(struct regmap *regmap,
+				   const struct jh7110_pll_info *info,
+				   struct jh7110_pll_regvals *ret)
+{
+	u32 val;
+
+	regmap_read(regmap, info->offsets.pd, &val);
+	ret->dacpd = (val & info->masks.dacpd) >> info->shifts.dacpd;
+	ret->dsmpd = (val & info->masks.dsmpd) >> info->shifts.dsmpd;
+
+	regmap_read(regmap, info->offsets.fbdiv, &val);
+	ret->fbdiv = (val & info->masks.fbdiv) >> info->shifts.fbdiv;
+
+	regmap_read(regmap, info->offsets.frac, &val);
+	ret->frac = (val & JH7110_PLL_FRAC_MASK) >> JH7110_PLL_FRAC_SHIFT;
+	ret->postdiv1 = (val & JH7110_PLL_POSTDIV1_MASK) >> JH7110_PLL_POSTDIV1_SHIFT;
+
+	regmap_read(regmap, info->offsets.prediv, &val);
+	ret->prediv = (val & JH7110_PLL_PREDIV_MASK) >> JH7110_PLL_PREDIV_SHIFT;
+}
+
+static unsigned long jh7110_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+	struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
+	struct jh7110_pll_regvals val;
+	unsigned long rate;
+
+	jh7110_pll_regvals_get(priv->regmap, &jh7110_plls[pll->idx], &val);
+
+	/*
+	 * dacpd = dsmpd = 0: fraction mode
+	 * dacpd = dsmpd = 1: integer mode, frac value ignored
+	 *
+	 * rate = parent * (fbdiv + frac/2^24) / prediv / 2^postdiv1
+	 *      = (parent * fbdiv + parent * frac / 2^24) / (prediv * 2^postdiv1)
+	 */
+	if (val.dacpd == 0 && val.dsmpd == 0)
+		rate = parent_rate * val.frac / (1UL << 24);
+	else if (val.dacpd == 1 && val.dsmpd == 1)
+		rate = 0;
+	else
+		return 0;
+
+	rate += parent_rate * val.fbdiv;
+	rate /= val.prediv << val.postdiv1;
+
+	return rate;
+}
+
+static int jh7110_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+	struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+	const struct jh7110_pll_info *info = &jh7110_plls[pll->idx];
+	const struct jh7110_pll_preset *selected = &info->presets[0];
+	unsigned int idx;
+
+	/* if the parent rate doesn't match our expectations the presets won't work */
+	if (req->best_parent_rate != JH7110_PLL_OSC_RATE) {
+		req->rate = jh7110_pll_recalc_rate(hw, req->best_parent_rate);
+		return 0;
+	}
+
+	/* find highest rate lower or equal to the requested rate */
+	for (idx = 1; idx < info->npresets; idx++) {
+		const struct jh7110_pll_preset *val = &info->presets[idx];
+
+		if (req->rate < val->freq)
+			break;
+
+		selected = val;
+	}
+
+	req->rate = selected->freq;
+	return 0;
+}
+
+static int jh7110_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long parent_rate)
+{
+	struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+	struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
+	const struct jh7110_pll_info *info = &jh7110_plls[pll->idx];
+	const struct jh7110_pll_preset *val;
+	unsigned int idx;
+
+	/* if the parent rate doesn't match our expectations the presets won't work */
+	if (parent_rate != JH7110_PLL_OSC_RATE)
+		return -EINVAL;
+
+	for (idx = 0, val = &info->presets[0]; idx < info->npresets; idx++, val++) {
+		if (val->freq == rate)
+			goto found;
+	}
+	return -EINVAL;
+
+found:
+	if (val->mode == JH7110_PLL_MODE_FRACTION)
+		regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_FRAC_MASK,
+				   val->frac << JH7110_PLL_FRAC_SHIFT);
+
+	regmap_update_bits(priv->regmap, info->offsets.pd, info->masks.dacpd,
+			   (u32)val->mode << info->shifts.dacpd);
+	regmap_update_bits(priv->regmap, info->offsets.pd, info->masks.dsmpd,
+			   (u32)val->mode << info->shifts.dsmpd);
+	regmap_update_bits(priv->regmap, info->offsets.prediv, JH7110_PLL_PREDIV_MASK,
+			   (u32)val->prediv << JH7110_PLL_PREDIV_SHIFT);
+	regmap_update_bits(priv->regmap, info->offsets.fbdiv, info->masks.fbdiv,
+			   val->fbdiv << info->shifts.fbdiv);
+	regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_POSTDIV1_MASK,
+			   (u32)val->postdiv1 << JH7110_PLL_POSTDIV1_SHIFT);
+
+	return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static int jh7110_pll_registers_read(struct seq_file *s, void *unused)
+{
+	struct jh7110_pll_data *pll = s->private;
+	struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
+	struct jh7110_pll_regvals val;
+
+	jh7110_pll_regvals_get(priv->regmap, &jh7110_plls[pll->idx], &val);
+
+	seq_printf(s, "fbdiv=%u\n"
+		      "frac=%u\n"
+		      "prediv=%u\n"
+		      "postdiv1=%u\n"
+		      "dacpd=%u\n"
+		      "dsmpd=%u\n",
+		      val.fbdiv, val.frac, val.prediv, val.postdiv1,
+		      val.dacpd, val.dsmpd);
+
+	return 0;
+}
+
+static int jh7110_pll_registers_open(struct inode *inode, struct file *f)
+{
+	return single_open(f, jh7110_pll_registers_read, inode->i_private);
+}
+
+static const struct file_operations jh7110_pll_registers_ops = {
+	.owner = THIS_MODULE,
+	.open = jh7110_pll_registers_open,
+	.release = single_release,
+	.read = seq_read,
+	.llseek = seq_lseek
+};
+
+static void jh7110_pll_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+	struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+
+	debugfs_create_file("registers", 0400, dentry, pll,
+			    &jh7110_pll_registers_ops);
+}
+#else
+#define jh7110_pll_debug_init NULL
+#endif
+
+static const struct clk_ops jh7110_pll_ops = {
+	.recalc_rate = jh7110_pll_recalc_rate,
+	.determine_rate = jh7110_pll_determine_rate,
+	.set_rate = jh7110_pll_set_rate,
+	.debug_init = jh7110_pll_debug_init,
+};
+
+static struct clk_hw *jh7110_pll_get(struct of_phandle_args *clkspec, void *data)
+{
+	struct jh7110_pll_priv *priv = data;
+	unsigned int idx = clkspec->args[0];
+
+	if (idx < JH7110_PLLCLK_END)
+		return &priv->pll[idx].hw;
+
+	return ERR_PTR(-EINVAL);
+}
+
+static int jh7110_pll_probe(struct platform_device *pdev)
+{
+	struct jh7110_pll_priv *priv;
+	unsigned int idx;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+	priv->regmap = syscon_node_to_regmap(priv->dev->of_node->parent);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	for (idx = 0; idx < JH7110_PLLCLK_END; idx++) {
+		struct clk_parent_data parents = {
+			.index = 0,
+		};
+		struct clk_init_data init = {
+			.name = jh7110_plls[idx].name,
+			.ops = &jh7110_pll_ops,
+			.parent_data = &parents,
+			.num_parents = 1,
+			.flags = 0,
+		};
+		struct jh7110_pll_data *pll = &priv->pll[idx];
+
+		pll->hw.init = &init;
+		pll->idx = idx;
+
+		ret = devm_clk_hw_register(&pdev->dev, &pll->hw);
+		if (ret)
+			return ret;
+	}
+
+	return devm_of_clk_add_hw_provider(&pdev->dev, jh7110_pll_get, priv);
+}
+
+static const struct of_device_id jh7110_pll_match[] = {
+	{ .compatible = "starfive,jh7110-pll" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, jh7110_pll_match);
+
+static struct platform_driver jh7110_pll_driver = {
+	.driver = {
+		.name = "clk-starfive-jh7110-pll",
+		.of_match_table = jh7110_pll_match,
+	},
+};
+builtin_platform_driver_probe(jh7110_pll_driver, jh7110_pll_probe);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
                   ` (3 preceding siblings ...)
  2023-06-13 12:58 ` [PATCH v5 4/7] clk: starfive: Add StarFive JH7110 PLL clock driver Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 6/7] riscv: dts: starfive: jh7110: Add syscon nodes Xingyu Wu
  2023-06-13 12:58 ` [PATCH v5 7/7] riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node Xingyu Wu
  6 siblings, 0 replies; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

Modify PLL clocks source to be got from DTS or
the fixed factor clocks.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 drivers/clk/starfive/Kconfig                  |  1 +
 .../clk/starfive/clk-starfive-jh7110-sys.c    | 45 +++++++++++--------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
index 5195f7be5213..978b78ec08b1 100644
--- a/drivers/clk/starfive/Kconfig
+++ b/drivers/clk/starfive/Kconfig
@@ -35,6 +35,7 @@ config CLK_STARFIVE_JH7110_SYS
 	select AUXILIARY_BUS
 	select CLK_STARFIVE_JH71X0
 	select RESET_STARFIVE_JH7110 if RESET_CONTROLLER
+	select CLK_STARFIVE_JH7110_PLL
 	default ARCH_STARFIVE
 	help
 	  Say yes here to support the system clock controller on the
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
index e6031345ef05..d56f48013388 100644
--- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
+++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/auxiliary_bus.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -386,6 +387,7 @@ EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);
 
 static int __init jh7110_syscrg_probe(struct platform_device *pdev)
 {
+	bool use_fixed_pll = true;	/* PLL clocks use fixed factor clocks or PLL driver */
 	struct jh71x0_clk_priv *priv;
 	unsigned int idx;
 	int ret;
@@ -402,28 +404,29 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-	/*
-	 * These PLL clocks are not actually fixed factor clocks and can be
-	 * controlled by the syscon registers of JH7110. They will be dropped
-	 * and registered in the PLL clock driver instead.
-	 */
+	if (!IS_ERR(devm_clk_get(priv->dev, "pll0_out")))
+		use_fixed_pll = false;	/* can get pll clocks from PLL driver */
+
+	/* Use fixed factor clocks if can not get the PLL clocks from DTS */
+	if (use_fixed_pll) {
 	/* 24MHz -> 1000.0MHz */
-	priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
-							 "osc", 0, 125, 3);
-	if (IS_ERR(priv->pll[0]))
-		return PTR_ERR(priv->pll[0]);
+		priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
+								 "osc", 0, 125, 3);
+		if (IS_ERR(priv->pll[0]))
+			return PTR_ERR(priv->pll[0]);
 
 	/* 24MHz -> 1066.0MHz */
-	priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
-							 "osc", 0, 533, 12);
-	if (IS_ERR(priv->pll[1]))
-		return PTR_ERR(priv->pll[1]);
+		priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
+								 "osc", 0, 533, 12);
+		if (IS_ERR(priv->pll[1]))
+			return PTR_ERR(priv->pll[1]);
 
 	/* 24MHz -> 1188.0MHz */
-	priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
-							 "osc", 0, 99, 2);
-	if (IS_ERR(priv->pll[2]))
-		return PTR_ERR(priv->pll[2]);
+		priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
+								 "osc", 0, 99, 2);
+		if (IS_ERR(priv->pll[2]))
+			return PTR_ERR(priv->pll[2]);
+	}
 
 	for (idx = 0; idx < JH7110_SYSCLK_END; idx++) {
 		u32 max = jh7110_sysclk_data[idx].max;
@@ -462,8 +465,14 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
 				parents[i].fw_name = "tdm_ext";
 			else if (pidx == JH7110_SYSCLK_MCLK_EXT)
 				parents[i].fw_name = "mclk_ext";
-			else
+			else if (use_fixed_pll)
 				parents[i].hw = priv->pll[pidx - JH7110_SYSCLK_PLL0_OUT];
+			else if (pidx == JH7110_SYSCLK_PLL0_OUT)
+				parents[i].fw_name = "pll0_out";
+			else if (pidx == JH7110_SYSCLK_PLL1_OUT)
+				parents[i].fw_name = "pll1_out";
+			else if (pidx == JH7110_SYSCLK_PLL2_OUT)
+				parents[i].fw_name = "pll2_out";
 		}
 
 		clk->hw.init = &init;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 6/7] riscv: dts: starfive: jh7110: Add syscon nodes
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
                   ` (4 preceding siblings ...)
  2023-06-13 12:58 ` [PATCH v5 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 19:58   ` Conor Dooley
  2023-06-13 12:58 ` [PATCH v5 7/7] riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node Xingyu Wu
  6 siblings, 1 reply; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

From: William Qiu <william.qiu@starfivetech.com>

Add stg_syscon/sys_syscon/aon_syscon/PLL nodes for JH7110 Soc.

Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 4c5fdb905da8..11dd4c9d64b0 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -353,6 +353,11 @@ i2c2: i2c@10050000 {
 			status = "disabled";
 		};
 
+		stg_syscon: syscon@10240000 {
+			compatible = "starfive,jh7110-stg-syscon", "syscon";
+			reg = <0x0 0x10240000 0x0 0x1000>;
+		};
+
 		uart3: serial@12000000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x0 0x12000000 0x0 0x10000>;
@@ -457,6 +462,17 @@ syscrg: clock-controller@13020000 {
 			#reset-cells = <1>;
 		};
 
+		sys_syscon: syscon@13030000 {
+			compatible = "starfive,jh7110-sys-syscon", "syscon", "simple-mfd";
+			reg = <0x0 0x13030000 0x0 0x1000>;
+
+			pllclk: clock-controller {
+				compatible = "starfive,jh7110-pll";
+				clocks = <&osc>;
+				#clock-cells = <1>;
+			};
+		};
+
 		sysgpio: pinctrl@13040000 {
 			compatible = "starfive,jh7110-sys-pinctrl";
 			reg = <0x0 0x13040000 0x0 0x10000>;
@@ -486,6 +502,12 @@ aoncrg: clock-controller@17000000 {
 			#reset-cells = <1>;
 		};
 
+		aon_syscon: syscon@17010000 {
+			compatible = "starfive,jh7110-aon-syscon", "syscon";
+			reg = <0x0 0x17010000 0x0 0x1000>;
+			#power-domain-cells = <1>;
+		};
+
 		aongpio: pinctrl@17020000 {
 			compatible = "starfive,jh7110-aon-pinctrl";
 			reg = <0x0 0x17020000 0x0 0x10000>;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 7/7] riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node
  2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
                   ` (5 preceding siblings ...)
  2023-06-13 12:58 ` [PATCH v5 6/7] riscv: dts: starfive: jh7110: Add syscon nodes Xingyu Wu
@ 2023-06-13 12:58 ` Xingyu Wu
  2023-06-13 19:55   ` Conor Dooley
  6 siblings, 1 reply; 19+ messages in thread
From: Xingyu Wu @ 2023-06-13 12:58 UTC (permalink / raw)
  To: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, Xingyu Wu,
	William Qiu, linux-kernel, linux-clk

Modify the SYSCRG node to add PLL clocks input from
PLL clocks driver.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 11dd4c9d64b0..cdfd036a0e6c 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -452,12 +452,16 @@ syscrg: clock-controller@13020000 {
 				 <&gmac1_rgmii_rxin>,
 				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
 				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
-				 <&tdm_ext>, <&mclk_ext>;
+				 <&tdm_ext>, <&mclk_ext>,
+				 <&pllclk JH7110_CLK_PLL0_OUT>,
+				 <&pllclk JH7110_CLK_PLL1_OUT>,
+				 <&pllclk JH7110_CLK_PLL2_OUT>;
 			clock-names = "osc", "gmac1_rmii_refin",
 				      "gmac1_rgmii_rxin",
 				      "i2stx_bclk_ext", "i2stx_lrck_ext",
 				      "i2srx_bclk_ext", "i2srx_lrck_ext",
-				      "tdm_ext", "mclk_ext";
+				      "tdm_ext", "mclk_ext",
+				      "pll0_out", "pll1_out", "pll2_out";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 		};
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-13 12:58 ` [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module Xingyu Wu
@ 2023-06-13 18:31   ` Krzysztof Kozlowski
  2023-06-28  6:44     ` Xingyu Wu
  2023-06-13 18:32   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 19+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-13 18:31 UTC (permalink / raw)
  To: Xingyu Wu, linux-riscv, devicetree, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
	Conor Dooley, Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, William Qiu,
	linux-kernel, linux-clk

On 13/06/2023 14:58, Xingyu Wu wrote:
> From: William Qiu <william.qiu@starfivetech.com>
> 
> Add documentation to describe StarFive System Controller Registers.
> 
> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> new file mode 100644
> index 000000000000..a81190f8a54d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 SoC system controller
> +
> +maintainers:
> +  - William Qiu <william.qiu@starfivetech.com>
> +
> +description: |
> +  The StarFive JH7110 SoC system controller provides register information such
> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: starfive,jh7110-sys-syscon
> +          - const: syscon
> +          - const: simple-mfd
> +      - items:
> +          - enum:
> +              - starfive,jh7110-aon-syscon
> +              - starfive,jh7110-stg-syscon
> +          - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-controller:
> +    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> +    type: object
> +
> +  "#power-domain-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-aon-syscon
> +    then:
> +      required:
> +        - "#power-domain-cells"

Where did you implement the results of the discussion that only some
devices can have power and clock controller?

According to your code all of above - sys, aon and stg - have clock and
power controllers. If not, then the code is not correct, so please do
not respond with what is where (like you did last time) but actually
implement what you say.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-13 12:58 ` [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module Xingyu Wu
  2023-06-13 18:31   ` Krzysztof Kozlowski
@ 2023-06-13 18:32   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-13 18:32 UTC (permalink / raw)
  To: Xingyu Wu, linux-riscv, devicetree, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
	Conor Dooley, Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, William Qiu,
	linux-kernel, linux-clk

On 13/06/2023 14:58, Xingyu Wu wrote:
> From: William Qiu <william.qiu@starfivetech.com>
> 
> Add documentation to describe StarFive System Controller Registers.
> 
> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> new file mode 100644
> index 000000000000..a81190f8a54d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 SoC system controller
> +
> +maintainers:
> +  - William Qiu <william.qiu@starfivetech.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The StarFive JH7110 SoC system controller provides register information such
> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: starfive,jh7110-sys-syscon
> +          - const: syscon
> +          - const: simple-mfd
> +      - items:
> +          - enum:
> +              - starfive,jh7110-aon-syscon
> +              - starfive,jh7110-stg-syscon
> +          - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-controller:
> +    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> +    type: object
> +
> +  "#power-domain-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-aon-syscon
> +    then:
> +      required:
> +        - "#power-domain-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    syscon@10240000 {
> +        compatible = "starfive,jh7110-stg-syscon", "syscon";
> +        reg = <0x10240000 0x1000>;

Extend example - add clock controller, add power-domains to STG (since
it has them, as you claim in the binding).

Make this one example complete.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
  2023-06-13 12:58 ` [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs Xingyu Wu
@ 2023-06-13 18:34   ` Krzysztof Kozlowski
  2023-06-13 19:17     ` Conor Dooley
  0 siblings, 1 reply; 19+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-13 18:34 UTC (permalink / raw)
  To: Xingyu Wu, linux-riscv, devicetree, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
	Conor Dooley, Emil Renner Berthing
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, William Qiu,
	linux-kernel, linux-clk

On 13/06/2023 14:58, Xingyu Wu wrote:
> Add optional PLL clock inputs from PLL clock generator.

Are you sure that PLLs are optional? Usually they are not...

> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../clock/starfive,jh7110-syscrg.yaml         | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> index 84373ae31644..5536e5f9e20b 100644
> --- a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> @@ -39,6 +39,33 @@ properties:
>            - description: External TDM clock
>            - description: External audio master clock
>  
> +      - items:
> +          - description: Main Oscillator (24 MHz)
> +          - description: GMAC1 RMII reference or GMAC1 RGMII RX
> +          - description: External I2S TX bit clock
> +          - description: External I2S TX left/right channel clock
> +          - description: External I2S RX bit clock
> +          - description: External I2S RX left/right channel clock
> +          - description: External TDM clock
> +          - description: External audio master clock
> +          - description: PLL0
> +          - description: PLL1
> +          - description: PLL2

Add these three to the existing entry with minItems.

> +
> +      - items:
> +          - description: Main Oscillator (24 MHz)
> +          - description: GMAC1 RMII reference
> +          - description: GMAC1 RGMII RX
> +          - description: External I2S TX bit clock
> +          - description: External I2S TX left/right channel clock
> +          - description: External I2S RX bit clock
> +          - description: External I2S RX left/right channel clock
> +          - description: External TDM clock
> +          - description: External audio master clock
> +          - description: PLL0
> +          - description: PLL1
> +          - description: PLL2

Add these three to the existing entry with minItems.



> +
>    clock-names:
>      oneOf:
>        - items:
> @@ -64,6 +91,35 @@ properties:
>            - const: tdm_ext
>            - const: mclk_ext
>  
> +      - items:
> +          - const: osc
> +          - enum:
> +              - gmac1_rmii_refin
> +              - gmac1_rgmii_rxin
> +          - const: i2stx_bclk_ext
> +          - const: i2stx_lrck_ext
> +          - const: i2srx_bclk_ext
> +          - const: i2srx_lrck_ext
> +          - const: tdm_ext
> +          - const: mclk_ext
> +          - const: pll0_out
> +          - const: pll1_out
> +          - const: pll2_out

Add these three to the existing entry with minItems.


> +
> +      - items:
> +          - const: osc
> +          - const: gmac1_rmii_refin
> +          - const: gmac1_rgmii_rxin
> +          - const: i2stx_bclk_ext
> +          - const: i2stx_lrck_ext
> +          - const: i2srx_bclk_ext
> +          - const: i2srx_lrck_ext
> +          - const: tdm_ext
> +          - const: mclk_ext
> +          - const: pll0_out
> +          - const: pll1_out
> +          - const: pll2_out

Add these three to the existing entry with minItems.



Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator
  2023-06-13 12:58 ` [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator Xingyu Wu
@ 2023-06-13 19:06   ` Conor Dooley
  0 siblings, 0 replies; 19+ messages in thread
From: Conor Dooley @ 2023-06-13 19:06 UTC (permalink / raw)
  To: Xingyu Wu
  Cc: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Hal Feng, William Qiu, linux-kernel, linux-clk

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

Hey Xingyu,

Couple nitpick items to be fixed if you resubmit.

On Tue, Jun 13, 2023 at 08:58:46PM +0800, Xingyu Wu wrote:
> +  This PLL are high speed, low jitter frequency synthesizers in JH7110.

nit: These PLLs are

> +  Each PLL clocks work in integer mode or fraction mode by some dividers,
> +  and the configuration registers and dividers are set in several syscon
> +  registers. So pll node should be a child of SYS-SYSCON node.

nit: Each PLL can work in integer or fractional mode, with controlled by
configuration registers in the sys syscon.

> +  The formula for calculating frequency is that,

nit: s/ that//

> +examples:
> +  - |
> +    pll-clock-controller {

nit: s/pll-//

> diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
> index 06257bfd9ac1..086a6ddcf380 100644
> --- a/include/dt-bindings/clock/starfive,jh7110-crg.h
> +++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
> @@ -6,6 +6,12 @@
>  #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>  #define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>  
> +/* PLL clocks */
> +#define JH7110_CLK_PLL0_OUT			0
> +#define JH7110_CLK_PLL1_OUT			1
> +#define JH7110_CLK_PLL2_OUT			2
> +#define JH7110_PLLCLK_END			3

Please CC me on the patches fixing this for U-Boot :)

Nitpicking aside, which only needs to be fixed if you end up submitting
a new version for other reasons,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
  2023-06-13 18:34   ` Krzysztof Kozlowski
@ 2023-06-13 19:17     ` Conor Dooley
  0 siblings, 0 replies; 19+ messages in thread
From: Conor Dooley @ 2023-06-13 19:17 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Xingyu Wu, linux-riscv, devicetree, Michael Turquette,
	Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
	Conor Dooley, Emil Renner Berthing, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, Hal Feng, William Qiu, linux-kernel,
	linux-clk

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

On Tue, Jun 13, 2023 at 08:34:25PM +0200, Krzysztof Kozlowski wrote:
> On 13/06/2023 14:58, Xingyu Wu wrote:
> > Add optional PLL clock inputs from PLL clock generator.
> 
> Are you sure that PLLs are optional? Usually they are not...

They're not. What's happening here is the original binding was defined
without these clocks (obviously, since they're only being added now) so
for the driver they are still "optional" to keep compatibility.
In mainline, the driver takes the "osc" input and registers some
fixed-factor clocks to mimic these PLLs & after this patchset that is
only done as a fallback if the clock inputs to the clock controller,
from the PLLs, are missing.
They should not be optional in the dt-binding because they're not
optional in the hardware afaik!

Cheers,
Conor.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 7/7] riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node
  2023-06-13 12:58 ` [PATCH v5 7/7] riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node Xingyu Wu
@ 2023-06-13 19:55   ` Conor Dooley
  0 siblings, 0 replies; 19+ messages in thread
From: Conor Dooley @ 2023-06-13 19:55 UTC (permalink / raw)
  To: Xingyu Wu
  Cc: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Hal Feng, William Qiu, linux-kernel, linux-clk

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

On Tue, Jun 13, 2023 at 08:58:52PM +0800, Xingyu Wu wrote:
> Modify the SYSCRG node to add PLL clocks input from
> PLL clocks driver.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 6/7] riscv: dts: starfive: jh7110: Add syscon nodes
  2023-06-13 12:58 ` [PATCH v5 6/7] riscv: dts: starfive: jh7110: Add syscon nodes Xingyu Wu
@ 2023-06-13 19:58   ` Conor Dooley
  0 siblings, 0 replies; 19+ messages in thread
From: Conor Dooley @ 2023-06-13 19:58 UTC (permalink / raw)
  To: Xingyu Wu
  Cc: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Krzysztof Kozlowski, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Hal Feng, William Qiu, linux-kernel, linux-clk

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

On Tue, Jun 13, 2023 at 08:58:51PM +0800, Xingyu Wu wrote:
> From: William Qiu <william.qiu@starfivetech.com>
> 
> Add stg_syscon/sys_syscon/aon_syscon/PLL nodes for JH7110 Soc.
> 
> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-13 18:31   ` Krzysztof Kozlowski
@ 2023-06-28  6:44     ` Xingyu Wu
  2023-06-28 17:34       ` Conor Dooley
  0 siblings, 1 reply; 19+ messages in thread
From: Xingyu Wu @ 2023-06-28  6:44 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-riscv, devicetree, Michael Turquette, Stephen Boyd,
	Rob Herring, Philipp Zabel, Conor Dooley, Emil Renner Berthing,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, William Qiu,
	linux-kernel, linux-clk

On 2023/6/14 2:31, Krzysztof Kozlowski wrote:
> On 13/06/2023 14:58, Xingyu Wu wrote:
>> From: William Qiu <william.qiu@starfivetech.com>
>> 
>> Add documentation to describe StarFive System Controller Registers.
>> 
>> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
>>  MAINTAINERS                                   |  7 +++
>>  2 files changed, 69 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> new file mode 100644
>> index 000000000000..a81190f8a54d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> @@ -0,0 +1,62 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH7110 SoC system controller
>> +
>> +maintainers:
>> +  - William Qiu <william.qiu@starfivetech.com>
>> +
>> +description: |
>> +  The StarFive JH7110 SoC system controller provides register information such
>> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - const: starfive,jh7110-sys-syscon
>> +          - const: syscon
>> +          - const: simple-mfd
>> +      - items:
>> +          - enum:
>> +              - starfive,jh7110-aon-syscon
>> +              - starfive,jh7110-stg-syscon
>> +          - const: syscon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clock-controller:
>> +    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
>> +    type: object
>> +
>> +  "#power-domain-cells":
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: starfive,jh7110-aon-syscon
>> +    then:
>> +      required:
>> +        - "#power-domain-cells"
> 
> Where did you implement the results of the discussion that only some
> devices can have power and clock controller?
> 
> According to your code all of above - sys, aon and stg - have clock and
> power controllers. If not, then the code is not correct, so please do
> not respond with what is where (like you did last time) but actually
> implement what you say.
> 

Hi Krzysztof, I need to modify the code to implement it.
If I drop the 'clock-controller' and '"#power-domain-cells"' in properites, and change to this:

--- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -29,28 +29,33 @@ properties:
   reg:
     maxItems: 1
 
-  clock-controller:
-    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
-    type: object
-
-  "#power-domain-cells":
-    const: 1
-
 required:
   - compatible
   - reg
 
 allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-sys-syscon
+    then:
+      properties:
+        clock-controller:
+          $ref: /schemas/clock/starfive,jh7110-pll.yaml#
+          type: object
+
   - if:
       properties:
         compatible:
           contains:
             const: starfive,jh7110-aon-syscon
     then:
-      required:
-        - "#power-domain-cells"
+      properties:
+        "#power-domain-cells":
+          const: 1
 
-additionalProperties: false
+additionalProperties: true


Would it be better to show that sys-syscon only has clock-controller and aon-syscon is power controller?

Best regards,
Xingyu Wu


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-28  6:44     ` Xingyu Wu
@ 2023-06-28 17:34       ` Conor Dooley
  2023-06-29  6:42         ` Xingyu Wu
  0 siblings, 1 reply; 19+ messages in thread
From: Conor Dooley @ 2023-06-28 17:34 UTC (permalink / raw)
  To: Xingyu Wu
  Cc: Krzysztof Kozlowski, linux-riscv, devicetree, Michael Turquette,
	Stephen Boyd, Rob Herring, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Hal Feng, William Qiu, linux-kernel, linux-clk

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

On Wed, Jun 28, 2023 at 02:44:10PM +0800, Xingyu Wu wrote:
> On 2023/6/14 2:31, Krzysztof Kozlowski wrote:
> > On 13/06/2023 14:58, Xingyu Wu wrote:
> >> From: William Qiu <william.qiu@starfivetech.com>
> >> 
> >> Add documentation to describe StarFive System Controller Registers.
> >> 
> >> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> >> ---
> >>  .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
> >>  MAINTAINERS                                   |  7 +++
> >>  2 files changed, 69 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> >> 
> >> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> >> new file mode 100644
> >> index 000000000000..a81190f8a54d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> >> @@ -0,0 +1,62 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: StarFive JH7110 SoC system controller
> >> +
> >> +maintainers:
> >> +  - William Qiu <william.qiu@starfivetech.com>
> >> +
> >> +description: |
> >> +  The StarFive JH7110 SoC system controller provides register information such
> >> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    oneOf:
> >> +      - items:
> >> +          - const: starfive,jh7110-sys-syscon
> >> +          - const: syscon
> >> +          - const: simple-mfd
> >> +      - items:
> >> +          - enum:
> >> +              - starfive,jh7110-aon-syscon
> >> +              - starfive,jh7110-stg-syscon
> >> +          - const: syscon
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clock-controller:
> >> +    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> >> +    type: object
> >> +
> >> +  "#power-domain-cells":
> >> +    const: 1
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const: starfive,jh7110-aon-syscon
> >> +    then:
> >> +      required:
> >> +        - "#power-domain-cells"
> > 
> > Where did you implement the results of the discussion that only some
> > devices can have power and clock controller?
> > 
> > According to your code all of above - sys, aon and stg - have clock and
> > power controllers. If not, then the code is not correct, so please do
> > not respond with what is where (like you did last time) but actually
> > implement what you say.
> > 
> 
> Hi Krzysztof, I need to modify the code to implement it.
> If I drop the 'clock-controller' and '"#power-domain-cells"' in properites, and change to this:
> 
> --- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -29,28 +29,33 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  clock-controller:
> -    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> -    type: object
> -
> -  "#power-domain-cells":
> -    const: 1
> -
>  required:
>    - compatible
>    - reg
>  
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-sys-syscon
> +    then:
> +      properties:
> +        clock-controller:
> +          $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> +          type: object

Why do this?
Why not define the property has you have been doing, but only allow it
on the syscons that support it?
See the section starting at L205 of example-schema.yaml.

> +
>    - if:
>        properties:
>          compatible:
>            contains:
>              const: starfive,jh7110-aon-syscon
>      then:
> -      required:
> -        - "#power-domain-cells"
> +      properties:
> +        "#power-domain-cells":
> +          const: 1
>  

> -additionalProperties: false
> +additionalProperties: true

Why do you need this?
Allowing "additionalProperties: true" sounds like you've got some prblem
that you are trying to hide...

> Would it be better to show that sys-syscon only has clock-controller and aon-syscon is power controller?

You should only permit the properties where they are valid, yes.

Cheers,
Conor.


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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-28 17:34       ` Conor Dooley
@ 2023-06-29  6:42         ` Xingyu Wu
  2023-06-29  9:01           ` Conor Dooley
  0 siblings, 1 reply; 19+ messages in thread
From: Xingyu Wu @ 2023-06-29  6:42 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Krzysztof Kozlowski, linux-riscv, devicetree, Michael Turquette,
	Stephen Boyd, Rob Herring, Philipp Zabel, Conor Dooley,
	Emil Renner Berthing, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Hal Feng, William Qiu, linux-kernel, linux-clk

On 2023/6/29 1:34, Conor Dooley wrote:
> On Wed, Jun 28, 2023 at 02:44:10PM +0800, Xingyu Wu wrote:
>> On 2023/6/14 2:31, Krzysztof Kozlowski wrote:
>> > On 13/06/2023 14:58, Xingyu Wu wrote:
>> >> From: William Qiu <william.qiu@starfivetech.com>
>> >> 
>> >> Add documentation to describe StarFive System Controller Registers.
>> >> 
>> >> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> >> ---
>> >>  .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
>> >>  MAINTAINERS                                   |  7 +++
>> >>  2 files changed, 69 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> >> 
>> >> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> >> new file mode 100644
>> >> index 000000000000..a81190f8a54d
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> >> @@ -0,0 +1,62 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: StarFive JH7110 SoC system controller
>> >> +
>> >> +maintainers:
>> >> +  - William Qiu <william.qiu@starfivetech.com>
>> >> +
>> >> +description: |
>> >> +  The StarFive JH7110 SoC system controller provides register information such
>> >> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    oneOf:
>> >> +      - items:
>> >> +          - const: starfive,jh7110-sys-syscon
>> >> +          - const: syscon
>> >> +          - const: simple-mfd
>> >> +      - items:
>> >> +          - enum:
>> >> +              - starfive,jh7110-aon-syscon
>> >> +              - starfive,jh7110-stg-syscon
>> >> +          - const: syscon
>> >> +
>> >> +  reg:
>> >> +    maxItems: 1
>> >> +
>> >> +  clock-controller:
>> >> +    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
>> >> +    type: object
>> >> +
>> >> +  "#power-domain-cells":
>> >> +    const: 1
>> >> +
>> >> +required:
>> >> +  - compatible
>> >> +  - reg
>> >> +
>> >> +allOf:
>> >> +  - if:
>> >> +      properties:
>> >> +        compatible:
>> >> +          contains:
>> >> +            const: starfive,jh7110-aon-syscon
>> >> +    then:
>> >> +      required:
>> >> +        - "#power-domain-cells"
>> > 
>> > Where did you implement the results of the discussion that only some
>> > devices can have power and clock controller?
>> > 
>> > According to your code all of above - sys, aon and stg - have clock and
>> > power controllers. If not, then the code is not correct, so please do
>> > not respond with what is where (like you did last time) but actually
>> > implement what you say.
>> > 
>> 
>> Hi Krzysztof, I need to modify the code to implement it.
>> If I drop the 'clock-controller' and '"#power-domain-cells"' in properites, and change to this:
>> 
>> --- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>> @@ -29,28 +29,33 @@ properties:
>>    reg:
>>      maxItems: 1
>>  
>> -  clock-controller:
>> -    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
>> -    type: object
>> -
>> -  "#power-domain-cells":
>> -    const: 1
>> -
>>  required:
>>    - compatible
>>    - reg
>>  
>>  allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: starfive,jh7110-sys-syscon
>> +    then:
>> +      properties:
>> +        clock-controller:
>> +          $ref: /schemas/clock/starfive,jh7110-pll.yaml#
>> +          type: object
> 
> Why do this?
> Why not define the property has you have been doing, but only allow it
> on the syscons that support it?
> See the section starting at L205 of example-schema.yaml.
> 
>> +
>>    - if:
>>        properties:
>>          compatible:
>>            contains:
>>              const: starfive,jh7110-aon-syscon
>>      then:
>> -      required:
>> -        - "#power-domain-cells"
>> +      properties:
>> +        "#power-domain-cells":
>> +          const: 1
>>  
> 
>> -additionalProperties: false
>> +additionalProperties: true
> 
> Why do you need this?
> Allowing "additionalProperties: true" sounds like you've got some prblem
> that you are trying to hide...
> 
>> Would it be better to show that sys-syscon only has clock-controller and aon-syscon is power controller?
> 
> You should only permit the properties where they are valid, yes.
> 

Yeah, following your advice, I modified the codes and there are two options:

--- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -41,6 +41,16 @@ required:
   - reg
 
 allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-sys-syscon
+    then:
+      required:
+        - clock-controller
+      properties:
+        "#power-domain-cells": false
   - if:
       properties:
         compatible:
           contains:
             const: starfive,jh7110-aon-syscon
     then:
       required:
         - "#power-domain-cells"
+      properties:
+        clock-controller: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-stg-syscon
+    then:
+      properties:
+        clock-controller: false
+        "#power-domain-cells": false
 
 additionalProperties: false

Or :

--- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -41,6 +41,17 @@ required:
   - reg
 
 allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-sys-syscon
+    then:
+      required:
+        - clock-controller
+    else:
+      properties:
+        clock-controller: false
   - if:
       properties:
         compatible:
           contains:
             const: starfive,jh7110-aon-syscon
     then:
       required:
         - "#power-domain-cells"
+    else:
+      properties:
+        "#power-domain-cells": false
 
 additionalProperties: false

Which one is better? Thanks.

Best regards,
Xingyu Wu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
  2023-06-29  6:42         ` Xingyu Wu
@ 2023-06-29  9:01           ` Conor Dooley
  0 siblings, 0 replies; 19+ messages in thread
From: Conor Dooley @ 2023-06-29  9:01 UTC (permalink / raw)
  To: Xingyu Wu
  Cc: Conor Dooley, Krzysztof Kozlowski, linux-riscv, devicetree,
	Michael Turquette, Stephen Boyd, Rob Herring, Philipp Zabel,
	Conor Dooley, Emil Renner Berthing, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, Hal Feng, William Qiu, linux-kernel,
	linux-clk

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

On Thu, Jun 29, 2023 at 02:42:39PM +0800, Xingyu Wu wrote:
> On 2023/6/29 1:34, Conor Dooley wrote:
> > On Wed, Jun 28, 2023 at 02:44:10PM +0800, Xingyu Wu wrote:
> >> On 2023/6/14 2:31, Krzysztof Kozlowski wrote:
> >> > On 13/06/2023 14:58, Xingyu Wu wrote:
> >> >> From: William Qiu <william.qiu@starfivetech.com>

> >> >> +allOf:
> >> >> +  - if:
> >> >> +      properties:
> >> >> +        compatible:
> >> >> +          contains:
> >> >> +            const: starfive,jh7110-aon-syscon
> >> >> +    then:
> >> >> +      required:
> >> >> +        - "#power-domain-cells"
> >> > 
> >> > Where did you implement the results of the discussion that only some
> >> > devices can have power and clock controller?
> >> > 
> >> > According to your code all of above - sys, aon and stg - have clock and
> >> > power controllers. If not, then the code is not correct, so please do
> >> > not respond with what is where (like you did last time) but actually
> >> > implement what you say.

> Yeah, following your advice, I modified the codes and there are two options:
> 
> --- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -41,6 +41,16 @@ required:
>    - reg
>  
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-sys-syscon
> +    then:
> +      required:
> +        - clock-controller
> +      properties:
> +        "#power-domain-cells": false
>    - if:
>        properties:
>          compatible:
>            contains:
>              const: starfive,jh7110-aon-syscon
>      then:
>        required:
>          - "#power-domain-cells"
> +      properties:
> +        clock-controller: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-stg-syscon
> +    then:
> +      properties:
> +        clock-controller: false
> +        "#power-domain-cells": false
>  
>  additionalProperties: false
> 
> Or :
> 
> --- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -41,6 +41,17 @@ required:
>    - reg
>  
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-sys-syscon
> +    then:
> +      required:
> +        - clock-controller
> +    else:
> +      properties:
> +        clock-controller: false
>    - if:
>        properties:
>          compatible:
>            contains:
>              const: starfive,jh7110-aon-syscon
>      then:
>        required:
>          - "#power-domain-cells"
> +    else:
> +      properties:
> +        "#power-domain-cells": false
>  
>  additionalProperties: false
> 
> Which one is better? Thanks.

This second one looks better to me, as it achieves the same thing in a
simpler way.

Cheers,
Conor.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-06-29  9:01 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-13 12:58 [PATCH v5 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC Xingyu Wu
2023-06-13 12:58 ` [PATCH v5 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator Xingyu Wu
2023-06-13 19:06   ` Conor Dooley
2023-06-13 12:58 ` [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module Xingyu Wu
2023-06-13 18:31   ` Krzysztof Kozlowski
2023-06-28  6:44     ` Xingyu Wu
2023-06-28 17:34       ` Conor Dooley
2023-06-29  6:42         ` Xingyu Wu
2023-06-29  9:01           ` Conor Dooley
2023-06-13 18:32   ` Krzysztof Kozlowski
2023-06-13 12:58 ` [PATCH v5 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs Xingyu Wu
2023-06-13 18:34   ` Krzysztof Kozlowski
2023-06-13 19:17     ` Conor Dooley
2023-06-13 12:58 ` [PATCH v5 4/7] clk: starfive: Add StarFive JH7110 PLL clock driver Xingyu Wu
2023-06-13 12:58 ` [PATCH v5 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS Xingyu Wu
2023-06-13 12:58 ` [PATCH v5 6/7] riscv: dts: starfive: jh7110: Add syscon nodes Xingyu Wu
2023-06-13 19:58   ` Conor Dooley
2023-06-13 12:58 ` [PATCH v5 7/7] riscv: dts: starfive: jh7110: Add PLL clock source in SYSCRG node Xingyu Wu
2023-06-13 19:55   ` Conor Dooley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).