[PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Michal Wilczynski]

This patch series introduces support for the Imagination IMG BXM-4-64
GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
managing the GPU's complex power-up and power-down sequence, which
involves multiple clocks and resets.

The TH1520 GPU requires a specific sequence to be followed for its
clocks and resets to ensure correct operation. Initial discussions and
an earlier version of this series explored managing this via the generic
power domain (genpd) framework. However, following further discussions
with kernel maintainers [1], the approach has been reworked to utilize
the dedicated power sequencing (pwrseq) framework.

This revised series now employs a new pwrseq provider driver
(pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
encapsulates the SoC specific power sequence details. The Imagination
GPU driver (pvr_device.c) is updated to act as a consumer of this power
sequencer, requesting the "gpu-power" target. The sequencer driver,
during its match phase with the GPU device, acquires the necessary clock
and reset handles from the GPU device node to perform the full sequence.

This approach aligns with the goal of abstracting SoC specific power
management details away from generic device drivers and leverages the
pwrseq framework as recommended.

The series is structured as follows:

Patch 1: Adds device tree bindings for the new T-HEAD TH1520 GPU
         power sequencer provider.
Patch 2: Introduces the pwrseq-thead-gpu driver to manage the GPU's
         power-on/off sequence.
Patch 3: Updates the Imagination DRM driver to utilize the pwrseq
         framework for TH1520 GPU power management.
Patch 4: Adds the TH1520 GPU compatible string to the Imagination
         GPU DT bindings.
Patch 5: Adds the missing reset controller header include in the
         TH1520 DTS include file.
Patch 6: Adds the device tree node for the GPU power sequencer to
         the TH1520 DTS include file.
Patch 7: Adds the GPU device tree node for the IMG BXM-4-64 GPU to
         the TH1520 DTS include file.
Patch 8: Enables compilation of the drm/imagination on the RISC-V
         architecture

This patchset finishes the work started in bigger series [2] by adding
all the remaining GPU power sequencing piece. After this patchset the GPU
probes correctly.

This series supersedes the previous genpd based approach. Testing on
T-HEAD TH1520 SoC indicates the new pwrseq based solution works
correctly.

This time it's based on linux-next, as there are dependent patches not
yet merged, but present in linux-next like clock and reset patches.

An open point in Patch 7/8 concerns the GPU memory clock (gpu_mem_clk),
defined as a fixed-clock. The specific hardware frequency for this clock
on the TH1520 could not be determined from available public
documentation. Consequently, clock-frequency = <0>; has been used as a
placeholder to enable driver functionality.

Link to v2 of this series - [3].

v3:

 - re-worked cover letter completely
 - complete architectural rework from using extended genpd callbacks to a
   dedicated pwrseq provider driver
 - introduced pwrseq-thead-gpu.c and associated DT bindings
   (thead,th1520-gpu-pwrseq)
 - the Imagination driver now calls devm_pwrseq_get() and uses
   pwrseq_power_on() / pwrseq_power_off() for the TH1520 GPU
 - removed the platform_resources_managed flag from dev_pm_info and
   associated logic
 - the new pwrseq driver's match() function now acquires consumer-specific
   resources (GPU clocks, GPU core reset) directly from the consumer device

v2:

Extended the series by adding two new commits:
 - introduced a new platform_resources_managed flag in dev_pm_info along
   with helper functions, allowing drivers to detect when clocks and resets
   are managed by the platform
 - updated the DRM Imagination driver to skip claiming clocks when
   platform_resources_managed is set

Split the original bindings update:
 - the AON firmware bindings now only add the GPU clkgen reset (the GPU
   core reset remains handled by the GPU node)

Reworked the TH1520 PM domain driver to:
 - acquire GPU clocks and reset dynamically using attach_dev/detach_dev
   callbacks
 - handle clkgen reset internally, while GPU core reset is obtained from
   the consumer device node
 - added a check to enforce that only a single device can be attached to
   the GPU PM domain

[1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
[2] - https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/
[3] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-0-70c5af2af96c@samsung.com/

---
Michal Wilczynski (8):
      dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer
      power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
      drm/imagination: Use pwrseq for TH1520 GPU power management
      dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings
      riscv: dts: thead: th1520: Add missing reset controller header include
      riscv: dts: thead: Add GPU power sequencer node
      riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node
      drm/imagination: Enable PowerVR driver for RISC-V

 .../devicetree/bindings/gpu/img,powervr-rogue.yaml |   9 +-
 .../bindings/power/thead,th1520-pwrseq.yaml        |  42 +++++
 MAINTAINERS                                        |   2 +
 arch/riscv/boot/dts/thead/th1520.dtsi              |  29 ++++
 drivers/gpu/drm/imagination/Kconfig                |   3 +-
 drivers/gpu/drm/imagination/pvr_device.c           |  33 +++-
 drivers/gpu/drm/imagination/pvr_device.h           |   6 +
 drivers/gpu/drm/imagination/pvr_power.c            |  82 +++++----
 drivers/power/sequencing/Kconfig                   |   8 +
 drivers/power/sequencing/Makefile                  |   1 +
 drivers/power/sequencing/pwrseq-thead-gpu.c        | 183 +++++++++++++++++++++
 11 files changed, 363 insertions(+), 35 deletions(-)
---
base-commit: 49473fe7fdb5fbbe5bbfa51083792c17df63d317
change-id: 20250414-apr_14_for_sending-5b3917817acc

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

[PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Michal Wilczynski]

Introduce device tree bindings for a new power sequencer provider
dedicated to the T-HEAD TH1520 SoC's GPU.

The thead,th1520-gpu-pwrseq compatible designates a node that will
manage the complex power-up and power-down sequence for the GPU. This
sequencer requires a handle to the GPU's clock generator reset line
(gpu-clkgen), which is specified in its device tree node.

This binding will be used by a new pwrseq driver to abstract the
SoC specific power management details from the generic GPU driver.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../bindings/power/thead,th1520-pwrseq.yaml        | 42 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 43 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..4c302abfb76fb9e243946f4eefa333c6b02e59d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/thead,th1520-pwrseq.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 GPU Power Sequencer
+
+maintainers:
+  - Michal Wilczynski <m.wilczynski@samsung.com>
+
+description: |
+  This binding describes the power sequencer for the T-HEAD TH1520 GPU.
+  This sequencer handles the specific power-up and power-down sequences
+  required by the GPU, including managing clocks and resets from both the
+  sequencer and the GPU device itself.
+
+properties:
+  compatible:
+    const: thead,th1520-gpu-pwrseq
+
+  resets:
+    description: A phandle to the GPU clock generator reset.
+    maxItems: 1
+
+  reset-names:
+    const: gpu-clkgen
+
+required:
+  - compatible
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    gpu_pwrseq: pwrseq {
+        compatible = "thead,th1520-gpu-pwrseq";
+        resets = <&rst 0>;
+        reset-names = "gpu-clkgen";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d59a5910e632350a4d72a761c6c5ce1d3a1bc34..78e3067df1152929de638244b03264733d08556e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21355,6 +21355,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/power/thead,th1520-pwrseq.yaml
 F:	Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c

-- 
2.34.1

[PATCH v3 2/8] power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver

[Michal Wilczynski]

Introduce the pwrseq-thead-gpu driver, a power sequencer provider for
the Imagination BXM-4-64 GPU on the T-HEAD TH1520 SoC.

The TH1520 GPU requires a specific sequence to correctly initialize and
power down its resources:
 - Enable GPU clocks (core and sys).
 - De-assert the GPU clock generator reset (clkgen_reset).
 - Introduce a short hardware-required delay.
 - De-assert the GPU core reset. The power-down sequence performs these
   steps in reverse.

Implement this sequence via the pwrseq_power_on and pwrseq_power_off
callbacks. It binds to the "thead,th1520-gpu-pwrseq" device tree node,
from which it acquires the clkgen_reset.

Crucially, the driver's match function is called when a consumer
(the Imagination GPU driver) requests the "gpu-power" target. During
this match, the sequencer uses devm_clk_bulk_get() and
devm_reset_control_get_exclusive() on the consumer's device to
obtain handles to the GPU's "core" and "sys" clocks, and the GPU core
reset. These, along with its own clkgen_reset, allow it to perform
the complete sequence.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS                                 |   1 +
 drivers/power/sequencing/Kconfig            |   8 ++
 drivers/power/sequencing/Makefile           |   1 +
 drivers/power/sequencing/pwrseq-thead-gpu.c | 183 ++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 78e3067df1152929de638244b03264733d08556e..237b37a3f6296a72323657419789dc6fdad1b5d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21364,6 +21364,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/power/sequencing/pwrseq-thead-gpu.c
 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
diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
index ddcc42a984921c55667c46ac586d259625e1f1a7..935428ce8cf44794b7eb943f722ace5021237af2 100644
--- a/drivers/power/sequencing/Kconfig
+++ b/drivers/power/sequencing/Kconfig
@@ -27,4 +27,12 @@ config POWER_SEQUENCING_QCOM_WCN
 	  this driver is needed for correct power control or else we'd risk not
 	  respecting the required delays between enabling Bluetooth and WLAN.
 
+config POWER_SEQUENCING_THEAD_GPU
+	tristate "T-HEAD TH1520 GPU power sequencing driver"
+	depends on ARCH_THEAD
+	help
+	  Say Y here to enable the power sequencing driver for the TH1520 SoC
+	  GPU. This driver handles the complex clock and reset sequence
+	  required to power on the Imagination BXM GPU on this platform.
+
 endif
diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
index 2eec2df7912d11827f9ba914177dd2c882e44bce..647f81f4013ab825630f069d2e0f6d22159f1f56 100644
--- a/drivers/power/sequencing/Makefile
+++ b/drivers/power/sequencing/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_POWER_SEQUENCING)		+= pwrseq-core.o
 pwrseq-core-y				:= core.o
 
 obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN)	+= pwrseq-qcom-wcn.o
+obj-$(CONFIG_POWER_SEQUENCING_THEAD_GPU) += pwrseq-thead-gpu.o
diff --git a/drivers/power/sequencing/pwrseq-thead-gpu.c b/drivers/power/sequencing/pwrseq-thead-gpu.c
new file mode 100644
index 0000000000000000000000000000000000000000..e4c15c3d62eee0c088710c4d134ac2c4b16e2b06
--- /dev/null
+++ b/drivers/power/sequencing/pwrseq-thead-gpu.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * T-HEAD TH1520 GPU Power Sequencer Driver
+ *
+ * Copyright (c) 2025 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ *
+ * This driver implements the power sequence for the Imagination BXM GPU
+ * on the T-HEAD TH1520 SoC. The sequence requires coordinating resources
+ * from both the sequencer's device node (clkgen_reset) and the GPU's
+ * device node (clocks and core reset).
+ *
+ * The `match` function is used to acquire the GPU's resources when the
+ * GPU driver requests the "gpu-power" sequence target.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/reset.h>
+
+struct pwrseq_thead_gpu_ctx {
+	struct pwrseq_device *pwrseq;
+	struct reset_control *clkgen_reset;
+
+	/* Consumer resources */
+	struct clk_bulk_data *clks;
+	int num_clks;
+	struct reset_control *gpu_reset;
+};
+
+static int pwrseq_thead_gpu_power_on(struct pwrseq_device *pwrseq)
+{
+	struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+	int ret;
+
+	if (!ctx->clks || !ctx->gpu_reset)
+		return -ENODEV;
+
+	ret = clk_bulk_prepare_enable(ctx->num_clks, ctx->clks);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(ctx->clkgen_reset);
+	if (ret)
+		goto err_disable_clks;
+
+	/*
+	 * 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);
+
+	ret = reset_control_deassert(ctx->gpu_reset);
+	if (ret)
+		goto err_assert_clkgen;
+
+	return 0;
+
+err_assert_clkgen:
+	reset_control_assert(ctx->clkgen_reset);
+err_disable_clks:
+	clk_bulk_disable_unprepare(ctx->num_clks, ctx->clks);
+	return ret;
+}
+
+static int pwrseq_thead_gpu_power_off(struct pwrseq_device *pwrseq)
+{
+	struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+	if (!ctx->clks || !ctx->gpu_reset)
+		return -ENODEV;
+
+	reset_control_assert(ctx->gpu_reset);
+	reset_control_assert(ctx->clkgen_reset);
+	clk_bulk_disable_unprepare(ctx->num_clks, ctx->clks);
+
+	return 0;
+}
+
+static const struct pwrseq_unit_data pwrseq_thead_gpu_unit = {
+	.name = "gpu-power-sequence",
+	.enable = pwrseq_thead_gpu_power_on,
+	.disable = pwrseq_thead_gpu_power_off,
+};
+
+static const struct pwrseq_target_data pwrseq_thead_gpu_target = {
+	.name = "gpu-power",
+	.unit = &pwrseq_thead_gpu_unit,
+};
+
+static const struct pwrseq_target_data *pwrseq_thead_gpu_targets[] = {
+	&pwrseq_thead_gpu_target,
+	NULL
+};
+
+static int pwrseq_thead_gpu_match(struct pwrseq_device *pwrseq, struct device *dev)
+{
+	struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+	static const char *const clk_names[] = { "core", "sys" };
+	int i, ret;
+
+	/* We only match the specific T-HEAD TH1520 GPU compatible */
+	if (!of_device_is_compatible(dev->of_node, "thead,th1520-gpu"))
+		return 0;
+
+	/* Prevent multiple consumers from attaching */
+	if (ctx->gpu_reset || ctx->clks)
+		return -EBUSY;
+
+	ctx->num_clks = ARRAY_SIZE(clk_names);
+	ctx->clks = devm_kcalloc(dev, ctx->num_clks, sizeof(*ctx->clks), GFP_KERNEL);
+	if (!ctx->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < ctx->num_clks; i++)
+		ctx->clks[i].id = clk_names[i];
+
+	ret = devm_clk_bulk_get(dev, ctx->num_clks, ctx->clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get GPU clocks\n");
+
+	ctx->gpu_reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(ctx->gpu_reset))
+		return dev_err_probe(dev, PTR_ERR(ctx->gpu_reset), "Failed to get GPU reset\n");
+
+	return 1;
+}
+
+static int pwrseq_thead_gpu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwrseq_thead_gpu_ctx *ctx;
+	struct pwrseq_config config = {};
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->clkgen_reset = devm_reset_control_get_exclusive(dev, "gpu-clkgen");
+	if (IS_ERR(ctx->clkgen_reset))
+		return dev_err_probe(dev, PTR_ERR(ctx->clkgen_reset),
+				     "Failed to get GPU clkgen reset\n");
+
+	config.parent = dev;
+	config.owner = THIS_MODULE;
+	config.drvdata = ctx;
+	config.match = pwrseq_thead_gpu_match;
+	config.targets = pwrseq_thead_gpu_targets;
+
+	ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+	if (IS_ERR(ctx->pwrseq))
+		return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+				     "Failed to register power sequencer\n");
+
+	return 0;
+}
+
+static const struct of_device_id pwrseq_thead_gpu_of_match[] = {
+	{ .compatible = "thead,th1520-gpu-pwrseq" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwrseq_thead_gpu_of_match);
+
+static struct platform_driver pwrseq_thead_gpu_driver = {
+	.driver = {
+		.name = "pwrseq-thead-gpu",
+		.of_match_table = pwrseq_thead_gpu_of_match,
+	},
+	.probe = pwrseq_thead_gpu_probe,
+};
+module_platform_driver(pwrseq_thead_gpu_driver);
+
+MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
+MODULE_DESCRIPTION("T-HEAD TH1520 GPU power sequencer driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

Update the Imagination PVR DRM driver to leverage the pwrseq framework
for managing the power sequence of the GPU on the T-HEAD TH1520 SoC.

In pvr_device_init(), the driver now attempts to get a handle to the
"gpu-power" sequencer target using devm_pwrseq_get(). If successful,
the responsibility for powering on and off the GPU's core clocks and
resets is delegated to the power sequencer. Consequently, the GPU
driver conditionally skips acquiring the GPU reset line if the pwrseq
handle is obtained, as the sequencer's match function will acquire it.
Clock handles are still acquired by the GPU driver for other purposes
like devfreq.

The runtime PM callbacks, pvr_power_device_resume() and
pvr_power_device_suspend(), are modified to call pwrseq_power_on() and
pwrseq_power_off() respectively when the sequencer is present.  If no
sequencer is found, the driver falls back to its existing manual clock
and reset management. A helper function,
pvr_power_off_sequence_manual(), is introduced to encapsulate the manual
power-down logic.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/Kconfig      |  1 +
 drivers/gpu/drm/imagination/pvr_device.c | 33 +++++++++++--
 drivers/gpu/drm/imagination/pvr_device.h |  6 +++
 drivers/gpu/drm/imagination/pvr_power.c  | 82 +++++++++++++++++++++-----------
 4 files changed, 89 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..737ace77c4f1247c687cc1fde2f139fc2e118c50 100644
--- a/drivers/gpu/drm/imagination/Kconfig
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -11,6 +11,7 @@ config DRM_POWERVR
 	select DRM_SCHED
 	select DRM_GPUVM
 	select FW_LOADER
+  select POWER_SEQUENCING
 	help
 	  Choose this option if you have a system that has an Imagination
 	  Technologies PowerVR (Series 6 or later) or IMG GPU.
diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..19d48bbc828cf2b8dbead602e90ff88780152124 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/pwrseq/consumer.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/stddef.h>
@@ -631,10 +632,34 @@ 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;
+	/*
+	 * Try to get a power sequencer. If successful, it will handle clocks
+	 * and resets. Otherwise, we fall back to managing them ourselves.
+	 */
+	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
+	if (IS_ERR(pvr_dev->pwrseq)) {
+		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
+
+		/*
+		 * If the error is -EPROBE_DEFER, it's because the
+		 * optional sequencer provider is not present
+		 * and it's safe to fall back on manual power-up.
+		 */
+		if (pwrseq_err == -EPROBE_DEFER)
+			pvr_dev->pwrseq = NULL;
+		else
+			return dev_err_probe(dev, pwrseq_err,
+					     "Failed to get power sequencer\n");
+	}
+
+	/* Get the reset line for the GPU, but since it's exclusive only
+	 * get it if the pwerseq is NULL.
+	 */
+	if (!pvr_dev->pwrseq) {
+		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);
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index 7cb01c38d2a9c3fc71effe789d4dfe54eddd93ee..0b6d53994d25abe32a2f1b133a4efa574e150da9 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -37,6 +37,9 @@ struct clk;
 /* Forward declaration from <linux/firmware.h>. */
 struct firmware;
 
+/* Forward declaration from <linux/pwrseq/consumer.h */
+struct pwrseq_desc;
+
 /**
  * struct pvr_gpu_id - Hardware GPU ID information for a PowerVR device
  * @b: Branch ID.
@@ -148,6 +151,9 @@ struct pvr_device {
 	 */
 	struct reset_control *reset;
 
+	/** @pwrseq: Pointer to a power sequencer, if one is used. */
+	struct pwrseq_desc *pwrseq;
+
 	/** @irq: IRQ number. */
 	int irq;
 
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index 41f5d89e78b854cf6993838868a4416a220b490a..b7dcc03d7d9e06f03f059124fbe21dd844de713e 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -21,6 +21,7 @@
 #include <linux/reset.h>
 #include <linux/timer.h>
 #include <linux/types.h>
+#include <linux/pwrseq/consumer.h>
 #include <linux/workqueue.h>
 
 #define POWER_SYNC_TIMEOUT_US (1000000) /* 1s */
@@ -234,6 +235,19 @@ pvr_watchdog_init(struct pvr_device *pvr_dev)
 	return 0;
 }
 
+static int pvr_power_off_sequence_manual(struct pvr_device *pvr_dev)
+{
+	int err;
+
+	err = reset_control_assert(pvr_dev->reset);
+
+	clk_disable_unprepare(pvr_dev->mem_clk);
+	clk_disable_unprepare(pvr_dev->sys_clk);
+	clk_disable_unprepare(pvr_dev->core_clk);
+
+	return err;
+}
+
 int
 pvr_power_device_suspend(struct device *dev)
 {
@@ -252,11 +266,10 @@ pvr_power_device_suspend(struct device *dev)
 			goto err_drm_dev_exit;
 	}
 
-	clk_disable_unprepare(pvr_dev->mem_clk);
-	clk_disable_unprepare(pvr_dev->sys_clk);
-	clk_disable_unprepare(pvr_dev->core_clk);
-
-	err = reset_control_assert(pvr_dev->reset);
+	if (pvr_dev->pwrseq)
+		err = pwrseq_power_off(pvr_dev->pwrseq);
+	else
+		err = pvr_power_off_sequence_manual(pvr_dev);
 
 err_drm_dev_exit:
 	drm_dev_exit(idx);
@@ -276,44 +289,55 @@ pvr_power_device_resume(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
-	err = clk_prepare_enable(pvr_dev->core_clk);
-	if (err)
-		goto err_drm_dev_exit;
+	if (pvr_dev->pwrseq) {
+		err = pwrseq_power_on(pvr_dev->pwrseq);
+		if (err)
+			goto err_drm_dev_exit;
+	} else {
+		err = clk_prepare_enable(pvr_dev->core_clk);
+		if (err)
+			goto err_drm_dev_exit;
 
-	err = clk_prepare_enable(pvr_dev->sys_clk);
-	if (err)
-		goto err_core_clk_disable;
+		err = clk_prepare_enable(pvr_dev->sys_clk);
+		if (err)
+			goto err_core_clk_disable;
 
-	err = clk_prepare_enable(pvr_dev->mem_clk);
-	if (err)
-		goto err_sys_clk_disable;
+		err = clk_prepare_enable(pvr_dev->mem_clk);
+		if (err)
+			goto err_sys_clk_disable;
 
-	/*
-	 * 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);
+		/*
+		 * 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);
 
-	err = reset_control_deassert(pvr_dev->reset);
-	if (err)
-		goto err_mem_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_reset_assert;
+			goto err_power_off;
 	}
 
 	drm_dev_exit(idx);
 
 	return 0;
 
-err_reset_assert:
-	reset_control_assert(pvr_dev->reset);
+err_power_off:
+	if (pvr_dev->pwrseq)
+		pwrseq_power_off(pvr_dev->pwrseq);
+	else
+		pvr_power_off_sequence_manual(pvr_dev);
+
+	goto err_drm_dev_exit;
 
 err_mem_clk_disable:
 	clk_disable_unprepare(pvr_dev->mem_clk);

-- 
2.34.1

[PATCH v3 4/8] dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings

[Michal Wilczynski]

Update the img,powervr-rogue.yaml device tree bindings to include the
T-HEAD TH1520 SoC's specific GPU compatible string.

The thead,th1520-gpu compatible, along with its full chain
img,img-bxm-4-64, and img,img-rogue, is added to the
list of recognized GPU types. This allows the Imagination DRM driver
to correctly bind to the GPU node defined in the TH1520 device tree.

The power-domains property requirement for img,img-bxm-4-64 is also
ensured by adding it to the relevant allOf condition.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index 4450e2e73b3ccf74d29f0e31e2e6687d7cbe5d65..c12837a0d39b8c3043b9133d444cc33a59135c33 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -26,6 +26,11 @@ properties:
               - ti,j721s2-gpu
           - const: img,img-bxs-4-64
           - const: img,img-rogue
+      - items:
+          - enum:
+              - thead,th1520-gpu
+          - const: img,img-bxm-4-64
+          - const: img,img-rogue
 
       # This legacy combination of compatible strings was introduced early on
       # before the more specific GPU identifiers were used.
@@ -93,7 +98,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: img,img-axe-1-16m
+            enum:
+              - img,img-axe-1-16m
+              - img,img-bxm-4-64
     then:
       properties:
         power-domains:

-- 
2.34.1

[PATCH v3 5/8] riscv: dts: thead: th1520: Add missing reset controller header include

[Michal Wilczynski]

TH1520_RESET_ID_GPU_CLKGEN and TH1520_RESET_ID_GPU are required for GPU
power sequencing to work.  To make these symbols available, add the
necessary include for the T-HEAD TH1520 reset controller bindings.

This change was dropped during conflict resolution [1].

[1] - https://lore.kernel.org/all/aAvfn2mq0Ksi8DF2@x1/

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 1db0054c4e093400e9dbebcee5fcfa5b5cae6e32..bdbb1b985b0b76cf669a9bf40c6ec37258329056 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";

-- 
2.34.1

[PATCH v3 6/8] riscv: dts: thead: Add GPU power sequencer node

[Michal Wilczynski]

Add the device tree node for the T-HEAD TH1520 GPU power sequencer
(gpu_pwrseq) to the th1520.dtsi file.

This node instantiates the thead,th1520-gpu-pwrseq driver, which
is responsible for managing the GPU's power-on/off sequence. The node
specifies the gpu-clkgen reset, which is one of the resources
controlled by this sequencer.

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index bdbb1b985b0b76cf669a9bf40c6ec37258329056..6170eec79e919b606a2046ac8f52db07e47ef441 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -238,6 +238,12 @@ aon: aon {
 		#power-domain-cells = <1>;
 	};
 
+	gpu_pwrseq: pwrseq {
+		compatible = "thead,th1520-gpu-pwrseq";
+		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
+		reset-names = "gpu-clkgen";
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;

-- 
2.34.1

[PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[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.

Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
controlled programatically.

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

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
 		#clock-cells = <0>;
 	};
 
+	gpu_mem_clk: mem-clk {
+		compatible = "fixed-clock";
+		clock-frequency = <0>;
+		clock-output-names = "gpu_mem_clk";
+		#clock-cells = <0>;
+	};
+
 	stmmac_axi_config: stmmac-axi-config {
 		snps,wr_osr_lmt = <15>;
 		snps,rd_osr_lmt = <15>;
@@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
+		gpu: gpu@ffef400000 {
+			compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
+				     "img,img-rogue";
+			reg = <0xff 0xef400000 0x0 0x100000>;
+			interrupt-parent = <&plic>;
+			interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk_vo CLK_GPU_CORE>,
+				 <&gpu_mem_clk>,
+				 <&clk_vo CLK_GPU_CFG_ACLK>;
+			clock-names = "core", "mem", "sys";
+			power-domains = <&aon TH1520_GPU_PD>;
+			power-domain-names = "a";
+			resets = <&rst TH1520_RESET_ID_GPU>;
+		};
+
 		rst: reset-controller@ffef528000 {
 			compatible = "thead,th1520-reset";
 			reg = <0xff 0xef528000 0x0 0x4f>;

-- 
2.34.1

[PATCH v3 8/8] 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 737ace77c4f1247c687cc1fde2f139fc2e118c50..3b773879d781b17549455fac252cec8adfd3a9c8 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

Re: [PATCH v3 5/8] riscv: dts: thead: th1520: Add missing reset controller header include

[Drew Fustini]

On Fri, May 30, 2025 at 12:23:52AM +0200, Michal Wilczynski wrote:
> TH1520_RESET_ID_GPU_CLKGEN and TH1520_RESET_ID_GPU are required for GPU
> power sequencing to work.  To make these symbols available, add the
> necessary include for the T-HEAD TH1520 reset controller bindings.
> 
> This change was dropped during conflict resolution [1].
> 
> [1] - https://lore.kernel.org/all/aAvfn2mq0Ksi8DF2@x1/
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 1db0054c4e093400e9dbebcee5fcfa5b5cae6e32..bdbb1b985b0b76cf669a9bf40c6ec37258329056 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";
> 
> -- 
> 2.34.1
> 

Thanks for adding the header back now that there is no longer a
conflict. I'll look at the rest of this series and the rust pwm series
soon, but I just want to send a quick Rb on this patch right now.

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

Re: [PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Drew Fustini]

On Fri, May 30, 2025 at 12:23:47AM +0200, Michal Wilczynski wrote:
> This patch series introduces support for the Imagination IMG BXM-4-64
> GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
> managing the GPU's complex power-up and power-down sequence, which
> involves multiple clocks and resets.
> 
> The TH1520 GPU requires a specific sequence to be followed for its
> clocks and resets to ensure correct operation. Initial discussions and
> an earlier version of this series explored managing this via the generic
> power domain (genpd) framework. However, following further discussions
> with kernel maintainers [1], the approach has been reworked to utilize
> the dedicated power sequencing (pwrseq) framework.
> 
> This revised series now employs a new pwrseq provider driver
> (pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
> encapsulates the SoC specific power sequence details. The Imagination
> GPU driver (pvr_device.c) is updated to act as a consumer of this power
> sequencer, requesting the "gpu-power" target. The sequencer driver,
> during its match phase with the GPU device, acquires the necessary clock
> and reset handles from the GPU device node to perform the full sequence.
> 
> This approach aligns with the goal of abstracting SoC specific power
> management details away from generic device drivers and leverages the
> pwrseq framework as recommended.
> 
> The series is structured as follows:
> 
> Patch 1: Adds device tree bindings for the new T-HEAD TH1520 GPU
>          power sequencer provider.
> Patch 2: Introduces the pwrseq-thead-gpu driver to manage the GPU's
>          power-on/off sequence.
> Patch 3: Updates the Imagination DRM driver to utilize the pwrseq
>          framework for TH1520 GPU power management.
> Patch 4: Adds the TH1520 GPU compatible string to the Imagination
>          GPU DT bindings.
> Patch 5: Adds the missing reset controller header include in the
>          TH1520 DTS include file.
> Patch 6: Adds the device tree node for the GPU power sequencer to
>          the TH1520 DTS include file.
> Patch 7: Adds the GPU device tree node for the IMG BXM-4-64 GPU to
>          the TH1520 DTS include file.
> Patch 8: Enables compilation of the drm/imagination on the RISC-V
>          architecture
> 
> This patchset finishes the work started in bigger series [2] by adding
> all the remaining GPU power sequencing piece. After this patchset the GPU
> probes correctly.
> 
> This series supersedes the previous genpd based approach. Testing on
> T-HEAD TH1520 SoC indicates the new pwrseq based solution works
> correctly.
> 
> This time it's based on linux-next, as there are dependent patches not
> yet merged, but present in linux-next like clock and reset patches.
> 
> An open point in Patch 7/8 concerns the GPU memory clock (gpu_mem_clk),
> defined as a fixed-clock. The specific hardware frequency for this clock
> on the TH1520 could not be determined from available public
> documentation. Consequently, clock-frequency = <0>; has been used as a
> placeholder to enable driver functionality.
> 
> Link to v2 of this series - [3].
> 
> v3:
> 
>  - re-worked cover letter completely
>  - complete architectural rework from using extended genpd callbacks to a
>    dedicated pwrseq provider driver
>  - introduced pwrseq-thead-gpu.c and associated DT bindings
>    (thead,th1520-gpu-pwrseq)
>  - the Imagination driver now calls devm_pwrseq_get() and uses
>    pwrseq_power_on() / pwrseq_power_off() for the TH1520 GPU
>  - removed the platform_resources_managed flag from dev_pm_info and
>    associated logic
>  - the new pwrseq driver's match() function now acquires consumer-specific
>    resources (GPU clocks, GPU core reset) directly from the consumer device
> 
> v2:
> 
> Extended the series by adding two new commits:
>  - introduced a new platform_resources_managed flag in dev_pm_info along
>    with helper functions, allowing drivers to detect when clocks and resets
>    are managed by the platform
>  - updated the DRM Imagination driver to skip claiming clocks when
>    platform_resources_managed is set
> 
> Split the original bindings update:
>  - the AON firmware bindings now only add the GPU clkgen reset (the GPU
>    core reset remains handled by the GPU node)
> 
> Reworked the TH1520 PM domain driver to:
>  - acquire GPU clocks and reset dynamically using attach_dev/detach_dev
>    callbacks
>  - handle clkgen reset internally, while GPU core reset is obtained from
>    the consumer device node
>  - added a check to enforce that only a single device can be attached to
>    the GPU PM domain
> 
> [1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
> [2] - https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/
> [3] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-0-70c5af2af96c@samsung.com/
> 
> ---
> Michal Wilczynski (8):
>       dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer
>       power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
>       drm/imagination: Use pwrseq for TH1520 GPU power management
>       dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings
>       riscv: dts: thead: th1520: Add missing reset controller header include
>       riscv: dts: thead: Add GPU power sequencer node
>       riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node
>       drm/imagination: Enable PowerVR driver for RISC-V
> 
>  .../devicetree/bindings/gpu/img,powervr-rogue.yaml |   9 +-
>  .../bindings/power/thead,th1520-pwrseq.yaml        |  42 +++++
>  MAINTAINERS                                        |   2 +
>  arch/riscv/boot/dts/thead/th1520.dtsi              |  29 ++++
>  drivers/gpu/drm/imagination/Kconfig                |   3 +-
>  drivers/gpu/drm/imagination/pvr_device.c           |  33 +++-
>  drivers/gpu/drm/imagination/pvr_device.h           |   6 +
>  drivers/gpu/drm/imagination/pvr_power.c            |  82 +++++----
>  drivers/power/sequencing/Kconfig                   |   8 +
>  drivers/power/sequencing/Makefile                  |   1 +
>  drivers/power/sequencing/pwrseq-thead-gpu.c        | 183 +++++++++++++++++++++
>  11 files changed, 363 insertions(+), 35 deletions(-)
> ---
> base-commit: 49473fe7fdb5fbbe5bbfa51083792c17df63d317
> change-id: 20250414-apr_14_for_sending-5b3917817acc
> 
> Best regards,
> -- 
> Michal Wilczynski <m.wilczynski@samsung.com>
> 

Thank you for continuing to work on this series.

I applied it to next-20250530 and the boot hangs:

<snip>
[    0.895622] mmc0: new HS400 MMC card at address 0001
[    0.902638] mmcblk0: mmc0:0001 8GTF4R 7.28 GiB
[    0.915454]  mmcblk0: p1 p2 p3
[    0.916613] debug_vm_pgtable: [debug_vm_pgtable         ]: Validating architecture page table helpers
[    0.920107] mmcblk0boot0: mmc0:0001 8GTF4R 4.00 MiB
[    0.936592] mmcblk0boot1: mmc0:0001 8GTF4R 4.00 MiB
[    0.944986] mmcblk0rpmb: mmc0:0001 8GTF4R 512 KiB, chardev (243:0)
[    0.947700] mmc1: new UHS-I speed DDR50 SDHC card at address aaaa
[    0.961368] mmcblk1: mmc1:aaaa SU16G 14.8 GiB
[    0.969639]  mmcblk1: p1 p2 p3
[    0.986688] printk: legacy console [ttyS0] disabled
[    0.992468] ffe7014000.serial: ttyS0 at MMIO 0xffe7014000 (irq = 23, base_baud = 6250000) is a 16550A
[    1.002085] printk: legacy console [ttyS0] enabled
[    1.002085] printk: legacy console [ttyS0] enabled
[    1.011784] printk: legacy bootconsole [uart0] disabled
[    1.011784] printk: legacy bootconsole [uart0] disabled
[    1.024633] stackdepot: allocating hash table of 524288 entries via kvcalloc
<no more output>

I pasted the full boot log [1]. I have clk_ignore_unused in the kernel
cmdline so I don't think it is related to disabling clocks. Boot does
complete okay if I set the gpu node status to disabled.

Any ideas of what might fix the boot hang?

Thanks,
Drew

[1] https://gist.github.com/pdp7/44bd6de63fb9274a66a705ad807690b6

Re: [PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Michal Wilczynski]

On 6/2/25 01:05, Drew Fustini wrote:
> On Fri, May 30, 2025 at 12:23:47AM +0200, Michal Wilczynski wrote:
>> This patch series introduces support for the Imagination IMG BXM-4-64
>> GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
>> managing the GPU's complex power-up and power-down sequence, which
>> involves multiple clocks and resets.
>>
>> The TH1520 GPU requires a specific sequence to be followed for its
>> clocks and resets to ensure correct operation. Initial discussions and
>> an earlier version of this series explored managing this via the generic
>> power domain (genpd) framework. However, following further discussions
>> with kernel maintainers [1], the approach has been reworked to utilize
>> the dedicated power sequencing (pwrseq) framework.
>>
>> This revised series now employs a new pwrseq provider driver
>> (pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
>> encapsulates the SoC specific power sequence details. The Imagination
>> GPU driver (pvr_device.c) is updated to act as a consumer of this power
>> sequencer, requesting the "gpu-power" target. The sequencer driver,
>> during its match phase with the GPU device, acquires the necessary clock
>> and reset handles from the GPU device node to perform the full sequence.
>>
>> This approach aligns with the goal of abstracting SoC specific power
>> management details away from generic device drivers and leverages the
>> pwrseq framework as recommended.
>>
>> The series is structured as follows:
>>
>> Patch 1: Adds device tree bindings for the new T-HEAD TH1520 GPU
>>          power sequencer provider.
>> Patch 2: Introduces the pwrseq-thead-gpu driver to manage the GPU's
>>          power-on/off sequence.
>> Patch 3: Updates the Imagination DRM driver to utilize the pwrseq
>>          framework for TH1520 GPU power management.
>> Patch 4: Adds the TH1520 GPU compatible string to the Imagination
>>          GPU DT bindings.
>> Patch 5: Adds the missing reset controller header include in the
>>          TH1520 DTS include file.
>> Patch 6: Adds the device tree node for the GPU power sequencer to
>>          the TH1520 DTS include file.
>> Patch 7: Adds the GPU device tree node for the IMG BXM-4-64 GPU to
>>          the TH1520 DTS include file.
>> Patch 8: Enables compilation of the drm/imagination on the RISC-V
>>          architecture
>>
>> This patchset finishes the work started in bigger series [2] by adding
>> all the remaining GPU power sequencing piece. After this patchset the GPU
>> probes correctly.
>>
>> This series supersedes the previous genpd based approach. Testing on
>> T-HEAD TH1520 SoC indicates the new pwrseq based solution works
>> correctly.
>>
>> This time it's based on linux-next, as there are dependent patches not
>> yet merged, but present in linux-next like clock and reset patches.
>>
>> An open point in Patch 7/8 concerns the GPU memory clock (gpu_mem_clk),
>> defined as a fixed-clock. The specific hardware frequency for this clock
>> on the TH1520 could not be determined from available public
>> documentation. Consequently, clock-frequency = <0>; has been used as a
>> placeholder to enable driver functionality.
>>
>> Link to v2 of this series - [3].
>>
>> v3:
>>
>>  - re-worked cover letter completely
>>  - complete architectural rework from using extended genpd callbacks to a
>>    dedicated pwrseq provider driver
>>  - introduced pwrseq-thead-gpu.c and associated DT bindings
>>    (thead,th1520-gpu-pwrseq)
>>  - the Imagination driver now calls devm_pwrseq_get() and uses
>>    pwrseq_power_on() / pwrseq_power_off() for the TH1520 GPU
>>  - removed the platform_resources_managed flag from dev_pm_info and
>>    associated logic
>>  - the new pwrseq driver's match() function now acquires consumer-specific
>>    resources (GPU clocks, GPU core reset) directly from the consumer device
>>
>> v2:
>>
>> Extended the series by adding two new commits:
>>  - introduced a new platform_resources_managed flag in dev_pm_info along
>>    with helper functions, allowing drivers to detect when clocks and resets
>>    are managed by the platform
>>  - updated the DRM Imagination driver to skip claiming clocks when
>>    platform_resources_managed is set
>>
>> Split the original bindings update:
>>  - the AON firmware bindings now only add the GPU clkgen reset (the GPU
>>    core reset remains handled by the GPU node)
>>
>> Reworked the TH1520 PM domain driver to:
>>  - acquire GPU clocks and reset dynamically using attach_dev/detach_dev
>>    callbacks
>>  - handle clkgen reset internally, while GPU core reset is obtained from
>>    the consumer device node
>>  - added a check to enforce that only a single device can be attached to
>>    the GPU PM domain
>>
>> [1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
>> [2] - https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/
>> [3] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-0-70c5af2af96c@samsung.com/
>>
>> ---
>> Michal Wilczynski (8):
>>       dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer
>>       power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
>>       drm/imagination: Use pwrseq for TH1520 GPU power management
>>       dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings
>>       riscv: dts: thead: th1520: Add missing reset controller header include
>>       riscv: dts: thead: Add GPU power sequencer node
>>       riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node
>>       drm/imagination: Enable PowerVR driver for RISC-V
>>
>>  .../devicetree/bindings/gpu/img,powervr-rogue.yaml |   9 +-
>>  .../bindings/power/thead,th1520-pwrseq.yaml        |  42 +++++
>>  MAINTAINERS                                        |   2 +
>>  arch/riscv/boot/dts/thead/th1520.dtsi              |  29 ++++
>>  drivers/gpu/drm/imagination/Kconfig                |   3 +-
>>  drivers/gpu/drm/imagination/pvr_device.c           |  33 +++-
>>  drivers/gpu/drm/imagination/pvr_device.h           |   6 +
>>  drivers/gpu/drm/imagination/pvr_power.c            |  82 +++++----
>>  drivers/power/sequencing/Kconfig                   |   8 +
>>  drivers/power/sequencing/Makefile                  |   1 +
>>  drivers/power/sequencing/pwrseq-thead-gpu.c        | 183 +++++++++++++++++++++
>>  11 files changed, 363 insertions(+), 35 deletions(-)
>> ---
>> base-commit: 49473fe7fdb5fbbe5bbfa51083792c17df63d317
>> change-id: 20250414-apr_14_for_sending-5b3917817acc
>>
>> Best regards,
>> -- 
>> Michal Wilczynski <m.wilczynski@samsung.com>
>>
> 
> Thank you for continuing to work on this series.
> 
> I applied it to next-20250530 and the boot hangs:
> 
> <snip>
> [    0.895622] mmc0: new HS400 MMC card at address 0001
> [    0.902638] mmcblk0: mmc0:0001 8GTF4R 7.28 GiB
> [    0.915454]  mmcblk0: p1 p2 p3
> [    0.916613] debug_vm_pgtable: [debug_vm_pgtable         ]: Validating architecture page table helpers
> [    0.920107] mmcblk0boot0: mmc0:0001 8GTF4R 4.00 MiB
> [    0.936592] mmcblk0boot1: mmc0:0001 8GTF4R 4.00 MiB
> [    0.944986] mmcblk0rpmb: mmc0:0001 8GTF4R 512 KiB, chardev (243:0)
> [    0.947700] mmc1: new UHS-I speed DDR50 SDHC card at address aaaa
> [    0.961368] mmcblk1: mmc1:aaaa SU16G 14.8 GiB
> [    0.969639]  mmcblk1: p1 p2 p3
> [    0.986688] printk: legacy console [ttyS0] disabled
> [    0.992468] ffe7014000.serial: ttyS0 at MMIO 0xffe7014000 (irq = 23, base_baud = 6250000) is a 16550A
> [    1.002085] printk: legacy console [ttyS0] enabled
> [    1.002085] printk: legacy console [ttyS0] enabled
> [    1.011784] printk: legacy bootconsole [uart0] disabled
> [    1.011784] printk: legacy bootconsole [uart0] disabled
> [    1.024633] stackdepot: allocating hash table of 524288 entries via kvcalloc
> <no more output>
> 
> I pasted the full boot log [1]. I have clk_ignore_unused in the kernel
> cmdline so I don't think it is related to disabling clocks. Boot does
> complete okay if I set the gpu node status to disabled.
> 
> Any ideas of what might fix the boot hang?
> 
> Thanks,
> Drew

Hi,
Thanks a lot for testing and promptly providing debug data. I think the
problem is with the fallback logic implemented in the pvr_device.c:
	/*
	 * Try to get a power sequencer. If successful, it will handle clocks
	 * and resets. Otherwise, we fall back to managing them ourselves.
	 */
	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
	if (IS_ERR(pvr_dev->pwrseq)) {
		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);

		/*
		 * If the error is -EPROBE_DEFER, it's because the
		 * optional sequencer provider is not present
		 * and it's safe to fall back on manual power-up.
		 */
		if (pwrseq_err == -EPROBE_DEFER)
			pvr_dev->pwrseq = NULL;
		else
			return dev_err_probe(dev, pwrseq_err,
					     "Failed to get power sequencer\n");
	}


Since you have:
# CONFIG_POWER_SEQUENCING_THEAD_GPU is not set
The fallback logic assumes that there is no pwrseq provider for
'gpu-power' and falls back on generic driver to do the initial power
sequence. Obviously for TH1520 the generic driver fails to do that
correctly, and the register access hangs.

So the code seems to behave as designed.

By the way, there are quite a lot of Kconfig options added recently to
TH1520 SoC that haven't made it's way to defconfig for risc-v. Do you
think it's a good idea to add them there ?

> 
> [1] https://protect2.fireeye.com/v1/url?k=ac29f739-cda2e203-ac287c76-74fe4860008a-0bca62898bcc20b4&q=1&e=e86ea026-835e-453e-a61b-1d30c11073e1&u=https%3A%2F%2Fgist.github.com%2Fpdp7%2F44bd6de63fb9274a66a705ad807690b6
> 

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

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Bartosz Golaszewski]

On Fri, May 30, 2025 at 12:24 AM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> Introduce device tree bindings for a new power sequencer provider
> dedicated to the T-HEAD TH1520 SoC's GPU.
>
> The thead,th1520-gpu-pwrseq compatible designates a node that will
> manage the complex power-up and power-down sequence for the GPU. This
> sequencer requires a handle to the GPU's clock generator reset line
> (gpu-clkgen), which is specified in its device tree node.
>
> This binding will be used by a new pwrseq driver to abstract the
> SoC specific power management details from the generic GPU driver.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/power/thead,th1520-pwrseq.yaml        | 42 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 43 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..4c302abfb76fb9e243946f4eefa333c6b02e59d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/thead,th1520-pwrseq.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: T-HEAD TH1520 GPU Power Sequencer
> +
> +maintainers:
> +  - Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +description: |
> +  This binding describes the power sequencer for the T-HEAD TH1520 GPU.
> +  This sequencer handles the specific power-up and power-down sequences
> +  required by the GPU, including managing clocks and resets from both the
> +  sequencer and the GPU device itself.
> +
> +properties:
> +  compatible:
> +    const: thead,th1520-gpu-pwrseq
> +

Before I review the rest: is this actually a physical device that
takes care of the power sequencing? Some kind of a power management
unit for the GPU? If so, I bet it's not called "power sequencer" so
let's use its actual name as per the datasheet?

Bart

Re: [PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Drew Fustini]

On Mon, Jun 02, 2025 at 10:03:09AM +0200, Michal Wilczynski wrote:
> 
> 
> On 6/2/25 01:05, Drew Fustini wrote:
> > On Fri, May 30, 2025 at 12:23:47AM +0200, Michal Wilczynski wrote:
> >> This patch series introduces support for the Imagination IMG BXM-4-64
> >> GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
> >> managing the GPU's complex power-up and power-down sequence, which
> >> involves multiple clocks and resets.
> >>
> >> The TH1520 GPU requires a specific sequence to be followed for its
> >> clocks and resets to ensure correct operation. Initial discussions and
> >> an earlier version of this series explored managing this via the generic
> >> power domain (genpd) framework. However, following further discussions
> >> with kernel maintainers [1], the approach has been reworked to utilize
> >> the dedicated power sequencing (pwrseq) framework.
> >>
> >> This revised series now employs a new pwrseq provider driver
> >> (pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
> >> encapsulates the SoC specific power sequence details. The Imagination
> >> GPU driver (pvr_device.c) is updated to act as a consumer of this power
> >> sequencer, requesting the "gpu-power" target. The sequencer driver,
> >> during its match phase with the GPU device, acquires the necessary clock
> >> and reset handles from the GPU device node to perform the full sequence.
> >>
> >> This approach aligns with the goal of abstracting SoC specific power
> >> management details away from generic device drivers and leverages the
> >> pwrseq framework as recommended.
> >>
> >> The series is structured as follows:
> >>
> >> Patch 1: Adds device tree bindings for the new T-HEAD TH1520 GPU
> >>          power sequencer provider.
> >> Patch 2: Introduces the pwrseq-thead-gpu driver to manage the GPU's
> >>          power-on/off sequence.
> >> Patch 3: Updates the Imagination DRM driver to utilize the pwrseq
> >>          framework for TH1520 GPU power management.
> >> Patch 4: Adds the TH1520 GPU compatible string to the Imagination
> >>          GPU DT bindings.
> >> Patch 5: Adds the missing reset controller header include in the
> >>          TH1520 DTS include file.
> >> Patch 6: Adds the device tree node for the GPU power sequencer to
> >>          the TH1520 DTS include file.
> >> Patch 7: Adds the GPU device tree node for the IMG BXM-4-64 GPU to
> >>          the TH1520 DTS include file.
> >> Patch 8: Enables compilation of the drm/imagination on the RISC-V
> >>          architecture
> >>
> >> This patchset finishes the work started in bigger series [2] by adding
> >> all the remaining GPU power sequencing piece. After this patchset the GPU
> >> probes correctly.
> >>
> >> This series supersedes the previous genpd based approach. Testing on
> >> T-HEAD TH1520 SoC indicates the new pwrseq based solution works
> >> correctly.
> >>
> >> This time it's based on linux-next, as there are dependent patches not
> >> yet merged, but present in linux-next like clock and reset patches.
> >>
> >> An open point in Patch 7/8 concerns the GPU memory clock (gpu_mem_clk),
> >> defined as a fixed-clock. The specific hardware frequency for this clock
> >> on the TH1520 could not be determined from available public
> >> documentation. Consequently, clock-frequency = <0>; has been used as a
> >> placeholder to enable driver functionality.
> >>
> >> Link to v2 of this series - [3].
> >>
> >> v3:
> >>
> >>  - re-worked cover letter completely
> >>  - complete architectural rework from using extended genpd callbacks to a
> >>    dedicated pwrseq provider driver
> >>  - introduced pwrseq-thead-gpu.c and associated DT bindings
> >>    (thead,th1520-gpu-pwrseq)
> >>  - the Imagination driver now calls devm_pwrseq_get() and uses
> >>    pwrseq_power_on() / pwrseq_power_off() for the TH1520 GPU
> >>  - removed the platform_resources_managed flag from dev_pm_info and
> >>    associated logic
> >>  - the new pwrseq driver's match() function now acquires consumer-specific
> >>    resources (GPU clocks, GPU core reset) directly from the consumer device
> >>
> >> v2:
> >>
> >> Extended the series by adding two new commits:
> >>  - introduced a new platform_resources_managed flag in dev_pm_info along
> >>    with helper functions, allowing drivers to detect when clocks and resets
> >>    are managed by the platform
> >>  - updated the DRM Imagination driver to skip claiming clocks when
> >>    platform_resources_managed is set
> >>
> >> Split the original bindings update:
> >>  - the AON firmware bindings now only add the GPU clkgen reset (the GPU
> >>    core reset remains handled by the GPU node)
> >>
> >> Reworked the TH1520 PM domain driver to:
> >>  - acquire GPU clocks and reset dynamically using attach_dev/detach_dev
> >>    callbacks
> >>  - handle clkgen reset internally, while GPU core reset is obtained from
> >>    the consumer device node
> >>  - added a check to enforce that only a single device can be attached to
> >>    the GPU PM domain
> >>
> >> [1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
> >> [2] - https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/
> >> [3] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-0-70c5af2af96c@samsung.com/
> >>
> >> ---
> >> Michal Wilczynski (8):
> >>       dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer
> >>       power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
> >>       drm/imagination: Use pwrseq for TH1520 GPU power management
> >>       dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings
> >>       riscv: dts: thead: th1520: Add missing reset controller header include
> >>       riscv: dts: thead: Add GPU power sequencer node
> >>       riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node
> >>       drm/imagination: Enable PowerVR driver for RISC-V
> >>
> >>  .../devicetree/bindings/gpu/img,powervr-rogue.yaml |   9 +-
> >>  .../bindings/power/thead,th1520-pwrseq.yaml        |  42 +++++
> >>  MAINTAINERS                                        |   2 +
> >>  arch/riscv/boot/dts/thead/th1520.dtsi              |  29 ++++
> >>  drivers/gpu/drm/imagination/Kconfig                |   3 +-
> >>  drivers/gpu/drm/imagination/pvr_device.c           |  33 +++-
> >>  drivers/gpu/drm/imagination/pvr_device.h           |   6 +
> >>  drivers/gpu/drm/imagination/pvr_power.c            |  82 +++++----
> >>  drivers/power/sequencing/Kconfig                   |   8 +
> >>  drivers/power/sequencing/Makefile                  |   1 +
> >>  drivers/power/sequencing/pwrseq-thead-gpu.c        | 183 +++++++++++++++++++++
> >>  11 files changed, 363 insertions(+), 35 deletions(-)
> >> ---
> >> base-commit: 49473fe7fdb5fbbe5bbfa51083792c17df63d317
> >> change-id: 20250414-apr_14_for_sending-5b3917817acc
> >>
> >> Best regards,
> >> -- 
> >> Michal Wilczynski <m.wilczynski@samsung.com>
> >>
> > 
> > Thank you for continuing to work on this series.
> > 
> > I applied it to next-20250530 and the boot hangs:
> > 
> > <snip>
> > [    0.895622] mmc0: new HS400 MMC card at address 0001
> > [    0.902638] mmcblk0: mmc0:0001 8GTF4R 7.28 GiB
> > [    0.915454]  mmcblk0: p1 p2 p3
> > [    0.916613] debug_vm_pgtable: [debug_vm_pgtable         ]: Validating architecture page table helpers
> > [    0.920107] mmcblk0boot0: mmc0:0001 8GTF4R 4.00 MiB
> > [    0.936592] mmcblk0boot1: mmc0:0001 8GTF4R 4.00 MiB
> > [    0.944986] mmcblk0rpmb: mmc0:0001 8GTF4R 512 KiB, chardev (243:0)
> > [    0.947700] mmc1: new UHS-I speed DDR50 SDHC card at address aaaa
> > [    0.961368] mmcblk1: mmc1:aaaa SU16G 14.8 GiB
> > [    0.969639]  mmcblk1: p1 p2 p3
> > [    0.986688] printk: legacy console [ttyS0] disabled
> > [    0.992468] ffe7014000.serial: ttyS0 at MMIO 0xffe7014000 (irq = 23, base_baud = 6250000) is a 16550A
> > [    1.002085] printk: legacy console [ttyS0] enabled
> > [    1.002085] printk: legacy console [ttyS0] enabled
> > [    1.011784] printk: legacy bootconsole [uart0] disabled
> > [    1.011784] printk: legacy bootconsole [uart0] disabled
> > [    1.024633] stackdepot: allocating hash table of 524288 entries via kvcalloc
> > <no more output>
> > 
> > I pasted the full boot log [1]. I have clk_ignore_unused in the kernel
> > cmdline so I don't think it is related to disabling clocks. Boot does
> > complete okay if I set the gpu node status to disabled.
> > 
> > Any ideas of what might fix the boot hang?
> > 
> > Thanks,
> > Drew
> 
> Hi,
> Thanks a lot for testing and promptly providing debug data. I think the
> problem is with the fallback logic implemented in the pvr_device.c:
> 	/*
> 	 * Try to get a power sequencer. If successful, it will handle clocks
> 	 * and resets. Otherwise, we fall back to managing them ourselves.
> 	 */
> 	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
> 	if (IS_ERR(pvr_dev->pwrseq)) {
> 		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
> 
> 		/*
> 		 * If the error is -EPROBE_DEFER, it's because the
> 		 * optional sequencer provider is not present
> 		 * and it's safe to fall back on manual power-up.
> 		 */
> 		if (pwrseq_err == -EPROBE_DEFER)
> 			pvr_dev->pwrseq = NULL;
> 		else
> 			return dev_err_probe(dev, pwrseq_err,
> 					     "Failed to get power sequencer\n");
> 	}
> 
> 
> Since you have:
> # CONFIG_POWER_SEQUENCING_THEAD_GPU is not set
> The fallback logic assumes that there is no pwrseq provider for
> 'gpu-power' and falls back on generic driver to do the initial power
> sequence. Obviously for TH1520 the generic driver fails to do that
> correctly, and the register access hangs.

Ah! Yeah, I missed setting CONFIG_POWER_SEQUENCING_THEAD_GPU. The boot
completes okay now that is enabled.

> 
> So the code seems to behave as designed.
> 
> By the way, there are quite a lot of Kconfig options added recently to
> TH1520 SoC that haven't made it's way to defconfig for risc-v. Do you
> think it's a good idea to add them there ?

Yeah, I think we should have all the recent Kconfing options enabled by
default. I'm not sure if it should be in the riscv defconfig and as new
selects under ARCH_THEAD in Kconfig.socs. I just asked conor and palmer
on irc to see what would work best.

Thanks,
Drew

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Michal Wilczynski]

On 6/2/25 16:46, Bartosz Golaszewski wrote:
> On Fri, May 30, 2025 at 12:24 AM Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> Introduce device tree bindings for a new power sequencer provider
>> dedicated to the T-HEAD TH1520 SoC's GPU.
>>
>> The thead,th1520-gpu-pwrseq compatible designates a node that will
>> manage the complex power-up and power-down sequence for the GPU. This
>> sequencer requires a handle to the GPU's clock generator reset line
>> (gpu-clkgen), which is specified in its device tree node.
>>
>> This binding will be used by a new pwrseq driver to abstract the
>> SoC specific power management details from the generic GPU driver.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  .../bindings/power/thead,th1520-pwrseq.yaml        | 42 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4c302abfb76fb9e243946f4eefa333c6b02e59d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/thead,th1520-pwrseq.yaml
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://protect2.fireeye.com/v1/url?k=55ca3a77-34b7d20f-55cbb138-74fe485fffb1-4da99284aaf5bdf2&q=1&e=085ffc69-21ad-4abd-9147-970a308c8818&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpower%2Fthead%2Cth1520-pwrseq.yaml%23
>> +$schema: https://protect2.fireeye.com/v1/url?k=8e9b901c-efe67864-8e9a1b53-74fe485fffb1-c964471a6655716e&q=1&e=085ffc69-21ad-4abd-9147-970a308c8818&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>> +
>> +title: T-HEAD TH1520 GPU Power Sequencer
>> +
>> +maintainers:
>> +  - Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +description: |
>> +  This binding describes the power sequencer for the T-HEAD TH1520 GPU.
>> +  This sequencer handles the specific power-up and power-down sequences
>> +  required by the GPU, including managing clocks and resets from both the
>> +  sequencer and the GPU device itself.
>> +
>> +properties:
>> +  compatible:
>> +    const: thead,th1520-gpu-pwrseq
>> +
> 
> Before I review the rest: is this actually a physical device that
> takes care of the power sequencing? Some kind of a power management
> unit for the GPU? If so, I bet it's not called "power sequencer" so
> let's use its actual name as per the datasheet?

Hi Bart,
Thanks for your feedback. 

The hardware block responsible for powering up the components in the
TH1520 SoC datasheet is called AON (Always On). However, we already have
a DT node named aon that serves as a power domain provider
(Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml).

Following the discussion [1] about needing a separate DT node for the
power sequencing capabilities of this AON block, and thinking further
about it, I think the binding should be more generic. The AON block can
manage power sequences for more than just the GPU (e.g. NPU, AUDIO,
DSP).

The compatible string could be updated like so:
"thead,th1520-aon-pwrseq"

And the description:
"
  This binding describes the hardware capabilities within the Always-On
  (AON) block of the T-HEAD TH1520 SoC responsible for controlling and
  sequencing the power supply to various integrated peripherals, such as
  the GPU, NPU, Audio, and DSP.
"

The exact power architecture of the SoC is described in the chapter
6.3.2 (Power Architecture) [2]. The "VDEC/NPU/VENC/GPU/DSP Power Up/
Power Down" is described in chapter 6.4.2.3.

[1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
[2] - https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf

> 
> Bart
> 

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

Re: [PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Drew Fustini]

On Mon, Jun 02, 2025 at 10:33:07AM -0700, Drew Fustini wrote:
> On Mon, Jun 02, 2025 at 10:03:09AM +0200, Michal Wilczynski wrote:
> > 
> > 
> > On 6/2/25 01:05, Drew Fustini wrote:
> > > On Fri, May 30, 2025 at 12:23:47AM +0200, Michal Wilczynski wrote:
> > >> This patch series introduces support for the Imagination IMG BXM-4-64
> > >> GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
> > >> managing the GPU's complex power-up and power-down sequence, which
> > >> involves multiple clocks and resets.
> > >>
> > >> The TH1520 GPU requires a specific sequence to be followed for its
> > >> clocks and resets to ensure correct operation. Initial discussions and
> > >> an earlier version of this series explored managing this via the generic
> > >> power domain (genpd) framework. However, following further discussions
> > >> with kernel maintainers [1], the approach has been reworked to utilize
> > >> the dedicated power sequencing (pwrseq) framework.
> > >>
> > >> This revised series now employs a new pwrseq provider driver
> > >> (pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
> > >> encapsulates the SoC specific power sequence details. The Imagination
> > >> GPU driver (pvr_device.c) is updated to act as a consumer of this power
> > >> sequencer, requesting the "gpu-power" target. The sequencer driver,
> > >> during its match phase with the GPU device, acquires the necessary clock
> > >> and reset handles from the GPU device node to perform the full sequence.
> > >>
> > >> This approach aligns with the goal of abstracting SoC specific power
> > >> management details away from generic device drivers and leverages the
> > >> pwrseq framework as recommended.
> > >>
> > >> The series is structured as follows:
> > >>
> > >> Patch 1: Adds device tree bindings for the new T-HEAD TH1520 GPU
> > >>          power sequencer provider.
> > >> Patch 2: Introduces the pwrseq-thead-gpu driver to manage the GPU's
> > >>          power-on/off sequence.
> > >> Patch 3: Updates the Imagination DRM driver to utilize the pwrseq
> > >>          framework for TH1520 GPU power management.
> > >> Patch 4: Adds the TH1520 GPU compatible string to the Imagination
> > >>          GPU DT bindings.
> > >> Patch 5: Adds the missing reset controller header include in the
> > >>          TH1520 DTS include file.
> > >> Patch 6: Adds the device tree node for the GPU power sequencer to
> > >>          the TH1520 DTS include file.
> > >> Patch 7: Adds the GPU device tree node for the IMG BXM-4-64 GPU to
> > >>          the TH1520 DTS include file.
> > >> Patch 8: Enables compilation of the drm/imagination on the RISC-V
> > >>          architecture
> > >>
> > >> This patchset finishes the work started in bigger series [2] by adding
> > >> all the remaining GPU power sequencing piece. After this patchset the GPU
> > >> probes correctly.
> > >>
> > >> This series supersedes the previous genpd based approach. Testing on
> > >> T-HEAD TH1520 SoC indicates the new pwrseq based solution works
> > >> correctly.
> > >>
> > >> This time it's based on linux-next, as there are dependent patches not
> > >> yet merged, but present in linux-next like clock and reset patches.
> > >>
> > >> An open point in Patch 7/8 concerns the GPU memory clock (gpu_mem_clk),
> > >> defined as a fixed-clock. The specific hardware frequency for this clock
> > >> on the TH1520 could not be determined from available public
> > >> documentation. Consequently, clock-frequency = <0>; has been used as a
> > >> placeholder to enable driver functionality.
> > >>
> > >> Link to v2 of this series - [3].
> > >>
> > >> v3:
> > >>
> > >>  - re-worked cover letter completely
> > >>  - complete architectural rework from using extended genpd callbacks to a
> > >>    dedicated pwrseq provider driver
> > >>  - introduced pwrseq-thead-gpu.c and associated DT bindings
> > >>    (thead,th1520-gpu-pwrseq)
> > >>  - the Imagination driver now calls devm_pwrseq_get() and uses
> > >>    pwrseq_power_on() / pwrseq_power_off() for the TH1520 GPU
> > >>  - removed the platform_resources_managed flag from dev_pm_info and
> > >>    associated logic
> > >>  - the new pwrseq driver's match() function now acquires consumer-specific
> > >>    resources (GPU clocks, GPU core reset) directly from the consumer device
> > >>
> > >> v2:
> > >>
> > >> Extended the series by adding two new commits:
> > >>  - introduced a new platform_resources_managed flag in dev_pm_info along
> > >>    with helper functions, allowing drivers to detect when clocks and resets
> > >>    are managed by the platform
> > >>  - updated the DRM Imagination driver to skip claiming clocks when
> > >>    platform_resources_managed is set
> > >>
> > >> Split the original bindings update:
> > >>  - the AON firmware bindings now only add the GPU clkgen reset (the GPU
> > >>    core reset remains handled by the GPU node)
> > >>
> > >> Reworked the TH1520 PM domain driver to:
> > >>  - acquire GPU clocks and reset dynamically using attach_dev/detach_dev
> > >>    callbacks
> > >>  - handle clkgen reset internally, while GPU core reset is obtained from
> > >>    the consumer device node
> > >>  - added a check to enforce that only a single device can be attached to
> > >>    the GPU PM domain
> > >>
> > >> [1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
> > >> [2] - https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/
> > >> [3] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-0-70c5af2af96c@samsung.com/
> > >>
> > >> ---
> > >> Michal Wilczynski (8):
> > >>       dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer
> > >>       power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
> > >>       drm/imagination: Use pwrseq for TH1520 GPU power management
> > >>       dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings
> > >>       riscv: dts: thead: th1520: Add missing reset controller header include
> > >>       riscv: dts: thead: Add GPU power sequencer node
> > >>       riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node
> > >>       drm/imagination: Enable PowerVR driver for RISC-V
> > >>
> > >>  .../devicetree/bindings/gpu/img,powervr-rogue.yaml |   9 +-
> > >>  .../bindings/power/thead,th1520-pwrseq.yaml        |  42 +++++
> > >>  MAINTAINERS                                        |   2 +
> > >>  arch/riscv/boot/dts/thead/th1520.dtsi              |  29 ++++
> > >>  drivers/gpu/drm/imagination/Kconfig                |   3 +-
> > >>  drivers/gpu/drm/imagination/pvr_device.c           |  33 +++-
> > >>  drivers/gpu/drm/imagination/pvr_device.h           |   6 +
> > >>  drivers/gpu/drm/imagination/pvr_power.c            |  82 +++++----
> > >>  drivers/power/sequencing/Kconfig                   |   8 +
> > >>  drivers/power/sequencing/Makefile                  |   1 +
> > >>  drivers/power/sequencing/pwrseq-thead-gpu.c        | 183 +++++++++++++++++++++
> > >>  11 files changed, 363 insertions(+), 35 deletions(-)
> > >> ---
> > >> base-commit: 49473fe7fdb5fbbe5bbfa51083792c17df63d317
> > >> change-id: 20250414-apr_14_for_sending-5b3917817acc
> > >>
> > >> Best regards,
> > >> -- 
> > >> Michal Wilczynski <m.wilczynski@samsung.com>
> > >>
> > > 
> > > Thank you for continuing to work on this series.
> > > 
> > > I applied it to next-20250530 and the boot hangs:
> > > 
> > > <snip>
> > > [    0.895622] mmc0: new HS400 MMC card at address 0001
> > > [    0.902638] mmcblk0: mmc0:0001 8GTF4R 7.28 GiB
> > > [    0.915454]  mmcblk0: p1 p2 p3
> > > [    0.916613] debug_vm_pgtable: [debug_vm_pgtable         ]: Validating architecture page table helpers
> > > [    0.920107] mmcblk0boot0: mmc0:0001 8GTF4R 4.00 MiB
> > > [    0.936592] mmcblk0boot1: mmc0:0001 8GTF4R 4.00 MiB
> > > [    0.944986] mmcblk0rpmb: mmc0:0001 8GTF4R 512 KiB, chardev (243:0)
> > > [    0.947700] mmc1: new UHS-I speed DDR50 SDHC card at address aaaa
> > > [    0.961368] mmcblk1: mmc1:aaaa SU16G 14.8 GiB
> > > [    0.969639]  mmcblk1: p1 p2 p3
> > > [    0.986688] printk: legacy console [ttyS0] disabled
> > > [    0.992468] ffe7014000.serial: ttyS0 at MMIO 0xffe7014000 (irq = 23, base_baud = 6250000) is a 16550A
> > > [    1.002085] printk: legacy console [ttyS0] enabled
> > > [    1.002085] printk: legacy console [ttyS0] enabled
> > > [    1.011784] printk: legacy bootconsole [uart0] disabled
> > > [    1.011784] printk: legacy bootconsole [uart0] disabled
> > > [    1.024633] stackdepot: allocating hash table of 524288 entries via kvcalloc
> > > <no more output>
> > > 
> > > I pasted the full boot log [1]. I have clk_ignore_unused in the kernel
> > > cmdline so I don't think it is related to disabling clocks. Boot does
> > > complete okay if I set the gpu node status to disabled.
> > > 
> > > Any ideas of what might fix the boot hang?
> > > 
> > > Thanks,
> > > Drew
> > 
> > Hi,
> > Thanks a lot for testing and promptly providing debug data. I think the
> > problem is with the fallback logic implemented in the pvr_device.c:
> > 	/*
> > 	 * Try to get a power sequencer. If successful, it will handle clocks
> > 	 * and resets. Otherwise, we fall back to managing them ourselves.
> > 	 */
> > 	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
> > 	if (IS_ERR(pvr_dev->pwrseq)) {
> > 		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
> > 
> > 		/*
> > 		 * If the error is -EPROBE_DEFER, it's because the
> > 		 * optional sequencer provider is not present
> > 		 * and it's safe to fall back on manual power-up.
> > 		 */
> > 		if (pwrseq_err == -EPROBE_DEFER)
> > 			pvr_dev->pwrseq = NULL;
> > 		else
> > 			return dev_err_probe(dev, pwrseq_err,
> > 					     "Failed to get power sequencer\n");
> > 	}
> > 
> > 
> > Since you have:
> > # CONFIG_POWER_SEQUENCING_THEAD_GPU is not set
> > The fallback logic assumes that there is no pwrseq provider for
> > 'gpu-power' and falls back on generic driver to do the initial power
> > sequence. Obviously for TH1520 the generic driver fails to do that
> > correctly, and the register access hangs.
> 
> Ah! Yeah, I missed setting CONFIG_POWER_SEQUENCING_THEAD_GPU. The boot
> completes okay now that is enabled.
> 
> > 
> > So the code seems to behave as designed.
> > 
> > By the way, there are quite a lot of Kconfig options added recently to
> > TH1520 SoC that haven't made it's way to defconfig for risc-v. Do you
> > think it's a good idea to add them there ?
> 
> Yeah, I think we should have all the recent Kconfing options enabled by
> default. I'm not sure if it should be in the riscv defconfig and as new
> selects under ARCH_THEAD in Kconfig.socs. I just asked conor and palmer
> on irc to see what would work best.

Conor pointed out to me that 'select' is not appropriate for drivers. He
said we should put what the platform reasonably needs in defconfig, as a
module if possible. Palmer said that there are different opinions on the
purpose of defconfig, but he thinks it should have what is required to
boot common distros on common silicon.

Thanks,
Drew

Re: [PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Ulf Hansson]

On Fri, 30 May 2025 at 00:24, Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> This patch series introduces support for the Imagination IMG BXM-4-64
> GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
> managing the GPU's complex power-up and power-down sequence, which
> involves multiple clocks and resets.
>
> The TH1520 GPU requires a specific sequence to be followed for its
> clocks and resets to ensure correct operation. Initial discussions and
> an earlier version of this series explored managing this via the generic
> power domain (genpd) framework. However, following further discussions
> with kernel maintainers [1], the approach has been reworked to utilize
> the dedicated power sequencing (pwrseq) framework.
>
> This revised series now employs a new pwrseq provider driver
> (pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
> encapsulates the SoC specific power sequence details. The Imagination
> GPU driver (pvr_device.c) is updated to act as a consumer of this power
> sequencer, requesting the "gpu-power" target. The sequencer driver,
> during its match phase with the GPU device, acquires the necessary clock
> and reset handles from the GPU device node to perform the full sequence.
>
> This approach aligns with the goal of abstracting SoC specific power
> management details away from generic device drivers and leverages the
> pwrseq framework as recommended.

Just wanted to share my view. I have looked through the series and to
me this seems like the correct approach, nice work!

Feel free to add my Reviewed-by tag for the series, even if I think
there may be some comments to address from Bartosz etc.

[...]

Kind regards
Uffe

Re: [PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[Ulf Hansson]

On Fri, 30 May 2025 at 00:24, Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
> the GPU using the drm/imagination driver.
>
> By adding this node, the kernel can recognize and initialize the GPU,
> providing graphics acceleration capabilities on the Lichee Pi 4A and
> other boards based on the TH1520 SoC.
>
> Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
> controlled programatically.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
>                 #clock-cells = <0>;
>         };
>
> +       gpu_mem_clk: mem-clk {
> +               compatible = "fixed-clock";
> +               clock-frequency = <0>;
> +               clock-output-names = "gpu_mem_clk";
> +               #clock-cells = <0>;
> +       };
> +
>         stmmac_axi_config: stmmac-axi-config {
>                 snps,wr_osr_lmt = <15>;
>                 snps,rd_osr_lmt = <15>;
> @@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
>                         #clock-cells = <1>;
>                 };
>
> +               gpu: gpu@ffef400000 {
> +                       compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
> +                                    "img,img-rogue";
> +                       reg = <0xff 0xef400000 0x0 0x100000>;
> +                       interrupt-parent = <&plic>;
> +                       interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&clk_vo CLK_GPU_CORE>,
> +                                <&gpu_mem_clk>,
> +                                <&clk_vo CLK_GPU_CFG_ACLK>;
> +                       clock-names = "core", "mem", "sys";
> +                       power-domains = <&aon TH1520_GPU_PD>;
> +                       power-domain-names = "a";

If the power-domain-names are really needed, please pick a
useful/descriptive name.

[...]

Kind regards
Uffe

Re: [PATCH v3 4/8] dt-bindings: gpu: Add TH1520 GPU compatible to Imagination bindings

[Krzysztof Kozlowski]

On Fri, May 30, 2025 at 12:23:51AM GMT, Michal Wilczynski wrote:
> Update the img,powervr-rogue.yaml device tree bindings to include the
> T-HEAD TH1520 SoC's specific GPU compatible string.
> 
> The thead,th1520-gpu compatible, along with its full chain
> img,img-bxm-4-64, and img,img-rogue, is added to the
> list of recognized GPU types. This allows the Imagination DRM driver
> to correctly bind to the GPU node defined in the TH1520 device tree.

If there is going to be any new version:

Last sentence feels redundant or obvious. That's the point of DT anyway,
so not need to state that. I suggest dropping it or replacing with some
hardware description.

A nit, subject: drop second/last, redundant "to Imagination bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Use proper prefix, so:
dt-bindings: gpu: img,powervr-rogue: Add foo bar

> 
> The power-domains property requirement for img,img-bxm-4-64 is also
> ensured by adding it to the relevant allOf condition.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index 4450e2e73b3ccf74d29f0e31e2e6687d7cbe5d65..c12837a0d39b8c3043b9133d444cc33a59135c33 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -26,6 +26,11 @@ properties:
>                - ti,j721s2-gpu
>            - const: img,img-bxs-4-64
>            - const: img,img-rogue
> +      - items:
> +          - enum:
> +              - thead,th1520-gpu
> +          - const: img,img-bxm-4-64

We keep ordering by fallbacks, so this should go above items with
img,img-bxs-4-64 fallback (m < s).

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

Best regards,
Krzysztof

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Krzysztof Kozlowski]

On Mon, Jun 02, 2025 at 10:29:13PM GMT, Michal Wilczynski wrote:
> >> +description: |
> >> +  This binding describes the power sequencer for the T-HEAD TH1520 GPU.
> >> +  This sequencer handles the specific power-up and power-down sequences
> >> +  required by the GPU, including managing clocks and resets from both the
> >> +  sequencer and the GPU device itself.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: thead,th1520-gpu-pwrseq
> >> +
> > 
> > Before I review the rest: is this actually a physical device that
> > takes care of the power sequencing? Some kind of a power management
> > unit for the GPU? If so, I bet it's not called "power sequencer" so
> > let's use its actual name as per the datasheet?
> 
> Hi Bart,
> Thanks for your feedback. 
> 
> The hardware block responsible for powering up the components in the
> TH1520 SoC datasheet is called AON (Always On). However, we already have
> a DT node named aon that serves as a power domain provider
> (Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml).

So no. One device, one device node (sometimes with cildren nodes). You
do not get another device node just because someone wrote incomplete
binding or because driver looks differently.

> 
> Following the discussion [1] about needing a separate DT node for the
> power sequencing capabilities of this AON block, and thinking further
> about it, I think the binding should be more generic. The AON block can
> manage power sequences for more than just the GPU (e.g. NPU, AUDIO,
> DSP).
> 
> The compatible string could be updated like so:
> "thead,th1520-aon-pwrseq"

Should not be separate node, you already have one for AON.

Best regards,
Krzysztof

Re: [PATCH v3 5/8] riscv: dts: thead: th1520: Add missing reset controller header include

[Krzysztof Kozlowski]

On Fri, May 30, 2025 at 12:23:52AM GMT, Michal Wilczynski wrote:
> TH1520_RESET_ID_GPU_CLKGEN and TH1520_RESET_ID_GPU are required for GPU
> power sequencing to work.  To make these symbols available, add the
> necessary include for the T-HEAD TH1520 reset controller bindings.

How would it compile/build without it? If there are no users, then do
not add unused header just to add it.

Best regards,
Krzysztof

Re: [PATCH v3 6/8] riscv: dts: thead: Add GPU power sequencer node

[Krzysztof Kozlowski]

On Fri, May 30, 2025 at 12:23:53AM GMT, Michal Wilczynski wrote:
> Add the device tree node for the T-HEAD TH1520 GPU power sequencer
> (gpu_pwrseq) to the th1520.dtsi file.
> 
> This node instantiates the thead,th1520-gpu-pwrseq driver, which

Explain the hardware, not what drivers do.

> is responsible for managing the GPU's power-on/off sequence. The node
> specifies the gpu-clkgen reset, which is one of the resources
> controlled by this sequencer.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index bdbb1b985b0b76cf669a9bf40c6ec37258329056..6170eec79e919b606a2046ac8f52db07e47ef441 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -238,6 +238,12 @@ aon: aon {
>  		#power-domain-cells = <1>;
>  	};
>  
> +	gpu_pwrseq: pwrseq {

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

> +		compatible = "thead,th1520-gpu-pwrseq";
> +		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
> +		reset-names = "gpu-clkgen";

What is the point of pwrseq if there is no consumer/user of it? Looks
like simple placeholder and anyway maybe the future consumer should just
use reset directly.

Best regards,
Krzysztof

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On Fri, May 30, 2025 at 12:23:50AM GMT, Michal Wilczynski wrote:
> Update the Imagination PVR DRM driver to leverage the pwrseq framework
> for managing the power sequence of the GPU on the T-HEAD TH1520 SoC.
> 
> In pvr_device_init(), the driver now attempts to get a handle to the
> "gpu-power" sequencer target using devm_pwrseq_get(). If successful,
> the responsibility for powering on and off the GPU's core clocks and
> resets is delegated to the power sequencer. Consequently, the GPU
> driver conditionally skips acquiring the GPU reset line if the pwrseq
> handle is obtained, as the sequencer's match function will acquire it.
> Clock handles are still acquired by the GPU driver for other purposes
> like devfreq.
> 
> The runtime PM callbacks, pvr_power_device_resume() and
> pvr_power_device_suspend(), are modified to call pwrseq_power_on() and
> pwrseq_power_off() respectively when the sequencer is present.  If no
> sequencer is found, the driver falls back to its existing manual clock
> and reset management. A helper function,
> pvr_power_off_sequence_manual(), is introduced to encapsulate the manual
> power-down logic.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/gpu/drm/imagination/Kconfig      |  1 +
>  drivers/gpu/drm/imagination/pvr_device.c | 33 +++++++++++--
>  drivers/gpu/drm/imagination/pvr_device.h |  6 +++
>  drivers/gpu/drm/imagination/pvr_power.c  | 82 +++++++++++++++++++++-----------
>  4 files changed, 89 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
> index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..737ace77c4f1247c687cc1fde2f139fc2e118c50 100644
> --- a/drivers/gpu/drm/imagination/Kconfig
> +++ b/drivers/gpu/drm/imagination/Kconfig
> @@ -11,6 +11,7 @@ config DRM_POWERVR
>  	select DRM_SCHED
>  	select DRM_GPUVM
>  	select FW_LOADER
> +  select POWER_SEQUENCING

Messed indent.

>  	help
>  	  Choose this option if you have a system that has an Imagination
>  	  Technologies PowerVR (Series 6 or later) or IMG GPU.
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..19d48bbc828cf2b8dbead602e90ff88780152124 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/pwrseq/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/stddef.h>
> @@ -631,10 +632,34 @@ 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;
> +	/*
> +	 * Try to get a power sequencer. If successful, it will handle clocks
> +	 * and resets. Otherwise, we fall back to managing them ourselves.
> +	 */
> +	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
> +	if (IS_ERR(pvr_dev->pwrseq)) {
> +		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
> +
> +		/*
> +		 * If the error is -EPROBE_DEFER, it's because the
> +		 * optional sequencer provider is not present
> +		 * and it's safe to fall back on manual power-up.

It is safe but why it is desirable? The rule is rather to defer the
probe, assuming this is probe path.

Best regards,
Krzysztof

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Bartosz Golaszewski]

On Tue, Jun 3, 2025 at 3:19 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Jun 02, 2025 at 10:29:13PM GMT, Michal Wilczynski wrote:
> > >> +description: |
> > >> +  This binding describes the power sequencer for the T-HEAD TH1520 GPU.
> > >> +  This sequencer handles the specific power-up and power-down sequences
> > >> +  required by the GPU, including managing clocks and resets from both the
> > >> +  sequencer and the GPU device itself.
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    const: thead,th1520-gpu-pwrseq
> > >> +
> > >
> > > Before I review the rest: is this actually a physical device that
> > > takes care of the power sequencing? Some kind of a power management
> > > unit for the GPU? If so, I bet it's not called "power sequencer" so
> > > let's use its actual name as per the datasheet?
> >
> > Hi Bart,
> > Thanks for your feedback.
> >
> > The hardware block responsible for powering up the components in the
> > TH1520 SoC datasheet is called AON (Always On). However, we already have
> > a DT node named aon that serves as a power domain provider
> > (Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml).
>
> So no. One device, one device node (sometimes with cildren nodes). You
> do not get another device node just because someone wrote incomplete
> binding or because driver looks differently.
>
> >
> > Following the discussion [1] about needing a separate DT node for the
> > power sequencing capabilities of this AON block, and thinking further
> > about it, I think the binding should be more generic. The AON block can
> > manage power sequences for more than just the GPU (e.g. NPU, AUDIO,
> > DSP).
> >
> > The compatible string could be updated like so:
> > "thead,th1520-aon-pwrseq"
>
> Should not be separate node, you already have one for AON.
>

Agreed. And as far as implementation goes, you can have the same
driver be a PM domain AND pwrseq provider. It just has to bind to the
device node that represents an actual component, not a made-up
"convenience" node.

Bartosz

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Bartosz Golaszewski]

On Tue, Jun 3, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > The compatible string could be updated like so:
> > > "thead,th1520-aon-pwrseq"
> >
> > Should not be separate node, you already have one for AON.
> >
>
> Agreed. And as far as implementation goes, you can have the same
> driver be a PM domain AND pwrseq provider. It just has to bind to the
> device node that represents an actual component, not a made-up
> "convenience" node.
>

I'm seeing that there's already a main driver under
drivers/pmdomain/thead/th1520-pm-domains.c and a "logical sub-driver"
under drivers/firmware/thead,th1520-aon.c which exposes
th1520_aon_init() called by the former. Maybe just follow that
pattern, add a module under drivers/power/sequencing/ called
pwrseq-th1520-pwrseq.c and call its init function from the pm-domains
module?

Bart

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Michal Wilczynski]

On 6/3/25 15:35, Bartosz Golaszewski wrote:
> On Tue, Jun 3, 2025 at 3:19 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Mon, Jun 02, 2025 at 10:29:13PM GMT, Michal Wilczynski wrote:
>>>>> +description: |
>>>>> +  This binding describes the power sequencer for the T-HEAD TH1520 GPU.
>>>>> +  This sequencer handles the specific power-up and power-down sequences
>>>>> +  required by the GPU, including managing clocks and resets from both the
>>>>> +  sequencer and the GPU device itself.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: thead,th1520-gpu-pwrseq
>>>>> +
>>>>
>>>> Before I review the rest: is this actually a physical device that
>>>> takes care of the power sequencing? Some kind of a power management
>>>> unit for the GPU? If so, I bet it's not called "power sequencer" so
>>>> let's use its actual name as per the datasheet?
>>>
>>> Hi Bart,
>>> Thanks for your feedback.
>>>
>>> The hardware block responsible for powering up the components in the
>>> TH1520 SoC datasheet is called AON (Always On). However, we already have
>>> a DT node named aon that serves as a power domain provider
>>> (Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml).
>>
>> So no. One device, one device node (sometimes with cildren nodes). You
>> do not get another device node just because someone wrote incomplete
>> binding or because driver looks differently.

The child option could potentially be an option here, as it would make
some sense, at least for me - since the child pwrseq DT node would
describe part of the AON block.

>>
>>>
>>> Following the discussion [1] about needing a separate DT node for the
>>> power sequencing capabilities of this AON block, and thinking further
>>> about it, I think the binding should be more generic. The AON block can
>>> manage power sequences for more than just the GPU (e.g. NPU, AUDIO,
>>> DSP).
>>>
>>> The compatible string could be updated like so:
>>> "thead,th1520-aon-pwrseq"
>>
>> Should not be separate node, you already have one for AON.
>>
> 
> Agreed. And as far as implementation goes, you can have the same
> driver be a PM domain AND pwrseq provider. It just has to bind to the
> device node that represents an actual component, not a made-up
> "convenience" node.

Sure - this can be done using existing AON node.

To keep the pwrseq code organized in drivers/power/sequencing/, a
similar approach to our th1520-pd driver interfacing with the AON
firmware library (drivers/firmware/thead,th1520-aon.c) could work.

The idea would be to treat code like pwrseq-thead-aon.c (changed from a
current pwrseq-thead-gpu.c) as a library. It would export its necessary
functions (e.g., for specific sequence init/deinit steps) using
EXPORT_SYMBOL_GPL. The main AON driver would then call these to provide
the pwrseq functionality.

This will introduce a compile-time dependency, as expected.

An alternative would be to keep the driver in drivers/power/sequencing/
as a platform driver and start it up using, for example, an auxiliary
bus. This is similar to what the JH7110 clock driver
(drivers/clk/starfive/clk-starfive-jh7110-sys.c) is doing with a reset
driver. This could offer a cleaner separation of roles if that's
preferred.

Please let me know which way would be preferred.

> 
> Bartosz
> 

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

Re: [PATCH v3 5/8] riscv: dts: thead: th1520: Add missing reset controller header include

[Michal Wilczynski]

On 6/3/25 15:20, Krzysztof Kozlowski wrote:
> On Fri, May 30, 2025 at 12:23:52AM GMT, Michal Wilczynski wrote:
>> TH1520_RESET_ID_GPU_CLKGEN and TH1520_RESET_ID_GPU are required for GPU
>> power sequencing to work.  To make these symbols available, add the
>> necessary include for the T-HEAD TH1520 reset controller bindings.
> 
> How would it compile/build without it? If there are no users, then do
> not add unused header just to add it.

The patch 7 in the series need it, so I've added the header.

> 
> Best regards,
> Krzysztof
> 
> 

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

Re: [PATCH v3 0/8] Add TH1520 GPU support with power sequencing

[Michal Wilczynski]

On 6/3/25 14:25, Ulf Hansson wrote:
> On Fri, 30 May 2025 at 00:24, Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> This patch series introduces support for the Imagination IMG BXM-4-64
>> GPU found on the T-HEAD TH1520 SoC. A key aspect of this support is
>> managing the GPU's complex power-up and power-down sequence, which
>> involves multiple clocks and resets.
>>
>> The TH1520 GPU requires a specific sequence to be followed for its
>> clocks and resets to ensure correct operation. Initial discussions and
>> an earlier version of this series explored managing this via the generic
>> power domain (genpd) framework. However, following further discussions
>> with kernel maintainers [1], the approach has been reworked to utilize
>> the dedicated power sequencing (pwrseq) framework.
>>
>> This revised series now employs a new pwrseq provider driver
>> (pwrseq-thead-gpu.c) specifically for the TH1520 GPU. This driver
>> encapsulates the SoC specific power sequence details. The Imagination
>> GPU driver (pvr_device.c) is updated to act as a consumer of this power
>> sequencer, requesting the "gpu-power" target. The sequencer driver,
>> during its match phase with the GPU device, acquires the necessary clock
>> and reset handles from the GPU device node to perform the full sequence.
>>
>> This approach aligns with the goal of abstracting SoC specific power
>> management details away from generic device drivers and leverages the
>> pwrseq framework as recommended.
> 
> Just wanted to share my view. I have looked through the series and to
> me this seems like the correct approach, nice work!
> 
> Feel free to add my Reviewed-by tag for the series, even if I think
> there may be some comments to address from Bartosz etc.

Thanks for providing the direction and reviewing the code, the pwrseq
API seems to be perfect to achieve the goals of this series. Appreciate
your help !

> 
> [...]
> 
> Kind regards
> Uffe
> 

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

Re: [PATCH v3 6/8] riscv: dts: thead: Add GPU power sequencer node

[Michal Wilczynski]

On 6/3/25 15:22, Krzysztof Kozlowski wrote:
> On Fri, May 30, 2025 at 12:23:53AM GMT, Michal Wilczynski wrote:
>> Add the device tree node for the T-HEAD TH1520 GPU power sequencer
>> (gpu_pwrseq) to the th1520.dtsi file.
>>
>> This node instantiates the thead,th1520-gpu-pwrseq driver, which
> 
> Explain the hardware, not what drivers do.
> 
>> is responsible for managing the GPU's power-on/off sequence. The node
>> specifies the gpu-clkgen reset, which is one of the resources
>> controlled by this sequencer.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index bdbb1b985b0b76cf669a9bf40c6ec37258329056..6170eec79e919b606a2046ac8f52db07e47ef441 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -238,6 +238,12 @@ aon: aon {
>>  		#power-domain-cells = <1>;
>>  	};
>>  
>> +	gpu_pwrseq: pwrseq {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://protect2.fireeye.com/v1/url?k=a53ea5d3-c4434f50-a53f2e9c-74fe48600158-c81092475ef416b3&q=1&e=d333d06b-0b06-493e-a358-e29ca542dfe7&u=https%3A%2F%2Fdevicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.html%23generic-names-recommendation
> 
>> +		compatible = "thead,th1520-gpu-pwrseq";
>> +		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
>> +		reset-names = "gpu-clkgen";
> 
> What is the point of pwrseq if there is no consumer/user of it? Looks
> like simple placeholder and anyway maybe the future consumer should just
> use reset directly.

Yeah I think you're right, I wanted to explore adding the pwrseq
provider in separate node per discussion in v2 [1]. But for the v4 I
think I'll revert to the v2 way of handling this reset [2].

[1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
[2] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-2-70c5af2af96c@samsung.com/

> 
> Best regards,
> Krzysztof
> 
> 

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

Re: [PATCH v3 5/8] riscv: dts: thead: th1520: Add missing reset controller header include

[Krzysztof Kozlowski]

On 03/06/2025 20:26, Michal Wilczynski wrote:
> 
> 
> On 6/3/25 15:20, Krzysztof Kozlowski wrote:
>> On Fri, May 30, 2025 at 12:23:52AM GMT, Michal Wilczynski wrote:
>>> TH1520_RESET_ID_GPU_CLKGEN and TH1520_RESET_ID_GPU are required for GPU
>>> power sequencing to work.  To make these symbols available, add the
>>> necessary include for the T-HEAD TH1520 reset controller bindings.
>>
>> How would it compile/build without it? If there are no users, then do
>> not add unused header just to add it.
> 
> The patch 7 in the series need it, so I've added the header.

Then this should be squashed with patch 7. Adding unused header is not a
logical change on its own.

Best regards,
Krzysztof

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/3/25 15:28, Krzysztof Kozlowski wrote:
> On Fri, May 30, 2025 at 12:23:50AM GMT, Michal Wilczynski wrote:
>> Update the Imagination PVR DRM driver to leverage the pwrseq framework
>> for managing the power sequence of the GPU on the T-HEAD TH1520 SoC.
>>
>> In pvr_device_init(), the driver now attempts to get a handle to the
>> "gpu-power" sequencer target using devm_pwrseq_get(). If successful,
>> the responsibility for powering on and off the GPU's core clocks and
>> resets is delegated to the power sequencer. Consequently, the GPU
>> driver conditionally skips acquiring the GPU reset line if the pwrseq
>> handle is obtained, as the sequencer's match function will acquire it.
>> Clock handles are still acquired by the GPU driver for other purposes
>> like devfreq.
>>
>> The runtime PM callbacks, pvr_power_device_resume() and
>> pvr_power_device_suspend(), are modified to call pwrseq_power_on() and
>> pwrseq_power_off() respectively when the sequencer is present.  If no
>> sequencer is found, the driver falls back to its existing manual clock
>> and reset management. A helper function,
>> pvr_power_off_sequence_manual(), is introduced to encapsulate the manual
>> power-down logic.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/gpu/drm/imagination/Kconfig      |  1 +
>>  drivers/gpu/drm/imagination/pvr_device.c | 33 +++++++++++--
>>  drivers/gpu/drm/imagination/pvr_device.h |  6 +++
>>  drivers/gpu/drm/imagination/pvr_power.c  | 82 +++++++++++++++++++++-----------
>>  4 files changed, 89 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
>> index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..737ace77c4f1247c687cc1fde2f139fc2e118c50 100644
>> --- a/drivers/gpu/drm/imagination/Kconfig
>> +++ b/drivers/gpu/drm/imagination/Kconfig
>> @@ -11,6 +11,7 @@ config DRM_POWERVR
>>  	select DRM_SCHED
>>  	select DRM_GPUVM
>>  	select FW_LOADER
>> +  select POWER_SEQUENCING
> 
> Messed indent.
> 
>>  	help
>>  	  Choose this option if you have a system that has an Imagination
>>  	  Technologies PowerVR (Series 6 or later) or IMG GPU.
>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
>> index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..19d48bbc828cf2b8dbead602e90ff88780152124 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/pwrseq/consumer.h>
>>  #include <linux/reset.h>
>>  #include <linux/slab.h>
>>  #include <linux/stddef.h>
>> @@ -631,10 +632,34 @@ 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;
>> +	/*
>> +	 * Try to get a power sequencer. If successful, it will handle clocks
>> +	 * and resets. Otherwise, we fall back to managing them ourselves.
>> +	 */
>> +	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
>> +	if (IS_ERR(pvr_dev->pwrseq)) {
>> +		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
>> +
>> +		/*
>> +		 * If the error is -EPROBE_DEFER, it's because the
>> +		 * optional sequencer provider is not present
>> +		 * and it's safe to fall back on manual power-up.
> 
> It is safe but why it is desirable? The rule is rather to defer the
> probe, assuming this is probe path.

Yeah this is probe path.

The GPU node will depend on the AON node, which will be the sole
provider for the 'gpu-power' sequencer (based on the discussion in patch
1).

Therefore, if the AON/pwrseq driver has already completed its probe, and
devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
(because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
interpretation is that the AON driver did not register this optional
sequencer. Since AON is the only anticipated source, it implies the
sequencer won't become available later from its designated provider.
This specific scenario is when we intend to fall back to manual power
management, which pvr driver is capable of, it just doesn't work for the
TH1520 SoC, as it has a custom requirements implemented by the pwrseq
driver.

> 
> Best regards,
> Krzysztof
> 
> 

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

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Michal Wilczynski]

On 6/3/25 16:49, Bartosz Golaszewski wrote:
> On Tue, Jun 3, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>
>>>> The compatible string could be updated like so:
>>>> "thead,th1520-aon-pwrseq"
>>>
>>> Should not be separate node, you already have one for AON.
>>>
>>
>> Agreed. And as far as implementation goes, you can have the same
>> driver be a PM domain AND pwrseq provider. It just has to bind to the
>> device node that represents an actual component, not a made-up
>> "convenience" node.
>>
> 
> I'm seeing that there's already a main driver under
> drivers/pmdomain/thead/th1520-pm-domains.c and a "logical sub-driver"
> under drivers/firmware/thead,th1520-aon.c which exposes
> th1520_aon_init() called by the former. Maybe just follow that
> pattern, add a module under drivers/power/sequencing/ called
> pwrseq-th1520-pwrseq.c and call its init function from the pm-domains
> module?

Right, sorry I haven't noticed this and responded to previous message.
Thanks for the direction !

> 
> Bart
> 

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On 03/06/2025 21:43, Michal Wilczynski wrote:
>>> +	 * and resets. Otherwise, we fall back to managing them ourselves.
>>> +	 */
>>> +	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
>>> +	if (IS_ERR(pvr_dev->pwrseq)) {
>>> +		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
>>> +
>>> +		/*
>>> +		 * If the error is -EPROBE_DEFER, it's because the
>>> +		 * optional sequencer provider is not present
>>> +		 * and it's safe to fall back on manual power-up.
>>
>> It is safe but why it is desirable? The rule is rather to defer the
>> probe, assuming this is probe path.
> 
> Yeah this is probe path.
> 
> The GPU node will depend on the AON node, which will be the sole
> provider for the 'gpu-power' sequencer (based on the discussion in patch
> 1).
> 
> Therefore, if the AON/pwrseq driver has already completed its probe, and
> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
> interpretation is that the AON driver did not register this optional
> sequencer. Since AON is the only anticipated source, it implies the
> sequencer won't become available later from its designated provider.

I don't understand why you made this assumption. AON could be a module
and this driver built-in. AON will likely probe later.



Best regards,
Krzysztof

Re: [PATCH v3 1/8] dt-bindings: power: Add T-HEAD TH1520 GPU power sequencer

[Bartosz Golaszewski]

On Tue, Jun 3, 2025 at 8:24 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
> >
> > Agreed. And as far as implementation goes, you can have the same
> > driver be a PM domain AND pwrseq provider. It just has to bind to the
> > device node that represents an actual component, not a made-up
> > "convenience" node.
>
> Sure - this can be done using existing AON node.
>
> To keep the pwrseq code organized in drivers/power/sequencing/, a
> similar approach to our th1520-pd driver interfacing with the AON
> firmware library (drivers/firmware/thead,th1520-aon.c) could work.
>
> The idea would be to treat code like pwrseq-thead-aon.c (changed from a
> current pwrseq-thead-gpu.c) as a library. It would export its necessary
> functions (e.g., for specific sequence init/deinit steps) using
> EXPORT_SYMBOL_GPL. The main AON driver would then call these to provide
> the pwrseq functionality.
>
> This will introduce a compile-time dependency, as expected.
>
> An alternative would be to keep the driver in drivers/power/sequencing/
> as a platform driver and start it up using, for example, an auxiliary
> bus. This is similar to what the JH7110 clock driver
> (drivers/clk/starfive/clk-starfive-jh7110-sys.c) is doing with a reset
> driver. This could offer a cleaner separation of roles if that's
> preferred.
>
> Please let me know which way would be preferred.

I forgot the auxiliary bus is a thing. Yeah, definitely use that, it's
more elegant than a library function IMO.

Bart

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/4/25 08:36, Krzysztof Kozlowski wrote:
> On 03/06/2025 21:43, Michal Wilczynski wrote:
>>>> +	 * and resets. Otherwise, we fall back to managing them ourselves.
>>>> +	 */
>>>> +	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
>>>> +	if (IS_ERR(pvr_dev->pwrseq)) {
>>>> +		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
>>>> +
>>>> +		/*
>>>> +		 * If the error is -EPROBE_DEFER, it's because the
>>>> +		 * optional sequencer provider is not present
>>>> +		 * and it's safe to fall back on manual power-up.
>>>
>>> It is safe but why it is desirable? The rule is rather to defer the
>>> probe, assuming this is probe path.
>>
>> Yeah this is probe path.
>>
>> The GPU node will depend on the AON node, which will be the sole
>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>> 1).
>>
>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>> interpretation is that the AON driver did not register this optional
>> sequencer. Since AON is the only anticipated source, it implies the
>> sequencer won't become available later from its designated provider.
> 
> I don't understand why you made this assumption. AON could be a module
> and this driver built-in. AON will likely probe later.

You're absolutely right that AON could be a module and would generally
probe later in that scenario. However, the GPU device also has a
'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
the PM domain provider) were a late probing module, the GPU driver's
probe would hit -EPROBE_DEFER when its power domain is requested
which happens before attempting to get other resources like a power
sequencer.

So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
"gpu-power") call, it strongly implies the AON driver has already
successfully probed.

This leads to the core challenge with the optional 'gpu-power'
sequencer: Even if the AON driver has already probed, if it then chooses
not to register the "gpu-power" sequence (because it's an optional
feature), pwrseq_get() will still find "no device matched" on the
pwrseq_bus and return EPROBE_DEFER.

If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
it could wait indefinitely for an optional sequence that its
already probed AON provider will not supply.

Anyway I think you're right, that this is probably confusing and we
shouldn't rely on this behavior.

To solve this, and to allow the GPU driver to correctly handle
-EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
boolean property on the GPU's DT node, e.g.
img,gpu-expects-power-sequencer. If the GPU node provides this property
it means the pwrseq 'gpu-power' is required.

I didn't want to use this approach at first, as it seemed to me like the
pwrseq API had a hands-off approach for the DT with its matching logic,
but it seems unavoidable at this point.

> 
> 
> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On 04/06/2025 13:53, Michal Wilczynski wrote:
>>>
>>> The GPU node will depend on the AON node, which will be the sole
>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>>> 1).
>>>
>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>>> interpretation is that the AON driver did not register this optional
>>> sequencer. Since AON is the only anticipated source, it implies the
>>> sequencer won't become available later from its designated provider.
>>
>> I don't understand why you made this assumption. AON could be a module
>> and this driver built-in. AON will likely probe later.
> 
> You're absolutely right that AON could be a module and would generally
> probe later in that scenario. However, the GPU device also has a
> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
> the PM domain provider) were a late probing module, the GPU driver's
> probe would hit -EPROBE_DEFER when its power domain is requested
> which happens before attempting to get other resources like a power
> sequencer.

Huh, so basically you imply certain hardware design and certain DTS
description in your driver code. Well, that's clearly fragile design to
me, because you should not rely how hardware properties are presented in
DTS. Will work here on th1520 with this DTS, won't work with something else.

Especially that this looks like generic Imagination GPU code, common to
multiple devices, not TH1520 only specific.

> 
> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
> "gpu-power") call, it strongly implies the AON driver has already
> successfully probed.
> 
> This leads to the core challenge with the optional 'gpu-power'
> sequencer: Even if the AON driver has already probed, if it then chooses
> not to register the "gpu-power" sequence (because it's an optional
> feature), pwrseq_get() will still find "no device matched" on the
> pwrseq_bus and return EPROBE_DEFER.
> 
> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
> it could wait indefinitely for an optional sequence that its
> already probed AON provider will not supply.
> 
> Anyway I think you're right, that this is probably confusing and we
> shouldn't rely on this behavior.
> 
> To solve this, and to allow the GPU driver to correctly handle
> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
> boolean property on the GPU's DT node, e.g.
> img,gpu-expects-power-sequencer. If the GPU node provides this property
> it means the pwrseq 'gpu-power' is required.

No, that would be driver design in DTS.

I think the main problem is the pwrseq API: you should get via phandle,
not name of the pwrseq controller. That's how all producer-consumer
relationships are done in OF platforms.

It's also fragile to rely on names in case of systems with multiple
similar devices. This does not affect your platform and this hardware in
general, but shows issues with interface: imagine multiple gpus and
multiple pwr sequence devices. Which one should be obtained?
gpu-power-1? But if GPUs are the same class of devices (e.g. 2x TG1520
GPU) this is just imprecise.


Best regards,
Krzysztof

Re: [PATCH v3 6/8] riscv: dts: thead: Add GPU power sequencer node

[Bartosz Golaszewski]

On Tue, Jun 3, 2025 at 8:45 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
>
>
> On 6/3/25 15:22, Krzysztof Kozlowski wrote:
> > On Fri, May 30, 2025 at 12:23:53AM GMT, Michal Wilczynski wrote:
> >> Add the device tree node for the T-HEAD TH1520 GPU power sequencer
> >> (gpu_pwrseq) to the th1520.dtsi file.
> >>
> >> This node instantiates the thead,th1520-gpu-pwrseq driver, which
> >
> > Explain the hardware, not what drivers do.
> >
> >> is responsible for managing the GPU's power-on/off sequence. The node
> >> specifies the gpu-clkgen reset, which is one of the resources
> >> controlled by this sequencer.
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  arch/riscv/boot/dts/thead/th1520.dtsi | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index bdbb1b985b0b76cf669a9bf40c6ec37258329056..6170eec79e919b606a2046ac8f52db07e47ef441 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -238,6 +238,12 @@ aon: aon {
> >>              #power-domain-cells = <1>;
> >>      };
> >>
> >> +    gpu_pwrseq: pwrseq {
> >
> > Node names should be generic. See also an explanation and list of
> > examples (not exhaustive) in DT specification:
> > https://protect2.fireeye.com/v1/url?k=a53ea5d3-c4434f50-a53f2e9c-74fe48600158-c81092475ef416b3&q=1&e=d333d06b-0b06-493e-a358-e29ca542dfe7&u=https%3A%2F%2Fdevicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.html%23generic-names-recommendation
> >
> >> +            compatible = "thead,th1520-gpu-pwrseq";
> >> +            resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
> >> +            reset-names = "gpu-clkgen";
> >
> > What is the point of pwrseq if there is no consumer/user of it? Looks
> > like simple placeholder and anyway maybe the future consumer should just
> > use reset directly.
>
> Yeah I think you're right, I wanted to explore adding the pwrseq
> provider in separate node per discussion in v2 [1]. But for the v4 I
> think I'll revert to the v2 way of handling this reset [2].
>
> [1] - https://lore.kernel.org/all/CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com/
> [2] - https://lore.kernel.org/all/20250414-apr_14_for_sending-v2-2-70c5af2af96c@samsung.com/
>

I think you still need to connect the GPU node with its pwrseq
provider (which will be the aon node in this case). But you already
have this link - the aon power domain. You can parse it in the pwrseq
match callback to determine which GPU is powered by which AON module.

Bart

Re: [PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[Michal Wilczynski]

On 6/3/25 14:27, Ulf Hansson wrote:
> On Fri, 30 May 2025 at 00:24, Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
>> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
>> the GPU using the drm/imagination driver.
>>
>> By adding this node, the kernel can recognize and initialize the GPU,
>> providing graphics acceleration capabilities on the Lichee Pi 4A and
>> other boards based on the TH1520 SoC.
>>
>> Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
>> controlled programatically.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
>>                 #clock-cells = <0>;
>>         };
>>
>> +       gpu_mem_clk: mem-clk {
>> +               compatible = "fixed-clock";
>> +               clock-frequency = <0>;
>> +               clock-output-names = "gpu_mem_clk";
>> +               #clock-cells = <0>;
>> +       };
>> +
>>         stmmac_axi_config: stmmac-axi-config {
>>                 snps,wr_osr_lmt = <15>;
>>                 snps,rd_osr_lmt = <15>;
>> @@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
>>                         #clock-cells = <1>;
>>                 };
>>
>> +               gpu: gpu@ffef400000 {
>> +                       compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
>> +                                    "img,img-rogue";
>> +                       reg = <0xff 0xef400000 0x0 0x100000>;
>> +                       interrupt-parent = <&plic>;
>> +                       interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
>> +                       clocks = <&clk_vo CLK_GPU_CORE>,
>> +                                <&gpu_mem_clk>,
>> +                                <&clk_vo CLK_GPU_CFG_ACLK>;
>> +                       clock-names = "core", "mem", "sys";
>> +                       power-domains = <&aon TH1520_GPU_PD>;
>> +                       power-domain-names = "a";
> 
> If the power-domain-names are really needed, please pick a
> useful/descriptive name.

Yeah they are required. Even though this convention doesn't seem to be
enforced by the dt-binding it seems like it's hard-coded into the driver
330e76d31697 ("drm/imagination: Add power domain control"). So I don't
think I have any choice here.

> 
> [...]
> 
> Kind regards
> Uffe
> 

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

Re: [PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[Ulf Hansson]

On Wed, 4 Jun 2025 at 14:40, Michal Wilczynski <m.wilczynski@samsung.com> wrote:
>
>
>
> On 6/3/25 14:27, Ulf Hansson wrote:
> > On Fri, 30 May 2025 at 00:24, Michal Wilczynski
> > <m.wilczynski@samsung.com> wrote:
> >>
> >> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
> >> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
> >> the GPU using the drm/imagination driver.
> >>
> >> By adding this node, the kernel can recognize and initialize the GPU,
> >> providing graphics acceleration capabilities on the Lichee Pi 4A and
> >> other boards based on the TH1520 SoC.
> >>
> >> Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
> >> controlled programatically.
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  arch/riscv/boot/dts/thead/th1520.dtsi | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
> >>                 #clock-cells = <0>;
> >>         };
> >>
> >> +       gpu_mem_clk: mem-clk {
> >> +               compatible = "fixed-clock";
> >> +               clock-frequency = <0>;
> >> +               clock-output-names = "gpu_mem_clk";
> >> +               #clock-cells = <0>;
> >> +       };
> >> +
> >>         stmmac_axi_config: stmmac-axi-config {
> >>                 snps,wr_osr_lmt = <15>;
> >>                 snps,rd_osr_lmt = <15>;
> >> @@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
> >>                         #clock-cells = <1>;
> >>                 };
> >>
> >> +               gpu: gpu@ffef400000 {
> >> +                       compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
> >> +                                    "img,img-rogue";
> >> +                       reg = <0xff 0xef400000 0x0 0x100000>;
> >> +                       interrupt-parent = <&plic>;
> >> +                       interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
> >> +                       clocks = <&clk_vo CLK_GPU_CORE>,
> >> +                                <&gpu_mem_clk>,
> >> +                                <&clk_vo CLK_GPU_CFG_ACLK>;
> >> +                       clock-names = "core", "mem", "sys";
> >> +                       power-domains = <&aon TH1520_GPU_PD>;
> >> +                       power-domain-names = "a";
> >
> > If the power-domain-names are really needed, please pick a
> > useful/descriptive name.
>
> Yeah they are required. Even though this convention doesn't seem to be
> enforced by the dt-binding it seems like it's hard-coded into the driver
> 330e76d31697 ("drm/imagination: Add power domain control"). So I don't
> think I have any choice here.

Well, unless there is a DT doc describing the power-domain-names it's
perfectly fine to change the driver too.

Moreover, it looks like 330e76d31697 is a brand new commit, just in
linux-next, so not even included in a release yet.

Kind regards
Uffe

Re: [PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[Matt Coster]

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

On 03/06/2025 13:27, Ulf Hansson wrote:
> On Fri, 30 May 2025 at 00:24, Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
>> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
>> the GPU using the drm/imagination driver.
>>
>> By adding this node, the kernel can recognize and initialize the GPU,
>> providing graphics acceleration capabilities on the Lichee Pi 4A and
>> other boards based on the TH1520 SoC.
>>
>> Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
>> controlled programatically.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
>>                 #clock-cells = <0>;
>>         };
>>
>> +       gpu_mem_clk: mem-clk {
>> +               compatible = "fixed-clock";
>> +               clock-frequency = <0>;
>> +               clock-output-names = "gpu_mem_clk";
>> +               #clock-cells = <0>;
>> +       };
>> +
>>         stmmac_axi_config: stmmac-axi-config {
>>                 snps,wr_osr_lmt = <15>;
>>                 snps,rd_osr_lmt = <15>;
>> @@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
>>                         #clock-cells = <1>;
>>                 };
>>
>> +               gpu: gpu@ffef400000 {
>> +                       compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
>> +                                    "img,img-rogue";
>> +                       reg = <0xff 0xef400000 0x0 0x100000>;
>> +                       interrupt-parent = <&plic>;
>> +                       interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
>> +                       clocks = <&clk_vo CLK_GPU_CORE>,
>> +                                <&gpu_mem_clk>,
>> +                                <&clk_vo CLK_GPU_CFG_ACLK>;
>> +                       clock-names = "core", "mem", "sys";
>> +                       power-domains = <&aon TH1520_GPU_PD>;
>> +                       power-domain-names = "a";
> 
> If the power-domain-names are really needed, please pick a
> useful/descriptive name.

This isn't the first time our unfortunate power domain names have come
up [1][2]. Sadly, we're stuck with them for Rogue.

Matt

[1]: https://lore.kernel.org/r/ff4e96e4-ebc2-4c50-9715-82ba3d7b8612@imgtec.com/
[2]: https://lore.kernel.org/r/cc6a19b3-ba35-465c-9fa6-a764df7c01c1@imgtec.com/

> 
> [...]
> 
> Kind regards
> Uffe


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

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/4/25 14:07, Krzysztof Kozlowski wrote:
> On 04/06/2025 13:53, Michal Wilczynski wrote:
>>>>
>>>> The GPU node will depend on the AON node, which will be the sole
>>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>>>> 1).
>>>>
>>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>>>> interpretation is that the AON driver did not register this optional
>>>> sequencer. Since AON is the only anticipated source, it implies the
>>>> sequencer won't become available later from its designated provider.
>>>
>>> I don't understand why you made this assumption. AON could be a module
>>> and this driver built-in. AON will likely probe later.
>>
>> You're absolutely right that AON could be a module and would generally
>> probe later in that scenario. However, the GPU device also has a
>> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
>> the PM domain provider) were a late probing module, the GPU driver's
>> probe would hit -EPROBE_DEFER when its power domain is requested
>> which happens before attempting to get other resources like a power
>> sequencer.
> 
> Huh, so basically you imply certain hardware design and certain DTS
> description in your driver code. Well, that's clearly fragile design to
> me, because you should not rely how hardware properties are presented in
> DTS. Will work here on th1520 with this DTS, won't work with something else.
> 
> Especially that this looks like generic Imagination GPU code, common to
> multiple devices, not TH1520 only specific.
> 
>>
>> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
>> "gpu-power") call, it strongly implies the AON driver has already
>> successfully probed.
>>
>> This leads to the core challenge with the optional 'gpu-power'
>> sequencer: Even if the AON driver has already probed, if it then chooses
>> not to register the "gpu-power" sequence (because it's an optional
>> feature), pwrseq_get() will still find "no device matched" on the
>> pwrseq_bus and return EPROBE_DEFER.
>>
>> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
>> it could wait indefinitely for an optional sequence that its
>> already probed AON provider will not supply.
>>
>> Anyway I think you're right, that this is probably confusing and we
>> shouldn't rely on this behavior.
>>
>> To solve this, and to allow the GPU driver to correctly handle
>> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
>> boolean property on the GPU's DT node, e.g.
>> img,gpu-expects-power-sequencer. If the GPU node provides this property
>> it means the pwrseq 'gpu-power' is required.
> 
> No, that would be driver design in DTS.
> 
> I think the main problem is the pwrseq API: you should get via phandle,
> not name of the pwrseq controller. That's how all producer-consumer
> relationships are done in OF platforms.

Bart,
Given Krzysztof's valid concerns about the current name based
lookup in pwrseq_get() and the benefits of phandle based resource
linking in OF platforms: Would you be open to a proposal for extending
the pwrseq API to allow consumers to obtain a sequencer (or a specific
target sequence) via a phandle defined in their Device Tree node? For
instance, a consumer device could specify power-sequencer =
<&aon> and a new API variant could resolve this.

> 
> It's also fragile to rely on names in case of systems with multiple
> similar devices. This does not affect your platform and this hardware in
> general, but shows issues with interface: imagine multiple gpus and
> multiple pwr sequence devices. Which one should be obtained?
> gpu-power-1? But if GPUs are the same class of devices (e.g. 2x TG1520
> GPU) this is just imprecise.
> 
> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Bartosz Golaszewski]

On Thu, Jun 5, 2025 at 9:47 AM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
>
>
> On 6/4/25 14:07, Krzysztof Kozlowski wrote:
> > On 04/06/2025 13:53, Michal Wilczynski wrote:
> >>>>
> >>>> The GPU node will depend on the AON node, which will be the sole
> >>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
> >>>> 1).
> >>>>
> >>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
> >>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
> >>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
> >>>> interpretation is that the AON driver did not register this optional
> >>>> sequencer. Since AON is the only anticipated source, it implies the
> >>>> sequencer won't become available later from its designated provider.
> >>>
> >>> I don't understand why you made this assumption. AON could be a module
> >>> and this driver built-in. AON will likely probe later.
> >>
> >> You're absolutely right that AON could be a module and would generally
> >> probe later in that scenario. However, the GPU device also has a
> >> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
> >> the PM domain provider) were a late probing module, the GPU driver's
> >> probe would hit -EPROBE_DEFER when its power domain is requested
> >> which happens before attempting to get other resources like a power
> >> sequencer.
> >
> > Huh, so basically you imply certain hardware design and certain DTS
> > description in your driver code. Well, that's clearly fragile design to
> > me, because you should not rely how hardware properties are presented in
> > DTS. Will work here on th1520 with this DTS, won't work with something else.
> >
> > Especially that this looks like generic Imagination GPU code, common to
> > multiple devices, not TH1520 only specific.
> >
> >>
> >> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
> >> "gpu-power") call, it strongly implies the AON driver has already
> >> successfully probed.
> >>
> >> This leads to the core challenge with the optional 'gpu-power'
> >> sequencer: Even if the AON driver has already probed, if it then chooses
> >> not to register the "gpu-power" sequence (because it's an optional
> >> feature), pwrseq_get() will still find "no device matched" on the
> >> pwrseq_bus and return EPROBE_DEFER.
> >>
> >> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
> >> it could wait indefinitely for an optional sequence that its
> >> already probed AON provider will not supply.
> >>
> >> Anyway I think you're right, that this is probably confusing and we
> >> shouldn't rely on this behavior.
> >>
> >> To solve this, and to allow the GPU driver to correctly handle
> >> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
> >> boolean property on the GPU's DT node, e.g.
> >> img,gpu-expects-power-sequencer. If the GPU node provides this property
> >> it means the pwrseq 'gpu-power' is required.
> >
> > No, that would be driver design in DTS.
> >
> > I think the main problem is the pwrseq API: you should get via phandle,
> > not name of the pwrseq controller. That's how all producer-consumer
> > relationships are done in OF platforms.
>
> Bart,
> Given Krzysztof's valid concerns about the current name based
> lookup in pwrseq_get() and the benefits of phandle based resource
> linking in OF platforms: Would you be open to a proposal for extending
> the pwrseq API to allow consumers to obtain a sequencer (or a specific
> target sequence) via a phandle defined in their Device Tree node? For
> instance, a consumer device could specify power-sequencer =
> <&aon> and a new API variant could resolve this.
>

I can be open to it all I want, but I bet Krzysztof won't be open to
introducing anything like a power-sequencer device property in DT
bindings. Simply because there's no such thing in the physical world.
The concept behind the power sequencing framework was to bind
providers to consumers based on existing links modelling real device
properties (which a "power-sequencer" is not). I commented on it under
another email saying that you already have a link here - the
power-domains property taking the aon phandle. In your pwrseq
provider's match() callback you can parse and resolve it back to the
aon node thus making sure you're matching the consumer with the
correct provider.

Please take a look at the existing wcn pwrseq driver which does a
similar thing but parses the regulator properties of the power
management unit (in the pwrseq_qcom_wcn_match() function).

We've tried to do something like what you're proposing for years and
it always got stuck on the fact that DT must not make up bogus
properties only to satisfy the driver implementation. We've done it in
the past, that's true, but just because we didn't know any better and
DT maintainers are currently much stricter as to what kind of
properties to allow.

Bartosz

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On 05/06/2025 10:10, Bartosz Golaszewski wrote:
>>
>> Bart,
>> Given Krzysztof's valid concerns about the current name based
>> lookup in pwrseq_get() and the benefits of phandle based resource
>> linking in OF platforms: Would you be open to a proposal for extending
>> the pwrseq API to allow consumers to obtain a sequencer (or a specific
>> target sequence) via a phandle defined in their Device Tree node? For
>> instance, a consumer device could specify power-sequencer =
>> <&aon> and a new API variant could resolve this.
>>
> 
> I can be open to it all I want, but I bet Krzysztof won't be open to
> introducing anything like a power-sequencer device property in DT
> bindings. Simply because there's no such thing in the physical world.

Yep

> The concept behind the power sequencing framework was to bind
> providers to consumers based on existing links modelling real device
> properties (which a "power-sequencer" is not). I commented on it under
> another email saying that you already have a link here - the
> power-domains property taking the aon phandle. In your pwrseq

Exactly.


Best regards,
Krzysztof

Re: [PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[Ulf Hansson]

On Wed, 4 Jun 2025 at 18:48, Matt Coster <Matt.Coster@imgtec.com> wrote:
>
> On 03/06/2025 13:27, Ulf Hansson wrote:
> > On Fri, 30 May 2025 at 00:24, Michal Wilczynski
> > <m.wilczynski@samsung.com> wrote:
> >>
> >> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
> >> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
> >> the GPU using the drm/imagination driver.
> >>
> >> By adding this node, the kernel can recognize and initialize the GPU,
> >> providing graphics acceleration capabilities on the Lichee Pi 4A and
> >> other boards based on the TH1520 SoC.
> >>
> >> Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
> >> controlled programatically.
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  arch/riscv/boot/dts/thead/th1520.dtsi | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
> >>                 #clock-cells = <0>;
> >>         };
> >>
> >> +       gpu_mem_clk: mem-clk {
> >> +               compatible = "fixed-clock";
> >> +               clock-frequency = <0>;
> >> +               clock-output-names = "gpu_mem_clk";
> >> +               #clock-cells = <0>;
> >> +       };
> >> +
> >>         stmmac_axi_config: stmmac-axi-config {
> >>                 snps,wr_osr_lmt = <15>;
> >>                 snps,rd_osr_lmt = <15>;
> >> @@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
> >>                         #clock-cells = <1>;
> >>                 };
> >>
> >> +               gpu: gpu@ffef400000 {
> >> +                       compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
> >> +                                    "img,img-rogue";
> >> +                       reg = <0xff 0xef400000 0x0 0x100000>;
> >> +                       interrupt-parent = <&plic>;
> >> +                       interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
> >> +                       clocks = <&clk_vo CLK_GPU_CORE>,
> >> +                                <&gpu_mem_clk>,
> >> +                                <&clk_vo CLK_GPU_CFG_ACLK>;
> >> +                       clock-names = "core", "mem", "sys";
> >> +                       power-domains = <&aon TH1520_GPU_PD>;
> >> +                       power-domain-names = "a";
> >
> > If the power-domain-names are really needed, please pick a
> > useful/descriptive name.
>
> This isn't the first time our unfortunate power domain names have come
> up [1][2]. Sadly, we're stuck with them for Rogue.

Wow, that's really crazy.

BTW, are there any reasons why you can't rely on attaching them by
index instead, via dev_pm_domain_attach_by_id() for example? Thus
entirely drop the names in the DT docs?

It sounds like the names don't really have a meaning, or do they?

>
> Matt
>
> [1]: https://lore.kernel.org/r/ff4e96e4-ebc2-4c50-9715-82ba3d7b8612@imgtec.com/
> [2]: https://lore.kernel.org/r/cc6a19b3-ba35-465c-9fa6-a764df7c01c1@imgtec.com/
>
> >

[...]

Kind regards
Uffe

Re: [PATCH v3 7/8] riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node

[Matt Coster]

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

On 05/06/2025 10:57, Ulf Hansson wrote:
> On Wed, 4 Jun 2025 at 18:48, Matt Coster <Matt.Coster@imgtec.com> wrote:
>>
>> On 03/06/2025 13:27, Ulf Hansson wrote:
>>> On Fri, 30 May 2025 at 00:24, Michal Wilczynski
>>> <m.wilczynski@samsung.com> wrote:
>>>>
>>>> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
>>>> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
>>>> the GPU using the drm/imagination driver.
>>>>
>>>> By adding this node, the kernel can recognize and initialize the GPU,
>>>> providing graphics acceleration capabilities on the Lichee Pi 4A and
>>>> other boards based on the TH1520 SoC.
>>>>
>>>> Add fixed clock gpu_mem_clk, as the MEM clock on the T-HEAD SoC can't be
>>>> controlled programatically.
>>>>
>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>>> ---
>>>>  arch/riscv/boot/dts/thead/th1520.dtsi | 22 ++++++++++++++++++++++
>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> index 6170eec79e919b606a2046ac8f52db07e47ef441..ee937bbdb7c08439a70306f035b1cc82ddb4bae2 100644
>>>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> @@ -225,6 +225,13 @@ aonsys_clk: clock-73728000 {
>>>>                 #clock-cells = <0>;
>>>>         };
>>>>
>>>> +       gpu_mem_clk: mem-clk {
>>>> +               compatible = "fixed-clock";
>>>> +               clock-frequency = <0>;
>>>> +               clock-output-names = "gpu_mem_clk";
>>>> +               #clock-cells = <0>;
>>>> +       };
>>>> +
>>>>         stmmac_axi_config: stmmac-axi-config {
>>>>                 snps,wr_osr_lmt = <15>;
>>>>                 snps,rd_osr_lmt = <15>;
>>>> @@ -504,6 +511,21 @@ clk: clock-controller@ffef010000 {
>>>>                         #clock-cells = <1>;
>>>>                 };
>>>>
>>>> +               gpu: gpu@ffef400000 {
>>>> +                       compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
>>>> +                                    "img,img-rogue";
>>>> +                       reg = <0xff 0xef400000 0x0 0x100000>;
>>>> +                       interrupt-parent = <&plic>;
>>>> +                       interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                       clocks = <&clk_vo CLK_GPU_CORE>,
>>>> +                                <&gpu_mem_clk>,
>>>> +                                <&clk_vo CLK_GPU_CFG_ACLK>;
>>>> +                       clock-names = "core", "mem", "sys";
>>>> +                       power-domains = <&aon TH1520_GPU_PD>;
>>>> +                       power-domain-names = "a";
>>>
>>> If the power-domain-names are really needed, please pick a
>>> useful/descriptive name.
>>
>> This isn't the first time our unfortunate power domain names have come
>> up [1][2]. Sadly, we're stuck with them for Rogue.
> 
> Wow, that's really crazy.
> 
> BTW, are there any reasons why you can't rely on attaching them by
> index instead, via dev_pm_domain_attach_by_id() for example? Thus
> entirely drop the names in the DT docs?
> 
> It sounds like the names don't really have a meaning, or do they?

These ABC names don't have a special meaning, indices would work just
fine here. I was thinking ahead towards our current-gen Volcanic
architecture when I added power-domain-names to the schema, since that
does have actual useful names and I wanted to keep the two as close as
possible. Plus it's nice for them to line up with the docs.

> 
>>
>> Matt
>>
>> [1]: https://lore.kernel.org/r/ff4e96e4-ebc2-4c50-9715-82ba3d7b8612@imgtec.com/ 
>> [2]: https://lore.kernel.org/r/cc6a19b3-ba35-465c-9fa6-a764df7c01c1@imgtec.com/ 
>>
>>>
> 
> [...]
> 
> Kind regards
> Uffe


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

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/5/25 10:10, Bartosz Golaszewski wrote:
> On Thu, Jun 5, 2025 at 9:47 AM Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>>
>>
>> On 6/4/25 14:07, Krzysztof Kozlowski wrote:
>>> On 04/06/2025 13:53, Michal Wilczynski wrote:
>>>>>>
>>>>>> The GPU node will depend on the AON node, which will be the sole
>>>>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>>>>>> 1).
>>>>>>
>>>>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>>>>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>>>>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>>>>>> interpretation is that the AON driver did not register this optional
>>>>>> sequencer. Since AON is the only anticipated source, it implies the
>>>>>> sequencer won't become available later from its designated provider.
>>>>>
>>>>> I don't understand why you made this assumption. AON could be a module
>>>>> and this driver built-in. AON will likely probe later.
>>>>
>>>> You're absolutely right that AON could be a module and would generally
>>>> probe later in that scenario. However, the GPU device also has a
>>>> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
>>>> the PM domain provider) were a late probing module, the GPU driver's
>>>> probe would hit -EPROBE_DEFER when its power domain is requested
>>>> which happens before attempting to get other resources like a power
>>>> sequencer.
>>>
>>> Huh, so basically you imply certain hardware design and certain DTS
>>> description in your driver code. Well, that's clearly fragile design to
>>> me, because you should not rely how hardware properties are presented in
>>> DTS. Will work here on th1520 with this DTS, won't work with something else.
>>>
>>> Especially that this looks like generic Imagination GPU code, common to
>>> multiple devices, not TH1520 only specific.
>>>
>>>>
>>>> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
>>>> "gpu-power") call, it strongly implies the AON driver has already
>>>> successfully probed.
>>>>
>>>> This leads to the core challenge with the optional 'gpu-power'
>>>> sequencer: Even if the AON driver has already probed, if it then chooses
>>>> not to register the "gpu-power" sequence (because it's an optional
>>>> feature), pwrseq_get() will still find "no device matched" on the
>>>> pwrseq_bus and return EPROBE_DEFER.
>>>>
>>>> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
>>>> it could wait indefinitely for an optional sequence that its
>>>> already probed AON provider will not supply.
>>>>
>>>> Anyway I think you're right, that this is probably confusing and we
>>>> shouldn't rely on this behavior.
>>>>
>>>> To solve this, and to allow the GPU driver to correctly handle
>>>> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
>>>> boolean property on the GPU's DT node, e.g.
>>>> img,gpu-expects-power-sequencer. If the GPU node provides this property
>>>> it means the pwrseq 'gpu-power' is required.
>>>
>>> No, that would be driver design in DTS.
>>>
>>> I think the main problem is the pwrseq API: you should get via phandle,
>>> not name of the pwrseq controller. That's how all producer-consumer
>>> relationships are done in OF platforms.
>>
>> Bart,
>> Given Krzysztof's valid concerns about the current name based
>> lookup in pwrseq_get() and the benefits of phandle based resource
>> linking in OF platforms: Would you be open to a proposal for extending
>> the pwrseq API to allow consumers to obtain a sequencer (or a specific
>> target sequence) via a phandle defined in their Device Tree node? For
>> instance, a consumer device could specify power-sequencer =
>> <&aon> and a new API variant could resolve this.
>>
> 
> I can be open to it all I want, but I bet Krzysztof won't be open to
> introducing anything like a power-sequencer device property in DT
> bindings. Simply because there's no such thing in the physical world.
> The concept behind the power sequencing framework was to bind
> providers to consumers based on existing links modelling real device
> properties (which a "power-sequencer" is not). I commented on it under
> another email saying that you already have a link here - the
> power-domains property taking the aon phandle. In your pwrseq
> provider's match() callback you can parse and resolve it back to the
> aon node thus making sure you're matching the consumer with the
> correct provider.
> 
> Please take a look at the existing wcn pwrseq driver which does a
> similar thing but parses the regulator properties of the power
> management unit (in the pwrseq_qcom_wcn_match() function).
> 
> We've tried to do something like what you're proposing for years and
> it always got stuck on the fact that DT must not make up bogus
> properties only to satisfy the driver implementation. We've done it in
> the past, that's true, but just because we didn't know any better and
> DT maintainers are currently much stricter as to what kind of
> properties to allow.

Hi Bartosz, Krzysztof, Matt

Thanks for the detailed explanation. I understand and agree with your
point about using existing device tree links. The pwrseq framework's
design makes perfect sense, and matching via the power-domains property
is the right way forward for the provider.

Just to clarify, my intention is to add the power-domains check to my
existing .match() function, which already validates the consumer's
compatible ("thead,th1520-gpu"). Combining these two checks will create
an even stronger, more specific match. I will proceed with this change.

However, this leads me back to a fundamental issue with the
consumer side implementation in the generic pvr_device.c driver. The
current fallback code is:

/*
 * If the error is -EPROBE_DEFER, it's because the
 * optional sequencer provider is not present
 * and it's safe to fall back on manual power-up.
 */
if (pwrseq_err == -EPROBE_DEFER)
        pvr_dev->pwrseq = NULL;

As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
change this to a standard deferred probe, the pvr_device.c driver will
break on all other supported SoCs. It would wait indefinitely for a
pwrseq-thead-gpu provider that will never appear on those platforms.

The core of the problem is that any solution within the provider's
.match() function cannot help here. On other SoCs, the
pwrseq-thead-gpu.c driver is not even compiled, so its .match() function
will never be executed. The generic consumer driver needs a way to know
whether it should even attempt to get a sequencer.

This brings me back to the idea of a DT property, and I'd like to frame
it from a different perspective. A property like needs-power-sequencer
(perhaps vendor-prefixed as thead,needs-power-sequencer) isn't meant to
describe driver behavior, but rather to describe a physical integration
requirement of the hardware.

For the TH1520, the SoC integrators made a design choice that mandates a
specific software driven power on sequence. On other supported SoCs, as
noted by the Imagination developers [1], this sequencing is handled by
the hardware itself. Describing this platform specific requirement this
hardware quirk in the DT seems to be the most accurate way to model the
physical reality.

What would be the recommended way to proceed? The generic consumer
driver needs a way to know if it should expect a sequencer for a
particular platform, and the DT seems like the most appropriate place to
describe this hardware specific need.

If a DT property remains unacceptable, the only other path I can see is
a minimal change to the core pwrseq API itself, perhaps by adding a
non deferring devm_pwrseq_try_get() function.

[1] - https://lore.kernel.org/all/fd46f443-b1f9-4f82-8d73-117cda093315@imgtec.com/

> 
> Bartosz
> 

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Bartosz Golaszewski]

On Wed, Jun 11, 2025 at 2:01 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
>
>
> On 6/5/25 10:10, Bartosz Golaszewski wrote:
> > On Thu, Jun 5, 2025 at 9:47 AM Michal Wilczynski
> > <m.wilczynski@samsung.com> wrote:
> >>
> >>
> >>
> >> On 6/4/25 14:07, Krzysztof Kozlowski wrote:
> >>> On 04/06/2025 13:53, Michal Wilczynski wrote:
> >>>>>>
> >>>>>> The GPU node will depend on the AON node, which will be the sole
> >>>>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
> >>>>>> 1).
> >>>>>>
> >>>>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
> >>>>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
> >>>>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
> >>>>>> interpretation is that the AON driver did not register this optional
> >>>>>> sequencer. Since AON is the only anticipated source, it implies the
> >>>>>> sequencer won't become available later from its designated provider.
> >>>>>
> >>>>> I don't understand why you made this assumption. AON could be a module
> >>>>> and this driver built-in. AON will likely probe later.
> >>>>
> >>>> You're absolutely right that AON could be a module and would generally
> >>>> probe later in that scenario. However, the GPU device also has a
> >>>> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
> >>>> the PM domain provider) were a late probing module, the GPU driver's
> >>>> probe would hit -EPROBE_DEFER when its power domain is requested
> >>>> which happens before attempting to get other resources like a power
> >>>> sequencer.
> >>>
> >>> Huh, so basically you imply certain hardware design and certain DTS
> >>> description in your driver code. Well, that's clearly fragile design to
> >>> me, because you should not rely how hardware properties are presented in
> >>> DTS. Will work here on th1520 with this DTS, won't work with something else.
> >>>
> >>> Especially that this looks like generic Imagination GPU code, common to
> >>> multiple devices, not TH1520 only specific.
> >>>
> >>>>
> >>>> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
> >>>> "gpu-power") call, it strongly implies the AON driver has already
> >>>> successfully probed.
> >>>>
> >>>> This leads to the core challenge with the optional 'gpu-power'
> >>>> sequencer: Even if the AON driver has already probed, if it then chooses
> >>>> not to register the "gpu-power" sequence (because it's an optional
> >>>> feature), pwrseq_get() will still find "no device matched" on the
> >>>> pwrseq_bus and return EPROBE_DEFER.
> >>>>
> >>>> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
> >>>> it could wait indefinitely for an optional sequence that its
> >>>> already probed AON provider will not supply.
> >>>>
> >>>> Anyway I think you're right, that this is probably confusing and we
> >>>> shouldn't rely on this behavior.
> >>>>
> >>>> To solve this, and to allow the GPU driver to correctly handle
> >>>> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
> >>>> boolean property on the GPU's DT node, e.g.
> >>>> img,gpu-expects-power-sequencer. If the GPU node provides this property
> >>>> it means the pwrseq 'gpu-power' is required.
> >>>
> >>> No, that would be driver design in DTS.
> >>>
> >>> I think the main problem is the pwrseq API: you should get via phandle,
> >>> not name of the pwrseq controller. That's how all producer-consumer
> >>> relationships are done in OF platforms.
> >>
> >> Bart,
> >> Given Krzysztof's valid concerns about the current name based
> >> lookup in pwrseq_get() and the benefits of phandle based resource
> >> linking in OF platforms: Would you be open to a proposal for extending
> >> the pwrseq API to allow consumers to obtain a sequencer (or a specific
> >> target sequence) via a phandle defined in their Device Tree node? For
> >> instance, a consumer device could specify power-sequencer =
> >> <&aon> and a new API variant could resolve this.
> >>
> >
> > I can be open to it all I want, but I bet Krzysztof won't be open to
> > introducing anything like a power-sequencer device property in DT
> > bindings. Simply because there's no such thing in the physical world.
> > The concept behind the power sequencing framework was to bind
> > providers to consumers based on existing links modelling real device
> > properties (which a "power-sequencer" is not). I commented on it under
> > another email saying that you already have a link here - the
> > power-domains property taking the aon phandle. In your pwrseq
> > provider's match() callback you can parse and resolve it back to the
> > aon node thus making sure you're matching the consumer with the
> > correct provider.
> >
> > Please take a look at the existing wcn pwrseq driver which does a
> > similar thing but parses the regulator properties of the power
> > management unit (in the pwrseq_qcom_wcn_match() function).
> >
> > We've tried to do something like what you're proposing for years and
> > it always got stuck on the fact that DT must not make up bogus
> > properties only to satisfy the driver implementation. We've done it in
> > the past, that's true, but just because we didn't know any better and
> > DT maintainers are currently much stricter as to what kind of
> > properties to allow.
>
> Hi Bartosz, Krzysztof, Matt
>
> Thanks for the detailed explanation. I understand and agree with your
> point about using existing device tree links. The pwrseq framework's
> design makes perfect sense, and matching via the power-domains property
> is the right way forward for the provider.
>
> Just to clarify, my intention is to add the power-domains check to my
> existing .match() function, which already validates the consumer's
> compatible ("thead,th1520-gpu"). Combining these two checks will create
> an even stronger, more specific match. I will proceed with this change.
>
> However, this leads me back to a fundamental issue with the
> consumer side implementation in the generic pvr_device.c driver. The
> current fallback code is:
>
> /*
>  * If the error is -EPROBE_DEFER, it's because the
>  * optional sequencer provider is not present
>  * and it's safe to fall back on manual power-up.
>  */
> if (pwrseq_err == -EPROBE_DEFER)
>         pvr_dev->pwrseq = NULL;
>
> As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
> change this to a standard deferred probe, the pvr_device.c driver will
> break on all other supported SoCs. It would wait indefinitely for a
> pwrseq-thead-gpu provider that will never appear on those platforms.
>
> The core of the problem is that any solution within the provider's
> .match() function cannot help here. On other SoCs, the
> pwrseq-thead-gpu.c driver is not even compiled, so its .match() function
> will never be executed. The generic consumer driver needs a way to know
> whether it should even attempt to get a sequencer.
>
> This brings me back to the idea of a DT property, and I'd like to frame
> it from a different perspective. A property like needs-power-sequencer
> (perhaps vendor-prefixed as thead,needs-power-sequencer) isn't meant to
> describe driver behavior, but rather to describe a physical integration
> requirement of the hardware.
>
> For the TH1520, the SoC integrators made a design choice that mandates a
> specific software driven power on sequence. On other supported SoCs, as
> noted by the Imagination developers [1], this sequencing is handled by
> the hardware itself. Describing this platform specific requirement this
> hardware quirk in the DT seems to be the most accurate way to model the
> physical reality.
>

Krzysztof, this sounds a bit like the qcom,controlled-remotely and
qcom,powered-remotely boolean properties we have for the BAM DMA nodes
on Qualcomm platforms, doesn't it? They too, tell the driver to not
enable certain resources as it's the TrustZone that will do it. Maybe
it is a valid use-case for something more generic? And if not, then
something like thead,hlos-controlled for this particular use-case.

Bartosz

> What would be the recommended way to proceed? The generic consumer
> driver needs a way to know if it should expect a sequencer for a
> particular platform, and the DT seems like the most appropriate place to
> describe this hardware specific need.
>
> If a DT property remains unacceptable, the only other path I can see is
> a minimal change to the core pwrseq API itself, perhaps by adding a
> non deferring devm_pwrseq_try_get() function.
>
> [1] - https://lore.kernel.org/all/fd46f443-b1f9-4f82-8d73-117cda093315@imgtec.com/
>
> >
> > Bartosz
> >
>
> Best regards,
> --
> Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On 11/06/2025 14:01, Michal Wilczynski wrote:
> 
> However, this leads me back to a fundamental issue with the
> consumer side implementation in the generic pvr_device.c driver. The
> current fallback code is:
> 
> /*
>  * If the error is -EPROBE_DEFER, it's because the
>  * optional sequencer provider is not present
>  * and it's safe to fall back on manual power-up.
>  */
> if (pwrseq_err == -EPROBE_DEFER)
>         pvr_dev->pwrseq = NULL;
> 
> As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
> change this to a standard deferred probe, the pvr_device.c driver will

Why? You have specific compatible for executing such quirks only for
given platform.

> break on all other supported SoCs. It would wait indefinitely for a
> pwrseq-thead-gpu provider that will never appear on those platforms.
> 



Best regards,
Krzysztof

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On 11/06/2025 14:32, Bartosz Golaszewski wrote:
> On Wed, Jun 11, 2025 at 2:01 PM Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>>
>>
>> On 6/5/25 10:10, Bartosz Golaszewski wrote:
>>> On Thu, Jun 5, 2025 at 9:47 AM Michal Wilczynski
>>> <m.wilczynski@samsung.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/4/25 14:07, Krzysztof Kozlowski wrote:
>>>>> On 04/06/2025 13:53, Michal Wilczynski wrote:
>>>>>>>>
>>>>>>>> The GPU node will depend on the AON node, which will be the sole
>>>>>>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>>>>>>>> 1).
>>>>>>>>
>>>>>>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>>>>>>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>>>>>>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>>>>>>>> interpretation is that the AON driver did not register this optional
>>>>>>>> sequencer. Since AON is the only anticipated source, it implies the
>>>>>>>> sequencer won't become available later from its designated provider.
>>>>>>>
>>>>>>> I don't understand why you made this assumption. AON could be a module
>>>>>>> and this driver built-in. AON will likely probe later.
>>>>>>
>>>>>> You're absolutely right that AON could be a module and would generally
>>>>>> probe later in that scenario. However, the GPU device also has a
>>>>>> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
>>>>>> the PM domain provider) were a late probing module, the GPU driver's
>>>>>> probe would hit -EPROBE_DEFER when its power domain is requested
>>>>>> which happens before attempting to get other resources like a power
>>>>>> sequencer.
>>>>>
>>>>> Huh, so basically you imply certain hardware design and certain DTS
>>>>> description in your driver code. Well, that's clearly fragile design to
>>>>> me, because you should not rely how hardware properties are presented in
>>>>> DTS. Will work here on th1520 with this DTS, won't work with something else.
>>>>>
>>>>> Especially that this looks like generic Imagination GPU code, common to
>>>>> multiple devices, not TH1520 only specific.
>>>>>
>>>>>>
>>>>>> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
>>>>>> "gpu-power") call, it strongly implies the AON driver has already
>>>>>> successfully probed.
>>>>>>
>>>>>> This leads to the core challenge with the optional 'gpu-power'
>>>>>> sequencer: Even if the AON driver has already probed, if it then chooses
>>>>>> not to register the "gpu-power" sequence (because it's an optional
>>>>>> feature), pwrseq_get() will still find "no device matched" on the
>>>>>> pwrseq_bus and return EPROBE_DEFER.
>>>>>>
>>>>>> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
>>>>>> it could wait indefinitely for an optional sequence that its
>>>>>> already probed AON provider will not supply.
>>>>>>
>>>>>> Anyway I think you're right, that this is probably confusing and we
>>>>>> shouldn't rely on this behavior.
>>>>>>
>>>>>> To solve this, and to allow the GPU driver to correctly handle
>>>>>> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
>>>>>> boolean property on the GPU's DT node, e.g.
>>>>>> img,gpu-expects-power-sequencer. If the GPU node provides this property
>>>>>> it means the pwrseq 'gpu-power' is required.
>>>>>
>>>>> No, that would be driver design in DTS.
>>>>>
>>>>> I think the main problem is the pwrseq API: you should get via phandle,
>>>>> not name of the pwrseq controller. That's how all producer-consumer
>>>>> relationships are done in OF platforms.
>>>>
>>>> Bart,
>>>> Given Krzysztof's valid concerns about the current name based
>>>> lookup in pwrseq_get() and the benefits of phandle based resource
>>>> linking in OF platforms: Would you be open to a proposal for extending
>>>> the pwrseq API to allow consumers to obtain a sequencer (or a specific
>>>> target sequence) via a phandle defined in their Device Tree node? For
>>>> instance, a consumer device could specify power-sequencer =
>>>> <&aon> and a new API variant could resolve this.
>>>>
>>>
>>> I can be open to it all I want, but I bet Krzysztof won't be open to
>>> introducing anything like a power-sequencer device property in DT
>>> bindings. Simply because there's no such thing in the physical world.
>>> The concept behind the power sequencing framework was to bind
>>> providers to consumers based on existing links modelling real device
>>> properties (which a "power-sequencer" is not). I commented on it under
>>> another email saying that you already have a link here - the
>>> power-domains property taking the aon phandle. In your pwrseq
>>> provider's match() callback you can parse and resolve it back to the
>>> aon node thus making sure you're matching the consumer with the
>>> correct provider.
>>>
>>> Please take a look at the existing wcn pwrseq driver which does a
>>> similar thing but parses the regulator properties of the power
>>> management unit (in the pwrseq_qcom_wcn_match() function).
>>>
>>> We've tried to do something like what you're proposing for years and
>>> it always got stuck on the fact that DT must not make up bogus
>>> properties only to satisfy the driver implementation. We've done it in
>>> the past, that's true, but just because we didn't know any better and
>>> DT maintainers are currently much stricter as to what kind of
>>> properties to allow.
>>
>> Hi Bartosz, Krzysztof, Matt
>>
>> Thanks for the detailed explanation. I understand and agree with your
>> point about using existing device tree links. The pwrseq framework's
>> design makes perfect sense, and matching via the power-domains property
>> is the right way forward for the provider.
>>
>> Just to clarify, my intention is to add the power-domains check to my
>> existing .match() function, which already validates the consumer's
>> compatible ("thead,th1520-gpu"). Combining these two checks will create
>> an even stronger, more specific match. I will proceed with this change.
>>
>> However, this leads me back to a fundamental issue with the
>> consumer side implementation in the generic pvr_device.c driver. The
>> current fallback code is:
>>
>> /*
>>  * If the error is -EPROBE_DEFER, it's because the
>>  * optional sequencer provider is not present
>>  * and it's safe to fall back on manual power-up.
>>  */
>> if (pwrseq_err == -EPROBE_DEFER)
>>         pvr_dev->pwrseq = NULL;
>>
>> As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
>> change this to a standard deferred probe, the pvr_device.c driver will
>> break on all other supported SoCs. It would wait indefinitely for a
>> pwrseq-thead-gpu provider that will never appear on those platforms.
>>
>> The core of the problem is that any solution within the provider's
>> .match() function cannot help here. On other SoCs, the
>> pwrseq-thead-gpu.c driver is not even compiled, so its .match() function
>> will never be executed. The generic consumer driver needs a way to know
>> whether it should even attempt to get a sequencer.
>>
>> This brings me back to the idea of a DT property, and I'd like to frame
>> it from a different perspective. A property like needs-power-sequencer
>> (perhaps vendor-prefixed as thead,needs-power-sequencer) isn't meant to
>> describe driver behavior, but rather to describe a physical integration
>> requirement of the hardware.
>>
>> For the TH1520, the SoC integrators made a design choice that mandates a
>> specific software driven power on sequence. On other supported SoCs, as
>> noted by the Imagination developers [1], this sequencing is handled by
>> the hardware itself. Describing this platform specific requirement this
>> hardware quirk in the DT seems to be the most accurate way to model the
>> physical reality.
>>
> 
> Krzysztof, this sounds a bit like the qcom,controlled-remotely and
> qcom,powered-remotely boolean properties we have for the BAM DMA nodes
> on Qualcomm platforms, doesn't it? They too, tell the driver to not
> enable certain resources as it's the TrustZone that will do it. Maybe
> it is a valid use-case for something more generic? And if not, then
> something like thead,hlos-controlled for this particular use-case.

Depends what "SoC integrators" mean. Following sentence suggests it is
about this given SoC, so it looks 100% purely compatible-deducible. You
have here something different for this one given hardware. That
something different is defined by compatible.

That's why every hardware binding MUST have specific compatible. Also
that's one of the reasons why we really dislike generic compatibles like
one used here, but it is a separate thing.

Best regards,
Krzysztof

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/13/25 08:44, Krzysztof Kozlowski wrote:
> On 11/06/2025 14:01, Michal Wilczynski wrote:
>>
>> However, this leads me back to a fundamental issue with the
>> consumer side implementation in the generic pvr_device.c driver. The
>> current fallback code is:
>>
>> /*
>>  * If the error is -EPROBE_DEFER, it's because the
>>  * optional sequencer provider is not present
>>  * and it's safe to fall back on manual power-up.
>>  */
>> if (pwrseq_err == -EPROBE_DEFER)
>>         pvr_dev->pwrseq = NULL;
>>
>> As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
>> change this to a standard deferred probe, the pvr_device.c driver will
> 
> Why? You have specific compatible for executing such quirks only for
> given platform.

This is due to how the pwrseq API works; it constructs a bus on which
provider devices may appear at any time. With the current API, there is
no way to express that a provider for a specific target will never
appear. ('gpu-power' is the generic target name, and of course, more
specific binding is handled in the provider's .match callback - based on
the compatible and the node phandle like discussed previously).

For all other supported SoCs, no such provider will ever appear on the
bus, and the current pwrseq API doesn't allow a generic consumer to know
this.

However, your suggestion of handling this with a platform specific
driver is a good path forward. It would still require a minimal addition
to the pwrseq API to work. For example, a new SoC specific driver for
"thead,th1520" could call a new function like
pwrseq_enable_optional_target("gpu-power") during its probe. This would
signal to the pwrseq core that this target is expected on the platform.
Therefore, when the Imagination driver later calls pwrseq_get() on a
TH1520, it would correctly result in either a match or a deferral.

On all other platforms, this optional target would not be enabled. The
pwrseq_get() call would then immediately return -ENODEV instead of
deferring, which solves the problem and allows the other supported SoCs
to probe correctly.

I wonder whether Bartosz would be okay with such an addition.

> 
>> break on all other supported SoCs. It would wait indefinitely for a
>> pwrseq-thead-gpu provider that will never appear on those platforms.
>>
> 
> 
> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/13/25 10:25, Michal Wilczynski wrote:
> 
> 
> On 6/13/25 08:44, Krzysztof Kozlowski wrote:
>> On 11/06/2025 14:01, Michal Wilczynski wrote:
>>>
>>> However, this leads me back to a fundamental issue with the
>>> consumer side implementation in the generic pvr_device.c driver. The
>>> current fallback code is:
>>>
>>> /*
>>>  * If the error is -EPROBE_DEFER, it's because the
>>>  * optional sequencer provider is not present
>>>  * and it's safe to fall back on manual power-up.
>>>  */
>>> if (pwrseq_err == -EPROBE_DEFER)
>>>         pvr_dev->pwrseq = NULL;
>>>
>>> As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
>>> change this to a standard deferred probe, the pvr_device.c driver will
>>
>> Why? You have specific compatible for executing such quirks only for
>> given platform.

I realized now that you may have meant the "thead,th1520-gpu" compatible,
not the "thead,th1520" SoC compatible.

In any case, the whole reason for using the pwrseq framework is to avoid
polluting the generic driver with SoC specific logic and instead offload
that responsibility to a pwrseq provider. Therefore, I can't simply add
a check like if (compatible == "thead,th1520-gpu") to the generic driver
to decide whether to get a power sequencer. This entire matching
responsibility was intended to be offloaded to the pwrseq framework.

> 
> This is due to how the pwrseq API works; it constructs a bus on which
> provider devices may appear at any time. With the current API, there is
> no way to express that a provider for a specific target will never
> appear. ('gpu-power' is the generic target name, and of course, more
> specific binding is handled in the provider's .match callback - based on
> the compatible and the node phandle like discussed previously).
> 
> For all other supported SoCs, no such provider will ever appear on the
> bus, and the current pwrseq API doesn't allow a generic consumer to know
> this.
> 
> However, your suggestion of handling this with a platform specific
> driver is a good path forward. It would still require a minimal addition
> to the pwrseq API to work. For example, a new SoC specific driver for
> "thead,th1520" could call a new function like
> pwrseq_enable_optional_target("gpu-power") during its probe. This would
> signal to the pwrseq core that this target is expected on the platform.
> Therefore, when the Imagination driver later calls pwrseq_get() on a
> TH1520, it would correctly result in either a match or a deferral.
> 
> On all other platforms, this optional target would not be enabled. The
> pwrseq_get() call would then immediately return -ENODEV instead of
> deferring, which solves the problem and allows the other supported SoCs
> to probe correctly.
> 
> I wonder whether Bartosz would be okay with such an addition.
> 
>>
>>> break on all other supported SoCs. It would wait indefinitely for a
>>> pwrseq-thead-gpu provider that will never appear on those platforms.
>>>
>>
>>
>>
>> Best regards,
>> Krzysztof
>>
> 
> Best regards,

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

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Krzysztof Kozlowski]

On 13/06/2025 11:49, Michal Wilczynski wrote:
> 
> 
> On 6/13/25 10:25, Michal Wilczynski wrote:
>>
>>
>> On 6/13/25 08:44, Krzysztof Kozlowski wrote:
>>> On 11/06/2025 14:01, Michal Wilczynski wrote:
>>>>
>>>> However, this leads me back to a fundamental issue with the
>>>> consumer side implementation in the generic pvr_device.c driver. The
>>>> current fallback code is:
>>>>
>>>> /*
>>>>  * If the error is -EPROBE_DEFER, it's because the
>>>>  * optional sequencer provider is not present
>>>>  * and it's safe to fall back on manual power-up.
>>>>  */
>>>> if (pwrseq_err == -EPROBE_DEFER)
>>>>         pvr_dev->pwrseq = NULL;
>>>>
>>>> As Krzysztof noted, simply ignoring -EPROBE_DEFER is not ideal. But if I
>>>> change this to a standard deferred probe, the pvr_device.c driver will
>>>
>>> Why? You have specific compatible for executing such quirks only for
>>> given platform.
> 
> I realized now that you may have meant the "thead,th1520-gpu" compatible,
> not the "thead,th1520" SoC compatible.
> 
> In any case, the whole reason for using the pwrseq framework is to avoid
> polluting the generic driver with SoC specific logic and instead offload
> that responsibility to a pwrseq provider. Therefore, I can't simply add
> a check like if (compatible == "thead,th1520-gpu") to the generic driver
> to decide whether to get a power sequencer. This entire matching
> responsibility was intended to be offloaded to the pwrseq framework.

No, just do how all drivers are doing - driver match data, describing
that there is some component, e.g. quirks/flags, number of clocks and
their names typically. In your case - name or presence of pwrseq.



Best regards,
Krzysztof

Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Bartosz Golaszewski]

On Fri, Jun 13, 2025 at 10:25 AM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> > Why? You have specific compatible for executing such quirks only for
> > given platform.
>
> This is due to how the pwrseq API works; it constructs a bus on which
> provider devices may appear at any time. With the current API, there is
> no way to express that a provider for a specific target will never
> appear. ('gpu-power' is the generic target name, and of course, more
> specific binding is handled in the provider's .match callback - based on
> the compatible and the node phandle like discussed previously).
>

This is by design and also the reason why we don't expose a
pwrseq_get_optional(). If the driver calls pwrseq_get() at all then it
expects the provider to eventually appear or will fail to initialize.

That still doesn't answer the question: why can't you have a callback
in your match data which - for this mode only - would call
pwrseq_get()/power_on()/power_off()?

> For all other supported SoCs, no such provider will ever appear on the
> bus, and the current pwrseq API doesn't allow a generic consumer to know
> this.
>
> However, your suggestion of handling this with a platform specific
> driver is a good path forward. It would still require a minimal addition
> to the pwrseq API to work. For example, a new SoC specific driver for
> "thead,th1520" could call a new function like
> pwrseq_enable_optional_target("gpu-power") during its probe. This would
> signal to the pwrseq core that this target is expected on the platform.
> Therefore, when the Imagination driver later calls pwrseq_get() on a
> TH1520, it would correctly result in either a match or a deferral.
>

I don't want to use the word optional. I think what you're thinking of
is a kind of "deferred" get where you get a valid handle, use it and
then, the provider eventually arrives it will get attached and its
state aligned with the current state of all the existing handles. I'm
not against it, it sounds quite elegant but there's an issue of error
reporting in this case. We'd need a notifier of some kind so that
consumers could subscribe and be notified about errors happening
behind the scenes.

Bart