[RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A

[RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A

[Michal Wilczynski]

The LicheePi 4A board, featuring the T-HEAD TH1520 SoC, includes an Imagination
Technologies BXM-4-64 GPU. Initial support for this GPU was provided through a
downstream driver [1]. Recently, efforts have been made to upstream support for
the Rogue family GPUs, which the BXM-4-64 is part of [2].

While the initial upstream driver focused on the AXE-1-16 GPU, newer patches
have introduced support for the BXS-4-64 GPU [3]. The modern upstream
drm/imagination driver is expected to support the BXM-4-64 as well [4][5]. As
this support is being developed, it's crucial to upstream the necessary glue
code including clock and power-domain drivers so they're ready for integration
with the drm/imagination driver.

Recent Progress:

Firmware Improvements:
Since August, the vendor has provided updated firmware
[6][7] that correctly initiates the firmware for the BXM-4-64.

Mesa Driver Testing:
The vendor-supplied Mesa driver [8] partially works with Vulkan examples, such
as rendering a triangle using Sascha Willems' Vulkan samples [9]. Although the
triangle isn't rendered correctly (only the blue background appears), shader
job submissions function properly, and IOCTL calls are correctly invoked.  For
testing, we used the following resources:

Kernel Source: Custom kernel with necessary modifications [10].
Mesa Driver: Vendor-provided Mesa implementation [11].

Dependencies:
Testing required a functional Display Processing Unit (DPU) and HDMI driver,
which are currently not upstreamed. Efforts are underway to upstream the DPU
DC8200 driver used in StarFive boards [12], which is the same DPU used on the
LicheePi 4A. Once the DPU and HDMI drivers are upstreamed, GPU support can be
fully upstream.

Testing Status:
This series has been tested by performing a probe-only operation, confirming
that the firmware begins execution. The probe function initiates firmware
execution and waits for the firmware to flip a specific status bit.

[   12.637880] powervr ffef400000.gpu: [drm] loaded firmware powervr/rogue_36.52.104.182_v1.fw
[   12.648979] powervr ffef400000.gpu: [drm] FW version v1.0 (build 6645434 OS)
[   12.678906] [drm] Initialized powervr 1.0.0 for ffef400000.gpu on minor 0

Power Management:
Full power management capabilities require implementing the T-HEAD SoC AON
protocol messaging via the hardware mailbox. While support for the mailbox was
merged in kernel 6.13 [13], the AON protocol implementation is still in development.
Therefore, this series focuses on preparing the groundwork without full power
management support at this time.

References:

[1] Downstream Driver Source:
    https://gitlab.freedesktop.org/frankbinns/powervr/-/blob/cb1929932095649a24f051b9cfdd2cd2ceab35cb/drivers/gpu/drm/img-rogue/Kconfig

[2] Initial Upstream Driver Series:
    https://lore.kernel.org/all/cover.1700668843.git.donald.robson@imgtec.com/

[3] BXS-4-64 GPU Support Patches:
    https://lore.kernel.org/all/20241105-sets-bxs-4-64-patch-v1-v1-0-4ed30e865892@imgtec.com/

[4] Firmware Issue Discussion 1:
    https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/1

[5] Firmware Issue Discussion 2:
    https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/2

[6] Firmware Update Commit 1:
    https://gitlab.freedesktop.org/imagination/linux-firmware/-/commit/6ac2247e9a1d1837af495fb6d0fbd6f35547c2d1

[7] Firmware Update Commit 2:
    https://gitlab.freedesktop.org/imagination/linux-firmware/-/commit/efbebc90f25adb2b2e1499e3cc24ea3f3c3e4f4c

[8] Vendor-Provided Mesa Driver:
    https://gitlab.freedesktop.org/imagination/mesa/-/tree/dev/devinfo

[9] Sascha Willems' Vulkan Samples:     https://github.com/SaschaWillems/Vulkan

[10] Test Kernel Source:
    https://github.com/mwilczy/linux/tree/2_December_reference_linux_kernel_imagination

[11] Test Mesa Driver:
    https://github.com/mwilczy/mesa-reference

[12] DPU DC8200 Driver Upstream Attempt:
    https://lore.kernel.org/all/20241120061848.196754-1-keith.zhao@starfivetech.com/

[13] Pull request kernel 6.13 for mailbox
    https://lore.kernel.org/all/CABb+yY33qnivK-PzqpSMgmtbFid4nS8wcNvP7wED9DXrYAyLKg@mail.gmail.com/

Michal Wilczynski (14):
  clk: thead: Refactor TH1520 clock driver to share common code
  dt-bindings: clock: thead,th1520: Rename header file
  clk: thead: Enable clock gates with regmaps
  clk: thead: Add clock driver for TH1520 Video Output subsystem
  dt-bindings: clock: thead,th1520: Add support for Video Output
    subsystem
  dt-bindings: clock: thead,th1520: Rename YAML schema file
  soc: thead: power-domain: Add skeleton power-domain driver for TH1520
  dt-bindings: power: thead,th1520: Add support for power domains
  riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs
  drm/imagination: Add support for IMG BXM-4-64 GPU
  drm/imagination: Enable PowerVR driver for RISC-V
  riscv: dts: Add Video Output clock and syscon regmap nodes
  riscv: dts: Introduce power domain node with simple-bus compatible
  riscv: dts: Add GPU node to TH1520 device tree

 .../bindings/clock/thead,th1520-clk-ap.yaml   |  53 ---
 .../bindings/clock/thead,th1520-clk.yaml      |  72 +++++
 .../bindings/mailbox/thead,th1520-mbox.yaml   |   2 +-
 .../bindings/power/thead,th1520-power.yaml    |  52 +++
 MAINTAINERS                                   |   9 +-
 arch/riscv/Kconfig.socs                       |   1 +
 arch/riscv/boot/dts/thead/th1520.dtsi         |  37 ++-
 drivers/clk/thead/Kconfig                     |  11 +
 drivers/clk/thead/Makefile                    |   3 +-
 drivers/clk/thead/clk-th1520-ap.c             | 301 +-----------------
 drivers/clk/thead/clk-th1520-vo.c             | 168 ++++++++++
 drivers/clk/thead/clk-th1520.c                | 194 +++++++++++
 drivers/clk/thead/clk-th1520.h                | 149 +++++++++
 drivers/gpu/drm/imagination/Kconfig           |   2 +-
 drivers/gpu/drm/imagination/pvr_drv.c         |   1 +
 drivers/pmdomain/Kconfig                      |   1 +
 drivers/pmdomain/Makefile                     |   1 +
 drivers/pmdomain/thead/Kconfig                |  12 +
 drivers/pmdomain/thead/Makefile               |   2 +
 drivers/pmdomain/thead/th1520-pm-domains.c    | 195 ++++++++++++
 ...ead,th1520-clk-ap.h => thead,th1520-clk.h} |  34 ++
 .../dt-bindings/power/thead,th1520-power.h    |  19 ++
 22 files changed, 960 insertions(+), 359 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
 create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml
 create mode 100644 drivers/clk/thead/clk-th1520-vo.c
 create mode 100644 drivers/clk/thead/clk-th1520.c
 create mode 100644 drivers/clk/thead/clk-th1520.h
 create mode 100644 drivers/pmdomain/thead/Kconfig
 create mode 100644 drivers/pmdomain/thead/Makefile
 create mode 100644 drivers/pmdomain/thead/th1520-pm-domains.c
 rename include/dt-bindings/clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} (71%)
 create mode 100644 include/dt-bindings/power/thead,th1520-power.h

-- 
2.34.1

[RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code

[Michal Wilczynski]

The T-Head TH1520 SoC includes various clocks for different subsystems
like Application Processor (AP) and Video Output (VO) [1]. Currently, the
clock driver only implements AP clocks.

Since the programming interface for these clocks is identical across
subsystems, refactor the code to move common functions into
clk-th1520.c. This prepares the driver to support VO clocks by reducing
code duplication and improving maintainability.

No functional changes are introduced with this refactoring.

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>
---
 MAINTAINERS                       |   2 +-
 drivers/clk/thead/Makefile        |   2 +-
 drivers/clk/thead/clk-th1520-ap.c | 301 +-----------------------------
 drivers/clk/thead/clk-th1520.c    | 188 +++++++++++++++++++
 drivers/clk/thead/clk-th1520.h    | 134 +++++++++++++
 5 files changed, 326 insertions(+), 301 deletions(-)
 create mode 100644 drivers/clk/thead/clk-th1520.c
 create mode 100644 drivers/clk/thead/clk-th1520.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1e930c7a58b1..7c85abf1dd1e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20188,7 +20188,7 @@ F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
 F:	Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
 F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
 F:	arch/riscv/boot/dts/thead/
-F:	drivers/clk/thead/clk-th1520-ap.c
+F:	drivers/clk/thead/
 F:	drivers/mailbox/mailbox-th1520.c
 F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
 F:	drivers/pinctrl/pinctrl-th1520.c
diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
index 7ee0bec1f251..d7cf88390b69 100644
--- a/drivers/clk/thead/Makefile
+++ b/drivers/clk/thead/Makefile
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
+obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index 17e32ae08720..a6015805b859 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -5,297 +5,9 @@
  *  Authors: Yangtao Li <frank.li@vivo.com>
  */
 
-#include <dt-bindings/clock/thead,th1520-clk-ap.h>
-#include <linux/bitfield.h>
-#include <linux/clk-provider.h>
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/regmap.h>
-
-#define TH1520_PLL_POSTDIV2	GENMASK(26, 24)
-#define TH1520_PLL_POSTDIV1	GENMASK(22, 20)
-#define TH1520_PLL_FBDIV	GENMASK(19, 8)
-#define TH1520_PLL_REFDIV	GENMASK(5, 0)
-#define TH1520_PLL_BYPASS	BIT(30)
-#define TH1520_PLL_DSMPD	BIT(24)
-#define TH1520_PLL_FRAC		GENMASK(23, 0)
-#define TH1520_PLL_FRAC_BITS    24
-
-struct ccu_internal {
-	u8	shift;
-	u8	width;
-};
-
-struct ccu_div_internal {
-	u8	shift;
-	u8	width;
-	u32	flags;
-};
-
-struct ccu_common {
-	int		clkid;
-	struct regmap	*map;
-	u16		cfg0;
-	u16		cfg1;
-	struct clk_hw	hw;
-};
-
-struct ccu_mux {
-	struct ccu_internal	mux;
-	struct ccu_common	common;
-};
-
-struct ccu_gate {
-	u32			enable;
-	struct ccu_common	common;
-};
-
-struct ccu_div {
-	u32			enable;
-	struct ccu_div_internal	div;
-	struct ccu_internal	mux;
-	struct ccu_common	common;
-};
-
-struct ccu_pll {
-	struct ccu_common	common;
-};
-
-#define TH_CCU_ARG(_shift, _width)					\
-	{								\
-		.shift	= _shift,					\
-		.width	= _width,					\
-	}
-
-#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)			\
-	{								\
-		.shift	= _shift,					\
-		.width	= _width,					\
-		.flags	= _flags,					\
-	}
-
-#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags)	\
-	struct ccu_gate _struct = {					\
-		.enable	= _gate,					\
-		.common	= {						\
-			.clkid		= _clkid,			\
-			.cfg0		= _reg,				\
-			.hw.init	= CLK_HW_INIT_PARENTS_DATA(	\
-						_name,			\
-						_parent,		\
-						&clk_gate_ops,		\
-						_flags),		\
-		}							\
-	}
-
-static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
-{
-	return container_of(hw, struct ccu_common, hw);
-}
-
-static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
-{
-	struct ccu_common *common = hw_to_ccu_common(hw);
-
-	return container_of(common, struct ccu_mux, common);
-}
-
-static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
-{
-	struct ccu_common *common = hw_to_ccu_common(hw);
+#include "clk-th1520.h"
 
-	return container_of(common, struct ccu_pll, common);
-}
-
-static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
-{
-	struct ccu_common *common = hw_to_ccu_common(hw);
-
-	return container_of(common, struct ccu_div, common);
-}
-
-static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
-{
-	struct ccu_common *common = hw_to_ccu_common(hw);
-
-	return container_of(common, struct ccu_gate, common);
-}
-
-static u8 ccu_get_parent_helper(struct ccu_common *common,
-				struct ccu_internal *mux)
-{
-	unsigned int val;
-	u8 parent;
-
-	regmap_read(common->map, common->cfg0, &val);
-	parent = val >> mux->shift;
-	parent &= GENMASK(mux->width - 1, 0);
-
-	return parent;
-}
-
-static int ccu_set_parent_helper(struct ccu_common *common,
-				 struct ccu_internal *mux,
-				 u8 index)
-{
-	return regmap_update_bits(common->map, common->cfg0,
-			GENMASK(mux->width - 1, 0) << mux->shift,
-			index << mux->shift);
-}
-
-static void ccu_disable_helper(struct ccu_common *common, u32 gate)
-{
-	if (!gate)
-		return;
-	regmap_update_bits(common->map, common->cfg0,
-			   gate, ~gate);
-}
-
-static int ccu_enable_helper(struct ccu_common *common, u32 gate)
-{
-	unsigned int val;
-	int ret;
-
-	if (!gate)
-		return 0;
-
-	ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
-	regmap_read(common->map, common->cfg0, &val);
-	return ret;
-}
-
-static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
-{
-	unsigned int val;
-
-	if (!gate)
-		return true;
-
-	regmap_read(common->map, common->cfg0, &val);
-	return val & gate;
-}
-
-static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
-					 unsigned long parent_rate)
-{
-	struct ccu_div *cd = hw_to_ccu_div(hw);
-	unsigned long rate;
-	unsigned int val;
-
-	regmap_read(cd->common.map, cd->common.cfg0, &val);
-	val = val >> cd->div.shift;
-	val &= GENMASK(cd->div.width - 1, 0);
-	rate = divider_recalc_rate(hw, parent_rate, val, NULL,
-				   cd->div.flags, cd->div.width);
-
-	return rate;
-}
-
-static u8 ccu_div_get_parent(struct clk_hw *hw)
-{
-	struct ccu_div *cd = hw_to_ccu_div(hw);
-
-	return ccu_get_parent_helper(&cd->common, &cd->mux);
-}
-
-static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
-{
-	struct ccu_div *cd = hw_to_ccu_div(hw);
-
-	return ccu_set_parent_helper(&cd->common, &cd->mux, index);
-}
-
-static void ccu_div_disable(struct clk_hw *hw)
-{
-	struct ccu_div *cd = hw_to_ccu_div(hw);
-
-	ccu_disable_helper(&cd->common, cd->enable);
-}
-
-static int ccu_div_enable(struct clk_hw *hw)
-{
-	struct ccu_div *cd = hw_to_ccu_div(hw);
-
-	return ccu_enable_helper(&cd->common, cd->enable);
-}
-
-static int ccu_div_is_enabled(struct clk_hw *hw)
-{
-	struct ccu_div *cd = hw_to_ccu_div(hw);
-
-	return ccu_is_enabled_helper(&cd->common, cd->enable);
-}
-
-static const struct clk_ops ccu_div_ops = {
-	.disable	= ccu_div_disable,
-	.enable		= ccu_div_enable,
-	.is_enabled	= ccu_div_is_enabled,
-	.get_parent	= ccu_div_get_parent,
-	.set_parent	= ccu_div_set_parent,
-	.recalc_rate	= ccu_div_recalc_rate,
-	.determine_rate	= clk_hw_determine_rate_no_reparent,
-};
-
-static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
-						unsigned long parent_rate)
-{
-	struct ccu_pll *pll = hw_to_ccu_pll(hw);
-	unsigned long div, mul, frac;
-	unsigned int cfg0, cfg1;
-	u64 rate = parent_rate;
-
-	regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
-	regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
-
-	mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
-	div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
-	if (!(cfg1 & TH1520_PLL_DSMPD)) {
-		mul <<= TH1520_PLL_FRAC_BITS;
-		frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
-		mul += frac;
-		div <<= TH1520_PLL_FRAC_BITS;
-	}
-	rate = parent_rate * mul;
-	rate = rate / div;
-	return rate;
-}
-
-static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
-						    unsigned long parent_rate)
-{
-	struct ccu_pll *pll = hw_to_ccu_pll(hw);
-	unsigned long div, rate = parent_rate;
-	unsigned int cfg0, cfg1;
-
-	regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
-	regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
-
-	if (cfg1 & TH1520_PLL_BYPASS)
-		return rate;
-
-	div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
-	      FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
-
-	rate = rate / div;
-
-	return rate;
-}
-
-static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
-					 unsigned long parent_rate)
-{
-	unsigned long rate = parent_rate;
-
-	rate = th1520_pll_vco_recalc_rate(hw, rate);
-	rate = th1520_pll_postdiv_recalc_rate(hw, rate);
-
-	return rate;
-}
-
-static const struct clk_ops clk_pll_ops = {
-	.recalc_rate	= ccu_pll_recalc_rate,
-};
+#define NR_CLKS	(CLK_UART_SCLK + 1)
 
 static const struct clk_parent_data osc_24m_clk[] = {
 	{ .index = 0 }
@@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
 	&sram3_clk.common,
 };
 
-#define NR_CLKS	(CLK_UART_SCLK + 1)
-
-static const struct regmap_config th1520_clk_regmap_config = {
-	.reg_bits = 32,
-	.val_bits = 32,
-	.reg_stride = 4,
-	.fast_io = true,
-};
-
 static int th1520_clk_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
