[PATCH v4 00/18] Enable drm/imagination BXM-4-64 Support for LicheePi 4A

[PATCH v4 00/18] 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. Support for the mailbox was merged
in kernel 6.13 [13], and the AON protocol implementation is part of this
series, since v2. Therefore this series support full power management
capabilities for the GPU driver.

Thanks everyone for taking the time to review the last revision ! Your
guidance and the direction was very helpful.

Since this patchset is slowly getting closer to being ready I'm dropping the
RFC prefix with the v4.

v4:

Device Tree Changes:
- restructured power device tree bindings by abstracting power-domain IDs from
  firmware IDs, with the mappings now defined in a dedicated header file
- retained existing clock-names in img,powervr-rogue.yaml to maintain ABI
  compatibility, despite having only one downstream user
- enhanced documentation for new Video Output (VO) clock inputs in device tree
  bindings

Reset Driver Changes:
- reverted reset-cells configuration to single cell as in v2
- maintained reset definitions in device tree bindings while deferring
  implementation of watchdog timer (WDT) reset functionality
- addressed implementation issues in the DRM/Imagination reset driver

Power Domain Changes:
- added workaround to disable AUDIO power domain to prevent firmware crashes

v3:

Device Tree Changes:
 - consolidated device tree representation by merging aon and power-domain nodes
   while maintaining separate drivers internally
 - power-domain driver is now instantiated from within the aon driver
 - updated img,powervr-rogue.yaml to use allOf and oneOf for better schema
   organization

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

Code Quality and Documentation:
 - fixed optional module dependencies in Kconfig
 - added kernel-doc comments for all exported functions
 - implemented th1520_aon_remove() to properly clean up mailbox channel
   resources
 - removed unnecessary of.h header in multiple drivers
 - refactored reset driver to use zero cells

v2:

Removed AP_SUBSYS clock refactoring commits (1-6):
 - instead of refactoring, I opted to extend the current driver and its
   associated device tree node to include support for a second address space.

Expanded patchset scope to fully support power management capabilities:
 - introduced a new firmware driver to manage power-related operations.
 - rewrote the power-domain driver to function alongside the firmware driver.
   These nodes in the device tree lack direct address spaces, despite
   representing HW blocks. Control is achieved via firmware protocol messages
   transmitted through a mailbox to the E902 core.

Implemented a reset controller for the TH1520 SoC:
 - developed a reset controller driver for the TH1520 to manage reset
   sequences.
 - updated the drm/imagination driver to act as a reset controller consumer.
   While this patchset is focused on the LPI4A board, the reset controller is
   designed to be useful for other boards, such as the BPI-3F, which also require
   a reset sequence after power-up.

Updated dt-bindings:
 - added new dt-bindings for power, reset, and firmware nodes.
 - updated the powervr dt-binding to include reset support and new compatibles.
 - ran dtbs_check and dt_binding_check to ensure compliance.

Addressed code quality:
 - resolved all checkpatch issues using --strict, except for the call to
   devm_clk_hw_register_gate_parent_data().  The current implementation remains
   preferable in this context, and clang-format aligns with this choice.

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 (18):
  dt-bindings: clock: thead: Add TH1520 VO clock controller
  clk: thead: Add clock support for VO subsystem in T-Head TH1520 SoC
  dt-bindings: firmware: thead,th1520: Add support for firmware node
  firmware: thead: Add AON firmware protocol driver
  dt-bindings: power: Add TH1520 SoC power domains
  pmdomain: thead: Add power-domain driver for TH1520
  riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs
  dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller
  reset: thead: Add TH1520 reset controller driver
  drm/imagination: Add reset controller support for GPU initialization
  dt-bindings: gpu: Add 'resets' property for GPU initialization
  dt-bindings: gpu: Add support for T-HEAD TH1520 GPU
  drm/imagination: Add support for IMG BXM-4-64 GPU
  drm/imagination: Enable PowerVR driver for RISC-V
  riscv: dts: thead: Add device tree VO clock controller
  riscv: dts: thead: Introduce power domain nodes with aon firmware
  riscv: dts: thead: Introduce reset controller node
  riscv: dts: thead: Add GPU node to TH1520 device tree

 .../bindings/clock/thead,th1520-clk-ap.yaml   |  17 +-
 .../bindings/firmware/thead,th1520-aon.yaml   |  53 ++++
 .../bindings/gpu/img,powervr-rogue.yaml       |  42 ++-
 .../bindings/reset/thead,th1520-reset.yaml    |  44 +++
 MAINTAINERS                                   |   8 +
 arch/riscv/Kconfig.socs                       |   1 +
 arch/riscv/boot/dts/thead/th1520.dtsi         |  34 +++
 drivers/clk/thead/clk-th1520-ap.c             | 197 +++++++++++--
 drivers/firmware/Kconfig                      |   9 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/thead,th1520-aon.c           | 268 ++++++++++++++++++
 drivers/gpu/drm/imagination/Kconfig           |   2 +-
 drivers/gpu/drm/imagination/pvr_device.c      |  21 ++
 drivers/gpu/drm/imagination/pvr_device.h      |   9 +
 drivers/gpu/drm/imagination/pvr_drv.c         |   1 +
 drivers/gpu/drm/imagination/pvr_power.c       |  12 +-
 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    | 193 +++++++++++++
 drivers/reset/Kconfig                         |  10 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-th1520.c                  | 178 ++++++++++++
 .../dt-bindings/clock/thead,th1520-clk-ap.h   |  33 +++
 .../dt-bindings/power/thead,th1520-power.h    |  19 ++
 .../dt-bindings/reset/thead,th1520-reset.h    |  15 +
 .../linux/firmware/thead/thead,th1520-aon.h   | 197 +++++++++++++
 28 files changed, 1344 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
 create mode 100644 Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
 create mode 100644 drivers/firmware/thead,th1520-aon.c
 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 drivers/reset/reset-th1520.c
 create mode 100644 include/dt-bindings/power/thead,th1520-power.h
 create mode 100644 include/dt-bindings/reset/thead,th1520-reset.h
 create mode 100644 include/linux/firmware/thead/thead,th1520-aon.h

-- 
2.34.1

[PATCH v4 01/18] dt-bindings: clock: thead: Add TH1520 VO clock controller

[Michal Wilczynski]

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

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

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

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/clock/thead,th1520-clk-ap.yaml   | 17 ++++++++--
 .../dt-bindings/clock/thead,th1520-clk-ap.h   | 33 +++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
index 0129bd0ba4b3..9d058c00ab3d 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
@@ -8,7 +8,8 @@ title: T-HEAD TH1520 AP sub-system clock controller
 
 description: |
   The T-HEAD TH1520 AP sub-system clock controller configures the
-  CPU, DPU, GMAC and TEE PLLs.
+  CPU, DPU, GMAC and TEE PLLs. Additionally the VO subsystem configures
+  the clock gates for the HDMI, MIPI and the GPU.
 
   SoC reference manual
   https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
@@ -20,14 +21,24 @@ maintainers:
 
 properties:
   compatible:
-    const: thead,th1520-clk-ap
+    enum:
+      - thead,th1520-clk-ap
+      - thead,th1520-clk-vo
 
   reg:
     maxItems: 1
 
   clocks:
     items:
-      - description: main oscillator (24MHz)
+      - description: |
+          One input clock:
+          - For "thead,th1520-clk-ap": the clock input must be the 24 MHz
+            main oscillator.
+          - For "thead,th1520-clk-vo": the clock input must be the VIDEO_PLL,
+            which is configured by the AP clock controller. According to the
+            TH1520 manual, VIDEO_PLL is a Silicon Creations Sigma-Delta PLL
+            (integer PLL) typically running at 792 MHz (FOUTPOSTDIV), with
+            a maximum FOUTVCO of 2376 MHz.
 
   "#clock-cells":
     const: 1
diff --git a/include/dt-bindings/clock/thead,th1520-clk-ap.h b/include/dt-bindings/clock/thead,th1520-clk-ap.h
index a199784b3512..470fa34f9a9d 100644
--- a/include/dt-bindings/clock/thead,th1520-clk-ap.h
+++ b/include/dt-bindings/clock/thead,th1520-clk-ap.h
@@ -93,4 +93,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: [PATCH v4 01/18] dt-bindings: clock: thead: Add TH1520 VO clock controller

[Krzysztof Kozlowski]

On Tue, Jan 28, 2025 at 08:47:59PM +0100, Michal Wilczynski wrote:
> Add device tree bindings for the TH1520 Video Output (VO) subsystem
> clock controller. The VO sub-system manages clock gates for multimedia
> components including HDMI, MIPI, and GPU.
> 
> Document the VIDEO_PLL requirements for the VO clock controller, which
> receives its input from the AP clock controller. The VIDEO_PLL is a
> Silicon Creations Sigma-Delta (integer) PLL typically running at 792 MHz
> with maximum FOUTVCO of 2376 MHz.
> 
> This binding complements the existing AP sub-system clock controller
> which manages CPU, DPU, GMAC and TEE PLLs.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 17 ++++++++--
>  .../dt-bindings/clock/thead,th1520-clk-ap.h   | 33 +++++++++++++++++++
>  2 files changed, 47 insertions(+), 3 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v4 02/18] clk: thead: Add clock support for VO subsystem in T-Head TH1520 SoC

[Michal Wilczynski]

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

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

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

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

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

Re: [PATCH v4 02/18] clk: thead: Add clock support for VO subsystem in T-Head TH1520 SoC

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 12166 bytes --]

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

I see CORE and CFG clocks here; what about MEM? It's listed in the
linked TRM as BIT(2).

Cheers,
Matt

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

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v4 02/18] clk: thead: Add clock support for VO subsystem in T-Head TH1520 SoC

[Michal Wilczynski]

On 1/31/25 16:39, Matt Coster wrote:
> On 28/01/2025 19:48, Michal Wilczynski wrote:
>> The T-Head TH1520 SoC integrates a variety of clocks for its subsystems,
>> including the Application Processor (AP) and the Video Output (VO) [1].
>> Up until now, the T-Head clock driver only supported AP clocks.
>>
>> This commit extends the driver to provide clock functionality for the VO
>> subsystem. At this stage, the focus is on implementing the VO clock
>> gates, as these are currently the most relevant and required components
>> for enabling and disabling the VO subsystem functionality.  Future
>> enhancements may introduce additional VO-related clocks as necessary.
>>
>> Link: https://protect2.fireeye.com/v1/url?k=e2f52701-bd6e1e1e-e2f4ac4e-000babdfecba-2c98780e6a3b772c&q=1&e=4ed72090-09ae-4d99-9a05-caed33873a2b&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf  [1]
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/clk/thead/clk-th1520-ap.c | 197 +++++++++++++++++++++++++-----
>>  1 file changed, 169 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>> index 4c9555fc6184..57972589f120 100644
>> --- a/drivers/clk/thead/clk-th1520-ap.c
>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>> @@ -847,6 +847,67 @@ static CCU_GATE(CLK_SRAM1, sram1_clk, "sram1", axi_aclk_pd, 0x20c, BIT(3), 0);
>>  static CCU_GATE(CLK_SRAM2, sram2_clk, "sram2", axi_aclk_pd, 0x20c, BIT(2), 0);
>>  static CCU_GATE(CLK_SRAM3, sram3_clk, "sram3", axi_aclk_pd, 0x20c, BIT(1), 0);
>>  
>> +static CCU_GATE(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(0), 0);
>> +static CCU_GATE(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_clk_pd,
>> +		0x0, BIT(3), 0);
>> +static CCU_GATE(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
>> +		video_pll_clk_pd, 0x0, BIT(4), 0);
> 
> I see CORE and CFG clocks here; what about MEM? It's listed in the
> linked TRM as BIT(2).

It is however marked as 'Reserved' in manual, so I assumed I shouldn't
touch it.

Additionally referring to the vendor kernel, only two clocks are used to
program the GPU [1] in the old non-upstream driver.

[1] - https://github.com/revyos/thead-kernel/blob/lpi4a/drivers/gpu/drm/img-rogue/thead_sys.c#L400

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

[PATCH v4 03/18] dt-bindings: firmware: thead,th1520: Add support for firmware node

[Michal Wilczynski]

The kernel communicates with the E902 core through the mailbox
transport using AON firmware protocol. Add dt-bindings to document it
the dt node.

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

diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
new file mode 100644
index 000000000000..bbc183200400
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/thead,th1520-aon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 AON (Always-On) Firmware
+
+description: |
+  The Always-On (AON) subsystem in the TH1520 SoC is responsible for managing
+  low-power states, system wakeup events, and power management tasks. It is
+  designed to operate independently in a dedicated power domain, allowing it to
+  remain functional even during the SoC's deep sleep states.
+
+  At the heart of the AON subsystem is the E902, a low-power core that executes
+  firmware responsible for coordinating tasks such as power domain control,
+  clock management, and system wakeup signaling. Communication between the main
+  SoC and the AON subsystem is handled through a mailbox interface, which
+  enables message-based interactions with the AON firmware.
+
+maintainers:
+  - Michal Wilczynski <m.wilczynski@samsung.com>
+
+properties:
+  compatible:
+    const: thead,th1520-aon
+
+  mboxes:
+    maxItems: 1
+
+  mbox-names:
+    items:
+      - const: aon
+
+  "#power-domain-cells":
+    const: 1
+
+required:
+  - compatible
+  - mboxes
+  - mbox-names
+  - "#power-domain-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    aon: aon {
+        compatible = "thead,th1520-aon";
+        mboxes = <&mbox_910t 1>;
+        mbox-names = "aon";
+        #power-domain-cells = <1>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d49306cc17e3..002eaae363aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20340,6 +20340,7 @@ 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/firmware/thead,th1520-aon.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: [PATCH v4 03/18] dt-bindings: firmware: thead,th1520: Add support for firmware node

[Krzysztof Kozlowski]

On Tue, Jan 28, 2025 at 08:48:01PM +0100, Michal Wilczynski wrote:
> The kernel communicates with the E902 core through the mailbox
> transport using AON firmware protocol. Add dt-bindings to document it
> the dt node.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/firmware/thead,th1520-aon.yaml   | 53 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v4 04/18] firmware: thead: Add AON firmware protocol driver

[Michal Wilczynski]

The T-Head TH1520 SoC uses an E902 co-processor running Always-On (AON)
firmware to manage power, clock, and other system resources [1]. This
patch introduces a driver implementing the AON firmware protocol,
allowing the Linux kernel to communicate with the firmware via mailbox
channels.  Through an RPC-based interface, the kernel can initiate power
state transitions, update resource configurations, and perform other
AON-related tasks.

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/firmware/Kconfig                      |   9 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/thead,th1520-aon.c           | 268 ++++++++++++++++++
 .../linux/firmware/thead/thead,th1520-aon.h   | 197 +++++++++++++
 5 files changed, 477 insertions(+)
 create mode 100644 drivers/firmware/thead,th1520-aon.c
 create mode 100644 include/linux/firmware/thead/thead,th1520-aon.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 002eaae363aa..9a98b52fffdc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20346,10 +20346,12 @@ 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/firmware/thead,th1520-aon.c
 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/linux/firmware/thead/thead,th1520-aon.h
 
 RNBD BLOCK DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 71d8b26c4103..4d84288cc290 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -212,6 +212,15 @@ config SYSFB_SIMPLEFB
 
 	  If unsure, say Y.
 
