[PATCH V6 0/3] Add S4 SoC PLL and Peripheral clock controller

[PATCH V6 0/3] Add S4 SoC PLL and Peripheral clock controller

[Yu Tu]

1. Add S4 SoC PLL and Peripheral clock controller dt-bindings.
2. Add PLL and Peripheral clock controller driver for S4 SOC.

Yu Tu (3):
  dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock
    controller
  clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver
  clk: meson: s4: add support for Amlogic S4 SoC peripheral clock
    controller

V5 -> V6: Change send patch series, as well change format and clock flags.
V4 -> V5: change format and clock flags and adjust the patch series as suggested
by Jerome.
V3 -> V4: change format and clock flags.
V2 -> V3: Use two clock controller.
V1 -> V2: Change format as discussed in the email.

Link:https://lore.kernel.org/all/20221123021346.18136-1-yu.tu@amlogic.com/

 .../clock/amlogic,s4-peripherals-clkc.yaml    |  104 +
 .../bindings/clock/amlogic,s4-pll-clkc.yaml   |   50 +
 MAINTAINERS                                   |    1 +
 drivers/clk/meson/Kconfig                     |   25 +
 drivers/clk/meson/Makefile                    |    2 +
 drivers/clk/meson/s4-peripherals.c            | 3874 +++++++++++++++++
 drivers/clk/meson/s4-peripherals.h            |  218 +
 drivers/clk/meson/s4-pll.c                    |  875 ++++
 drivers/clk/meson/s4-pll.h                    |   88 +
 .../clock/amlogic,s4-peripherals-clkc.h       |  131 +
 .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |   30 +
 11 files changed, 5398 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
 create mode 100644 drivers/clk/meson/s4-peripherals.c
 create mode 100644 drivers/clk/meson/s4-peripherals.h
 create mode 100644 drivers/clk/meson/s4-pll.c
 create mode 100644 drivers/clk/meson/s4-pll.h
 create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
 create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Yu Tu]

Add the S4 PLL & peripheral clock controller dt-bindings in the s4 SoC
family.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 .../clock/amlogic,s4-peripherals-clkc.yaml    | 104 ++++++++++++++
 .../bindings/clock/amlogic,s4-pll-clkc.yaml   |  50 +++++++
 MAINTAINERS                                   |   1 +
 .../clock/amlogic,s4-peripherals-clkc.h       | 131 ++++++++++++++++++
 .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |  30 ++++
 5 files changed, 316 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
 create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
 create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h

diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
new file mode 100644
index 000000000000..2deeff497754
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,s4-peripherals-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson S serials Peripherals Clock Controller
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+  - Jerome Brunet <jbrunet@baylibre.com>
+  - Yu Tu <yu.tu@amlogic.com>
+
+properties:
+  compatible:
+    const: amlogic,s4-peripherals-clkc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: input fixed pll div2
+      - description: input fixed pll div2p5
+      - description: input fixed pll div3
+      - description: input fixed pll div4
+      - description: input fixed pll div5
+      - description: input fixed pll div7
+      - description: input hifi pll
+      - description: input gp0 pll
+      - description: input mpll0
+      - description: input mpll1
+      - description: input mpll2
+      - description: input mpll3
+      - description: input hdmi pll
+      - description: input oscillator (usually at 24MHz)
+      - description: input external 32kHz reference (optional)
+
+  clock-names:
+    items:
+      - const: fclk_div2
+      - const: fclk_div2p5
+      - const: fclk_div3
+      - const: fclk_div4
+      - const: fclk_div5
+      - const: fclk_div7
+      - const: hifi_pll
+      - const: gp0_pll
+      - const: mpll0
+      - const: mpll1
+      - const: mpll2
+      - const: mpll3
+      - const: hdmi_pll
+      - const: xtal
+      - const: ext_32k
+
+  "#clock-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h>
+
+    /* 32KHz reference crystal */
+    ext_32k: ref32k {
+        compatible = "fixed-clock";
+        #clock-cells = <0>;
+        clock-frequency = <32000>;
+    };
+
+    clkc_periphs: clock-controller@fe000000 {
+      compatible = "amlogic,s4-peripherals-clkc";
+      reg = <0xfe000000 0x49c>;
+      clocks = <&clkc_pll 3>,
+              <&clkc_pll 13>,
+              <&clkc_pll 5>,
+              <&clkc_pll 7>,
+              <&clkc_pll 9>,
+              <&clkc_pll 11>,
+              <&clkc_pll 17>,
+              <&clkc_pll 15>,
+              <&clkc_pll 25>,
+              <&clkc_pll 27>,
+              <&clkc_pll 29>,
+              <&clkc_pll 31>,
+              <&clkc_pll 20>,
+              <&xtal>,
+              <&ext_32k>;
+      clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", "fclk_div4",
+                    "fclk_div5", "fclk_div7", "hifi_pll", "gp0_pll",
+                    "mpll0", "mpll1", "mpll2", "mpll3", "hdmi_pll", "xtal",
+                    "ext_32k";
+      #clock-cells = <1>;
+    };
+...
diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
new file mode 100644
index 000000000000..aeda4861cebe
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson S serials PLL Clock Controller
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+  - Jerome Brunet <jbrunet@baylibre.com>
+  - Yu Tu <yu.tu@amlogic.com>
+
+properties:
+  compatible:
+    const: amlogic,s4-pll-clkc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: xtal
+
+  "#clock-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    clkc_pll: clock-controller@fe008000 {
+      compatible = "amlogic,s4-pll-clkc";
+      reg = <0xfe008000 0x1e8>;
+      clocks = <&xtal>;
+      clock-names = "xtal";
+      #clock-cells = <1>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f61eb221415b..26c82beeffda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1897,6 +1897,7 @@ L:	linux-amlogic@lists.infradead.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/amlogic*
 F:	drivers/clk/meson/
+F:	include/dt-bindings/clock/amlogic*
 F:	include/dt-bindings/clock/gxbb*
 F:	include/dt-bindings/clock/meson*
 
diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
new file mode 100644
index 000000000000..bbec5094d5c3
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
@@ -0,0 +1,131 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
+ * Author: Yu Tu <yu.tu@amlogic.com>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PERIPHERALS_CLKC_H
+#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PERIPHERALS_CLKC_H
+
+/*
+ * CLKID index values
+ */
+
+#define CLKID_RTC_CLK			4
+#define CLKID_SYS_CLK_B_GATE		7
+#define CLKID_SYS_CLK_A_GATE		10
+#define CLKID_SYS_CLK			11
+#define CLKID_CECA_32K_CLKOUT		16
+#define CLKID_CECB_32K_CLKOUT		21
+#define CLKID_SC_CLK_GATE		24
+#define CLKID_12_24M_CLK_SEL		27
+#define CLKID_VID_PLL			30
+#define CLKID_VCLK			37
+#define CLKID_VCLK2			38
+#define CLKID_VCLK_DIV1			39
+#define CLKID_VCLK2_DIV1		44
+#define CLKID_VCLK_DIV2			49
+#define CLKID_VCLK_DIV4			50
+#define CLKID_VCLK_DIV6			51
+#define CLKID_VCLK_DIV12		52
+#define CLKID_VCLK2_DIV2		53
+#define CLKID_VCLK2_DIV4		54
+#define CLKID_VCLK2_DIV6		55
+#define CLKID_VCLK2_DIV12		56
+#define CLKID_CTS_ENCI			61
+#define CLKID_CTS_ENCP			62
+#define CLKID_CTS_VDAC			63
+#define CLKID_HDMI			67
+#define CLKID_TS_CLK_GATE		69
+#define CLKID_MALI_0			72
+#define CLKID_MALI_1			75
+#define CLKID_MALI			76
+#define CLKID_VDEC_P0			79
+#define CLKID_VDEC_P1			82
+#define CLKID_VDEC_SEL			83
+#define CLKID_HEVCF_P0			86
+#define CLKID_HEVCF_P1			89
+#define CLKID_HEVCF_SEL			90
+#define CLKID_VPU_0			93
+#define CLKID_VPU_1			96
+#define CLKID_VPU			97
+#define CLKID_VPU_CLKB_TMP		100
+#define CLKID_VPU_CLKB			102
+#define CLKID_VPU_CLKC_P0		105
+#define CLKID_VPU_CLKC_P1		108
+#define CLKID_VPU_CLKC_SEL		109
+#define CLKID_VAPB_0			112
+#define CLKID_VAPB_1			115
+#define CLKID_VAPB			116
+#define CLKID_GE2D			117
+#define CLKID_VDIN_MEAS_GATE		120
+#define CLKID_SD_EMMC_C_CLK		123
+#define CLKID_SD_EMMC_A_CLK		126
+#define CLKID_SD_EMMC_B_CLK		129
+#define CLKID_SPICC0_GATE		132
+#define CLKID_PWM_A_GATE		135
+#define CLKID_PWM_B_GATE		138
+#define CLKID_PWM_C_GATE		141
+#define CLKID_PWM_D_GATE		144
+#define CLKID_PWM_E_GATE		147
+#define CLKID_PWM_F_GATE		150
+#define CLKID_PWM_G_GATE		153
+#define CLKID_PWM_H_GATE		156
+#define CLKID_PWM_I_GATE		159
+#define CLKID_PWM_J_GATE		162
+#define CLKID_SARADC_GATE		165
+#define CLKID_GEN_GATE			168
+#define CLKID_DDR			169
+#define CLKID_DOS			170
+#define CLKID_ETHPHY			171
+#define CLKID_MALI_GATE			172
+#define CLKID_AOCPU			173
+#define CLKID_AUCPU			174
+#define CLKID_CEC			175
+#define CLKID_SD_EMMC_A			176
+#define CLKID_SD_EMMC_B			177
+#define CLKID_NAND			178
+#define CLKID_SMARTCARD			179
+#define CLKID_ACODEC			180
+#define CLKID_SPIFC			181
+#define CLKID_MSR_CLK			182
+#define CLKID_IR_CTRL			183
+#define CLKID_AUDIO			184
+#define CLKID_ETH			185
+#define CLKID_UART_A			186
+#define CLKID_UART_B			187
+#define CLKID_UART_C			188
+#define CLKID_UART_D			189
+#define CLKID_UART_E			190
+#define CLKID_AIFIFO			191
+#define CLKID_TS_DDR			192
+#define CLKID_TS_PLL			193
+#define CLKID_G2D			194
+#define CLKID_SPICC0			195
+#define CLKID_SPICC1			196
+#define CLKID_USB			197
+#define CLKID_I2C_M_A			198
+#define CLKID_I2C_M_B			199
+#define CLKID_I2C_M_C			200
+#define CLKID_I2C_M_D			201
+#define CLKID_I2C_M_E			202
+#define CLKID_HDMITX_APB		203
+#define CLKID_I2C_S_A			204
+#define CLKID_USB1_TO_DDR		205
+#define CLKID_HDCP22			206
+#define CLKID_MMC_APB			207
+#define CLKID_RSA			208
+#define CLKID_CPU_DEBUG			209
+#define CLKID_VPU_INTR			210
+#define CLKID_DEMOD			211
+#define CLKID_SAR_ADC			212
+#define CLKID_GIC			213
+#define CLKID_PWM_AB			214
+#define CLKID_PWM_CD			215
+#define CLKID_PWM_EF			216
+#define CLKID_PWM_GH			217
+#define CLKID_PWM_IJ			218
+#define CLKID_HDCP22_ESMCLK_GATE	221
+#define CLKID_HDCP22_SKPCLK_GATE	224
+
+#endif /* _DT_BINDINGS_CLOCK_AMLOGIC_S4_PERIPHERALS_CLKC_H */
diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h
new file mode 100644
index 000000000000..345f87023886
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
+ * Author: Yu Tu <yu.tu@amlogic.com>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H
+#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H
+
+/*
+ * CLKID index values
+ */
+
+#define CLKID_FIXED_PLL			1
+#define CLKID_FCLK_DIV2			3
+#define CLKID_FCLK_DIV3			5
+#define CLKID_FCLK_DIV4			7
+#define CLKID_FCLK_DIV5			9
+#define CLKID_FCLK_DIV7			11
+#define CLKID_FCLK_DIV2P5		13
+#define CLKID_GP0_PLL			15
+#define CLKID_HIFI_PLL			17
+#define CLKID_HDMI_PLL			20
+#define CLKID_MPLL_50M			22
+#define CLKID_MPLL0			25
+#define CLKID_MPLL1			27
+#define CLKID_MPLL2			29
+#define CLKID_MPLL3			31
+
+#endif /* _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H */
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V6 2/3] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver

[Yu Tu]

Add the S4 PLL clock controller driver in the s4 SoC family.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/clk/meson/Kconfig  |  13 +
 drivers/clk/meson/Makefile |   1 +
 drivers/clk/meson/s4-pll.c | 875 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/s4-pll.h |  88 ++++
 4 files changed, 977 insertions(+)
 create mode 100644 drivers/clk/meson/s4-pll.c
 create mode 100644 drivers/clk/meson/s4-pll.h

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index fc002c155bc3..a663c90a3f3b 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -115,4 +115,17 @@ config COMMON_CLK_G12A
 	help
 	  Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
 	  devices, aka g12a. Say Y if you want peripherals to work.
+
+config COMMON_CLK_S4_PLL
+	tristate "S4 SoC PLL clock controllers support"
+	depends on ARM64
+	default y
+	select COMMON_CLK_MESON_MPLL
+	select COMMON_CLK_MESON_PLL
+	select COMMON_CLK_MESON_REGMAP
+	help
+	  Support for the pll clock controller on Amlogic S805X2 and S905Y4 devices,
+	  aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
+	  Say Y if you want the board to work, because plls are the parent of most
+	  peripherals.
 endmenu
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 6eca2a406ee3..376f49cc13f1 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
 obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
 obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