new file mode 100644
index 000000000000..e2bfe56de9af
--- /dev/null
+++ b/drivers/clk/thead/clk-th1520.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
+ * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
+ *  Authors: Yangtao Li <frank.li@vivo.com>
+ */
+
+#include "clk-th1520.h"
+
+static u8 ccu_get_parent_helper(struct ccu_common *common,
+				struct ccu_internal *mux)
+{
+	unsigned int val;
+	u8 parent;
+
+	regmap_read(common->map, common->cfg0, &val);
+	parent = val >> mux->shift;
+	parent &= GENMASK(mux->width - 1, 0);
+
+	return parent;
+}
+
+static int ccu_set_parent_helper(struct ccu_common *common,
+				 struct ccu_internal *mux, u8 index)
+{
+	return regmap_update_bits(common->map, common->cfg0,
+				  GENMASK(mux->width - 1, 0) << mux->shift,
+				  index << mux->shift);
+}
+
+static void ccu_disable_helper(struct ccu_common *common, u32 gate)
+{
+	if (!gate)
+		return;
+	regmap_update_bits(common->map, common->cfg0, gate, ~gate);
+}
+
+static int ccu_enable_helper(struct ccu_common *common, u32 gate)
+{
+	unsigned int val;
+	int ret;
+
+	if (!gate)
+		return 0;
+
+	ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
+	regmap_read(common->map, common->cfg0, &val);
+	return ret;
+}
+
+static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
+{
+	unsigned int val;
+
+	if (!gate)
+		return true;
+
+	regmap_read(common->map, common->cfg0, &val);
+	return val & gate;
+}
+
+static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct ccu_div *cd = hw_to_ccu_div(hw);
+	unsigned long rate;
+	unsigned int val;
+
+	regmap_read(cd->common.map, cd->common.cfg0, &val);
+	val = val >> cd->div.shift;
+	val &= GENMASK(cd->div.width - 1, 0);
+	rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
+				   cd->div.width);
+
+	return rate;
+}
+
+static u8 ccu_div_get_parent(struct clk_hw *hw)
+{
+	struct ccu_div *cd = hw_to_ccu_div(hw);
+
+	return ccu_get_parent_helper(&cd->common, &cd->mux);
+}
+
+static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct ccu_div *cd = hw_to_ccu_div(hw);
+
+	return ccu_set_parent_helper(&cd->common, &cd->mux, index);
+}
+
+static void ccu_div_disable(struct clk_hw *hw)
+{
+	struct ccu_div *cd = hw_to_ccu_div(hw);
+
+	ccu_disable_helper(&cd->common, cd->enable);
+}
+
+static int ccu_div_enable(struct clk_hw *hw)
+{
+	struct ccu_div *cd = hw_to_ccu_div(hw);
+
+	return ccu_enable_helper(&cd->common, cd->enable);
+}
+
+static int ccu_div_is_enabled(struct clk_hw *hw)
+{
+	struct ccu_div *cd = hw_to_ccu_div(hw);
+
+	return ccu_is_enabled_helper(&cd->common, cd->enable);
+}
+
+const struct clk_ops ccu_div_ops = {
+	.disable = ccu_div_disable,
+	.enable = ccu_div_enable,
+	.is_enabled = ccu_div_is_enabled,
+	.get_parent = ccu_div_get_parent,
+	.set_parent = ccu_div_set_parent,
+	.recalc_rate = ccu_div_recalc_rate,
+	.determine_rate = clk_hw_determine_rate_no_reparent,
+};
+
+static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct ccu_pll *pll = hw_to_ccu_pll(hw);
+	unsigned long div, mul, frac;
+	unsigned int cfg0, cfg1;
+	u64 rate = parent_rate;
+
+	regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
+	regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
+
+	mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
+	div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
+	if (!(cfg1 & TH1520_PLL_DSMPD)) {
+		mul <<= TH1520_PLL_FRAC_BITS;
+		frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
+		mul += frac;
+		div <<= TH1520_PLL_FRAC_BITS;
+	}
+	rate = parent_rate * mul;
+	rate = rate / div;
+	return rate;
+}
+
+static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
+						    unsigned long parent_rate)
+{
+	struct ccu_pll *pll = hw_to_ccu_pll(hw);
+	unsigned long div, rate = parent_rate;
+	unsigned int cfg0, cfg1;
+
+	regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
+	regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
+
+	if (cfg1 & TH1520_PLL_BYPASS)
+		return rate;
+
+	div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
+	      FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
+
+	rate = rate / div;
+
+	return rate;
+}
+
+static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	unsigned long rate = parent_rate;
+
+	rate = th1520_pll_vco_recalc_rate(hw, rate);
+	rate = th1520_pll_postdiv_recalc_rate(hw, rate);
+
+	return rate;
+}
+
+const struct clk_ops clk_pll_ops = {
+	.recalc_rate = ccu_pll_recalc_rate,
+};
+
+const struct regmap_config th1520_clk_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
new file mode 100644
index 000000000000..285d41e65008
--- /dev/null
+++ b/drivers/clk/thead/clk-th1520.h
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
+ * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
+ *  Authors: Yangtao Li <frank.li@vivo.com>
+ *
+ * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
+ */
+
+#ifndef CLK_TH1520_H
+#define CLK_TH1520_H
+
+#include <dt-bindings/clock/thead,th1520-clk-ap.h>
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define TH1520_PLL_POSTDIV2	GENMASK(26, 24)
+#define TH1520_PLL_POSTDIV1	GENMASK(22, 20)
+#define TH1520_PLL_FBDIV	GENMASK(19, 8)
+#define TH1520_PLL_REFDIV	GENMASK(5, 0)
+#define TH1520_PLL_BYPASS	BIT(30)
+#define TH1520_PLL_DSMPD	BIT(24)
+#define TH1520_PLL_FRAC		GENMASK(23, 0)
+#define TH1520_PLL_FRAC_BITS    24
+
+struct ccu_internal {
+	u8	shift;
+	u8	width;
+};
+
+struct ccu_div_internal {
+	u8	shift;
+	u8	width;
+	u32	flags;
+};
+
+struct ccu_common {
+	int		clkid;
+	struct regmap	*map;
+	u16		cfg0;
+	u16		cfg1;
+	struct clk_hw	hw;
+};
+
+struct ccu_mux {
+	struct ccu_internal	mux;
+	struct ccu_common	common;
+};
+
+struct ccu_gate {
+	u32			enable;
+	struct ccu_common	common;
+};
+
+struct ccu_div {
+	u32			enable;
+	struct ccu_div_internal	div;
+	struct ccu_internal	mux;
+	struct ccu_common	common;
+};
+
+struct ccu_pll {
+	struct ccu_common	common;
+};
+
+#define TH_CCU_ARG(_shift, _width)					\
+	{								\
+		.shift	= _shift,					\
+		.width	= _width,					\
+	}
+
+#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)			\
+	{								\
+		.shift	= _shift,					\
+		.width	= _width,					\
+		.flags	= _flags,					\
+	}
+
+#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags)	\
+	struct ccu_gate _struct = {					\
+		.enable	= _gate,					\
+		.common	= {						\
+			.clkid		= _clkid,			\
+			.cfg0		= _reg,				\
+			.hw.init	= CLK_HW_INIT_PARENTS_DATA(	\
+						_name,			\
+						_parent,		\
+						&clk_gate_ops,		\
+						_flags),		\
+		}							\
+	}
+
+static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
+{
+	return container_of(hw, struct ccu_common, hw);
+}
+
+static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
+{
+	struct ccu_common *common = hw_to_ccu_common(hw);
+
+	return container_of(common, struct ccu_mux, common);
+}
+
+static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
+{
+	struct ccu_common *common = hw_to_ccu_common(hw);
+
+	return container_of(common, struct ccu_pll, common);
+}
+
+static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
+{
+	struct ccu_common *common = hw_to_ccu_common(hw);
+
+	return container_of(common, struct ccu_div, common);
+}
+
+static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
+{
+	struct ccu_common *common = hw_to_ccu_common(hw);
+
+	return container_of(common, struct ccu_gate, common);
+}
+
+extern const struct clk_ops ccu_div_ops;
+extern const struct clk_ops clk_pll_ops;
+extern const struct regmap_config th1520_clk_regmap_config;
+
+#endif /* CLK_TH1520_H */
-- 
2.34.1

Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code

[Stephen Boyd]

Quoting Michal Wilczynski (2024-12-03 05:41:24)
> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
> index 7ee0bec1f251..d7cf88390b69 100644
> --- a/drivers/clk/thead/Makefile
> +++ b/drivers/clk/thead/Makefile
> @@ -1,2 +1,2 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o

Can the -ap driver be extended instead? Or are the clks in a different
IO region?

> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index 17e32ae08720..a6015805b859 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -5,297 +5,9 @@
>   *  Authors: Yangtao Li <frank.li@vivo.com>
>   */
>  
> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>

Presumably this should stay here.

> -#include <linux/bitfield.h>
> -#include <linux/clk-provider.h>
> -#include <linux/device.h>
> -#include <linux/module.h>
> -#include <linux/platform_device.h>
> -#include <linux/regmap.h>

These should all stay here as well.

> -
> -#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
> -#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
> -#define TH1520_PLL_FBDIV       GENMASK(19, 8)
> -#define TH1520_PLL_REFDIV      GENMASK(5, 0)
> -#define TH1520_PLL_BYPASS      BIT(30)
> -#define TH1520_PLL_DSMPD       BIT(24)
> -#define TH1520_PLL_FRAC                GENMASK(23, 0)
> -#define TH1520_PLL_FRAC_BITS    24
> -
> -struct ccu_internal {
> -       u8      shift;
> -       u8      width;
> -};
> -
> -struct ccu_div_internal {
> -       u8      shift;
> -       u8      width;
> -       u32     flags;
> -};
> -
> -struct ccu_common {
> -       int             clkid;
> -       struct regmap   *map;
> -       u16             cfg0;
> -       u16             cfg1;
> -       struct clk_hw   hw;
> -};
> -
> -struct ccu_mux {
> -       struct ccu_internal     mux;
> -       struct ccu_common       common;
> -};
> -
> -struct ccu_gate {
> -       u32                     enable;
> -       struct ccu_common       common;
> -};
> -
> -struct ccu_div {
> -       u32                     enable;
> -       struct ccu_div_internal div;
> -       struct ccu_internal     mux;
> -       struct ccu_common       common;
> -};
> -
> -struct ccu_pll {
> -       struct ccu_common       common;
> -};
> -
> -#define TH_CCU_ARG(_shift, _width)                                     \
> -       {                                                               \
> -               .shift  = _shift,                                       \
> -               .width  = _width,                                       \
> -       }
> -
> -#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)                       \
> -       {                                                               \
> -               .shift  = _shift,                                       \
> -               .width  = _width,                                       \
> -               .flags  = _flags,                                       \
> -       }
> -
> -#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
> -       struct ccu_gate _struct = {                                     \
> -               .enable = _gate,                                        \
> -               .common = {                                             \
> -                       .clkid          = _clkid,                       \
> -                       .cfg0           = _reg,                         \
> -                       .hw.init        = CLK_HW_INIT_PARENTS_DATA(     \
> -                                               _name,                  \
> -                                               _parent,                \
> -                                               &clk_gate_ops,          \
> -                                               _flags),                \
> -               }                                                       \
> -       }
> -
> -static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> -{
> -       return container_of(hw, struct ccu_common, hw);
> -}
> -
> -static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
> -{
> -       struct ccu_common *common = hw_to_ccu_common(hw);
> -
> -       return container_of(common, struct ccu_mux, common);
> -}
> -
> -static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
> -{
> -       struct ccu_common *common = hw_to_ccu_common(hw);
> +#include "clk-th1520.h"
>  
> -       return container_of(common, struct ccu_pll, common);
> -}
> -
> -static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
> -{
> -       struct ccu_common *common = hw_to_ccu_common(hw);
> -
> -       return container_of(common, struct ccu_div, common);
> -}
> -
> -static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
> -{
> -       struct ccu_common *common = hw_to_ccu_common(hw);
> -
> -       return container_of(common, struct ccu_gate, common);
> -}
> -
> -static u8 ccu_get_parent_helper(struct ccu_common *common,
> -                               struct ccu_internal *mux)
> -{
> -       unsigned int val;
> -       u8 parent;
> -
> -       regmap_read(common->map, common->cfg0, &val);
> -       parent = val >> mux->shift;
> -       parent &= GENMASK(mux->width - 1, 0);
> -
> -       return parent;
> -}
> -
> -static int ccu_set_parent_helper(struct ccu_common *common,
> -                                struct ccu_internal *mux,
> -                                u8 index)
> -{
> -       return regmap_update_bits(common->map, common->cfg0,
> -                       GENMASK(mux->width - 1, 0) << mux->shift,
> -                       index << mux->shift);
> -}
> -
> -static void ccu_disable_helper(struct ccu_common *common, u32 gate)
> -{
> -       if (!gate)
> -               return;
> -       regmap_update_bits(common->map, common->cfg0,
> -                          gate, ~gate);
> -}
> -
> -static int ccu_enable_helper(struct ccu_common *common, u32 gate)
> -{
> -       unsigned int val;
> -       int ret;
> -
> -       if (!gate)
> -               return 0;
> -
> -       ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
> -       regmap_read(common->map, common->cfg0, &val);
> -       return ret;
> -}
> -
> -static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
> -{
> -       unsigned int val;
> -
> -       if (!gate)
> -               return true;
> -
> -       regmap_read(common->map, common->cfg0, &val);
> -       return val & gate;
> -}
> -
> -static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
> -                                        unsigned long parent_rate)
> -{
> -       struct ccu_div *cd = hw_to_ccu_div(hw);
> -       unsigned long rate;
> -       unsigned int val;
> -
> -       regmap_read(cd->common.map, cd->common.cfg0, &val);
> -       val = val >> cd->div.shift;
> -       val &= GENMASK(cd->div.width - 1, 0);
> -       rate = divider_recalc_rate(hw, parent_rate, val, NULL,
> -                                  cd->div.flags, cd->div.width);
> -
> -       return rate;
> -}
> -
> -static u8 ccu_div_get_parent(struct clk_hw *hw)
> -{
> -       struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> -       return ccu_get_parent_helper(&cd->common, &cd->mux);
> -}
> -
> -static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
> -{
> -       struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> -       return ccu_set_parent_helper(&cd->common, &cd->mux, index);
> -}
> -
> -static void ccu_div_disable(struct clk_hw *hw)
> -{
> -       struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> -       ccu_disable_helper(&cd->common, cd->enable);
> -}
> -
> -static int ccu_div_enable(struct clk_hw *hw)
> -{
> -       struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> -       return ccu_enable_helper(&cd->common, cd->enable);
> -}
> -
> -static int ccu_div_is_enabled(struct clk_hw *hw)
> -{
> -       struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> -       return ccu_is_enabled_helper(&cd->common, cd->enable);
> -}
> -
> -static const struct clk_ops ccu_div_ops = {
> -       .disable        = ccu_div_disable,
> -       .enable         = ccu_div_enable,
> -       .is_enabled     = ccu_div_is_enabled,
> -       .get_parent     = ccu_div_get_parent,
> -       .set_parent     = ccu_div_set_parent,
> -       .recalc_rate    = ccu_div_recalc_rate,
> -       .determine_rate = clk_hw_determine_rate_no_reparent,
> -};
> -
> -static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
> -                                               unsigned long parent_rate)
> -{
> -       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> -       unsigned long div, mul, frac;
> -       unsigned int cfg0, cfg1;
> -       u64 rate = parent_rate;
> -
> -       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> -       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> -
> -       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
> -       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
> -       if (!(cfg1 & TH1520_PLL_DSMPD)) {
> -               mul <<= TH1520_PLL_FRAC_BITS;
> -               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
> -               mul += frac;
> -               div <<= TH1520_PLL_FRAC_BITS;
> -       }
> -       rate = parent_rate * mul;
> -       rate = rate / div;
> -       return rate;
> -}
> -
> -static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
> -                                                   unsigned long parent_rate)
> -{
> -       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> -       unsigned long div, rate = parent_rate;
> -       unsigned int cfg0, cfg1;
> -
> -       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> -       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> -
> -       if (cfg1 & TH1520_PLL_BYPASS)
> -               return rate;
> -
> -       div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
> -             FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
> -
> -       rate = rate / div;
> -
> -       return rate;
> -}
> -
> -static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> -                                        unsigned long parent_rate)
> -{
> -       unsigned long rate = parent_rate;
> -
> -       rate = th1520_pll_vco_recalc_rate(hw, rate);
> -       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
> -
> -       return rate;
> -}
> -
> -static const struct clk_ops clk_pll_ops = {
> -       .recalc_rate    = ccu_pll_recalc_rate,
> -};
> +#define NR_CLKS        (CLK_UART_SCLK + 1)
>  
>  static const struct clk_parent_data osc_24m_clk[] = {
>         { .index = 0 }
> @@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
>         &sram3_clk.common,
>  };
>  
> -#define NR_CLKS        (CLK_UART_SCLK + 1)

Why did this move?

> -
> -static const struct regmap_config th1520_clk_regmap_config = {
> -       .reg_bits = 32,
> -       .val_bits = 32,
> -       .reg_stride = 4,
> -       .fast_io = true,
> -};
> -
>  static int th1520_clk_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
> new file mode 100644
> index 000000000000..e2bfe56de9af
> --- /dev/null
> +++ b/drivers/clk/thead/clk-th1520.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> + *  Authors: Yangtao Li <frank.li@vivo.com>
> + */
> +
> +#include "clk-th1520.h"