+config TH1520_AON_PROTOCOL
+	tristate "Always-On firmware protocol"
+	depends on ARCH_THEAD || COMPILE_TEST
+	help
+	  Power, clock, and resource management capabilities on the TH1520 SoC are
+	  managed by the E902 core. Firmware running on this core communicates with
+	  the kernel through the Always-On protocol, using hardware mailbox as a medium.
+	  Say yes if you need such capabilities.
+
 config TI_SCI_PROTOCOL
 	tristate "TI System Control Interface (TISCI) Message Protocol"
 	depends on TI_MESSAGE_MANAGER
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 7a8d486e718f..5db9c042430c 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
 obj-$(CONFIG_SYSFB)		+= sysfb.o
 obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
+obj-$(CONFIG_TH1520_AON_PROTOCOL) += thead,th1520-aon.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
diff --git a/drivers/firmware/thead,th1520-aon.c b/drivers/firmware/thead,th1520-aon.c
new file mode 100644
index 000000000000..fc2fe32c8a54
--- /dev/null
+++ b/drivers/firmware/thead,th1520-aon.c
@@ -0,0 +1,268 @@
+// 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/firmware/thead/thead,th1520-aon.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define MAX_RX_TIMEOUT (msecs_to_jiffies(3000))
+#define MAX_TX_TIMEOUT 500
+
+struct th1520_aon_chan {
+	struct platform_device *pd;
+	struct mbox_chan *ch;
+	struct th1520_aon_rpc_ack_common ack_msg;
+	struct mbox_client cl;
+	struct completion done;
+
+	/* make sure only one RPC is performed at a time */
+	struct mutex transaction_lock;
+};
+
+struct th1520_aon_msg_req_set_resource_power_mode {
+	struct th1520_aon_rpc_msg_hdr hdr;
+	u16 resource;
+	u16 mode;
+	u16 reserved[10];
+} __packed __aligned(1);
+
+/*
+ * This type is used to indicate error response for most functions.
+ */
+enum th1520_aon_error_codes {
+	LIGHT_AON_ERR_NONE = 0, /* Success */
+	LIGHT_AON_ERR_VERSION = 1, /* Incompatible API version */
+	LIGHT_AON_ERR_CONFIG = 2, /* Configuration error */
+	LIGHT_AON_ERR_PARM = 3, /* Bad parameter */
+	LIGHT_AON_ERR_NOACCESS = 4, /* Permission error (no access) */
+	LIGHT_AON_ERR_LOCKED = 5, /* Permission error (locked) */
+	LIGHT_AON_ERR_UNAVAILABLE = 6, /* Unavailable (out of resources) */
+	LIGHT_AON_ERR_NOTFOUND = 7, /* Not found */
+	LIGHT_AON_ERR_NOPOWER = 8, /* No power */
+	LIGHT_AON_ERR_IPC = 9, /* Generic IPC error */
+	LIGHT_AON_ERR_BUSY = 10, /* Resource is currently busy/active */
+	LIGHT_AON_ERR_FAIL = 11, /* General I/O failure */
+	LIGHT_AON_ERR_LAST
+};
+
+static int th1520_aon_linux_errmap[LIGHT_AON_ERR_LAST] = {
+	0, /* LIGHT_AON_ERR_NONE */
+	-EINVAL, /* LIGHT_AON_ERR_VERSION */
+	-EINVAL, /* LIGHT_AON_ERR_CONFIG */
+	-EINVAL, /* LIGHT_AON_ERR_PARM */
+	-EACCES, /* LIGHT_AON_ERR_NOACCESS */
+	-EACCES, /* LIGHT_AON_ERR_LOCKED */
+	-ERANGE, /* LIGHT_AON_ERR_UNAVAILABLE */
+	-EEXIST, /* LIGHT_AON_ERR_NOTFOUND */
+	-EPERM, /* LIGHT_AON_ERR_NOPOWER */
+	-EPIPE, /* LIGHT_AON_ERR_IPC */
+	-EBUSY, /* LIGHT_AON_ERR_BUSY */
+	-EIO, /* LIGHT_AON_ERR_FAIL */
+};
+
+static inline int th1520_aon_to_linux_errno(int errno)
+{
+	if (errno >= LIGHT_AON_ERR_NONE && errno < LIGHT_AON_ERR_LAST)
+		return th1520_aon_linux_errmap[errno];
+
+	return -EIO;
+}
+
+static void th1520_aon_rx_callback(struct mbox_client *c, void *rx_msg)
+{
+	struct th1520_aon_chan *aon_chan =
+		container_of(c, struct th1520_aon_chan, cl);
+	struct th1520_aon_rpc_msg_hdr *hdr =
+		(struct th1520_aon_rpc_msg_hdr *)rx_msg;
+	u8 recv_size = sizeof(struct th1520_aon_rpc_msg_hdr) + hdr->size;
+
+	if (recv_size != sizeof(struct th1520_aon_rpc_ack_common)) {
+		dev_err(c->dev, "Invalid ack size, not completing\n");
+		return;
+	}
+
+	memcpy(&aon_chan->ack_msg, rx_msg, recv_size);
+	complete(&aon_chan->done);
+}
+
+/**
+ * th1520_aon_call_rpc() - Send an RPC request to the TH1520 AON subsystem
+ * @aon_chan: Pointer to the AON channel structure
+ * @msg: Pointer to the message (RPC payload) that will be sent
+ *
+ * This function sends an RPC message to the TH1520 AON subsystem via mailbox.
+ * It takes the provided @msg buffer, formats it with version and service flags,
+ * then blocks until the RPC completes or times out. The completion is signaled
+ * by the `aon_chan->done` completion, which is waited upon for a duration
+ * defined by `MAX_RX_TIMEOUT`.
+ *
+ * Return:
+ * * 0 on success
+ * * -ETIMEDOUT if the RPC call times out
+ * * A negative error code if the mailbox send fails or if AON responds with
+ *   a non-zero error code (converted via th1520_aon_to_linux_errno()).
+ */
+int th1520_aon_call_rpc(struct th1520_aon_chan *aon_chan, void *msg)
+{
+	struct th1520_aon_rpc_msg_hdr *hdr = msg;
+	int ret;
+
+	mutex_lock(&aon_chan->transaction_lock);
+	reinit_completion(&aon_chan->done);
+
+	RPC_SET_VER(hdr, TH1520_AON_RPC_VERSION);
+	RPC_SET_SVC_ID(hdr, hdr->svc);
+	RPC_SET_SVC_FLAG_MSG_TYPE(hdr, RPC_SVC_MSG_TYPE_DATA);
+	RPC_SET_SVC_FLAG_ACK_TYPE(hdr, RPC_SVC_MSG_NEED_ACK);
+
+	ret = mbox_send_message(aon_chan->ch, msg);
+	if (ret < 0) {
+		dev_err(aon_chan->cl.dev, "RPC send msg failed: %d\n", ret);
+		goto out;
+	}
+
+	if (!wait_for_completion_timeout(&aon_chan->done, MAX_RX_TIMEOUT)) {
+		dev_err(aon_chan->cl.dev, "RPC send msg timeout\n");
+		mutex_unlock(&aon_chan->transaction_lock);
+		return -ETIMEDOUT;
+	}
+
+	ret = aon_chan->ack_msg.err_code;
+
+out:
+	mutex_unlock(&aon_chan->transaction_lock);
+
+	return th1520_aon_to_linux_errno(ret);
+}
+EXPORT_SYMBOL_GPL(th1520_aon_call_rpc);
+
+/**
+ * th1520_aon_power_update() - Change power state of a resource via TH1520 AON
+ * @aon_chan: Pointer to the AON channel structure
+ * @rsrc: Resource ID whose power state needs to be updated
+ * @power_on: Boolean indicating whether the resource should be powered on (true)
+ *            or powered off (false)
+ *
+ * This function requests the TH1520 AON subsystem to set the power mode of the
+ * given resource (@rsrc) to either on or off. It constructs the message in
+ * `struct th1520_aon_msg_req_set_resource_power_mode` and then invokes
+ * th1520_aon_call_rpc() to make the request. If the AON call fails, an error
+ * message is logged along with the specific return code.
+ *
+ * Return:
+ * * 0 on success
+ * * A negative error code in case of failures (propagated from
+ *   th1520_aon_call_rpc()).
+ */
+int th1520_aon_power_update(struct th1520_aon_chan *aon_chan, u16 rsrc,
+			    bool power_on)
+{
+	struct th1520_aon_msg_req_set_resource_power_mode msg = {};
+	struct th1520_aon_rpc_msg_hdr *hdr = &msg.hdr;
+	int ret;
+
+	hdr->svc = TH1520_AON_RPC_SVC_PM;
+	hdr->func = TH1520_AON_PM_FUNC_SET_RESOURCE_POWER_MODE;
+	hdr->size = TH1520_AON_RPC_MSG_NUM;
+
+	RPC_SET_BE16(&msg.resource, 0, rsrc);
+	RPC_SET_BE16(&msg.resource, 2,
+		     (power_on ? TH1520_AON_PM_PW_MODE_ON :
+				 TH1520_AON_PM_PW_MODE_OFF));
+
+	ret = th1520_aon_call_rpc(aon_chan, &msg);
+	if (ret)
+		dev_err(aon_chan->cl.dev, "failed to power %s resource %d ret %d\n",
+			power_on ? "up" : "off", rsrc, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(th1520_aon_power_update);
+
+static int th1520_aon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct th1520_aon_chan *aon_chan;
+	struct mbox_client *cl;
+	int ret;
+	struct platform_device_info pdevinfo = {
+		.name = "th1520-pd",
+		.id = PLATFORM_DEVID_AUTO,
+		.parent = dev,
+	};
+
+	aon_chan = devm_kzalloc(dev, sizeof(*aon_chan), GFP_KERNEL);
+	if (!aon_chan)
+		return -ENOMEM;
+
+	cl = &aon_chan->cl;
+	cl->dev = dev;
+	cl->tx_block = true;
+	cl->tx_tout = MAX_TX_TIMEOUT;
+	cl->rx_callback = th1520_aon_rx_callback;
+
+	aon_chan->ch = mbox_request_channel_byname(cl, "aon");
+	if (IS_ERR(aon_chan->ch))
+		return dev_err_probe(dev, PTR_ERR(aon_chan->ch),
+				     "Failed to request aon mbox chan\n");
+
+	mutex_init(&aon_chan->transaction_lock);
+	init_completion(&aon_chan->done);
+
+	platform_set_drvdata(pdev, aon_chan);
+
+	aon_chan->pd = platform_device_register_full(&pdevinfo);
+	ret = PTR_ERR_OR_ZERO(aon_chan->pd);
+	if (ret) {
+		dev_err(dev, "Failed to register child device 'th1520-pd': %d\n", ret);
+		goto free_mbox_chan;
+	}
+
+	ret = devm_of_platform_populate(dev);
+	if (ret)
+		goto unregister_pd;
+
+	return 0;
+
+unregister_pd:
+	platform_device_unregister(aon_chan->pd);
+free_mbox_chan:
+	mbox_free_channel(aon_chan->ch);
+
+	return ret;
+}
+
+static void th1520_aon_remove(struct platform_device *pdev)
+{
+	struct th1520_aon_chan *aon_chan = platform_get_drvdata(pdev);
+
+	platform_device_unregister(aon_chan->pd);
+	mbox_free_channel(aon_chan->ch);
+}
+
+static const struct of_device_id th1520_aon_match[] = {
+	{ .compatible = "thead,th1520-aon" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, th1520_aon_match);
+
+static struct platform_driver th1520_aon_driver = {
+	.driver = {
+		.name = "th1520-aon",
+		.of_match_table = th1520_aon_match,
+	},
+	.probe = th1520_aon_probe,
+	.remove = th1520_aon_remove,
+};
+module_platform_driver(th1520_aon_driver);
+
+MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
+MODULE_DESCRIPTION("T-HEAD TH1520 Always-On firmware driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/firmware/thead/thead,th1520-aon.h b/include/linux/firmware/thead/thead,th1520-aon.h
new file mode 100644
index 000000000000..c7272599ea08
--- /dev/null
+++ b/include/linux/firmware/thead/thead,th1520-aon.h
@@ -0,0 +1,197 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2021 Alibaba Group Holding Limited.
+ */
+
+#ifndef _THEAD_AON_H
+#define _THEAD_AON_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+#define AON_RPC_MSG_MAGIC (0xef)
+#define TH1520_AON_RPC_VERSION 2
+#define TH1520_AON_RPC_MSG_NUM 7
+
+extern struct th1520_aon_chan *aon_chan;
+
+enum th1520_aon_rpc_svc {
+	TH1520_AON_RPC_SVC_UNKNOWN = 0,
+	TH1520_AON_RPC_SVC_PM = 1,
+	TH1520_AON_RPC_SVC_MISC = 2,
+	TH1520_AON_RPC_SVC_AVFS = 3,
+	TH1520_AON_RPC_SVC_SYS = 4,
+	TH1520_AON_RPC_SVC_WDG = 5,
+	TH1520_AON_RPC_SVC_LPM = 6,
+	TH1520_AON_RPC_SVC_MAX = 0x3F,
+};
+
+enum th1520_aon_misc_func {
+	TH1520_AON_MISC_FUNC_UNKNOWN = 0,
+	TH1520_AON_MISC_FUNC_SET_CONTROL = 1,
+	TH1520_AON_MISC_FUNC_GET_CONTROL = 2,
+	TH1520_AON_MISC_FUNC_REGDUMP_CFG = 3,
+};
+
+enum th1520_aon_wdg_func {
+	TH1520_AON_WDG_FUNC_UNKNOWN = 0,
+	TH1520_AON_WDG_FUNC_START = 1,
+	TH1520_AON_WDG_FUNC_STOP = 2,
+	TH1520_AON_WDG_FUNC_PING = 3,
+	TH1520_AON_WDG_FUNC_TIMEOUTSET = 4,
+	TH1520_AON_WDG_FUNC_RESTART = 5,
+	TH1520_AON_WDG_FUNC_GET_STATE = 6,
+	TH1520_AON_WDG_FUNC_POWER_OFF = 7,
+	TH1520_AON_WDG_FUNC_AON_WDT_ON = 8,
+	TH1520_AON_WDG_FUNC_AON_WDT_OFF = 9,
+};
+
+enum th1520_aon_sys_func {
+	TH1520_AON_SYS_FUNC_UNKNOWN = 0,
+	TH1520_AON_SYS_FUNC_AON_RESERVE_MEM = 1,
+};
+
+enum th1520_aon_lpm_func {
+	TH1520_AON_LPM_FUNC_UNKNOWN = 0,
+	TH1520_AON_LPM_FUNC_REQUIRE_STR = 1,
+	TH1520_AON_LPM_FUNC_RESUME_STR = 2,
+	TH1520_AON_LPM_FUNC_REQUIRE_STD = 3,
+	TH1520_AON_LPM_FUNC_CPUHP = 4,
+	TH1520_AON_LPM_FUNC_REGDUMP_CFG = 5,
+};
+
+enum th1520_aon_pm_func {
+	TH1520_AON_PM_FUNC_UNKNOWN = 0,
+	TH1520_AON_PM_FUNC_SET_RESOURCE_REGULATOR = 1,
+	TH1520_AON_PM_FUNC_GET_RESOURCE_REGULATOR = 2,
+	TH1520_AON_PM_FUNC_SET_RESOURCE_POWER_MODE = 3,
+	TH1520_AON_PM_FUNC_PWR_SET = 4,
+	TH1520_AON_PM_FUNC_PWR_GET = 5,
+	TH1520_AON_PM_FUNC_CHECK_FAULT = 6,
+	TH1520_AON_PM_FUNC_GET_TEMPERATURE = 7,
+};
+
+struct th1520_aon_rpc_msg_hdr {
+	u8 ver; /* version of msg hdr */
+	u8 size; /*  msg size ,uinit in bytes,the size includes rpc msg header self */
+	u8 svc; /* rpc main service id */
+	u8 func; /* rpc sub func id of specific service, sent by caller */
+} __packed __aligned(1);
+
+struct th1520_aon_rpc_ack_common {
+	struct th1520_aon_rpc_msg_hdr hdr;
+	u8 err_code;
+} __packed __aligned(1);
+
+#define RPC_SVC_MSG_TYPE_DATA 0
+#define RPC_SVC_MSG_TYPE_ACK 1
+#define RPC_SVC_MSG_NEED_ACK 0
+#define RPC_SVC_MSG_NO_NEED_ACK 1
+
+#define RPC_GET_VER(MESG) ((MESG)->ver)
+#define RPC_SET_VER(MESG, VER) ((MESG)->ver = (VER))
+#define RPC_GET_SVC_ID(MESG) ((MESG)->svc & 0x3F)
+#define RPC_SET_SVC_ID(MESG, ID) ((MESG)->svc |= 0x3F & (ID))
+#define RPC_GET_SVC_FLAG_MSG_TYPE(MESG) (((MESG)->svc & 0x80) >> 7)
+#define RPC_SET_SVC_FLAG_MSG_TYPE(MESG, TYPE) ((MESG)->svc |= (TYPE) << 7)
+#define RPC_GET_SVC_FLAG_ACK_TYPE(MESG) (((MESG)->svc & 0x40) >> 6)
+#define RPC_SET_SVC_FLAG_ACK_TYPE(MESG, ACK) ((MESG)->svc |= (ACK) << 6)
+
+#define RPC_SET_BE64(MESG, OFFSET, SET_DATA)                                \
+	do {                                                                \
+		u8 *data = (u8 *)(MESG);                                    \
+		u64 _offset = (OFFSET);                                     \
+		u64 _set_data = (SET_DATA);                                 \
+		data[_offset + 7] = _set_data & 0xFF;                       \
+		data[_offset + 6] = (_set_data & 0xFF00) >> 8;              \
+		data[_offset + 5] = (_set_data & 0xFF0000) >> 16;           \
+		data[_offset + 4] = (_set_data & 0xFF000000) >> 24;         \
+		data[_offset + 3] = (_set_data & 0xFF00000000) >> 32;       \
+		data[_offset + 2] = (_set_data & 0xFF0000000000) >> 40;     \
+		data[_offset + 1] = (_set_data & 0xFF000000000000) >> 48;   \
+		data[_offset + 0] = (_set_data & 0xFF00000000000000) >> 56; \
+	} while (0)
+
+#define RPC_SET_BE32(MESG, OFFSET, SET_DATA)			    \
+	do {							    \
+		u8 *data = (u8 *)(MESG);			    \
+		u64 _offset = (OFFSET);				    \
+		u64 _set_data = (SET_DATA);			    \
+		data[_offset + 3] = (_set_data) & 0xFF;		    \
+		data[_offset + 2] = (_set_data & 0xFF00) >> 8;	    \
+		data[_offset + 1] = (_set_data & 0xFF0000) >> 16;   \
+		data[_offset + 0] = (_set_data & 0xFF000000) >> 24; \
+	} while (0)
+
+#define RPC_SET_BE16(MESG, OFFSET, SET_DATA)		       \
+	do {						       \
+		u8 *data = (u8 *)(MESG);		       \
+		u64 _offset = (OFFSET);			       \
+		u64 _set_data = (SET_DATA);		       \
+		data[_offset + 1] = (_set_data) & 0xFF;	       \
+		data[_offset + 0] = (_set_data & 0xFF00) >> 8; \
+	} while (0)
+
+#define RPC_SET_U8(MESG, OFFSET, SET_DATA)	  \
+	do {					  \
+		u8 *data = (u8 *)(MESG);	  \
+		data[OFFSET] = (SET_DATA) & 0xFF; \
+	} while (0)
+
+#define RPC_GET_BE64(MESG, OFFSET, PTR)                                      \
+	do {                                                                 \
+		u8 *data = (u8 *)(MESG);                                     \
+		u64 _offset = (OFFSET);                                      \
+		*(u32 *)(PTR) =                                              \
+			(data[_offset + 7] | data[_offset + 6] << 8 |        \
+			 data[_offset + 5] << 16 | data[_offset + 4] << 24 | \
+			 data[_offset + 3] << 32 | data[_offset + 2] << 40 | \
+			 data[_offset + 1] << 48 | data[_offset + 0] << 56); \
+	} while (0)
+
+#define RPC_GET_BE32(MESG, OFFSET, PTR)                                      \
+	do {                                                                 \
+		u8 *data = (u8 *)(MESG);                                     \
+		u64 _offset = (OFFSET);                                      \
+		*(u32 *)(PTR) =                                              \
+			(data[_offset + 3] | data[_offset + 2] << 8 |        \
+			 data[_offset + 1] << 16 | data[_offset + 0] << 24); \
+	} while (0)
+
+#define RPC_GET_BE16(MESG, OFFSET, PTR)                                       \
+	do {                                                                  \
+		u8 *data = (u8 *)(MESG);                                      \
+		u64 _offset = (OFFSET);                                       \
+		*(u16 *)(PTR) = (data[_offset + 1] | data[_offset + 0] << 8); \
+	} while (0)
+
+#define RPC_GET_U8(MESG, OFFSET, PTR)          \
+	do {                                   \
+		u8 *data = (u8 *)(MESG);       \
+		*(u8 *)(PTR) = (data[OFFSET]); \
+	} while (0)
+
+/*
+ * Defines for SC PM Power Mode
+ */
+#define TH1520_AON_PM_PW_MODE_OFF 0 /* Power off */
+#define TH1520_AON_PM_PW_MODE_STBY 1 /* Power in standby */
+#define TH1520_AON_PM_PW_MODE_LP 2 /* Power in low-power */
+#define TH1520_AON_PM_PW_MODE_ON 3 /* Power on */
+
+/*
+ * Defines for AON power islands
+ */
+#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
+
+int th1520_aon_call_rpc(struct th1520_aon_chan *aon_chan, void *msg);
+int th1520_aon_power_update(struct th1520_aon_chan *aon_chan, u16 rsrc,
+			    bool power_on);
+
+#endif /* _THEAD_AON_H */
-- 
2.34.1

Re: [PATCH v4 04/18] firmware: thead: Add AON firmware protocol driver

[Ulf Hansson]

On Tue, 28 Jan 2025 at 20:48, Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> The T-Head TH1520 SoC uses an E902 co-processor running Always-On (AON)
> firmware to manage power, clock, and other system resources [1]. This
> patch introduces a driver implementing the AON firmware protocol,
> allowing the Linux kernel to communicate with the firmware via mailbox
> channels.  Through an RPC-based interface, the kernel can initiate power
> state transitions, update resource configurations, and perform other
> AON-related tasks.
>
> 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>
> ---

[...]

> +
> +static int th1520_aon_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct th1520_aon_chan *aon_chan;
> +       struct mbox_client *cl;
> +       int ret;
> +       struct platform_device_info pdevinfo = {
> +               .name = "th1520-pd",
> +               .id = PLATFORM_DEVID_AUTO,
> +               .parent = dev,
> +       };
> +
> +       aon_chan = devm_kzalloc(dev, sizeof(*aon_chan), GFP_KERNEL);
> +       if (!aon_chan)
> +               return -ENOMEM;
> +
> +       cl = &aon_chan->cl;
> +       cl->dev = dev;
> +       cl->tx_block = true;
> +       cl->tx_tout = MAX_TX_TIMEOUT;
> +       cl->rx_callback = th1520_aon_rx_callback;
> +
> +       aon_chan->ch = mbox_request_channel_byname(cl, "aon");
> +       if (IS_ERR(aon_chan->ch))
> +               return dev_err_probe(dev, PTR_ERR(aon_chan->ch),
> +                                    "Failed to request aon mbox chan\n");
> +
> +       mutex_init(&aon_chan->transaction_lock);
> +       init_completion(&aon_chan->done);
> +
> +       platform_set_drvdata(pdev, aon_chan);
> +
> +       aon_chan->pd = platform_device_register_full(&pdevinfo);
> +       ret = PTR_ERR_OR_ZERO(aon_chan->pd);
> +       if (ret) {
> +               dev_err(dev, "Failed to register child device 'th1520-pd': %d\n", ret);
> +               goto free_mbox_chan;
> +       }
> +
> +       ret = devm_of_platform_populate(dev);
> +       if (ret)
> +               goto unregister_pd;
> +
> +       return 0;
> +
> +unregister_pd:
> +       platform_device_unregister(aon_chan->pd);
> +free_mbox_chan:
> +       mbox_free_channel(aon_chan->ch);
> +
> +       return ret;
> +}

Rather than implementing this as a driver, I suggest limiting this to
a set of exported library functions.

In this way, you don't need to register a platform device, but can
instead let the power-domain provider driver in patch6, to be the one
that matches on the "thead,th1520-aon" compatible to probe.

> +
> +static void th1520_aon_remove(struct platform_device *pdev)
> +{
> +       struct th1520_aon_chan *aon_chan = platform_get_drvdata(pdev);
> +
> +       platform_device_unregister(aon_chan->pd);
> +       mbox_free_channel(aon_chan->ch);
> +}
> +
> +static const struct of_device_id th1520_aon_match[] = {
> +       { .compatible = "thead,th1520-aon" },
> +       { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, th1520_aon_match);
> +
> +static struct platform_driver th1520_aon_driver = {
> +       .driver = {
> +               .name = "th1520-aon",
> +               .of_match_table = th1520_aon_match,
> +       },
> +       .probe = th1520_aon_probe,
> +       .remove = th1520_aon_remove,
> +};
> +module_platform_driver(th1520_aon_driver);
> +
> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
> +MODULE_DESCRIPTION("T-HEAD TH1520 Always-On firmware driver");
> +MODULE_LICENSE("GPL");

[...]

Kind regards
Uffe

[PATCH v4 05/18] dt-bindings: power: Add TH1520 SoC power domains

[Michal Wilczynski]

Add power domain ID's for the TH1520 SoC power domains.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS                                   |  1 +
 .../dt-bindings/power/thead,th1520-power.h    | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 include/dt-bindings/power/thead,th1520-power.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9a98b52fffdc..4106b0e26589 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20351,6 +20351,7 @@ 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/power/thead,th1520-power.h
 F:	include/linux/firmware/thead/thead,th1520-aon.h
 
 RNBD BLOCK DRIVERS
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..8395bd1459f3
--- /dev/null
+++ b/include/dt-bindings/power/thead,th1520-power.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * 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_AUDIO_PD		0
+#define TH1520_VDEC_PD		1
+#define TH1520_NPU_PD		2
+#define TH1520_VENC_PD		3
+#define TH1520_GPU_PD		4
+#define TH1520_DSP0_PD		5
+#define TH1520_DSP1_PD		6
+
+#endif
-- 
2.34.1

Re: [PATCH v4 05/18] dt-bindings: power: Add TH1520 SoC power domains

[Krzysztof Kozlowski]

On Tue, Jan 28, 2025 at 08:48:03PM +0100, Michal Wilczynski wrote:
> Add power domain ID's for the TH1520 SoC power domains.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS                                   |  1 +
>  .../dt-bindings/power/thead,th1520-power.h    | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
>  create mode 100644 include/dt-bindings/power/thead,th1520-power.h

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v4 06/18] pmdomain: thead: Add 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");

Introduce a power-domain driver for the TH1520 SoC, which is using AON
firmware protocol to communicate with E902 core through the hardware
mailbox. This way it can send power on/off commands to the E902 core.

The interaction with AUDIO power island e.g trying to turn it OFF proved
to crash the firmware running on the E902 core. Introduce the workaround
to disable interacting with the power island.

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                                |   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 | 193 +++++++++++++++++++++
 6 files changed, 210 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

diff --git a/MAINTAINERS b/MAINTAINERS
index 4106b0e26589..33c804f1a60f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20350,6 +20350,7 @@ F:	drivers/firmware/thead,th1520-aon.c
 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/
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 F:	include/dt-bindings/power/thead,th1520-power.h
 F:	include/linux/firmware/thead/thead,th1520-aon.h
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..c7a1ac0c61dc
--- /dev/null
+++ b/drivers/pmdomain/thead/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config TH1520_PM_DOMAINS
+	tristate "Support TH1520 Power Domains"
+	depends on TH1520_AON_PROTOCOL || !TH1520_AON_PROTOCOL
+	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, NPU,
+	  and 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..7cf4ea519623
--- /dev/null
+++ b/drivers/pmdomain/thead/th1520-pm-domains.c
@@ -0,0 +1,193 @@
+// 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/firmware/thead/thead,th1520-aon.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+
+#include <dt-bindings/power/thead,th1520-power.h>
+
+struct th1520_power_domain {
+	struct th1520_aon_chan *aon_chan;
+	struct generic_pm_domain genpd;
+	u32 rsrc;
+};
+
+struct th1520_power_info {
+	const char *name;
+	u32 rsrc;
+	bool disabled;
+};
+
+/*
+ * The AUDIO power domain is marked as disabled to prevent the driver from
+ * managing its power state. Direct AON firmware calls to control this power
+ * island trigger a firmware bug causing system instability. Until this
+ * firmware issue is resolved, the AUDIO power domain must remain disabled
+ * to avoid crashes.
+ */
+static const struct th1520_power_info th1520_pd_ranges[] = {
+	[TH1520_AUDIO_PD] = {"audio", TH1520_AON_AUDIO_PD, true },
+	[TH1520_VDEC_PD] = { "vdec", TH1520_AON_VDEC_PD, false },
+	[TH1520_NPU_PD] = { "npu", TH1520_AON_NPU_PD, false },
+	[TH1520_VENC_PD] = { "venc", TH1520_AON_VENC_PD, false },
+	[TH1520_GPU_PD] = { "gpu", TH1520_AON_GPU_PD, false },
+	[TH1520_DSP0_PD] = { "dsp0", TH1520_AON_DSP0_PD, false },
+	[TH1520_DSP1_PD] = { "dsp1", TH1520_AON_DSP1_PD, false }
+};
+
+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 int th1520_pd_power_on(struct generic_pm_domain *domain)
+{
+	struct th1520_power_domain *pd = to_th1520_power_domain(domain);
+
+	return th1520_aon_power_update(pd->aon_chan, pd->rsrc, true);
+}
+
+static int th1520_pd_power_off(struct generic_pm_domain *domain)
+{
+	struct th1520_power_domain *pd = to_th1520_power_domain(domain);
+
+	return th1520_aon_power_update(pd->aon_chan, pd->rsrc, false);
+}
+
+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;
+
+		if (th1520_pd_ranges[i].disabled)
+			continue;
+
+		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)
+		return ERR_PTR(ret);
+
+	return pd;
+}
+
+static void th1520_pd_init_all_off(struct generic_pm_domain **domains,
+				   struct device *dev)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
+		struct th1520_power_domain *pd;
+
+		if (th1520_pd_ranges[i].disabled)
+			continue;
+
+		pd = to_th1520_power_domain(domains[i]);
+
+		ret = th1520_aon_power_update(pd->aon_chan, pd->rsrc, false);
+		if (ret)
+			dev_err(dev,
+				"Failed to initially power down power domain %s\n",
+				pd->genpd.name);
+	}
+}
+
+static int th1520_pd_probe(struct platform_device *pdev)
+{
+	struct generic_pm_domain **domains;
+	struct genpd_onecell_data *pd_data;
+	struct th1520_aon_chan *aon_chan;
+	struct device *dev = &pdev->dev;
+	int i;
+
+	aon_chan = dev_get_drvdata(dev->parent);
+	if (!aon_chan) {
+		dev_err(dev, "Failed to get AON channel from parent\n");
+		return -EINVAL;
+	}
+
+	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;
+
+		if (th1520_pd_ranges[i].disabled)
+			continue;
+
+		pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
+		if (IS_ERR(pd))
+			return PTR_ERR(pd);
+
+		pd->aon_chan = aon_chan;
+		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;
+
+	/*
+	 * Initialize all power domains to off to ensure they start in a
+	 * low-power state. This allows device drivers to manage power
+	 * domains by turning them on or off as needed.
+	 */
+	th1520_pd_init_all_off(domains, dev);
+
+	return of_genpd_add_provider_onecell(dev->parent->of_node, pd_data);
+}
+
+static struct platform_driver th1520_pd_driver = {
+	.driver = {
+		.name = "th1520-pd",
+	},
+	.probe = th1520_pd_probe,
+};
+module_platform_driver(th1520_pd_driver);
+
+MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
+MODULE_DESCRIPTION("T-HEAD TH1520 SoC power domain controller");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v4 06/18] pmdomain: thead: Add power-domain driver for TH1520