+obj-$(CONFIG_COMMON_CLK_S4_PLL) += s4-pll.o
diff --git a/drivers/clk/meson/s4-pll.c b/drivers/clk/meson/s4-pll.c
new file mode 100644
index 000000000000..227d4fd7547d
--- /dev/null
+++ b/drivers/clk/meson/s4-pll.c
@@ -0,0 +1,875 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson-S4 PLL Clock Controller Driver
+ *
+ * Copyright (c) 2021 Amlogic, inc.
+ * Author: Yu Tu <yu.tu@amlogic.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "clk-mpll.h"
+#include "clk-pll.h"
+#include "clk-regmap.h"
+#include "s4-pll.h"
+
+static DEFINE_SPINLOCK(meson_clk_lock);
+
+static struct clk_regmap s4_fixed_pll_dco = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_FIXPLL_CTRL1,
+			.shift   = 0,
+			.width   = 17,
+		},
+		.l = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.rst = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 29,
+			.width   = 1,
+		},
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fixed_pll_dco",
+		.ops = &meson_clk_pll_ro_ops,
+		.parent_data = (const struct clk_parent_data []) {
+			{ .fw_name = "xtal", }
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fixed_pll = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = ANACTRL_FIXPLL_CTRL0,
+		.shift = 16,
+		.width = 2,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fixed_pll",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fixed_pll_dco.hw
+		},
+		.num_parents = 1,
+		/*
+		 * This clock won't ever change at runtime so
+		 * CLK_SET_RATE_PARENT is not required
+		 */
+	},
+};
+
+static struct clk_fixed_factor s4_fclk_div2_div = {
+	.mult = 1,
+	.div = 2,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div2_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fclk_div2 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL1,
+		.bit_idx = 24,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div2",
+		.ops = &clk_regmap_gate_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fclk_div2_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_fixed_factor s4_fclk_div3_div = {
+	.mult = 1,
+	.div = 3,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div3_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fclk_div3 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL1,
+		.bit_idx = 20,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div3",
+		.ops = &clk_regmap_gate_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fclk_div3_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_fixed_factor s4_fclk_div4_div = {
+	.mult = 1,
+	.div = 4,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div4_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fclk_div4 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL1,
+		.bit_idx = 21,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div4",
+		.ops = &clk_regmap_gate_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fclk_div4_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_fixed_factor s4_fclk_div5_div = {
+	.mult = 1,
+	.div = 5,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div5_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fclk_div5 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL1,
+		.bit_idx = 22,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div5",
+		.ops = &clk_regmap_gate_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fclk_div5_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_fixed_factor s4_fclk_div7_div = {
+	.mult = 1,
+	.div = 7,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div7_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fclk_div7 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL1,
+		.bit_idx = 23,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div7",
+		.ops = &clk_regmap_gate_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fclk_div7_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_fixed_factor s4_fclk_div2p5_div = {
+	.mult = 2,
+	.div = 5,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div2p5_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fixed_pll.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_fclk_div2p5 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL1,
+		.bit_idx = 25,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div2p5",
+		.ops = &clk_regmap_gate_ro_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fclk_div2p5_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static const struct pll_mult_range s4_gp0_pll_mult_range = {
+	.min = 125,
+	.max = 250,
+};
+
+/*
+ * Internal gp0 pll emulation configuration parameters
+ */
+static const struct reg_sequence s4_gp0_init_regs[] = {
+	{ .reg = ANACTRL_GP0PLL_CTRL1,	.def = 0x00000000 },
+	{ .reg = ANACTRL_GP0PLL_CTRL2,	.def = 0x00000000 },
+	{ .reg = ANACTRL_GP0PLL_CTRL3,	.def = 0x48681c00 },
+	{ .reg = ANACTRL_GP0PLL_CTRL4,	.def = 0x88770290 },
+	{ .reg = ANACTRL_GP0PLL_CTRL5,	.def = 0x39272000 },
+	{ .reg = ANACTRL_GP0PLL_CTRL6,	.def = 0x56540000 }
+};
+
+static struct clk_regmap s4_gp0_pll_dco = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_GP0PLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_GP0PLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_GP0PLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_GP0PLL_CTRL1,
+			.shift   = 0,
+			.width   = 17,
+		},
+		.l = {
+			.reg_off = ANACTRL_GP0PLL_CTRL0,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.rst = {
+			.reg_off = ANACTRL_GP0PLL_CTRL0,
+			.shift   = 29,
+			.width   = 1,
+		},
+		.range = &s4_gp0_pll_mult_range,
+		.init_regs = s4_gp0_init_regs,
+		.init_count = ARRAY_SIZE(s4_gp0_init_regs),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "gp0_pll_dco",
+		.ops = &meson_clk_pll_ops,
+		.parent_data = (const struct clk_parent_data []) {
+			{ .fw_name = "xtal", }
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_gp0_pll = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = ANACTRL_GP0PLL_CTRL0,
+		.shift = 16,
+		.width = 3,
+		.flags = (CLK_DIVIDER_POWER_OF_TWO |
+			  CLK_DIVIDER_ROUND_CLOSEST),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "gp0_pll",
+		.ops = &clk_regmap_divider_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_gp0_pll_dco.hw
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+/*
+ * Internal hifi pll emulation configuration parameters
+ */
+static const struct reg_sequence s4_hifi_init_regs[] = {
+	{ .reg = ANACTRL_HIFIPLL_CTRL1,	.def = 0x00010e56 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL2,	.def = 0x00000000 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL3,	.def = 0x6a285c00 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL4,	.def = 0x65771290 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL5,	.def = 0x39272000 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL6,	.def = 0x56540000 }
+};
+
+static struct clk_regmap s4_hifi_pll_dco = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL1,
+			.shift   = 0,
+			.width   = 17,
+		},
+		.l = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.rst = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 29,
+			.width   = 1,
+		},
+		.range = &s4_gp0_pll_mult_range,
+		.init_regs = s4_hifi_init_regs,
+		.init_count = ARRAY_SIZE(s4_hifi_init_regs),
+		.flags = CLK_MESON_PLL_ROUND_CLOSEST,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "hifi_pll_dco",
+		.ops = &meson_clk_pll_ops,
+		.parent_data = (const struct clk_parent_data []) {
+			{ .fw_name = "xtal", }
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_hifi_pll = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = ANACTRL_HIFIPLL_CTRL0,
+		.shift = 16,
+		.width = 2,
+		.flags = (CLK_DIVIDER_POWER_OF_TWO |
+			  CLK_DIVIDER_ROUND_CLOSEST),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "hifi_pll",
+		.ops = &clk_regmap_divider_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_hifi_pll_dco.hw
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap s4_hdmi_pll_dco = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_HDMIPLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_HDMIPLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_HDMIPLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_HDMIPLL_CTRL1,
+			.shift   = 0,
+			.width   = 17,
+		},
+		.l = {
+			.reg_off = ANACTRL_HDMIPLL_CTRL0,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.rst = {
+			.reg_off = ANACTRL_HDMIPLL_CTRL0,
+			.shift   = 29,
+			.width   = 1,
+		},
+		.range = &s4_gp0_pll_mult_range,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "hdmi_pll_dco",
+		.ops = &meson_clk_pll_ops,
+		.parent_data = (const struct clk_parent_data []) {
+			{ .fw_name = "xtal", }
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_hdmi_pll_od = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = ANACTRL_HDMIPLL_CTRL0,
+		.shift = 16,
+		.width = 4,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "hdmi_pll_od",
+		.ops = &clk_regmap_divider_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_hdmi_pll_dco.hw
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap s4_hdmi_pll = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = ANACTRL_HDMIPLL_CTRL0,
+		.shift = 20,
+		.width = 2,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "hdmi_pll",
+		.ops = &clk_regmap_divider_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_hdmi_pll_od.hw
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_fixed_factor s4_mpll_50m_div = {
+	.mult = 1,
+	.div = 80,
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll_50m_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fixed_pll_dco.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_mpll_50m = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = ANACTRL_FIXPLL_CTRL3,
+		.mask = 0x1,
+		.shift = 5,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll_50m",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_data = (const struct clk_parent_data []) {
+			{ .fw_name = "xtal", },
+			{ .hw = &s4_mpll_50m_div.hw },
+		},
+		.num_parents = 2,
+	},
+};
+
+static struct clk_fixed_factor s4_mpll_prediv = {
+	.mult = 1,
+	.div = 2,
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll_prediv",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_fixed_pll_dco.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static const struct reg_sequence s4_mpll0_init_regs[] = {
+	{ .reg = ANACTRL_MPLL_CTRL2, .def = 0x40000033 }
+};
+
+static struct clk_regmap s4_mpll0_div = {
+	.data = &(struct meson_clk_mpll_data){
+		.sdm = {
+			.reg_off = ANACTRL_MPLL_CTRL1,
+			.shift   = 0,
+			.width   = 14,
+		},
+		.sdm_en = {
+			.reg_off = ANACTRL_MPLL_CTRL1,
+			.shift   = 30,
+			.width	 = 1,
+		},
+		.n2 = {
+			.reg_off = ANACTRL_MPLL_CTRL1,
+			.shift   = 20,
+			.width   = 9,
+		},
+		.ssen = {
+			.reg_off = ANACTRL_MPLL_CTRL1,
+			.shift   = 29,
+			.width	 = 1,
+		},
+		.lock = &meson_clk_lock,
+		.init_regs = s4_mpll0_init_regs,
+		.init_count = ARRAY_SIZE(s4_mpll0_init_regs),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll0_div",
+		.ops = &meson_clk_mpll_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_mpll_prediv.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_mpll0 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_MPLL_CTRL1,
+		.bit_idx = 31,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll0",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_mpll0_div.hw },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static const struct reg_sequence s4_mpll1_init_regs[] = {
+	{ .reg = ANACTRL_MPLL_CTRL4,	.def = 0x40000033 }
+};
+
+static struct clk_regmap s4_mpll1_div = {
+	.data = &(struct meson_clk_mpll_data){
+		.sdm = {
+			.reg_off = ANACTRL_MPLL_CTRL3,
+			.shift   = 0,
+			.width   = 14,
+		},
+		.sdm_en = {
+			.reg_off = ANACTRL_MPLL_CTRL3,
+			.shift   = 30,
+			.width	 = 1,
+		},
+		.n2 = {
+			.reg_off = ANACTRL_MPLL_CTRL3,
+			.shift   = 20,
+			.width   = 9,
+		},
+		.ssen = {
+			.reg_off = ANACTRL_MPLL_CTRL3,
+			.shift   = 29,
+			.width	 = 1,
+		},
+		.lock = &meson_clk_lock,
+		.init_regs = s4_mpll1_init_regs,
+		.init_count = ARRAY_SIZE(s4_mpll1_init_regs),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll1_div",
+		.ops = &meson_clk_mpll_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_mpll_prediv.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_mpll1 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_MPLL_CTRL3,
+		.bit_idx = 31,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll1",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_mpll1_div.hw },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static const struct reg_sequence s4_mpll2_init_regs[] = {
+	{ .reg = ANACTRL_MPLL_CTRL6, .def = 0x40000033 }
+};
+
+static struct clk_regmap s4_mpll2_div = {
+	.data = &(struct meson_clk_mpll_data){
+		.sdm = {
+			.reg_off = ANACTRL_MPLL_CTRL5,
+			.shift   = 0,
+			.width   = 14,
+		},
+		.sdm_en = {
+			.reg_off = ANACTRL_MPLL_CTRL5,
+			.shift   = 30,
+			.width	 = 1,
+		},
+		.n2 = {
+			.reg_off = ANACTRL_MPLL_CTRL5,
+			.shift   = 20,
+			.width   = 9,
+		},
+		.ssen = {
+			.reg_off = ANACTRL_MPLL_CTRL5,
+			.shift   = 29,
+			.width	 = 1,
+		},
+		.lock = &meson_clk_lock,
+		.init_regs = s4_mpll2_init_regs,
+		.init_count = ARRAY_SIZE(s4_mpll2_init_regs),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll2_div",
+		.ops = &meson_clk_mpll_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_mpll_prediv.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_mpll2 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_MPLL_CTRL5,
+		.bit_idx = 31,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll2",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_mpll2_div.hw },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static const struct reg_sequence s4_mpll3_init_regs[] = {
+	{ .reg = ANACTRL_MPLL_CTRL8, .def = 0x40000033 }
+};
+
+static struct clk_regmap s4_mpll3_div = {
+	.data = &(struct meson_clk_mpll_data){
+		.sdm = {
+			.reg_off = ANACTRL_MPLL_CTRL7,
+			.shift   = 0,
+			.width   = 14,
+		},
+		.sdm_en = {
+			.reg_off = ANACTRL_MPLL_CTRL7,
+			.shift   = 30,
+			.width	 = 1,
+		},
+		.n2 = {
+			.reg_off = ANACTRL_MPLL_CTRL7,
+			.shift   = 20,
+			.width   = 9,
+		},
+		.ssen = {
+			.reg_off = ANACTRL_MPLL_CTRL7,
+			.shift   = 29,
+			.width	 = 1,
+		},
+		.lock = &meson_clk_lock,
+		.init_regs = s4_mpll3_init_regs,
+		.init_count = ARRAY_SIZE(s4_mpll3_init_regs),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll3_div",
+		.ops = &meson_clk_mpll_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&s4_mpll_prediv.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap s4_mpll3 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_MPLL_CTRL7,
+		.bit_idx = 31,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll3",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) { &s4_mpll3_div.hw },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+/* Array of all clocks provided by this provider */
+static struct clk_hw_onecell_data s4_pll_hw_onecell_data = {
+	.hws = {
+		[CLKID_FIXED_PLL_DCO]		= &s4_fixed_pll_dco.hw,
+		[CLKID_FIXED_PLL]		= &s4_fixed_pll.hw,
+		[CLKID_FCLK_DIV2_DIV]		= &s4_fclk_div2_div.hw,
+		[CLKID_FCLK_DIV2]		= &s4_fclk_div2.hw,
+		[CLKID_FCLK_DIV3_DIV]		= &s4_fclk_div3_div.hw,
+		[CLKID_FCLK_DIV3]		= &s4_fclk_div3.hw,
+		[CLKID_FCLK_DIV4_DIV]		= &s4_fclk_div4_div.hw,
+		[CLKID_FCLK_DIV4]		= &s4_fclk_div4.hw,
+		[CLKID_FCLK_DIV5_DIV]		= &s4_fclk_div5_div.hw,
+		[CLKID_FCLK_DIV5]		= &s4_fclk_div5.hw,
+		[CLKID_FCLK_DIV7_DIV]		= &s4_fclk_div7_div.hw,
+		[CLKID_FCLK_DIV7]		= &s4_fclk_div7.hw,
+		[CLKID_FCLK_DIV2P5_DIV]		= &s4_fclk_div2p5_div.hw,
+		[CLKID_FCLK_DIV2P5]		= &s4_fclk_div2p5.hw,
+		[CLKID_GP0_PLL_DCO]		= &s4_gp0_pll_dco.hw,
+		[CLKID_GP0_PLL]			= &s4_gp0_pll.hw,
+		[CLKID_HIFI_PLL_DCO]		= &s4_hifi_pll_dco.hw,
+		[CLKID_HIFI_PLL]		= &s4_hifi_pll.hw,
+		[CLKID_HDMI_PLL_DCO]		= &s4_hdmi_pll_dco.hw,
+		[CLKID_HDMI_PLL_OD]		= &s4_hdmi_pll_od.hw,
+		[CLKID_HDMI_PLL]		= &s4_hdmi_pll.hw,
+		[CLKID_MPLL_50M_DIV]		= &s4_mpll_50m_div.hw,
+		[CLKID_MPLL_50M]		= &s4_mpll_50m.hw,
+		[CLKID_MPLL_PREDIV]		= &s4_mpll_prediv.hw,
+		[CLKID_MPLL0_DIV]		= &s4_mpll0_div.hw,
+		[CLKID_MPLL0]			= &s4_mpll0.hw,
+		[CLKID_MPLL1_DIV]		= &s4_mpll1_div.hw,
+		[CLKID_MPLL1]			= &s4_mpll1.hw,
+		[CLKID_MPLL2_DIV]		= &s4_mpll2_div.hw,
+		[CLKID_MPLL2]			= &s4_mpll2.hw,
+		[CLKID_MPLL3_DIV]		= &s4_mpll3_div.hw,
+		[CLKID_MPLL3]			= &s4_mpll3.hw,
+		[NR_PLL_CLKS]			= NULL
+	},
+	.num = NR_PLL_CLKS,
+};
+
+static struct clk_regmap *const s4_pll_clk_regmaps[] = {
+	&s4_fixed_pll_dco,
+	&s4_fixed_pll,
+	&s4_fclk_div2,
+	&s4_fclk_div3,
+	&s4_fclk_div4,
+	&s4_fclk_div5,
+	&s4_fclk_div7,
+	&s4_fclk_div2p5,
+	&s4_gp0_pll_dco,
+	&s4_gp0_pll,
+	&s4_hifi_pll_dco,
+	&s4_hifi_pll,
+	&s4_hdmi_pll_dco,
+	&s4_hdmi_pll_od,
+	&s4_hdmi_pll,
+	&s4_mpll_50m,
+	&s4_mpll0_div,
+	&s4_mpll0,
+	&s4_mpll1_div,
+	&s4_mpll1,
+	&s4_mpll2_div,
+	&s4_mpll2,
+	&s4_mpll3_div,
+	&s4_mpll3,
+};
+
+static const struct reg_sequence s4_init_regs[] = {
+	{ .reg = ANACTRL_MPLL_CTRL0,	.def = 0x00000543 },
+};
+
+static struct regmap_config clkc_regmap_config = {
+	.reg_bits       = 32,
+	.val_bits       = 32,
+	.reg_stride     = 4,
+};
+
+static int meson_s4_pll_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	void __iomem *base;
+	int ret, i;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ret = regmap_multi_reg_write(regmap, s4_init_regs, ARRAY_SIZE(s4_init_regs));
+	if (ret) {
+		dev_err(dev, "Failed to init registers\n");
+		return ret;
+	}
+
+	/* Populate regmap for the regmap backed clocks */
+	for (i = 0; i < ARRAY_SIZE(s4_pll_clk_regmaps); i++)
+		s4_pll_clk_regmaps[i]->map = regmap;
+
+	for (i = 0; i < s4_pll_hw_onecell_data.num; i++) {
+		/* array might be sparse */
+		if (!s4_pll_hw_onecell_data.hws[i])
+			continue;
+
+		ret = devm_clk_hw_register(dev, s4_pll_hw_onecell_data.hws[i]);
+		if (ret) {
+			dev_err(dev, "Clock registration failed\n");
+			return ret;
+		}
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   &s4_pll_hw_onecell_data);
+}
+
+static const struct of_device_id clkc_match_table[] = {
+	{
+		.compatible = "amlogic,s4-pll-clkc",
+	},
+	{}
+};
+
+static struct platform_driver s4_driver = {
+	.probe		= meson_s4_pll_probe,
+	.driver		= {
+		.name	= "s4-pll-clkc",
+		.of_match_table = clkc_match_table,
+	},
+};
+
+module_platform_driver(s4_driver);
+MODULE_LICENSE("GPL");
diff --git a/drivers/clk/meson/s4-pll.h b/drivers/clk/meson/s4-pll.h
new file mode 100644
index 000000000000..332f2d7b94b4
--- /dev/null
+++ b/drivers/clk/meson/s4-pll.h
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, inc.
+ * Author: Yu Tu <yu.tu@amlogic.com>
+ */
+
+#ifndef __MESON_S4_PLL_H__
+#define __MESON_S4_PLL_H__
+
+/* ANA_CTRL - Registers
+ * REG_BASE:  REGISTER_BASE_ADDR = 0xfe008000
+ */
+#define ANACTRL_FIXPLL_CTRL0                       0x040
+#define ANACTRL_FIXPLL_CTRL1                       0x044
+#define ANACTRL_FIXPLL_CTRL2                       0x048
+#define ANACTRL_FIXPLL_CTRL3                       0x04c
+#define ANACTRL_FIXPLL_CTRL4                       0x050
+#define ANACTRL_FIXPLL_CTRL5                       0x054
+#define ANACTRL_FIXPLL_CTRL6                       0x058
+#define ANACTRL_FIXPLL_STS                         0x05c
+#define ANACTRL_GP0PLL_CTRL0                       0x080
+#define ANACTRL_GP0PLL_CTRL1                       0x084
+#define ANACTRL_GP0PLL_CTRL2                       0x088
+#define ANACTRL_GP0PLL_CTRL3                       0x08c
+#define ANACTRL_GP0PLL_CTRL4                       0x090
+#define ANACTRL_GP0PLL_CTRL5                       0x094
+#define ANACTRL_GP0PLL_CTRL6                       0x098
+#define ANACTRL_GP0PLL_STS                         0x09c
+#define ANACTRL_HIFIPLL_CTRL0                      0x100
+#define ANACTRL_HIFIPLL_CTRL1                      0x104
+#define ANACTRL_HIFIPLL_CTRL2                      0x108
+#define ANACTRL_HIFIPLL_CTRL3                      0x10c
+#define ANACTRL_HIFIPLL_CTRL4                      0x110
+#define ANACTRL_HIFIPLL_CTRL5                      0x114
+#define ANACTRL_HIFIPLL_CTRL6                      0x118
+#define ANACTRL_HIFIPLL_STS                        0x11c
+#define ANACTRL_MPLL_CTRL0                         0x180
+#define ANACTRL_MPLL_CTRL1                         0x184
+#define ANACTRL_MPLL_CTRL2                         0x188
+#define ANACTRL_MPLL_CTRL3                         0x18c
+#define ANACTRL_MPLL_CTRL4                         0x190
+#define ANACTRL_MPLL_CTRL5                         0x194
+#define ANACTRL_MPLL_CTRL6                         0x198
+#define ANACTRL_MPLL_CTRL7                         0x19c
+#define ANACTRL_MPLL_CTRL8                         0x1a0
+#define ANACTRL_MPLL_STS                           0x1a4
+#define ANACTRL_HDMIPLL_CTRL0                      0x1c0
+#define ANACTRL_HDMIPLL_CTRL1                      0x1c4
+#define ANACTRL_HDMIPLL_CTRL2                      0x1c8
+#define ANACTRL_HDMIPLL_CTRL3                      0x1cc
+#define ANACTRL_HDMIPLL_CTRL4                      0x1d0
+#define ANACTRL_HDMIPLL_CTRL5                      0x1d4
+#define ANACTRL_HDMIPLL_CTRL6                      0x1d8
+#define ANACTRL_HDMIPLL_STS                        0x1dc
+#define ANACTRL_HDMIPLL_VLOCK                      0x1e4
+
+/*
+ * CLKID index values
+ *
+ * These indices are entirely contrived and do not map onto the hardware.
+ * It has now been decided to expose everything by default in the DT header:
+ * include/dt-bindings/clock/axg-clkc.h. Only the clocks ids we don't want
+ * to expose, such as the internal muxes and dividers of composite clocks,
+ * will remain defined here.
+ */
+#define CLKID_FIXED_PLL_DCO		0
+#define CLKID_FCLK_DIV2_DIV		2
+#define CLKID_FCLK_DIV3_DIV		4
+#define CLKID_FCLK_DIV4_DIV		6
+#define CLKID_FCLK_DIV5_DIV		8
+#define CLKID_FCLK_DIV7_DIV		10
+#define CLKID_FCLK_DIV2P5_DIV		12
+#define CLKID_GP0_PLL_DCO		14
+#define CLKID_HIFI_PLL_DCO		16
+#define CLKID_HDMI_PLL_DCO		18
+#define CLKID_HDMI_PLL_OD		19
+#define CLKID_MPLL_50M_DIV		21
+#define CLKID_MPLL_PREDIV		23
+#define CLKID_MPLL0_DIV			24
+#define CLKID_MPLL1_DIV			26
+#define CLKID_MPLL2_DIV			28
+#define CLKID_MPLL3_DIV			30
+
+#define NR_PLL_CLKS			32
+/* include the CLKIDs that have been made part of the DT binding */
+#include <dt-bindings/clock/amlogic,s4-pll-clkc.h>
+
+#endif /* __MESON_S4_PLL_H__ */
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Krzysztof Kozlowski]

On 16/01/2023 08:42, Yu Tu wrote:
> Add the S4 PLL & peripheral clock controller dt-bindings in the s4 SoC
> family.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>  .../clock/amlogic,s4-peripherals-clkc.yaml    | 104 ++++++++++++++
>  .../bindings/clock/amlogic,s4-pll-clkc.yaml   |  50 +++++++
>  MAINTAINERS                                   |   1 +
>  .../clock/amlogic,s4-peripherals-clkc.h       | 131 ++++++++++++++++++
>  .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |  30 ++++
>  5 files changed, 316 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>  create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
> new file mode 100644
> index 000000000000..2deeff497754
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
> @@ -0,0 +1,104 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,s4-peripherals-clkc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic Meson S serials Peripherals Clock Controller
> +
> +maintainers:
> +  - Neil Armstrong <neil.armstrong@linaro.org>
> +  - Jerome Brunet <jbrunet@baylibre.com>
> +  - Yu Tu <yu.tu@amlogic.com>
> +
> +properties:
> +  compatible:
> +    const: amlogic,s4-peripherals-clkc
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: input fixed pll div2
> +      - description: input fixed pll div2p5
> +      - description: input fixed pll div3
> +      - description: input fixed pll div4
> +      - description: input fixed pll div5
> +      - description: input fixed pll div7
> +      - description: input hifi pll
> +      - description: input gp0 pll
> +      - description: input mpll0
> +      - description: input mpll1
> +      - description: input mpll2
> +      - description: input mpll3
> +      - description: input hdmi pll
> +      - description: input oscillator (usually at 24MHz)
> +      - description: input external 32kHz reference (optional)
> +
> +  clock-names:
> +    items:
> +      - const: fclk_div2
> +      - const: fclk_div2p5
> +      - const: fclk_div3
> +      - const: fclk_div4
> +      - const: fclk_div5
> +      - const: fclk_div7
> +      - const: hifi_pll
> +      - const: gp0_pll
> +      - const: mpll0
> +      - const: mpll1
> +      - const: mpll2
> +      - const: mpll3
> +      - const: hdmi_pll
> +      - const: xtal
> +      - const: ext_32k
> +
> +  "#clock-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - "#clock-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h>
> +
> +    /* 32KHz reference crystal */
> +    ext_32k: ref32k {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-frequency = <32000>;
> +    };

This wasn't here before. Drop it. It is trivial and it is not needed to
illustrate your device bindings. All clock bindings use it...

> +
> +    clkc_periphs: clock-controller@fe000000 {
> +      compatible = "amlogic,s4-peripherals-clkc";
> +      reg = <0xfe000000 0x49c>;
> +      clocks = <&clkc_pll 3>,
> +              <&clkc_pll 13>,
> +              <&clkc_pll 5>,
> +              <&clkc_pll 7>,
> +              <&clkc_pll 9>,
> +              <&clkc_pll 11>,
> +              <&clkc_pll 17>,
> +              <&clkc_pll 15>,
> +              <&clkc_pll 25>,
> +              <&clkc_pll 27>,
> +              <&clkc_pll 29>,
> +              <&clkc_pll 31>,
> +              <&clkc_pll 20>,
> +              <&xtal>,
> +              <&ext_32k>;
> +      clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", "fclk_div4",
> +                    "fclk_div5", "fclk_div7", "hifi_pll", "gp0_pll",
> +                    "mpll0", "mpll1", "mpll2", "mpll3", "hdmi_pll", "xtal",
> +                    "ext_32k";
> +      #clock-cells = <1>;
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
> new file mode 100644
> index 000000000000..aeda4861cebe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic Meson S serials PLL Clock Controller
> +
> +maintainers:
> +  - Neil Armstrong <neil.armstrong@linaro.org>
> +  - Jerome Brunet <jbrunet@baylibre.com>
> +  - Yu Tu <yu.tu@amlogic.com>
> +
> +properties:
> +  compatible:
> +    const: amlogic,s4-pll-clkc
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xtal
> +
> +  "#clock-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - "#clock-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clkc_pll: clock-controller@fe008000 {
> +      compatible = "amlogic,s4-pll-clkc";
> +      reg = <0xfe008000 0x1e8>;
> +      clocks = <&xtal>;
> +      clock-names = "xtal";
> +      #clock-cells = <1>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f61eb221415b..26c82beeffda 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1897,6 +1897,7 @@ L:	linux-amlogic@lists.infradead.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/clock/amlogic*
>  F:	drivers/clk/meson/
> +F:	include/dt-bindings/clock/amlogic*
>  F:	include/dt-bindings/clock/gxbb*
>  F:	include/dt-bindings/clock/meson*
>  
> diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
> new file mode 100644
> index 000000000000..bbec5094d5c3
> --- /dev/null
> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */

Unusual license... are you sure to license the bindings under GPLv4 or
GPLv5? Fine by me.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Yu Tu]

Hi Krzysztof,
	Thank you for your quick reply.

On 2023/1/16 16:29, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
> 
> On 16/01/2023 08:42, Yu Tu wrote:
>> Add the S4 PLL & peripheral clock controller dt-bindings in the s4 SoC
>> family.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   .../clock/amlogic,s4-peripherals-clkc.yaml    | 104 ++++++++++++++
>>   .../bindings/clock/amlogic,s4-pll-clkc.yaml   |  50 +++++++
>>   MAINTAINERS                                   |   1 +
>>   .../clock/amlogic,s4-peripherals-clkc.h       | 131 ++++++++++++++++++
>>   .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |  30 ++++
>>   5 files changed, 316 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>   create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>   create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>> new file mode 100644
>> index 000000000000..2deeff497754
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>> @@ -0,0 +1,104 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-peripherals-clkc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic Meson S serials Peripherals Clock Controller
>> +
>> +maintainers:
>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>> +  - Jerome Brunet <jbrunet@baylibre.com>
>> +  - Yu Tu <yu.tu@amlogic.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: amlogic,s4-peripherals-clkc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: input fixed pll div2
>> +      - description: input fixed pll div2p5
>> +      - description: input fixed pll div3
>> +      - description: input fixed pll div4
>> +      - description: input fixed pll div5
>> +      - description: input fixed pll div7
>> +      - description: input hifi pll
>> +      - description: input gp0 pll
>> +      - description: input mpll0
>> +      - description: input mpll1
>> +      - description: input mpll2
>> +      - description: input mpll3
>> +      - description: input hdmi pll
>> +      - description: input oscillator (usually at 24MHz)
>> +      - description: input external 32kHz reference (optional)
>> +
>> +  clock-names:
>> +    items:
>> +      - const: fclk_div2
>> +      - const: fclk_div2p5
>> +      - const: fclk_div3
>> +      - const: fclk_div4
>> +      - const: fclk_div5
>> +      - const: fclk_div7
>> +      - const: hifi_pll
>> +      - const: gp0_pll
>> +      - const: mpll0
>> +      - const: mpll1
>> +      - const: mpll2
>> +      - const: mpll3
>> +      - const: hdmi_pll
>> +      - const: xtal
>> +      - const: ext_32k
>> +
>> +  "#clock-cells":
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - "#clock-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h>
>> +
>> +    /* 32KHz reference crystal */
>> +    ext_32k: ref32k {
>> +        compatible = "fixed-clock";
>> +        #clock-cells = <0>;
>> +        clock-frequency = <32000>;
>> +    };
> 
> This wasn't here before. Drop it. It is trivial and it is not needed to
> illustrate your device bindings. All clock bindings use it...
> 

I'm fine with that. I don't know if Jerome agrees with that. Wait for 
him. See what he says.

>> +
>> +    clkc_periphs: clock-controller@fe000000 {
>> +      compatible = "amlogic,s4-peripherals-clkc";
>> +      reg = <0xfe000000 0x49c>;
>> +      clocks = <&clkc_pll 3>,
>> +              <&clkc_pll 13>,
>> +              <&clkc_pll 5>,
>> +              <&clkc_pll 7>,
>> +              <&clkc_pll 9>,
>> +              <&clkc_pll 11>,
>> +              <&clkc_pll 17>,
>> +              <&clkc_pll 15>,
>> +              <&clkc_pll 25>,
>> +              <&clkc_pll 27>,
>> +              <&clkc_pll 29>,
>> +              <&clkc_pll 31>,
>> +              <&clkc_pll 20>,
>> +              <&xtal>,
>> +              <&ext_32k>;
>> +      clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", "fclk_div4",
>> +                    "fclk_div5", "fclk_div7", "hifi_pll", "gp0_pll",
>> +                    "mpll0", "mpll1", "mpll2", "mpll3", "hdmi_pll", "xtal",
>> +                    "ext_32k";
>> +      #clock-cells = <1>;
>> +    };
>> +...
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>> new file mode 100644
>> index 000000000000..aeda4861cebe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic Meson S serials PLL Clock Controller
>> +
>> +maintainers:
>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>> +  - Jerome Brunet <jbrunet@baylibre.com>
>> +  - Yu Tu <yu.tu@amlogic.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: amlogic,s4-pll-clkc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xtal
>> +
>> +  "#clock-cells":
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - "#clock-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    clkc_pll: clock-controller@fe008000 {
>> +      compatible = "amlogic,s4-pll-clkc";
>> +      reg = <0xfe008000 0x1e8>;
>> +      clocks = <&xtal>;
>> +      clock-names = "xtal";
>> +      #clock-cells = <1>;
>> +    };
>> +
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f61eb221415b..26c82beeffda 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1897,6 +1897,7 @@ L:	linux-amlogic@lists.infradead.org
>>   S:	Maintained
>>   F:	Documentation/devicetree/bindings/clock/amlogic*
>>   F:	drivers/clk/meson/
>> +F:	include/dt-bindings/clock/amlogic*
>>   F:	include/dt-bindings/clock/gxbb*
>>   F:	include/dt-bindings/clock/meson*
>>   
>> diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>> new file mode 100644
>> index 000000000000..bbec5094d5c3
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>> @@ -0,0 +1,131 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> 
> Unusual license... are you sure to license the bindings under GPLv4 or
> GPLv5? Fine by me.
> 

Yes.

> Best regards,
> Krzysztof
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Kevin Hilman]

Yu Tu <yu.tu@amlogic.com> writes:

> On 2023/1/16 16:29, Krzysztof Kozlowski wrote:

[...]

>>> diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>> new file mode 100644
>>> index 000000000000..bbec5094d5c3
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>> @@ -0,0 +1,131 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> 
>> Unusual license... are you sure to license the bindings under GPLv4 or
>> GPLv5? Fine by me.
>> 
>
> Yes.

The rest of the bindings for Amlogic SoCs are GPL-2.0 (without the '+').
Adding the dual-license for MIT seems fine, but adding the '+' is
curious.

It would be helpful if you could please explain why you'd like these
bindings to be licensed differently than the rest of the SoC family.

Kevin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 0/3] Add S4 SoC PLL and Peripheral clock controller

[Jerome Brunet]

On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:

> 1. Add S4 SoC PLL and Peripheral clock controller dt-bindings.
> 2. Add PLL and Peripheral clock controller driver for S4 SOC.
>
> Yu Tu (3):
>   dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock
>     controller
>   clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver
>   clk: meson: s4: add support for Amlogic S4 SoC peripheral clock
>     controller

You are adding 2 controller driver with this series.
Making 2 driver patches on clk/ is good. Please do the same for the bindings

>
> V5 -> V6: Change send patch series, as well change format and clock flags.
> V4 -> V5: change format and clock flags and adjust the patch series as suggested
> by Jerome.
> V3 -> V4: change format and clock flags.
> V2 -> V3: Use two clock controller.
> V1 -> V2: Change format as discussed in the email.
>
> Link:https://lore.kernel.org/all/20221123021346.18136-1-yu.tu@amlogic.com/
>
>  .../clock/amlogic,s4-peripherals-clkc.yaml    |  104 +
>  .../bindings/clock/amlogic,s4-pll-clkc.yaml   |   50 +
>  MAINTAINERS                                   |    1 +
>  drivers/clk/meson/Kconfig                     |   25 +
>  drivers/clk/meson/Makefile                    |    2 +
>  drivers/clk/meson/s4-peripherals.c            | 3874 +++++++++++++++++
>  drivers/clk/meson/s4-peripherals.h            |  218 +
>  drivers/clk/meson/s4-pll.c                    |  875 ++++
>  drivers/clk/meson/s4-pll.h                    |   88 +
>  .../clock/amlogic,s4-peripherals-clkc.h       |  131 +
>  .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |   30 +
>  11 files changed, 5398 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>  create mode 100644 drivers/clk/meson/s4-peripherals.c
>  create mode 100644 drivers/clk/meson/s4-peripherals.h
>  create mode 100644 drivers/clk/meson/s4-pll.c
>  create mode 100644 drivers/clk/meson/s4-pll.h
>  create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>  create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
>
>
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 2/3] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver

[Jerome Brunet]

On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:

> Add the S4 PLL clock controller driver in the s4 SoC family.
>
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---

[...]

> +
> +static struct clk_regmap s4_fclk_div2 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = ANACTRL_FIXPLL_CTRL1,
> +		.bit_idx = 24,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div2",
> +		.ops = &clk_regmap_gate_ro_ops,

On the previous SoC, these fixed divider gate were not read-only.
They are marked as critical when necessary, with the appropriate
comment.

Why is it different on the s4 ?

> +		.parent_hws = (const struct clk_hw *[]) {
> +			&s4_fclk_div2_div.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +

[...]

> +#ifndef __MESON_S4_PLL_H__
> +#define __MESON_S4_PLL_H__
> +
> +/* ANA_CTRL - Registers
> + * REG_BASE:  REGISTER_BASE_ADDR = 0xfe008000

This multi-line comment style is wrong in clk/
REG_BASE is not used so I'm not sure this is useful

> + */
> +#define ANACTRL_FIXPLL_CTRL0                       0x040
> +#define ANACTRL_FIXPLL_CTRL1                       0x044
> +#define ANACTRL_FIXPLL_CTRL2                       0x048
> +#define ANACTRL_FIXPLL_CTRL3                       0x04c
> +#define ANACTRL_FIXPLL_CTRL4                       0x050
> +#define ANACTRL_FIXPLL_CTRL5                       0x054
> +#define ANACTRL_FIXPLL_CTRL6                       0x058
> +#define ANACTRL_FIXPLL_STS                         0x05c
> +#define ANACTRL_GP0PLL_CTRL0                       0x080
> +#define ANACTRL_GP0PLL_CTRL1                       0x084
> +#define ANACTRL_GP0PLL_CTRL2                       0x088
> +#define ANACTRL_GP0PLL_CTRL3                       0x08c
> +#define ANACTRL_GP0PLL_CTRL4                       0x090
> +#define ANACTRL_GP0PLL_CTRL5                       0x094
> +#define ANACTRL_GP0PLL_CTRL6                       0x098
> +#define ANACTRL_GP0PLL_STS                         0x09c
> +#define ANACTRL_HIFIPLL_CTRL0                      0x100
> +#define ANACTRL_HIFIPLL_CTRL1                      0x104
> +#define ANACTRL_HIFIPLL_CTRL2                      0x108
> +#define ANACTRL_HIFIPLL_CTRL3                      0x10c
> +#define ANACTRL_HIFIPLL_CTRL4                      0x110
> +#define ANACTRL_HIFIPLL_CTRL5                      0x114
> +#define ANACTRL_HIFIPLL_CTRL6                      0x118
> +#define ANACTRL_HIFIPLL_STS                        0x11c
> +#define ANACTRL_MPLL_CTRL0                         0x180
> +#define ANACTRL_MPLL_CTRL1                         0x184
> +#define ANACTRL_MPLL_CTRL2                         0x188
> +#define ANACTRL_MPLL_CTRL3                         0x18c
> +#define ANACTRL_MPLL_CTRL4                         0x190
> +#define ANACTRL_MPLL_CTRL5                         0x194
> +#define ANACTRL_MPLL_CTRL6                         0x198
> +#define ANACTRL_MPLL_CTRL7                         0x19c
> +#define ANACTRL_MPLL_CTRL8                         0x1a0
> +#define ANACTRL_MPLL_STS                           0x1a4
> +#define ANACTRL_HDMIPLL_CTRL0                      0x1c0
> +#define ANACTRL_HDMIPLL_CTRL1                      0x1c4
> +#define ANACTRL_HDMIPLL_CTRL2                      0x1c8
> +#define ANACTRL_HDMIPLL_CTRL3                      0x1cc
> +#define ANACTRL_HDMIPLL_CTRL4                      0x1d0
> +#define ANACTRL_HDMIPLL_CTRL5                      0x1d4
> +#define ANACTRL_HDMIPLL_CTRL6                      0x1d8
> +#define ANACTRL_HDMIPLL_STS                        0x1dc
> +#define ANACTRL_HDMIPLL_VLOCK                      0x1e4
> +
> +/*
> + * CLKID index values
> + *
> + * These indices are entirely contrived and do not map onto the hardware.
> + * It has now been decided to expose everything by default in the DT header:
> + * include/dt-bindings/clock/axg-clkc.h. Only the clocks ids we don't want
> + * to expose, such as the internal muxes and dividers of composite clocks,
> + * will remain defined here.
> + */
> +#define CLKID_FIXED_PLL_DCO		0
> +#define CLKID_FCLK_DIV2_DIV		2
> +#define CLKID_FCLK_DIV3_DIV		4
> +#define CLKID_FCLK_DIV4_DIV		6
> +#define CLKID_FCLK_DIV5_DIV		8
> +#define CLKID_FCLK_DIV7_DIV		10
> +#define CLKID_FCLK_DIV2P5_DIV		12
> +#define CLKID_GP0_PLL_DCO		14
> +#define CLKID_HIFI_PLL_DCO		16
> +#define CLKID_HDMI_PLL_DCO		18
> +#define CLKID_HDMI_PLL_OD		19
> +#define CLKID_MPLL_50M_DIV		21
> +#define CLKID_MPLL_PREDIV		23
> +#define CLKID_MPLL0_DIV			24
> +#define CLKID_MPLL1_DIV			26
> +#define CLKID_MPLL2_DIV			28
> +#define CLKID_MPLL3_DIV			30
> +
> +#define NR_PLL_CLKS			32
> +/* include the CLKIDs that have been made part of the DT binding */
> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h>
> +
> +#endif /* __MESON_S4_PLL_H__ */


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Jerome Brunet]

On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:

> Add the peripherals clock controller driver in the s4 SoC family.
>
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>

[...]

> +
> +/* Video Clocks */
> +static struct clk_regmap s4_vid_pll_div = {
> +	.data = &(struct meson_vid_pll_div_data){
> +		.val = {
> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
> +			.shift   = 0,
> +			.width   = 15,
> +		},
> +		.sel = {
> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
> +			.shift   = 16,
> +			.width   = 2,
> +		},
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "vid_pll_div",
> +		/*
> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
> +		 * clock is the default value of this register. When designing the
> +		 * video module of the chip, a default value that can meet the
> +		 * requirements of the video module will be solidified according
> +		 * to the usage requirements of the chip, so as to facilitate chip
> +		 * simulation. So this is ro_ops.
> +		 * It is important to note that this clock is not used on this
> +		 * chip and is described only for the integrity of the clock tree.
> +		 */

If it is reset value and will be applicable to all the design, regarless
of the use-case, then yes RO ops is OK

From what I understand here, the value will depend on the use-case requirements.
This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.

> +		.ops = &meson_vid_pll_div_ro_ops,
> +		.parent_data = (const struct clk_parent_data []) {
> +			{ .fw_name = "hdmi_pll", }
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +

> +
> +/* VDEC clocks */
> +static const struct clk_parent_data s4_dec_parent_data[] = {
> +	{ .fw_name = "fclk_div2p5", },
> +	{ .fw_name = "fclk_div3", },
> +	{ .fw_name = "fclk_div4", },
> +	{ .fw_name = "fclk_div5", },
> +	{ .fw_name = "fclk_div7", },
> +	{ .fw_name = "hifi_pll", },
> +	{ .fw_name = "gp0_pll", },
> +	{ .fw_name = "xtal", }
> +};
> +
> +static struct clk_regmap s4_vdec_p0_mux = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = CLKCTRL_VDEC_CLK_CTRL,
> +		.mask = 0x7,
> +		.shift = 9,
> +		.flags = CLK_MUX_ROUND_CLOSEST,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "vdec_p0_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = s4_dec_parent_data,
> +		.num_parents = ARRAY_SIZE(s4_dec_parent_data),
> +		/*
> +		 * When the driver uses this clock, needs to specify the patent clock
> +		 * he wants in the dts.

s/patent/parent ?
s/he wants/it requires ?

> +		 */
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
> +	},
> +};
> +

[...]

> +static const struct clk_parent_data s4_vpu_clkc_parent_data[] = {
> +	{ .fw_name = "fclk_div4", },
> +	{ .fw_name = "fclk_div3", },
> +	{ .fw_name = "fclk_div5", },
> +	{ .fw_name = "fclk_div7", },
> +	{ .fw_name = "mpll1", },
> +	{ .hw = &s4_vid_pll.hw },
> +	{ .fw_name = "mpll2", },
> +	{ .fw_name = "gp0_pll", },
> +};
> +
> +static struct clk_regmap s4_vpu_clkc_p0_mux  = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = CLKCTRL_VPU_CLKC_CTRL,
> +		.mask = 0x7,
> +		.shift = 9,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "vpu_clkc_p0_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = s4_vpu_clkc_parent_data,
> +		.num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data),
> +		/*
> +		 * When the driver uses this clock, needs to specify the patent clock
> +		 * he wants in the dts.
> +		 */

That's quite a lot of occurences of the same comment.
At the same time, other clocks with the same flag have no comment.

Please make general comment, before the Video/VPU section, explaining
which clocks needs on a use-case basis (through DT) and possibly how it
should be set, what should drive the choices.

> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
> +	},
> +};
> +

> +
> +/* EMMC/NAND clock */
> +static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = {
> +	{ .fw_name = "xtal", },
> +	{ .fw_name = "fclk_div2", },
> +	{ .fw_name = "fclk_div3", },
> +	{ .fw_name = "hifi_pll", },
> +	{ .fw_name = "fclk_div2p5", },
> +	{ .fw_name = "mpll2", },
> +	{ .fw_name = "mpll3", },
> +	{ .fw_name = "gp0_pll", },
> +};
> +
> +static struct clk_regmap s4_sd_emmc_c_clk0_sel = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = CLKCTRL_NAND_CLK_CTRL,
> +		.mask = 0x7,
> +		.shift = 9,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "sd_emmc_c_clk0_sel",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = s4_sd_emmc_clk0_parent_data,
> +		.num_parents = ARRAY_SIZE(s4_sd_emmc_clk0_parent_data),
> +		/*
> +		 * When the driver uses this clock, needs to specify the patent clock
> +		 * he wants in the dts.
> +		 */

I'm getting a bit suspicious about the use (and abuse ...) of this flag.
I don't quite get how selecting the base PLL for MMC should be done on
use-case basis and should be up the board DT ...

Other SoC have all used fdiv2 so far. Do you expect this setting to be
part of the dtsi SoC file ?

> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
> +	},
> +};
> +