Explicitly include linux headers here, don't just put them all into a
header file. It helps us easily see what C files are using different
parts of the kernel infrastructure.

> +
> +static u8 ccu_get_parent_helper(struct ccu_common *common,
> +                               struct ccu_internal *mux)
> +{
> +       unsigned int val;
> +       u8 parent;
> +
> +       regmap_read(common->map, common->cfg0, &val);
> +       parent = val >> mux->shift;
> +       parent &= GENMASK(mux->width - 1, 0);
> +
> +       return parent;
> +}
> +
> +static int ccu_set_parent_helper(struct ccu_common *common,
> +                                struct ccu_internal *mux, u8 index)
> +{
> +       return regmap_update_bits(common->map, common->cfg0,
> +                                 GENMASK(mux->width - 1, 0) << mux->shift,
> +                                 index << mux->shift);
> +}
> +
> +static void ccu_disable_helper(struct ccu_common *common, u32 gate)
> +{
> +       if (!gate)
> +               return;
> +       regmap_update_bits(common->map, common->cfg0, gate, ~gate);
> +}
> +
> +static int ccu_enable_helper(struct ccu_common *common, u32 gate)
> +{
> +       unsigned int val;
> +       int ret;
> +
> +       if (!gate)
> +               return 0;
> +
> +       ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
> +       regmap_read(common->map, common->cfg0, &val);
> +       return ret;
> +}
> +
> +static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
> +{
> +       unsigned int val;
> +
> +       if (!gate)
> +               return true;
> +
> +       regmap_read(common->map, common->cfg0, &val);
> +       return val & gate;
> +}
> +
> +static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +       unsigned long rate;
> +       unsigned int val;
> +
> +       regmap_read(cd->common.map, cd->common.cfg0, &val);
> +       val = val >> cd->div.shift;
> +       val &= GENMASK(cd->div.width - 1, 0);
> +       rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
> +                                  cd->div.width);
> +
> +       return rate;
> +}
> +
> +static u8 ccu_div_get_parent(struct clk_hw *hw)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> +       return ccu_get_parent_helper(&cd->common, &cd->mux);
> +}
> +
> +static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> +       return ccu_set_parent_helper(&cd->common, &cd->mux, index);
> +}
> +
> +static void ccu_div_disable(struct clk_hw *hw)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> +       ccu_disable_helper(&cd->common, cd->enable);
> +}
> +
> +static int ccu_div_enable(struct clk_hw *hw)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> +       return ccu_enable_helper(&cd->common, cd->enable);
> +}
> +
> +static int ccu_div_is_enabled(struct clk_hw *hw)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> +       return ccu_is_enabled_helper(&cd->common, cd->enable);
> +}
> +
> +const struct clk_ops ccu_div_ops = {
> +       .disable = ccu_div_disable,
> +       .enable = ccu_div_enable,
> +       .is_enabled = ccu_div_is_enabled,
> +       .get_parent = ccu_div_get_parent,
> +       .set_parent = ccu_div_set_parent,
> +       .recalc_rate = ccu_div_recalc_rate,
> +       .determine_rate = clk_hw_determine_rate_no_reparent,
> +};
> +
> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +       unsigned long div, mul, frac;
> +       unsigned int cfg0, cfg1;
> +       u64 rate = parent_rate;
> +
> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> +
> +       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
> +       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
> +       if (!(cfg1 & TH1520_PLL_DSMPD)) {
> +               mul <<= TH1520_PLL_FRAC_BITS;
> +               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
> +               mul += frac;
> +               div <<= TH1520_PLL_FRAC_BITS;
> +       }
> +       rate = parent_rate * mul;
> +       rate = rate / div;
> +       return rate;
> +}
> +
> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
> +                                                   unsigned long parent_rate)
> +{
> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +       unsigned long div, rate = parent_rate;
> +       unsigned int cfg0, cfg1;
> +
> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> +
> +       if (cfg1 & TH1520_PLL_BYPASS)
> +               return rate;
> +
> +       div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
> +             FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
> +
> +       rate = rate / div;
> +
> +       return rate;
> +}
> +
> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       unsigned long rate = parent_rate;
> +
> +       rate = th1520_pll_vco_recalc_rate(hw, rate);
> +       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
> +
> +       return rate;
> +}
> +
> +const struct clk_ops clk_pll_ops = {
> +       .recalc_rate = ccu_pll_recalc_rate,
> +};
> +
> +const struct regmap_config th1520_clk_regmap_config = {

I don't get why this is moved.

> +       .reg_bits = 32,
> +       .val_bits = 32,
> +       .reg_stride = 4,
> +       .fast_io = true,
> +};
> diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
> new file mode 100644
> index 000000000000..285d41e65008
> --- /dev/null
> +++ b/drivers/clk/thead/clk-th1520.h
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> + *  Authors: Yangtao Li <frank.li@vivo.com>
> + *
> + * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
> + */
> +
> +#ifndef CLK_TH1520_H
> +#define CLK_TH1520_H
> +
> +#include <dt-bindings/clock/thead,th1520-clk-ap.h>

dt-bindings comes after linux includes, but this shouldn't be here
anyway.

> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>

Seems we have to have this one for clk_hw.

> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>

I don't see these includes used here, so remove them.

> +#include <linux/regmap.h>

Forward declare regmap and drop the include

struct regmap;

> +
> +#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
> +#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
> +#define TH1520_PLL_FBDIV       GENMASK(19, 8)
> +#define TH1520_PLL_REFDIV      GENMASK(5, 0)
> +#define TH1520_PLL_BYPASS      BIT(30)
> +#define TH1520_PLL_DSMPD       BIT(24)
> +#define TH1520_PLL_FRAC                GENMASK(23, 0)
> +#define TH1520_PLL_FRAC_BITS    24

Are these going to be used in multiple drivers?

> +
> +struct ccu_internal {
> +       u8      shift;
> +       u8      width;
> +};
> +
> +struct ccu_div_internal {
> +       u8      shift;
> +       u8      width;
> +       u32     flags;
> +};
> +
> +struct ccu_common {
> +       int             clkid;
> +       struct regmap   *map;
> +       u16             cfg0;
> +       u16             cfg1;
> +       struct clk_hw   hw;
> +};
> +
> +struct ccu_mux {
> +       struct ccu_internal     mux;
> +       struct ccu_common       common;
> +};
> +
> +struct ccu_gate {
> +       u32                     enable;
> +       struct ccu_common       common;
> +};
> +
> +struct ccu_div {
> +       u32                     enable;
> +       struct ccu_div_internal div;
> +       struct ccu_internal     mux;
> +       struct ccu_common       common;
> +};
> +
> +struct ccu_pll {
> +       struct ccu_common       common;
> +};
> +
> +#define TH_CCU_ARG(_shift, _width)                                     \
> +       {                                                               \
> +               .shift  = _shift,                                       \
> +               .width  = _width,                                       \
> +       }
> +
> +#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)                       \
> +       {                                                               \
> +               .shift  = _shift,                                       \
> +               .width  = _width,                                       \
> +               .flags  = _flags,                                       \
> +       }
> +
> +#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
> +       struct ccu_gate _struct = {                                     \
> +               .enable = _gate,                                        \
> +               .common = {                                             \
> +                       .clkid          = _clkid,                       \
> +                       .cfg0           = _reg,                         \
> +                       .hw.init        = CLK_HW_INIT_PARENTS_DATA(     \
> +                                               _name,                  \
> +                                               _parent,                \
> +                                               &clk_gate_ops,          \
> +                                               _flags),                \
> +               }                                                       \
> +       }
> +
> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct ccu_common, hw);
> +}
> +
> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
> +{
> +       struct ccu_common *common = hw_to_ccu_common(hw);
> +
> +       return container_of(common, struct ccu_mux, common);
> +}
> +
> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
> +{
> +       struct ccu_common *common = hw_to_ccu_common(hw);
> +
> +       return container_of(common, struct ccu_pll, common);
> +}
> +
> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
> +{
> +       struct ccu_common *common = hw_to_ccu_common(hw);
> +
> +       return container_of(common, struct ccu_div, common);
> +}
> +
> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
> +{
> +       struct ccu_common *common = hw_to_ccu_common(hw);
> +
> +       return container_of(common, struct ccu_gate, common);
> +}
> +
> +extern const struct clk_ops ccu_div_ops;
> +extern const struct clk_ops clk_pll_ops;
> +extern const struct regmap_config th1520_clk_regmap_config;

Why is the regmap config exported?

Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code

[Michal Wilczynski]

On 12/3/24 20:56, Stephen Boyd wrote:
> Quoting Michal Wilczynski (2024-12-03 05:41:24)
>> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
>> index 7ee0bec1f251..d7cf88390b69 100644
>> --- a/drivers/clk/thead/Makefile
>> +++ b/drivers/clk/thead/Makefile
>> @@ -1,2 +1,2 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
>> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
> 
> Can the -ap driver be extended instead? Or are the clks in a different
> IO region?

The Video Output (VO) clocks reside in a different address space as
defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate
driver made sense to maintain clarity and adhere to the existing
convention of having one driver per subsystem, similar to the
AP-specific driver.