[Ulf Hansson]

On Tue, 28 Jan 2025 at 20:48, Michal Wilczynski
<m.wilczynski@samsung.com> 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");
>
> Introduce a power-domain driver for the TH1520 SoC, which is using AON
> firmware protocol to communicate with E902 core through the hardware
> mailbox. This way it can send power on/off commands to the E902 core.
>
> The interaction with AUDIO power island e.g trying to turn it OFF proved
> to crash the firmware running on the E902 core. Introduce the workaround
> to disable interacting with the power island.
>
> 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>
> ---

[...]

> +
> +static int th1520_pd_probe(struct platform_device *pdev)
> +{
> +       struct generic_pm_domain **domains;
> +       struct genpd_onecell_data *pd_data;
> +       struct th1520_aon_chan *aon_chan;
> +       struct device *dev = &pdev->dev;
> +       int i;
> +
> +       aon_chan = dev_get_drvdata(dev->parent);
> +       if (!aon_chan) {
> +               dev_err(dev, "Failed to get AON channel from parent\n");
> +               return -EINVAL;
> +       }

As pointed out on patch4. Rather than receiving the aon_chang from the
parent device like this, it seems better to receive it from a call to
a library function provided by the FW library.

> +
> +       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;
> +
> +               if (th1520_pd_ranges[i].disabled)
> +                       continue;
> +
> +               pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
> +               if (IS_ERR(pd))
> +                       return PTR_ERR(pd);
> +
> +               pd->aon_chan = aon_chan;
> +               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;
> +
> +       /*
> +        * Initialize all power domains to off to ensure they start in a
> +        * low-power state. This allows device drivers to manage power
> +        * domains by turning them on or off as needed.
> +        */
> +       th1520_pd_init_all_off(domains, dev);
> +
> +       return of_genpd_add_provider_onecell(dev->parent->of_node, pd_data);
> +}
> +
> +static struct platform_driver th1520_pd_driver = {
> +       .driver = {
> +               .name = "th1520-pd",
> +       },
> +       .probe = th1520_pd_probe,
> +};
> +module_platform_driver(th1520_pd_driver);