> +
> +/* SPICC Clock */
> +static const struct clk_parent_data s4_spicc_parent_data[] = {
> +	{ .fw_name = "xtal", },
> +	{ .hw = &s4_sys_clk.hw },
> +	{ .fw_name = "fclk_div4", },
> +	{ .fw_name = "fclk_div3", },
> +	{ .fw_name = "fclk_div2", },
> +	{ .fw_name = "fclk_div5", },
> +	{ .fw_name = "fclk_div7", },
> +};
> +
> +static struct clk_regmap s4_spicc0_mux = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = CLKCTRL_SPICC_CLK_CTRL,
> +		.mask = 0x7,
> +		.shift = 7,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "spicc0_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = s4_spicc_parent_data,
> +		.num_parents = ARRAY_SIZE(s4_spicc_parent_data),
> +		/*
> +		 * When the driver uses this clock, needs to specify the patent clock
> +		 * he wants in the dts.
> +		 */

This is getting too far. All the parent clocks are fixed.
Let CCF do the job of picking the most adequate clock for the job
instead of manually settings things

> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
> +	},
> +};
> +


> +
> +/* PWM Clock */
> +static const struct clk_parent_data s4_pwm_parent_data[] = {
> +	{ .fw_name = "xtal", },
> +	{ .hw = &s4_vid_pll.hw },
> +	{ .fw_name = "fclk_div4", },
> +	{ .fw_name = "fclk_div3", },
> +};
> +
> +static struct clk_regmap s4_pwm_a_mux = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CLKCTRL_PWM_CLK_AB_CTRL,
> +		.mask = 0x3,
> +		.shift = 9,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "pwm_a_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = s4_pwm_parent_data,
> +		.num_parents = ARRAY_SIZE(s4_pwm_parent_data),
> +		/*
> +		 * When the driver uses this clock, needs to specify the patent clock
> +		 * he wants in the dts.
> +		 */