[1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
> 
>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>> index 17e32ae08720..a6015805b859 100644
>> --- a/drivers/clk/thead/clk-th1520-ap.c
>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>> @@ -5,297 +5,9 @@
>>   *  Authors: Yangtao Li <frank.li@vivo.com>
>>   */
>>  
>> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
> 
> Presumably this should stay here.
> 
>> -#include <linux/bitfield.h>
>> -#include <linux/clk-provider.h>
>> -#include <linux/device.h>
>> -#include <linux/module.h>
>> -#include <linux/platform_device.h>
>> -#include <linux/regmap.h>
> 
> These should all stay here as well.
> 
>> -
>> -#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
>> -#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
>> -#define TH1520_PLL_FBDIV       GENMASK(19, 8)
>> -#define TH1520_PLL_REFDIV      GENMASK(5, 0)
>> -#define TH1520_PLL_BYPASS      BIT(30)
>> -#define TH1520_PLL_DSMPD       BIT(24)
>> -#define TH1520_PLL_FRAC                GENMASK(23, 0)
>> -#define TH1520_PLL_FRAC_BITS    24
>> -
>> -struct ccu_internal {
>> -       u8      shift;
>> -       u8      width;
>> -};
>> -
>> -struct ccu_div_internal {
>> -       u8      shift;
>> -       u8      width;
>> -       u32     flags;
>> -};
>> -
>> -struct ccu_common {
>> -       int             clkid;
>> -       struct regmap   *map;
>> -       u16             cfg0;
>> -       u16             cfg1;
>> -       struct clk_hw   hw;
>> -};
>> -
>> -struct ccu_mux {
>> -       struct ccu_internal     mux;
>> -       struct ccu_common       common;
>> -};
>> -
>> -struct ccu_gate {
>> -       u32                     enable;
>> -       struct ccu_common       common;
>> -};
>> -
>> -struct ccu_div {
>> -       u32                     enable;
>> -       struct ccu_div_internal div;
>> -       struct ccu_internal     mux;
>> -       struct ccu_common       common;
>> -};
>> -
>> -struct ccu_pll {
>> -       struct ccu_common       common;
>> -};
>> -
>> -#define TH_CCU_ARG(_shift, _width)                                     \
>> -       {                                                               \
>> -               .shift  = _shift,                                       \
>> -               .width  = _width,                                       \
>> -       }
>> -
>> -#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)                       \
>> -       {                                                               \
>> -               .shift  = _shift,                                       \
>> -               .width  = _width,                                       \
>> -               .flags  = _flags,                                       \
>> -       }
>> -
>> -#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
>> -       struct ccu_gate _struct = {                                     \
>> -               .enable = _gate,                                        \
>> -               .common = {                                             \
>> -                       .clkid          = _clkid,                       \
>> -                       .cfg0           = _reg,                         \
>> -                       .hw.init        = CLK_HW_INIT_PARENTS_DATA(     \
>> -                                               _name,                  \
>> -                                               _parent,                \
>> -                                               &clk_gate_ops,          \
>> -                                               _flags),                \
>> -               }                                                       \
>> -       }
>> -
>> -static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> -{
>> -       return container_of(hw, struct ccu_common, hw);
>> -}
>> -
>> -static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> -       return container_of(common, struct ccu_mux, common);
>> -}
>> -
>> -static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> +#include "clk-th1520.h"
>>  
>> -       return container_of(common, struct ccu_pll, common);
>> -}
>> -
>> -static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> -       return container_of(common, struct ccu_div, common);
>> -}
>> -
>> -static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> -       return container_of(common, struct ccu_gate, common);
>> -}
>> -
>> -static u8 ccu_get_parent_helper(struct ccu_common *common,
>> -                               struct ccu_internal *mux)
>> -{
>> -       unsigned int val;
>> -       u8 parent;
>> -
>> -       regmap_read(common->map, common->cfg0, &val);
>> -       parent = val >> mux->shift;
>> -       parent &= GENMASK(mux->width - 1, 0);
>> -
>> -       return parent;
>> -}
>> -
>> -static int ccu_set_parent_helper(struct ccu_common *common,
>> -                                struct ccu_internal *mux,
>> -                                u8 index)
>> -{
>> -       return regmap_update_bits(common->map, common->cfg0,
>> -                       GENMASK(mux->width - 1, 0) << mux->shift,
>> -                       index << mux->shift);
>> -}
>> -
>> -static void ccu_disable_helper(struct ccu_common *common, u32 gate)
>> -{
>> -       if (!gate)
>> -               return;
>> -       regmap_update_bits(common->map, common->cfg0,
>> -                          gate, ~gate);
>> -}
>> -
>> -static int ccu_enable_helper(struct ccu_common *common, u32 gate)
>> -{
>> -       unsigned int val;
>> -       int ret;
>> -
>> -       if (!gate)
>> -               return 0;
>> -
>> -       ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
>> -       regmap_read(common->map, common->cfg0, &val);
>> -       return ret;
>> -}
>> -
>> -static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
>> -{
>> -       unsigned int val;
>> -
>> -       if (!gate)
>> -               return true;
>> -
>> -       regmap_read(common->map, common->cfg0, &val);
>> -       return val & gate;
>> -}
>> -
>> -static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> -                                        unsigned long parent_rate)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -       unsigned long rate;
>> -       unsigned int val;
>> -
>> -       regmap_read(cd->common.map, cd->common.cfg0, &val);
>> -       val = val >> cd->div.shift;
>> -       val &= GENMASK(cd->div.width - 1, 0);
>> -       rate = divider_recalc_rate(hw, parent_rate, val, NULL,
>> -                                  cd->div.flags, cd->div.width);
>> -
>> -       return rate;
>> -}
>> -
>> -static u8 ccu_div_get_parent(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_get_parent_helper(&cd->common, &cd->mux);
>> -}
>> -
>> -static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_set_parent_helper(&cd->common, &cd->mux, index);
>> -}
>> -
>> -static void ccu_div_disable(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       ccu_disable_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static int ccu_div_enable(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_enable_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static int ccu_div_is_enabled(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_is_enabled_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static const struct clk_ops ccu_div_ops = {
>> -       .disable        = ccu_div_disable,
>> -       .enable         = ccu_div_enable,
>> -       .is_enabled     = ccu_div_is_enabled,
>> -       .get_parent     = ccu_div_get_parent,
>> -       .set_parent     = ccu_div_set_parent,
>> -       .recalc_rate    = ccu_div_recalc_rate,
>> -       .determine_rate = clk_hw_determine_rate_no_reparent,
>> -};
>> -
>> -static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
>> -                                               unsigned long parent_rate)
>> -{
>> -       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> -       unsigned long div, mul, frac;
>> -       unsigned int cfg0, cfg1;
>> -       u64 rate = parent_rate;
>> -
>> -       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> -       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> -
>> -       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
>> -       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
>> -       if (!(cfg1 & TH1520_PLL_DSMPD)) {
>> -               mul <<= TH1520_PLL_FRAC_BITS;
>> -               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
>> -               mul += frac;
>> -               div <<= TH1520_PLL_FRAC_BITS;
>> -       }
>> -       rate = parent_rate * mul;
>> -       rate = rate / div;
>> -       return rate;
>> -}
>> -
>> -static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
>> -                                                   unsigned long parent_rate)
>> -{
>> -       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> -       unsigned long div, rate = parent_rate;
>> -       unsigned int cfg0, cfg1;
>> -
>> -       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> -       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> -
>> -       if (cfg1 & TH1520_PLL_BYPASS)
>> -               return rate;
>> -
>> -       div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
>> -             FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
>> -
>> -       rate = rate / div;
>> -
>> -       return rate;
>> -}
>> -
>> -static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> -                                        unsigned long parent_rate)
>> -{
>> -       unsigned long rate = parent_rate;
>> -
>> -       rate = th1520_pll_vco_recalc_rate(hw, rate);
>> -       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
>> -
>> -       return rate;
>> -}
>> -
>> -static const struct clk_ops clk_pll_ops = {
>> -       .recalc_rate    = ccu_pll_recalc_rate,
>> -};
>> +#define NR_CLKS        (CLK_UART_SCLK + 1)
>>  
>>  static const struct clk_parent_data osc_24m_clk[] = {
>>         { .index = 0 }
>> @@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
>>         &sram3_clk.common,
>>  };
>>  
>> -#define NR_CLKS        (CLK_UART_SCLK + 1)
> 
> Why did this move?
> 
>> -
>> -static const struct regmap_config th1520_clk_regmap_config = {
>> -       .reg_bits = 32,
>> -       .val_bits = 32,
>> -       .reg_stride = 4,
>> -       .fast_io = true,
>> -};
>> -
>>  static int th1520_clk_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>> diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
>> new file mode 100644
>> index 000000000000..e2bfe56de9af
>> --- /dev/null
>> +++ b/drivers/clk/thead/clk-th1520.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + *  Authors: Yangtao Li <frank.li@vivo.com>
>> + */
>> +
>> +#include "clk-th1520.h"
> 
> Explicitly include linux headers here, don't just put them all into a
> header file. It helps us easily see what C files are using different
> parts of the kernel infrastructure.
> 
>> +
>> +static u8 ccu_get_parent_helper(struct ccu_common *common,
>> +                               struct ccu_internal *mux)
>> +{
>> +       unsigned int val;
>> +       u8 parent;
>> +
>> +       regmap_read(common->map, common->cfg0, &val);
>> +       parent = val >> mux->shift;
>> +       parent &= GENMASK(mux->width - 1, 0);
>> +
>> +       return parent;
>> +}
>> +
>> +static int ccu_set_parent_helper(struct ccu_common *common,
>> +                                struct ccu_internal *mux, u8 index)
>> +{
>> +       return regmap_update_bits(common->map, common->cfg0,
>> +                                 GENMASK(mux->width - 1, 0) << mux->shift,
>> +                                 index << mux->shift);
>> +}
>> +
>> +static void ccu_disable_helper(struct ccu_common *common, u32 gate)
>> +{
>> +       if (!gate)
>> +               return;
>> +       regmap_update_bits(common->map, common->cfg0, gate, ~gate);
>> +}
>> +
>> +static int ccu_enable_helper(struct ccu_common *common, u32 gate)
>> +{
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       if (!gate)
>> +               return 0;
>> +
>> +       ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
>> +       regmap_read(common->map, common->cfg0, &val);
>> +       return ret;
>> +}
>> +
>> +static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
>> +{
>> +       unsigned int val;
>> +
>> +       if (!gate)
>> +               return true;
>> +
>> +       regmap_read(common->map, common->cfg0, &val);
>> +       return val & gate;
>> +}
>> +
>> +static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> +                                        unsigned long parent_rate)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +       unsigned long rate;
>> +       unsigned int val;
>> +
>> +       regmap_read(cd->common.map, cd->common.cfg0, &val);
>> +       val = val >> cd->div.shift;
>> +       val &= GENMASK(cd->div.width - 1, 0);
>> +       rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
>> +                                  cd->div.width);
>> +
>> +       return rate;
>> +}
>> +
>> +static u8 ccu_div_get_parent(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_get_parent_helper(&cd->common, &cd->mux);
>> +}
>> +
>> +static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_set_parent_helper(&cd->common, &cd->mux, index);
>> +}
>> +
>> +static void ccu_div_disable(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       ccu_disable_helper(&cd->common, cd->enable);
>> +}
>> +
>> +static int ccu_div_enable(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_enable_helper(&cd->common, cd->enable);
>> +}
>> +
>> +static int ccu_div_is_enabled(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_is_enabled_helper(&cd->common, cd->enable);
>> +}
>> +
>> +const struct clk_ops ccu_div_ops = {
>> +       .disable = ccu_div_disable,
>> +       .enable = ccu_div_enable,
>> +       .is_enabled = ccu_div_is_enabled,
>> +       .get_parent = ccu_div_get_parent,
>> +       .set_parent = ccu_div_set_parent,
>> +       .recalc_rate = ccu_div_recalc_rate,
>> +       .determine_rate = clk_hw_determine_rate_no_reparent,
>> +};
>> +
>> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
>> +                                               unsigned long parent_rate)
>> +{
>> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +       unsigned long div, mul, frac;
>> +       unsigned int cfg0, cfg1;
>> +       u64 rate = parent_rate;
>> +
>> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> +
>> +       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
>> +       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
>> +       if (!(cfg1 & TH1520_PLL_DSMPD)) {
>> +               mul <<= TH1520_PLL_FRAC_BITS;
>> +               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
>> +               mul += frac;
>> +               div <<= TH1520_PLL_FRAC_BITS;
>> +       }
>> +       rate = parent_rate * mul;
>> +       rate = rate / div;
>> +       return rate;
>> +}
>> +
>> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
>> +                                                   unsigned long parent_rate)
>> +{
>> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +       unsigned long div, rate = parent_rate;
>> +       unsigned int cfg0, cfg1;
>> +
>> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> +
>> +       if (cfg1 & TH1520_PLL_BYPASS)
>> +               return rate;
>> +
>> +       div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
>> +             FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
>> +
>> +       rate = rate / div;
>> +
>> +       return rate;
>> +}
>> +
>> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> +                                        unsigned long parent_rate)
>> +{
>> +       unsigned long rate = parent_rate;
>> +
>> +       rate = th1520_pll_vco_recalc_rate(hw, rate);
>> +       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
>> +
>> +       return rate;
>> +}
>> +
>> +const struct clk_ops clk_pll_ops = {
>> +       .recalc_rate = ccu_pll_recalc_rate,
>> +};
>> +
>> +const struct regmap_config th1520_clk_regmap_config = {
> 
> I don't get why this is moved.
> 
>> +       .reg_bits = 32,
>> +       .val_bits = 32,
>> +       .reg_stride = 4,
>> +       .fast_io = true,
>> +};
>> diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
>> new file mode 100644
>> index 000000000000..285d41e65008
>> --- /dev/null
>> +++ b/drivers/clk/thead/clk-th1520.h
>> @@ -0,0 +1,134 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + *  Authors: Yangtao Li <frank.li@vivo.com>
>> + *
>> + * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
>> + */
>> +
>> +#ifndef CLK_TH1520_H
>> +#define CLK_TH1520_H
>> +
>> +#include <dt-bindings/clock/thead,th1520-clk-ap.h>
> 
> dt-bindings comes after linux includes, but this shouldn't be here
> anyway.
> 
>> +#include <linux/bitfield.h>
>> +#include <linux/clk-provider.h>
> 
> Seems we have to have this one for clk_hw.
> 
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
> 
> I don't see these includes used here, so remove them.
> 
>> +#include <linux/regmap.h>
> 
> Forward declare regmap and drop the include
> 
> struct regmap;
> 
>> +
>> +#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
>> +#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
>> +#define TH1520_PLL_FBDIV       GENMASK(19, 8)
>> +#define TH1520_PLL_REFDIV      GENMASK(5, 0)
>> +#define TH1520_PLL_BYPASS      BIT(30)
>> +#define TH1520_PLL_DSMPD       BIT(24)
>> +#define TH1520_PLL_FRAC                GENMASK(23, 0)
>> +#define TH1520_PLL_FRAC_BITS    24
> 
> Are these going to be used in multiple drivers?
> 
>> +
>> +struct ccu_internal {
>> +       u8      shift;
>> +       u8      width;
>> +};
>> +
>> +struct ccu_div_internal {
>> +       u8      shift;
>> +       u8      width;
>> +       u32     flags;
>> +};
>> +
>> +struct ccu_common {
>> +       int             clkid;
>> +       struct regmap   *map;
>> +       u16             cfg0;
>> +       u16             cfg1;
>> +       struct clk_hw   hw;
>> +};
>> +
>> +struct ccu_mux {
>> +       struct ccu_internal     mux;
>> +       struct ccu_common       common;
>> +};
>> +
>> +struct ccu_gate {
>> +       u32                     enable;
>> +       struct ccu_common       common;
>> +};
>> +
>> +struct ccu_div {
>> +       u32                     enable;
>> +       struct ccu_div_internal div;
>> +       struct ccu_internal     mux;
>> +       struct ccu_common       common;
>> +};
>> +
>> +struct ccu_pll {
>> +       struct ccu_common       common;
>> +};
>> +
>> +#define TH_CCU_ARG(_shift, _width)                                     \
>> +       {                                                               \
>> +               .shift  = _shift,                                       \
>> +               .width  = _width,                                       \
>> +       }
>> +
>> +#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)                       \
>> +       {                                                               \
>> +               .shift  = _shift,                                       \
>> +               .width  = _width,                                       \
>> +               .flags  = _flags,                                       \
>> +       }
>> +
>> +#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
>> +       struct ccu_gate _struct = {                                     \
>> +               .enable = _gate,                                        \
>> +               .common = {                                             \
>> +                       .clkid          = _clkid,                       \
>> +                       .cfg0           = _reg,                         \
>> +                       .hw.init        = CLK_HW_INIT_PARENTS_DATA(     \
>> +                                               _name,                  \
>> +                                               _parent,                \
>> +                                               &clk_gate_ops,          \
>> +                                               _flags),                \
>> +               }                                                       \
>> +       }
>> +
>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> +{
>> +       return container_of(hw, struct ccu_common, hw);
>> +}
>> +
>> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_mux, common);
>> +}
>> +
>> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_pll, common);
>> +}
>> +
>> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_div, common);
>> +}
>> +
>> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_gate, common);
>> +}
>> +
>> +extern const struct clk_ops ccu_div_ops;
>> +extern const struct clk_ops clk_pll_ops;
>> +extern const struct regmap_config th1520_clk_regmap_config;
> 
> Why is the regmap config exported?

The regmap_config is exported to allow reuse across multiple drivers.
Initially, I passed the clock VOSYS address space using the reg property
and created the regmap from it, enabling other drivers to utilize the
same configuration. Later, I switched to a regmap-based syscon approach
but haven’t moved the regmap_config back to the AP driver.

Based on Krzysztof's feedback, using the reg property to pass
memory-mapped registers is preferred. If needed, I can create a separate
regmap_config for the VO subsystem instead of reusing the existing one.

I will also address the other points you mentioned in your review.

Thank you for your feedback.
Michał

> 

Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code

[Krzysztof Kozlowski]

On 04/12/2024 14:54, Michal Wilczynski wrote:
> 
> 
> On 12/3/24 20:56, Stephen Boyd wrote:
>> Quoting Michal Wilczynski (2024-12-03 05:41:24)
>>> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
>>> index 7ee0bec1f251..d7cf88390b69 100644
>>> --- a/drivers/clk/thead/Makefile
>>> +++ b/drivers/clk/thead/Makefile
>>> @@ -1,2 +1,2 @@
>>>  # SPDX-License-Identifier: GPL-2.0
>>> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
>>> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
>>
>> Can the -ap driver be extended instead? Or are the clks in a different
>> IO region?
> 
> The Video Output (VO) clocks reside in a different address space as
> defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate
> driver made sense to maintain clarity and adhere to the existing

There is no such rule, no convention even. But there is a rule and
convention of re-using drivers.

> convention of having one driver per subsystem, similar to the

You have here two drivers per subsystem, so even if there was such a
rule, you just broke it.

> AP-specific driver.
> 
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
>>
>>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>>> index 17e32ae08720..a6015805b859 100644
>>> --- a/drivers/clk/thead/clk-th1520-ap.c
>>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>>> @@ -5,297 +5,9 @@
>>>   *  Authors: Yangtao Li <frank.li@vivo.com>
>>>   */
>>>  
>>> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
>>
>> Presumably this should stay here.
>>
>>> -#include <linux/bitfield.h>
>>> -#include <linux/clk-provider.h>
>>> -#include <linux/device.h>
>>> -#include <linux/module.h>
>>> -#include <linux/platform_device.h>
>>> -#include <linux/regmap.h>
>>

...

>>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>>> +{
>>> +       return container_of(hw, struct ccu_common, hw);
>>> +}
>>> +
>>> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_mux, common);
>>> +}
>>> +
>>> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_pll, common);
>>> +}
>>> +
>>> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_div, common);
>>> +}
>>> +
>>> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_gate, common);
>>> +}
>>> +
>>> +extern const struct clk_ops ccu_div_ops;
>>> +extern const struct clk_ops clk_pll_ops;
>>> +extern const struct regmap_config th1520_clk_regmap_config;
>>
>> Why is the regmap config exported?
> 
> The regmap_config is exported to allow reuse across multiple drivers.
> Initially, I passed the clock VOSYS address space using the reg property
> and created the regmap from it, enabling other drivers to utilize the
> same configuration. Later, I switched to a regmap-based syscon approach
> but haven’t moved the regmap_config back to the AP driver.


It anyway in your original code cannot be exported. If you want to use
syscon, then use syscon, not exporting regmaps manually.




Best regards,
Krzysztof

[RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file

[Michal Wilczynski]

As support for clocks from new subsystems is being added to the T-Head
TH1520 SoC, the header file name should reflect this broader scope. The
existing header file 'thead,th1520-clk-ap.h' includes the '-ap' suffix,
indicating it's specific to the Application Processor (AP) subsystem.

Rename the header file to 'thead,th1520-clk.h' to generalize it for all
subsystems.  Update all references to this header file accordingly.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../devicetree/bindings/clock/thead,th1520-clk-ap.yaml        | 4 ++--
 .../devicetree/bindings/mailbox/thead,th1520-mbox.yaml        | 2 +-
 MAINTAINERS                                                   | 2 +-
 arch/riscv/boot/dts/thead/th1520.dtsi                         | 2 +-
 drivers/clk/thead/clk-th1520.h                                | 2 +-
 .../clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h}       | 0
 6 files changed, 6 insertions(+), 6 deletions(-)
 rename include/dt-bindings/clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} (100%)

diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
index 0129bd0ba4b3..4a0806af2bf9 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
@@ -32,7 +32,7 @@ properties:
   "#clock-cells":
     const: 1
     description:
-      See <dt-bindings/clock/thead,th1520-clk-ap.h> for valid indices.
+      See <dt-bindings/clock/thead,th1520-clk.h> for valid indices.
 
 required:
   - compatible
@@ -44,7 +44,7 @@ additionalProperties: false
 
 examples:
   - |
-    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+    #include <dt-bindings/clock/thead,th1520-clk.h>
     clock-controller@ef010000 {
         compatible = "thead,th1520-clk-ap";
         reg = <0xef010000 0x1000>;
diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
index 0971fb97896e..0b58b8d0d351 100644
--- a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
@@ -68,7 +68,7 @@ additionalProperties: false
 
 examples:
   - |
-    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+    #include <dt-bindings/clock/thead,th1520-clk.h>
     soc {
       #address-cells = <2>;
       #size-cells = <2>;
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c85abf1dd1e..bd4bbf07d588 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20192,7 +20192,7 @@ F:	drivers/clk/thead/
 F:	drivers/mailbox/mailbox-th1520.c
 F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
 F:	drivers/pinctrl/pinctrl-th1520.c
-F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
+F:	include/dt-bindings/clock/thead,th1520-clk.h
 
 RNBD BLOCK DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index acfe030e803a..dc2d554b4a71 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -5,7 +5,7 @@
  */
 
 #include <dt-bindings/interrupt-controller/irq.h>
-#include <dt-bindings/clock/thead,th1520-clk-ap.h>
+#include <dt-bindings/clock/thead,th1520-clk.h>
 
 / {
 	compatible = "thead,th1520";
diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
index 285d41e65008..5d30f55e88a1 100644
--- a/drivers/clk/thead/clk-th1520.h
+++ b/drivers/clk/thead/clk-th1520.h
@@ -10,7 +10,7 @@
 #ifndef CLK_TH1520_H
 #define CLK_TH1520_H
 
-#include <dt-bindings/clock/thead,th1520-clk-ap.h>
+#include <dt-bindings/clock/thead,th1520-clk.h>
 #include <linux/bitfield.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
diff --git a/include/dt-bindings/clock/thead,th1520-clk-ap.h b/include/dt-bindings/clock/thead,th1520-clk.h
similarity index 100%
rename from include/dt-bindings/clock/thead,th1520-clk-ap.h
rename to include/dt-bindings/clock/thead,th1520-clk.h
-- 
2.34.1

Re: [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file

[Rob Herring (Arm)]

On Tue, 03 Dec 2024 14:41:25 +0100, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the header file name should reflect this broader scope. The
> existing header file 'thead,th1520-clk-ap.h' includes the '-ap' suffix,
> indicating it's specific to the Application Processor (AP) subsystem.
> 
> Rename the header file to 'thead,th1520-clk.h' to generalize it for all
> subsystems.  Update all references to this header file accordingly.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../devicetree/bindings/clock/thead,th1520-clk-ap.yaml        | 4 ++--
>  .../devicetree/bindings/mailbox/thead,th1520-mbox.yaml        | 2 +-
>  MAINTAINERS                                                   | 2 +-
>  arch/riscv/boot/dts/thead/th1520.dtsi                         | 2 +-
>  drivers/clk/thead/clk-th1520.h                                | 2 +-
>  .../clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h}       | 0
>  6 files changed, 6 insertions(+), 6 deletions(-)
>  rename include/dt-bindings/clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} (100%)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.example.dts:24:18: fatal error: dt-bindings/clock/thead,th1520-clk.h: No such file or directory
   24 |         #include <dt-bindings/clock/thead,th1520-clk.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.example.dts:18:18: fatal error: dt-bindings/clock/thead,th1520-clk.h: No such file or directory
   18 |         #include <dt-bindings/clock/thead,th1520-clk.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.example.dtb] Error 1
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-3-m.wilczynski@samsung.com

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

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

pip3 install dtschema --upgrade

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

Re: [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the header file name should reflect this broader scope. The

No, there is no such rule, so "should" is not appropriate.


> existing header file 'thead,th1520-clk-ap.h' includes the '-ap' suffix,
> indicating it's specific to the Application Processor (AP) subsystem.
> 
> Rename the header file to 'thead,th1520-clk.h' to generalize it for all
> subsystems.  Update all references to this header file accordingly.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../devicetree/bindings/clock/thead,th1520-clk-ap.yaml        | 4 ++--
>  .../devicetree/bindings/mailbox/thead,th1520-mbox.yaml        | 2 +-
>  MAINTAINERS                                                   | 2 +-
>  arch/riscv/boot/dts/thead/th1520.dtsi                         | 2 +-
>  drivers/clk/thead/clk-th1520.h                                | 2 +-
>  .../clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h}       | 0

Don't mix driver, bindings and DTS. See checkpatch warning. Don't rename
files just because you prefer other names.

Best regards,
Krzysztof

[RFC PATCH v1 03/14] clk: thead: Enable clock gates with regmaps

[Michal Wilczynski]

The current implementation of the CCU_GATE macro assumes direct access
to memory-mapped registers, which isn't suitable when using regmaps for
register access. In the TH1520 SoC, the address space for the VO (Video
Output) subsystem clocks is shared with other control registers, such as
those used for resetting the GPU. To prevent conflicts and ensure
synchronized access, it's important to access these registers via a
regmap.

This patch updates the CCU_GATE macro to support regmap-based access by
reusing the clk_ops from the divider clocks (ccu_div_ops). This change
allows the clock gates to be controlled through regmap, enabling proper
synchronization when multiple components interact with the shared
address space.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/clk/thead/clk-th1520.c | 10 ++++++++--
 drivers/clk/thead/clk-th1520.h | 15 +++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
index e2bfe56de9af..3ada8b98bd8e 100644
--- a/drivers/clk/thead/clk-th1520.c
+++ b/drivers/clk/thead/clk-th1520.c
@@ -120,8 +120,14 @@ const struct clk_ops ccu_div_ops = {
 	.determine_rate = clk_hw_determine_rate_no_reparent,
 };
 
-static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
-						unsigned long parent_rate)
+const struct clk_ops ccu_gate_ops = {
+	.disable = ccu_div_disable,
+	.enable = ccu_div_enable,
+	.is_enabled = ccu_div_is_enabled,
+};
+
+unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
 {
 	struct ccu_pll *pll = hw_to_ccu_pll(hw);
 	unsigned long div, mul, frac;
diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
index 5d30f55e88a1..532afbbfea01 100644
--- a/drivers/clk/thead/clk-th1520.h
+++ b/drivers/clk/thead/clk-th1520.h
@@ -94,6 +94,20 @@ struct ccu_pll {
 		}							\
 	}
 
+#define CCU_GATE_REGMAP(_clkid, _struct, _name, _parent, _reg, _gate, _flags)	\
+	struct ccu_gate _struct = {						\
+		.enable	= _gate,						\
+		.common	= {							\
+			.clkid		= _clkid,				\
+			.cfg0		= _reg,					\
+			.hw.init	= CLK_HW_INIT_PARENTS_DATA(		\
+						_name,				\
+						_parent,			\
+						&ccu_gate_ops,			\
+						_flags),			\
+		}								\
+	}
+
 static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
 {
 	return container_of(hw, struct ccu_common, hw);
@@ -130,5 +144,6 @@ static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
 extern const struct clk_ops ccu_div_ops;
 extern const struct clk_ops clk_pll_ops;
 extern const struct regmap_config th1520_clk_regmap_config;
+extern const struct clk_ops ccu_gate_ops;
 
 #endif /* CLK_TH1520_H */
-- 
2.34.1

[RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem

[Michal Wilczynski]

The Video Output (VO) module on the T-Head TH1520 SoC has its own set of
clocks that need proper management. This commit introduces the
clk-th1520-vo driver to support the VO subsystem clocks.

Currently, only the clock gates are implemented, as they are the primary
relevant clocks for the VO subsystem at this stage.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/clk/thead/Kconfig                    |  11 ++
 drivers/clk/thead/Makefile                   |   1 +
 drivers/clk/thead/clk-th1520-vo.c            | 168 +++++++++++++++++++
 include/dt-bindings/clock/thead,th1520-clk.h |  34 ++++
 4 files changed, 214 insertions(+)
 create mode 100644 drivers/clk/thead/clk-th1520-vo.c

diff --git a/drivers/clk/thead/Kconfig b/drivers/clk/thead/Kconfig
index 95e0d9eb965e..937927a1a4b8 100644
--- a/drivers/clk/thead/Kconfig
+++ b/drivers/clk/thead/Kconfig
@@ -11,3 +11,14 @@ config CLK_THEAD_TH1520_AP
 	  on the T-HEAD TH1520 SoC. This includes configuration of
 	  both CPU PLLs, both DPU PLLs as well as the GMAC, VIDEO,
 	  and TEE PLLs.
+
+config CLK_THEAD_TH1520_VO
+	bool "T-HEAD TH1520 VO clock support"
+	depends on ARCH_THEAD || COMPILE_TEST
+	depends on 64BIT
+	default ARCH_THEAD
+	select REGMAP_MMIO
+	help
+	  Say yes here to support the VO sub system clock controller
+	  on the T-HEAD TH1520 SoC. This includes clock gates for the
+	  Video Output components like HDMI, MIPI, DPU and GPU.
diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
index d7cf88390b69..9afaee27b0b9 100644
--- a/drivers/clk/thead/Makefile
+++ b/drivers/clk/thead/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
+obj-$(CONFIG_CLK_THEAD_TH1520_VO) += clk-th1520.o clk-th1520-vo.o
diff --git a/drivers/clk/thead/clk-th1520-vo.c b/drivers/clk/thead/clk-th1520-vo.c
new file mode 100644
index 000000000000..3c6d246ab53a
--- /dev/null
+++ b/drivers/clk/thead/clk-th1520-vo.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#include <linux/mfd/syscon.h>
+#include "clk-th1520.h"
+
+#define NR_CLKS (CLK_HDMI_PIXCLK + 1)
+
+static const struct clk_parent_data video_pll_pd[] = {
+	/* TODO: provide a proper parent here */
+	NULL,
+};
+
+static CCU_GATE_REGMAP(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
+		       video_pll_pd, 0x50, BIT(0), 0);
+static CCU_GATE_REGMAP(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_pd,
+		       0x50, BIT(3), 0);
+static CCU_GATE_REGMAP(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
+		       video_pll_pd, 0x50, BIT(4), 0);
+static CCU_GATE_REGMAP(CLK_DPU_PIXELCLK0, dpu0_pixelclk, "dpu0-pixelclk",
+		       video_pll_pd, 0x50, BIT(5), 0);
+static CCU_GATE_REGMAP(CLK_DPU_PIXELCLK1, dpu1_pixelclk, "dpu1-pixelclk",
+		       video_pll_pd, 0x50, BIT(6), 0);
+static CCU_GATE_REGMAP(CLK_DPU_HCLK, dpu_hclk, "dpu-hclk", video_pll_pd, 0x50,
+		       BIT(7), 0);
+static CCU_GATE_REGMAP(CLK_DPU_ACLK, dpu_aclk, "dpu-aclk", video_pll_pd, 0x50,
+		       BIT(8), 0);
+static CCU_GATE_REGMAP(CLK_DPU_CCLK, dpu_cclk, "dpu-cclk", video_pll_pd, 0x50,
+		       BIT(9), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_SFR, hdmi_sfr_clk, "hdmi-sfr-clk", video_pll_pd,
+		       0x50, BIT(10), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_PCLK, hdmi_pclk, "hdmi-pclk", video_pll_pd,
+		       0x50, BIT(11), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_CEC, hdmi_cec_clk, "hdmi-cec-clk", video_pll_pd,
+		       0x50, BIT(12), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI0_PCLK, mipi_dsi0_pclk, "mipi-dsi0-pclk",
+		       video_pll_pd, 0x50, BIT(13), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI1_PCLK, mipi_dsi1_pclk, "mipi-dsi1-pclk",
+		       video_pll_pd, 0x50, BIT(14), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI0_CFG, mipi_dsi0_cfg_clk,
+		       "mipi-dsi0-cfg-clk", video_pll_pd, 0x50, BIT(15), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI1_CFG, mipi_dsi1_cfg_clk,
+		       "mipi-dsi1-cfg-clk", video_pll_pd, 0x50, BIT(16), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI0_REFCLK, mipi_dsi0_refclk,
+		       "mipi-dsi0-refclk", video_pll_pd, 0x50, BIT(17), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI1_REFCLK, mipi_dsi1_refclk,
+		       "mipi-dsi1-refclk", video_pll_pd, 0x50, BIT(18), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_I2S, hdmi_i2c_clk, "hdmi-i2c-clk", video_pll_pd,
+		       0x50, BIT(19), 0);
+static CCU_GATE_REGMAP(CLK_X2H_DPU1_ACLK, x2h_dpu1_aclk, "x2h-dpu1-aclk",
+		       video_pll_pd, 0x50, BIT(20), 0);
+static CCU_GATE_REGMAP(CLK_X2H_DPU_ACLK, x2h_dpu_aclk, "x2h-dpu-aclk",
+		       video_pll_pd, 0x50, BIT(21), 0);
+static CCU_GATE_REGMAP(CLK_AXI4_VO_PCLK, axi4_vo_pclk, "axi4-vo-pclk",
+		       video_pll_pd, 0x50, BIT(22), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_VOSYS_DPU_PCLK, iopmp_vosys_dpu_pclk,
+		       "iopmp-vosys-dpu-pclk", video_pll_pd, 0x50, BIT(23), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_VOSYS_DPU1_PCLK, iopmp_vosys_dpu1_pclk,
+		       "iopmp-vosys-dpu1-pclk", video_pll_pd, 0x50, BIT(24), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_VOSYS_GPU_PCLK, iopmp_vosys_gpu_pclk,
+		       "iopmp-vosys-gpu-pclk", video_pll_pd, 0x50, BIT(25), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_DPU1_ACLK, iopmp_dpu1_aclk, "iopmp-dpu1-aclk",
+		       video_pll_pd, 0x50, BIT(27), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_DPU_ACLK, iopmp_dpu_aclk, "iopmp-dpu-aclk",
+		       video_pll_pd, 0x50, BIT(28), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_GPU_ACLK, iopmp_gpu_aclk, "iopmp-gpu-aclk",
+		       video_pll_pd, 0x50, BIT(29), 0);
+static CCU_GATE_REGMAP(CLK_MIPIDSI0_PIXCLK, mipi_dsi0_pixclk,
+		       "mipi-dsi0-pixclk", video_pll_pd, 0x50, BIT(30), 0);
+static CCU_GATE_REGMAP(CLK_MIPIDSI1_PIXCLK, mipi_dsi1_pixclk,
+		       "mipi-dsi1-pixclk", video_pll_pd, 0x50, BIT(31), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_PIXCLK, hdmi_pixclk, "hdmi-pixclk",
+		       video_pll_pd, 0x54, BIT(0), 0);
+
+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_i2c_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 int th1520_clk_vo_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct clk_hw_onecell_data *priv;
+	struct regmap *map;
+	struct clk_hw *hw;
+	int ret, i;
+
+	priv = devm_kzalloc(dev, struct_size(priv, hws, NR_CLKS), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->num = NR_CLKS;
+
+	map = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	for (i = 0; i < ARRAY_SIZE(th1520_vo_gate_clks); i++) {
+		struct ccu_gate *cg = hw_to_ccu_gate(&th1520_vo_gate_clks[i]->hw);
+
+		th1520_vo_gate_clks[i]->map = map;
+
+		ret = devm_clk_hw_register(dev, &th1520_vo_gate_clks[i]->hw);
+		if (ret)
+			return ret;
+
+		priv->hws[cg->common.clkid] = hw;
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id th1520_clk_vo_match[] = {
+	{
+		.compatible = "thead,th1520-clk-vo",
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, th1520_clk_vo_match);
+
+static struct platform_driver th1520_clk_vo_driver = {
+	.probe		= th1520_clk_vo_probe,
+	.driver		= {
+		.name	= "th1520-clk-vo",
+		.of_match_table = th1520_clk_vo_match,
+	},
+};
+module_platform_driver(th1520_clk_vo_driver);
+
+MODULE_DESCRIPTION("T-HEAD TH1520 VO Clock driver");
+MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/dt-bindings/clock/thead,th1520-clk.h b/include/dt-bindings/clock/thead,th1520-clk.h
index a199784b3512..86a7cf2c9acf 100644
--- a/include/dt-bindings/clock/thead,th1520-clk.h
+++ b/include/dt-bindings/clock/thead,th1520-clk.h
@@ -7,6 +7,7 @@
 #ifndef _DT_BINDINGS_CLK_TH1520_H_
 #define _DT_BINDINGS_CLK_TH1520_H_
 
+/* AP clocks */
 #define CLK_CPU_PLL0		0
 #define CLK_CPU_PLL1		1
 #define CLK_GMAC_PLL		2
@@ -93,4 +94,37 @@
 #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_CORE			1
+#define CLK_GPU_CFG_ACLK		2
+#define CLK_DPU_PIXELCLK0		3
+#define CLK_DPU_PIXELCLK1		4
+#define CLK_DPU_HCLK			5
+#define CLK_DPU_ACLK			6
+#define CLK_DPU_CCLK			7
+#define CLK_HDMI_SFR			8
+#define CLK_HDMI_PCLK			9
+#define CLK_HDMI_CEC			10
+#define CLK_MIPI_DSI0_PCLK		11
+#define CLK_MIPI_DSI1_PCLK		12
+#define CLK_MIPI_DSI0_CFG		13
+#define CLK_MIPI_DSI1_CFG		14
+#define CLK_MIPI_DSI0_REFCLK		15
+#define CLK_MIPI_DSI1_REFCLK		16
+#define CLK_HDMI_I2S			17
+#define CLK_X2H_DPU1_ACLK		18
+#define CLK_X2H_DPU_ACLK		19
+#define CLK_AXI4_VO_PCLK		20
+#define CLK_IOPMP_VOSYS_DPU_PCLK	21
+#define CLK_IOPMP_VOSYS_DPU1_PCLK	22
+#define CLK_IOPMP_VOSYS_GPU_PCLK	23
+#define CLK_IOPMP_DPU1_ACLK		24
+#define CLK_IOPMP_DPU_ACLK		25
+#define CLK_IOPMP_GPU_ACLK		26
+#define CLK_MIPIDSI0_PIXCLK		27
+#define CLK_MIPIDSI1_PIXCLK		28
+#define CLK_HDMI_PIXCLK			29
+
 #endif
-- 
2.34.1

Re: [RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The Video Output (VO) module on the T-Head TH1520 SoC has its own set of
> clocks that need proper management. This commit introduces the
> clk-th1520-vo driver to support the VO subsystem clocks.
> 
> Currently, only the clock gates are implemented, as they are the primary
> relevant clocks for the VO subsystem at this stage.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/clk/thead/Kconfig                    |  11 ++
>  drivers/clk/thead/Makefile                   |   1 +
>  drivers/clk/thead/clk-th1520-vo.c            | 168 +++++++++++++++++++
>  include/dt-bindings/clock/thead,th1520-clk.h |  34 ++++
Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.

Best regards,
Krzysztof

[RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Michal Wilczynski]

The device tree bindings for the T-Head TH1520 SoC clocks currently
support only the Application Processor (AP) subsystem. This commit
extends the bindings to include the Video Output (VO) subsystem clocks.

Update the YAML schema to define the VO subsystem clocks, allowing the
clock driver to configure and manage these clocks appropriately. This
addition is necessary to enable the proper operation of the video output
features on the TH1520 SoC.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
index 4a0806af2bf9..5a8f1041f766 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
@@ -4,11 +4,13 @@
 $id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: T-HEAD TH1520 AP sub-system clock controller
+title: T-HEAD TH1520 sub-systems clock controller
 
 description: |
-  The T-HEAD TH1520 AP sub-system clock controller configures the
-  CPU, DPU, GMAC and TEE PLLs.
+  The T-HEAD TH1520 sub-systems clock controller configures the
+  CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
+  subsystem clock gates can be configured 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,7 +22,9 @@ maintainers:
 
 properties:
   compatible:
-    const: thead,th1520-clk-ap
+    enum:
+      - thead,th1520-clk-ap
+      - thead,th1520-clk-vo
 
   reg:
     maxItems: 1
@@ -29,6 +33,17 @@ properties:
     items:
       - description: main oscillator (24MHz)
 
+  thead,vosys-regmap:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      Phandle to a syscon node representing the shared register
+      space of the VO (Video Output) subsystem. This register space
+      includes both clock control registers and other control
+      registers used for operations like resetting the GPU. Since
+      these registers reside in the same address space, access to
+      them is coordinated through a shared syscon regmap provided by
+      the specified syscon node.
+
   "#clock-cells":
     const: 1
     description:
@@ -36,8 +51,6 @@ properties:
 
 required:
   - compatible
-  - reg
-  - clocks
   - "#clock-cells"
 
 additionalProperties: false
@@ -51,3 +64,9 @@ examples:
         clocks = <&osc>;
         #clock-cells = <1>;
     };
+
+    clock-controller-vo {
+        compatible = "thead,th1520-clk-vo";
+        thead,vosys-regmap = <&vosys_regmap>;
+        #clock-cells = <1>;
+    };
-- 
2.34.1

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Rob Herring (Arm)]

On Tue, 03 Dec 2024 14:41:28 +0100, Michal Wilczynski wrote:
> The device tree bindings for the T-Head TH1520 SoC clocks currently
> support only the Application Processor (AP) subsystem. This commit
> extends the bindings to include the Video Output (VO) subsystem clocks.
> 
> Update the YAML schema to define the VO subsystem clocks, allowing the
> clock driver to configure and manage these clocks appropriately. This
> addition is necessary to enable the proper operation of the video output
> features on the TH1520 SoC.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-6-m.wilczynski@samsung.com

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

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

pip3 install dtschema --upgrade

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

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The device tree bindings for the T-Head TH1520 SoC clocks currently
> support only the Application Processor (AP) subsystem. This commit
> extends the bindings to include the Video Output (VO) subsystem clocks.
> 
> Update the YAML schema to define the VO subsystem clocks, allowing the
> clock driver to configure and manage these clocks appropriately. This
> addition is necessary to enable the proper operation of the video output
> features on the TH1520 SoC.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> index 4a0806af2bf9..5a8f1041f766 100644
> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> @@ -4,11 +4,13 @@
>  $id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: T-HEAD TH1520 AP sub-system clock controller
> +title: T-HEAD TH1520 sub-systems clock controller
>  
>  description: |
> -  The T-HEAD TH1520 AP sub-system clock controller configures the
> -  CPU, DPU, GMAC and TEE PLLs.
> +  The T-HEAD TH1520 sub-systems clock controller configures the
> +  CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
> +  subsystem clock gates can be configured 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,7 +22,9 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: thead,th1520-clk-ap
> +    enum:
> +      - thead,th1520-clk-ap
> +      - thead,th1520-clk-vo
>  
>    reg:
>      maxItems: 1
> @@ -29,6 +33,17 @@ properties:
>      items:
>        - description: main oscillator (24MHz)
>  
> +  thead,vosys-regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to a syscon node representing the shared register
> +      space of the VO (Video Output) subsystem. This register space
> +      includes both clock control registers and other control
> +      registers used for operations like resetting the GPU. Since


It seems you wanted to implement reset controller...

> +      these registers reside in the same address space, access to
> +      them is coordinated through a shared syscon regmap provided by
> +      the specified syscon node.

Drop last sentence. syscon regmap is a Linux term, not hardware one.

Anyway, this needs to be constrained per variant.

> +
>    "#clock-cells":
>      const: 1
>      description:
> @@ -36,8 +51,6 @@ properties:
>  
>  required:
>    - compatible
> -  - reg

No, that's a clear NAK. You claim you have no address space but in the
same time you have address space via regmap.

> -  - clocks

Nope, not explained, unless you wanted to make it different per variants.

>    - "#clock-cells"
>  
>  additionalProperties: false
> @@ -51,3 +64,9 @@ examples:
>          clocks = <&osc>;
>          #clock-cells = <1>;
>      };
> +
> +    clock-controller-vo {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        compatible = "thead,th1520-clk-vo";
> +        thead,vosys-regmap = <&vosys_regmap>;

That's a "reg" property. Do not encode address space as something else.


> +        #clock-cells = <1>;
> +    };


Best regards,
Krzysztof

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Michal Wilczynski]

On 12/3/24 16:45, Krzysztof Kozlowski wrote:
> On 03/12/2024 14:41, Michal Wilczynski wrote:
>> The device tree bindings for the T-Head TH1520 SoC clocks currently
>> support only the Application Processor (AP) subsystem. This commit
>> extends the bindings to include the Video Output (VO) subsystem clocks.
>>
>> Update the YAML schema to define the VO subsystem clocks, allowing the
>> clock driver to configure and manage these clocks appropriately. This
>> addition is necessary to enable the proper operation of the video output
>> features on the TH1520 SoC.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> index 4a0806af2bf9..5a8f1041f766 100644
>> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> @@ -4,11 +4,13 @@
>>  $id: https://protect2.fireeye.com/v1/url?k=f595e769-941ef222-f5946c26-74fe485fb305-6d0b73471bbfc1a2&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Fthead%2Cth1520-clk-ap.yaml%23
>>  $schema: https://protect2.fireeye.com/v1/url?k=5b94114b-3a1f0400-5b959a04-74fe485fb305-0e2c50f5c24cf3e9&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>  
>> -title: T-HEAD TH1520 AP sub-system clock controller
>> +title: T-HEAD TH1520 sub-systems clock controller
>>  
>>  description: |
>> -  The T-HEAD TH1520 AP sub-system clock controller configures the
>> -  CPU, DPU, GMAC and TEE PLLs.
>> +  The T-HEAD TH1520 sub-systems clock controller configures the
>> +  CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
>> +  subsystem clock gates can be configured for the HDMI, MIPI and
>> +  the GPU.
>>  
>>    SoC reference manual
>>    https://protect2.fireeye.com/v1/url?k=cceb6120-ad60746b-cceaea6f-74fe485fb305-b294b70f1b52a5ab&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf
>> @@ -20,7 +22,9 @@ maintainers:
>>  
>>  properties:
>>    compatible:
>> -    const: thead,th1520-clk-ap
>> +    enum:
>> +      - thead,th1520-clk-ap
>> +      - thead,th1520-clk-vo
>>  
>>    reg:
>>      maxItems: 1
>> @@ -29,6 +33,17 @@ properties:
>>      items:
>>        - description: main oscillator (24MHz)
>>  
>> +  thead,vosys-regmap:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: |
>> +      Phandle to a syscon node representing the shared register
>> +      space of the VO (Video Output) subsystem. This register space
>> +      includes both clock control registers and other control
>> +      registers used for operations like resetting the GPU. Since
> 
> 
> It seems you wanted to implement reset controller...

Thank you for your feedback.

I understand your concern about the reset controller. In the TH1520 SoC,
the GPU doesn't have its own reset controller. Instead, its reset is
managed through the power domain's registers as part of the power-up
sequence.

According to the Video Image Processing Manual 1.4.1 [1], the GPU
requires the following steps to power up:

1) Enable the GPU clock gate.
2) After 32 core clock cycles, release the GPU soft reset

Since these steps are closely tied to power management, I implemented
the reset functionality within the power-domain driver.

Because the GPU doesn't support the resets property, introducing a reset
controller wouldn't align with the existing device tree well.

[1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
> 
>> +      these registers reside in the same address space, access to
>> +      them is coordinated through a shared syscon regmap provided by
>> +      the specified syscon node.
> 
> Drop last sentence. syscon regmap is a Linux term, not hardware one.
> 
> Anyway, this needs to be constrained per variant.
> 
>> +
>>    "#clock-cells":
>>      const: 1
>>      description:
>> @@ -36,8 +51,6 @@ properties:
>>  
>>  required:
>>    - compatible
>> -  - reg
> 
> No, that's a clear NAK. You claim you have no address space but in the
> same time you have address space via regmap.

I see your concern. The VOSYS subsystem's address space includes
registers for various components, such as clock gates and reset
controls, which are scattered throughout the address space as specified
in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
regmap for access, but I realize this might not be the best approach.

To address this, I'll specify the 'reg' property in each node to define
the address ranges explicitly fragmenting the address space for the VOSYS
manually.

vosys_clk: clock-controller@ffef528050 {
	compatible = "thead,th1520-clk-vo";
	reg = <0xff 0xef528050 0x0 0x8>;
	#clock-cells = <1>;
};

pd: power-domain@ffef528000 {
	compatible = "thead,th1520-pd";
	reg = <0xff 0xef528000 0x0 0x8>;
	#power-domain-cells = <1>;
};

I would be happy to proceed like this if that's okay.

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

> 
>> -  - clocks
> 
> Nope, not explained, unless you wanted to make it different per variants.
> 
>>    - "#clock-cells"
>>  
>>  additionalProperties: false
>> @@ -51,3 +64,9 @@ examples:
>>          clocks = <&osc>;
>>          #clock-cells = <1>;
>>      };
>> +
>> +    clock-controller-vo {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://protect2.fireeye.com/v1/url?k=1e9cac96-7f17b9dd-1e9d27d9-74fe485fb305-f0538482114df941&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fdevicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.html%23generic-names-recommendation
> 
> 
>> +        compatible = "thead,th1520-clk-vo";
>> +        thead,vosys-regmap = <&vosys_regmap>;
> 
> That's a "reg" property. Do not encode address space as something else.
> 
> 
>> +        #clock-cells = <1>;
>> +    };
> 
> 
> Best regards,
> Krzysztof
> 

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Stephen Boyd]

Quoting Michal Wilczynski (2024-12-04 02:11:26)
> On 12/3/24 16:45, Krzysztof Kozlowski wrote:
> > On 03/12/2024 14:41, Michal Wilczynski wrote:
> 
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
> > 
> >> +      these registers reside in the same address space, access to
> >> +      them is coordinated through a shared syscon regmap provided by
> >> +      the specified syscon node.
> > 
> > Drop last sentence. syscon regmap is a Linux term, not hardware one.
> > 
> > Anyway, this needs to be constrained per variant.
> > 
> >> +
> >>    "#clock-cells":
> >>      const: 1
> >>      description:
> >> @@ -36,8 +51,6 @@ properties:
> >>  
> >>  required:
> >>    - compatible
> >> -  - reg
> > 
> > No, that's a clear NAK. You claim you have no address space but in the
> > same time you have address space via regmap.
> 
> I see your concern. The VOSYS subsystem's address space includes
> registers for various components, such as clock gates and reset
> controls, which are scattered throughout the address space as specified
> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
> regmap for access, but I realize this might not be the best approach.
> 
> To address this, I'll specify the 'reg' property in each node to define
> the address ranges explicitly fragmenting the address space for the VOSYS
> manually.
> 
> vosys_clk: clock-controller@ffef528050 {
>         compatible = "thead,th1520-clk-vo";
>         reg = <0xff 0xef528050 0x0 0x8>;
>         #clock-cells = <1>;
> };
> 
> pd: power-domain@ffef528000 {
>         compatible = "thead,th1520-pd";
>         reg = <0xff 0xef528000 0x0 0x8>;
>         #power-domain-cells = <1>;
> };

You should have one node:

    clock-controller@ffef528000 {
      compatible = "thead,th1520-vo";
      reg = <0xff 0xef528050 0x0 0x1a04>;
      #clock-cells = <1>;
      #power-domain-cells = <1>;
    };

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Stephen Boyd]

Quoting Stephen Boyd (2024-12-04 12:21:11)
> Quoting Michal Wilczynski (2024-12-04 02:11:26)
> > 
> > To address this, I'll specify the 'reg' property in each node to define
> > the address ranges explicitly fragmenting the address space for the VOSYS
> > manually.
> > 
> > vosys_clk: clock-controller@ffef528050 {
> >         compatible = "thead,th1520-clk-vo";
> >         reg = <0xff 0xef528050 0x0 0x8>;
> >         #clock-cells = <1>;
> > };
> > 
> > pd: power-domain@ffef528000 {
> >         compatible = "thead,th1520-pd";
> >         reg = <0xff 0xef528000 0x0 0x8>;
> >         #power-domain-cells = <1>;
> > };
> 
> You should have one node:
> 
>     clock-controller@ffef528000 {
>       compatible = "thead,th1520-vo";
>       reg = <0xff 0xef528050 0x0 0x1a04>;

Apologies for the typo. Here's the correct line:

	reg = <0xff 0xef528000 0x0 0x1a04>;

>       #clock-cells = <1>;
>       #power-domain-cells = <1>;
>     };
>

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Krzysztof Kozlowski]

On 04/12/2024 21:21, Stephen Boyd wrote:
>>> No, that's a clear NAK. You claim you have no address space but in the
>>> same time you have address space via regmap.
>>
>> I see your concern. The VOSYS subsystem's address space includes
>> registers for various components, such as clock gates and reset
>> controls, which are scattered throughout the address space as specified
>> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
>> regmap for access, but I realize this might not be the best approach.
>>
>> To address this, I'll specify the 'reg' property in each node to define
>> the address ranges explicitly fragmenting the address space for the VOSYS
>> manually.
>>
>> vosys_clk: clock-controller@ffef528050 {
>>         compatible = "thead,th1520-clk-vo";
>>         reg = <0xff 0xef528050 0x0 0x8>;
>>         #clock-cells = <1>;
>> };
>>
>> pd: power-domain@ffef528000 {
>>         compatible = "thead,th1520-pd";
>>         reg = <0xff 0xef528000 0x0 0x8>;
>>         #power-domain-cells = <1>;
>> };
> 
> You should have one node:
> 
>     clock-controller@ffef528000 {
>       compatible = "thead,th1520-vo";
>       reg = <0xff 0xef528050 0x0 0x1a04>;

Yes, obviously, but probably entire block:

<0xff 0xef528000 0x0 0x1000>;

>       #clock-cells = <1>;
>       #power-domain-cells = <1>;
>     };


Best regards,
Krzysztof

Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Krzysztof Kozlowski]

On 04/12/2024 11:11, Michal Wilczynski wrote:
> 
> 
> On 12/3/24 16:45, Krzysztof Kozlowski wrote:
>> On 03/12/2024 14:41, Michal Wilczynski wrote:
>>> The device tree bindings for the T-Head TH1520 SoC clocks currently
>>> support only the Application Processor (AP) subsystem. This commit
>>> extends the bindings to include the Video Output (VO) subsystem clocks.
>>>
>>> Update the YAML schema to define the VO subsystem clocks, allowing the
>>> clock driver to configure and manage these clocks appropriately. This
>>> addition is necessary to enable the proper operation of the video output
>>> features on the TH1520 SoC.
>>>
>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>> ---
>>>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
>>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>>> index 4a0806af2bf9..5a8f1041f766 100644
>>> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>>> @@ -4,11 +4,13 @@
>>>  $id: https://protect2.fireeye.com/v1/url?k=f595e769-941ef222-f5946c26-74fe485fb305-6d0b73471bbfc1a2&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Fthead%2Cth1520-clk-ap.yaml%23
>>>  $schema: https://protect2.fireeye.com/v1/url?k=5b94114b-3a1f0400-5b959a04-74fe485fb305-0e2c50f5c24cf3e9&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>>  
>>> -title: T-HEAD TH1520 AP sub-system clock controller
>>> +title: T-HEAD TH1520 sub-systems clock controller
>>>  
>>>  description: |
>>> -  The T-HEAD TH1520 AP sub-system clock controller configures the
>>> -  CPU, DPU, GMAC and TEE PLLs.
>>> +  The T-HEAD TH1520 sub-systems clock controller configures the
>>> +  CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
>>> +  subsystem clock gates can be configured for the HDMI, MIPI and
>>> +  the GPU.
>>>  
>>>    SoC reference manual
>>>    https://protect2.fireeye.com/v1/url?k=cceb6120-ad60746b-cceaea6f-74fe485fb305-b294b70f1b52a5ab&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf
>>> @@ -20,7 +22,9 @@ maintainers:
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: thead,th1520-clk-ap
>>> +    enum:
>>> +      - thead,th1520-clk-ap
>>> +      - thead,th1520-clk-vo
>>>  
>>>    reg:
>>>      maxItems: 1
>>> @@ -29,6 +33,17 @@ properties:
>>>      items:
>>>        - description: main oscillator (24MHz)
>>>  
>>> +  thead,vosys-regmap:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: |
>>> +      Phandle to a syscon node representing the shared register
>>> +      space of the VO (Video Output) subsystem. This register space
>>> +      includes both clock control registers and other control
>>> +      registers used for operations like resetting the GPU. Since
>>
>>
>> It seems you wanted to implement reset controller...
> 
> Thank you for your feedback.
> 
> I understand your concern about the reset controller. In the TH1520 SoC,
> the GPU doesn't have its own reset controller. Instead, its reset is
> managed through the power domain's registers as part of the power-up
> sequence.
> 
> According to the Video Image Processing Manual 1.4.1 [1], the GPU
> requires the following steps to power up:
> 
> 1) Enable the GPU clock gate.
> 2) After 32 core clock cycles, release the GPU soft reset
> 
> Since these steps are closely tied to power management, I implemented
> the reset functionality within the power-domain driver.
> 
> Because the GPU doesn't support the resets property, introducing a reset
> controller wouldn't align with the existing device tree well.

So add resets to GPU. You said here that VO has reset registers, so it
should be a reset controller.

> 
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
>>
>>> +      these registers reside in the same address space, access to
>>> +      them is coordinated through a shared syscon regmap provided by
>>> +      the specified syscon node.
>>
>> Drop last sentence. syscon regmap is a Linux term, not hardware one.
>>
>> Anyway, this needs to be constrained per variant.
>>
>>> +
>>>    "#clock-cells":
>>>      const: 1
>>>      description:
>>> @@ -36,8 +51,6 @@ properties:
>>>  
>>>  required:
>>>    - compatible
>>> -  - reg
>>
>> No, that's a clear NAK. You claim you have no address space but in the
>> same time you have address space via regmap.
> 
> I see your concern. The VOSYS subsystem's address space includes
> registers for various components, such as clock gates and reset
> controls, which are scattered throughout the address space as specified
> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
> regmap for access, but I realize this might not be the best approach.

No, you miss the point of how device nodes are supposed to look like.
Don't bring your driver architecture here.

You cannot have regmap/syscon if you do not have reg. Your VOSYS is a
clock, reset and whatever-provider. Your power domain - if it does not
have reg - just does not exist. There is no such device and we do not
care if you have such device DRIVER.

> 
> To address this, I'll specify the 'reg' property in each node to define
> the address ranges explicitly fragmenting the address space for the VOSYS
> manually.
> 
> vosys_clk: clock-controller@ffef528050 {
> 	compatible = "thead,th1520-clk-vo";
> 	reg = <0xff 0xef528050 0x0 0x8>;
> 	#clock-cells = <1>;
> };
> 
> pd: power-domain@ffef528000 {
> 	compatible = "thead,th1520-pd";
> 	reg = <0xff 0xef528000 0x0 0x8>;
> 	#power-domain-cells = <1>;
> };