There is no ->remove() callback.

Either add one or make this a builtin_platform_driver() with
"suppress_bind_attrs = true".

> +
> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
> +MODULE_DESCRIPTION("T-HEAD TH1520 SoC power domain controller");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>

Besides the minor thing above, this looks good to me!

Kind regards
Uffe

[PATCH v4 07/18] 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 1916cf7ba450..83833ded8908 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -53,6 +53,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

[PATCH v4 08/18] dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller

[Michal Wilczynski]

Add a YAML schema for the T-HEAD TH1520 SoC reset controller. This
controller manages resets for subsystems such as the GPU within the
TH1520 SoC.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/reset/thead,th1520-reset.yaml    | 44 +++++++++++++++++++
 MAINTAINERS                                   |  2 +
 .../dt-bindings/reset/thead,th1520-reset.h    | 15 +++++++
 3 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
 create mode 100644 include/dt-bindings/reset/thead,th1520-reset.h

diff --git a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
new file mode 100644
index 000000000000..f2e91d0add7a
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/thead,th1520-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 SoC Reset Controller
+
+description:
+  The T-HEAD TH1520 reset controller is a hardware block that asserts/deasserts
+  resets for SoC subsystems.
+
+maintainers:
+  - Michal Wilczynski <m.wilczynski@samsung.com>
+
+properties:
+  compatible:
+    enum:
+      - thead,th1520-reset
+
+  reg:
+    maxItems: 1
+
+  "#reset-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      rst: reset-controller@ffef528000 {
+        compatible = "thead,th1520-reset";
+        reg = <0xff 0xef528000 0x0 0x1000>;
+        #reset-cells = <1>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 33c804f1a60f..b4e21d814481 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20344,6 +20344,7 @@ F:	Documentation/devicetree/bindings/firmware/thead,th1520-aon.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/reset/thead,th1520-reset.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c
 F:	drivers/firmware/thead,th1520-aon.c
@@ -20353,6 +20354,7 @@ F:	drivers/pinctrl/pinctrl-th1520.c
 F:	drivers/pmdomain/thead/
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 F:	include/dt-bindings/power/thead,th1520-power.h
+F:	include/dt-bindings/reset/thead,th1520-reset.h
 F:	include/linux/firmware/thead/thead,th1520-aon.h
 
 RNBD BLOCK DRIVERS
diff --git a/include/dt-bindings/reset/thead,th1520-reset.h b/include/dt-bindings/reset/thead,th1520-reset.h
new file mode 100644
index 000000000000..6bc445c5e39c
--- /dev/null
+++ b/include/dt-bindings/reset/thead,th1520-reset.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#ifndef _DT_BINDINGS_TH1520_RESET_H
+#define _DT_BINDINGS_TH1520_RESET_H
+
+#define TH1520_RESET_ID_GPU	0
+#define TH1520_RESET_ID_NPU	1
+#define TH1520_RESET_ID_WDT0	2
+#define TH1520_RESET_ID_WDT1	3
+
+#endif /* _DT_BINDINGS_TH1520_RESET_H */
-- 
2.34.1

Re: [PATCH v4 08/18] dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller

[Krzysztof Kozlowski]

On Tue, Jan 28, 2025 at 08:48:06PM +0100, Michal Wilczynski wrote:
> Add a YAML schema for the T-HEAD TH1520 SoC reset controller. This
> controller manages resets for subsystems such as the GPU within the
> TH1520 SoC.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/reset/thead,th1520-reset.yaml    | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  2 +
>  .../dt-bindings/reset/thead,th1520-reset.h    | 15 +++++++
>  3 files changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
>  create mode 100644 include/dt-bindings/reset/thead,th1520-reset.h

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Michal Wilczynski]

Add reset controller driver for the T-HEAD TH1520 SoC that manages
hardware reset lines for various subsystems. The driver currently
implements support for GPU reset control, with infrastructure in place
to extend support for NPU and Watchdog Timer resets in future updates.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS                  |   1 +
 drivers/reset/Kconfig        |  10 ++
 drivers/reset/Makefile       |   1 +
 drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
 4 files changed, 190 insertions(+)
 create mode 100644 drivers/reset/reset-th1520.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b4e21d814481..d71b8c68ae48 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20352,6 +20352,7 @@ 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/
+F:	drivers/reset/reset-th1520.c
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 F:	include/dt-bindings/power/thead,th1520-power.h
 F:	include/dt-bindings/reset/thead,th1520-reset.h
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 5b3abb6db248..fa0943c3d1de 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -272,6 +272,16 @@ config RESET_SUNXI
 	help
 	  This enables the reset driver for Allwinner SoCs.
 
+config RESET_TH1520
+	tristate "T-HEAD 1520 reset controller"
+	depends on ARCH_THEAD || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	  This driver provides support for the T-HEAD TH1520 SoC reset controller,
+	  which manages hardware reset lines for SoC components such as the GPU.
+	  Enable this option if you need to control hardware resets on TH1520-based
+	  systems.
+
 config RESET_TI_SCI
 	tristate "TI System Control Interface (TI-SCI) reset driver"
 	depends on TI_SCI_PROTOCOL || (COMPILE_TEST && TI_SCI_PROTOCOL=n)
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 677c4d1e2632..d6c2774407ae 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
 obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
+obj-$(CONFIG_RESET_TH1520) += reset-th1520.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
 obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_TI_TPS380X) += reset-tps380x.o
diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
new file mode 100644
index 000000000000..48afbc9f1cdd
--- /dev/null
+++ b/drivers/reset/reset-th1520.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/reset/thead,th1520-reset.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_reset_priv {
+	struct reset_controller_dev rcdev;
+	struct regmap *map;
+	struct mutex gpu_seq_lock;  /* protects gpu assert/deassert sequence */
+};
+
+static inline struct th1520_reset_priv *
+to_th1520_reset(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct th1520_reset_priv, rcdev);
+}
+
+static void th1520_rst_gpu_enable(struct regmap *reg,
+				  struct mutex *gpu_seq_lock)
+{
+	int val;
+
+	mutex_lock(gpu_seq_lock);
+
+	/* 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);
+
+	/*
+	 * According to the hardware manual, a delay of at least 32 clock
+	 * cycles is required between de-asserting the clkgen reset and
+	 * de-asserting the GPU reset. Assuming a worst-case scenario with
+	 * a very high GPU clock frequency, a delay of 1 microsecond is
+	 * sufficient to ensure this requirement is met across all
+	 * feasible GPU clock speeds.
+	 */
+	udelay(1);
+
+	/* rst gpu */
+	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);
+
+	mutex_unlock(gpu_seq_lock);
+}
+
+static void th1520_rst_gpu_disable(struct regmap *reg,
+				   struct mutex *gpu_seq_lock)
+{
+	mutex_lock(gpu_seq_lock);
+
+	regmap_update_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_RST_CFG_MASK, 0x0);
+
+	mutex_unlock(gpu_seq_lock);
+}
+
+static int th1520_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
+
+	switch (id) {
+	case TH1520_RESET_ID_GPU:
+		th1520_rst_gpu_disable(priv->map, &priv->gpu_seq_lock);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int th1520_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
+
+	switch (id) {
+	case TH1520_RESET_ID_GPU:
+		th1520_rst_gpu_enable(priv->map, &priv->gpu_seq_lock);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int th1520_reset_xlate(struct reset_controller_dev *rcdev,
+			      const struct of_phandle_args *reset_spec)
+{
+	unsigned int index = reset_spec->args[0];
+
+	/* currently, only GPU reset is implemented in this driver */
+	if (index == TH1520_RESET_ID_GPU)
+		return index;
+
+	return -EOPNOTSUPP;
+}
+
+static const struct reset_control_ops th1520_reset_ops = {
+	.assert	= th1520_reset_assert,
+	.deassert = th1520_reset_deassert,
+};
+
+static const struct regmap_config th1520_reset_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static int th1520_reset_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct th1520_reset_priv *priv;
+	void __iomem *base;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->map = devm_regmap_init_mmio(dev, base,
+					  &th1520_reset_regmap_config);
+	if (IS_ERR(priv->map))
+		return PTR_ERR(priv->map);
+
+	mutex_init(&priv->gpu_seq_lock);
+
+	priv->rcdev.owner = THIS_MODULE;
+	priv->rcdev.nr_resets = 1;
+	priv->rcdev.ops = &th1520_reset_ops;
+	priv->rcdev.of_node = dev->of_node;
+	priv->rcdev.of_xlate = th1520_reset_xlate;
+	priv->rcdev.of_reset_n_cells = 1;
+
+	return devm_reset_controller_register(dev, &priv->rcdev);
+}
+
+static const struct of_device_id th1520_reset_match[] = {
+	{ .compatible = "thead,th1520-reset" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, th1520_reset_match);
+
+static struct platform_driver th1520_reset_driver = {
+	.driver = {
+		.name = "th1520-reset",
+		.of_match_table = th1520_reset_match,
+	},
+	.probe = th1520_reset_probe,
+};
+module_platform_driver(th1520_reset_driver);
+
+MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
+MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Philipp Zabel]

On Di, 2025-01-28 at 20:48 +0100, Michal Wilczynski wrote:
> Add reset controller driver for the T-HEAD TH1520 SoC that manages
> hardware reset lines for various subsystems. The driver currently
> implements support for GPU reset control, with infrastructure in place
> to extend support for NPU and Watchdog Timer resets in future updates.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS | 1 +
>  drivers/reset/Kconfig | 10 ++
>  drivers/reset/Makefile | 1 +
>  drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
>  4 files changed, 190 insertions(+)
>  create mode 100644 drivers/reset/reset-th1520.c
> 
[...]
> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> new file mode 100644
> index 000000000000..48afbc9f1cdd
> --- /dev/null
> +++ b/drivers/reset/reset-th1520.c
> @@ -0,0 +1,178 @@
[...]
> +static int th1520_reset_xlate(struct reset_controller_dev *rcdev,
> + const struct of_phandle_args *reset_spec)
> +{
> + unsigned int index = reset_spec->args[0];
> +
> + /* currently, only GPU reset is implemented in this driver */
> + if (index == TH1520_RESET_ID_GPU)
> + return index;
> +
> + return -EOPNOTSUPP;

It is customary to return -EINVAL for unsupported resets.

Further, you don't have to implement a custom .of_xlate at all.
With nr_resets being set to 1, and because TH1520_RESET_ID_GPU happens
to be 0, the of_reset_simple_xlate() default implementation will do
exactly the same.

[...]
> +static int th1520_reset_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct th1520_reset_priv *priv;
> + void __iomem *base;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + priv->map = devm_regmap_init_mmio(dev, base,
> + &th1520_reset_regmap_config);
> + if (IS_ERR(priv->map))
> + return PTR_ERR(priv->map);
> +
> + mutex_init(&priv->gpu_seq_lock);
> +
> + priv->rcdev.owner = THIS_MODULE;
> + priv->rcdev.nr_resets = 1;
> + priv->rcdev.ops = &th1520_reset_ops;
> + priv->rcdev.of_node = dev->of_node;
>    A. > + priv->rcdev.of_xlate = th1520_reset_xlate;
> + priv->rcdev.of_reset_n_cells = 1;

You could just drop these two lines again. With that,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 9104 bytes --]

On 28/01/2025 19:48, Michal Wilczynski wrote:
> Add reset controller driver for the T-HEAD TH1520 SoC that manages
> hardware reset lines for various subsystems. The driver currently
> implements support for GPU reset control, with infrastructure in place
> to extend support for NPU and Watchdog Timer resets in future updates.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS                  |   1 +
>  drivers/reset/Kconfig        |  10 ++
>  drivers/reset/Makefile       |   1 +
>  drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
>  4 files changed, 190 insertions(+)
>  create mode 100644 drivers/reset/reset-th1520.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b4e21d814481..d71b8c68ae48 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20352,6 +20352,7 @@ 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/
> +F:	drivers/reset/reset-th1520.c
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>  F:	include/dt-bindings/power/thead,th1520-power.h
>  F:	include/dt-bindings/reset/thead,th1520-reset.h
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 5b3abb6db248..fa0943c3d1de 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -272,6 +272,16 @@ config RESET_SUNXI
>  	help
>  	  This enables the reset driver for Allwinner SoCs.
>  
> +config RESET_TH1520
> +	tristate "T-HEAD 1520 reset controller"
> +	depends on ARCH_THEAD || COMPILE_TEST
> +	select REGMAP_MMIO
> +	help
> +	  This driver provides support for the T-HEAD TH1520 SoC reset controller,
> +	  which manages hardware reset lines for SoC components such as the GPU.
> +	  Enable this option if you need to control hardware resets on TH1520-based
> +	  systems.
> +
>  config RESET_TI_SCI
>  	tristate "TI System Control Interface (TI-SCI) reset driver"
>  	depends on TI_SCI_PROTOCOL || (COMPILE_TEST && TI_SCI_PROTOCOL=n)
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 677c4d1e2632..d6c2774407ae 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> +obj-$(CONFIG_RESET_TH1520) += reset-th1520.o
>  obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
>  obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
>  obj-$(CONFIG_RESET_TI_TPS380X) += reset-tps380x.o
> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> new file mode 100644
> index 000000000000..48afbc9f1cdd
> --- /dev/null
> +++ b/drivers/reset/reset-th1520.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Samsung Electronics Co., Ltd.
> + * Author: Michal Wilczynski <m.wilczynski@samsung.com>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/reset/thead,th1520-reset.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_reset_priv {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *map;
> +	struct mutex gpu_seq_lock;  /* protects gpu assert/deassert sequence */
> +};
> +
> +static inline struct th1520_reset_priv *
> +to_th1520_reset(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct th1520_reset_priv, rcdev);
> +}
> +
> +static void th1520_rst_gpu_enable(struct regmap *reg,
> +				  struct mutex *gpu_seq_lock)
> +{
> +	int val;
> +
> +	mutex_lock(gpu_seq_lock);
> +
> +	/* 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);

Do you know what this resets? From our side, the GPU only has a single
reset line (which I assume to be GPU_RESET).

> +
> +	/*
> +	 * According to the hardware manual, a delay of at least 32 clock
> +	 * cycles is required between de-asserting the clkgen reset and
> +	 * de-asserting the GPU reset. Assuming a worst-case scenario with
> +	 * a very high GPU clock frequency, a delay of 1 microsecond is
> +	 * sufficient to ensure this requirement is met across all
> +	 * feasible GPU clock speeds.
> +	 */
> +	udelay(1);

I don't love that this procedure appears in the platform reset driver.
I appreciate it may not be clear from the SoC TRM, but this is the
standard reset procedure for all IMG Rogue GPUs. The currently
supported TI SoC handles this in silicon, when power up/down requests
are sent so we never needed to encode it in the driver before.

Strictly speaking, the 32 cycle delay is required between power and
clocks being enabled and the reset line being deasserted. If nothing
here touches power or clocks (which I don't think it should), the delay
could potentially be lifted to the GPU driver.

Is it expected that if a device exposes a reset in devicetree that it
can be cleanly reset without interaction with the device driver itself?
I.E. in this case, is it required that the reset driver alone can cleanly
reset the GPU?

Cheers,
Matt

> +
> +	/* rst gpu */
> +	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);
> +
> +	mutex_unlock(gpu_seq_lock);
> +}
> +
> +static void th1520_rst_gpu_disable(struct regmap *reg,
> +				   struct mutex *gpu_seq_lock)
> +{
> +	mutex_lock(gpu_seq_lock);
> +
> +	regmap_update_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_RST_CFG_MASK, 0x0);
> +
> +	mutex_unlock(gpu_seq_lock);
> +}
> +
> +static int th1520_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
> +
> +	switch (id) {
> +	case TH1520_RESET_ID_GPU:
> +		th1520_rst_gpu_disable(priv->map, &priv->gpu_seq_lock);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int th1520_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
> +
> +	switch (id) {
> +	case TH1520_RESET_ID_GPU:
> +		th1520_rst_gpu_enable(priv->map, &priv->gpu_seq_lock);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int th1520_reset_xlate(struct reset_controller_dev *rcdev,
> +			      const struct of_phandle_args *reset_spec)
> +{
> +	unsigned int index = reset_spec->args[0];
> +
> +	/* currently, only GPU reset is implemented in this driver */
> +	if (index == TH1520_RESET_ID_GPU)
> +		return index;
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct reset_control_ops th1520_reset_ops = {
> +	.assert	= th1520_reset_assert,
> +	.deassert = th1520_reset_deassert,
> +};
> +
> +static const struct regmap_config th1520_reset_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int th1520_reset_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct th1520_reset_priv *priv;
> +	void __iomem *base;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->map = devm_regmap_init_mmio(dev, base,
> +					  &th1520_reset_regmap_config);
> +	if (IS_ERR(priv->map))
> +		return PTR_ERR(priv->map);
> +
> +	mutex_init(&priv->gpu_seq_lock);
> +
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.nr_resets = 1;
> +	priv->rcdev.ops = &th1520_reset_ops;
> +	priv->rcdev.of_node = dev->of_node;
> +	priv->rcdev.of_xlate = th1520_reset_xlate;
> +	priv->rcdev.of_reset_n_cells = 1;
> +
> +	return devm_reset_controller_register(dev, &priv->rcdev);
> +}
> +
> +static const struct of_device_id th1520_reset_match[] = {
> +	{ .compatible = "thead,th1520-reset" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, th1520_reset_match);
> +
> +static struct platform_driver th1520_reset_driver = {
> +	.driver = {
> +		.name = "th1520-reset",
> +		.of_match_table = th1520_reset_match,
> +	},
> +	.probe = th1520_reset_probe,
> +};
> +module_platform_driver(th1520_reset_driver);
> +
> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
> +MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller");
> +MODULE_LICENSE("GPL");

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Michal Wilczynski]

On 1/31/25 16:39, Matt Coster wrote:
> On 28/01/2025 19:48, Michal Wilczynski wrote:
>> Add reset controller driver for the T-HEAD TH1520 SoC that manages
>> hardware reset lines for various subsystems. The driver currently
>> implements support for GPU reset control, with infrastructure in place
>> to extend support for NPU and Watchdog Timer resets in future updates.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  MAINTAINERS                  |   1 +
>>  drivers/reset/Kconfig        |  10 ++
>>  drivers/reset/Makefile       |   1 +
>>  drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
>>  4 files changed, 190 insertions(+)
>>  create mode 100644 drivers/reset/reset-th1520.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b4e21d814481..d71b8c68ae48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20352,6 +20352,7 @@ 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/
>> +F:	drivers/reset/reset-th1520.c
>>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>>  F:	include/dt-bindings/power/thead,th1520-power.h
>>  F:	include/dt-bindings/reset/thead,th1520-reset.h
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 5b3abb6db248..fa0943c3d1de 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -272,6 +272,16 @@ config RESET_SUNXI
>>  	help
>>  	  This enables the reset driver for Allwinner SoCs.
>>  
>> +config RESET_TH1520
>> +	tristate "T-HEAD 1520 reset controller"
>> +	depends on ARCH_THEAD || COMPILE_TEST
>> +	select REGMAP_MMIO
>> +	help
>> +	  This driver provides support for the T-HEAD TH1520 SoC reset controller,
>> +	  which manages hardware reset lines for SoC components such as the GPU.
>> +	  Enable this option if you need to control hardware resets on TH1520-based
>> +	  systems.
>> +
>>  config RESET_TI_SCI
>>  	tristate "TI System Control Interface (TI-SCI) reset driver"
>>  	depends on TI_SCI_PROTOCOL || (COMPILE_TEST && TI_SCI_PROTOCOL=n)
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 677c4d1e2632..d6c2774407ae 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
>>  obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
>>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
>> +obj-$(CONFIG_RESET_TH1520) += reset-th1520.o
>>  obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
>>  obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
>>  obj-$(CONFIG_RESET_TI_TPS380X) += reset-tps380x.o
>> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
>> new file mode 100644
>> index 000000000000..48afbc9f1cdd
>> --- /dev/null
>> +++ b/drivers/reset/reset-th1520.c
>> @@ -0,0 +1,178 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2024 Samsung Electronics Co., Ltd.
>> + * Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/reset/thead,th1520-reset.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_reset_priv {
>> +	struct reset_controller_dev rcdev;
>> +	struct regmap *map;
>> +	struct mutex gpu_seq_lock;  /* protects gpu assert/deassert sequence */
>> +};
>> +
>> +static inline struct th1520_reset_priv *
>> +to_th1520_reset(struct reset_controller_dev *rcdev)
>> +{
>> +	return container_of(rcdev, struct th1520_reset_priv, rcdev);
>> +}
>> +
>> +static void th1520_rst_gpu_enable(struct regmap *reg,
>> +				  struct mutex *gpu_seq_lock)
>> +{
>> +	int val;
>> +
>> +	mutex_lock(gpu_seq_lock);
>> +
>> +	/* 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);
> 
> Do you know what this resets? From our side, the GPU only has a single
> reset line (which I assume to be GPU_RESET).

This is clock generator reset, as described in the manual 5.4.2.6.1
GPU_RST_CFG. It does reside in the same register as the GPU reset line.

I think this is required because the MEM clock gate is somehow broken
and marked as 'reserved' in manual, so instead as a workaround, since we
can't reliably enable the 'mem' clock it's a good idea to reset the
whole CLKGEN of the GPU.

> 
>> +
>> +	/*
>> +	 * According to the hardware manual, a delay of at least 32 clock
>> +	 * cycles is required between de-asserting the clkgen reset and
>> +	 * de-asserting the GPU reset. Assuming a worst-case scenario with
>> +	 * a very high GPU clock frequency, a delay of 1 microsecond is
>> +	 * sufficient to ensure this requirement is met across all
>> +	 * feasible GPU clock speeds.
>> +	 */
>> +	udelay(1);
> 
> I don't love that this procedure appears in the platform reset driver.
> I appreciate it may not be clear from the SoC TRM, but this is the
> standard reset procedure for all IMG Rogue GPUs. The currently
> supported TI SoC handles this in silicon, when power up/down requests
> are sent so we never needed to encode it in the driver before.
> 
> Strictly speaking, the 32 cycle delay is required between power and
> clocks being enabled and the reset line being deasserted. If nothing
> here touches power or clocks (which I don't think it should), the delay
> could potentially be lifted to the GPU driver.

Yeah you're making excellent points here, I think it would be a good    
idea to place the delay in the GPU driver, since this is specific to the
whole family of the GPU's not the SoC itself.

> 
> Is it expected that if a device exposes a reset in devicetree that it
> can be cleanly reset without interaction with the device driver itself?
> I.E. in this case, is it required that the reset driver alone can cleanly
> reset the GPU?

I'm not sure what the community as a whole thinks about that, so maybe
someone else can answer this, but I would code SoC specific stuff in the
reset driver for the SoC, and the GPU specific stuff (like the delay) in
the GPU driver code. I wasn't sure whether the delay was specific to the
SoC or the GPU so I've put it here.

> 
> Cheers,
> Matt
> 
>> +
>> +	/* rst gpu */
>> +	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);
>> +
>> +	mutex_unlock(gpu_seq_lock);
>> +}
>> +
>> +static void th1520_rst_gpu_disable(struct regmap *reg,
>> +				   struct mutex *gpu_seq_lock)
>> +{
>> +	mutex_lock(gpu_seq_lock);
>> +
>> +	regmap_update_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_RST_CFG_MASK, 0x0);
>> +
>> +	mutex_unlock(gpu_seq_lock);
>> +}
>> +
>> +static int th1520_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
>> +
>> +	switch (id) {
>> +	case TH1520_RESET_ID_GPU:
>> +		th1520_rst_gpu_disable(priv->map, &priv->gpu_seq_lock);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int th1520_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	struct th1520_reset_priv *priv = to_th1520_reset(rcdev);
>> +
>> +	switch (id) {
>> +	case TH1520_RESET_ID_GPU:
>> +		th1520_rst_gpu_enable(priv->map, &priv->gpu_seq_lock);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int th1520_reset_xlate(struct reset_controller_dev *rcdev,
>> +			      const struct of_phandle_args *reset_spec)
>> +{
>> +	unsigned int index = reset_spec->args[0];
>> +
>> +	/* currently, only GPU reset is implemented in this driver */
>> +	if (index == TH1520_RESET_ID_GPU)
>> +		return index;
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct reset_control_ops th1520_reset_ops = {
>> +	.assert	= th1520_reset_assert,
>> +	.deassert = th1520_reset_deassert,
>> +};
>> +
>> +static const struct regmap_config th1520_reset_regmap_config = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = 4,
>> +	.fast_io = true,
>> +};
>> +
>> +static int th1520_reset_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct th1520_reset_priv *priv;
>> +	void __iomem *base;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	priv->map = devm_regmap_init_mmio(dev, base,
>> +					  &th1520_reset_regmap_config);
>> +	if (IS_ERR(priv->map))
>> +		return PTR_ERR(priv->map);
>> +
>> +	mutex_init(&priv->gpu_seq_lock);
>> +
>> +	priv->rcdev.owner = THIS_MODULE;
>> +	priv->rcdev.nr_resets = 1;
>> +	priv->rcdev.ops = &th1520_reset_ops;
>> +	priv->rcdev.of_node = dev->of_node;
>> +	priv->rcdev.of_xlate = th1520_reset_xlate;
>> +	priv->rcdev.of_reset_n_cells = 1;
>> +
>> +	return devm_reset_controller_register(dev, &priv->rcdev);
>> +}
>> +
>> +static const struct of_device_id th1520_reset_match[] = {
>> +	{ .compatible = "thead,th1520-reset" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, th1520_reset_match);
>> +
>> +static struct platform_driver th1520_reset_driver = {
>> +	.driver = {
>> +		.name = "th1520-reset",
>> +		.of_match_table = th1520_reset_match,
>> +	},
>> +	.probe = th1520_reset_probe,
>> +};
>> +module_platform_driver(th1520_reset_driver);
>> +
>> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
>> +MODULE_DESCRIPTION("T-HEAD TH1520 SoC reset controller");
>> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Philipp Zabel]

On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote:
> 
> On 1/31/25 16:39, Matt Coster wrote:
> > On 28/01/2025 19:48, Michal Wilczynski wrote:
> > > Add reset controller driver for the T-HEAD TH1520 SoC that manages
> > > hardware reset lines for various subsystems. The driver currently
> > > implements support for GPU reset control, with infrastructure in place
> > > to extend support for NPU and Watchdog Timer resets in future updates.
> > > 
> > > Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> > > ---
> > >  MAINTAINERS                  |   1 +
> > >  drivers/reset/Kconfig        |  10 ++
> > >  drivers/reset/Makefile       |   1 +
> > >  drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
> > >  4 files changed, 190 insertions(+)
> > >  create mode 100644 drivers/reset/reset-th1520.c
> > > 
[...]
> > > diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> > > new file mode 100644
> > > index 000000000000..48afbc9f1cdd
> > > --- /dev/null
> > > +++ b/drivers/reset/reset-th1520.c
> > > @@ -0,0 +1,178 @@
[...]
> > > +static void th1520_rst_gpu_enable(struct regmap *reg,
> > > +				  struct mutex *gpu_seq_lock)
> > > +{
> > > +	int val;
> > > +
> > > +	mutex_lock(gpu_seq_lock);
> > > +
> > > +	/* 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);

BIT(2) is not documented, but cleared here.

> > > +
> > > +	/* rst gpu clkgen */
> > > +	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_CLKGEN_RST);
> > 
> > Do you know what this resets? From our side, the GPU only has a single
> > reset line (which I assume to be GPU_RESET).
> 
> This is clock generator reset, as described in the manual 5.4.2.6.1
> GPU_RST_CFG. It does reside in the same register as the GPU reset line.
> 
> I think this is required because the MEM clock gate is somehow broken
> and marked as 'reserved' in manual, so instead as a workaround, since we
> can't reliably enable the 'mem' clock it's a good idea to reset the
> whole CLKGEN of the GPU.

If this is a workaround for broken gating of the "mem" clock, would it
be possible (and reasonable) to make this a separate reset control that
is handled by the clock driver? ...

> > > +
> > > +	/*
> > > +	 * According to the hardware manual, a delay of at least 32 clock
> > > +	 * cycles is required between de-asserting the clkgen reset and
> > > +	 * de-asserting the GPU reset. Assuming a worst-case scenario with
> > > +	 * a very high GPU clock frequency, a delay of 1 microsecond is
> > > +	 * sufficient to ensure this requirement is met across all
> > > +	 * feasible GPU clock speeds.
> > > +	 */
> > > +	udelay(1);
> > 
> > I don't love that this procedure appears in the platform reset driver.
> > I appreciate it may not be clear from the SoC TRM, but this is the
> > standard reset procedure for all IMG Rogue GPUs. The currently
> > supported TI SoC handles this in silicon, when power up/down requests
> > are sent so we never needed to encode it in the driver before.
> > 
> > Strictly speaking, the 32 cycle delay is required between power and
> > clocks being enabled and the reset line being deasserted. If nothing
> > here touches power or clocks (which I don't think it should), the delay
> > could potentially be lifted to the GPU driver.

... This could be expressed as a delay between clk_prepare_enable() and
reset_control_deassert() in the GPU driver then.

> Yeah you're making excellent points here, I think it would be a good    
> idea to place the delay in the GPU driver, since this is specific to the
> whole family of the GPU's not the SoC itself.
>
> > Is it expected that if a device exposes a reset in devicetree that it
> > can be cleanly reset without interaction with the device driver itself?
> > I.E. in this case, is it required that the reset driver alone can cleanly
> > reset the GPU?

No, the "resets" property should just describe the physical
connection(s) between reset controller and the device.

It is fine for the device driver to manually assert the reset, enable
clocks and power, delay, and then deassert the reset, if that is the
device specific reset procedure.

> I'm not sure what the community as a whole thinks about that, so maybe
> someone else can answer this, but I would code SoC specific stuff in the
> reset driver for the SoC, and the GPU specific stuff (like the delay) in
> the GPU driver code. I wasn't sure whether the delay was specific to the
> SoC or the GPU so I've put it here.

I agree.

regards
Philipp

Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Michal Wilczynski]

On 2/4/25 18:18, Philipp Zabel wrote:
> On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote:
>>
>> On 1/31/25 16:39, Matt Coster wrote:
>>> On 28/01/2025 19:48, Michal Wilczynski wrote:
>>>> Add reset controller driver for the T-HEAD TH1520 SoC that manages
>>>> hardware reset lines for various subsystems. The driver currently
>>>> implements support for GPU reset control, with infrastructure in place
>>>> to extend support for NPU and Watchdog Timer resets in future updates.
>>>>
>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>>> ---
>>>>  MAINTAINERS                  |   1 +
>>>>  drivers/reset/Kconfig        |  10 ++
>>>>  drivers/reset/Makefile       |   1 +
>>>>  drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
>>>>  4 files changed, 190 insertions(+)
>>>>  create mode 100644 drivers/reset/reset-th1520.c
>>>>
> [...]
>>>> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
>>>> new file mode 100644
>>>> index 000000000000..48afbc9f1cdd
>>>> --- /dev/null
>>>> +++ b/drivers/reset/reset-th1520.c
>>>> @@ -0,0 +1,178 @@
> [...]
>>>> +static void th1520_rst_gpu_enable(struct regmap *reg,
>>>> +				  struct mutex *gpu_seq_lock)
>>>> +{
>>>> +	int val;
>>>> +
>>>> +	mutex_lock(gpu_seq_lock);
>>>> +
>>>> +	/* 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);
> 
> BIT(2) is not documented, but cleared here.

Yeah shouldn't be cleared, thanks !

> 
>>>> +
>>>> +	/* rst gpu clkgen */
>>>> +	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_CLKGEN_RST);
>>>
>>> Do you know what this resets? From our side, the GPU only has a single
>>> reset line (which I assume to be GPU_RESET).
>>
>> This is clock generator reset, as described in the manual 5.4.2.6.1
>> GPU_RST_CFG. It does reside in the same register as the GPU reset line.
>>
>> I think this is required because the MEM clock gate is somehow broken
>> and marked as 'reserved' in manual, so instead as a workaround, since we
>> can't reliably enable the 'mem' clock it's a good idea to reset the
>> whole CLKGEN of the GPU.
> 
> If this is a workaround for broken gating of the "mem" clock, would it
> be possible (and reasonable) to make this a separate reset control that
> is handled by the clock driver? ...

Thank you for the detailed feedback, Philipp.

After further consideration, I believe keeping the current reset driver
implementation would be preferable to moving the CLKGEN reset handling
to the clock driver. While it's technically possible to implement this
in the clock driver, I have concerns about the added complexity:

1. We'd need to expose the CLKGEN reset separately in the reset driver
2. The clock driver's dt-bindings would need modification to add an
   optional resets property
3. We'd need custom clk_ops for all three clock gates (including a dummy
   'mem' gate)
4. Each clock gate's .enable operation would need to handle CLKGEN reset
   deassertion

While the clock framework could theoretically handle this, there's no
clean way to express the requirement that the CLKGEN reset should only
be deasserted after all clocks in the group are enabled. We could
implement this explicitly, but it would make the code more complex and
harder to understand.

The current solution in the reset driver is simpler and clearer - it
treats this as what it really is: a TH1520-specific reset sequence.
Looking at other similar SoCs like the BPI-F3, we can see this is truly
THEAD-specific - the BPI-F3 has just a single reset line with no CLKGEN
bit to manage. When you assert/deassert the GPU reset line on the
TH1520, it handles everything needed for a clean reset on this specific
SoC. This keeps the implementation contained and straightforward.

Regarding the delay between clock enable and reset deassert - for SoCs
like BPI-F3 with a single reset line, implementing this in the GPU
consumer driver makes perfect sense. However, for the T-HEAD SoC, moving
the delay there would actually complicate things since we need to manage
both the CLKGEN and GPU reset lines in a specific sequence. Having this
handled entirely within the reset driver keeps the implementation
cleaner.

Does this reasoning align with your thoughts? I'm happy to explore the
clock driver approach further if you still see significant advantages to
that solution.

> 
>>>> +
>>>> +	/*
>>>> +	 * According to the hardware manual, a delay of at least 32 clock
>>>> +	 * cycles is required between de-asserting the clkgen reset and
>>>> +	 * de-asserting the GPU reset. Assuming a worst-case scenario with
>>>> +	 * a very high GPU clock frequency, a delay of 1 microsecond is
>>>> +	 * sufficient to ensure this requirement is met across all
>>>> +	 * feasible GPU clock speeds.
>>>> +	 */
>>>> +	udelay(1);
>>>
>>> I don't love that this procedure appears in the platform reset driver.
>>> I appreciate it may not be clear from the SoC TRM, but this is the
>>> standard reset procedure for all IMG Rogue GPUs. The currently
>>> supported TI SoC handles this in silicon, when power up/down requests
>>> are sent so we never needed to encode it in the driver before.
>>>
>>> Strictly speaking, the 32 cycle delay is required between power and
>>> clocks being enabled and the reset line being deasserted. If nothing
>>> here touches power or clocks (which I don't think it should), the delay
>>> could potentially be lifted to the GPU driver.
> 
> ... This could be expressed as a delay between clk_prepare_enable() and
> reset_control_deassert() in the GPU driver then.
> 
>> Yeah you're making excellent points here, I think it would be a good    
>> idea to place the delay in the GPU driver, since this is specific to the
>> whole family of the GPU's not the SoC itself.
>>
>>> Is it expected that if a device exposes a reset in devicetree that it
>>> can be cleanly reset without interaction with the device driver itself?
>>> I.E. in this case, is it required that the reset driver alone can cleanly
>>> reset the GPU?
> 
> No, the "resets" property should just describe the physical
> connection(s) between reset controller and the device.
> 
> It is fine for the device driver to manually assert the reset, enable
> clocks and power, delay, and then deassert the reset, if that is the
> device specific reset procedure.
> 
>> I'm not sure what the community as a whole thinks about that, so maybe
>> someone else can answer this, but I would code SoC specific stuff in the
>> reset driver for the SoC, and the GPU specific stuff (like the delay) in
>> the GPU driver code. I wasn't sure whether the delay was specific to the
>> SoC or the GPU so I've put it here.
> 
> I agree.
> 
> regards
> Philipp
> 

Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Philipp Zabel]

On Mo, 2025-02-10 at 19:17 +0100, Michal Wilczynski wrote:
> On 2/4/25 18:18, Philipp Zabel wrote:
> > On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote:
[...]
> > > I think this is required because the MEM clock gate is somehow broken
> > > and marked as 'reserved' in manual, so instead as a workaround, since we
> > > can't reliably enable the 'mem' clock it's a good idea to reset the
> > > whole CLKGEN of the GPU.
> > 
> > If this is a workaround for broken gating of the "mem" clock, would it
> > be possible (and reasonable) to make this a separate reset control that
> > is handled by the clock driver? ...
> 
> Thank you for the detailed feedback, Philipp.
> 
> After further consideration, I believe keeping the current reset driver
> implementation would be preferable to moving the CLKGEN reset handling
> to the clock driver. While it's technically possible to implement this
> in the clock driver, I have concerns about the added complexity:
> 
> 1. We'd need to expose the CLKGEN reset separately in the reset driver

I'd expect this to simplify the reset driver.

> 2. The clock driver's dt-bindings would need modification to add an
>    optional resets property

If it describes the hardware correctly, that should be fine.

> 3. We'd need custom clk_ops for all three clock gates (including a dummy
>    'mem' gate)
> 4. Each clock gate's .enable operation would need to handle CLKGEN reset
>    deassertion