Same here ... this is really going to far.

> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
> +	},
> +};
> +

> +
> +static struct clk_regmap s4_saradc_mux = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CLKCTRL_SAR_CLK_CTRL,
> +		.mask = 0x3,
> +		.shift = 9,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "saradc_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = (const struct clk_parent_data []) {
> +			{ .fw_name = "xtal", },
> +			{ .hw = &s4_sys_clk.hw },
> +		},
> +		.num_parents = 2,
> +		/*
> +		 * When the driver uses this clock, needs to specify the patent clock
> +		 * he wants in the dts.
> +		 */

For each clock type, if this flag is going to be used, I'd like a clear
explanation about why it is use-case dependent and why you need manual
control over this. Same applies to all the occurence.

> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
> +	},
> +};

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Yu Tu]

Hi Kevin,

On 2023/1/19 8:38, Kevin Hilman wrote:
> [ EXTERNAL EMAIL ]
> 
> Yu Tu <yu.tu@amlogic.com> writes:
> 
>> On 2023/1/16 16:29, Krzysztof Kozlowski wrote:
> 
> [...]
> 
>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>> new file mode 100644
>>>> index 000000000000..bbec5094d5c3
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>> @@ -0,0 +1,131 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>>
>>> Unusual license... are you sure to license the bindings under GPLv4 or
>>> GPLv5? Fine by me.
>>>
>>
>> Yes.
> 
> The rest of the bindings for Amlogic SoCs are GPL-2.0 (without the '+').
> Adding the dual-license for MIT seems fine, but adding the '+' is
> curious.
> 
> It would be helpful if you could please explain why you'd like these
> bindings to be licensed differently than the rest of the SoC family.
> 

I actually refer to the previous g12a Soc.
https://elixir.bootlin.com/linux/v6.2-rc4/source/include/dt-bindings/clock/g12a-clkc.h
https://elixir.bootlin.com/linux/v6.2-rc4/source/include/dt-bindings/clock/axg-clkc.h
[...]

So if you think it is not necessary, I will delete the '+' as you 
suggested. Don't know what you choose?

> Kevin
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 0/3] Add S4 SoC PLL and Peripheral clock controller

[Yu Tu]

Hi Jerome,

On 2023/1/19 19:18, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> 1. Add S4 SoC PLL and Peripheral clock controller dt-bindings.
>> 2. Add PLL and Peripheral clock controller driver for S4 SOC.
>>
>> Yu Tu (3):
>>    dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock
>>      controller
>>    clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver
>>    clk: meson: s4: add support for Amlogic S4 SoC peripheral clock
>>      controller
> 
> You are adding 2 controller driver with this series.
> Making 2 driver patches on clk/ is good. Please do the same for the bindings
> 
okay.I will change the next edition as you suggest.

Please check Krzysztof's opinion. Do you agree? Thank you!

>>
>> V5 -> V6: Change send patch series, as well change format and clock flags.
>> V4 -> V5: change format and clock flags and adjust the patch series as suggested
>> by Jerome.
>> V3 -> V4: change format and clock flags.
>> V2 -> V3: Use two clock controller.
>> V1 -> V2: Change format as discussed in the email.
>>
>> Link:https://lore.kernel.org/all/20221123021346.18136-1-yu.tu@amlogic.com/
>>
>>   .../clock/amlogic,s4-peripherals-clkc.yaml    |  104 +
>>   .../bindings/clock/amlogic,s4-pll-clkc.yaml   |   50 +
>>   MAINTAINERS                                   |    1 +
>>   drivers/clk/meson/Kconfig                     |   25 +
>>   drivers/clk/meson/Makefile                    |    2 +
>>   drivers/clk/meson/s4-peripherals.c            | 3874 +++++++++++++++++
>>   drivers/clk/meson/s4-peripherals.h            |  218 +
>>   drivers/clk/meson/s4-pll.c                    |  875 ++++
>>   drivers/clk/meson/s4-pll.h                    |   88 +
>>   .../clock/amlogic,s4-peripherals-clkc.h       |  131 +
>>   .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |   30 +
>>   11 files changed, 5398 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>   create mode 100644 drivers/clk/meson/s4-peripherals.c
>>   create mode 100644 drivers/clk/meson/s4-peripherals.h
>>   create mode 100644 drivers/clk/meson/s4-pll.c
>>   create mode 100644 drivers/clk/meson/s4-pll.h
>>   create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>   create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
>>
>>
>> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 2/3] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver

[Yu Tu]

Hi Jerome,

On 2023/1/19 19:20, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Add the S4 PLL clock controller driver in the s4 SoC family.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
> 
> [...]
> 
>> +
>> +static struct clk_regmap s4_fclk_div2 = {
>> +	.data = &(struct clk_regmap_gate_data){
>> +		.offset = ANACTRL_FIXPLL_CTRL1,
>> +		.bit_idx = 24,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "fclk_div2",
>> +		.ops = &clk_regmap_gate_ro_ops,
> 
> On the previous SoC, these fixed divider gate were not read-only.
> They are marked as critical when necessary, with the appropriate
> comment.
> 
> Why is it different on the s4 ?

In fact, this part of the SOC is no different from the previous G12a/b 
and so on.

I remember that my first version was made according to G12A, and I 
changed this way under your suggestion.

Maybe you were busy and forgot. For me, this mode and the previous g12a 
mode function is ok. I can do either. So now how do you decide to go 
that way?

> 
>> +		.parent_hws = (const struct clk_hw *[]) {
>> +			&s4_fclk_div2_div.hw
>> +		},
>> +		.num_parents = 1,
>> +	},
>> +};
>> +
> 
> [...]
> 
>> +#ifndef __MESON_S4_PLL_H__
>> +#define __MESON_S4_PLL_H__
>> +
>> +/* ANA_CTRL - Registers
>> + * REG_BASE:  REGISTER_BASE_ADDR = 0xfe008000
> 
> This multi-line comment style is wrong in clk/
> REG_BASE is not used so I'm not sure this is useful

I will remove REG_BASE and  change this format in next version.

> 
>> + */
>> +#define ANACTRL_FIXPLL_CTRL0                       0x040
>> +#define ANACTRL_FIXPLL_CTRL1                       0x044
>> +#define ANACTRL_FIXPLL_CTRL2                       0x048
>> +#define ANACTRL_FIXPLL_CTRL3                       0x04c
>> +#define ANACTRL_FIXPLL_CTRL4                       0x050
>> +#define ANACTRL_FIXPLL_CTRL5                       0x054
>> +#define ANACTRL_FIXPLL_CTRL6                       0x058
>> +#define ANACTRL_FIXPLL_STS                         0x05c
>> +#define ANACTRL_GP0PLL_CTRL0                       0x080
>> +#define ANACTRL_GP0PLL_CTRL1                       0x084
>> +#define ANACTRL_GP0PLL_CTRL2                       0x088
>> +#define ANACTRL_GP0PLL_CTRL3                       0x08c
>> +#define ANACTRL_GP0PLL_CTRL4                       0x090
>> +#define ANACTRL_GP0PLL_CTRL5                       0x094
>> +#define ANACTRL_GP0PLL_CTRL6                       0x098
>> +#define ANACTRL_GP0PLL_STS                         0x09c
>> +#define ANACTRL_HIFIPLL_CTRL0                      0x100
>> +#define ANACTRL_HIFIPLL_CTRL1                      0x104
>> +#define ANACTRL_HIFIPLL_CTRL2                      0x108
>> +#define ANACTRL_HIFIPLL_CTRL3                      0x10c
>> +#define ANACTRL_HIFIPLL_CTRL4                      0x110
>> +#define ANACTRL_HIFIPLL_CTRL5                      0x114
>> +#define ANACTRL_HIFIPLL_CTRL6                      0x118
>> +#define ANACTRL_HIFIPLL_STS                        0x11c
>> +#define ANACTRL_MPLL_CTRL0                         0x180
>> +#define ANACTRL_MPLL_CTRL1                         0x184
>> +#define ANACTRL_MPLL_CTRL2                         0x188
>> +#define ANACTRL_MPLL_CTRL3                         0x18c
>> +#define ANACTRL_MPLL_CTRL4                         0x190
>> +#define ANACTRL_MPLL_CTRL5                         0x194
>> +#define ANACTRL_MPLL_CTRL6                         0x198
>> +#define ANACTRL_MPLL_CTRL7                         0x19c
>> +#define ANACTRL_MPLL_CTRL8                         0x1a0
>> +#define ANACTRL_MPLL_STS                           0x1a4
>> +#define ANACTRL_HDMIPLL_CTRL0                      0x1c0
>> +#define ANACTRL_HDMIPLL_CTRL1                      0x1c4
>> +#define ANACTRL_HDMIPLL_CTRL2                      0x1c8
>> +#define ANACTRL_HDMIPLL_CTRL3                      0x1cc
>> +#define ANACTRL_HDMIPLL_CTRL4                      0x1d0
>> +#define ANACTRL_HDMIPLL_CTRL5                      0x1d4
>> +#define ANACTRL_HDMIPLL_CTRL6                      0x1d8
>> +#define ANACTRL_HDMIPLL_STS                        0x1dc
>> +#define ANACTRL_HDMIPLL_VLOCK                      0x1e4
>> +
>> +/*
>> + * CLKID index values
>> + *
>> + * These indices are entirely contrived and do not map onto the hardware.
>> + * It has now been decided to expose everything by default in the DT header:
>> + * include/dt-bindings/clock/axg-clkc.h. Only the clocks ids we don't want
>> + * to expose, such as the internal muxes and dividers of composite clocks,
>> + * will remain defined here.
>> + */
>> +#define CLKID_FIXED_PLL_DCO		0
>> +#define CLKID_FCLK_DIV2_DIV		2
>> +#define CLKID_FCLK_DIV3_DIV		4
>> +#define CLKID_FCLK_DIV4_DIV		6
>> +#define CLKID_FCLK_DIV5_DIV		8
>> +#define CLKID_FCLK_DIV7_DIV		10
>> +#define CLKID_FCLK_DIV2P5_DIV		12
>> +#define CLKID_GP0_PLL_DCO		14
>> +#define CLKID_HIFI_PLL_DCO		16
>> +#define CLKID_HDMI_PLL_DCO		18
>> +#define CLKID_HDMI_PLL_OD		19
>> +#define CLKID_MPLL_50M_DIV		21
>> +#define CLKID_MPLL_PREDIV		23
>> +#define CLKID_MPLL0_DIV			24
>> +#define CLKID_MPLL1_DIV			26
>> +#define CLKID_MPLL2_DIV			28
>> +#define CLKID_MPLL3_DIV			30
>> +
>> +#define NR_PLL_CLKS			32
>> +/* include the CLKIDs that have been made part of the DT binding */
>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h>
>> +
>> +#endif /* __MESON_S4_PLL_H__ */
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Yu Tu]