I don't think you really get the point here. Clock controllers and power
domains per one clock or domain are also a no-go.

Best regards,
Krzysztof

[RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file

[Michal Wilczynski]

As support for clocks from new subsystems is being added to the T-Head
TH1520 SoC, the Device Tree binding YAML schema file name should reflect
this broader scope.  The existing schema file 'thead,th1520-clk-ap.yaml'
includes the '-ap' suffix, indicating it's specific to the Application
Processor (AP) subsystem.

Rename the YAML schema file to 'thead,th1520-clk.yaml' to generalize it
for all subsystems. Update all references to this schema file
accordingly.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml}   | 2 +-
 MAINTAINERS                                                     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename Documentation/devicetree/bindings/clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} (96%)

diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
similarity index 96%
rename from Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
rename to Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
index 5a8f1041f766..416c8882942e 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
+$id: http://devicetree.org/schemas/clock/thead,th1520-clk.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: T-HEAD TH1520 sub-systems clock controller
diff --git a/MAINTAINERS b/MAINTAINERS
index bd4bbf07d588..2f8f529e6a31 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20183,7 +20183,7 @@ M:	Fu Wei <wefu@redhat.com>
 L:	linux-riscv@lists.infradead.org
 S:	Maintained
 T:	git https://github.com/pdp7/linux.git
-F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+F:	Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
 F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
 F:	Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
 F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
-- 
2.34.1

Re: [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file

[Rob Herring (Arm)]

On Tue, 03 Dec 2024 14:41:29 +0100, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the Device Tree binding YAML schema file name should reflect
> this broader scope.  The existing schema file 'thead,th1520-clk-ap.yaml'
> includes the '-ap' suffix, indicating it's specific to the Application
> Processor (AP) subsystem.
> 
> Rename the YAML schema file to 'thead,th1520-clk.yaml' to generalize it
> for all subsystems. Update all references to this schema file
> accordingly.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml}   | 2 +-
>  MAINTAINERS                                                     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>  rename Documentation/devicetree/bindings/clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} (96%)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-7-m.wilczynski@samsung.com

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

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

pip3 install dtschema --upgrade

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

Re: [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the Device Tree binding YAML schema file name should reflect
> this broader scope.  The existing schema file 'thead,th1520-clk-ap.yaml'
> includes the '-ap' suffix, indicating it's specific to the Application
> Processor (AP) subsystem.
> 
> Rename the YAML schema file to 'thead,th1520-clk.yaml' to generalize it
> for all subsystems. Update all references to this schema file
> accordingly.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml}   | 2 +-
>  MAINTAINERS                                                     | 2 +-

NAK, don't rename just because you added one more compatible (and anyway
never a separate patch).

Best regards,
Krzysztof

[RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520

[Michal Wilczynski]

The T-Head TH1520 SoC contains multiple power islands that can be
programmatically turned on and off using the AON (Always-On) protocol
and a hardware mailbox [1]. The relevant mailbox driver has already been
merged into the mainline kernel in commit 5d4d263e1c6b ("mailbox:
Introduce support for T-head TH1520 Mailbox driver"); however, the AON
implementation is still under development.

This commit introduces a skeleton power-domain driver for the TH1520
SoC, designed to be easily extended to work with the AON protocol in the
future.  Currently, it only supports the GPU. Since there is no
mechanism yet to turn the GPU power island on, the driver will only set
the relevant registers to bring the GPU out of the reset state.  This
should be done after the power-up sequence requested through the mailbox
is completed.

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>
---
 MAINTAINERS                                   |   2 +
 drivers/pmdomain/Kconfig                      |   1 +
 drivers/pmdomain/Makefile                     |   1 +
 drivers/pmdomain/thead/Kconfig                |  12 ++
 drivers/pmdomain/thead/Makefile               |   2 +
 drivers/pmdomain/thead/th1520-pm-domains.c    | 195 ++++++++++++++++++
 .../dt-bindings/power/thead,th1520-power.h    |  19 ++
 7 files changed, 232 insertions(+)
 create mode 100644 drivers/pmdomain/thead/Kconfig
 create mode 100644 drivers/pmdomain/thead/Makefile
 create mode 100644 drivers/pmdomain/thead/th1520-pm-domains.c
 create mode 100644 include/dt-bindings/power/thead,th1520-power.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2f8f529e6a31..16fb58aa74b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20192,7 +20192,9 @@ F:	drivers/clk/thead/
 F:	drivers/mailbox/mailbox-th1520.c
 F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
 F:	drivers/pinctrl/pinctrl-th1520.c
+F:	drivers/pmdomain/thead/th1520-pm-domains.c
 F:	include/dt-bindings/clock/thead,th1520-clk.h
+F:	include/dt-bindings/power/thead,th1520-power.h
 
 RNBD BLOCK DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/drivers/pmdomain/Kconfig b/drivers/pmdomain/Kconfig
index 23c64851a5b0..91f04ace35d4 100644
--- a/drivers/pmdomain/Kconfig
+++ b/drivers/pmdomain/Kconfig
@@ -16,6 +16,7 @@ source "drivers/pmdomain/st/Kconfig"
 source "drivers/pmdomain/starfive/Kconfig"
 source "drivers/pmdomain/sunxi/Kconfig"
 source "drivers/pmdomain/tegra/Kconfig"
+source "drivers/pmdomain/thead/Kconfig"
 source "drivers/pmdomain/ti/Kconfig"
 source "drivers/pmdomain/xilinx/Kconfig"
 
diff --git a/drivers/pmdomain/Makefile b/drivers/pmdomain/Makefile
index a68ece2f4c68..7030f44a49df 100644
--- a/drivers/pmdomain/Makefile
+++ b/drivers/pmdomain/Makefile
@@ -14,6 +14,7 @@ obj-y					+= st/
 obj-y					+= starfive/
 obj-y					+= sunxi/
 obj-y					+= tegra/
+obj-y					+= thead/
 obj-y					+= ti/
 obj-y					+= xilinx/
 obj-y					+= core.o governor.o
diff --git a/drivers/pmdomain/thead/Kconfig b/drivers/pmdomain/thead/Kconfig
new file mode 100644
index 000000000000..8a063b3b96f3
--- /dev/null
+++ b/drivers/pmdomain/thead/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config TH1520_PM_DOMAINS
+	bool "Support TH1520 Power Domains"
+	depends on ARCH_THEAD || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	  This driver enables power domain management for the T-HEAD
+	  TH-1520 SoC. On this SoC there are number of power domains,
+	  which can be managed independently. For example GPU, AUDIO,
+	  NPU, DPU reside in their own power domains which can be
+	  turned on/off.
diff --git a/drivers/pmdomain/thead/Makefile b/drivers/pmdomain/thead/Makefile
new file mode 100644
index 000000000000..adfdf5479c68
--- /dev/null
+++ b/drivers/pmdomain/thead/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_TH1520_PM_DOMAINS)		+= th1520-pm-domains.o
diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
new file mode 100644
index 000000000000..60bdd011b017
--- /dev/null
+++ b/drivers/pmdomain/thead/th1520-pm-domains.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Alibaba Group Holding Limited.
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/power/thead,th1520-power.h>
+
+/* register offset in VOSYS_REGMAP */
+#define TH1520_GPU_RST_CFG		0x0
+#define TH1520_GPU_RST_CFG_MASK		GENMASK(2, 0)
+
+/* register values */
+#define TH1520_GPU_SW_GPU_RST		BIT(0)
+#define TH1520_GPU_SW_CLKGEN_RST	BIT(1)
+
+struct th1520_power_domain {
+	struct regmap *reg;
+	struct generic_pm_domain genpd;
+	u32 rsrc;
+};
+
+struct th1520_power_info {
+	char *name;
+	u32 rsrc;
+};
+
+static const struct th1520_power_info th1520_pd_ranges[] = {
+	{ "gpu", TH1520_AON_GPU_PD }
+};
+
+static inline struct th1520_power_domain *
+to_th1520_power_domain(struct generic_pm_domain *genpd)
+{
+	return container_of(genpd, struct th1520_power_domain, genpd);
+}
+
+static void th1520_rst_gpu_enable(struct regmap *reg)
+{
+	int val;
+
+	/* if the GPU is not in a reset state it, put it into one */
+	regmap_read(reg, TH1520_GPU_RST_CFG, &val);
+	if (val) {
+		regmap_update_bits(reg, TH1520_GPU_RST_CFG,
+				   TH1520_GPU_RST_CFG_MASK, 0x0);
+	}
+
+	/* rst gpu clkgen */
+	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_CLKGEN_RST);
+	/* rst gpu */
+	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);
+}
+
+static void th1520_rst_gpu_disable(struct regmap *reg)
+{
+	regmap_update_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_RST_CFG_MASK, 0x0);
+}
+
+static int th1520_pd_power_on(struct generic_pm_domain *domain)
+{
+	struct th1520_power_domain *pd = to_th1520_power_domain(domain);
+
+	/* The missing component here is the call to E902 core through the
+	 * AON protocol using hardware mailbox.
+	 */
+
+	/* Initially after the power up the GPU and GPU clocks are
+	 * in the reset state. Get them from that state to normal operation.
+	 */
+	th1520_rst_gpu_enable(pd->reg);
+
+	return 0;
+}
+
+static int th1520_pd_power_off(struct generic_pm_domain *domain)
+{
+	struct th1520_power_domain *pd = to_th1520_power_domain(domain);
+
+	/* The missing component here is the call to E902 core through the
+	 * AON protocol using hardware mailbox.
+	 */
+
+	/* Put the GPU into reset state after powering it off */
+	th1520_rst_gpu_disable(pd->reg);
+
+	return 0;
+}
+
+static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
+						 void *data)
+{
+	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
+	struct genpd_onecell_data *pd_data = data;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
+		struct th1520_power_domain *pd;
+
+		pd = to_th1520_power_domain(pd_data->domains[i]);
+		if (pd->rsrc == spec->args[0]) {
+			domain = &pd->genpd;
+			break;
+		}
+	}
+
+	return domain;
+}
+
+static struct th1520_power_domain *
+th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
+{
+	struct th1520_power_domain *pd;
+	int ret;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return ERR_PTR(-ENOMEM);
+
+	pd->rsrc = pi->rsrc;
+	pd->genpd.power_on = th1520_pd_power_on;
+	pd->genpd.power_off = th1520_pd_power_off;
+	pd->genpd.name = pi->name;
+
+	ret = pm_genpd_init(&pd->genpd, NULL, true);
+	if (ret) {
+		devm_kfree(dev, pd);
+		return ERR_PTR(ret);
+	}
+
+	return pd;
+}
+
+static int th1520_pd_probe(struct platform_device *pdev)
+{
+	struct generic_pm_domain **domains;
+	struct genpd_onecell_data *pd_data;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regmap *reg;
+	int i;
+
+	reg = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	domains = devm_kcalloc(dev, ARRAY_SIZE(th1520_pd_ranges),
+			       sizeof(*domains), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
+	if (!pd_data)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
+		struct th1520_power_domain *pd;
+
+		pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
+		if (IS_ERR_OR_NULL(pd))
+			continue;
+
+		pd->reg = reg;
+		domains[i] = &pd->genpd;
+		dev_dbg(dev, "added power domain %s\n", pd->genpd.name);
+	}
+
+	pd_data->domains = domains;
+	pd_data->num_domains = ARRAY_SIZE(th1520_pd_ranges);
+	pd_data->xlate = th1520_pd_xlate;
+
+	return of_genpd_add_provider_onecell(dev->of_node, pd_data);
+}
+
+static const struct of_device_id th1520_pd_match[] = {
+	{ .compatible = "thead,th1520-pd",},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver th1520_pd_driver = {
+	.driver = {
+		.name = "th1520-pd",
+		.of_match_table = th1520_pd_match,
+	},
+	.probe = th1520_pd_probe,
+};
+builtin_platform_driver(th1520_pd_driver);
diff --git a/include/dt-bindings/power/thead,th1520-power.h b/include/dt-bindings/power/thead,th1520-power.h
new file mode 100644
index 000000000000..30fb4e9892e7
--- /dev/null
+++ b/include/dt-bindings/power/thead,th1520-power.h
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2022 Alibaba Group Holding Limited.
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#ifndef __DT_BINDINGS_POWER_TH1520_H
+#define __DT_BINDINGS_POWER_TH1520_H
+
+#define TH1520_AON_AUDIO_PD	0
+#define TH1520_AON_VDEC_PD	1
+#define TH1520_AON_NPU_PD	2
+#define TH1520_AON_VENC_PD	3
+#define TH1520_AON_GPU_PD	4
+#define TH1520_AON_DSP0_PD	5
+#define TH1520_AON_DSP1_PD	6
+
+#endif
-- 
2.34.1

Re: [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The T-Head TH1520 SoC contains multiple power islands that can be
> programmatically turned on and off using the AON (Always-On) protocol
> and a hardware mailbox [1]. The relevant mailbox driver has already been
> merged into the mainline kernel in commit 5d4d263e1c6b ("mailbox:
> Introduce support for T-head TH1520 Mailbox driver"); however, the AON
> implementation is still under development.
> 
> This commit introduces a skeleton power-domain driver for the TH1520
> SoC, designed to be easily extended to work with the AON protocol in the
> future.  Currently, it only supports the GPU. Since there is no
> mechanism yet to turn the GPU power island on, the driver will only set
> the relevant registers to bring the GPU out of the reset state.  This
> should be done after the power-up sequence requested through the mailbox
> is completed.
> 
> 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>
> ---
>  MAINTAINERS                                   |   2 +
>  drivers/pmdomain/Kconfig                      |   1 +
>  drivers/pmdomain/Makefile                     |   1 +
>  drivers/pmdomain/thead/Kconfig                |  12 ++
>  drivers/pmdomain/thead/Makefile               |   2 +
>  drivers/pmdomain/thead/th1520-pm-domains.c    | 195 ++++++++++++++++++
>  .../dt-bindings/power/thead,th1520-power.h    |  19 ++
>  7 files changed, 232 insertions(+)


Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.


>  create mode 100644 drivers/pmdomain/thead/Kconfig
>  create mode 100644 drivers/pmdomain/thead/Makefile
>  create mode 100644 drivers/pmdomain/thead/th1520-pm-domains.c
>  create mode 100644 include/dt-bindings/power/thead,th1520-power.h
> 


...

> +
> +static int th1520_pd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct th1520_power_domain *pd = to_th1520_power_domain(domain);
> +
> +	/* The missing component here is the call to E902 core through the

Use Linux coding style comments (see coding style). This applies to
multiple places in your code.

> +	 * AON protocol using hardware mailbox.
> +	 */
> +
> +	/* Put the GPU into reset state after powering it off */
> +	th1520_rst_gpu_disable(pd->reg);
> +
> +	return 0;
> +}
> +
> +static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
> +						 void *data)
> +{
> +	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
> +	struct genpd_onecell_data *pd_data = data;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
> +		struct th1520_power_domain *pd;
> +
> +		pd = to_th1520_power_domain(pd_data->domains[i]);
> +		if (pd->rsrc == spec->args[0]) {
> +			domain = &pd->genpd;
> +			break;
> +		}
> +	}
> +
> +	return domain;
> +}
> +
> +static struct th1520_power_domain *
> +th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
> +{
> +	struct th1520_power_domain *pd;
> +	int ret;
> +
> +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pd->rsrc = pi->rsrc;
> +	pd->genpd.power_on = th1520_pd_power_on;
> +	pd->genpd.power_off = th1520_pd_power_off;
> +	pd->genpd.name = pi->name;
> +
> +	ret = pm_genpd_init(&pd->genpd, NULL, true);
> +	if (ret) {
> +		devm_kfree(dev, pd);

You should rather fail the probe. Failures of power domains are important.

> +		return ERR_PTR(ret);
> +	}
> +
> +	return pd;
> +}
> +
> +static int th1520_pd_probe(struct platform_device *pdev)
> +{
> +	struct generic_pm_domain **domains;
> +	struct genpd_onecell_data *pd_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct regmap *reg;
> +	int i;
> +
> +	reg = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +
> +	domains = devm_kcalloc(dev, ARRAY_SIZE(th1520_pd_ranges),
> +			       sizeof(*domains), GFP_KERNEL);
> +	if (!domains)
> +		return -ENOMEM;
> +
> +	pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
> +	if (!pd_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
> +		struct th1520_power_domain *pd;
> +
> +		pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
> +		if (IS_ERR_OR_NULL(pd))
> +			continue;
> +
> +		pd->reg = reg;
> +		domains[i] = &pd->genpd;
> +		dev_dbg(dev, "added power domain %s\n", pd->genpd.name);
> +	}
> +
> +	pd_data->domains = domains;
> +	pd_data->num_domains = ARRAY_SIZE(th1520_pd_ranges);
> +	pd_data->xlate = th1520_pd_xlate;
> +
> +	return of_genpd_add_provider_onecell(dev->of_node, pd_data);
> +}
> +
> +static const struct of_device_id th1520_pd_match[] = {
> +	{ .compatible = "thead,th1520-pd",},
> +	{ /* sentinel */ }
> +};
> +


Make the driver tristate and module. There is nothing here which
prevents it being a module.


> +builtin_platform_driver(th1520_pd_driver);
> diff --git a/include/dt-bindings/power/thead,th1520-power.h b/include/dt-bindings/power/thead,th1520-power.h
> new file mode 100644
> index 000000000000..30fb4e9892e7
> --- /dev/null
> +++ b/include/dt-bindings/power/thead,th1520-power.h
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0+


Wrong license. See checkpatch.