I accept these arguments, as I have no good feeling for how much
complexity this would actually add.

In my mind it shouldn't be much: the GPU clocks could all share the
same refcounted implementation. The first clock to get enabled would
ungate both GPU_CORE and GPU_CFG_ACLK gates and deassert
GPU_SW_CLKGEN_RST, all in one place. The remaining enable(s) would be
no-ops. Would that work?

Whether a separate "dummy" MEM clock for the DT bindings is added or
not would not make a difference.

> While the clock framework could theoretically handle this, there's no
> clean way to express the requirement that the CLKGEN reset should only
> be deasserted after all clocks in the group are enabled. We could
> implement this explicitly, but it would make the code more complex and
> harder to understand.

Doing this in the clock driver would have the advantage of clk_enabled
GPU clocks actually staying physically enabled, without the reset
driver disabling them via GPU_SW_CLKGEN_RST from the outside.

> The current solution in the reset driver is simpler and clearer - it
> treats this as what it really is: a TH1520-specific reset sequence.

Yes. What this also is: a workaround for a SoC specific defect in the
clock tree. I think it belongs in the clock driver because of this.


[...]
> Regarding the delay between clock enable and reset deassert - for SoCs
> like BPI-F3 with a single reset line, implementing this in the GPU
> consumer driver makes perfect sense. However, for the T-HEAD SoC, moving
> the delay there would actually complicate things since we need to manage
> both the CLKGEN and GPU reset lines in a specific sequence. Having this
> handled entirely within the reset driver keeps the implementation
> cleaner.

You could delay in both places, it's just a microsecond after all.
Whether the workaround is implemented in the reset driver or in the
clock driver, I wouldn't want the GPU driver to have to carry a special
case for TH1520.

> Does this reasoning align with your thoughts? I'm happy to explore the
> clock driver approach further if you still see significant advantages to
> that solution.

I won't object to carry this in the reset driver if the clock
implementation turns out to be unreasonably complex, but I currently
don't expect that to be the case. Please give it a shot.

regards
Philipp

[PATCH v4 10/18] drm/imagination: Add reset controller support for GPU initialization

[Michal Wilczynski]

Certain platforms, such as the T-Head TH1520 and Banana Pi BPI-F3,
require a controlled GPU reset sequence during the power-up procedure
to ensure proper initialization. Without this reset, the GPU may remain
in an undefined state, potentially leading to stability or performance
issues.

This commit integrates a dedicated reset controller within the
drm/imagination driver. By doing so, the driver can coordinate the
necessary reset operations as part of the normal GPU bring-up process,
improving reliability and ensuring that the hardware is ready for
operation.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/pvr_device.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_device.h |  9 +++++++++
 drivers/gpu/drm/imagination/pvr_power.c  | 12 +++++++++++-
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 1704c0268589..ef73e95157ee 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -25,6 +25,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/stddef.h>
 #include <linux/types.h>
@@ -120,6 +121,21 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
 	return 0;
 }
 
+static int pvr_device_reset_init(struct pvr_device *pvr_dev)
+{
+	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
+	struct reset_control *reset;
+
+	reset = devm_reset_control_get_optional_exclusive(drm_dev->dev, NULL);
+	if (IS_ERR(reset))
+		return dev_err_probe(drm_dev->dev, PTR_ERR(reset),
+				     "failed to get gpu reset line\n");
+
+	pvr_dev->reset = reset;
+
+	return 0;
+}
+
 /**
  * pvr_device_process_active_queues() - Process all queue related events.
  * @pvr_dev: PowerVR device to check
@@ -509,6 +525,11 @@ pvr_device_init(struct pvr_device *pvr_dev)
 	if (err)
 		return err;
 
+	/* Get the reset line for the GPU */
+	err = pvr_device_reset_init(pvr_dev);
+	if (err)
+		return err;
+
 	/* Explicitly power the GPU so we can access control registers before the FW is booted. */
 	err = pm_runtime_resume_and_get(dev);
 	if (err)
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index 6d0dfacb677b..f6576c08111c 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -131,6 +131,15 @@ struct pvr_device {
 	 */
 	struct clk *mem_clk;
 
+	/**
+	 * @reset: Optional reset line.
+	 *
+	 * This may be used on some platforms to provide a reset line that needs to be de-asserted
+	 * after power-up procedure. It would also need to be asserted after the power-down
+	 * procedure.
+	 */
+	struct reset_control *reset;
+
 	/** @irq: IRQ number. */
 	int irq;
 
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index ba7816fd28ec..e39460d594bd 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -15,6 +15,7 @@
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/timer.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
@@ -252,6 +253,8 @@ pvr_power_device_suspend(struct device *dev)
 	clk_disable_unprepare(pvr_dev->sys_clk);
 	clk_disable_unprepare(pvr_dev->core_clk);
 
+	err = reset_control_assert(pvr_dev->reset);
+
 err_drm_dev_exit:
 	drm_dev_exit(idx);
 
@@ -282,16 +285,23 @@ pvr_power_device_resume(struct device *dev)
 	if (err)
 		goto err_sys_clk_disable;
 
+	err = reset_control_deassert(pvr_dev->reset);
+	if (err)
+		goto err_mem_clk_disable;
+
 	if (pvr_dev->fw_dev.booted) {
 		err = pvr_power_fw_enable(pvr_dev);
 		if (err)
-			goto err_mem_clk_disable;
+			goto err_reset_assert;
 	}
 
 	drm_dev_exit(idx);
 
 	return 0;
 
+err_reset_assert:
+	reset_control_assert(pvr_dev->reset);
+
 err_mem_clk_disable:
 	clk_disable_unprepare(pvr_dev->mem_clk);
 
-- 
2.34.1

Re: [PATCH v4 10/18] drm/imagination: Add reset controller support for GPU initialization

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 5213 bytes --]

On 28/01/2025 19:48, Michal Wilczynski wrote:
> Certain platforms, such as the T-Head TH1520 and Banana Pi BPI-F3,
> require a controlled GPU reset sequence during the power-up procedure
> to ensure proper initialization. Without this reset, the GPU may remain
> in an undefined state, potentially leading to stability or performance
> issues.

Can you reword this to clarify that _all_ IMG Rogue GPUs have a reset
line that participates in the power-up sequence but some SoCs handle
this in silicon and/or firmware without exposing the reset line
directly (as the currently supported TI SoC does).

> 
> This commit integrates a dedicated reset controller within the
> drm/imagination driver. By doing so, the driver can coordinate the
> necessary reset operations as part of the normal GPU bring-up process,
> improving reliability and ensuring that the hardware is ready for
> operation.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/gpu/drm/imagination/pvr_device.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/imagination/pvr_device.h |  9 +++++++++
>  drivers/gpu/drm/imagination/pvr_power.c  | 12 +++++++++++-
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index 1704c0268589..ef73e95157ee 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -25,6 +25,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/stddef.h>
>  #include <linux/types.h>
> @@ -120,6 +121,21 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
>  	return 0;
>  }
>  
> +static int pvr_device_reset_init(struct pvr_device *pvr_dev)
> +{
> +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +	struct reset_control *reset;
> +
> +	reset = devm_reset_control_get_optional_exclusive(drm_dev->dev, NULL);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(drm_dev->dev, PTR_ERR(reset),
> +				     "failed to get gpu reset line\n");
> +
> +	pvr_dev->reset = reset;
> +
> +	return 0;
> +}
> +
>  /**
>   * pvr_device_process_active_queues() - Process all queue related events.
>   * @pvr_dev: PowerVR device to check
> @@ -509,6 +525,11 @@ pvr_device_init(struct pvr_device *pvr_dev)
>  	if (err)
>  		return err;
>  
> +	/* Get the reset line for the GPU */
> +	err = pvr_device_reset_init(pvr_dev);
> +	if (err)
> +		return err;
> +
>  	/* Explicitly power the GPU so we can access control registers before the FW is booted. */
>  	err = pm_runtime_resume_and_get(dev);
>  	if (err)
> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> index 6d0dfacb677b..f6576c08111c 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.h
> +++ b/drivers/gpu/drm/imagination/pvr_device.h
> @@ -131,6 +131,15 @@ struct pvr_device {
>  	 */
>  	struct clk *mem_clk;
>  
> +	/**
> +	 * @reset: Optional reset line.
> +	 *
> +	 * This may be used on some platforms to provide a reset line that needs to be de-asserted
> +	 * after power-up procedure. It would also need to be asserted after the power-down
> +	 * procedure.
> +	 */
> +	struct reset_control *reset;
> +
>  	/** @irq: IRQ number. */
>  	int irq;
>  
> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index ba7816fd28ec..e39460d594bd 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
> @@ -15,6 +15,7 @@
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/timer.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
> @@ -252,6 +253,8 @@ pvr_power_device_suspend(struct device *dev)
>  	clk_disable_unprepare(pvr_dev->sys_clk);
>  	clk_disable_unprepare(pvr_dev->core_clk);
>  
> +	err = reset_control_assert(pvr_dev->reset);
> +
>  err_drm_dev_exit:
>  	drm_dev_exit(idx);
>  
> @@ -282,16 +285,23 @@ pvr_power_device_resume(struct device *dev)
>  	if (err)
>  		goto err_sys_clk_disable;

This is where I'd expect to see the 32 cycle delay that's currently in
P9 ("reset: thead: Add TH1520 reset controller driver").

If it turns out that delay is required in the reset driver, would you be
opposed to adding it here as well? It's a very small amount of time and
would make this codepath more versatile to future reset controllers.

Cheers,
Matt

>  
> +	err = reset_control_deassert(pvr_dev->reset);
> +	if (err)
> +		goto err_mem_clk_disable;
> +
>  	if (pvr_dev->fw_dev.booted) {
>  		err = pvr_power_fw_enable(pvr_dev);
>  		if (err)
> -			goto err_mem_clk_disable;
> +			goto err_reset_assert;
>  	}
>  
>  	drm_dev_exit(idx);
>  
>  	return 0;
>  
> +err_reset_assert:
> +	reset_control_assert(pvr_dev->reset);
> +
>  err_mem_clk_disable:
>  	clk_disable_unprepare(pvr_dev->mem_clk);
>  

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[PATCH v4 11/18] dt-bindings: gpu: Add 'resets' property for GPU initialization

[Michal Wilczynski]

Many RISC-V boards featuring Imagination Technologies GPUs require a
reset line to be de-asserted as part of the GPU power-up sequence. To
support this, add a 'resets' property to the GPU device tree bindings.
This ensures the GPU can be properly initialized on these platforms.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index 256e252f8087..bb607d4b1e07 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -37,6 +37,9 @@ properties:
   power-domains:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.34.1

Re: [PATCH v4 11/18] dt-bindings: gpu: Add 'resets' property for GPU initialization

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 1291 bytes --]

On 28/01/2025 19:48, Michal Wilczynski wrote:
> Many RISC-V boards featuring Imagination Technologies GPUs require a
> reset line to be de-asserted as part of the GPU power-up sequence. To
> support this, add a 'resets' property to the GPU device tree bindings.
> This ensures the GPU can be properly initialized on these platforms.

My comment on P10 ("drm/imagination: Add reset controller support for
GPU initialization") applies here too - would you mind rewording this?

Cheers,
Matt

> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index 256e252f8087..bb607d4b1e07 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -37,6 +37,9 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[PATCH v4 12/18] dt-bindings: gpu: Add support for T-HEAD TH1520 GPU

[Michal Wilczynski]

Add bindings for the PowerVR BXM-4-64 GPU integrated in the T-HEAD
TH1520 SoC.  This GPU requires two clocks.

Document the integration details including clock, reset, power domain
and interrupt assignments. Add a dt-bindings example showing the proper
usage of the compatible string "thead,th1520-gpu" along with
"img,img-bxm".

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/gpu/img,powervr-rogue.yaml       | 39 +++++++++++++++++--
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index bb607d4b1e07..b0d9635704d8 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -12,10 +12,15 @@ maintainers:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - ti,am62-gpu
-      - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
+    oneOf:
+      - items:
+          - enum:
+              - ti,am62-gpu
+          - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
+      - items:
+          - enum:
+              - thead,th1520-gpu
+          - const: img,img-bxm
 
   reg:
     maxItems: 1
@@ -60,6 +65,17 @@ allOf:
         clocks:
           maxItems: 1
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: thead,th1520-gpu
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
@@ -74,3 +90,18 @@ examples:
         interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
         power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
     };
+
+    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+    #include <dt-bindings/power/thead,th1520-power.h>
+    #include <dt-bindings/reset/thead,th1520-reset.h>
+
+    gpu: gpu@fff0000 {
+        compatible = "thead,th1520-gpu", "img,img-bxm";
+        reg = <0xfff0000 0x1000>;
+        interrupt-parent = <&plic>;
+        interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk CLK_GPU_CORE>, <&clk CLK_GPU_CFG_ACLK>;
+        clock-names = "core", "mem";
+        power-domains = <&pd TH1520_GPU_PD>;
+        resets = <&rst TH1520_RESET_ID_GPU>;
+    };
-- 
2.34.1

Re: [PATCH v4 12/18] dt-bindings: gpu: Add support for T-HEAD TH1520 GPU

[Rob Herring (Arm)]

On Tue, 28 Jan 2025 20:48:10 +0100, Michal Wilczynski wrote:
> Add bindings for the PowerVR BXM-4-64 GPU integrated in the T-HEAD
> TH1520 SoC.  This GPU requires two clocks.
> 
> Document the integration details including clock, reset, power domain
> and interrupt assignments. Add a dt-bindings example showing the proper
> usage of the compatible string "thead,th1520-gpu" along with
> "img,img-bxm".
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/gpu/img,powervr-rogue.yaml       | 39 +++++++++++++++++--
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/gpu/img,powervr-rogue.example.dts:46.28-29 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/gpu/img,powervr-rogue.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
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/20250128194816.2185326-13-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: [PATCH v4 12/18] dt-bindings: gpu: Add support for T-HEAD TH1520 GPU

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 3658 bytes --]

On 28/01/2025 19:48, Michal Wilczynski wrote:
> Add bindings for the PowerVR BXM-4-64 GPU integrated in the T-HEAD
> TH1520 SoC.  This GPU requires two clocks.

None of the IMG Rogue GPUs use two clocks; they're all either one or
three. The TRM for the TH1520 I have shows the standard three (core,
cfg and mem). I mentioned this on P2 ("clk: thead: Add clock support for
VO subsystem in T-Head TH1520 SoC"); can you add the missing clock here
too?

> Document the integration details including clock, reset, power domain
> and interrupt assignments. Add a dt-bindings example showing the proper
> usage of the compatible string "thead,th1520-gpu" along with
> "img,img-bxm".
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/gpu/img,powervr-rogue.yaml       | 39 +++++++++++++++++--
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index bb607d4b1e07..b0d9635704d8 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -12,10 +12,15 @@ maintainers:
>  
>  properties:
>    compatible:
> -    items:
> -      - enum:
> -          - ti,am62-gpu
> -      - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
> +    oneOf:
> +      - items:
> +          - enum:
> +              - ti,am62-gpu
> +          - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
> +      - items:
> +          - enum:
> +              - thead,th1520-gpu
> +          - const: img,img-bxm

This is going to be the main conflict between this series and the other
B-Series series I mentioned on the cover letter. One of the main changes
in that series is to rework how our compatible strings are structured;
that would make this "thead,th1520-gpu", "img,img-bxm-4-64",
"img,img-rogue". Would you mind holding this change back until the other
series lands so we can avoid carrying a second deprecated compatible
string?

>  
>    reg:
>      maxItems: 1
> @@ -60,6 +65,17 @@ allOf:
>          clocks:
>            maxItems: 1
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: thead,th1520-gpu
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 2
> +          maxItems: 2

As mentioned before, this doesn't represent the hardware. Please bump to
3 and add the missing clock.

> +
>  examples:
>    - |
>      #include <dt-bindings/interrupt-controller/irq.h>
> @@ -74,3 +90,18 @@ examples:
>          interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>          power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
>      };
> +
> +    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
> +    #include <dt-bindings/power/thead,th1520-power.h>
> +    #include <dt-bindings/reset/thead,th1520-reset.h>
> +
> +    gpu: gpu@fff0000 {
> +        compatible = "thead,th1520-gpu", "img,img-bxm";
> +        reg = <0xfff0000 0x1000>;
> +        interrupt-parent = <&plic>;
> +        interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clk CLK_GPU_CORE>, <&clk CLK_GPU_CFG_ACLK>;
> +        clock-names = "core", "mem";

You have CFG mapped to "mem" here. Out of curiosity, was that mismatch
required to make things work?

Cheers,
Matt

> +        power-domains = <&pd TH1520_GPU_PD>;
> +        resets = <&rst TH1520_RESET_ID_GPU>;
> +    };

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v4 12/18] dt-bindings: gpu: Add support for T-HEAD TH1520 GPU

[Michal Wilczynski]

On 1/31/25 16:39, Matt Coster wrote:
> On 28/01/2025 19:48, Michal Wilczynski wrote:
>> Add bindings for the PowerVR BXM-4-64 GPU integrated in the T-HEAD
>> TH1520 SoC.  This GPU requires two clocks.
> 
> None of the IMG Rogue GPUs use two clocks; they're all either one or
> three. The TRM for the TH1520 I have shows the standard three (core,
> cfg and mem). I mentioned this on P2 ("clk: thead: Add clock support for
> VO subsystem in T-Head TH1520 SoC"); can you add the missing clock here
> too?
> 
>> Document the integration details including clock, reset, power domain
>> and interrupt assignments. Add a dt-bindings example showing the proper
>> usage of the compatible string "thead,th1520-gpu" along with
>> "img,img-bxm".
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  .../bindings/gpu/img,powervr-rogue.yaml       | 39 +++++++++++++++++--
>>  1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> index bb607d4b1e07..b0d9635704d8 100644
>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> @@ -12,10 +12,15 @@ maintainers:
>>  
>>  properties:
>>    compatible:
>> -    items:
>> -      - enum:
>> -          - ti,am62-gpu
>> -      - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - ti,am62-gpu
>> +          - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
>> +      - items:
>> +          - enum:
>> +              - thead,th1520-gpu
>> +          - const: img,img-bxm
> 
> This is going to be the main conflict between this series and the other
> B-Series series I mentioned on the cover letter. One of the main changes
> in that series is to rework how our compatible strings are structured;
> that would make this "thead,th1520-gpu", "img,img-bxm-4-64",
> "img,img-rogue". Would you mind holding this change back until the other
> series lands so we can avoid carrying a second deprecated compatible
> string?

Sure that's completely fine !

> 
>>  
>>    reg:
>>      maxItems: 1
>> @@ -60,6 +65,17 @@ allOf:
>>          clocks:
>>            maxItems: 1
>>  
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: thead,th1520-gpu
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 2
>> +          maxItems: 2
> 
> As mentioned before, this doesn't represent the hardware. Please bump to
> 3 and add the missing clock.
> 
>> +
>>  examples:
>>    - |
>>      #include <dt-bindings/interrupt-controller/irq.h>
>> @@ -74,3 +90,18 @@ examples:
>>          interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>>          power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
>>      };
>> +
>> +    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
>> +    #include <dt-bindings/power/thead,th1520-power.h>
>> +    #include <dt-bindings/reset/thead,th1520-reset.h>
>> +
>> +    gpu: gpu@fff0000 {
>> +        compatible = "thead,th1520-gpu", "img,img-bxm";
>> +        reg = <0xfff0000 0x1000>;
>> +        interrupt-parent = <&plic>;
>> +        interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&clk CLK_GPU_CORE>, <&clk CLK_GPU_CFG_ACLK>;
>> +        clock-names = "core", "mem";
> 
> You have CFG mapped to "mem" here. Out of curiosity, was that mismatch
> required to make things work?

Yeah exactly, I understand that from the GPU perspective there are three
clocks, but only two are programmable from the SoC perspective.

So maybe a placeholder clock should be added in the devicetree then,
since the clock exists, but is reserved.

> 
> Cheers,
> Matt
> 
>> +        power-domains = <&pd TH1520_GPU_PD>;
>> +        resets = <&rst TH1520_RESET_ID_GPU>;
>> +    };
> 

[PATCH v4 13/18] 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" 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.

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 0639502137b4..cbd7f5d37cc4 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1474,6 +1474,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", .data = NULL },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
2.34.1

Re: [PATCH v4 13/18] drm/imagination: Add support for IMG BXM-4-64 GPU

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 1510 bytes --]

On 28/01/2025 19:48, 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" 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.
> 
> 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 0639502137b4..cbd7f5d37cc4 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1474,6 +1474,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", .data = NULL },

This is a more minor conflict with the other B-Series series mentioned
on the cover letter; we'd like to switch to matching against the more
generic "img,img-rogue" so this would be unnecessary.

However, you do need to add a MODULE_FIRMWARE() tag at the bottom of
this file either way.

Cheers,
Matt

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[PATCH v4 14/18] 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

[PATCH v4 15/18] riscv: dts: thead: Add device tree VO clock controller

[Michal Wilczynski]

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

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

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

[PATCH v4 16/18] riscv: dts: thead: Introduce power domain nodes with aon firmware

[Michal Wilczynski]

The DRM Imagination GPU requires a power-domain driver. In the T-HEAD
TH1520 SoC implements power management capabilities through the E902
core, which can be communicated with through the mailbox, using firmware
protocol.

Add AON node, which servers as a power-domain controller.

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index d4cba0713cab..2f5389579abc 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-ap.h>
+#include <dt-bindings/power/thead,th1520-power.h>
 
 / {
 	compatible = "thead,th1520";
@@ -229,6 +230,13 @@ stmmac_axi_config: stmmac-axi-config {
 		snps,blen = <0 0 64 32 0 0 0>;
 	};
 
+	aon: aon {
+		compatible = "thead,th1520-aon";
+		mboxes = <&mbox_910t 1>;
+		mbox-names = "aon";
+		#power-domain-cells = <1>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;
-- 
2.34.1

[PATCH v4 17/18] riscv: dts: thead: Introduce reset controller node

[Michal Wilczynski]

T-HEAD TH1520 SoC requires to put the GPU out of the reset state as part
of the power-up sequence.

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 2f5389579abc..bdbb1b985b0b 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -7,6 +7,7 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/clock/thead,th1520-clk-ap.h>
 #include <dt-bindings/power/thead,th1520-power.h>
+#include <dt-bindings/reset/thead,th1520-reset.h>
 
 / {
 	compatible = "thead,th1520";
@@ -497,6 +498,12 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
+		rst: reset-controller@ffef528000 {
+			compatible = "thead,th1520-reset";
+			reg = <0xff 0xef528000 0x0 0x4f>;
+			#reset-cells = <1>;
+		};
+
 		clk_vo: clock-controller@ffef528050 {
 			compatible = "thead,th1520-clk-vo";
 			reg = <0xff 0xef528050 0x0 0xfb0>;
-- 
2.34.1

[PATCH v4 18/18] riscv: dts: thead: 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.

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 bdbb1b985b0b..b4b3add0d98d 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -498,6 +498,18 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
+		gpu: gpu@ffef400000 {
+			compatible = "thead,th1520-gpu", "img,img-bxm";
+			reg = <0xff 0xef400000 0x0 0x100000>;
+			interrupt-parent = <&plic>;
+			interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk_vo CLK_GPU_CORE>,
+				 <&clk_vo CLK_GPU_CFG_ACLK>;
+			clock-names = "core", "mem";
+			power-domains = <&aon TH1520_GPU_PD>;
+			resets = <&rst TH1520_RESET_ID_GPU>;
+		};
+
 		rst: reset-controller@ffef528000 {
 			compatible = "thead,th1520-reset";
 			reg = <0xff 0xef528000 0x0 0x4f>;
-- 
2.34.1

Re: [PATCH v4 00/18] Enable drm/imagination BXM-4-64 Support for LicheePi 4A

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 12233 bytes --]

On 28/01/2025 19:47, Michal Wilczynski wrote:
> 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. Support for the mailbox was merged
> in kernel 6.13 [13], and the AON protocol implementation is part of this
> series, since v2. Therefore this series support full power management
> capabilities for the GPU driver.
> 
> Thanks everyone for taking the time to review the last revision ! Your
> guidance and the direction was very helpful.

Hi Michal,

This is awesome, thanks for contributing! I apologise for not responding
to earlier versions (things have been pretty busy here), but they have
very much been on my radar.

I see you're already aware of my other B-Series patchs[3]; would you
mind rebasing these changes on top of those? We're making some not
insignificant changes to the bindings which will conflict with any new
GPUs added in the meantime. I'll leave comments in this series where I
suspect the conflicts will appear.

Cheers,
Matt

> Since this patchset is slowly getting closer to being ready I'm dropping the
> RFC prefix with the v4.
> 
> v4:
> 
> Device Tree Changes:
> - restructured power device tree bindings by abstracting power-domain IDs from
>   firmware IDs, with the mappings now defined in a dedicated header file
> - retained existing clock-names in img,powervr-rogue.yaml to maintain ABI
>   compatibility, despite having only one downstream user
> - enhanced documentation for new Video Output (VO) clock inputs in device tree
>   bindings
> 
> Reset Driver Changes:
> - reverted reset-cells configuration to single cell as in v2
> - maintained reset definitions in device tree bindings while deferring
>   implementation of watchdog timer (WDT) reset functionality
> - addressed implementation issues in the DRM/Imagination reset driver
> 
> Power Domain Changes:
> - added workaround to disable AUDIO power domain to prevent firmware crashes
> 
> v3:
> 
> Device Tree Changes:
>  - consolidated device tree representation by merging aon and power-domain nodes
>    while maintaining separate drivers internally
>  - power-domain driver is now instantiated from within the aon driver
>  - updated img,powervr-rogue.yaml to use allOf and oneOf for better schema
>    organization
> 
> AP Clock Driver Improvements:
>  - reworked driver to support multiple clock controllers through .compatible
>    and .data instead of using multiple address spaces in dt-binding. This change
>    allows to re-use the driver code for multiple clock controllers
> 
> Code Quality and Documentation:
>  - fixed optional module dependencies in Kconfig
>  - added kernel-doc comments for all exported functions
>  - implemented th1520_aon_remove() to properly clean up mailbox channel
>    resources
>  - removed unnecessary of.h header in multiple drivers
>  - refactored reset driver to use zero cells
> 
> v2:
> 
> Removed AP_SUBSYS clock refactoring commits (1-6):
>  - instead of refactoring, I opted to extend the current driver and its
>    associated device tree node to include support for a second address space.
> 
> Expanded patchset scope to fully support power management capabilities:
>  - introduced a new firmware driver to manage power-related operations.
>  - rewrote the power-domain driver to function alongside the firmware driver.
>    These nodes in the device tree lack direct address spaces, despite
>    representing HW blocks. Control is achieved via firmware protocol messages
>    transmitted through a mailbox to the E902 core.
> 
> Implemented a reset controller for the TH1520 SoC:
>  - developed a reset controller driver for the TH1520 to manage reset
>    sequences.
>  - updated the drm/imagination driver to act as a reset controller consumer.
>    While this patchset is focused on the LPI4A board, the reset controller is
>    designed to be useful for other boards, such as the BPI-3F, which also require
>    a reset sequence after power-up.
> 
> Updated dt-bindings:
>  - added new dt-bindings for power, reset, and firmware nodes.
>  - updated the powervr dt-binding to include reset support and new compatibles.
>  - ran dtbs_check and dt_binding_check to ensure compliance.
> 
> Addressed code quality:
>  - resolved all checkpatch issues using --strict, except for the call to
>    devm_clk_hw_register_gate_parent_data().  The current implementation remains
>    preferable in this context, and clang-format aligns with this choice.
> 
> 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 (18):
>   dt-bindings: clock: thead: Add TH1520 VO clock controller
>   clk: thead: Add clock support for VO subsystem in T-Head TH1520 SoC
>   dt-bindings: firmware: thead,th1520: Add support for firmware node
>   firmware: thead: Add AON firmware protocol driver
>   dt-bindings: power: Add TH1520 SoC power domains
>   pmdomain: thead: Add power-domain driver for TH1520
>   riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs
>   dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller
>   reset: thead: Add TH1520 reset controller driver
>   drm/imagination: Add reset controller support for GPU initialization
>   dt-bindings: gpu: Add 'resets' property for GPU initialization
>   dt-bindings: gpu: Add support for T-HEAD TH1520 GPU
>   drm/imagination: Add support for IMG BXM-4-64 GPU
>   drm/imagination: Enable PowerVR driver for RISC-V
>   riscv: dts: thead: Add device tree VO clock controller
>   riscv: dts: thead: Introduce power domain nodes with aon firmware
>   riscv: dts: thead: Introduce reset controller node
>   riscv: dts: thead: Add GPU node to TH1520 device tree
> 
>  .../bindings/clock/thead,th1520-clk-ap.yaml   |  17 +-
>  .../bindings/firmware/thead,th1520-aon.yaml   |  53 ++++
>  .../bindings/gpu/img,powervr-rogue.yaml       |  42 ++-
>  .../bindings/reset/thead,th1520-reset.yaml    |  44 +++
>  MAINTAINERS                                   |   8 +
>  arch/riscv/Kconfig.socs                       |   1 +
>  arch/riscv/boot/dts/thead/th1520.dtsi         |  34 +++
>  drivers/clk/thead/clk-th1520-ap.c             | 197 +++++++++++--
>  drivers/firmware/Kconfig                      |   9 +
>  drivers/firmware/Makefile                     |   1 +
>  drivers/firmware/thead,th1520-aon.c           | 268 ++++++++++++++++++
>  drivers/gpu/drm/imagination/Kconfig           |   2 +-
>  drivers/gpu/drm/imagination/pvr_device.c      |  21 ++
>  drivers/gpu/drm/imagination/pvr_device.h      |   9 +
>  drivers/gpu/drm/imagination/pvr_drv.c         |   1 +
>  drivers/gpu/drm/imagination/pvr_power.c       |  12 +-
>  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    | 193 +++++++++++++
>  drivers/reset/Kconfig                         |  10 +
>  drivers/reset/Makefile                        |   1 +
>  drivers/reset/reset-th1520.c                  | 178 ++++++++++++
>  .../dt-bindings/clock/thead,th1520-clk-ap.h   |  33 +++
>  .../dt-bindings/power/thead,th1520-power.h    |  19 ++
>  .../dt-bindings/reset/thead,th1520-reset.h    |  15 +
>  .../linux/firmware/thead/thead,th1520-aon.h   | 197 +++++++++++++
>  28 files changed, 1344 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>  create mode 100644 Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
>  create mode 100644 drivers/firmware/thead,th1520-aon.c
>  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 drivers/reset/reset-th1520.c
>  create mode 100644 include/dt-bindings/power/thead,th1520-power.h
>  create mode 100644 include/dt-bindings/reset/thead,th1520-reset.h
>  create mode 100644 include/linux/firmware/thead/thead,th1520-aon.h
> 

-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v4 00/18] Enable drm/imagination BXM-4-64 Support for LicheePi 4A

[Michal Wilczynski]

On 1/31/25 16:39, Matt Coster wrote:
> On 28/01/2025 19:47, Michal Wilczynski wrote:
>> 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. Support for the mailbox was merged
>> in kernel 6.13 [13], and the AON protocol implementation is part of this
>> series, since v2. Therefore this series support full power management
>> capabilities for the GPU driver.
>>
>> Thanks everyone for taking the time to review the last revision ! Your
>> guidance and the direction was very helpful.
> 
> Hi Michal,
> 
> This is awesome, thanks for contributing! I apologise for not responding
> to earlier versions (things have been pretty busy here), but they have
> very much been on my radar.
> 
> I see you're already aware of my other B-Series patchs[3]; would you
> mind rebasing these changes on top of those? We're making some not
> insignificant changes to the bindings which will conflict with any new
> GPUs added in the meantime. I'll leave comments in this series where I
> suspect the conflicts will appear.
> 
> Cheers,
> Matt


Hi Matt,

Thanks for your review, and no worries about the delay it’s totally
understandable. I appreciate the heads-up about the dt-bindings work in
progress. I just wanted to share a working solution for now, but I’ll
definitely rebase these patches once your changes are in place.

Cheers, Michal

> 

>