Hi
On 2023/1/19 19:37, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Add the peripherals clock controller driver in the s4 SoC family.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> 
> [...]
> 
>> +
>> +/* Video Clocks */
>> +static struct clk_regmap s4_vid_pll_div = {
>> +	.data = &(struct meson_vid_pll_div_data){
>> +		.val = {
>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>> +			.shift   = 0,
>> +			.width   = 15,
>> +		},
>> +		.sel = {
>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>> +			.shift   = 16,
>> +			.width   = 2,
>> +		},
>> +	},
>> +	.hw.init = &(struct clk_init_data) {
>> +		.name = "vid_pll_div",
>> +		/*
>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>> +		 * clock is the default value of this register. When designing the
>> +		 * video module of the chip, a default value that can meet the
>> +		 * requirements of the video module will be solidified according
>> +		 * to the usage requirements of the chip, so as to facilitate chip
>> +		 * simulation. So this is ro_ops.
>> +		 * It is important to note that this clock is not used on this
>> +		 * chip and is described only for the integrity of the clock tree.
>> +		 */
> 
> If it is reset value and will be applicable to all the design, regarless
> of the use-case, then yes RO ops is OK
> 
>>From what I understand here, the value will depend on the use-case requirements.
> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.

Check the previous chip history, the actual scene is not used at all, 
basically is used in simulation. So the previous SOC was "ro_ops" 
without any problems.  This S4 SOC is not actually useful either.

So when you were upstream, you had no problem making "ro_ops". I wonder 
if I could delete this useless clock, so you don't have to worry about it.

> 
>> +		.ops = &meson_vid_pll_div_ro_ops,
>> +		.parent_data = (const struct clk_parent_data []) {
>> +			{ .fw_name = "hdmi_pll", }
>> +		},
>> +		.num_parents = 1,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
> 
>> +
>> +/* VDEC clocks */
>> +static const struct clk_parent_data s4_dec_parent_data[] = {
>> +	{ .fw_name = "fclk_div2p5", },
>> +	{ .fw_name = "fclk_div3", },
>> +	{ .fw_name = "fclk_div4", },
>> +	{ .fw_name = "fclk_div5", },
>> +	{ .fw_name = "fclk_div7", },
>> +	{ .fw_name = "hifi_pll", },
>> +	{ .fw_name = "gp0_pll", },
>> +	{ .fw_name = "xtal", }
>> +};
>> +
>> +static struct clk_regmap s4_vdec_p0_mux = {
>> +	.data = &(struct clk_regmap_mux_data){
>> +		.offset = CLKCTRL_VDEC_CLK_CTRL,
>> +		.mask = 0x7,
>> +		.shift = 9,
>> +		.flags = CLK_MUX_ROUND_CLOSEST,
>> +	},
>> +	.hw.init = &(struct clk_init_data) {
>> +		.name = "vdec_p0_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_data = s4_dec_parent_data,
>> +		.num_parents = ARRAY_SIZE(s4_dec_parent_data),
>> +		/*
>> +		 * When the driver uses this clock, needs to specify the patent clock
>> +		 * he wants in the dts.
> 
> s/patent/parent ?
> s/he wants/it requires ?

Okay.

> 
>> +		 */
>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
> 
> [...]
> 
>> +static const struct clk_parent_data s4_vpu_clkc_parent_data[] = {
>> +	{ .fw_name = "fclk_div4", },
>> +	{ .fw_name = "fclk_div3", },
>> +	{ .fw_name = "fclk_div5", },
>> +	{ .fw_name = "fclk_div7", },
>> +	{ .fw_name = "mpll1", },
>> +	{ .hw = &s4_vid_pll.hw },
>> +	{ .fw_name = "mpll2", },
>> +	{ .fw_name = "gp0_pll", },
>> +};
>> +
>> +static struct clk_regmap s4_vpu_clkc_p0_mux  = {
>> +	.data = &(struct clk_regmap_mux_data){
>> +		.offset = CLKCTRL_VPU_CLKC_CTRL,
>> +		.mask = 0x7,
>> +		.shift = 9,
>> +	},
>> +	.hw.init = &(struct clk_init_data) {
>> +		.name = "vpu_clkc_p0_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_data = s4_vpu_clkc_parent_data,
>> +		.num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data),
>> +		/*
>> +		 * When the driver uses this clock, needs to specify the patent clock
>> +		 * he wants in the dts.
>> +		 */
> 
> That's quite a lot of occurences of the same comment.
> At the same time, other clocks with the same flag have no comment.
> 
> Please make general comment, before the Video/VPU section, explaining
> which clocks needs on a use-case basis (through DT) and possibly how it
> should be set, what should drive the choices.
> 

The owner of the corresponding driver module wants to have a fixed 
clock, but I can't explain every specific reason. So I'm going to change 
it all to.flags = CLK_SET_RATE_PARENT in the next version. Let CCF 
choose the appropriate clock as you suggested. If there is a 
corresponding module you want to change, ask him to give you a specific 
explanation. Do you think that's all right?

I will not reply to you below.

>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
> 
>> +
>> +/* EMMC/NAND clock */
>> +static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = {
>> +	{ .fw_name = "xtal", },
>> +	{ .fw_name = "fclk_div2", },
>> +	{ .fw_name = "fclk_div3", },
>> +	{ .fw_name = "hifi_pll", },
>> +	{ .fw_name = "fclk_div2p5", },
>> +	{ .fw_name = "mpll2", },
>> +	{ .fw_name = "mpll3", },
>> +	{ .fw_name = "gp0_pll", },
>> +};
>> +
>> +static struct clk_regmap s4_sd_emmc_c_clk0_sel = {
>> +	.data = &(struct clk_regmap_mux_data){
>> +		.offset = CLKCTRL_NAND_CLK_CTRL,
>> +		.mask = 0x7,
>> +		.shift = 9,
>> +	},
>> +	.hw.init = &(struct clk_init_data) {
>> +		.name = "sd_emmc_c_clk0_sel",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_data = s4_sd_emmc_clk0_parent_data,
>> +		.num_parents = ARRAY_SIZE(s4_sd_emmc_clk0_parent_data),
>> +		/*
>> +		 * When the driver uses this clock, needs to specify the patent clock
>> +		 * he wants in the dts.
>> +		 */
> 
> I'm getting a bit suspicious about the use (and abuse ...) of this flag.
> I don't quite get how selecting the base PLL for MMC should be done on
> use-case basis and should be up the board DT ...
> 
> Other SoC have all used fdiv2 so far. Do you expect this setting to be
> part of the dtsi SoC file ?
> 
>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
> 
>> +
>> +/* SPICC Clock */
>> +static const struct clk_parent_data s4_spicc_parent_data[] = {
>> +	{ .fw_name = "xtal", },
>> +	{ .hw = &s4_sys_clk.hw },
>> +	{ .fw_name = "fclk_div4", },
>> +	{ .fw_name = "fclk_div3", },
>> +	{ .fw_name = "fclk_div2", },
>> +	{ .fw_name = "fclk_div5", },
>> +	{ .fw_name = "fclk_div7", },
>> +};
>> +
>> +static struct clk_regmap s4_spicc0_mux = {
>> +	.data = &(struct clk_regmap_mux_data){
>> +		.offset = CLKCTRL_SPICC_CLK_CTRL,
>> +		.mask = 0x7,
>> +		.shift = 7,
>> +	},
>> +	.hw.init = &(struct clk_init_data) {
>> +		.name = "spicc0_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_data = s4_spicc_parent_data,
>> +		.num_parents = ARRAY_SIZE(s4_spicc_parent_data),
>> +		/*
>> +		 * When the driver uses this clock, needs to specify the patent clock
>> +		 * he wants in the dts.
>> +		 */
> 
> This is getting too far. All the parent clocks are fixed.
> Let CCF do the job of picking the most adequate clock for the job
> instead of manually settings things
> 
>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
> 
> 
>> +
>> +/* PWM Clock */
>> +static const struct clk_parent_data s4_pwm_parent_data[] = {
>> +	{ .fw_name = "xtal", },
>> +	{ .hw = &s4_vid_pll.hw },
>> +	{ .fw_name = "fclk_div4", },
>> +	{ .fw_name = "fclk_div3", },
>> +};
>> +
>> +static struct clk_regmap s4_pwm_a_mux = {
>> +	.data = &(struct clk_regmap_mux_data) {
>> +		.offset = CLKCTRL_PWM_CLK_AB_CTRL,
>> +		.mask = 0x3,
>> +		.shift = 9,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "pwm_a_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_data = s4_pwm_parent_data,
>> +		.num_parents = ARRAY_SIZE(s4_pwm_parent_data),
>> +		/*
>> +		 * When the driver uses this clock, needs to specify the patent clock
>> +		 * he wants in the dts.
>> +		 */
> 
> Same here ... this is really going to far.
> 
>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
> 
>> +
>> +static struct clk_regmap s4_saradc_mux = {
>> +	.data = &(struct clk_regmap_mux_data) {
>> +		.offset = CLKCTRL_SAR_CLK_CTRL,
>> +		.mask = 0x3,
>> +		.shift = 9,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "saradc_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_data = (const struct clk_parent_data []) {
>> +			{ .fw_name = "xtal", },
>> +			{ .hw = &s4_sys_clk.hw },
>> +		},
>> +		.num_parents = 2,
>> +		/*
>> +		 * When the driver uses this clock, needs to specify the patent clock
>> +		 * he wants in the dts.
>> +		 */
> 
> For each clock type, if this flag is going to be used, I'd like a clear
> explanation about why it is use-case dependent and why you need manual
> control over this. Same applies to all the occurence.
> 
>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>> +	},
>> +};
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Jerome Brunet]

On Mon 16 Jan 2023 at 17:31, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Krzysztof,
> 	Thank you for your quick reply.
>
> On 2023/1/16 16:29, Krzysztof Kozlowski wrote:
>> [ EXTERNAL EMAIL ]
>> On 16/01/2023 08:42, Yu Tu wrote:
>>> Add the S4 PLL & peripheral clock controller dt-bindings in the s4 SoC
>>> family.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> ---
>>>   .../clock/amlogic,s4-peripherals-clkc.yaml    | 104 ++++++++++++++
>>>   .../bindings/clock/amlogic,s4-pll-clkc.yaml   |  50 +++++++
>>>   MAINTAINERS                                   |   1 +
>>>   .../clock/amlogic,s4-peripherals-clkc.h       | 131 ++++++++++++++++++
>>>   .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |  30 ++++
>>>   5 files changed, 316 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>>   create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>   create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>> b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>> new file mode 100644
>>> index 000000000000..2deeff497754
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>> @@ -0,0 +1,104 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-peripherals-clkc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Amlogic Meson S serials Peripherals Clock Controller
>>> +
>>> +maintainers:
>>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>>> +  - Jerome Brunet <jbrunet@baylibre.com>
>>> +  - Yu Tu <yu.tu@amlogic.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: amlogic,s4-peripherals-clkc
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: input fixed pll div2
>>> +      - description: input fixed pll div2p5
>>> +      - description: input fixed pll div3
>>> +      - description: input fixed pll div4
>>> +      - description: input fixed pll div5
>>> +      - description: input fixed pll div7
>>> +      - description: input hifi pll
>>> +      - description: input gp0 pll
>>> +      - description: input mpll0
>>> +      - description: input mpll1
>>> +      - description: input mpll2
>>> +      - description: input mpll3
>>> +      - description: input hdmi pll
>>> +      - description: input oscillator (usually at 24MHz)
>>> +      - description: input external 32kHz reference (optional)
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: fclk_div2
>>> +      - const: fclk_div2p5
>>> +      - const: fclk_div3
>>> +      - const: fclk_div4
>>> +      - const: fclk_div5
>>> +      - const: fclk_div7
>>> +      - const: hifi_pll
>>> +      - const: gp0_pll
>>> +      - const: mpll0
>>> +      - const: mpll1
>>> +      - const: mpll2
>>> +      - const: mpll3
>>> +      - const: hdmi_pll
>>> +      - const: xtal
>>> +      - const: ext_32k
>>> +
>>> +  "#clock-cells":
>>> +    const: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clocks
>>> +  - clock-names
>>> +  - "#clock-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h>
>>> +
>>> +    /* 32KHz reference crystal */
>>> +    ext_32k: ref32k {
>>> +        compatible = "fixed-clock";
>>> +        #clock-cells = <0>;
>>> +        clock-frequency = <32000>;
>>> +    };
>> This wasn't here before. Drop it. It is trivial and it is not needed to
>> illustrate your device bindings. All clock bindings use it...
>> 
>
> I'm fine with that. I don't know if Jerome agrees with that. Wait for
> him. See what he says.

This is a simple change related to your binding example.
Krzysztof is one of the DT maintainer. Please follow his recommendation.

>
>>> +
>>> +    clkc_periphs: clock-controller@fe000000 {
>>> +      compatible = "amlogic,s4-peripherals-clkc";
>>> +      reg = <0xfe000000 0x49c>;
>>> +      clocks = <&clkc_pll 3>,
>>> +              <&clkc_pll 13>,
>>> +              <&clkc_pll 5>,
>>> +              <&clkc_pll 7>,
>>> +              <&clkc_pll 9>,
>>> +              <&clkc_pll 11>,
>>> +              <&clkc_pll 17>,
>>> +              <&clkc_pll 15>,
>>> +              <&clkc_pll 25>,
>>> +              <&clkc_pll 27>,
>>> +              <&clkc_pll 29>,
>>> +              <&clkc_pll 31>,
>>> +              <&clkc_pll 20>,
>>> +              <&xtal>,
>>> +              <&ext_32k>;
>>> +      clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", "fclk_div4",
>>> +                    "fclk_div5", "fclk_div7", "hifi_pll", "gp0_pll",
>>> +                    "mpll0", "mpll1", "mpll2", "mpll3", "hdmi_pll", "xtal",
>>> +                    "ext_32k";
>>> +      #clock-cells = <1>;
>>> +    };
>>> +...
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>> b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>> new file mode 100644
>>> index 000000000000..aeda4861cebe
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>> @@ -0,0 +1,50 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Amlogic Meson S serials PLL Clock Controller
>>> +
>>> +maintainers:
>>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>>> +  - Jerome Brunet <jbrunet@baylibre.com>
>>> +  - Yu Tu <yu.tu@amlogic.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: amlogic,s4-pll-clkc
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: xtal
>>> +
>>> +  "#clock-cells":
>>> +    const: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clocks
>>> +  - clock-names
>>> +  - "#clock-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    clkc_pll: clock-controller@fe008000 {
>>> +      compatible = "amlogic,s4-pll-clkc";
>>> +      reg = <0xfe008000 0x1e8>;
>>> +      clocks = <&xtal>;
>>> +      clock-names = "xtal";
>>> +      #clock-cells = <1>;
>>> +    };
>>> +
>>> +...
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index f61eb221415b..26c82beeffda 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1897,6 +1897,7 @@ L:	linux-amlogic@lists.infradead.org
>>>   S:	Maintained
>>>   F:	Documentation/devicetree/bindings/clock/amlogic*
>>>   F:	drivers/clk/meson/
>>> +F:	include/dt-bindings/clock/amlogic*
>>>   F:	include/dt-bindings/clock/gxbb*
>>>   F:	include/dt-bindings/clock/meson*
>>>   diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>> b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>> new file mode 100644
>>> index 000000000000..bbec5094d5c3
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>> @@ -0,0 +1,131 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> Unusual license... are you sure to license the bindings under GPLv4 or
>> GPLv5? Fine by me.
>> 
>
> Yes.
>
>> Best regards,
>> Krzysztof
>> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 2/3] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver

[Jerome Brunet]

On Fri 20 Jan 2023 at 10:58, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Jerome,
>
> On 2023/1/19 19:20, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> Add the S4 PLL clock controller driver in the s4 SoC family.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> ---
>> [...]
>> 
>>> +
>>> +static struct clk_regmap s4_fclk_div2 = {
>>> +	.data = &(struct clk_regmap_gate_data){
>>> +		.offset = ANACTRL_FIXPLL_CTRL1,
>>> +		.bit_idx = 24,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data){
>>> +		.name = "fclk_div2",
>>> +		.ops = &clk_regmap_gate_ro_ops,
>> On the previous SoC, these fixed divider gate were not read-only.
>> They are marked as critical when necessary, with the appropriate
>> comment.
>> Why is it different on the s4 ?
>
> In fact, this part of the SOC is no different from the previous G12a/b and
> so on.
>
> I remember that my first version was made according to G12A, and I changed
> this way under your suggestion.
>
> Maybe you were busy and forgot. For me, this mode and the previous g12a
> mode function is ok. I can do either. So now how do you decide to go that
> way?

No I did not forgot.
I told you that cannot put CRITICAL (or IGNORE_USED) without explaining
why. I stand by this. Same goes for RO ops. 

>
>> 
>>> +		.parent_hws = (const struct clk_hw *[]) {
>>> +			&s4_fclk_div2_div.hw
>>> +		},
>>> +		.num_parents = 1,
>>> +	},
>>> +};
>>> +
>> [...]
>> 
>>> +#ifndef __MESON_S4_PLL_H__
>>> +#define __MESON_S4_PLL_H__
>>> +
>>> +/* ANA_CTRL - Registers
>>> + * REG_BASE:  REGISTER_BASE_ADDR = 0xfe008000
>> This multi-line comment style is wrong in clk/
>> REG_BASE is not used so I'm not sure this is useful
>
> I will remove REG_BASE and  change this format in next version.
>
>> 
>>> + */
>>> +#define ANACTRL_FIXPLL_CTRL0                       0x040
>>> +#define ANACTRL_FIXPLL_CTRL1                       0x044
>>> +#define ANACTRL_FIXPLL_CTRL2                       0x048
>>> +#define ANACTRL_FIXPLL_CTRL3                       0x04c
>>> +#define ANACTRL_FIXPLL_CTRL4                       0x050
>>> +#define ANACTRL_FIXPLL_CTRL5                       0x054
>>> +#define ANACTRL_FIXPLL_CTRL6                       0x058
>>> +#define ANACTRL_FIXPLL_STS                         0x05c
>>> +#define ANACTRL_GP0PLL_CTRL0                       0x080
>>> +#define ANACTRL_GP0PLL_CTRL1                       0x084
>>> +#define ANACTRL_GP0PLL_CTRL2                       0x088
>>> +#define ANACTRL_GP0PLL_CTRL3                       0x08c
>>> +#define ANACTRL_GP0PLL_CTRL4                       0x090
>>> +#define ANACTRL_GP0PLL_CTRL5                       0x094
>>> +#define ANACTRL_GP0PLL_CTRL6                       0x098
>>> +#define ANACTRL_GP0PLL_STS                         0x09c
>>> +#define ANACTRL_HIFIPLL_CTRL0                      0x100
>>> +#define ANACTRL_HIFIPLL_CTRL1                      0x104
>>> +#define ANACTRL_HIFIPLL_CTRL2                      0x108
>>> +#define ANACTRL_HIFIPLL_CTRL3                      0x10c
>>> +#define ANACTRL_HIFIPLL_CTRL4                      0x110
>>> +#define ANACTRL_HIFIPLL_CTRL5                      0x114
>>> +#define ANACTRL_HIFIPLL_CTRL6                      0x118
>>> +#define ANACTRL_HIFIPLL_STS                        0x11c
>>> +#define ANACTRL_MPLL_CTRL0                         0x180
>>> +#define ANACTRL_MPLL_CTRL1                         0x184
>>> +#define ANACTRL_MPLL_CTRL2                         0x188
>>> +#define ANACTRL_MPLL_CTRL3                         0x18c
>>> +#define ANACTRL_MPLL_CTRL4                         0x190
>>> +#define ANACTRL_MPLL_CTRL5                         0x194
>>> +#define ANACTRL_MPLL_CTRL6                         0x198
>>> +#define ANACTRL_MPLL_CTRL7                         0x19c
>>> +#define ANACTRL_MPLL_CTRL8                         0x1a0
>>> +#define ANACTRL_MPLL_STS                           0x1a4
>>> +#define ANACTRL_HDMIPLL_CTRL0                      0x1c0
>>> +#define ANACTRL_HDMIPLL_CTRL1                      0x1c4
>>> +#define ANACTRL_HDMIPLL_CTRL2                      0x1c8
>>> +#define ANACTRL_HDMIPLL_CTRL3                      0x1cc
>>> +#define ANACTRL_HDMIPLL_CTRL4                      0x1d0
>>> +#define ANACTRL_HDMIPLL_CTRL5                      0x1d4
>>> +#define ANACTRL_HDMIPLL_CTRL6                      0x1d8
>>> +#define ANACTRL_HDMIPLL_STS                        0x1dc
>>> +#define ANACTRL_HDMIPLL_VLOCK                      0x1e4
>>> +
>>> +/*
>>> + * CLKID index values
>>> + *
>>> + * These indices are entirely contrived and do not map onto the hardware.
>>> + * It has now been decided to expose everything by default in the DT header:
>>> + * include/dt-bindings/clock/axg-clkc.h. Only the clocks ids we don't want
>>> + * to expose, such as the internal muxes and dividers of composite clocks,
>>> + * will remain defined here.
>>> + */
>>> +#define CLKID_FIXED_PLL_DCO		0
>>> +#define CLKID_FCLK_DIV2_DIV		2
>>> +#define CLKID_FCLK_DIV3_DIV		4
>>> +#define CLKID_FCLK_DIV4_DIV		6
>>> +#define CLKID_FCLK_DIV5_DIV		8
>>> +#define CLKID_FCLK_DIV7_DIV		10
>>> +#define CLKID_FCLK_DIV2P5_DIV		12
>>> +#define CLKID_GP0_PLL_DCO		14
>>> +#define CLKID_HIFI_PLL_DCO		16
>>> +#define CLKID_HDMI_PLL_DCO		18
>>> +#define CLKID_HDMI_PLL_OD		19
>>> +#define CLKID_MPLL_50M_DIV		21
>>> +#define CLKID_MPLL_PREDIV		23
>>> +#define CLKID_MPLL0_DIV			24
>>> +#define CLKID_MPLL1_DIV			26
>>> +#define CLKID_MPLL2_DIV			28
>>> +#define CLKID_MPLL3_DIV			30
>>> +
>>> +#define NR_PLL_CLKS			32
>>> +/* include the CLKIDs that have been made part of the DT binding */
>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h>
>>> +
>>> +#endif /* __MESON_S4_PLL_H__ */
>> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Jerome Brunet]

On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi
> On 2023/1/19 19:37, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> [...]
>> 
>>> +
>>> +/* Video Clocks */
>>> +static struct clk_regmap s4_vid_pll_div = {
>>> +	.data = &(struct meson_vid_pll_div_data){
>>> +		.val = {
>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>> +			.shift   = 0,
>>> +			.width   = 15,
>>> +		},
>>> +		.sel = {
>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>> +			.shift   = 16,
>>> +			.width   = 2,
>>> +		},
>>> +	},
>>> +	.hw.init = &(struct clk_init_data) {
>>> +		.name = "vid_pll_div",
>>> +		/*
>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>> +		 * clock is the default value of this register. When designing the
>>> +		 * video module of the chip, a default value that can meet the
>>> +		 * requirements of the video module will be solidified according
>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>> +		 * simulation. So this is ro_ops.
>>> +		 * It is important to note that this clock is not used on this
>>> +		 * chip and is described only for the integrity of the clock tree.
>>> +		 */
>> If it is reset value and will be applicable to all the design, regarless
>> of the use-case, then yes RO ops is OK
>> 
>>>From what I understand here, the value will depend on the use-case requirements.
>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>
> Check the previous chip history, the actual scene is not used at all,
> basically is used in simulation. So the previous SOC was "ro_ops" without
> any problems.  This S4 SOC is not actually useful either.
>
> So when you were upstream, you had no problem making "ro_ops". I wonder if
> I could delete this useless clock, so you don't have to worry about it.

I don't know what to make of this. What is the point of adding a useless
clock ?

>
>> 
>>> +		.ops = &meson_vid_pll_div_ro_ops,
>>> +		.parent_data = (const struct clk_parent_data []) {
>>> +			{ .fw_name = "hdmi_pll", }
>>> +		},
>>> +		.num_parents = 1,
>>> +		.flags = CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>>> +
>> 
>>> +
>>> +/* VDEC clocks */
>>> +static const struct clk_parent_data s4_dec_parent_data[] = {
>>> +	{ .fw_name = "fclk_div2p5", },
>>> +	{ .fw_name = "fclk_div3", },
>>> +	{ .fw_name = "fclk_div4", },
>>> +	{ .fw_name = "fclk_div5", },
>>> +	{ .fw_name = "fclk_div7", },
>>> +	{ .fw_name = "hifi_pll", },
>>> +	{ .fw_name = "gp0_pll", },
>>> +	{ .fw_name = "xtal", }
>>> +};
>>> +
>>> +static struct clk_regmap s4_vdec_p0_mux = {
>>> +	.data = &(struct clk_regmap_mux_data){
>>> +		.offset = CLKCTRL_VDEC_CLK_CTRL,
>>> +		.mask = 0x7,
>>> +		.shift = 9,
>>> +		.flags = CLK_MUX_ROUND_CLOSEST,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data) {
>>> +		.name = "vdec_p0_mux",
>>> +		.ops = &clk_regmap_mux_ops,
>>> +		.parent_data = s4_dec_parent_data,
>>> +		.num_parents = ARRAY_SIZE(s4_dec_parent_data),
>>> +		/*
>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>> +		 * he wants in the dts.
>> s/patent/parent ?
>> s/he wants/it requires ?
>
> Okay.
>
>> 
>>> +		 */
>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>>> +
>> [...]
>> 
>>> +static const struct clk_parent_data s4_vpu_clkc_parent_data[] = {
>>> +	{ .fw_name = "fclk_div4", },
>>> +	{ .fw_name = "fclk_div3", },
>>> +	{ .fw_name = "fclk_div5", },
>>> +	{ .fw_name = "fclk_div7", },
>>> +	{ .fw_name = "mpll1", },
>>> +	{ .hw = &s4_vid_pll.hw },
>>> +	{ .fw_name = "mpll2", },
>>> +	{ .fw_name = "gp0_pll", },
>>> +};
>>> +
>>> +static struct clk_regmap s4_vpu_clkc_p0_mux  = {
>>> +	.data = &(struct clk_regmap_mux_data){
>>> +		.offset = CLKCTRL_VPU_CLKC_CTRL,
>>> +		.mask = 0x7,
>>> +		.shift = 9,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data) {
>>> +		.name = "vpu_clkc_p0_mux",
>>> +		.ops = &clk_regmap_mux_ops,
>>> +		.parent_data = s4_vpu_clkc_parent_data,
>>> +		.num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data),
>>> +		/*
>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>> +		 * he wants in the dts.
>>> +		 */
>> That's quite a lot of occurences of the same comment.
>> At the same time, other clocks with the same flag have no comment.
>> Please make general comment, before the Video/VPU section, explaining
>> which clocks needs on a use-case basis (through DT) and possibly how it
>> should be set, what should drive the choices.
>> 
>
> The owner of the corresponding driver module wants to have a fixed clock,
> but I can't explain every specific reason.

Why can't you ?

> So I'm going to change it all
> to.flags = CLK_SET_RATE_PARENT in the next version. Let CCF choose the
> appropriate clock as you suggested. If there is a corresponding module you
> want to change, ask him to give you a specific explanation. Do you think
> that's all right?

If the flag is actually required and there is a reason, no it is not.

>
> I will not reply to you below.

Noted.

>
>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>>> +
>> 
>>> +
>>> +/* EMMC/NAND clock */
>>> +static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = {
>>> +	{ .fw_name = "xtal", },
>>> +	{ .fw_name = "fclk_div2", },
>>> +	{ .fw_name = "fclk_div3", },
>>> +	{ .fw_name = "hifi_pll", },
>>> +	{ .fw_name = "fclk_div2p5", },
>>> +	{ .fw_name = "mpll2", },
>>> +	{ .fw_name = "mpll3", },
>>> +	{ .fw_name = "gp0_pll", },
>>> +};
>>> +
>>> +static struct clk_regmap s4_sd_emmc_c_clk0_sel = {
>>> +	.data = &(struct clk_regmap_mux_data){
>>> +		.offset = CLKCTRL_NAND_CLK_CTRL,
>>> +		.mask = 0x7,
>>> +		.shift = 9,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data) {
>>> +		.name = "sd_emmc_c_clk0_sel",
>>> +		.ops = &clk_regmap_mux_ops,
>>> +		.parent_data = s4_sd_emmc_clk0_parent_data,
>>> +		.num_parents = ARRAY_SIZE(s4_sd_emmc_clk0_parent_data),
>>> +		/*
>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>> +		 * he wants in the dts.
>>> +		 */
>> I'm getting a bit suspicious about the use (and abuse ...) of this flag.
>> I don't quite get how selecting the base PLL for MMC should be done on
>> use-case basis and should be up the board DT ...
>> Other SoC have all used fdiv2 so far. Do you expect this setting to be
>> part of the dtsi SoC file ?
>> 
>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>>> +
>> 
>>> +
>>> +/* SPICC Clock */
>>> +static const struct clk_parent_data s4_spicc_parent_data[] = {
>>> +	{ .fw_name = "xtal", },
>>> +	{ .hw = &s4_sys_clk.hw },
>>> +	{ .fw_name = "fclk_div4", },
>>> +	{ .fw_name = "fclk_div3", },
>>> +	{ .fw_name = "fclk_div2", },
>>> +	{ .fw_name = "fclk_div5", },
>>> +	{ .fw_name = "fclk_div7", },
>>> +};
>>> +
>>> +static struct clk_regmap s4_spicc0_mux = {
>>> +	.data = &(struct clk_regmap_mux_data){
>>> +		.offset = CLKCTRL_SPICC_CLK_CTRL,
>>> +		.mask = 0x7,
>>> +		.shift = 7,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data) {
>>> +		.name = "spicc0_mux",
>>> +		.ops = &clk_regmap_mux_ops,
>>> +		.parent_data = s4_spicc_parent_data,
>>> +		.num_parents = ARRAY_SIZE(s4_spicc_parent_data),
>>> +		/*
>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>> +		 * he wants in the dts.
>>> +		 */
>> This is getting too far. All the parent clocks are fixed.
>> Let CCF do the job of picking the most adequate clock for the job
>> instead of manually settings things
>> 
>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>>> +
>> 
>>> +
>>> +/* PWM Clock */
>>> +static const struct clk_parent_data s4_pwm_parent_data[] = {
>>> +	{ .fw_name = "xtal", },
>>> +	{ .hw = &s4_vid_pll.hw },
>>> +	{ .fw_name = "fclk_div4", },
>>> +	{ .fw_name = "fclk_div3", },
>>> +};
>>> +
>>> +static struct clk_regmap s4_pwm_a_mux = {
>>> +	.data = &(struct clk_regmap_mux_data) {
>>> +		.offset = CLKCTRL_PWM_CLK_AB_CTRL,
>>> +		.mask = 0x3,
>>> +		.shift = 9,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data){
>>> +		.name = "pwm_a_mux",
>>> +		.ops = &clk_regmap_mux_ops,
>>> +		.parent_data = s4_pwm_parent_data,
>>> +		.num_parents = ARRAY_SIZE(s4_pwm_parent_data),
>>> +		/*
>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>> +		 * he wants in the dts.
>>> +		 */
>> Same here ... this is really going to far.
>> 
>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>>> +
>> 
>>> +
>>> +static struct clk_regmap s4_saradc_mux = {
>>> +	.data = &(struct clk_regmap_mux_data) {
>>> +		.offset = CLKCTRL_SAR_CLK_CTRL,
>>> +		.mask = 0x3,
>>> +		.shift = 9,
>>> +	},
>>> +	.hw.init = &(struct clk_init_data){
>>> +		.name = "saradc_mux",
>>> +		.ops = &clk_regmap_mux_ops,
>>> +		.parent_data = (const struct clk_parent_data []) {
>>> +			{ .fw_name = "xtal", },
>>> +			{ .hw = &s4_sys_clk.hw },
>>> +		},
>>> +		.num_parents = 2,
>>> +		/*
>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>> +		 * he wants in the dts.
>>> +		 */
>> For each clock type, if this flag is going to be used, I'd like a clear
>> explanation about why it is use-case dependent and why you need manual
>> control over this. Same applies to all the occurence.
>> 
>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>> +	},
>>> +};
>> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Kevin Hilman]

Yu Tu <yu.tu@amlogic.com> writes:

> Hi Kevin,
>
> On 2023/1/19 8:38, Kevin Hilman wrote:
>> [ EXTERNAL EMAIL ]
>> 
>> Yu Tu <yu.tu@amlogic.com> writes:
>> 
>>> On 2023/1/16 16:29, Krzysztof Kozlowski wrote:
>> 
>> [...]
>> 
>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>>> new file mode 100644
>>>>> index 000000000000..bbec5094d5c3
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>>> @@ -0,0 +1,131 @@
>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>>>
>>>> Unusual license... are you sure to license the bindings under GPLv4 or
>>>> GPLv5? Fine by me.
>>>>
>>>
>>> Yes.
>> 
>> The rest of the bindings for Amlogic SoCs are GPL-2.0 (without the '+').
>> Adding the dual-license for MIT seems fine, but adding the '+' is
>> curious.
>> 
>> It would be helpful if you could please explain why you'd like these
>> bindings to be licensed differently than the rest of the SoC family.
>> 
>
> I actually refer to the previous g12a Soc.
> https://elixir.bootlin.com/linux/v6.2-rc4/source/include/dt-bindings/clock/g12a-clkc.h
> https://elixir.bootlin.com/linux/v6.2-rc4/source/include/dt-bindings/clock/axg-clkc.h
> [...]
>
> So if you think it is not necessary, I will delete the '+' as you 
> suggested. Don't know what you choose?

Drop the `+`

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Yu Tu]

On 2023/1/25 8:25, Kevin Hilman wrote:
> [ EXTERNAL EMAIL ]
> 
> Yu Tu <yu.tu@amlogic.com> writes:
> 
>> Hi Kevin,
>>
>> On 2023/1/19 8:38, Kevin Hilman wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Yu Tu <yu.tu@amlogic.com> writes:
>>>
>>>> On 2023/1/16 16:29, Krzysztof Kozlowski wrote:
>>>
>>> [...]
>>>
>>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..bbec5094d5c3
>>>>>> --- /dev/null
>>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>>>> @@ -0,0 +1,131 @@
>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>>>>
>>>>> Unusual license... are you sure to license the bindings under GPLv4 or
>>>>> GPLv5? Fine by me.
>>>>>
>>>>
>>>> Yes.
>>>
>>> The rest of the bindings for Amlogic SoCs are GPL-2.0 (without the '+').
>>> Adding the dual-license for MIT seems fine, but adding the '+' is
>>> curious.
>>>
>>> It would be helpful if you could please explain why you'd like these
>>> bindings to be licensed differently than the rest of the SoC family.
>>>
>>
>> I actually refer to the previous g12a Soc.
>> https://elixir.bootlin.com/linux/v6.2-rc4/source/include/dt-bindings/clock/g12a-clkc.h
>> https://elixir.bootlin.com/linux/v6.2-rc4/source/include/dt-bindings/clock/axg-clkc.h
>> [...]
>>
>> So if you think it is not necessary, I will delete the '+' as you
>> suggested. Don't know what you choose?
> 
> Drop the `+`

Okay.

> 
> Kevin
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller

[Yu Tu]

On 2023/1/20 17:37, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 16 Jan 2023 at 17:31, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi Krzysztof,
>> 	Thank you for your quick reply.
>>
>> On 2023/1/16 16:29, Krzysztof Kozlowski wrote:
>>> [ EXTERNAL EMAIL ]
>>> On 16/01/2023 08:42, Yu Tu wrote:
>>>> Add the S4 PLL & peripheral clock controller dt-bindings in the s4 SoC
>>>> family.
>>>>
>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>> ---
>>>>    .../clock/amlogic,s4-peripherals-clkc.yaml    | 104 ++++++++++++++
>>>>    .../bindings/clock/amlogic,s4-pll-clkc.yaml   |  50 +++++++
>>>>    MAINTAINERS                                   |   1 +
>>>>    .../clock/amlogic,s4-peripherals-clkc.h       | 131 ++++++++++++++++++
>>>>    .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |  30 ++++
>>>>    5 files changed, 316 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>>>    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>>>    create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>>    create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>>> b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>>> new file mode 100644
>>>> index 000000000000..2deeff497754
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
>>>> @@ -0,0 +1,104 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-peripherals-clkc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Amlogic Meson S serials Peripherals Clock Controller
>>>> +
>>>> +maintainers:
>>>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>>>> +  - Jerome Brunet <jbrunet@baylibre.com>
>>>> +  - Yu Tu <yu.tu@amlogic.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: amlogic,s4-peripherals-clkc
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: input fixed pll div2
>>>> +      - description: input fixed pll div2p5
>>>> +      - description: input fixed pll div3
>>>> +      - description: input fixed pll div4
>>>> +      - description: input fixed pll div5
>>>> +      - description: input fixed pll div7
>>>> +      - description: input hifi pll
>>>> +      - description: input gp0 pll
>>>> +      - description: input mpll0
>>>> +      - description: input mpll1
>>>> +      - description: input mpll2
>>>> +      - description: input mpll3
>>>> +      - description: input hdmi pll
>>>> +      - description: input oscillator (usually at 24MHz)
>>>> +      - description: input external 32kHz reference (optional)
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: fclk_div2
>>>> +      - const: fclk_div2p5
>>>> +      - const: fclk_div3
>>>> +      - const: fclk_div4
>>>> +      - const: fclk_div5
>>>> +      - const: fclk_div7
>>>> +      - const: hifi_pll
>>>> +      - const: gp0_pll
>>>> +      - const: mpll0
>>>> +      - const: mpll1
>>>> +      - const: mpll2
>>>> +      - const: mpll3
>>>> +      - const: hdmi_pll
>>>> +      - const: xtal
>>>> +      - const: ext_32k
>>>> +
>>>> +  "#clock-cells":
>>>> +    const: 1
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - clocks
>>>> +  - clock-names
>>>> +  - "#clock-cells"
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h>
>>>> +
>>>> +    /* 32KHz reference crystal */
>>>> +    ext_32k: ref32k {
>>>> +        compatible = "fixed-clock";
>>>> +        #clock-cells = <0>;
>>>> +        clock-frequency = <32000>;
>>>> +    };
>>> This wasn't here before. Drop it. It is trivial and it is not needed to
>>> illustrate your device bindings. All clock bindings use it...
>>>
>>
>> I'm fine with that. I don't know if Jerome agrees with that. Wait for
>> him. See what he says.
> 
> This is a simple change related to your binding example.
> Krzysztof is one of the DT maintainer. Please follow his recommendation.

Okay. Got it.

> 
>>
>>>> +
>>>> +    clkc_periphs: clock-controller@fe000000 {
>>>> +      compatible = "amlogic,s4-peripherals-clkc";
>>>> +      reg = <0xfe000000 0x49c>;
>>>> +      clocks = <&clkc_pll 3>,
>>>> +              <&clkc_pll 13>,
>>>> +              <&clkc_pll 5>,
>>>> +              <&clkc_pll 7>,
>>>> +              <&clkc_pll 9>,
>>>> +              <&clkc_pll 11>,
>>>> +              <&clkc_pll 17>,
>>>> +              <&clkc_pll 15>,
>>>> +              <&clkc_pll 25>,
>>>> +              <&clkc_pll 27>,
>>>> +              <&clkc_pll 29>,
>>>> +              <&clkc_pll 31>,
>>>> +              <&clkc_pll 20>,
>>>> +              <&xtal>,
>>>> +              <&ext_32k>;
>>>> +      clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", "fclk_div4",
>>>> +                    "fclk_div5", "fclk_div7", "hifi_pll", "gp0_pll",
>>>> +                    "mpll0", "mpll1", "mpll2", "mpll3", "hdmi_pll", "xtal",
>>>> +                    "ext_32k";
>>>> +      #clock-cells = <1>;
>>>> +    };
>>>> +...
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>>> b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>>> new file mode 100644
>>>> index 000000000000..aeda4861cebe
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
>>>> @@ -0,0 +1,50 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Amlogic Meson S serials PLL Clock Controller
>>>> +
>>>> +maintainers:
>>>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>>>> +  - Jerome Brunet <jbrunet@baylibre.com>
>>>> +  - Yu Tu <yu.tu@amlogic.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: amlogic,s4-pll-clkc
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: xtal
>>>> +
>>>> +  "#clock-cells":
>>>> +    const: 1
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - clocks
>>>> +  - clock-names
>>>> +  - "#clock-cells"
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    clkc_pll: clock-controller@fe008000 {
>>>> +      compatible = "amlogic,s4-pll-clkc";
>>>> +      reg = <0xfe008000 0x1e8>;
>>>> +      clocks = <&xtal>;
>>>> +      clock-names = "xtal";
>>>> +      #clock-cells = <1>;
>>>> +    };
>>>> +
>>>> +...
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index f61eb221415b..26c82beeffda 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1897,6 +1897,7 @@ L:	linux-amlogic@lists.infradead.org
>>>>    S:	Maintained
>>>>    F:	Documentation/devicetree/bindings/clock/amlogic*
>>>>    F:	drivers/clk/meson/
>>>> +F:	include/dt-bindings/clock/amlogic*
>>>>    F:	include/dt-bindings/clock/gxbb*
>>>>    F:	include/dt-bindings/clock/meson*
>>>>    diff --git a/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>> b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>> new file mode 100644
>>>> index 000000000000..bbec5094d5c3
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
>>>> @@ -0,0 +1,131 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>>> Unusual license... are you sure to license the bindings under GPLv4 or
>>> GPLv5? Fine by me.
>>>
>>
>> Yes.
>>
>>> Best regards,
>>> Krzysztof
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 2/3] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver

[Yu Tu]

On 2023/1/20 17:43, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Fri 20 Jan 2023 at 10:58, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi Jerome,
>>
>> On 2023/1/19 19:20, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Add the S4 PLL clock controller driver in the s4 SoC family.
>>>>
>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>> ---
>>> [...]
>>>
>>>> +
>>>> +static struct clk_regmap s4_fclk_div2 = {
>>>> +	.data = &(struct clk_regmap_gate_data){
>>>> +		.offset = ANACTRL_FIXPLL_CTRL1,
>>>> +		.bit_idx = 24,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "fclk_div2",
>>>> +		.ops = &clk_regmap_gate_ro_ops,
>>> On the previous SoC, these fixed divider gate were not read-only.
>>> They are marked as critical when necessary, with the appropriate
>>> comment.
>>> Why is it different on the s4 ?
>>
>> In fact, this part of the SOC is no different from the previous G12a/b and
>> so on.
>>
>> I remember that my first version was made according to G12A, and I changed
>> this way under your suggestion.
>>
>> Maybe you were busy and forgot. For me, this mode and the previous g12a
>> mode function is ok. I can do either. So now how do you decide to go that
>> way?
> 
> No I did not forgot.
> I told you that cannot put CRITICAL (or IGNORE_USED) without explaining
> why. I stand by this. Same goes for RO ops.
> 

I will add an explanation.

>>
>>>
>>>> +		.parent_hws = (const struct clk_hw *[]) {
>>>> +			&s4_fclk_div2_div.hw
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +	},
>>>> +};
>>>> +
>>> [...]
>>>
>>>> +#ifndef __MESON_S4_PLL_H__
>>>> +#define __MESON_S4_PLL_H__
>>>> +
>>>> +/* ANA_CTRL - Registers
>>>> + * REG_BASE:  REGISTER_BASE_ADDR = 0xfe008000
>>> This multi-line comment style is wrong in clk/
>>> REG_BASE is not used so I'm not sure this is useful
>>
>> I will remove REG_BASE and  change this format in next version.
>>
>>>
>>>> + */
>>>> +#define ANACTRL_FIXPLL_CTRL0                       0x040
>>>> +#define ANACTRL_FIXPLL_CTRL1                       0x044
>>>> +#define ANACTRL_FIXPLL_CTRL2                       0x048
>>>> +#define ANACTRL_FIXPLL_CTRL3                       0x04c
>>>> +#define ANACTRL_FIXPLL_CTRL4                       0x050
>>>> +#define ANACTRL_FIXPLL_CTRL5                       0x054
>>>> +#define ANACTRL_FIXPLL_CTRL6                       0x058
>>>> +#define ANACTRL_FIXPLL_STS                         0x05c
>>>> +#define ANACTRL_GP0PLL_CTRL0                       0x080
>>>> +#define ANACTRL_GP0PLL_CTRL1                       0x084
>>>> +#define ANACTRL_GP0PLL_CTRL2                       0x088
>>>> +#define ANACTRL_GP0PLL_CTRL3                       0x08c
>>>> +#define ANACTRL_GP0PLL_CTRL4                       0x090
>>>> +#define ANACTRL_GP0PLL_CTRL5                       0x094
>>>> +#define ANACTRL_GP0PLL_CTRL6                       0x098
>>>> +#define ANACTRL_GP0PLL_STS                         0x09c
>>>> +#define ANACTRL_HIFIPLL_CTRL0                      0x100
>>>> +#define ANACTRL_HIFIPLL_CTRL1                      0x104
>>>> +#define ANACTRL_HIFIPLL_CTRL2                      0x108
>>>> +#define ANACTRL_HIFIPLL_CTRL3                      0x10c
>>>> +#define ANACTRL_HIFIPLL_CTRL4                      0x110
>>>> +#define ANACTRL_HIFIPLL_CTRL5                      0x114
>>>> +#define ANACTRL_HIFIPLL_CTRL6                      0x118
>>>> +#define ANACTRL_HIFIPLL_STS                        0x11c
>>>> +#define ANACTRL_MPLL_CTRL0                         0x180
>>>> +#define ANACTRL_MPLL_CTRL1                         0x184
>>>> +#define ANACTRL_MPLL_CTRL2                         0x188
>>>> +#define ANACTRL_MPLL_CTRL3                         0x18c
>>>> +#define ANACTRL_MPLL_CTRL4                         0x190
>>>> +#define ANACTRL_MPLL_CTRL5                         0x194
>>>> +#define ANACTRL_MPLL_CTRL6                         0x198
>>>> +#define ANACTRL_MPLL_CTRL7                         0x19c
>>>> +#define ANACTRL_MPLL_CTRL8                         0x1a0
>>>> +#define ANACTRL_MPLL_STS                           0x1a4
>>>> +#define ANACTRL_HDMIPLL_CTRL0                      0x1c0
>>>> +#define ANACTRL_HDMIPLL_CTRL1                      0x1c4
>>>> +#define ANACTRL_HDMIPLL_CTRL2                      0x1c8
>>>> +#define ANACTRL_HDMIPLL_CTRL3                      0x1cc
>>>> +#define ANACTRL_HDMIPLL_CTRL4                      0x1d0
>>>> +#define ANACTRL_HDMIPLL_CTRL5                      0x1d4
>>>> +#define ANACTRL_HDMIPLL_CTRL6                      0x1d8
>>>> +#define ANACTRL_HDMIPLL_STS                        0x1dc
>>>> +#define ANACTRL_HDMIPLL_VLOCK                      0x1e4
>>>> +
>>>> +/*
>>>> + * CLKID index values
>>>> + *
>>>> + * These indices are entirely contrived and do not map onto the hardware.
>>>> + * It has now been decided to expose everything by default in the DT header:
>>>> + * include/dt-bindings/clock/axg-clkc.h. Only the clocks ids we don't want
>>>> + * to expose, such as the internal muxes and dividers of composite clocks,
>>>> + * will remain defined here.
>>>> + */
>>>> +#define CLKID_FIXED_PLL_DCO		0
>>>> +#define CLKID_FCLK_DIV2_DIV		2
>>>> +#define CLKID_FCLK_DIV3_DIV		4
>>>> +#define CLKID_FCLK_DIV4_DIV		6
>>>> +#define CLKID_FCLK_DIV5_DIV		8
>>>> +#define CLKID_FCLK_DIV7_DIV		10
>>>> +#define CLKID_FCLK_DIV2P5_DIV		12
>>>> +#define CLKID_GP0_PLL_DCO		14
>>>> +#define CLKID_HIFI_PLL_DCO		16
>>>> +#define CLKID_HDMI_PLL_DCO		18
>>>> +#define CLKID_HDMI_PLL_OD		19
>>>> +#define CLKID_MPLL_50M_DIV		21
>>>> +#define CLKID_MPLL_PREDIV		23
>>>> +#define CLKID_MPLL0_DIV			24
>>>> +#define CLKID_MPLL1_DIV			26
>>>> +#define CLKID_MPLL2_DIV			28
>>>> +#define CLKID_MPLL3_DIV			30
>>>> +
>>>> +#define NR_PLL_CLKS			32
>>>> +/* include the CLKIDs that have been made part of the DT binding */
>>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h>
>>>> +
>>>> +#endif /* __MESON_S4_PLL_H__ */
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Yu Tu]

On 2023/1/20 17:47, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi
>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>
>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> [...]
>>>
>>>> +
>>>> +/* Video Clocks */
>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>> +		.val = {
>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>> +			.shift   = 0,
>>>> +			.width   = 15,
>>>> +		},
>>>> +		.sel = {
>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>> +			.shift   = 16,
>>>> +			.width   = 2,
>>>> +		},
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "vid_pll_div",
>>>> +		/*
>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>> +		 * clock is the default value of this register. When designing the
>>>> +		 * video module of the chip, a default value that can meet the
>>>> +		 * requirements of the video module will be solidified according
>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>> +		 * simulation. So this is ro_ops.
>>>> +		 * It is important to note that this clock is not used on this
>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>> +		 */
>>> If it is reset value and will be applicable to all the design, regarless
>>> of the use-case, then yes RO ops is OK
>>>
>>> >From what I understand here, the value will depend on the use-case requirements.
>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>
>> Check the previous chip history, the actual scene is not used at all,
>> basically is used in simulation. So the previous SOC was "ro_ops" without
>> any problems.  This S4 SOC is not actually useful either.
>>
>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>> I could delete this useless clock, so you don't have to worry about it.
> 
> I don't know what to make of this. What is the point of adding a useless
> clock ?

As explained earlier this "vid_pll_div" is actually used in chip 
emulation. So next I'd like to know what you suggest to do with the clock?

By the way, the late S-Series chips removed this clock.

> 
>>
>>>
>>>> +		.ops = &meson_vid_pll_div_ro_ops,
>>>> +		.parent_data = (const struct clk_parent_data []) {
>>>> +			{ .fw_name = "hdmi_pll", }
>>>> +		},
>>>> +		.num_parents = 1,
>>>> +		.flags = CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>
>>>> +
>>>> +/* VDEC clocks */
>>>> +static const struct clk_parent_data s4_dec_parent_data[] = {
>>>> +	{ .fw_name = "fclk_div2p5", },
>>>> +	{ .fw_name = "fclk_div3", },
>>>> +	{ .fw_name = "fclk_div4", },
>>>> +	{ .fw_name = "fclk_div5", },
>>>> +	{ .fw_name = "fclk_div7", },
>>>> +	{ .fw_name = "hifi_pll", },
>>>> +	{ .fw_name = "gp0_pll", },
>>>> +	{ .fw_name = "xtal", }
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_vdec_p0_mux = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = CLKCTRL_VDEC_CLK_CTRL,
>>>> +		.mask = 0x7,
>>>> +		.shift = 9,
>>>> +		.flags = CLK_MUX_ROUND_CLOSEST,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "vdec_p0_mux",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = s4_dec_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(s4_dec_parent_data),
>>>> +		/*
>>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>>> +		 * he wants in the dts.
>>> s/patent/parent ?
>>> s/he wants/it requires ?
>>
>> Okay.
>>
>>>
>>>> +		 */
>>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>> [...]
>>>
>>>> +static const struct clk_parent_data s4_vpu_clkc_parent_data[] = {
>>>> +	{ .fw_name = "fclk_div4", },
>>>> +	{ .fw_name = "fclk_div3", },
>>>> +	{ .fw_name = "fclk_div5", },
>>>> +	{ .fw_name = "fclk_div7", },
>>>> +	{ .fw_name = "mpll1", },
>>>> +	{ .hw = &s4_vid_pll.hw },
>>>> +	{ .fw_name = "mpll2", },
>>>> +	{ .fw_name = "gp0_pll", },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_vpu_clkc_p0_mux  = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = CLKCTRL_VPU_CLKC_CTRL,
>>>> +		.mask = 0x7,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "vpu_clkc_p0_mux",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = s4_vpu_clkc_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data),
>>>> +		/*
>>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>>> +		 * he wants in the dts.
>>>> +		 */
>>> That's quite a lot of occurences of the same comment.
>>> At the same time, other clocks with the same flag have no comment.
>>> Please make general comment, before the Video/VPU section, explaining
>>> which clocks needs on a use-case basis (through DT) and possibly how it
>>> should be set, what should drive the choices.
>>>
>>
>> The owner of the corresponding driver module wants to have a fixed clock,
>> but I can't explain every specific reason.
> 
> Why can't you ?
> 
>> So I'm going to change it all
>> to.flags = CLK_SET_RATE_PARENT in the next version. Let CCF choose the
>> appropriate clock as you suggested. If there is a corresponding module you
>> want to change, ask him to give you a specific explanation. Do you think
>> that's all right?
> 
> If the flag is actually required and there is a reason, no it is not.
> 

I will try to communicate with each module that uses the corresponding 
clock and provide an explanation if necessary in next edition.

>>
>> I will not reply to you below.
> 
> Noted.
> 
>>
>>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>
>>>> +
>>>> +/* EMMC/NAND clock */
>>>> +static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .fw_name = "fclk_div2", },
>>>> +	{ .fw_name = "fclk_div3", },
>>>> +	{ .fw_name = "hifi_pll", },
>>>> +	{ .fw_name = "fclk_div2p5", },
>>>> +	{ .fw_name = "mpll2", },
>>>> +	{ .fw_name = "mpll3", },
>>>> +	{ .fw_name = "gp0_pll", },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_sd_emmc_c_clk0_sel = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = CLKCTRL_NAND_CLK_CTRL,
>>>> +		.mask = 0x7,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "sd_emmc_c_clk0_sel",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = s4_sd_emmc_clk0_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(s4_sd_emmc_clk0_parent_data),
>>>> +		/*
>>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>>> +		 * he wants in the dts.
>>>> +		 */
>>> I'm getting a bit suspicious about the use (and abuse ...) of this flag.
>>> I don't quite get how selecting the base PLL for MMC should be done on
>>> use-case basis and should be up the board DT ...
>>> Other SoC have all used fdiv2 so far. Do you expect this setting to be
>>> part of the dtsi SoC file ?
>>>
>>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>
>>>> +
>>>> +/* SPICC Clock */
>>>> +static const struct clk_parent_data s4_spicc_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &s4_sys_clk.hw },
>>>> +	{ .fw_name = "fclk_div4", },
>>>> +	{ .fw_name = "fclk_div3", },
>>>> +	{ .fw_name = "fclk_div2", },
>>>> +	{ .fw_name = "fclk_div5", },
>>>> +	{ .fw_name = "fclk_div7", },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_spicc0_mux = {
>>>> +	.data = &(struct clk_regmap_mux_data){
>>>> +		.offset = CLKCTRL_SPICC_CLK_CTRL,
>>>> +		.mask = 0x7,
>>>> +		.shift = 7,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data) {
>>>> +		.name = "spicc0_mux",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = s4_spicc_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(s4_spicc_parent_data),
>>>> +		/*
>>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>>> +		 * he wants in the dts.
>>>> +		 */
>>> This is getting too far. All the parent clocks are fixed.
>>> Let CCF do the job of picking the most adequate clock for the job
>>> instead of manually settings things
>>>
>>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>
>>>> +
>>>> +/* PWM Clock */
>>>> +static const struct clk_parent_data s4_pwm_parent_data[] = {
>>>> +	{ .fw_name = "xtal", },
>>>> +	{ .hw = &s4_vid_pll.hw },
>>>> +	{ .fw_name = "fclk_div4", },
>>>> +	{ .fw_name = "fclk_div3", },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_pwm_a_mux = {
>>>> +	.data = &(struct clk_regmap_mux_data) {
>>>> +		.offset = CLKCTRL_PWM_CLK_AB_CTRL,
>>>> +		.mask = 0x3,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "pwm_a_mux",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = s4_pwm_parent_data,
>>>> +		.num_parents = ARRAY_SIZE(s4_pwm_parent_data),
>>>> +		/*
>>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>>> +		 * he wants in the dts.
>>>> +		 */
>>> Same here ... this is really going to far.
>>>
>>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>> +
>>>
>>>> +
>>>> +static struct clk_regmap s4_saradc_mux = {
>>>> +	.data = &(struct clk_regmap_mux_data) {
>>>> +		.offset = CLKCTRL_SAR_CLK_CTRL,
>>>> +		.mask = 0x3,
>>>> +		.shift = 9,
>>>> +	},
>>>> +	.hw.init = &(struct clk_init_data){
>>>> +		.name = "saradc_mux",
>>>> +		.ops = &clk_regmap_mux_ops,
>>>> +		.parent_data = (const struct clk_parent_data []) {
>>>> +			{ .fw_name = "xtal", },
>>>> +			{ .hw = &s4_sys_clk.hw },
>>>> +		},
>>>> +		.num_parents = 2,
>>>> +		/*
>>>> +		 * When the driver uses this clock, needs to specify the patent clock
>>>> +		 * he wants in the dts.
>>>> +		 */
>>> For each clock type, if this flag is going to be used, I'd like a clear
>>> explanation about why it is use-case dependent and why you need manual
>>> control over this. Same applies to all the occurence.
>>>
>>>> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
>>>> +	},
>>>> +};
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Jerome Brunet]

On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2023/1/20 17:47, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> Hi
>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>
>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>> [...]
>>>>
>>>>> +
>>>>> +/* Video Clocks */
>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>> +		.val = {
>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>> +			.shift   = 0,
>>>>> +			.width   = 15,
>>>>> +		},
>>>>> +		.sel = {
>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>> +			.shift   = 16,
>>>>> +			.width   = 2,
>>>>> +		},
>>>>> +	},
>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>> +		.name = "vid_pll_div",
>>>>> +		/*
>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>> +		 * clock is the default value of this register. When designing the
>>>>> +		 * video module of the chip, a default value that can meet the
>>>>> +		 * requirements of the video module will be solidified according
>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>> +		 * simulation. So this is ro_ops.
>>>>> +		 * It is important to note that this clock is not used on this
>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>> +		 */
>>>> If it is reset value and will be applicable to all the design, regarless
>>>> of the use-case, then yes RO ops is OK
>>>>
>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>
>>> Check the previous chip history, the actual scene is not used at all,
>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>> any problems.  This S4 SOC is not actually useful either.
>>>
>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>> I could delete this useless clock, so you don't have to worry about it.
>> I don't know what to make of this. What is the point of adding a useless
>> clock ?
>
> As explained earlier this "vid_pll_div" is actually used in chip
> emulation. So next I'd like to know what you suggest to do with the clock?
>

If it does not exist in the actual SoC, please remove it

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Yu Tu]

On 2023/1/30 17:06, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> On 2023/1/20 17:47, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Hi
>>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>
>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>
>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>> [...]
>>>>>
>>>>>> +
>>>>>> +/* Video Clocks */
>>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>>> +		.val = {
>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>> +			.shift   = 0,
>>>>>> +			.width   = 15,
>>>>>> +		},
>>>>>> +		.sel = {
>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>> +			.shift   = 16,
>>>>>> +			.width   = 2,
>>>>>> +		},
>>>>>> +	},
>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>> +		.name = "vid_pll_div",
>>>>>> +		/*
>>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>>> +		 * clock is the default value of this register. When designing the
>>>>>> +		 * video module of the chip, a default value that can meet the
>>>>>> +		 * requirements of the video module will be solidified according
>>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>>> +		 * simulation. So this is ro_ops.
>>>>>> +		 * It is important to note that this clock is not used on this
>>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>>> +		 */
>>>>> If it is reset value and will be applicable to all the design, regarless
>>>>> of the use-case, then yes RO ops is OK
>>>>>
>>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>>
>>>> Check the previous chip history, the actual scene is not used at all,
>>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>>> any problems.  This S4 SOC is not actually useful either.
>>>>
>>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>>> I could delete this useless clock, so you don't have to worry about it.
>>> I don't know what to make of this. What is the point of adding a useless
>>> clock ?
>>
>> As explained earlier this "vid_pll_div" is actually used in chip
>> emulation. So next I'd like to know what you suggest to do with the clock?
>>
> 
> If it does not exist in the actual SoC, please remove it
> 

If I remove it, the "vid_pll_sel" clock will be missing a parent 
(vid_pll_div). I will use the table method and give the above reasons. 
Do you accept this method?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Jerome Brunet]

On Mon 30 Jan 2023 at 17:41, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2023/1/30 17:06, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> On 2023/1/20 17:47, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> Hi
>>>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>>>> [ EXTERNAL EMAIL ]
>>>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>
>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>
>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>>> [...]
>>>>>>
>>>>>>> +
>>>>>>> +/* Video Clocks */
>>>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>>>> +		.val = {
>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>> +			.shift   = 0,
>>>>>>> +			.width   = 15,
>>>>>>> +		},
>>>>>>> +		.sel = {
>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>> +			.shift   = 16,
>>>>>>> +			.width   = 2,
>>>>>>> +		},
>>>>>>> +	},
>>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>>> +		.name = "vid_pll_div",
>>>>>>> +		/*
>>>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>>>> +		 * clock is the default value of this register. When designing the
>>>>>>> +		 * video module of the chip, a default value that can meet the
>>>>>>> +		 * requirements of the video module will be solidified according
>>>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>>>> +		 * simulation. So this is ro_ops.
>>>>>>> +		 * It is important to note that this clock is not used on this
>>>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>>>> +		 */
>>>>>> If it is reset value and will be applicable to all the design, regarless
>>>>>> of the use-case, then yes RO ops is OK
>>>>>>
>>>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>>>
>>>>> Check the previous chip history, the actual scene is not used at all,
>>>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>>>> any problems.  This S4 SOC is not actually useful either.
>>>>>
>>>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>>>> I could delete this useless clock, so you don't have to worry about it.
>>>> I don't know what to make of this. What is the point of adding a useless
>>>> clock ?
>>>
>>> As explained earlier this "vid_pll_div" is actually used in chip
>>> emulation. So next I'd like to know what you suggest to do with the clock?
>>>
>> If it does not exist in the actual SoC, please remove it
>> 
>
> If I remove it, the "vid_pll_sel" clock will be missing a parent
> (vid_pll_div). I will use the table method and give the above reasons. Do
> you accept this method?

Either the clock exists or it does not.

If the HW actually exist, it is expected to be properly described.
If it does not, it obviously cannot be an input to another clock.

Please sort this out and make the necessary changes.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Yu Tu]

On 2023/1/30 17:47, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 30 Jan 2023 at 17:41, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> On 2023/1/30 17:06, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> On 2023/1/20 17:47, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>
>>>>>> Hi
>>>>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>>
>>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>>
>>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>>>> [...]
>>>>>>>
>>>>>>>> +
>>>>>>>> +/* Video Clocks */
>>>>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>>>>> +		.val = {
>>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>>> +			.shift   = 0,
>>>>>>>> +			.width   = 15,
>>>>>>>> +		},
>>>>>>>> +		.sel = {
>>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>>> +			.shift   = 16,
>>>>>>>> +			.width   = 2,
>>>>>>>> +		},
>>>>>>>> +	},
>>>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>>>> +		.name = "vid_pll_div",
>>>>>>>> +		/*
>>>>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>>>>> +		 * clock is the default value of this register. When designing the
>>>>>>>> +		 * video module of the chip, a default value that can meet the
>>>>>>>> +		 * requirements of the video module will be solidified according
>>>>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>>>>> +		 * simulation. So this is ro_ops.
>>>>>>>> +		 * It is important to note that this clock is not used on this
>>>>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>>>>> +		 */
>>>>>>> If it is reset value and will be applicable to all the design, regarless
>>>>>>> of the use-case, then yes RO ops is OK
>>>>>>>
>>>>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>>>>
>>>>>> Check the previous chip history, the actual scene is not used at all,
>>>>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>>>>> any problems.  This S4 SOC is not actually useful either.
>>>>>>
>>>>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>>>>> I could delete this useless clock, so you don't have to worry about it.
>>>>> I don't know what to make of this. What is the point of adding a useless
>>>>> clock ?
>>>>
>>>> As explained earlier this "vid_pll_div" is actually used in chip
>>>> emulation. So next I'd like to know what you suggest to do with the clock?
>>>>
>>> If it does not exist in the actual SoC, please remove it
>>>
>>
>> If I remove it, the "vid_pll_sel" clock will be missing a parent
>> (vid_pll_div). I will use the table method and give the above reasons. Do
>> you accept this method?
> 
> Either the clock exists or it does not.
> 
> If the HW actually exist, it is expected to be properly described.
> If it does not, it obviously cannot be an input to another clock.
> 
> Please sort this out and make the necessary changes.
> 

The CLKCTRL_VID_PLL_CLK_DIV register is actually described, but it is 
not used in the actual board. According to your reply just now, 
description is required, but I want to know how to describe it to meet 
your requirements.

Please give me some suggestions.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Jerome Brunet]

On Mon 30 Jan 2023 at 17:59, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2023/1/30 17:47, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Mon 30 Jan 2023 at 17:41, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> On 2023/1/30 17:06, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>> On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> On 2023/1/20 17:47, Jerome Brunet wrote:
>>>>>> [ EXTERNAL EMAIL ]
>>>>>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>>>
>>>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +/* Video Clocks */
>>>>>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>>>>>> +		.val = {
>>>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>>>> +			.shift   = 0,
>>>>>>>>> +			.width   = 15,
>>>>>>>>> +		},
>>>>>>>>> +		.sel = {
>>>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>>>> +			.shift   = 16,
>>>>>>>>> +			.width   = 2,
>>>>>>>>> +		},
>>>>>>>>> +	},
>>>>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>>>>> +		.name = "vid_pll_div",
>>>>>>>>> +		/*
>>>>>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>>>>>> +		 * clock is the default value of this register. When designing the
>>>>>>>>> +		 * video module of the chip, a default value that can meet the
>>>>>>>>> +		 * requirements of the video module will be solidified according
>>>>>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>>>>>> +		 * simulation. So this is ro_ops.
>>>>>>>>> +		 * It is important to note that this clock is not used on this
>>>>>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>>>>>> +		 */
>>>>>>>> If it is reset value and will be applicable to all the design, regarless
>>>>>>>> of the use-case, then yes RO ops is OK
>>>>>>>>
>>>>>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>>>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>>>>>
>>>>>>> Check the previous chip history, the actual scene is not used at all,
>>>>>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>>>>>> any problems.  This S4 SOC is not actually useful either.
>>>>>>>
>>>>>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>>>>>> I could delete this useless clock, so you don't have to worry about it.
>>>>>> I don't know what to make of this. What is the point of adding a useless
>>>>>> clock ?
>>>>>
>>>>> As explained earlier this "vid_pll_div" is actually used in chip
>>>>> emulation. So next I'd like to know what you suggest to do with the clock?
>>>>>
>>>> If it does not exist in the actual SoC, please remove it
>>>>
>>>
>>> If I remove it, the "vid_pll_sel" clock will be missing a parent
>>> (vid_pll_div). I will use the table method and give the above reasons. Do
>>> you accept this method?
>> Either the clock exists or it does not.
>> If the HW actually exist, it is expected to be properly described.
>> If it does not, it obviously cannot be an input to another clock.
>> Please sort this out and make the necessary changes.
>> 
>
> The CLKCTRL_VID_PLL_CLK_DIV register is actually described, but it is not
> used in the actual board. According to your reply just now, description is
> required, but I want to know how to describe it to meet your requirements.
>
> Please give me some suggestions.

Implementing things is NOT about usage, it is about correctness.
Either there is actually a clock in the silicon you are producing at the
Amlogic factory, or there is not. 

If the clock is there in the actual HW should be properly
described/implemented, as it "might" be used as an input to other clocks
- even if you personnaly don't.

If clock does not exists (nothing behind the registers, or broken, etc
...)  then, yes you'll need to use parent tables and document this.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Yu Tu]

On 2023/1/30 18:07, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 30 Jan 2023 at 17:59, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> On 2023/1/30 17:47, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Mon 30 Jan 2023 at 17:41, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> On 2023/1/30 17:06, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>> On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>
>>>>>> On 2023/1/20 17:47, Jerome Brunet wrote:
>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>>>>
>>>>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +/* Video Clocks */
>>>>>>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>>>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>>>>>>> +		.val = {
>>>>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>>>>> +			.shift   = 0,
>>>>>>>>>> +			.width   = 15,
>>>>>>>>>> +		},
>>>>>>>>>> +		.sel = {
>>>>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>>>>> +			.shift   = 16,
>>>>>>>>>> +			.width   = 2,
>>>>>>>>>> +		},
>>>>>>>>>> +	},
>>>>>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>>>>>> +		.name = "vid_pll_div",
>>>>>>>>>> +		/*
>>>>>>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>>>>>>> +		 * clock is the default value of this register. When designing the
>>>>>>>>>> +		 * video module of the chip, a default value that can meet the
>>>>>>>>>> +		 * requirements of the video module will be solidified according
>>>>>>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>>>>>>> +		 * simulation. So this is ro_ops.
>>>>>>>>>> +		 * It is important to note that this clock is not used on this
>>>>>>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>>>>>>> +		 */
>>>>>>>>> If it is reset value and will be applicable to all the design, regarless
>>>>>>>>> of the use-case, then yes RO ops is OK
>>>>>>>>>
>>>>>>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>>>>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>>>>>>
>>>>>>>> Check the previous chip history, the actual scene is not used at all,
>>>>>>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>>>>>>> any problems.  This S4 SOC is not actually useful either.
>>>>>>>>
>>>>>>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>>>>>>> I could delete this useless clock, so you don't have to worry about it.
>>>>>>> I don't know what to make of this. What is the point of adding a useless
>>>>>>> clock ?
>>>>>>
>>>>>> As explained earlier this "vid_pll_div" is actually used in chip
>>>>>> emulation. So next I'd like to know what you suggest to do with the clock?
>>>>>>
>>>>> If it does not exist in the actual SoC, please remove it
>>>>>
>>>>
>>>> If I remove it, the "vid_pll_sel" clock will be missing a parent
>>>> (vid_pll_div). I will use the table method and give the above reasons. Do
>>>> you accept this method?
>>> Either the clock exists or it does not.
>>> If the HW actually exist, it is expected to be properly described.
>>> If it does not, it obviously cannot be an input to another clock.
>>> Please sort this out and make the necessary changes.
>>>
>>
>> The CLKCTRL_VID_PLL_CLK_DIV register is actually described, but it is not
>> used in the actual board. According to your reply just now, description is
>> required, but I want to know how to describe it to meet your requirements.
>>
>> Please give me some suggestions.
> 
> Implementing things is NOT about usage, it is about correctness.
> Either there is actually a clock in the silicon you are producing at the
> Amlogic factory, or there is not.
> 
> If the clock is there in the actual HW should be properly
> described/implemented, as it "might" be used as an input to other clocks
> - even if you personnaly don't.
> 
> If clock does not exists (nothing behind the registers, or broken, etc
> ...)  then, yes you'll need to use parent tables and document this.
> 

According to your suggestion, we need to describe the clock 
(vid_pll_div).So it seems like we need to implement 
"meson_vid_pll_div_ops" and do a commit first. Then submit the S4 SOC 
clock driver. So change whether you take it or not?

Or if you have a better idea, let me know.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel