[PATCH v7 0/3] Add T-HEAD TH1520 VO clock support for LicheePi 4A GPU enablement

[PATCH v7 0/3] Add T-HEAD TH1520 VO clock support for LicheePi 4A GPU enablement

[Michal Wilczynski]

This is a subset of a larger patch series enabling the Imagination BXM-4-64 GPU
on the LicheePi 4A board, which is powered by the T-HEAD TH1520 SoC. While the
full series includes power-domain, reset, and firmware changes, this part
focuses solely on the clock subsystem needed for the GPU and other VO (video
output) blocks. By merging these clock patches independently, we prepare the
groundwork for future GPU integration via the `drm/imagination` driver.

The T-HEAD TH1520 SoC features multiple clock controllers. Initially, only the
AP clock controller was supported upstream. The patches below add support for
the VO (video output) clock controller, which manages GPU-related gates, HDMI,
and other multimedia clocks.

Bigger series cover letter:
https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/

v7:
- remove commits 3,4 from the patch series, those would handle empty MEM clock
  stub, and reset management. It's not necessary anymore, as this would be
  implemented in power-domain driver
- added the device tree patch at the end for the SoC maintainers to take after
  the other patches get OK-ed
- added Acked-by, from Connor for the dt-binding patch
- re-added Reviewed-by from Krzysztof, as the dt-binding patch is the same as
  for the v5

v6:
- squashed the "dt-bindings: clock: thead: Add GPU clkgen reset property"
  with the "dt-bindings: clock: thead: Add TH1520 VO clock controller". As
  a result, also removed the Reviewed-by from Krzysztof, since the new
  resets property has been introduced, which is mandatory in the VO
  case

v5:
- introduced a new macro CCU_GATE_CLK_OPS, which allows providing custom clk_ops.
  In the case of the 'MEM' clock, it provides empty clk_nops. Later, this clock
  is provided to the GPU node, thereby avoiding any ABI breakage
- used the CCU_GATE_CLK_OPS macro to implement a workaround for de-asserting
  the clkgen reset only after both core and sys clocks are enabled. This
  sequence is required to properly initialize the GPU

v4:
 - enhanced documentation for new Video Output (VO) clock inputs in device tree
   bindings

v3:
 - reworked driver to support multiple clock controllers through .compatible
   and .data instead of using multiple address spaces in dt-binding. This change
   allows to re-use the driver code for multiple clock controllers

v2:
 - removed AP_SUBSYS clock refactoring commits (1-6):
 - instead of refactoring, I opted to extend the current driver and its
   associated device tree node to include support for a second address space.
 - resolved all checkpatch issues using --strict, except for the call to
   devm_clk_hw_register_gate_parent_data().  The current implementation remains
   preferable in this context, and clang-format aligns with this choice

Michal Wilczynski (3):
  dt-bindings: clock: thead: Add TH1520 VO clock controller
  clk: thead: Add clock support for VO subsystem in T-HEAD TH1520 SoC
  riscv: dts: thead: Add device tree VO clock controller

 .../bindings/clock/thead,th1520-clk-ap.yaml   |  17 +-
 arch/riscv/boot/dts/thead/th1520.dtsi         |   7 +
 drivers/clk/thead/clk-th1520-ap.c             | 196 +++++++++++++++---
 .../dt-bindings/clock/thead,th1520-clk-ap.h   |  34 +++
 4 files changed, 223 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH v7 1/3] dt-bindings: clock: thead: Add TH1520 VO clock controller

[Michal Wilczynski]

Add device tree bindings for the TH1520 Video Output (VO) subsystem
clock controller. The VO sub-system manages clock gates for multimedia
components including HDMI, MIPI, and GPU.

Document the VIDEO_PLL requirements for the VO clock controller, which
receives its input from the AP clock controller. The VIDEO_PLL is a
Silicon Creations Sigma-Delta (integer) PLL typically running at 792 MHz
with maximum FOUTVCO of 2376 MHz.

This binding complements the existing AP sub-system clock controller
which manages CPU, DPU, GMAC and TEE PLLs.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/clock/thead,th1520-clk-ap.yaml   | 17 ++++++++--
 .../dt-bindings/clock/thead,th1520-clk-ap.h   | 34 +++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
index 0129bd0ba4b3..9d058c00ab3d 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
@@ -8,7 +8,8 @@ title: T-HEAD TH1520 AP sub-system clock controller
 
 description: |
   The T-HEAD TH1520 AP sub-system clock controller configures the
-  CPU, DPU, GMAC and TEE PLLs.
+  CPU, DPU, GMAC and TEE PLLs. Additionally the VO subsystem configures
+  the clock gates for the HDMI, MIPI and the GPU.
 
   SoC reference manual
   https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
@@ -20,14 +21,24 @@ maintainers:
 
 properties:
   compatible:
-    const: thead,th1520-clk-ap
+    enum:
+      - thead,th1520-clk-ap
+      - thead,th1520-clk-vo
 
   reg:
     maxItems: 1
 
   clocks:
     items:
-      - description: main oscillator (24MHz)
+      - description: |
+          One input clock:
+          - For "thead,th1520-clk-ap": the clock input must be the 24 MHz
+            main oscillator.
+          - For "thead,th1520-clk-vo": the clock input must be the VIDEO_PLL,
+            which is configured by the AP clock controller. According to the
+            TH1520 manual, VIDEO_PLL is a Silicon Creations Sigma-Delta PLL
+            (integer PLL) typically running at 792 MHz (FOUTPOSTDIV), with
+            a maximum FOUTVCO of 2376 MHz.
 
   "#clock-cells":
     const: 1
diff --git a/include/dt-bindings/clock/thead,th1520-clk-ap.h b/include/dt-bindings/clock/thead,th1520-clk-ap.h
index a199784b3512..09a9aa7b3ab1 100644
--- a/include/dt-bindings/clock/thead,th1520-clk-ap.h
+++ b/include/dt-bindings/clock/thead,th1520-clk-ap.h
@@ -93,4 +93,38 @@
 #define CLK_SRAM3		83
 #define CLK_PLL_GMAC_100M	84
 #define CLK_UART_SCLK		85
+
+/* VO clocks */
+#define CLK_AXI4_VO_ACLK		0
+#define CLK_GPU_MEM			1
+#define CLK_GPU_CORE			2
+#define CLK_GPU_CFG_ACLK		3
+#define CLK_DPU_PIXELCLK0		4
+#define CLK_DPU_PIXELCLK1		5
+#define CLK_DPU_HCLK			6
+#define CLK_DPU_ACLK			7
+#define CLK_DPU_CCLK			8
+#define CLK_HDMI_SFR			9
+#define CLK_HDMI_PCLK			10
+#define CLK_HDMI_CEC			11
+#define CLK_MIPI_DSI0_PCLK		12
+#define CLK_MIPI_DSI1_PCLK		13
+#define CLK_MIPI_DSI0_CFG		14
+#define CLK_MIPI_DSI1_CFG		15
+#define CLK_MIPI_DSI0_REFCLK		16
+#define CLK_MIPI_DSI1_REFCLK		17
+#define CLK_HDMI_I2S			18
+#define CLK_X2H_DPU1_ACLK		19
+#define CLK_X2H_DPU_ACLK		20
+#define CLK_AXI4_VO_PCLK		21
+#define CLK_IOPMP_VOSYS_DPU_PCLK	22
+#define CLK_IOPMP_VOSYS_DPU1_PCLK	23
+#define CLK_IOPMP_VOSYS_GPU_PCLK	24
+#define CLK_IOPMP_DPU1_ACLK		25
+#define CLK_IOPMP_DPU_ACLK		26
+#define CLK_IOPMP_GPU_ACLK		27
+#define CLK_MIPIDSI0_PIXCLK		28
+#define CLK_MIPIDSI1_PIXCLK		29
+#define CLK_HDMI_PIXCLK			30
+
 #endif
-- 
2.34.1

[PATCH v7 2/3] clk: thead: Add clock support for VO subsystem in T-HEAD TH1520 SoC

[Michal Wilczynski]

The T-Head TH1520 SoC integrates a variety of clocks for its subsystems,
including the Application Processor (AP) and the Video Output (VO) [1].
Up until now, the T-Head clock driver only supported AP clocks.

Extend the driver to provide clock functionality for the VO subsystem.
At this stage, the focus is on implementing the VO clock gates, as these
are currently the most relevant and required components for enabling and
disabling the VO subsystem functionality.  Future enhancements may
introduce additional VO-related clocks as necessary.

Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/clk/thead/clk-th1520-ap.c | 196 +++++++++++++++++++++++++-----
 1 file changed, 168 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index 4c9555fc6184..ebfb1d59401d 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -847,6 +847,67 @@ static CCU_GATE(CLK_SRAM1, sram1_clk, "sram1", axi_aclk_pd, 0x20c, BIT(3), 0);
 static CCU_GATE(CLK_SRAM2, sram2_clk, "sram2", axi_aclk_pd, 0x20c, BIT(2), 0);
 static CCU_GATE(CLK_SRAM3, sram3_clk, "sram3", axi_aclk_pd, 0x20c, BIT(1), 0);
 
+static CCU_GATE(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
+		video_pll_clk_pd, 0x0, BIT(0), 0);
+static CCU_GATE(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_clk_pd,
+		0x0, BIT(3), 0);
+static CCU_GATE(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
+		video_pll_clk_pd, 0x0, BIT(4), 0);
+static CCU_GATE(CLK_DPU_PIXELCLK0, dpu0_pixelclk, "dpu0-pixelclk",
+		video_pll_clk_pd, 0x0, BIT(5), 0);
+static CCU_GATE(CLK_DPU_PIXELCLK1, dpu1_pixelclk, "dpu1-pixelclk",
+		video_pll_clk_pd, 0x0, BIT(6), 0);
+static CCU_GATE(CLK_DPU_HCLK, dpu_hclk, "dpu-hclk", video_pll_clk_pd, 0x0,
+		BIT(7), 0);
+static CCU_GATE(CLK_DPU_ACLK, dpu_aclk, "dpu-aclk", video_pll_clk_pd, 0x0,
+		BIT(8), 0);
+static CCU_GATE(CLK_DPU_CCLK, dpu_cclk, "dpu-cclk", video_pll_clk_pd, 0x0,
+		BIT(9), 0);
+static CCU_GATE(CLK_HDMI_SFR, hdmi_sfr_clk, "hdmi-sfr-clk", video_pll_clk_pd,
+		0x0, BIT(10), 0);
+static CCU_GATE(CLK_HDMI_PCLK, hdmi_pclk, "hdmi-pclk", video_pll_clk_pd, 0x0,
+		BIT(11), 0);
+static CCU_GATE(CLK_HDMI_CEC, hdmi_cec_clk, "hdmi-cec-clk", video_pll_clk_pd,
+		0x0, BIT(12), 0);
+static CCU_GATE(CLK_MIPI_DSI0_PCLK, mipi_dsi0_pclk, "mipi-dsi0-pclk",
+		video_pll_clk_pd, 0x0, BIT(13), 0);
+static CCU_GATE(CLK_MIPI_DSI1_PCLK, mipi_dsi1_pclk, "mipi-dsi1-pclk",
+		video_pll_clk_pd, 0x0, BIT(14), 0);
+static CCU_GATE(CLK_MIPI_DSI0_CFG, mipi_dsi0_cfg_clk, "mipi-dsi0-cfg-clk",
+		video_pll_clk_pd, 0x0, BIT(15), 0);
+static CCU_GATE(CLK_MIPI_DSI1_CFG, mipi_dsi1_cfg_clk, "mipi-dsi1-cfg-clk",
+		video_pll_clk_pd, 0x0, BIT(16), 0);
+static CCU_GATE(CLK_MIPI_DSI0_REFCLK, mipi_dsi0_refclk, "mipi-dsi0-refclk",
+		video_pll_clk_pd, 0x0, BIT(17), 0);
+static CCU_GATE(CLK_MIPI_DSI1_REFCLK, mipi_dsi1_refclk, "mipi-dsi1-refclk",
+		video_pll_clk_pd, 0x0, BIT(18), 0);
+static CCU_GATE(CLK_HDMI_I2S, hdmi_i2s_clk, "hdmi-i2s-clk", video_pll_clk_pd,
+		0x0, BIT(19), 0);
+static CCU_GATE(CLK_X2H_DPU1_ACLK, x2h_dpu1_aclk, "x2h-dpu1-aclk",
+		video_pll_clk_pd, 0x0, BIT(20), 0);
+static CCU_GATE(CLK_X2H_DPU_ACLK, x2h_dpu_aclk, "x2h-dpu-aclk",
+		video_pll_clk_pd, 0x0, BIT(21), 0);
+static CCU_GATE(CLK_AXI4_VO_PCLK, axi4_vo_pclk, "axi4-vo-pclk",
+		video_pll_clk_pd, 0x0, BIT(22), 0);
+static CCU_GATE(CLK_IOPMP_VOSYS_DPU_PCLK, iopmp_vosys_dpu_pclk,
+		"iopmp-vosys-dpu-pclk", video_pll_clk_pd, 0x0, BIT(23), 0);
+static CCU_GATE(CLK_IOPMP_VOSYS_DPU1_PCLK, iopmp_vosys_dpu1_pclk,
+		"iopmp-vosys-dpu1-pclk", video_pll_clk_pd, 0x0, BIT(24), 0);
+static CCU_GATE(CLK_IOPMP_VOSYS_GPU_PCLK, iopmp_vosys_gpu_pclk,
+		"iopmp-vosys-gpu-pclk", video_pll_clk_pd, 0x0, BIT(25), 0);
+static CCU_GATE(CLK_IOPMP_DPU1_ACLK, iopmp_dpu1_aclk, "iopmp-dpu1-aclk",
+		video_pll_clk_pd, 0x0, BIT(27), 0);
+static CCU_GATE(CLK_IOPMP_DPU_ACLK, iopmp_dpu_aclk, "iopmp-dpu-aclk",
+		video_pll_clk_pd, 0x0, BIT(28), 0);
+static CCU_GATE(CLK_IOPMP_GPU_ACLK, iopmp_gpu_aclk, "iopmp-gpu-aclk",
+		video_pll_clk_pd, 0x0, BIT(29), 0);
+static CCU_GATE(CLK_MIPIDSI0_PIXCLK, mipi_dsi0_pixclk, "mipi-dsi0-pixclk",
+		video_pll_clk_pd, 0x0, BIT(30), 0);
+static CCU_GATE(CLK_MIPIDSI1_PIXCLK, mipi_dsi1_pixclk, "mipi-dsi1-pixclk",
+		video_pll_clk_pd, 0x0, BIT(31), 0);
+static CCU_GATE(CLK_HDMI_PIXCLK, hdmi_pixclk, "hdmi-pixclk", video_pll_clk_pd,
+		0x4, BIT(0), 0);
+
 static CLK_FIXED_FACTOR_HW(gmac_pll_clk_100m, "gmac-pll-clk-100m",
 			   &gmac_pll_clk.common.hw, 10, 1, 0);
 
@@ -963,7 +1024,38 @@ static struct ccu_common *th1520_gate_clks[] = {
 	&sram3_clk.common,
 };
 