Best regards,
Krzysztof

[RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains

[Michal Wilczynski]

Add power domain support to the Thead TH1520 clock controller bindings.
This enables devices to specify their power domain dependencies,
improving power management for components like the GPU.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/power/thead,th1520-power.yaml    | 52 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml

diff --git a/Documentation/devicetree/bindings/power/thead,th1520-power.yaml b/Documentation/devicetree/bindings/power/thead,th1520-power.yaml
new file mode 100644
index 000000000000..528af54f4ca6
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/thead,th1520-power.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/thead,th1520-power.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 Power Domain Controller
+
+maintainers:
+  - Michal Wilczynski <m.wilczynski@samsung.com>
+
+description: |
+  The T-HEAD TH1520 SoC includes a power domain controller responsible for
+  managing the power states of various hardware domains such as the GPU.
+
+  This binding describes the power domain controller node, which can be used by
+  devices to manage their power domains.
+
+properties:
+  compatible:
+    const: "thead,th1520-pd"
+
+  thead,vosys-regmap:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      Phandle to a syscon node representing the shared register space of the VO (Video Output) subsystem.
+      This register space includes both clock control registers and other control registers used for
+      operations like resetting the GPU. Since these registers reside in the same address space,
+      access to them is coordinated through a shared syscon regmap provided by the specified syscon node.
+
+  '#power-domain-cells':
+    const: 1
+
+required:
+  - compatible
+  - thead,vosys-regmap
+  - '#power-domain-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    vosys_regmap: vosys@ffef528000 {
+        compatible = "syscon";
+        reg = <0xff 0xef528000 0x0 0x1000>;
+    };
+
+    power-controller {
+        compatible = "thead,th1520-pd";
+        thead,vosys-regmap = <&vosys_regmap>;
+        #power-domain-cells = <1>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 16fb58aa74b1..acbe311087ad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20187,6 +20187,7 @@ F:	Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
 F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
 F:	Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
 F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
+F:	Documentation/devicetree/bindings/power/thead,th1520-power.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/
 F:	drivers/mailbox/mailbox-th1520.c
-- 
2.34.1

Re: [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains

[Rob Herring (Arm)]

On Tue, 03 Dec 2024 14:41:31 +0100, Michal Wilczynski wrote:
> Add power domain support to the Thead TH1520 clock controller bindings.
> This enables devices to specify their power domain dependencies,
> improving power management for components like the GPU.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/power/thead,th1520-power.yaml    | 52 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: vosys@ffef528000: compatible: ['syscon'] is too short
	from schema $id: http://devicetree.org/schemas/mfd/syscon-common.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: vosys@ffef528000: reg: [[255, 4015161344], [0, 4096]] is too long
	from schema $id: http://devicetree.org/schemas/mfd/syscon-common.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-9-m.wilczynski@samsung.com

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

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

pip3 install dtschema --upgrade

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

Re: [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> +
> +title: T-HEAD TH1520 Power Domain Controller
> +
> +maintainers:
> +  - Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +description: |
> +  The T-HEAD TH1520 SoC includes a power domain controller responsible for
> +  managing the power states of various hardware domains such as the GPU.
> +
> +  This binding describes the power domain controller node, which can be used by

Do not describe the binding. Describe the hardware. Entire paragraph
feels pointless.

> +  devices to manage their power domains.
> +
> +properties:
> +  compatible:
> +    const: "thead,th1520-pd"


You never tested the code you sent. Drop quotes. Limited review follows.

> +
> +  thead,vosys-regmap:

NAK.

'reg' is for this.


> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to a syscon node representing the shared register space of the VO (Video Output) subsystem.

Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool).


> +      This register space includes both clock control registers and other control registers used for
> +      operations like resetting the GPU. Since these registers reside in the same address space,
> +      access to them is coordinated through a shared syscon regmap provided by the specified syscon node.
> +
> +  '#power-domain-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - thead,vosys-regmap
> +  - '#power-domain-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    vosys_regmap: vosys@ffef528000 {
> +        compatible = "syscon";
> +        reg = <0xff 0xef528000 0x0 0x1000>;
> +    };

Drop, not related.



Best regards,
Krzysztof

[RFC PATCH v1 09/14] riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs

[Michal Wilczynski]

T-Head SoCs feature separate power domains (power islands) for major
components like the GPU, Audio, and NPU. To manage the power states of
these components effectively, the kernel requires generic power domain
support.

This commit enables `CONFIG_PM_GENERIC_DOMAINS` for T-Head SoCs,
allowing the power domain driver for these components to be compiled and
integrated. This ensures proper power management and energy efficiency
on T-Head platforms.

By selecting `PM_GENERIC_DOMAINS`, we provide the necessary framework
for the power domain drivers to function correctly on RISC-V
architecture with T-Head SoCs.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/Kconfig.socs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index f51bb24bc84c..c414dc618b66 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -48,6 +48,7 @@ config ARCH_THEAD
 	bool "T-HEAD RISC-V SoCs"
 	depends on MMU && !XIP_KERNEL
 	select ERRATA_THEAD
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  This enables support for the RISC-V based T-HEAD SoCs.
 
-- 
2.34.1

[RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU

[Michal Wilczynski]

The IMG BXM-4-64 GPU is integrated into the T-Head TH1520 SoC. This
commit adds the compatible string "img,img-bxm-4-64" to the device tree
match table in the drm/imagination driver, enabling support for this
GPU.

By including this GPU in the compatible devices list, the driver can
initialize and manage the BXM-4-64 GPU on the TH1520 SoC, providing
graphics acceleration capabilities upstream.

This commit doesn't touch the img,powervr-rogue.yaml on purpose, as the
new dt-bindings schema was proposed [1], but not merged yet.

Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/pvr_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 85ee9abd1811..8633a3a315b7 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1475,6 +1475,7 @@ static void pvr_remove(struct platform_device *plat_dev)
 
 static const struct of_device_id dt_match[] = {
 	{ .compatible = "img,img-axe", .data = NULL },
+	{ .compatible = "img,img-bxm-4-64", .data = NULL },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
2.34.1

Re: [RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The IMG BXM-4-64 GPU is integrated into the T-Head TH1520 SoC. This
> commit adds the compatible string "img,img-bxm-4-64" to the device tree
> match table in the drm/imagination driver, enabling support for this
> GPU.
> 
> By including this GPU in the compatible devices list, the driver can
> initialize and manage the BXM-4-64 GPU on the TH1520 SoC, providing
> graphics acceleration capabilities upstream.
> 
> This commit doesn't touch the img,powervr-rogue.yaml on purpose, as the
> new dt-bindings schema was proposed [1], but not merged yet.


That's not related to the commit. This commit *cannot ever* touch the
bindings.

> 
> Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/gpu/drm/imagination/pvr_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 85ee9abd1811..8633a3a315b7 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1475,6 +1475,7 @@ static void pvr_remove(struct platform_device *plat_dev)
>  
>  static const struct of_device_id dt_match[] = {
>  	{ .compatible = "img,img-axe", .data = NULL },
> +	{ .compatible = "img,img-bxm-4-64", .data = NULL },

Undocumented compatible. Combine relevant patches into one patchsets, so
we see entire picture.

Best regards,
Krzysztof

[RFC PATCH v1 11/14] drm/imagination: Enable PowerVR driver for RISC-V

[Michal Wilczynski]

Several RISC-V boards feature Imagination GPUs that are compatible with
the PowerVR driver. An example is the IMG BXM-4-64 GPU on the Lichee Pi
4A board. This commit adjusts the driver's Kconfig dependencies to allow
the PowerVR driver to be compiled on the RISC-V architecture.

By enabling compilation on RISC-V, we expand support for these GPUs,
providing graphics acceleration capabilities and enhancing hardware
compatibility on RISC-V platforms.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
index 3bfa2ac212dc..5f218896114c 100644
--- a/drivers/gpu/drm/imagination/Kconfig
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -3,7 +3,7 @@
 
 config DRM_POWERVR
 	tristate "Imagination Technologies PowerVR (Series 6 and later) & IMG Graphics"
-	depends on ARM64
+	depends on (ARM64 || RISCV)
 	depends on DRM
 	depends on PM
 	select DRM_EXEC
-- 
2.34.1

[RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes

[Michal Wilczynski]

The address space controlling the Video Output (VO) subsystem clocks
also contains control registers for GPU resets. To properly synchronize
access to this shared address space, create a syscon Device Tree node
for the VO registers and reference it in the clock controller node.

This change ensures coordinated access to the VO registers between the
clock controller and other drivers, preventing conflicts and maintaining
system stability.

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index dc2d554b4a71..39d39059160d 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -489,6 +489,18 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
+		vosys_clk: clock-controller {
+			compatible = "thead,th1520-clk-vo";
+			thead,vosys-regmap = <&vosys_reg>;
+			#clock-cells = <1>;
+		};
+
+		vosys_reg: vosys@ffef528000 {
+			compatible = "thead,th1520-vosys", "syscon";
+			reg = <0xff 0xef528000 0x0 0x1000>;
+			status = "okay";
+		};
+
 		dmac0: dma-controller@ffefc00000 {
 			compatible = "snps,axi-dma-1.01a";
 			reg = <0xff 0xefc00000 0x0 0x1000>;
-- 
2.34.1

Re: [RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The address space controlling the Video Output (VO) subsystem clocks
> also contains control registers for GPU resets. To properly synchronize
> access to this shared address space, create a syscon Device Tree node
> for the VO registers and reference it in the clock controller node.
> 
> This change ensures coordinated access to the VO registers between the
> clock controller and other drivers, preventing conflicts and maintaining
> system stability.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index dc2d554b4a71..39d39059160d 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -489,6 +489,18 @@ clk: clock-controller@ffef010000 {
>  			#clock-cells = <1>;
>  		};
>  
> +		vosys_clk: clock-controller {

Missing address space. You cannot have here nodes without unit address.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +			compatible = "thead,th1520-clk-vo";
> +			thead,vosys-regmap = <&vosys_reg>;
> +			#clock-cells = <1>;
> +		};
> +
> +		vosys_reg: vosys@ffef528000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation



> +			compatible = "thead,th1520-vosys", "syscon";
> +			reg = <0xff 0xef528000 0x0 0x1000>;
> +			status = "okay";

Where is it disabled? Drop.

Best regards,
Krzysztof

[RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible

[Michal Wilczynski]

The DRM Imagination GPU requires a power-domain driver, but the driver
for "thead,th1520-aon" is not yet available. To ensure that the 'aon'
node and its child 'pd' node are properly recognized and probed by the
kernel, add "simple-bus" to the compatible property of the 'aon' node.

This change allows the kernel to treat the 'aon' node as a simple bus,
enabling the child nodes to be probed and initialized independently. It
ensures that the power domain can be managed appropriately until the
specific AON driver is developed.

This commit introduces some errors while running dtbs_check, as the aon
doesn't have the dt-bindings yet.

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 39d39059160d..58f93ad3eb6e 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -6,6 +6,7 @@
 
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/clock/thead,th1520-clk.h>
+#include <dt-bindings/power/thead,th1520-power.h>
 
 / {
 	compatible = "thead,th1520";
@@ -229,6 +230,16 @@ stmmac_axi_config: stmmac-axi-config {
 		snps,blen = <0 0 64 32 0 0 0>;
 	};
 
+	aon {
+		compatible = "thead,th1520-aon", "simple-bus";
+
+		pd: power-domain {
+			compatible = "thead,th1520-pd";
+			thead,vosys-regmap = <&vosys_reg>;
+			#power-domain-cells = <1>;
+		};
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;
-- 
2.34.1

Re: [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The DRM Imagination GPU requires a power-domain driver, but the driver
> for "thead,th1520-aon" is not yet available. To ensure that the 'aon'
> node and its child 'pd' node are properly recognized and probed by the
> kernel, add "simple-bus" to the compatible property of the 'aon' node.
> 
> This change allows the kernel to treat the 'aon' node as a simple bus,
> enabling the child nodes to be probed and initialized independently. It
> ensures that the power domain can be managed appropriately until the
> specific AON driver is developed.
> 
> This commit introduces some errors while running dtbs_check, as the aon
> doesn't have the dt-bindings yet.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 39d39059160d..58f93ad3eb6e 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -6,6 +6,7 @@
>  
>  #include <dt-bindings/interrupt-controller/irq.h>
>  #include <dt-bindings/clock/thead,th1520-clk.h>
> +#include <dt-bindings/power/thead,th1520-power.h>
>  
>  / {
>  	compatible = "thead,th1520";
> @@ -229,6 +230,16 @@ stmmac_axi_config: stmmac-axi-config {
>  		snps,blen = <0 0 64 32 0 0 0>;
>  	};
>  
> +	aon {
> +		compatible = "thead,th1520-aon", "simple-bus";

1. No, that's not a bus.
2. Please run scripts/checkpatch.pl and fix reported warnings. Then
please run `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

Sorry, this patchset is not ready, unless by RFC you meant - do not
review, because it is not ready. Then it is fine. But then *clearly
express* this in cover letter, so we know what you expect from us (and I
would not waste my time to go through all this).

Best regards,
Krzysztof

Re: [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible

[Michal Wilczynski]

On 12/3/24 16:52, Krzysztof Kozlowski wrote:
> On 03/12/2024 14:41, Michal Wilczynski wrote:
>> The DRM Imagination GPU requires a power-domain driver, but the driver
>> for "thead,th1520-aon" is not yet available. To ensure that the 'aon'
>> node and its child 'pd' node are properly recognized and probed by the
>> kernel, add "simple-bus" to the compatible property of the 'aon' node.
>>
>> This change allows the kernel to treat the 'aon' node as a simple bus,
>> enabling the child nodes to be probed and initialized independently. It
>> ensures that the power domain can be managed appropriately until the
>> specific AON driver is developed.
>>
>> This commit introduces some errors while running dtbs_check, as the aon
>> doesn't have the dt-bindings yet.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index 39d39059160d..58f93ad3eb6e 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -6,6 +6,7 @@
>>  
>>  #include <dt-bindings/interrupt-controller/irq.h>
>>  #include <dt-bindings/clock/thead,th1520-clk.h>
>> +#include <dt-bindings/power/thead,th1520-power.h>
>>  
>>  / {
>>  	compatible = "thead,th1520";
>> @@ -229,6 +230,16 @@ stmmac_axi_config: stmmac-axi-config {
>>  		snps,blen = <0 0 64 32 0 0 0>;
>>  	};
>>  
>> +	aon {
>> +		compatible = "thead,th1520-aon", "simple-bus";
> 
> 1. No, that's not a bus.

I understand that using "simple-bus" for the 'aon' node was not
appropriate. Since the 'aon' node isn't needed for testing this
patchset, I will remove it and move the power-domain device tree node to
the SoC. Future changes to the 'aon' node will be handled in a separate
patchset.

> 2. Please run scripts/checkpatch.pl and fix reported warnings. Then
> please run `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.
> 
> Sorry, this patchset is not ready, unless by RFC you meant - do not
> review, because it is not ready. Then it is fine. But then *clearly
> express* this in cover letter, so we know what you expect from us (and I
> would not waste my time to go through all this).

My intention with this patchset was to gather feedback on the overall
direction of the changes. I understand that clearer communication in the
cover letter would have been beneficial. Moving forward, I will ensure
that the patch's readiness and expectations are explicitly stated.

Thanks a lot for your review.
Michał

> 
> Best regards,
> Krzysztof
> 

[RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree

[Michal Wilczynski]

Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
the GPU using the drm/imagination driver.

By adding this node, the kernel can recognize and initialize the GPU,
providing graphics acceleration capabilities on the Lichee Pi 4A and
other boards based on the TH1520 SoC.

This commit is following convention introduced here [1].

Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 58f93ad3eb6e..5023c0c29168 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -500,6 +500,18 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
+		gpu: gpu@ffef400000 {
+			compatible = "img,img-bxm-4-64", "img,img-rogue";
+			reg = <0xff 0xef400000 0x0 0x100000>;
+			interrupt-parent = <&plic>;
+			interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&vosys_clk CLK_GPU_CORE>,
+				 <&vosys_clk CLK_GPU_CFG_ACLK>;
+			clock-names = "core", "sys";
+			power-domains = <&pd TH1520_AON_GPU_PD>;
+			status = "okay";
+		};
+
 		vosys_clk: clock-controller {
 			compatible = "thead,th1520-clk-vo";
 			thead,vosys-regmap = <&vosys_reg>;
-- 
2.34.1

Re: [RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
> the GPU using the drm/imagination driver.
> 
> By adding this node, the kernel can recognize and initialize the GPU,
> providing graphics acceleration capabilities on the Lichee Pi 4A and
> other boards based on the TH1520 SoC.
> 
> This commit is following convention introduced here [1].
> 
> Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 58f93ad3eb6e..5023c0c29168 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -500,6 +500,18 @@ clk: clock-controller@ffef010000 {
>  			#clock-cells = <1>;
>  		};
>  
> +		gpu: gpu@ffef400000 {
> +			compatible = "img,img-bxm-4-64", "img,img-rogue";
> +			reg = <0xff 0xef400000 0x0 0x100000>;
> +			interrupt-parent = <&plic>;
> +			interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&vosys_clk CLK_GPU_CORE>,
> +				 <&vosys_clk CLK_GPU_CFG_ACLK>;
> +			clock-names = "core", "sys";
> +			power-domains = <&pd TH1520_AON_GPU_PD>;
> +			status = "okay";

Open existing DTSI and look how it is done. There is no single line like
'status = okay', so please do not introduce some entirely different
coding style.

Best regards,
Krzysztof