-#define NR_CLKS	(CLK_UART_SCLK + 1)
+static struct ccu_common *th1520_vo_gate_clks[] = {
+	&axi4_vo_aclk.common,
+	&gpu_core_clk.common,
+	&gpu_cfg_aclk.common,
+	&dpu0_pixelclk.common,
+	&dpu1_pixelclk.common,
+	&dpu_hclk.common,
+	&dpu_aclk.common,
+	&dpu_cclk.common,
+	&hdmi_sfr_clk.common,
+	&hdmi_pclk.common,
+	&hdmi_cec_clk.common,
+	&mipi_dsi0_pclk.common,
+	&mipi_dsi1_pclk.common,
+	&mipi_dsi0_cfg_clk.common,
+	&mipi_dsi1_cfg_clk.common,
+	&mipi_dsi0_refclk.common,
+	&mipi_dsi1_refclk.common,
+	&hdmi_i2s_clk.common,
+	&x2h_dpu1_aclk.common,
+	&x2h_dpu_aclk.common,
+	&axi4_vo_pclk.common,
+	&iopmp_vosys_dpu_pclk.common,
+	&iopmp_vosys_dpu1_pclk.common,
+	&iopmp_vosys_gpu_pclk.common,
+	&iopmp_dpu1_aclk.common,
+	&iopmp_dpu_aclk.common,
+	&iopmp_gpu_aclk.common,
+	&mipi_dsi0_pixclk.common,
+	&mipi_dsi1_pixclk.common,
+	&hdmi_pixclk.common
+};
 
 static const struct regmap_config th1520_clk_regmap_config = {
 	.reg_bits = 32,
@@ -972,8 +1064,44 @@ static const struct regmap_config th1520_clk_regmap_config = {
 	.fast_io = true,
 };
 
+struct th1520_plat_data {
+	struct ccu_common **th1520_pll_clks;
+	struct ccu_common **th1520_div_clks;
+	struct ccu_common **th1520_mux_clks;
+	struct ccu_common **th1520_gate_clks;
+
+	int nr_clks;
+	int nr_pll_clks;
+	int nr_div_clks;
+	int nr_mux_clks;
+	int nr_gate_clks;
+};
+
+static const struct th1520_plat_data th1520_ap_platdata = {
+	.th1520_pll_clks = th1520_pll_clks,
+	.th1520_div_clks = th1520_div_clks,
+	.th1520_mux_clks = th1520_mux_clks,
+	.th1520_gate_clks = th1520_gate_clks,
+
+	.nr_clks = CLK_UART_SCLK + 1,
+
+	.nr_pll_clks = ARRAY_SIZE(th1520_pll_clks),
+	.nr_div_clks = ARRAY_SIZE(th1520_div_clks),
+	.nr_mux_clks = ARRAY_SIZE(th1520_mux_clks),
+	.nr_gate_clks = ARRAY_SIZE(th1520_gate_clks),
+};
+
+static const struct th1520_plat_data th1520_vo_platdata = {
+	.th1520_gate_clks = th1520_vo_gate_clks,
+
+	.nr_clks = CLK_HDMI_PIXCLK + 1,
+
+	.nr_gate_clks = ARRAY_SIZE(th1520_vo_gate_clks),
+};
+
 static int th1520_clk_probe(struct platform_device *pdev)
 {
+	const struct th1520_plat_data *plat_data;
 	struct device *dev = &pdev->dev;
 	struct clk_hw_onecell_data *priv;
 
@@ -982,11 +1110,16 @@ static int th1520_clk_probe(struct platform_device *pdev)
 	struct clk_hw *hw;
 	int ret, i;
 
-	priv = devm_kzalloc(dev, struct_size(priv, hws, NR_CLKS), GFP_KERNEL);
+	plat_data = device_get_match_data(&pdev->dev);
+	if (!plat_data)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "No device match data found\n");
+
+	priv = devm_kzalloc(dev, struct_size(priv, hws, plat_data->nr_clks), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	priv->num = NR_CLKS;
+	priv->num = plat_data->nr_clks;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -996,35 +1129,35 @@ static int th1520_clk_probe(struct platform_device *pdev)
 	if (IS_ERR(map))
 		return PTR_ERR(map);
 
-	for (i = 0; i < ARRAY_SIZE(th1520_pll_clks); i++) {
-		struct ccu_pll *cp = hw_to_ccu_pll(&th1520_pll_clks[i]->hw);
+	for (i = 0; i < plat_data->nr_pll_clks; i++) {
+		struct ccu_pll *cp = hw_to_ccu_pll(&plat_data->th1520_pll_clks[i]->hw);
 
-		th1520_pll_clks[i]->map = map;
+		plat_data->th1520_pll_clks[i]->map = map;
 
-		ret = devm_clk_hw_register(dev, &th1520_pll_clks[i]->hw);
+		ret = devm_clk_hw_register(dev, &plat_data->th1520_pll_clks[i]->hw);
 		if (ret)
 			return ret;
 
 		priv->hws[cp->common.clkid] = &cp->common.hw;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(th1520_div_clks); i++) {
-		struct ccu_div *cd = hw_to_ccu_div(&th1520_div_clks[i]->hw);
+	for (i = 0; i < plat_data->nr_div_clks; i++) {
+		struct ccu_div *cd = hw_to_ccu_div(&plat_data->th1520_div_clks[i]->hw);
 
-		th1520_div_clks[i]->map = map;
+		plat_data->th1520_div_clks[i]->map = map;
 
-		ret = devm_clk_hw_register(dev, &th1520_div_clks[i]->hw);
+		ret = devm_clk_hw_register(dev, &plat_data->th1520_div_clks[i]->hw);
 		if (ret)
 			return ret;
 
 		priv->hws[cd->common.clkid] = &cd->common.hw;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(th1520_mux_clks); i++) {
-		struct ccu_mux *cm = hw_to_ccu_mux(&th1520_mux_clks[i]->hw);
+	for (i = 0; i < plat_data->nr_mux_clks; i++) {
+		struct ccu_mux *cm = hw_to_ccu_mux(&plat_data->th1520_mux_clks[i]->hw);
 		const struct clk_init_data *init = cm->common.hw.init;
 
-		th1520_mux_clks[i]->map = map;
+		plat_data->th1520_mux_clks[i]->map = map;
 		hw = devm_clk_hw_register_mux_parent_data_table(dev,
 								init->name,
 								init->parent_data,
@@ -1040,10 +1173,10 @@ static int th1520_clk_probe(struct platform_device *pdev)
 		priv->hws[cm->common.clkid] = hw;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(th1520_gate_clks); i++) {
-		struct ccu_gate *cg = hw_to_ccu_gate(&th1520_gate_clks[i]->hw);
+	for (i = 0; i < plat_data->nr_gate_clks; i++) {
+		struct ccu_gate *cg = hw_to_ccu_gate(&plat_data->th1520_gate_clks[i]->hw);
 
-		th1520_gate_clks[i]->map = map;
+		plat_data->th1520_gate_clks[i]->map = map;
 
 		hw = devm_clk_hw_register_gate_parent_data(dev,
 							   cg->common.hw.init->name,
@@ -1057,19 +1190,21 @@ static int th1520_clk_probe(struct platform_device *pdev)
 		priv->hws[cg->common.clkid] = hw;
 	}
 
-	ret = devm_clk_hw_register(dev, &osc12m_clk.hw);
-	if (ret)
-		return ret;
-	priv->hws[CLK_OSC12M] = &osc12m_clk.hw;
+	if (plat_data == &th1520_ap_platdata) {
+		ret = devm_clk_hw_register(dev, &osc12m_clk.hw);
+		if (ret)
+			return ret;
+		priv->hws[CLK_OSC12M] = &osc12m_clk.hw;
 
-	ret = devm_clk_hw_register(dev, &gmac_pll_clk_100m.hw);
-	if (ret)
-		return ret;
-	priv->hws[CLK_PLL_GMAC_100M] = &gmac_pll_clk_100m.hw;
+		ret = devm_clk_hw_register(dev, &gmac_pll_clk_100m.hw);
+		if (ret)
+			return ret;
+		priv->hws[CLK_PLL_GMAC_100M] = &gmac_pll_clk_100m.hw;
 
-	ret = devm_clk_hw_register(dev, &emmc_sdio_ref_clk.hw);
-	if (ret)
-		return ret;
+		ret = devm_clk_hw_register(dev, &emmc_sdio_ref_clk.hw);
+		if (ret)
+			return ret;
+	}
 
 	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv);
 	if (ret)
@@ -1081,6 +1216,11 @@ static int th1520_clk_probe(struct platform_device *pdev)
 static const struct of_device_id th1520_clk_match[] = {
 	{
 		.compatible = "thead,th1520-clk-ap",
+		.data = &th1520_ap_platdata,
+	},
+	{
+		.compatible = "thead,th1520-clk-vo",
+		.data = &th1520_vo_platdata,
 	},
 	{ /* sentinel */ },
 };
-- 
2.34.1

[PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Michal Wilczynski]

VO clocks reside in a different address space from the AP clocks on the
T-HEAD SoC. Add the device tree node of a clock-controller to handle
VO address space as well.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 527336417765..d4cba0713cab 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
+		clk_vo: clock-controller@ffef528050 {
+			compatible = "thead,th1520-clk-vo";
+			reg = <0xff 0xef528050 0x0 0xfb0>;
+			clocks = <&clk CLK_VIDEO_PLL>;
+			#clock-cells = <1>;
+		};
+
 		dmac0: dma-controller@ffefc00000 {
 			compatible = "snps,axi-dma-1.01a";
 			reg = <0xff 0xefc00000 0x0 0x1000>;
-- 
2.34.1

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Drew Fustini]

On Thu, Apr 03, 2025 at 11:44:25AM +0200, Michal Wilczynski wrote:
> VO clocks reside in a different address space from the AP clocks on the
> T-HEAD SoC. Add the device tree node of a clock-controller to handle
> VO address space as well.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 527336417765..d4cba0713cab 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
>  			#clock-cells = <1>;
>  		};
>  
> +		clk_vo: clock-controller@ffef528050 {
> +			compatible = "thead,th1520-clk-vo";
> +			reg = <0xff 0xef528050 0x0 0xfb0>;

Thanks for your patch. It is great to have more of the clocks supported
upstream.

The TH1520 System User Manual shows 0xFF_EF52_8000 for VO_SUBSYS on page
205. Is there a reason you decided to use 0xFF_EF52_8050 as the base?

I see on page 213 that the first register for VO_SUBSYS starts with
VOSYS_CLK_GATE at offset 0x50. I figure you did this to have the
CCU_GATE macros use offset of 0x0 instead 0x50.

I kind of think the reg property using the actual base address
(0xFF_EF52_8000) makes more sense as that's a closer match to the tables
in the manual. But I don't have a strong preference if you think think
using 0xef528050 makes the CCU_GATE macros easier to read.

-Drew

Re: [PATCH v7 2/3] clk: thead: Add clock support for VO subsystem in T-HEAD TH1520 SoC

[Drew Fustini]

On Thu, Apr 03, 2025 at 11:44:24AM +0200, Michal Wilczynski wrote:
> The T-Head TH1520 SoC integrates a variety of clocks for its subsystems,
> including the Application Processor (AP) and the Video Output (VO) [1].
> Up until now, the T-Head clock driver only supported AP clocks.
> 
> Extend the driver to provide clock functionality for the VO subsystem.
> At this stage, the focus is on implementing the VO clock gates, as these
> are currently the most relevant and required components for enabling and
> disabling the VO subsystem functionality.  Future enhancements may
> introduce additional VO-related clocks as necessary.
> 
> Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/clk/thead/clk-th1520-ap.c | 196 +++++++++++++++++++++++++-----
>  1 file changed, 168 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index 4c9555fc6184..ebfb1d59401d 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -847,6 +847,67 @@ static CCU_GATE(CLK_SRAM1, sram1_clk, "sram1", axi_aclk_pd, 0x20c, BIT(3), 0);
>  static CCU_GATE(CLK_SRAM2, sram2_clk, "sram2", axi_aclk_pd, 0x20c, BIT(2), 0);
>  static CCU_GATE(CLK_SRAM3, sram3_clk, "sram3", axi_aclk_pd, 0x20c, BIT(1), 0);
>  
> +static CCU_GATE(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
> +		video_pll_clk_pd, 0x0, BIT(0), 0);

Is CLKCTRL_GPU_MEM_CLK_EN (bit 2) skipped on purpose?

> +static CCU_GATE(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_clk_pd,
> +		0x0, BIT(3), 0);
> +static CCU_GATE(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
> +		video_pll_clk_pd, 0x0, BIT(4), 0);
> +static CCU_GATE(CLK_DPU_PIXELCLK0, dpu0_pixelclk, "dpu0-pixelclk",
> +		video_pll_clk_pd, 0x0, BIT(5), 0);
> +static CCU_GATE(CLK_DPU_PIXELCLK1, dpu1_pixelclk, "dpu1-pixelclk",
> +		video_pll_clk_pd, 0x0, BIT(6), 0);
> +static CCU_GATE(CLK_DPU_HCLK, dpu_hclk, "dpu-hclk", video_pll_clk_pd, 0x0,
> +		BIT(7), 0);
> +static CCU_GATE(CLK_DPU_ACLK, dpu_aclk, "dpu-aclk", video_pll_clk_pd, 0x0,
> +		BIT(8), 0);
> +static CCU_GATE(CLK_DPU_CCLK, dpu_cclk, "dpu-cclk", video_pll_clk_pd, 0x0,
> +		BIT(9), 0);
> +static CCU_GATE(CLK_HDMI_SFR, hdmi_sfr_clk, "hdmi-sfr-clk", video_pll_clk_pd,
> +		0x0, BIT(10), 0);
> +static CCU_GATE(CLK_HDMI_PCLK, hdmi_pclk, "hdmi-pclk", video_pll_clk_pd, 0x0,
> +		BIT(11), 0);
> +static CCU_GATE(CLK_HDMI_CEC, hdmi_cec_clk, "hdmi-cec-clk", video_pll_clk_pd,
> +		0x0, BIT(12), 0);
> +static CCU_GATE(CLK_MIPI_DSI0_PCLK, mipi_dsi0_pclk, "mipi-dsi0-pclk",
> +		video_pll_clk_pd, 0x0, BIT(13), 0);
> +static CCU_GATE(CLK_MIPI_DSI1_PCLK, mipi_dsi1_pclk, "mipi-dsi1-pclk",
> +		video_pll_clk_pd, 0x0, BIT(14), 0);
> +static CCU_GATE(CLK_MIPI_DSI0_CFG, mipi_dsi0_cfg_clk, "mipi-dsi0-cfg-clk",
> +		video_pll_clk_pd, 0x0, BIT(15), 0);
> +static CCU_GATE(CLK_MIPI_DSI1_CFG, mipi_dsi1_cfg_clk, "mipi-dsi1-cfg-clk",
> +		video_pll_clk_pd, 0x0, BIT(16), 0);
> +static CCU_GATE(CLK_MIPI_DSI0_REFCLK, mipi_dsi0_refclk, "mipi-dsi0-refclk",
> +		video_pll_clk_pd, 0x0, BIT(17), 0);
> +static CCU_GATE(CLK_MIPI_DSI1_REFCLK, mipi_dsi1_refclk, "mipi-dsi1-refclk",
> +		video_pll_clk_pd, 0x0, BIT(18), 0);
> +static CCU_GATE(CLK_HDMI_I2S, hdmi_i2s_clk, "hdmi-i2s-clk", video_pll_clk_pd,
> +		0x0, BIT(19), 0);
> +static CCU_GATE(CLK_X2H_DPU1_ACLK, x2h_dpu1_aclk, "x2h-dpu1-aclk",
> +		video_pll_clk_pd, 0x0, BIT(20), 0);
> +static CCU_GATE(CLK_X2H_DPU_ACLK, x2h_dpu_aclk, "x2h-dpu-aclk",
> +		video_pll_clk_pd, 0x0, BIT(21), 0);
> +static CCU_GATE(CLK_AXI4_VO_PCLK, axi4_vo_pclk, "axi4-vo-pclk",
> +		video_pll_clk_pd, 0x0, BIT(22), 0);
> +static CCU_GATE(CLK_IOPMP_VOSYS_DPU_PCLK, iopmp_vosys_dpu_pclk,
> +		"iopmp-vosys-dpu-pclk", video_pll_clk_pd, 0x0, BIT(23), 0);
> +static CCU_GATE(CLK_IOPMP_VOSYS_DPU1_PCLK, iopmp_vosys_dpu1_pclk,
> +		"iopmp-vosys-dpu1-pclk", video_pll_clk_pd, 0x0, BIT(24), 0);
> +static CCU_GATE(CLK_IOPMP_VOSYS_GPU_PCLK, iopmp_vosys_gpu_pclk,
> +		"iopmp-vosys-gpu-pclk", video_pll_clk_pd, 0x0, BIT(25), 0);
> +static CCU_GATE(CLK_IOPMP_DPU1_ACLK, iopmp_dpu1_aclk, "iopmp-dpu1-aclk",
> +		video_pll_clk_pd, 0x0, BIT(27), 0);
> +static CCU_GATE(CLK_IOPMP_DPU_ACLK, iopmp_dpu_aclk, "iopmp-dpu-aclk",
> +		video_pll_clk_pd, 0x0, BIT(28), 0);
> +static CCU_GATE(CLK_IOPMP_GPU_ACLK, iopmp_gpu_aclk, "iopmp-gpu-aclk",
> +		video_pll_clk_pd, 0x0, BIT(29), 0);
> +static CCU_GATE(CLK_MIPIDSI0_PIXCLK, mipi_dsi0_pixclk, "mipi-dsi0-pixclk",
> +		video_pll_clk_pd, 0x0, BIT(30), 0);
> +static CCU_GATE(CLK_MIPIDSI1_PIXCLK, mipi_dsi1_pixclk, "mipi-dsi1-pixclk",
> +		video_pll_clk_pd, 0x0, BIT(31), 0);
> +static CCU_GATE(CLK_HDMI_PIXCLK, hdmi_pixclk, "hdmi-pixclk", video_pll_clk_pd,
> +		0x4, BIT(0), 0);

Did you intentionally skip VOSYS_DPU_CCLK_CFG.VOSYS_DPU_CCLK_CFG and
TEST_CLK_CFG.TEST_CLK_CFG as they aren't needed?


Thanks,
Drew

Re: [PATCH v7 1/3] dt-bindings: clock: thead: Add TH1520 VO clock controller

[Drew Fustini]

On Thu, Apr 03, 2025 at 11:44:23AM +0200, Michal Wilczynski wrote:
> Add device tree bindings for the TH1520 Video Output (VO) subsystem
> clock controller. The VO sub-system manages clock gates for multimedia
> components including HDMI, MIPI, and GPU.
> 
> Document the VIDEO_PLL requirements for the VO clock controller, which
> receives its input from the AP clock controller. The VIDEO_PLL is a
> Silicon Creations Sigma-Delta (integer) PLL typically running at 792 MHz
> with maximum FOUTVCO of 2376 MHz.
> 
> This binding complements the existing AP sub-system clock controller
> which manages CPU, DPU, GMAC and TEE PLLs.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 17 ++++++++--
>  .../dt-bindings/clock/thead,th1520-clk-ap.h   | 34 +++++++++++++++++++
>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> index 0129bd0ba4b3..9d058c00ab3d 100644
> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> @@ -8,7 +8,8 @@ title: T-HEAD TH1520 AP sub-system clock controller
>  
>  description: |
>    The T-HEAD TH1520 AP sub-system clock controller configures the
> -  CPU, DPU, GMAC and TEE PLLs.
> +  CPU, DPU, GMAC and TEE PLLs. Additionally the VO subsystem configures
> +  the clock gates for the HDMI, MIPI and the GPU.
>  
>    SoC reference manual
>    https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
> @@ -20,14 +21,24 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: thead,th1520-clk-ap
> +    enum:
> +      - thead,th1520-clk-ap
> +      - thead,th1520-clk-vo
>  
>    reg:
>      maxItems: 1
>  
>    clocks:
>      items:
> -      - description: main oscillator (24MHz)
> +      - description: |
> +          One input clock:
> +          - For "thead,th1520-clk-ap": the clock input must be the 24 MHz
> +            main oscillator.
> +          - For "thead,th1520-clk-vo": the clock input must be the VIDEO_PLL,
> +            which is configured by the AP clock controller. According to the
> +            TH1520 manual, VIDEO_PLL is a Silicon Creations Sigma-Delta PLL
> +            (integer PLL) typically running at 792 MHz (FOUTPOSTDIV), with
> +            a maximum FOUTVCO of 2376 MHz.
>  
>    "#clock-cells":
>      const: 1
> diff --git a/include/dt-bindings/clock/thead,th1520-clk-ap.h b/include/dt-bindings/clock/thead,th1520-clk-ap.h
> index a199784b3512..09a9aa7b3ab1 100644
> --- a/include/dt-bindings/clock/thead,th1520-clk-ap.h
> +++ b/include/dt-bindings/clock/thead,th1520-clk-ap.h
> @@ -93,4 +93,38 @@
>  #define CLK_SRAM3		83
>  #define CLK_PLL_GMAC_100M	84
>  #define CLK_UART_SCLK		85
> +
> +/* VO clocks */
> +#define CLK_AXI4_VO_ACLK		0
> +#define CLK_GPU_MEM			1
> +#define CLK_GPU_CORE			2
> +#define CLK_GPU_CFG_ACLK		3
> +#define CLK_DPU_PIXELCLK0		4
> +#define CLK_DPU_PIXELCLK1		5
> +#define CLK_DPU_HCLK			6
> +#define CLK_DPU_ACLK			7
> +#define CLK_DPU_CCLK			8
> +#define CLK_HDMI_SFR			9
> +#define CLK_HDMI_PCLK			10
> +#define CLK_HDMI_CEC			11
> +#define CLK_MIPI_DSI0_PCLK		12
> +#define CLK_MIPI_DSI1_PCLK		13
> +#define CLK_MIPI_DSI0_CFG		14
> +#define CLK_MIPI_DSI1_CFG		15
> +#define CLK_MIPI_DSI0_REFCLK		16
> +#define CLK_MIPI_DSI1_REFCLK		17
> +#define CLK_HDMI_I2S			18
> +#define CLK_X2H_DPU1_ACLK		19
> +#define CLK_X2H_DPU_ACLK		20
> +#define CLK_AXI4_VO_PCLK		21
> +#define CLK_IOPMP_VOSYS_DPU_PCLK	22
> +#define CLK_IOPMP_VOSYS_DPU1_PCLK	23
> +#define CLK_IOPMP_VOSYS_GPU_PCLK	24
> +#define CLK_IOPMP_DPU1_ACLK		25
> +#define CLK_IOPMP_DPU_ACLK		26
> +#define CLK_IOPMP_GPU_ACLK		27
> +#define CLK_MIPIDSI0_PIXCLK		28
> +#define CLK_MIPIDSI1_PIXCLK		29
> +#define CLK_HDMI_PIXCLK			30
> +
>  #endif
> -- 
> 2.34.1
> 

Reviewed-by: Drew Fustini <drew@pdp7.com>

I think this makes sense and dt_binding_check looks clean.

Thanks,
Drew

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Michal Wilczynski]

On 4/5/25 01:16, Drew Fustini wrote:
> On Thu, Apr 03, 2025 at 11:44:25AM +0200, Michal Wilczynski wrote:
>> VO clocks reside in a different address space from the AP clocks on the
>> T-HEAD SoC. Add the device tree node of a clock-controller to handle
>> VO address space as well.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index 527336417765..d4cba0713cab 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
>>  			#clock-cells = <1>;
>>  		};
>>  
>> +		clk_vo: clock-controller@ffef528050 {
>> +			compatible = "thead,th1520-clk-vo";
>> +			reg = <0xff 0xef528050 0x0 0xfb0>;
> 
> Thanks for your patch. It is great to have more of the clocks supported
> upstream.
> 
> The TH1520 System User Manual shows 0xFF_EF52_8000 for VO_SUBSYS on page
> 205. Is there a reason you decided to use 0xFF_EF52_8050 as the base?
> 
> I see on page 213 that the first register for VO_SUBSYS starts with
> VOSYS_CLK_GATE at offset 0x50. I figure you did this to have the
> CCU_GATE macros use offset of 0x0 instead 0x50.
> 
> I kind of think the reg property using the actual base address
> (0xFF_EF52_8000) makes more sense as that's a closer match to the tables
> in the manual. But I don't have a strong preference if you think think
> using 0xef528050 makes the CCU_GATE macros easier to read.

Thank you for your comment.

This was discussed some time ago. The main issue was that the address
space was fragmented between clocks and resets. Initially, I proposed
using syscon as a way to abstract this, but the idea wasn't particularly
well received.

So at the start of the 0xFF_EF52_8000 there is a reset register GPU_RST_CFG
I need for resetting the GPU.

For reference, here's the earlier discussion: [1]

[1] - https://lore.kernel.org/all/1b05b11b2a8287c0ff4b6bdd079988c7.sboyd@kernel.org/

Regards,
Michał

> 
> -Drew
> 

Re: [PATCH v7 2/3] clk: thead: Add clock support for VO subsystem in T-HEAD TH1520 SoC

[Michal Wilczynski]

On 4/5/25 02:40, Drew Fustini wrote:
> On Thu, Apr 03, 2025 at 11:44:24AM +0200, Michal Wilczynski wrote:
>> The T-Head TH1520 SoC integrates a variety of clocks for its subsystems,
>> including the Application Processor (AP) and the Video Output (VO) [1].
>> Up until now, the T-Head clock driver only supported AP clocks.
>>
>> Extend the driver to provide clock functionality for the VO subsystem.
>> At this stage, the focus is on implementing the VO clock gates, as these
>> are currently the most relevant and required components for enabling and
>> disabling the VO subsystem functionality.  Future enhancements may
>> introduce additional VO-related clocks as necessary.
>>
>> Link: https://protect2.fireeye.com/v1/url?k=36dff7e6-5754e2d0-36de7ca9-74fe485cbff1-cfd601a10959d91c&q=1&e=fa692882-d05b-4276-bff3-01f7a237dd97&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf [1]
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/clk/thead/clk-th1520-ap.c | 196 +++++++++++++++++++++++++-----
>>  1 file changed, 168 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>> index 4c9555fc6184..ebfb1d59401d 100644
>> --- a/drivers/clk/thead/clk-th1520-ap.c
>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>> @@ -847,6 +847,67 @@ static CCU_GATE(CLK_SRAM1, sram1_clk, "sram1", axi_aclk_pd, 0x20c, BIT(3), 0);
>>  static CCU_GATE(CLK_SRAM2, sram2_clk, "sram2", axi_aclk_pd, 0x20c, BIT(2), 0);
>>  static CCU_GATE(CLK_SRAM3, sram3_clk, "sram3", axi_aclk_pd, 0x20c, BIT(1), 0);
>>  
>> +static CCU_GATE(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(0), 0);
> 
> Is CLKCTRL_GPU_MEM_CLK_EN (bit 2) skipped on purpose?

Hi,
This one is marked as "Reserved" in the manual, so yeah it's on purpose.

> 
>> +static CCU_GATE(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_clk_pd,
>> +		0x0, BIT(3), 0);
>> +static CCU_GATE(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(4), 0);
>> +static CCU_GATE(CLK_DPU_PIXELCLK0, dpu0_pixelclk, "dpu0-pixelclk",
>> +		video_pll_clk_pd, 0x0, BIT(5), 0);
>> +static CCU_GATE(CLK_DPU_PIXELCLK1, dpu1_pixelclk, "dpu1-pixelclk",
>> +		video_pll_clk_pd, 0x0, BIT(6), 0);
>> +static CCU_GATE(CLK_DPU_HCLK, dpu_hclk, "dpu-hclk", video_pll_clk_pd, 0x0,
>> +		BIT(7), 0);
>> +static CCU_GATE(CLK_DPU_ACLK, dpu_aclk, "dpu-aclk", video_pll_clk_pd, 0x0,
>> +		BIT(8), 0);
>> +static CCU_GATE(CLK_DPU_CCLK, dpu_cclk, "dpu-cclk", video_pll_clk_pd, 0x0,
>> +		BIT(9), 0);
>> +static CCU_GATE(CLK_HDMI_SFR, hdmi_sfr_clk, "hdmi-sfr-clk", video_pll_clk_pd,
>> +		0x0, BIT(10), 0);
>> +static CCU_GATE(CLK_HDMI_PCLK, hdmi_pclk, "hdmi-pclk", video_pll_clk_pd, 0x0,
>> +		BIT(11), 0);
>> +static CCU_GATE(CLK_HDMI_CEC, hdmi_cec_clk, "hdmi-cec-clk", video_pll_clk_pd,
>> +		0x0, BIT(12), 0);
>> +static CCU_GATE(CLK_MIPI_DSI0_PCLK, mipi_dsi0_pclk, "mipi-dsi0-pclk",
>> +		video_pll_clk_pd, 0x0, BIT(13), 0);
>> +static CCU_GATE(CLK_MIPI_DSI1_PCLK, mipi_dsi1_pclk, "mipi-dsi1-pclk",
>> +		video_pll_clk_pd, 0x0, BIT(14), 0);
>> +static CCU_GATE(CLK_MIPI_DSI0_CFG, mipi_dsi0_cfg_clk, "mipi-dsi0-cfg-clk",
>> +		video_pll_clk_pd, 0x0, BIT(15), 0);
>> +static CCU_GATE(CLK_MIPI_DSI1_CFG, mipi_dsi1_cfg_clk, "mipi-dsi1-cfg-clk",
>> +		video_pll_clk_pd, 0x0, BIT(16), 0);
>> +static CCU_GATE(CLK_MIPI_DSI0_REFCLK, mipi_dsi0_refclk, "mipi-dsi0-refclk",
>> +		video_pll_clk_pd, 0x0, BIT(17), 0);
>> +static CCU_GATE(CLK_MIPI_DSI1_REFCLK, mipi_dsi1_refclk, "mipi-dsi1-refclk",
>> +		video_pll_clk_pd, 0x0, BIT(18), 0);
>> +static CCU_GATE(CLK_HDMI_I2S, hdmi_i2s_clk, "hdmi-i2s-clk", video_pll_clk_pd,
>> +		0x0, BIT(19), 0);
>> +static CCU_GATE(CLK_X2H_DPU1_ACLK, x2h_dpu1_aclk, "x2h-dpu1-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(20), 0);
>> +static CCU_GATE(CLK_X2H_DPU_ACLK, x2h_dpu_aclk, "x2h-dpu-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(21), 0);
>> +static CCU_GATE(CLK_AXI4_VO_PCLK, axi4_vo_pclk, "axi4-vo-pclk",
>> +		video_pll_clk_pd, 0x0, BIT(22), 0);
>> +static CCU_GATE(CLK_IOPMP_VOSYS_DPU_PCLK, iopmp_vosys_dpu_pclk,
>> +		"iopmp-vosys-dpu-pclk", video_pll_clk_pd, 0x0, BIT(23), 0);
>> +static CCU_GATE(CLK_IOPMP_VOSYS_DPU1_PCLK, iopmp_vosys_dpu1_pclk,
>> +		"iopmp-vosys-dpu1-pclk", video_pll_clk_pd, 0x0, BIT(24), 0);
>> +static CCU_GATE(CLK_IOPMP_VOSYS_GPU_PCLK, iopmp_vosys_gpu_pclk,
>> +		"iopmp-vosys-gpu-pclk", video_pll_clk_pd, 0x0, BIT(25), 0);
>> +static CCU_GATE(CLK_IOPMP_DPU1_ACLK, iopmp_dpu1_aclk, "iopmp-dpu1-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(27), 0);
>> +static CCU_GATE(CLK_IOPMP_DPU_ACLK, iopmp_dpu_aclk, "iopmp-dpu-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(28), 0);
>> +static CCU_GATE(CLK_IOPMP_GPU_ACLK, iopmp_gpu_aclk, "iopmp-gpu-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(29), 0);
>> +static CCU_GATE(CLK_MIPIDSI0_PIXCLK, mipi_dsi0_pixclk, "mipi-dsi0-pixclk",
>> +		video_pll_clk_pd, 0x0, BIT(30), 0);
>> +static CCU_GATE(CLK_MIPIDSI1_PIXCLK, mipi_dsi1_pixclk, "mipi-dsi1-pixclk",
>> +		video_pll_clk_pd, 0x0, BIT(31), 0);
>> +static CCU_GATE(CLK_HDMI_PIXCLK, hdmi_pixclk, "hdmi-pixclk", video_pll_clk_pd,
>> +		0x4, BIT(0), 0);
> 
> Did you intentionally skip VOSYS_DPU_CCLK_CFG.VOSYS_DPU_CCLK_CFG and
> TEST_CLK_CFG.TEST_CLK_CFG as they aren't needed?

Yeah I couldn't see a use for them even in the vendor kernel so skipped
them. I guess they could be added when we figure some way to use them.

Regards,
Michał

> 
> 
> Thanks,
> Drew
> 

Re: [PATCH v7 2/3] clk: thead: Add clock support for VO subsystem in T-HEAD TH1520 SoC

[Drew Fustini]

On Mon, Apr 07, 2025 at 06:16:52PM +0200, Michal Wilczynski wrote:
> 
> 
> On 4/5/25 02:40, Drew Fustini wrote:
> > On Thu, Apr 03, 2025 at 11:44:24AM +0200, Michal Wilczynski wrote:
> >> The T-Head TH1520 SoC integrates a variety of clocks for its subsystems,
> >> including the Application Processor (AP) and the Video Output (VO) [1].
> >> Up until now, the T-Head clock driver only supported AP clocks.
> >>
> >> Extend the driver to provide clock functionality for the VO subsystem.
> >> At this stage, the focus is on implementing the VO clock gates, as these
> >> are currently the most relevant and required components for enabling and
> >> disabling the VO subsystem functionality.  Future enhancements may
> >> introduce additional VO-related clocks as necessary.
> >>
> >> Link: https://protect2.fireeye.com/v1/url?k=36dff7e6-5754e2d0-36de7ca9-74fe485cbff1-cfd601a10959d91c&q=1&e=fa692882-d05b-4276-bff3-01f7a237dd97&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf [1]
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  drivers/clk/thead/clk-th1520-ap.c | 196 +++++++++++++++++++++++++-----
> >>  1 file changed, 168 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> >> index 4c9555fc6184..ebfb1d59401d 100644
> >> --- a/drivers/clk/thead/clk-th1520-ap.c
> >> +++ b/drivers/clk/thead/clk-th1520-ap.c
> >> @@ -847,6 +847,67 @@ static CCU_GATE(CLK_SRAM1, sram1_clk, "sram1", axi_aclk_pd, 0x20c, BIT(3), 0);
> >>  static CCU_GATE(CLK_SRAM2, sram2_clk, "sram2", axi_aclk_pd, 0x20c, BIT(2), 0);
> >>  static CCU_GATE(CLK_SRAM3, sram3_clk, "sram3", axi_aclk_pd, 0x20c, BIT(1), 0);
> >>  
> >> +static CCU_GATE(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(0), 0);
> > 
> > Is CLKCTRL_GPU_MEM_CLK_EN (bit 2) skipped on purpose?
> 
> Hi,
> This one is marked as "Reserved" in the manual, so yeah it's on purpose.
> 
> > 
> >> +static CCU_GATE(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_clk_pd,
> >> +		0x0, BIT(3), 0);
> >> +static CCU_GATE(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(4), 0);
> >> +static CCU_GATE(CLK_DPU_PIXELCLK0, dpu0_pixelclk, "dpu0-pixelclk",
> >> +		video_pll_clk_pd, 0x0, BIT(5), 0);
> >> +static CCU_GATE(CLK_DPU_PIXELCLK1, dpu1_pixelclk, "dpu1-pixelclk",
> >> +		video_pll_clk_pd, 0x0, BIT(6), 0);
> >> +static CCU_GATE(CLK_DPU_HCLK, dpu_hclk, "dpu-hclk", video_pll_clk_pd, 0x0,
> >> +		BIT(7), 0);
> >> +static CCU_GATE(CLK_DPU_ACLK, dpu_aclk, "dpu-aclk", video_pll_clk_pd, 0x0,
> >> +		BIT(8), 0);
> >> +static CCU_GATE(CLK_DPU_CCLK, dpu_cclk, "dpu-cclk", video_pll_clk_pd, 0x0,
> >> +		BIT(9), 0);
> >> +static CCU_GATE(CLK_HDMI_SFR, hdmi_sfr_clk, "hdmi-sfr-clk", video_pll_clk_pd,
> >> +		0x0, BIT(10), 0);
> >> +static CCU_GATE(CLK_HDMI_PCLK, hdmi_pclk, "hdmi-pclk", video_pll_clk_pd, 0x0,
> >> +		BIT(11), 0);
> >> +static CCU_GATE(CLK_HDMI_CEC, hdmi_cec_clk, "hdmi-cec-clk", video_pll_clk_pd,
> >> +		0x0, BIT(12), 0);
> >> +static CCU_GATE(CLK_MIPI_DSI0_PCLK, mipi_dsi0_pclk, "mipi-dsi0-pclk",
> >> +		video_pll_clk_pd, 0x0, BIT(13), 0);
> >> +static CCU_GATE(CLK_MIPI_DSI1_PCLK, mipi_dsi1_pclk, "mipi-dsi1-pclk",
> >> +		video_pll_clk_pd, 0x0, BIT(14), 0);
> >> +static CCU_GATE(CLK_MIPI_DSI0_CFG, mipi_dsi0_cfg_clk, "mipi-dsi0-cfg-clk",
> >> +		video_pll_clk_pd, 0x0, BIT(15), 0);
> >> +static CCU_GATE(CLK_MIPI_DSI1_CFG, mipi_dsi1_cfg_clk, "mipi-dsi1-cfg-clk",
> >> +		video_pll_clk_pd, 0x0, BIT(16), 0);
> >> +static CCU_GATE(CLK_MIPI_DSI0_REFCLK, mipi_dsi0_refclk, "mipi-dsi0-refclk",
> >> +		video_pll_clk_pd, 0x0, BIT(17), 0);
> >> +static CCU_GATE(CLK_MIPI_DSI1_REFCLK, mipi_dsi1_refclk, "mipi-dsi1-refclk",
> >> +		video_pll_clk_pd, 0x0, BIT(18), 0);
> >> +static CCU_GATE(CLK_HDMI_I2S, hdmi_i2s_clk, "hdmi-i2s-clk", video_pll_clk_pd,
> >> +		0x0, BIT(19), 0);
> >> +static CCU_GATE(CLK_X2H_DPU1_ACLK, x2h_dpu1_aclk, "x2h-dpu1-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(20), 0);
> >> +static CCU_GATE(CLK_X2H_DPU_ACLK, x2h_dpu_aclk, "x2h-dpu-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(21), 0);
> >> +static CCU_GATE(CLK_AXI4_VO_PCLK, axi4_vo_pclk, "axi4-vo-pclk",
> >> +		video_pll_clk_pd, 0x0, BIT(22), 0);
> >> +static CCU_GATE(CLK_IOPMP_VOSYS_DPU_PCLK, iopmp_vosys_dpu_pclk,
> >> +		"iopmp-vosys-dpu-pclk", video_pll_clk_pd, 0x0, BIT(23), 0);
> >> +static CCU_GATE(CLK_IOPMP_VOSYS_DPU1_PCLK, iopmp_vosys_dpu1_pclk,
> >> +		"iopmp-vosys-dpu1-pclk", video_pll_clk_pd, 0x0, BIT(24), 0);
> >> +static CCU_GATE(CLK_IOPMP_VOSYS_GPU_PCLK, iopmp_vosys_gpu_pclk,
> >> +		"iopmp-vosys-gpu-pclk", video_pll_clk_pd, 0x0, BIT(25), 0);
> >> +static CCU_GATE(CLK_IOPMP_DPU1_ACLK, iopmp_dpu1_aclk, "iopmp-dpu1-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(27), 0);
> >> +static CCU_GATE(CLK_IOPMP_DPU_ACLK, iopmp_dpu_aclk, "iopmp-dpu-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(28), 0);
> >> +static CCU_GATE(CLK_IOPMP_GPU_ACLK, iopmp_gpu_aclk, "iopmp-gpu-aclk",
> >> +		video_pll_clk_pd, 0x0, BIT(29), 0);
> >> +static CCU_GATE(CLK_MIPIDSI0_PIXCLK, mipi_dsi0_pixclk, "mipi-dsi0-pixclk",
> >> +		video_pll_clk_pd, 0x0, BIT(30), 0);
> >> +static CCU_GATE(CLK_MIPIDSI1_PIXCLK, mipi_dsi1_pixclk, "mipi-dsi1-pixclk",
> >> +		video_pll_clk_pd, 0x0, BIT(31), 0);
> >> +static CCU_GATE(CLK_HDMI_PIXCLK, hdmi_pixclk, "hdmi-pixclk", video_pll_clk_pd,
> >> +		0x4, BIT(0), 0);
> > 
> > Did you intentionally skip VOSYS_DPU_CCLK_CFG.VOSYS_DPU_CCLK_CFG and
> > TEST_CLK_CFG.TEST_CLK_CFG as they aren't needed?
> 
> Yeah I couldn't see a use for them even in the vendor kernel so skipped
> them. I guess they could be added when we figure some way to use them.

Thanks, for the explanations. This looks good to me.

Reviewed-by: Drew Fustini <drew@pdp7.com>

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Drew Fustini]

On Mon, Apr 07, 2025 at 05:30:43PM +0200, Michal Wilczynski wrote:
> 
> 
> On 4/5/25 01:16, Drew Fustini wrote:
> > On Thu, Apr 03, 2025 at 11:44:25AM +0200, Michal Wilczynski wrote:
> >> VO clocks reside in a different address space from the AP clocks on the
> >> T-HEAD SoC. Add the device tree node of a clock-controller to handle
> >> VO address space as well.
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index 527336417765..d4cba0713cab 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
> >>  			#clock-cells = <1>;
> >>  		};
> >>  
> >> +		clk_vo: clock-controller@ffef528050 {
> >> +			compatible = "thead,th1520-clk-vo";
> >> +			reg = <0xff 0xef528050 0x0 0xfb0>;
> > 
> > Thanks for your patch. It is great to have more of the clocks supported
> > upstream.
> > 
> > The TH1520 System User Manual shows 0xFF_EF52_8000 for VO_SUBSYS on page
> > 205. Is there a reason you decided to use 0xFF_EF52_8050 as the base?
> > 
> > I see on page 213 that the first register for VO_SUBSYS starts with
> > VOSYS_CLK_GATE at offset 0x50. I figure you did this to have the
> > CCU_GATE macros use offset of 0x0 instead 0x50.
> > 
> > I kind of think the reg property using the actual base address
> > (0xFF_EF52_8000) makes more sense as that's a closer match to the tables
> > in the manual. But I don't have a strong preference if you think think
> > using 0xef528050 makes the CCU_GATE macros easier to read.
> 
> Thank you for your comment.
> 
> This was discussed some time ago. The main issue was that the address
> space was fragmented between clocks and resets. Initially, I proposed
> using syscon as a way to abstract this, but the idea wasn't particularly
> well received.
> 
> So at the start of the 0xFF_EF52_8000 there is a reset register GPU_RST_CFG
> I need for resetting the GPU.
> 
> For reference, here's the earlier discussion: [1]
> 
> [1] - https://lore.kernel.org/all/1b05b11b2a8287c0ff4b6bdd079988c7.sboyd@kernel.org/

Thanks for the explanation.

Reviewed-by: Drew Fustini <drew@pdp7.com>

Re: [PATCH v7 0/3] Add T-HEAD TH1520 VO clock support for LicheePi 4A GPU enablement

[Michal Wilczynski]

On 4/3/25 11:44, Michal Wilczynski wrote:
> This is a subset of a larger patch series enabling the Imagination BXM-4-64 GPU
> on the LicheePi 4A board, which is powered by the T-HEAD TH1520 SoC. While the
> full series includes power-domain, reset, and firmware changes, this part
> focuses solely on the clock subsystem needed for the GPU and other VO (video
> output) blocks. By merging these clock patches independently, we prepare the
> groundwork for future GPU integration via the `drm/imagination` driver.
> 
> The T-HEAD TH1520 SoC features multiple clock controllers. Initially, only the
> AP clock controller was supported upstream. The patches below add support for
> the VO (video output) clock controller, which manages GPU-related gates, HDMI,
> and other multimedia clocks.
> 
> Bigger series cover letter:
> https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/
> 
> v7:
> - remove commits 3,4 from the patch series, those would handle empty MEM clock
>   stub, and reset management. It's not necessary anymore, as this would be
>   implemented in power-domain driver
> - added the device tree patch at the end for the SoC maintainers to take after
>   the other patches get OK-ed
> - added Acked-by, from Connor for the dt-binding patch
> - re-added Reviewed-by from Krzysztof, as the dt-binding patch is the same as
>   for the v5
> 
> v6:
> - squashed the "dt-bindings: clock: thead: Add GPU clkgen reset property"
>   with the "dt-bindings: clock: thead: Add TH1520 VO clock controller". As
>   a result, also removed the Reviewed-by from Krzysztof, since the new
>   resets property has been introduced, which is mandatory in the VO
>   case
> 
> v5:
> - introduced a new macro CCU_GATE_CLK_OPS, which allows providing custom clk_ops.
>   In the case of the 'MEM' clock, it provides empty clk_nops. Later, this clock
>   is provided to the GPU node, thereby avoiding any ABI breakage
> - used the CCU_GATE_CLK_OPS macro to implement a workaround for de-asserting
>   the clkgen reset only after both core and sys clocks are enabled. This
>   sequence is required to properly initialize the GPU
> 
> v4:
>  - enhanced documentation for new Video Output (VO) clock inputs in device tree
>    bindings
> 
> v3:
>  - reworked driver to support multiple clock controllers through .compatible
>    and .data instead of using multiple address spaces in dt-binding. This change
>    allows to re-use the driver code for multiple clock controllers
> 
> v2:
>  - removed AP_SUBSYS clock refactoring commits (1-6):
>  - instead of refactoring, I opted to extend the current driver and its
>    associated device tree node to include support for a second address space.
>  - resolved all checkpatch issues using --strict, except for the call to
>    devm_clk_hw_register_gate_parent_data().  The current implementation remains
>    preferable in this context, and clang-format aligns with this choice
> 
> Michal Wilczynski (3):
>   dt-bindings: clock: thead: Add TH1520 VO clock controller
>   clk: thead: Add clock support for VO subsystem in T-HEAD TH1520 SoC
>   riscv: dts: thead: Add device tree VO clock controller
> 
>  .../bindings/clock/thead,th1520-clk-ap.yaml   |  17 +-
>  arch/riscv/boot/dts/thead/th1520.dtsi         |   7 +
>  drivers/clk/thead/clk-th1520-ap.c             | 196 +++++++++++++++---
>  .../dt-bindings/clock/thead,th1520-clk-ap.h   |  34 +++
>  4 files changed, 223 insertions(+), 31 deletions(-)
> 

Hi Stephen, I think Drew is already collecting DT commits for the next
merge window. Do you think the patches 1-2 will make it for the 6.16
release ?

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Stephen Boyd]

Quoting Michal Wilczynski (2025-04-07 08:30:43)
> On 4/5/25 01:16, Drew Fustini wrote:
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index 527336417765..d4cba0713cab 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
> >>                      #clock-cells = <1>;
> >>              };
> >>  
> >> +            clk_vo: clock-controller@ffef528050 {
> >> +                    compatible = "thead,th1520-clk-vo";
> >> +                    reg = <0xff 0xef528050 0x0 0xfb0>;
> > 
> > Thanks for your patch. It is great to have more of the clocks supported
> > upstream.
> > 
> > The TH1520 System User Manual shows 0xFF_EF52_8000 for VO_SUBSYS on page
> > 205. Is there a reason you decided to use 0xFF_EF52_8050 as the base?
> > 
> > I see on page 213 that the first register for VO_SUBSYS starts with
> > VOSYS_CLK_GATE at offset 0x50. I figure you did this to have the
> > CCU_GATE macros use offset of 0x0 instead 0x50.
> > 
> > I kind of think the reg property using the actual base address
> > (0xFF_EF52_8000) makes more sense as that's a closer match to the tables
> > in the manual. But I don't have a strong preference if you think think
> > using 0xef528050 makes the CCU_GATE macros easier to read.
> 
> Thank you for your comment.
> 
> This was discussed some time ago. The main issue was that the address
> space was fragmented between clocks and resets. Initially, I proposed
> using syscon as a way to abstract this, but the idea wasn't particularly
> well received.
> 
> So at the start of the 0xFF_EF52_8000 there is a reset register GPU_RST_CFG
> I need for resetting the GPU.
> 
> For reference, here's the earlier discussion: [1]
> 
> [1] - https://lore.kernel.org/all/1b05b11b2a8287c0ff4b6bdd079988c7.sboyd@kernel.org/
> 

In that email I said you should have one node
clock-controller@ffef528000. Why did 0x50 get added to the address?

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Michal Wilczynski]

On 4/30/25 00:29, Stephen Boyd wrote:
> Quoting Michal Wilczynski (2025-04-07 08:30:43)
>> On 4/5/25 01:16, Drew Fustini wrote:
>>>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> index 527336417765..d4cba0713cab 100644
>>>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> @@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
>>>>                      #clock-cells = <1>;
>>>>              };
>>>>  
>>>> +            clk_vo: clock-controller@ffef528050 {
>>>> +                    compatible = "thead,th1520-clk-vo";
>>>> +                    reg = <0xff 0xef528050 0x0 0xfb0>;
>>>
>>> Thanks for your patch. It is great to have more of the clocks supported
>>> upstream.
>>>
>>> The TH1520 System User Manual shows 0xFF_EF52_8000 for VO_SUBSYS on page
>>> 205. Is there a reason you decided to use 0xFF_EF52_8050 as the base?
>>>
>>> I see on page 213 that the first register for VO_SUBSYS starts with
>>> VOSYS_CLK_GATE at offset 0x50. I figure you did this to have the
>>> CCU_GATE macros use offset of 0x0 instead 0x50.
>>>
>>> I kind of think the reg property using the actual base address
>>> (0xFF_EF52_8000) makes more sense as that's a closer match to the tables
>>> in the manual. But I don't have a strong preference if you think think
>>> using 0xef528050 makes the CCU_GATE macros easier to read.
>>
>> Thank you for your comment.
>>
>> This was discussed some time ago. The main issue was that the address
>> space was fragmented between clocks and resets. Initially, I proposed
>> using syscon as a way to abstract this, but the idea wasn't particularly
>> well received.
>>
>> So at the start of the 0xFF_EF52_8000 there is a reset register GPU_RST_CFG
>> I need for resetting the GPU.
>>
>> For reference, here's the earlier discussion: [1]
>>
>> [1] - https://lore.kernel.org/all/1b05b11b2a8287c0ff4b6bdd079988c7.sboyd@kernel.org/
>>
> 
> In that email I said you should have one node
> clock-controller@ffef528000. Why did 0x50 get added to the address?

Hi Stephen,
In the v2 version of the patchset, there was no reset controller yet, so
I thought your comment was made referring to that earlier version.
This representation clearly describes the hardware correctly, which is
the requirement for the Device Tree.

The manual, in section 5.4.1.6 VO_SUBSYS, describes the reset registers
starting at 0xFF_EF52_8000:

GPU_RST_CFG             0x00
DPU_RST_CFG             0x04
MIPI_DSI0_RST_CFG       0x8
MIPI_DSI1_RST_CFG       0xc
HDMI_RST_CFG            0x14
AXI4_VO_DW_AXI          0x18
X2H_X4_VOSYS_DW_AXI_X2H 0x20

And the clock registers for VO_SUBSYS, manual section 4.4.1.6 start at offset 0x50:
VOSYS_CLK_GATE          0x50
VOSYS_CLK_GATE1         0x54
VOSYS_DPU_CCLK_CFG0     0x64
TEST_CLK_FREQ_STAT      0xc4
TEST_CLK_CFG            0xc8

So I considered this back then and thought it was appropriate to divide
it into two nodes, as the reset node wasn't being considered at that
time.

When looking for the reference [1], I didn't notice if you corrected
yourself later, but I do remember considering the single-node approach
at the time.

> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Drew Fustini]

On Wed, Apr 30, 2025 at 09:52:29AM +0200, Michal Wilczynski wrote:
> 
> 
> On 4/30/25 00:29, Stephen Boyd wrote:
> > Quoting Michal Wilczynski (2025-04-07 08:30:43)
> >> On 4/5/25 01:16, Drew Fustini wrote:
> >>>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >>>> index 527336417765..d4cba0713cab 100644
> >>>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >>>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >>>> @@ -489,6 +489,13 @@ clk: clock-controller@ffef010000 {
> >>>>                      #clock-cells = <1>;
> >>>>              };
> >>>>  
> >>>> +            clk_vo: clock-controller@ffef528050 {
> >>>> +                    compatible = "thead,th1520-clk-vo";
> >>>> +                    reg = <0xff 0xef528050 0x0 0xfb0>;
> >>>
> >>> Thanks for your patch. It is great to have more of the clocks supported
> >>> upstream.
> >>>
> >>> The TH1520 System User Manual shows 0xFF_EF52_8000 for VO_SUBSYS on page
> >>> 205. Is there a reason you decided to use 0xFF_EF52_8050 as the base?
> >>>
> >>> I see on page 213 that the first register for VO_SUBSYS starts with
> >>> VOSYS_CLK_GATE at offset 0x50. I figure you did this to have the
> >>> CCU_GATE macros use offset of 0x0 instead 0x50.
> >>>
> >>> I kind of think the reg property using the actual base address
> >>> (0xFF_EF52_8000) makes more sense as that's a closer match to the tables
> >>> in the manual. But I don't have a strong preference if you think think
> >>> using 0xef528050 makes the CCU_GATE macros easier to read.
> >>
> >> Thank you for your comment.
> >>
> >> This was discussed some time ago. The main issue was that the address
> >> space was fragmented between clocks and resets. Initially, I proposed
> >> using syscon as a way to abstract this, but the idea wasn't particularly
> >> well received.
> >>
> >> So at the start of the 0xFF_EF52_8000 there is a reset register GPU_RST_CFG
> >> I need for resetting the GPU.
> >>
> >> For reference, here's the earlier discussion: [1]
> >>
> >> [1] - https://lore.kernel.org/all/1b05b11b2a8287c0ff4b6bdd079988c7.sboyd@kernel.org/
> >>
> > 
> > In that email I said you should have one node
> > clock-controller@ffef528000. Why did 0x50 get added to the address?
> 
> Hi Stephen,
> In the v2 version of the patchset, there was no reset controller yet, so
> I thought your comment was made referring to that earlier version.
> This representation clearly describes the hardware correctly, which is
> the requirement for the Device Tree.
> 
> The manual, in section 5.4.1.6 VO_SUBSYS, describes the reset registers
> starting at 0xFF_EF52_8000:
> 
> GPU_RST_CFG             0x00
> DPU_RST_CFG             0x04
> MIPI_DSI0_RST_CFG       0x8
> MIPI_DSI1_RST_CFG       0xc
> HDMI_RST_CFG            0x14
> AXI4_VO_DW_AXI          0x18
> X2H_X4_VOSYS_DW_AXI_X2H 0x20
> 
> And the clock registers for VO_SUBSYS, manual section 4.4.1.6 start at offset 0x50:
> VOSYS_CLK_GATE          0x50
> VOSYS_CLK_GATE1         0x54
> VOSYS_DPU_CCLK_CFG0     0x64
> TEST_CLK_FREQ_STAT      0xc4
> TEST_CLK_CFG            0xc8
> 
> So I considered this back then and thought it was appropriate to divide
> it into two nodes, as the reset node wasn't being considered at that
> time.
> 
> When looking for the reference [1], I didn't notice if you corrected
> yourself later, but I do remember considering the single-node approach
> at the time.
> 
> > 
> 
> Best regards,
> -- 
> Michal Wilczynski <m.wilczynski@samsung.com>

I chatted with Stephen on irc about setting up a thead clk branch and
sending pull requests to Stephen.

Stephen - are there changes in this series that you want to see in order
to give your Reviewed-by?

Thanks,
Drew

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Stephen Boyd]

Quoting Michal Wilczynski (2025-04-30 00:52:29)
> 
> In the v2 version of the patchset, there was no reset controller yet, so
> I thought your comment was made referring to that earlier version.
> This representation clearly describes the hardware correctly, which is
> the requirement for the Device Tree.
> 
> The manual, in section 5.4.1.6 VO_SUBSYS, describes the reset registers
> starting at 0xFF_EF52_8000:
> 
> GPU_RST_CFG             0x00
> DPU_RST_CFG             0x04
> MIPI_DSI0_RST_CFG       0x8
> MIPI_DSI1_RST_CFG       0xc
> HDMI_RST_CFG            0x14
> AXI4_VO_DW_AXI          0x18
> X2H_X4_VOSYS_DW_AXI_X2H 0x20
> 
> And the clock registers for VO_SUBSYS, manual section 4.4.1.6 start at offset 0x50:
> VOSYS_CLK_GATE          0x50
> VOSYS_CLK_GATE1         0x54
> VOSYS_DPU_CCLK_CFG0     0x64
> TEST_CLK_FREQ_STAT      0xc4
> TEST_CLK_CFG            0xc8
> 
> So I considered this back then and thought it was appropriate to divide
> it into two nodes, as the reset node wasn't being considered at that
> time.
> 
> When looking for the reference [1], I didn't notice if you corrected
> yourself later, but I do remember considering the single-node approach
> at the time.
> 

If the two register ranges don't overlap then this is probably OK. I
imagine this is one device shipped by the hardware engineer, VO_SUBSYS,
which happens to be a clock and reset controller. This is quite common,
and we usually have one node with both #clock-cells and #reset-cells in
it. Then we use the auxiliary bus to create the reset device from the
clk driver with the same node. This helps match the device in the
datasheet to the node and compatible in DT without making the compatible
provider specific (clk or reset postfix).

That's another reason why we usually have one node. DT doesn't describe
software, i.e. the split between clk and reset frameworks may not exist
in other operating systems. We don't want to put the software design
decisions into the DT.

It may also be that a device like this consumes shared power resources
like clks or regulators that need to be enabled to drive the device, or
an IOMMU is used to translate the register mappings. We wouldn't want to
split the device in DT in that case so we can easily manage the power
resources or memory mappings for the device.

TL;DR: This is probably OK, but I'd be careful to not make it a thing.

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Michal Wilczynski]

On 5/6/25 23:30, Stephen Boyd wrote:
> Quoting Michal Wilczynski (2025-04-30 00:52:29)
>>
>> In the v2 version of the patchset, there was no reset controller yet, so
>> I thought your comment was made referring to that earlier version.
>> This representation clearly describes the hardware correctly, which is
>> the requirement for the Device Tree.
>>
>> The manual, in section 5.4.1.6 VO_SUBSYS, describes the reset registers
>> starting at 0xFF_EF52_8000:
>>
>> GPU_RST_CFG             0x00
>> DPU_RST_CFG             0x04
>> MIPI_DSI0_RST_CFG       0x8
>> MIPI_DSI1_RST_CFG       0xc
>> HDMI_RST_CFG            0x14
>> AXI4_VO_DW_AXI          0x18
>> X2H_X4_VOSYS_DW_AXI_X2H 0x20
>>
>> And the clock registers for VO_SUBSYS, manual section 4.4.1.6 start at offset 0x50:
>> VOSYS_CLK_GATE          0x50
>> VOSYS_CLK_GATE1         0x54
>> VOSYS_DPU_CCLK_CFG0     0x64
>> TEST_CLK_FREQ_STAT      0xc4
>> TEST_CLK_CFG            0xc8
>>
>> So I considered this back then and thought it was appropriate to divide
>> it into two nodes, as the reset node wasn't being considered at that
>> time.
>>
>> When looking for the reference [1], I didn't notice if you corrected
>> yourself later, but I do remember considering the single-node approach
>> at the time.
>>
> 
> If the two register ranges don't overlap then this is probably OK. I
> imagine this is one device shipped by the hardware engineer, VO_SUBSYS,
> which happens to be a clock and reset controller. This is quite common,
> and we usually have one node with both #clock-cells and #reset-cells in
> it. Then we use the auxiliary bus to create the reset device from the
> clk driver with the same node. This helps match the device in the
> datasheet to the node and compatible in DT without making the compatible
> provider specific (clk or reset postfix).
> 
> That's another reason why we usually have one node. DT doesn't describe
> software, i.e. the split between clk and reset frameworks may not exist
> in other operating systems. We don't want to put the software design
> decisions into the DT.
> 
> It may also be that a device like this consumes shared power resources
> like clks or regulators that need to be enabled to drive the device, or
> an IOMMU is used to translate the register mappings. We wouldn't want to
> split the device in DT in that case so we can easily manage the power
> resources or memory mappings for the device.
> 
> TL;DR: This is probably OK, but I'd be careful to not make it a thing.

Thank you very much for the comprehensive explanation. Because the
registers don’t overlap, it’s fine in this case. Since Drew also seem to
agree, we can probably push these patches forward, while keeping in mind
that for future SoCs it would be better to use a single node.

> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Drew Fustini]

On Wed, May 07, 2025 at 12:04:01PM +0200, Michal Wilczynski wrote:
> 
> 
> On 5/6/25 23:30, Stephen Boyd wrote:
> > Quoting Michal Wilczynski (2025-04-30 00:52:29)
> >>
> >> In the v2 version of the patchset, there was no reset controller yet, so
> >> I thought your comment was made referring to that earlier version.
> >> This representation clearly describes the hardware correctly, which is
> >> the requirement for the Device Tree.
> >>
> >> The manual, in section 5.4.1.6 VO_SUBSYS, describes the reset registers
> >> starting at 0xFF_EF52_8000:
> >>
> >> GPU_RST_CFG             0x00
> >> DPU_RST_CFG             0x04
> >> MIPI_DSI0_RST_CFG       0x8
> >> MIPI_DSI1_RST_CFG       0xc
> >> HDMI_RST_CFG            0x14
> >> AXI4_VO_DW_AXI          0x18
> >> X2H_X4_VOSYS_DW_AXI_X2H 0x20
> >>
> >> And the clock registers for VO_SUBSYS, manual section 4.4.1.6 start at offset 0x50:
> >> VOSYS_CLK_GATE          0x50
> >> VOSYS_CLK_GATE1         0x54
> >> VOSYS_DPU_CCLK_CFG0     0x64
> >> TEST_CLK_FREQ_STAT      0xc4
> >> TEST_CLK_CFG            0xc8
> >>
> >> So I considered this back then and thought it was appropriate to divide
> >> it into two nodes, as the reset node wasn't being considered at that
> >> time.
> >>
> >> When looking for the reference [1], I didn't notice if you corrected
> >> yourself later, but I do remember considering the single-node approach
> >> at the time.
> >>
> > 
> > If the two register ranges don't overlap then this is probably OK. I
> > imagine this is one device shipped by the hardware engineer, VO_SUBSYS,
> > which happens to be a clock and reset controller. This is quite common,
> > and we usually have one node with both #clock-cells and #reset-cells in
> > it. Then we use the auxiliary bus to create the reset device from the
> > clk driver with the same node. This helps match the device in the
> > datasheet to the node and compatible in DT without making the compatible
> > provider specific (clk or reset postfix).
> > 
> > That's another reason why we usually have one node. DT doesn't describe
> > software, i.e. the split between clk and reset frameworks may not exist
> > in other operating systems. We don't want to put the software design
> > decisions into the DT.
> > 
> > It may also be that a device like this consumes shared power resources
> > like clks or regulators that need to be enabled to drive the device, or
> > an IOMMU is used to translate the register mappings. We wouldn't want to
> > split the device in DT in that case so we can easily manage the power
> > resources or memory mappings for the device.
> > 
> > TL;DR: This is probably OK, but I'd be careful to not make it a thing.
> 
> Thank you very much for the comprehensive explanation. Because the
> registers don’t overlap, it’s fine in this case. Since Drew also seem to
> agree, we can probably push these patches forward, while keeping in mind
> that for future SoCs it would be better to use a single node.

Yes, I think in this instance it makes sense to go ahead. I sent a pull
request [1] to Stephen for my thead clk for-next tree with this series
applied.

Thanks,
Drew

[1] https://lore.kernel.org/all/aBus+Yc7kf%2FH2HE5@x1/