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

[PATCH v6 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: Introduces the pwrseq-thead-gpu auxiliary driver to manage the
         GPU's power-on/off sequence.
Patch 2: Adds device tree bindings for the gpu-clkgen reset to the
         existing thead,th1520-aon binding.
Patch 3: Extends the pm-domains driver to detect the gpu-clkgen reset
         and spawn the pwrseq-thead-gpu auxiliary driver.
Patch 4: Updates the Imagination DRM driver to utilize the pwrseq
         framework for TH1520 GPU power management.
Patch 5: Adds the thead,th1520-gpu compatible string to the PowerVR GPU
         device tree bindings.
Patch 6: Adds the gpu-clkgen reset property to the aon node in the
         TH1520 device tree source.
Patch 7: Adds the device tree node for the IMG BXM-4-64 GPU and its
         required fixed-clock.
Patch 8: Enables compilation of the Imagination PowerVR driver on the
         RISC-V architecture.

This patchset finishes the work started in bigger series [2] by adding
all 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.

Link to v5 of this series - [3].

v6:
 - check return values from reset_control_assert() and propagate the
   first error, ensuring all teardown steps are still attempted
 - the driver now stores a reference to the consumer's device node to
   ensure it binds to a single, specific device
 - rename Kconfig option to POWER_SEQUENCING_TH1520_GPU
 - remove COMPILE_TEST

v5:

- reworked the pwrseq-thead-gpu driver, now using manual resource
  management in .match and a .remove callback
- refactored the drm/imagination driver to use function pointers for
  power management instead of a boolean flag
- switched the pmdomain driver to use the generic
  device_property_match_string() helper
- added MMU and COMPILE_TEST dependencies to Kconfig to fix RISC-V
  build warnings.

v4:

- the pwrseq driver is now an auxiliary driver with a robust match
  function based on the power-domains property, spawned from the AON
  node 
- Imagination DRM driver now uses of_device_id match data to
  conditionally probe for the pwrseq, solving the cross platform
  probe deferral issue
- add Reviewed-by from Ulf for the entire series

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/20250618-apr_14_for_sending-v5-0-27ed33ea5c6f@samsung.com/

---
Michal Wilczynski (8):
      power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
      dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen
      pmdomain: thead: Instantiate GPU power sequencer via auxiliary bus
      drm/imagination: Use pwrseq for TH1520 GPU power management
      dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible
      riscv: dts: thead: th1520: Add GPU clkgen reset to AON node
      riscv: dts: thead: th1520: Add IMG BXM-4-64 GPU node
      drm/imagination: Enable PowerVR driver for RISC-V

 .../bindings/firmware/thead,th1520-aon.yaml        |   7 +
 .../devicetree/bindings/gpu/img,powervr-rogue.yaml |   9 +-
 MAINTAINERS                                        |   1 +
 arch/riscv/boot/dts/thead/th1520.dtsi              |  25 +++
 drivers/gpu/drm/imagination/Kconfig                |   4 +-
 drivers/gpu/drm/imagination/pvr_device.c           |  31 ++-
 drivers/gpu/drm/imagination/pvr_device.h           |  19 ++
 drivers/gpu/drm/imagination/pvr_drv.c              |  30 ++-
 drivers/gpu/drm/imagination/pvr_power.c            | 112 ++++++----
 drivers/gpu/drm/imagination/pvr_power.h            |   6 +
 drivers/pmdomain/thead/Kconfig                     |   1 +
 drivers/pmdomain/thead/th1520-pm-domains.c         |  51 +++++
 drivers/power/sequencing/Kconfig                   |   8 +
 drivers/power/sequencing/Makefile                  |   1 +
 drivers/power/sequencing/pwrseq-thead-gpu.c        | 247 +++++++++++++++++++++
 15 files changed, 503 insertions(+), 49 deletions(-)
---
base-commit: 4774cfe3543abb8ee98089f535e28ebfd45b975a
change-id: 20250414-apr_14_for_sending-5b3917817acc

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


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

[PATCH v6 1/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. This driver
controls an auxiliary device instantiated by the AON power domain.

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.

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 clk_bulk_get() and
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 clkgen_reset obtained from parent aon node, allow it to
perform the complete sequence.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
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 | 247 ++++++++++++++++++++++++++++
 4 files changed, 257 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0183c028fa18c397d30ec82fd419894f1f50a448..3283ff592215249bcf702dbb4ab710477243477e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21395,6 +21395,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..0f118d57c1ceddc03954c006f99b5990acf546d4 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_TH1520_GPU
+	tristate "T-HEAD TH1520 GPU power sequencing driver"
+	depends on ARCH_THEAD && AUXILIARY_BUS
+	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..96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1 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_TH1520_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..f267f95b5d0468b21eba15e633ef39fce2cc503f
--- /dev/null
+++ b/drivers/power/sequencing/pwrseq-thead-gpu.c
@@ -0,0 +1,247 @@
+// 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-4-64
+ * GPU on the T-HEAD TH1520 SoC. The sequence requires coordinating resources
+ * from both the sequencer's parent 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/auxiliary_bus.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/reset.h>
+
+#include <dt-bindings/power/thead,th1520-power.h>
+
+struct pwrseq_thead_gpu_ctx {
+	struct pwrseq_device *pwrseq;
+	struct reset_control *clkgen_reset;
+	struct device_node *aon_node;
+
+	/* Consumer resources */
+	struct device_node *consumer_node;
+	struct clk_bulk_data *clks;
+	int num_clks;
+	struct reset_control *gpu_reset;
+};
+
+static int pwrseq_thead_gpu_enable(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_disable(struct pwrseq_device *pwrseq)
+{
+	struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+	int ret = 0, err;
+
+	if (!ctx->clks || !ctx->gpu_reset)
+		return -ENODEV;
+
+	err = reset_control_assert(ctx->gpu_reset);
+	if (err)
+		ret = err;
+
+	err = reset_control_assert(ctx->clkgen_reset);
+	if (err && !ret)
+		ret = err;
+
+	clk_bulk_disable_unprepare(ctx->num_clks, ctx->clks);
+
+	/* ret stores values of the first error code */
+	return ret;
+}
+
+static const struct pwrseq_unit_data pwrseq_thead_gpu_unit = {
+	.name = "gpu-power-sequence",
+	.enable = pwrseq_thead_gpu_enable,
+	.disable = pwrseq_thead_gpu_disable,
+};
+
+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" };
+	struct of_phandle_args pwr_spec;
+	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;
+
+	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
+					 "#power-domain-cells", 0, &pwr_spec);
+	if (ret)
+		return 0;
+
+	/* Additionally verify consumer device has AON as power-domain */
+	if (pwr_spec.np != ctx->aon_node || pwr_spec.args[0] != TH1520_GPU_PD) {
+		of_node_put(pwr_spec.np);
+		return 0;
+	}
+
+	of_node_put(pwr_spec.np);
+
+	/* If a consumer is already bound, only allow a re-match from it */
+	if (ctx->consumer_node)
+		return ctx->consumer_node == dev->of_node;
+
+	ctx->num_clks = ARRAY_SIZE(clk_names);
+	ctx->clks = kcalloc(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 = clk_bulk_get(dev, ctx->num_clks, ctx->clks);
+	if (ret)
+		goto err_free_clks;
+
+	ctx->gpu_reset = reset_control_get_shared(dev, NULL);
+	if (IS_ERR(ctx->gpu_reset)) {
+		ret = PTR_ERR(ctx->gpu_reset);
+		goto err_put_clks;
+	}
+
+	ctx->consumer_node = of_node_get(dev->of_node);
+
+	return 1;
+
+err_put_clks:
+	clk_bulk_put(ctx->num_clks, ctx->clks);
+err_free_clks:
+	kfree(ctx->clks);
+	ctx->clks = NULL;
+
+	return ret;
+}
+
+static int pwrseq_thead_gpu_probe(struct auxiliary_device *adev,
+				  const struct auxiliary_device_id *id)
+{
+	struct device *dev = &adev->dev;
+	struct device *parent_dev = dev->parent;
+	struct pwrseq_thead_gpu_ctx *ctx;
+	struct pwrseq_config config = {};
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->aon_node = parent_dev->of_node;
+
+	ctx->clkgen_reset =
+		devm_reset_control_get_exclusive(parent_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 from parent\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");
+
+	auxiliary_set_drvdata(adev, ctx);
+
+	return 0;
+}
+
+static void pwrseq_thead_gpu_remove(struct auxiliary_device *adev)
+{
+	struct pwrseq_thead_gpu_ctx *ctx = auxiliary_get_drvdata(adev);
+
+	if (ctx->gpu_reset)
+		reset_control_put(ctx->gpu_reset);
+
+	if (ctx->clks) {
+		clk_bulk_put(ctx->num_clks, ctx->clks);
+		kfree(ctx->clks);
+	}
+
+	if (ctx->consumer_node)
+		of_node_put(ctx->consumer_node);
+}
+
+static const struct auxiliary_device_id pwrseq_thead_gpu_id_table[] = {
+	{ .name = "th1520_pm_domains.pwrseq-gpu" },
+	{},
+};
+MODULE_DEVICE_TABLE(auxiliary, pwrseq_thead_gpu_id_table);
+
+static struct auxiliary_driver pwrseq_thead_gpu_driver = {
+	.driver = {
+		.name = "pwrseq-thead-gpu",
+	},
+	.probe = pwrseq_thead_gpu_probe,
+	.remove = pwrseq_thead_gpu_remove,
+	.id_table = pwrseq_thead_gpu_id_table,
+};
+module_auxiliary_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


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

Re: [PATCH v6 1/8] power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver

[Bartosz Golaszewski]

On Mon, Jun 23, 2025 at 1:44 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> Introduce the pwrseq-thead-gpu driver, a power sequencer provider for
> the Imagination BXM-4-64 GPU on the T-HEAD TH1520 SoC. This driver
> controls an auxiliary device instantiated by the AON power domain.
>
> 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.
>
> 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 clk_bulk_get() and
> 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 clkgen_reset obtained from parent aon node, allow it to
> perform the complete sequence.
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---

[snip]

> +
> +       /* Additionally verify consumer device has AON as power-domain */
> +       if (pwr_spec.np != ctx->aon_node || pwr_spec.args[0] != TH1520_GPU_PD) {
> +               of_node_put(pwr_spec.np);
> +               return 0;
> +       }
> +
> +       of_node_put(pwr_spec.np);
> +
> +       /* If a consumer is already bound, only allow a re-match from it */
> +       if (ctx->consumer_node)
> +               return ctx->consumer_node == dev->of_node;
> +

That should be `!!(ctx->consumer_node == dev->of_node)` or preferably
`ctx->consumer_node == dev->of_node ? 1 : 0`. I can amend it when
applying if you have no objections. The rest looks good to me and I'd
like to pick it up into pwrseq/for-next in the next two days.

Bart

[snip]

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

Re: [PATCH v6 1/8] power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver

[Michal Wilczynski]

On 6/23/25 16:32, Bartosz Golaszewski wrote:
> On Mon, Jun 23, 2025 at 1:44 PM Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> Introduce the pwrseq-thead-gpu driver, a power sequencer provider for
>> the Imagination BXM-4-64 GPU on the T-HEAD TH1520 SoC. This driver
>> controls an auxiliary device instantiated by the AON power domain.
>>
>> 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.
>>
>> 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 clk_bulk_get() and
>> 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 clkgen_reset obtained from parent aon node, allow it to
>> perform the complete sequence.
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
> 
> [snip]
> 
>> +
>> +       /* Additionally verify consumer device has AON as power-domain */
>> +       if (pwr_spec.np != ctx->aon_node || pwr_spec.args[0] != TH1520_GPU_PD) {
>> +               of_node_put(pwr_spec.np);
>> +               return 0;
>> +       }
>> +
>> +       of_node_put(pwr_spec.np);
>> +
>> +       /* If a consumer is already bound, only allow a re-match from it */
>> +       if (ctx->consumer_node)
>> +               return ctx->consumer_node == dev->of_node;
>> +
> 
> That should be `!!(ctx->consumer_node == dev->of_node)` or preferably
> `ctx->consumer_node == dev->of_node ? 1 : 0`. I can amend it when
> applying if you have no objections. The rest looks good to me and I'd
> like to pick it up into pwrseq/for-next in the next two days.

Sure,
Thanks !

> 
> Bart
> 
> [snip]
> 

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

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

[PATCH v6 2/8] dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen

[Michal Wilczynski]

Extend the TH1520 AON to describe the GPU clkgen reset line, required
for proper GPU clock and reset sequencing.

The T-HEAD TH1520 GPU requires coordinated management of two clocks
(core and sys) and two resets (GPU core reset and GPU clkgen reset).
Only the clkgen reset is exposed at the AON level, to support SoC
specific initialization handled through a dedicated auxiliary power
sequencing driver. The GPU core reset remains described in the GPU
device node, as from the GPU driver's perspective, there is only a
single reset line [1].

This follows upstream maintainers' recommendations [2] to abstract SoC
specific details into the PM domain layer rather than exposing them to
drivers directly.

Link: https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/ - [1]
Link: https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/ - [2]

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Drew Fustini <drew@pdp7.com>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
index bbc183200400de7aadbb21fea21911f6f4227b09..3365124c7fd4736922717bd31caa13272f4a4ea6 100644
--- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
+++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
@@ -32,6 +32,13 @@ properties:
     items:
       - const: aon
 
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: gpu-clkgen
+
   "#power-domain-cells":
     const: 1
 

-- 
2.34.1


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

[PATCH v6 3/8] pmdomain: thead: Instantiate GPU power sequencer via auxiliary bus

[Michal Wilczynski]

In order to support the complex power sequencing required by the TH1520
GPU, the AON power domain driver must be responsible for initiating the
corresponding sequencer driver. This functionality is specific to
platforms where the GPU power sequencing hardware is controlled by the
AON block.

Extend the AON power domain driver to check for the presence of the
"gpu-clkgen" reset in its own device tree node.

If the property is found, create and register a new auxiliary device.
This device acts as a proxy that allows the dedicated `pwrseq-thead-gpu`
auxiliary driver to bind and take control of the sequencing logic.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/pmdomain/thead/Kconfig             |  1 +
 drivers/pmdomain/thead/th1520-pm-domains.c | 51 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/pmdomain/thead/Kconfig b/drivers/pmdomain/thead/Kconfig
index 7d52f8374b074167d508a80fd807929c53faef12..208828e0fa0dc91256bf808b905bea32bb84250d 100644
--- a/drivers/pmdomain/thead/Kconfig
+++ b/drivers/pmdomain/thead/Kconfig
@@ -4,6 +4,7 @@ config TH1520_PM_DOMAINS
 	tristate "Support TH1520 Power Domains"
 	depends on TH1520_AON_PROTOCOL
 	select REGMAP_MMIO
+	select AUXILIARY_BUS
 	help
 	  This driver enables power domain management for the T-HEAD
 	  TH-1520 SoC. On this SoC there are number of power domains,
diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
index f702e20306f469aeb0ed15e54bd4f8309f28018c..9040b698e7f7f2400163841530fecacfb0f917bc 100644
--- a/drivers/pmdomain/thead/th1520-pm-domains.c
+++ b/drivers/pmdomain/thead/th1520-pm-domains.c
@@ -5,6 +5,7 @@
  * Author: Michal Wilczynski <m.wilczynski@samsung.com>
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/firmware/thead/thead,th1520-aon.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
@@ -128,6 +129,50 @@ static void th1520_pd_init_all_off(struct generic_pm_domain **domains,
 	}
 }
 
+static void th1520_pd_pwrseq_unregister_adev(void *adev)
+{
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+static int th1520_pd_pwrseq_gpu_init(struct device *dev)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	/*
+	 * Correctly check only for the property's existence in the DT node.
+	 * We don't need to get/claim the reset here; that is the job of
+	 * the auxiliary driver that we are about to spawn.
+	 */
+	if (device_property_match_string(dev, "reset-names", "gpu-clkgen") < 0)
+		/*
+		 * This is not an error. It simply means the optional sequencer
+		 * is not described in the device tree.
+		 */
+		return 0;
+
+	adev = devm_kzalloc(dev, sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = "pwrseq-gpu";
+	adev->dev.parent = dev;
+
+	ret = auxiliary_device_init(adev);
+	if (ret)
+		return ret;
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(dev, th1520_pd_pwrseq_unregister_adev,
+					adev);
+}
+
 static int th1520_pd_probe(struct platform_device *pdev)
 {
 	struct generic_pm_domain **domains;
@@ -186,8 +231,14 @@ static int th1520_pd_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clean_genpd;
 
+	ret = th1520_pd_pwrseq_gpu_init(dev);
+	if (ret)
+		goto err_clean_provider;
+
 	return 0;
 
+err_clean_provider:
+	of_genpd_del_provider(dev->of_node);
 err_clean_genpd:
 	for (i--; i >= 0; i--)
 		pm_genpd_remove(domains[i]);

-- 
2.34.1


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

[PATCH v6 4/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.

To cleanly handle the TH1520's specific power requirements in the
generic driver, this patch implements the "driver match data" pattern.
The pvr_soc_data struct, associated with a compatible string in the
of_device_id table, now holds pointers to platform-specific power_on and
power_off functions.

At probe time, the driver inspects the assigned power_on function
pointer. If it points to the pwrseq variant, the driver calls
devm_pwrseq_get("gpu-power"), requiring a valid sequencer and deferring
probe on failure. Otherwise, it falls back to its standard manual reset
initialization.

The runtime PM callbacks, pvr_power_device_resume() and
pvr_power_device_suspend(), call the power_on and power_off function
pointers. Helper functions for both manual and pwrseq-based sequences
are introduced to support this.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/Kconfig      |   1 +
 drivers/gpu/drm/imagination/pvr_device.c |  31 +++++++--
 drivers/gpu/drm/imagination/pvr_device.h |  19 ++++++
 drivers/gpu/drm/imagination/pvr_drv.c    |  30 ++++++++-
 drivers/gpu/drm/imagination/pvr_power.c  | 112 ++++++++++++++++++++-----------
 drivers/gpu/drm/imagination/pvr_power.h  |   6 ++
 6 files changed, 152 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..5f9fff43d6baadc42ebf48d91729bfbf27e06caa 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..c1c24c441c821ccce59f7cd3f14544a91ef54ea9 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -23,8 +23,10 @@
 #include <linux/firmware.h>
 #include <linux/gfp.h>
 #include <linux/interrupt.h>
+#include <linux/of.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>
@@ -618,6 +620,9 @@ pvr_device_init(struct pvr_device *pvr_dev)
 	struct device *dev = drm_dev->dev;
 	int err;
 
+	/* Get the platform-specific data based on the compatible string. */
+	pvr_dev->soc_data = of_device_get_match_data(dev);
+
 	/*
 	 * Setup device parameters. We do this first in case other steps
 	 * depend on them.
@@ -631,10 +636,28 @@ 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;
+	/*
+	 * For platforms that require it, get the power sequencer.
+	 * For all others, perform manual reset initialization.
+	 */
+	if (pvr_dev->soc_data->power_on == pvr_power_on_sequence_pwrseq) {
+		pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
+		if (IS_ERR(pvr_dev->pwrseq)) {
+			/*
+			 * This platform requires a sequencer. If we can't get
+			 * it, we must return the error (including -EPROBE_DEFER
+			 * to wait for the provider to appear)
+			 */
+			return dev_err_probe(
+				dev, PTR_ERR(pvr_dev->pwrseq),
+				"Failed to get required power sequencer\n");
+		}
+	} else {
+		/* This platform does not use a sequencer, init reset manually. */
+		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..3f35025e84efac031d3261c849ef9fe105466423 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.
@@ -57,6 +60,16 @@ struct pvr_fw_version {
 	u16 major, minor;
 };
 
+/**
+ * struct pvr_soc_data - Platform specific data associated with a compatible string.
+ * @power_on: Pointer to the platform-specific power on function.
+ * @power_off: Pointer to the platform-specific power off function.
+ */
+struct pvr_soc_data {
+	int (*power_on)(struct pvr_device *pvr_dev);
+	int (*power_off)(struct pvr_device *pvr_dev);
+};
+
 /**
  * struct pvr_device - powervr-specific wrapper for &struct drm_device
  */
@@ -98,6 +111,9 @@ struct pvr_device {
 	/** @fw_version: Firmware version detected at runtime. */
 	struct pvr_fw_version fw_version;
 
+	/** @soc_data: Pointer to platform-specific quirk data. */
+	const struct pvr_soc_data *soc_data;
+
 	/** @regs_resource: Resource representing device control registers. */
 	struct resource *regs_resource;
 
@@ -148,6 +164,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_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index b058ec183bb30ab5c3db17ebaadf2754520a2a1f..97ccf4a73964ed3752ed1a798231c41cc5c70030 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1481,14 +1481,39 @@ static void pvr_remove(struct platform_device *plat_dev)
 }
 
 static const struct of_device_id dt_match[] = {
-	{ .compatible = "img,img-rogue", .data = NULL },
+	{
+		.compatible = "thead,th1520-gpu",
+		.data =
+			&(struct pvr_soc_data)
+			{
+				.power_on = pvr_power_on_sequence_pwrseq,
+				.power_off = pvr_power_off_sequence_pwrseq,
+			},
+	},
+	{
+		.compatible = "img,img-rogue",
+		.data =
+			&(struct pvr_soc_data)
+			{
+				.power_on = pvr_power_on_sequence_manual,
+				.power_off = pvr_power_off_sequence_manual,
+			},
+	},
 
 	/*
 	 * This legacy compatible string was introduced early on before the more generic
 	 * "img,img-rogue" was added. Keep it around here for compatibility, but never use
 	 * "img,img-axe" in new devicetrees.
 	 */
-	{ .compatible = "img,img-axe", .data = NULL },
+	{
+		.compatible = "img,img-axe",
+		.data =
+			&(struct pvr_soc_data)
+			{
+				.power_on = pvr_power_on_sequence_manual,
+				.power_off = pvr_power_off_sequence_manual,
+			},
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
@@ -1513,4 +1538,5 @@ MODULE_DESCRIPTION(PVR_DRIVER_DESC);
 MODULE_LICENSE("Dual MIT/GPL");
 MODULE_IMPORT_NS("DMA_BUF");
 MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");
+MODULE_FIRMWARE("powervr/rogue_36.52.104.182_v1.fw");
 MODULE_FIRMWARE("powervr/rogue_36.53.104.796_v1.fw");
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index 41f5d89e78b854cf6993838868a4416a220b490a..49b66856b9916b1d13efcc3db739de9be2de56b6 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/pwrseq/consumer.h>
 #include <linux/reset.h>
 #include <linux/timer.h>
 #include <linux/types.h>
@@ -234,6 +235,71 @@ pvr_watchdog_init(struct pvr_device *pvr_dev)
 	return 0;
 }
 
+int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev)
+{
+	return pwrseq_power_on(pvr_dev->pwrseq);
+}
+
+int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev)
+{
+	return pwrseq_power_off(pvr_dev->pwrseq);
+}
+
+int pvr_power_on_sequence_manual(struct pvr_device *pvr_dev)
+{
+	int err;
+
+	err = clk_prepare_enable(pvr_dev->core_clk);
+	if (err)
+		return err;
+
+	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;
+
+	/*
+	 * 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;
+
+	return 0;
+
+err_mem_clk_disable:
+	clk_disable_unprepare(pvr_dev->mem_clk);
+err_sys_clk_disable:
+	clk_disable_unprepare(pvr_dev->sys_clk);
+err_core_clk_disable:
+	clk_disable_unprepare(pvr_dev->core_clk);
+
+	return err;
+}
+
+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 +318,7 @@ 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);
+	err = pvr_dev->soc_data->power_off(pvr_dev);
 
 err_drm_dev_exit:
 	drm_dev_exit(idx);
@@ -276,54 +338,22 @@ pvr_power_device_resume(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
-	err = clk_prepare_enable(pvr_dev->core_clk);
+	err = pvr_dev->soc_data->power_on(pvr_dev);
 	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->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);
-
-	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_mem_clk_disable:
-	clk_disable_unprepare(pvr_dev->mem_clk);
-
-err_sys_clk_disable:
-	clk_disable_unprepare(pvr_dev->sys_clk);
-
-err_core_clk_disable:
-	clk_disable_unprepare(pvr_dev->core_clk);
-
+err_power_off:
+	pvr_dev->soc_data->power_off(pvr_dev);
 err_drm_dev_exit:
 	drm_dev_exit(idx);
 
diff --git a/drivers/gpu/drm/imagination/pvr_power.h b/drivers/gpu/drm/imagination/pvr_power.h
index ada85674a7ca762dcf92df40424230e1c3910342..d91d5f3f39b61f81121357f4187b1a6e09b3dec0 100644
--- a/drivers/gpu/drm/imagination/pvr_power.h
+++ b/drivers/gpu/drm/imagination/pvr_power.h
@@ -41,4 +41,10 @@ pvr_power_put(struct pvr_device *pvr_dev)
 int pvr_power_domains_init(struct pvr_device *pvr_dev);
 void pvr_power_domains_fini(struct pvr_device *pvr_dev);
 
+/* Power sequence functions */
+int pvr_power_on_sequence_manual(struct pvr_device *pvr_dev);
+int pvr_power_off_sequence_manual(struct pvr_device *pvr_dev);
+int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev);
+int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev);
+
 #endif /* PVR_POWER_H */

-- 
2.34.1


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

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

[Matt Coster]

[-- Attachment #1.1.1: Type: text/plain, Size: 15269 bytes --]

On 23/06/2025 12:42, 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.
> 
> To cleanly handle the TH1520's specific power requirements in the
> generic driver, this patch implements the "driver match data" pattern.
> The pvr_soc_data struct, associated with a compatible string in the
> of_device_id table, now holds pointers to platform-specific power_on and
> power_off functions.
> 
> At probe time, the driver inspects the assigned power_on function
> pointer. If it points to the pwrseq variant, the driver calls
> devm_pwrseq_get("gpu-power"), requiring a valid sequencer and deferring
> probe on failure. Otherwise, it falls back to its standard manual reset
> initialization.
> 
> The runtime PM callbacks, pvr_power_device_resume() and
> pvr_power_device_suspend(), call the power_on and power_off function
> pointers. Helper functions for both manual and pwrseq-based sequences
> are introduced to support this.

Hi Michal,

My apologies for not responding to previous revisions of this series. In
general, my main earlier complaints were already addressed by others and
the series generally looks good to me.

Just a few notes from me in this and subsequent patches.

> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/gpu/drm/imagination/Kconfig      |   1 +
>  drivers/gpu/drm/imagination/pvr_device.c |  31 +++++++--
>  drivers/gpu/drm/imagination/pvr_device.h |  19 ++++++
>  drivers/gpu/drm/imagination/pvr_drv.c    |  30 ++++++++-
>  drivers/gpu/drm/imagination/pvr_power.c  | 112 ++++++++++++++++++++-----------
>  drivers/gpu/drm/imagination/pvr_power.h  |   6 ++
>  6 files changed, 152 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
> index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..5f9fff43d6baadc42ebf48d91729bfbf27e06caa 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

Given this is (currently) the only platform that requires this
dependency, I'm not super enthusiastic about selecting it here. Given
the modular way the pwrseq code has been written below (which I really
like!), would it make sense to make that code conditional on
CONFIG_POWER_SEQUENCING rather than pulling it in here by default?

>  	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..c1c24c441c821ccce59f7cd3f14544a91ef54ea9 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -23,8 +23,10 @@
>  #include <linux/firmware.h>
>  #include <linux/gfp.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.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>
> @@ -618,6 +620,9 @@ pvr_device_init(struct pvr_device *pvr_dev)
>  	struct device *dev = drm_dev->dev;
>  	int err;
>  
> +	/* Get the platform-specific data based on the compatible string. */
> +	pvr_dev->soc_data = of_device_get_match_data(dev);
> +
>  	/*
>  	 * Setup device parameters. We do this first in case other steps
>  	 * depend on them.
> @@ -631,10 +636,28 @@ 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;
> +	/*
> +	 * For platforms that require it, get the power sequencer.
> +	 * For all others, perform manual reset initialization.
> +	 */
> +	if (pvr_dev->soc_data->power_on == pvr_power_on_sequence_pwrseq) {

Not a huge fan of this check. Semantically it doesn't make a lot of
sense – why would we only care about the power_on callback specifically?
See my comment below.

> +		pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
> +		if (IS_ERR(pvr_dev->pwrseq)) {
> +			/*
> +			 * This platform requires a sequencer. If we can't get
> +			 * it, we must return the error (including -EPROBE_DEFER
> +			 * to wait for the provider to appear)
> +			 */
> +			return dev_err_probe(
> +				dev, PTR_ERR(pvr_dev->pwrseq),
> +				"Failed to get required power sequencer\n");
> +		}
> +	} else {
> +		/* This platform does not use a sequencer, init reset manually. */
> +		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..3f35025e84efac031d3261c849ef9fe105466423 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.
> @@ -57,6 +60,16 @@ struct pvr_fw_version {
>  	u16 major, minor;
>  };
>  
> +/**
> + * struct pvr_soc_data - Platform specific data associated with a compatible string.
> + * @power_on: Pointer to the platform-specific power on function.
> + * @power_off: Pointer to the platform-specific power off function.
> + */
> +struct pvr_soc_data {

Nit: can we make this struct pvr_device_data? It's being used as the
top-level struct of_device_id.data value, which means it may contain
more than just SoC-specific details later on.

> +	int (*power_on)(struct pvr_device *pvr_dev);
> +	int (*power_off)(struct pvr_device *pvr_dev);

I'd prefer to see these two functions extracted to a separate struct so
_that_ structure can be defined as a constant for *_pwrseq and *_manual
variants and have _those constants_ used in the dt_match table (and, for
example, the check above).

> +};
> +
>  /**
>   * struct pvr_device - powervr-specific wrapper for &struct drm_device
>   */
> @@ -98,6 +111,9 @@ struct pvr_device {
>  	/** @fw_version: Firmware version detected at runtime. */
>  	struct pvr_fw_version fw_version;
>  
> +	/** @soc_data: Pointer to platform-specific quirk data. */
> +	const struct pvr_soc_data *soc_data;

With the type rename above, I guess this would fit better as something
like compatible_data or maybe runtime_data? Naming is hard :)

> +
>  	/** @regs_resource: Resource representing device control registers. */
>  	struct resource *regs_resource;
>  
> @@ -148,6 +164,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_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index b058ec183bb30ab5c3db17ebaadf2754520a2a1f..97ccf4a73964ed3752ed1a798231c41cc5c70030 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1481,14 +1481,39 @@ static void pvr_remove(struct platform_device *plat_dev)
>  }
>  
>  static const struct of_device_id dt_match[] = {
> -	{ .compatible = "img,img-rogue", .data = NULL },
> +	{
> +		.compatible = "thead,th1520-gpu",
> +		.data =
> +			&(struct pvr_soc_data)
> +			{
> +				.power_on = pvr_power_on_sequence_pwrseq,
> +				.power_off = pvr_power_off_sequence_pwrseq,
> +			},
> +	},
> +	{
> +		.compatible = "img,img-rogue",
> +		.data =
> +			&(struct pvr_soc_data)
> +			{
> +				.power_on = pvr_power_on_sequence_manual,
> +				.power_off = pvr_power_off_sequence_manual,
> +			},
> +	},

Can you define a "default" instance of this so it can be reused below?

>  
>  	/*
>  	 * This legacy compatible string was introduced early on before the more generic
>  	 * "img,img-rogue" was added. Keep it around here for compatibility, but never use
>  	 * "img,img-axe" in new devicetrees.
>  	 */
> -	{ .compatible = "img,img-axe", .data = NULL },
> +	{
> +		.compatible = "img,img-axe",
> +		.data =
> +			&(struct pvr_soc_data)
> +			{
> +				.power_on = pvr_power_on_sequence_manual,
> +				.power_off = pvr_power_off_sequence_manual,
> +			},
> +	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);
> @@ -1513,4 +1538,5 @@ MODULE_DESCRIPTION(PVR_DRIVER_DESC);
>  MODULE_LICENSE("Dual MIT/GPL");
>  MODULE_IMPORT_NS("DMA_BUF");
>  MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");
> +MODULE_FIRMWARE("powervr/rogue_36.52.104.182_v1.fw");
>  MODULE_FIRMWARE("powervr/rogue_36.53.104.796_v1.fw");
> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index 41f5d89e78b854cf6993838868a4416a220b490a..49b66856b9916b1d13efcc3db739de9be2de56b6 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
> @@ -18,6 +18,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pwrseq/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/timer.h>
>  #include <linux/types.h>
> @@ -234,6 +235,71 @@ pvr_watchdog_init(struct pvr_device *pvr_dev)
>  	return 0;
>  }
>  
> +int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev)
> +{
> +	return pwrseq_power_on(pvr_dev->pwrseq);
> +}
> +
> +int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev)
> +{
> +	return pwrseq_power_off(pvr_dev->pwrseq);
> +}

I'm not sure what the standard method of gracefully handling unsupported
configurations at runtime is, but I suppose we could just have
alternative stub versions of these two functions that error out if
!CONFIG_POWER_SEQUENCING.

> +
> +int pvr_power_on_sequence_manual(struct pvr_device *pvr_dev)
> +{
> +	int err;
> +
> +	err = clk_prepare_enable(pvr_dev->core_clk);
> +	if (err)
> +		return err;
> +
> +	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;
> +
> +	/*
> +	 * 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;
> +
> +	return 0;
> +
> +err_mem_clk_disable:
> +	clk_disable_unprepare(pvr_dev->mem_clk);
> +err_sys_clk_disable:
> +	clk_disable_unprepare(pvr_dev->sys_clk);
> +err_core_clk_disable:
> +	clk_disable_unprepare(pvr_dev->core_clk);

Nit: there were blank lines before each label before.

> +
> +	return err;
> +}
> +
> +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 +318,7 @@ 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);
> +	err = pvr_dev->soc_data->power_off(pvr_dev);
>  
>  err_drm_dev_exit:
>  	drm_dev_exit(idx);
> @@ -276,54 +338,22 @@ pvr_power_device_resume(struct device *dev)
>  	if (!drm_dev_enter(drm_dev, &idx))
>  		return -EIO;
>  
> -	err = clk_prepare_enable(pvr_dev->core_clk);
> +	err = pvr_dev->soc_data->power_on(pvr_dev);
>  	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->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);
> -
> -	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_mem_clk_disable:
> -	clk_disable_unprepare(pvr_dev->mem_clk);
> -
> -err_sys_clk_disable:
> -	clk_disable_unprepare(pvr_dev->sys_clk);
> -
> -err_core_clk_disable:
> -	clk_disable_unprepare(pvr_dev->core_clk);
> -
> +err_power_off:
> +	pvr_dev->soc_data->power_off(pvr_dev);
>  err_drm_dev_exit:
>  	drm_dev_exit(idx);
>  
> diff --git a/drivers/gpu/drm/imagination/pvr_power.h b/drivers/gpu/drm/imagination/pvr_power.h
> index ada85674a7ca762dcf92df40424230e1c3910342..d91d5f3f39b61f81121357f4187b1a6e09b3dec0 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.h
> +++ b/drivers/gpu/drm/imagination/pvr_power.h
> @@ -41,4 +41,10 @@ pvr_power_put(struct pvr_device *pvr_dev)
>  int pvr_power_domains_init(struct pvr_device *pvr_dev);
>  void pvr_power_domains_fini(struct pvr_device *pvr_dev);
>  
> +/* Power sequence functions */
> +int pvr_power_on_sequence_manual(struct pvr_device *pvr_dev);
> +int pvr_power_off_sequence_manual(struct pvr_device *pvr_dev);
> +int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev);
> +int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev);

Perhaps the extracted structure of function pointers could be declared
here as e.g. struct pvr_power_sequence_ops, allowing these functions to
be contained to pvr_power.c.

Cheers,
Matt

> +
>  #endif /* PVR_POWER_H */
> 


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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

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

[Michal Wilczynski]

On 6/24/25 15:53, Matt Coster wrote:
> On 23/06/2025 12:42, 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.
>>
>> To cleanly handle the TH1520's specific power requirements in the
>> generic driver, this patch implements the "driver match data" pattern.
>> The pvr_soc_data struct, associated with a compatible string in the
>> of_device_id table, now holds pointers to platform-specific power_on and
>> power_off functions.
>>
>> At probe time, the driver inspects the assigned power_on function
>> pointer. If it points to the pwrseq variant, the driver calls
>> devm_pwrseq_get("gpu-power"), requiring a valid sequencer and deferring
>> probe on failure. Otherwise, it falls back to its standard manual reset
>> initialization.
>>
>> The runtime PM callbacks, pvr_power_device_resume() and
>> pvr_power_device_suspend(), call the power_on and power_off function
>> pointers. Helper functions for both manual and pwrseq-based sequences
>> are introduced to support this.
> 
> Hi Michal,
> 
> My apologies for not responding to previous revisions of this series. In
> general, my main earlier complaints were already addressed by others and
> the series generally looks good to me.
> 
> Just a few notes from me in this and subsequent patches.
> 
>>

Thanks for the feedback.

I will send an updated revision based on the linux-next and skip patches
that have already been applied to Ulf's and Bartosz's trees.

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

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

[PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Michal Wilczynski]

Update the img,powervr-rogue.yaml 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.

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

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -21,6 +21,11 @@ properties:
           # work with newer dts.
           - const: img,img-axe
           - const: img,img-rogue
+      - items:
+          - enum:
+              - thead,th1520-gpu
+          - const: img,img-bxm-4-64
+          - const: img,img-rogue
       - items:
           - enum:
               - ti,j721s2-gpu
@@ -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


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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Matt Coster]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2461 bytes --]

On 23/06/2025 12:42, Michal Wilczynski wrote:
> Update the img,powervr-rogue.yaml 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.
> 
> The power-domains property requirement for img,img-bxm-4-64 is also
> ensured by adding it to the relevant allOf condition.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -21,6 +21,11 @@ properties:
>            # work with newer dts.
>            - const: img,img-axe
>            - const: img,img-rogue
> +      - items:
> +          - enum:
> +              - thead,th1520-gpu
> +          - const: img,img-bxm-4-64
> +          - const: img,img-rogue
>        - items:
>            - enum:
>                - ti,j721s2-gpu
> @@ -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

This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
really know what the right way to handle that in devicetree is given the
TH1520 appears to expose only a top-level domain for the entire GPU, but
there are definitely two separate domains underneath that as far as the
GPU is concerned (see the attached snippet from integration guide).

Since power nodes are ref-counted anyway, do we just use the same node
for both domains and let the driver up/down-count it twice?

Cheers,
Matt

>      then:
>        properties:
>          power-domains:
> 


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

[-- Attachment #1.1.1.2: Screenshot 2025-06-20 at 11.28.44.png --]
[-- Type: image/png, Size: 12166 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Michal Wilczynski]

On 6/24/25 15:53, Matt Coster wrote:
> On 23/06/2025 12:42, Michal Wilczynski wrote:
>> Update the img,powervr-rogue.yaml 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.
>>
>> The power-domains property requirement for img,img-bxm-4-64 is also
>> ensured by adding it to the relevant allOf condition.
>>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> @@ -21,6 +21,11 @@ properties:
>>            # work with newer dts.
>>            - const: img,img-axe
>>            - const: img,img-rogue
>> +      - items:
>> +          - enum:
>> +              - thead,th1520-gpu
>> +          - const: img,img-bxm-4-64
>> +          - const: img,img-rogue
>>        - items:
>>            - enum:
>>                - ti,j721s2-gpu
>> @@ -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
> 
> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
> really know what the right way to handle that in devicetree is given the
> TH1520 appears to expose only a top-level domain for the entire GPU, but
> there are definitely two separate domains underneath that as far as the
> GPU is concerned (see the attached snippet from integration guide).
> 
> Since power nodes are ref-counted anyway, do we just use the same node
> for both domains and let the driver up/down-count it twice?

Hi Matt,

Thanks for the very helpful insight. That's a great point, it seems the
SoC's design presents a tricky case for the bindings.

I see what you mean about potentially using the same power domain node
twice. My only hesitation is that it might be a bit unclear for someone
reading the devicetree later. Perhaps another option could be to relax
the constraint for this compatible?

Krzysztof, we'd be grateful for your thoughts on how to best model this
situation.

> 
> Cheers,
> Matt
> 
>>      then:
>>        properties:
>>          power-domains:
>>
> 
> 

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

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Krzysztof Kozlowski]

On 25/06/2025 14:45, Michal Wilczynski wrote:
> 
> 
> On 6/24/25 15:53, Matt Coster wrote:
>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>> Update the img,powervr-rogue.yaml 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.
>>>
>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>> ensured by adding it to the relevant allOf condition.
>>>
>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>> @@ -21,6 +21,11 @@ properties:
>>>            # work with newer dts.
>>>            - const: img,img-axe
>>>            - const: img,img-rogue
>>> +      - items:
>>> +          - enum:
>>> +              - thead,th1520-gpu
>>> +          - const: img,img-bxm-4-64
>>> +          - const: img,img-rogue
>>>        - items:
>>>            - enum:
>>>                - ti,j721s2-gpu
>>> @@ -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
>>
>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>> really know what the right way to handle that in devicetree is given the
>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>> there are definitely two separate domains underneath that as far as the
>> GPU is concerned (see the attached snippet from integration guide).
>>
>> Since power nodes are ref-counted anyway, do we just use the same node
>> for both domains and let the driver up/down-count it twice?
> 
> Hi Matt,
> 
> Thanks for the very helpful insight. That's a great point, it seems the
> SoC's design presents a tricky case for the bindings.
> 
> I see what you mean about potentially using the same power domain node
> twice. My only hesitation is that it might be a bit unclear for someone
> reading the devicetree later. Perhaps another option could be to relax
> the constraint for this compatible?
> 
> Krzysztof, we'd be grateful for your thoughts on how to best model this
> situation.


It's your hardware, you should tell us, not me. I don't know how many
power domains you have there, but for sure it is not one AND two domains
the same time. It is either one or two, because power domains are not
the same as regulator supplies.

Best regards,
Krzysztof

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Michal Wilczynski]

On 6/25/25 15:55, Krzysztof Kozlowski wrote:
> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>
>>
>> On 6/24/25 15:53, Matt Coster wrote:
>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>> Update the img,powervr-rogue.yaml 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.
>>>>
>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>> ensured by adding it to the relevant allOf condition.
>>>>
>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>> @@ -21,6 +21,11 @@ properties:
>>>>            # work with newer dts.
>>>>            - const: img,img-axe
>>>>            - const: img,img-rogue
>>>> +      - items:
>>>> +          - enum:
>>>> +              - thead,th1520-gpu
>>>> +          - const: img,img-bxm-4-64
>>>> +          - const: img,img-rogue
>>>>        - items:
>>>>            - enum:
>>>>                - ti,j721s2-gpu
>>>> @@ -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
>>>
>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>> really know what the right way to handle that in devicetree is given the
>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>> there are definitely two separate domains underneath that as far as the
>>> GPU is concerned (see the attached snippet from integration guide).
>>>
>>> Since power nodes are ref-counted anyway, do we just use the same node
>>> for both domains and let the driver up/down-count it twice?
>>
>> Hi Matt,
>>
>> Thanks for the very helpful insight. That's a great point, it seems the
>> SoC's design presents a tricky case for the bindings.
>>
>> I see what you mean about potentially using the same power domain node
>> twice. My only hesitation is that it might be a bit unclear for someone
>> reading the devicetree later. Perhaps another option could be to relax
>> the constraint for this compatible?
>>
>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>> situation.
> 
> 
> It's your hardware, you should tell us, not me. I don't know how many
> power domains you have there, but for sure it is not one AND two domains
> the same time. It is either one or two, because power domains are not
> the same as regulator supplies.

Hi Krzysztof, Matt,

The img,bxm-4-64 GPU IP itself is designed with two separate power
domains. The TH1520 SoC, which integrates this GPU, wires both of these
to a single OS controllable power gate (controlled via mailbox and E902
co-processor).

This means a devicetree for the TH1520 can only ever provide one power
domain for the GPU. However, a generic binding for img,bxm-4-64 should
account for a future SoC that might implement both power domains.

That's why I proposed to relax the constraints on the img,bmx-4-64 GPU.

This makes the binding accurately represent the GPU's full capabilities
while remaining compatible with SoCs like the TH1520 that have a limited
implementation.

Does this seem like the correct and robust approach to you?

> 
> Best regards,
> Krzysztof
> 

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

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Krzysztof Kozlowski]

On 25/06/2025 16:18, Michal Wilczynski wrote:
> 
> 
> On 6/25/25 15:55, Krzysztof Kozlowski wrote:
>> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>>
>>>
>>> On 6/24/25 15:53, Matt Coster wrote:
>>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>>> Update the img,powervr-rogue.yaml 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.
>>>>>
>>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>>> ensured by adding it to the relevant allOf condition.
>>>>>
>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>> @@ -21,6 +21,11 @@ properties:
>>>>>            # work with newer dts.
>>>>>            - const: img,img-axe
>>>>>            - const: img,img-rogue
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - thead,th1520-gpu
>>>>> +          - const: img,img-bxm-4-64
>>>>> +          - const: img,img-rogue
>>>>>        - items:
>>>>>            - enum:
>>>>>                - ti,j721s2-gpu
>>>>> @@ -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
>>>>
>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>>> really know what the right way to handle that in devicetree is given the
>>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>>> there are definitely two separate domains underneath that as far as the
>>>> GPU is concerned (see the attached snippet from integration guide).
>>>>
>>>> Since power nodes are ref-counted anyway, do we just use the same node
>>>> for both domains and let the driver up/down-count it twice?
>>>
>>> Hi Matt,
>>>
>>> Thanks for the very helpful insight. That's a great point, it seems the
>>> SoC's design presents a tricky case for the bindings.
>>>
>>> I see what you mean about potentially using the same power domain node
>>> twice. My only hesitation is that it might be a bit unclear for someone
>>> reading the devicetree later. Perhaps another option could be to relax
>>> the constraint for this compatible?
>>>
>>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>>> situation.
>>
>>
>> It's your hardware, you should tell us, not me. I don't know how many
>> power domains you have there, but for sure it is not one AND two domains
>> the same time. It is either one or two, because power domains are not
>> the same as regulator supplies.
> 
> Hi Krzysztof, Matt,
> 
> The img,bxm-4-64 GPU IP itself is designed with two separate power
> domains. The TH1520 SoC, which integrates this GPU, wires both of these
> to a single OS controllable power gate (controlled via mailbox and E902
> co-processor).

This helps... and also sounds a lot like regulator supplies, not power
domains. :/

> 
> This means a devicetree for the TH1520 can only ever provide one power
> domain for the GPU. However, a generic binding for img,bxm-4-64 should

If this was a supply, you would have two supplies. Anyway internal
wirings of GPU do not matter in such case and more important what the
SoC has wired. And it has one power domain.


> account for a future SoC that might implement both power domains.
> 
> That's why I proposed to relax the constraints on the img,bmx-4-64 GPU.

This should be constrained per each device, so 1 for you and 2 for
everyone else.

Best regards,
Krzysztof

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Matt Coster]

[-- Attachment #1.1.1: Type: text/plain, Size: 6533 bytes --]

On 25/06/2025 15:41, Krzysztof Kozlowski wrote:
> On 25/06/2025 16:18, Michal Wilczynski wrote:
>>
>>
>> On 6/25/25 15:55, Krzysztof Kozlowski wrote:
>>> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>>>
>>>>
>>>> On 6/24/25 15:53, Matt Coster wrote:
>>>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>>>> Update the img,powervr-rogue.yaml 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.
>>>>>>
>>>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>>>> ensured by adding it to the relevant allOf condition.
>>>>>>
>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>> @@ -21,6 +21,11 @@ properties:
>>>>>>            # work with newer dts.
>>>>>>            - const: img,img-axe
>>>>>>            - const: img,img-rogue
>>>>>> +      - items:
>>>>>> +          - enum:
>>>>>> +              - thead,th1520-gpu
>>>>>> +          - const: img,img-bxm-4-64
>>>>>> +          - const: img,img-rogue
>>>>>>        - items:
>>>>>>            - enum:
>>>>>>                - ti,j721s2-gpu
>>>>>> @@ -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
>>>>>
>>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>>>> really know what the right way to handle that in devicetree is given the
>>>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>>>> there are definitely two separate domains underneath that as far as the
>>>>> GPU is concerned (see the attached snippet from integration guide).
>>>>>
>>>>> Since power nodes are ref-counted anyway, do we just use the same node
>>>>> for both domains and let the driver up/down-count it twice?
>>>>
>>>> Hi Matt,
>>>>
>>>> Thanks for the very helpful insight. That's a great point, it seems the
>>>> SoC's design presents a tricky case for the bindings.
>>>>
>>>> I see what you mean about potentially using the same power domain node
>>>> twice. My only hesitation is that it might be a bit unclear for someone
>>>> reading the devicetree later. Perhaps another option could be to relax
>>>> the constraint for this compatible?
>>>>
>>>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>>>> situation.
>>>
>>>
>>> It's your hardware, you should tell us, not me. I don't know how many
>>> power domains you have there, but for sure it is not one AND two domains
>>> the same time. It is either one or two, because power domains are not
>>> the same as regulator supplies.
>>
>> Hi Krzysztof, Matt,
>>
>> The img,bxm-4-64 GPU IP itself is designed with two separate power
>> domains. The TH1520 SoC, which integrates this GPU, wires both of these
>> to a single OS controllable power gate (controlled via mailbox and E902
>> co-processor).
> 
> This helps... and also sounds a lot like regulator supplies, not power
> domains. :/

Apologies for taking so long to get back to you with this, I wanted to
make sure I had the whole picture from our side before commenting again.

From the GPU side, a "typical" integration of BXM-4-64 would use two
power domains.

Typically, these domains exist in silicon, regardless of whether they
are exposed to the host OS, because the SoC's power controller must have
control over them. As part of normal operation, the GPU firmware (always
in domain "a" on Rogue) will request the power-up/down of the other
domains, including during the initial boot sequence. This all happens
transparently to the OS. The GPU block itself has no power gating at
that level, it relies entirely on the SoC integration.

However, it turns out (unknown to me until very recently) that this
functionality is optional. The integrator can opt to forego the
power-saving functionality afforded by firmware-controlled power gating
and just throw everything into a single domain, which appears to be
what's happened here.

My only remaining issue here, then, is the naming. Since this
integration doesn't use discrete domains, saying it has one domain
called "a" isn't correct*. We should either:

 - Drop the name altogether for this integration (and others like it
   that don't use the low-power functionality, if there are any), or
 - Come up with a new domain name to signal this explicitly (perhaps
   simply "gpu")? Something that's unlikely to clash with the "real"
   names that are going to start appearing in the Volcanic bindings
   (where we finally ditched "a", "b", etc.).

Cheers,
Matt

*Yes, I know that's what we said for the AXE-1-16M, but that tiny GPU is
the exception to the rule; AFAIK it's the only one we've ever produced
that truly has only one power domain.

> 
>>
>> This means a devicetree for the TH1520 can only ever provide one power
>> domain for the GPU. However, a generic binding for img,bxm-4-64 should
> 
> If this was a supply, you would have two supplies. Anyway internal
> wirings of GPU do not matter in such case and more important what the
> SoC has wired. And it has one power domain.
> 
> 
>> account for a future SoC that might implement both power domains.
>>
>> That's why I proposed to relax the constraints on the img,bmx-4-64 GPU.
> 
> This should be constrained per each device, so 1 for you and 2 for
> everyone else.
> 
> Best regards,
> Krzysztof


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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Michal Wilczynski]

On 7/23/25 11:45, Matt Coster wrote:
> On 25/06/2025 15:41, Krzysztof Kozlowski wrote:
>> On 25/06/2025 16:18, Michal Wilczynski wrote:
>>>
>>>
>>> On 6/25/25 15:55, Krzysztof Kozlowski wrote:
>>>> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>>>>
>>>>>
>>>>> On 6/24/25 15:53, Matt Coster wrote:
>>>>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>>>>> Update the img,powervr-rogue.yaml 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.
>>>>>>>
>>>>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>>>>> ensured by adding it to the relevant allOf condition.
>>>>>>>
>>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>> @@ -21,6 +21,11 @@ properties:
>>>>>>>            # work with newer dts.
>>>>>>>            - const: img,img-axe
>>>>>>>            - const: img,img-rogue
>>>>>>> +      - items:
>>>>>>> +          - enum:
>>>>>>> +              - thead,th1520-gpu
>>>>>>> +          - const: img,img-bxm-4-64
>>>>>>> +          - const: img,img-rogue
>>>>>>>        - items:
>>>>>>>            - enum:
>>>>>>>                - ti,j721s2-gpu
>>>>>>> @@ -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
>>>>>>
>>>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>>>>> really know what the right way to handle that in devicetree is given the
>>>>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>>>>> there are definitely two separate domains underneath that as far as the
>>>>>> GPU is concerned (see the attached snippet from integration guide).
>>>>>>
>>>>>> Since power nodes are ref-counted anyway, do we just use the same node
>>>>>> for both domains and let the driver up/down-count it twice?
>>>>>
>>>>> Hi Matt,
>>>>>
>>>>> Thanks for the very helpful insight. That's a great point, it seems the
>>>>> SoC's design presents a tricky case for the bindings.
>>>>>
>>>>> I see what you mean about potentially using the same power domain node
>>>>> twice. My only hesitation is that it might be a bit unclear for someone
>>>>> reading the devicetree later. Perhaps another option could be to relax
>>>>> the constraint for this compatible?
>>>>>
>>>>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>>>>> situation.
>>>>
>>>>
>>>> It's your hardware, you should tell us, not me. I don't know how many
>>>> power domains you have there, but for sure it is not one AND two domains
>>>> the same time. It is either one or two, because power domains are not
>>>> the same as regulator supplies.
>>>
>>> Hi Krzysztof, Matt,
>>>
>>> The img,bxm-4-64 GPU IP itself is designed with two separate power
>>> domains. The TH1520 SoC, which integrates this GPU, wires both of these
>>> to a single OS controllable power gate (controlled via mailbox and E902
>>> co-processor).
>>
>> This helps... and also sounds a lot like regulator supplies, not power
>> domains. :/
> 
> Apologies for taking so long to get back to you with this, I wanted to
> make sure I had the whole picture from our side before commenting again.
> 
> From the GPU side, a "typical" integration of BXM-4-64 would use two
> power domains.
> 
> Typically, these domains exist in silicon, regardless of whether they
> are exposed to the host OS, because the SoC's power controller must have
> control over them. As part of normal operation, the GPU firmware (always
> in domain "a" on Rogue) will request the power-up/down of the other
> domains, including during the initial boot sequence. This all happens
> transparently to the OS. The GPU block itself has no power gating at
> that level, it relies entirely on the SoC integration.
> 
> However, it turns out (unknown to me until very recently) that this
> functionality is optional. The integrator can opt to forego the
> power-saving functionality afforded by firmware-controlled power gating
> and just throw everything into a single domain, which appears to be
> what's happened here.
> 
> My only remaining issue here, then, is the naming. Since this
> integration doesn't use discrete domains, saying it has one domain
> called "a" isn't correct*. We should either:
> 
>  - Drop the name altogether for this integration (and others like it
>    that don't use the low-power functionality, if there are any), or

Hi Matt,

Thanks for the detailed explanation, that clears things up perfectly.

I agree with your assessment. Dropping the power-domain-names property
for this integration seems like the cleanest solution. As you pointed
out, since the OS sees only a single, undifferentiated power domain,
giving it a name like "gpu" would be redundant. This approach correctly
models the hardware without setting a potentially confusing precedent.

To follow through on this, I assume we'll need to adjust
pvr_power_domains_init() to handle nodes that don't have the
power-domain-names property. Does that sound right to you?

>  - Come up with a new domain name to signal this explicitly (perhaps
>    simply "gpu")? Something that's unlikely to clash with the "real"
>    names that are going to start appearing in the Volcanic bindings
>    (where we finally ditched "a", "b", etc.).
> 
> Cheers,
> Matt
> 
> *Yes, I know that's what we said for the AXE-1-16M, but that tiny GPU is
> the exception to the rule; AFAIK it's the only one we've ever produced
> that truly has only one power domain.
> 
>>
>>>
>>> This means a devicetree for the TH1520 can only ever provide one power
>>> domain for the GPU. However, a generic binding for img,bxm-4-64 should
>>
>> If this was a supply, you would have two supplies. Anyway internal
>> wirings of GPU do not matter in such case and more important what the
>> SoC has wired. And it has one power domain.
>>
>>
>>> account for a future SoC that might implement both power domains.
>>>
>>> That's why I proposed to relax the constraints on the img,bmx-4-64 GPU.
>>
>> This should be constrained per each device, so 1 for you and 2 for
>> everyone else.
>>
>> Best regards,
>> Krzysztof
> 
> 

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

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Matt Coster]

[-- Attachment #1.1.1: Type: text/plain, Size: 8651 bytes --]

On 23/07/2025 17:26, Michal Wilczynski wrote:
> On 7/23/25 11:45, Matt Coster wrote:
>> On 25/06/2025 15:41, Krzysztof Kozlowski wrote:
>>> On 25/06/2025 16:18, Michal Wilczynski wrote:
>>>>
>>>>
>>>> On 6/25/25 15:55, Krzysztof Kozlowski wrote:
>>>>> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>>>>>
>>>>>>
>>>>>> On 6/24/25 15:53, Matt Coster wrote:
>>>>>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>>>>>> Update the img,powervr-rogue.yaml 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.
>>>>>>>>
>>>>>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>>>>>> ensured by adding it to the relevant allOf condition.
>>>>>>>>
>>>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>>> @@ -21,6 +21,11 @@ properties:
>>>>>>>>            # work with newer dts.
>>>>>>>>            - const: img,img-axe
>>>>>>>>            - const: img,img-rogue
>>>>>>>> +      - items:
>>>>>>>> +          - enum:
>>>>>>>> +              - thead,th1520-gpu
>>>>>>>> +          - const: img,img-bxm-4-64
>>>>>>>> +          - const: img,img-rogue
>>>>>>>>        - items:
>>>>>>>>            - enum:
>>>>>>>>                - ti,j721s2-gpu
>>>>>>>> @@ -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
>>>>>>>
>>>>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>>>>>> really know what the right way to handle that in devicetree is given the
>>>>>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>>>>>> there are definitely two separate domains underneath that as far as the
>>>>>>> GPU is concerned (see the attached snippet from integration guide).
>>>>>>>
>>>>>>> Since power nodes are ref-counted anyway, do we just use the same node
>>>>>>> for both domains and let the driver up/down-count it twice?
>>>>>>
>>>>>> Hi Matt,
>>>>>>
>>>>>> Thanks for the very helpful insight. That's a great point, it seems the
>>>>>> SoC's design presents a tricky case for the bindings.
>>>>>>
>>>>>> I see what you mean about potentially using the same power domain node
>>>>>> twice. My only hesitation is that it might be a bit unclear for someone
>>>>>> reading the devicetree later. Perhaps another option could be to relax
>>>>>> the constraint for this compatible?
>>>>>>
>>>>>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>>>>>> situation.
>>>>>
>>>>>
>>>>> It's your hardware, you should tell us, not me. I don't know how many
>>>>> power domains you have there, but for sure it is not one AND two domains
>>>>> the same time. It is either one or two, because power domains are not
>>>>> the same as regulator supplies.
>>>>
>>>> Hi Krzysztof, Matt,
>>>>
>>>> The img,bxm-4-64 GPU IP itself is designed with two separate power
>>>> domains. The TH1520 SoC, which integrates this GPU, wires both of these
>>>> to a single OS controllable power gate (controlled via mailbox and E902
>>>> co-processor).
>>>
>>> This helps... and also sounds a lot like regulator supplies, not power
>>> domains. :/
>>
>> Apologies for taking so long to get back to you with this, I wanted to
>> make sure I had the whole picture from our side before commenting again.
>>
>> From the GPU side, a "typical" integration of BXM-4-64 would use two
>> power domains.
>>
>> Typically, these domains exist in silicon, regardless of whether they
>> are exposed to the host OS, because the SoC's power controller must have
>> control over them. As part of normal operation, the GPU firmware (always
>> in domain "a" on Rogue) will request the power-up/down of the other
>> domains, including during the initial boot sequence. This all happens
>> transparently to the OS. The GPU block itself has no power gating at
>> that level, it relies entirely on the SoC integration.
>>
>> However, it turns out (unknown to me until very recently) that this
>> functionality is optional. The integrator can opt to forego the
>> power-saving functionality afforded by firmware-controlled power gating
>> and just throw everything into a single domain, which appears to be
>> what's happened here.
>>
>> My only remaining issue here, then, is the naming. Since this
>> integration doesn't use discrete domains, saying it has one domain
>> called "a" isn't correct*. We should either:
>>
>>  - Drop the name altogether for this integration (and others like it
>>    that don't use the low-power functionality, if there are any), or
> 
> Hi Matt,
> 
> Thanks for the detailed explanation, that clears things up perfectly.

I'm glad I could get to the bottom of this one, it was bothering me too!

> 
> I agree with your assessment. Dropping the power-domain-names property
> for this integration seems like the cleanest solution. As you pointed
> out, since the OS sees only a single, undifferentiated power domain,
> giving it a name like "gpu" would be redundant. This approach correctly
> models the hardware without setting a potentially confusing precedent.

That seems reasonable. I was very much on the fence for this one, so
I'll happily go along with dropping the name altogether.

Just to make sure I understand correctly, the change here would be to
move "required: - power-domain-names" from "img,img-rogue" to every
conditional block that constrains the number of domains?

Can we add negative constraints in conditionals? Then we could add
"power-domain-names: false" to the "thead,th1520-gpu" conditional block
alongside "power-domains: maxItems: 1".

> 
> To follow through on this, I assume we'll need to adjust
> pvr_power_domains_init() to handle nodes that don't have the
> power-domain-names property. Does that sound right to you?

You should get away without making any code changes here, since we
already shortcut on "domain_count <= 1" to just allow the pm_runtime to
deal with the single (or missing) domain directly.

If we ever start controlling the individual domains ourselves from the
kernel (rather than just ensuring they all come on and off in the
correct sequence), we can add checks/handling for the no-name case then.

Cheers,
Matt

> 
>>  - Come up with a new domain name to signal this explicitly (perhaps
>>    simply "gpu")? Something that's unlikely to clash with the "real"
>>    names that are going to start appearing in the Volcanic bindings
>>    (where we finally ditched "a", "b", etc.).
>>
>> Cheers,
>> Matt
>>
>> *Yes, I know that's what we said for the AXE-1-16M, but that tiny GPU is
>> the exception to the rule; AFAIK it's the only one we've ever produced
>> that truly has only one power domain.
>>
>>>
>>>>
>>>> This means a devicetree for the TH1520 can only ever provide one power
>>>> domain for the GPU. However, a generic binding for img,bxm-4-64 should
>>>
>>> If this was a supply, you would have two supplies. Anyway internal
>>> wirings of GPU do not matter in such case and more important what the
>>> SoC has wired. And it has one power domain.
>>>
>>>
>>>> account for a future SoC that might implement both power domains.
>>>>
>>>> That's why I proposed to relax the constraints on the img,bmx-4-64 GPU.
>>>
>>> This should be constrained per each device, so 1 for you and 2 for
>>> everyone else.
>>>
>>> Best regards,
>>> Krzysztof
>>
>>
> 
> Best regards,


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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Michal Wilczynski]

On 7/23/25 18:50, Matt Coster wrote:
> On 23/07/2025 17:26, Michal Wilczynski wrote:
>> On 7/23/25 11:45, Matt Coster wrote:
>>> On 25/06/2025 15:41, Krzysztof Kozlowski wrote:
>>>> On 25/06/2025 16:18, Michal Wilczynski wrote:
>>>>>
>>>>>
>>>>> On 6/25/25 15:55, Krzysztof Kozlowski wrote:
>>>>>> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/24/25 15:53, Matt Coster wrote:
>>>>>>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>>>>>>> Update the img,powervr-rogue.yaml 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.
>>>>>>>>>
>>>>>>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>>>>>>> ensured by adding it to the relevant allOf condition.
>>>>>>>>>
>>>>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>>>> @@ -21,6 +21,11 @@ properties:
>>>>>>>>>            # work with newer dts.
>>>>>>>>>            - const: img,img-axe
>>>>>>>>>            - const: img,img-rogue
>>>>>>>>> +      - items:
>>>>>>>>> +          - enum:
>>>>>>>>> +              - thead,th1520-gpu
>>>>>>>>> +          - const: img,img-bxm-4-64
>>>>>>>>> +          - const: img,img-rogue
>>>>>>>>>        - items:
>>>>>>>>>            - enum:
>>>>>>>>>                - ti,j721s2-gpu
>>>>>>>>> @@ -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
>>>>>>>>
>>>>>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>>>>>>> really know what the right way to handle that in devicetree is given the
>>>>>>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>>>>>>> there are definitely two separate domains underneath that as far as the
>>>>>>>> GPU is concerned (see the attached snippet from integration guide).
>>>>>>>>
>>>>>>>> Since power nodes are ref-counted anyway, do we just use the same node
>>>>>>>> for both domains and let the driver up/down-count it twice?
>>>>>>>
>>>>>>> Hi Matt,
>>>>>>>
>>>>>>> Thanks for the very helpful insight. That's a great point, it seems the
>>>>>>> SoC's design presents a tricky case for the bindings.
>>>>>>>
>>>>>>> I see what you mean about potentially using the same power domain node
>>>>>>> twice. My only hesitation is that it might be a bit unclear for someone
>>>>>>> reading the devicetree later. Perhaps another option could be to relax
>>>>>>> the constraint for this compatible?
>>>>>>>
>>>>>>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>>>>>>> situation.
>>>>>>
>>>>>>
>>>>>> It's your hardware, you should tell us, not me. I don't know how many
>>>>>> power domains you have there, but for sure it is not one AND two domains
>>>>>> the same time. It is either one or two, because power domains are not
>>>>>> the same as regulator supplies.
>>>>>
>>>>> Hi Krzysztof, Matt,
>>>>>
>>>>> The img,bxm-4-64 GPU IP itself is designed with two separate power
>>>>> domains. The TH1520 SoC, which integrates this GPU, wires both of these
>>>>> to a single OS controllable power gate (controlled via mailbox and E902
>>>>> co-processor).
>>>>
>>>> This helps... and also sounds a lot like regulator supplies, not power
>>>> domains. :/
>>>
>>> Apologies for taking so long to get back to you with this, I wanted to
>>> make sure I had the whole picture from our side before commenting again.
>>>
>>> From the GPU side, a "typical" integration of BXM-4-64 would use two
>>> power domains.
>>>
>>> Typically, these domains exist in silicon, regardless of whether they
>>> are exposed to the host OS, because the SoC's power controller must have
>>> control over them. As part of normal operation, the GPU firmware (always
>>> in domain "a" on Rogue) will request the power-up/down of the other
>>> domains, including during the initial boot sequence. This all happens
>>> transparently to the OS. The GPU block itself has no power gating at
>>> that level, it relies entirely on the SoC integration.
>>>
>>> However, it turns out (unknown to me until very recently) that this
>>> functionality is optional. The integrator can opt to forego the
>>> power-saving functionality afforded by firmware-controlled power gating
>>> and just throw everything into a single domain, which appears to be
>>> what's happened here.
>>>
>>> My only remaining issue here, then, is the naming. Since this
>>> integration doesn't use discrete domains, saying it has one domain
>>> called "a" isn't correct*. We should either:
>>>
>>>  - Drop the name altogether for this integration (and others like it
>>>    that don't use the low-power functionality, if there are any), or
>>
>> Hi Matt,
>>
>> Thanks for the detailed explanation, that clears things up perfectly.
> 
> I'm glad I could get to the bottom of this one, it was bothering me too!
> 
>>
>> I agree with your assessment. Dropping the power-domain-names property
>> for this integration seems like the cleanest solution. As you pointed
>> out, since the OS sees only a single, undifferentiated power domain,
>> giving it a name like "gpu" would be redundant. This approach correctly
>> models the hardware without setting a potentially confusing precedent.
> 
> That seems reasonable. I was very much on the fence for this one, so
> I'll happily go along with dropping the name altogether.
> 
> Just to make sure I understand correctly, the change here would be to
> move "required: - power-domain-names" from "img,img-rogue" to every
> conditional block that constrains the number of domains?
> 
> Can we add negative constraints in conditionals? Then we could add
> "power-domain-names: false" to the "thead,th1520-gpu" conditional block
> alongside "power-domains: maxItems: 1".

Yeah the diff with v7 would look like so:
diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index 263c40c8438e..338746f39cbb 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -89,6 +89,11 @@ allOf:
         compatible:
           contains:
             const: img,img-rogue
+      not:
+        properties:
+          compatible:
+            contains:
+              const: thead,th1520-gpu
     then:
       required:
         - power-domains
@@ -100,11 +105,12 @@ allOf:
           contains:
             const: thead,th1520-gpu
     then:
+      required:
+        - power-domains
       properties:
         power-domains:
           maxItems: 1
-        power-domain-names:
-          maxItems: 1
+        power-domain-names: false
 
   - if:
       properties:


This change ensures power-domain-names is required for all img,img-rogue
devices, except for thead,th1520-gpu. For the thead specifically,
power-domains remains required, but power-domain-names is explicitly
forbidden.

> 
>>
>> To follow through on this, I assume we'll need to adjust
>> pvr_power_domains_init() to handle nodes that don't have the
>> power-domain-names property. Does that sound right to you?
> 
> You should get away without making any code changes here, since we
> already shortcut on "domain_count <= 1" to just allow the pm_runtime to
> deal with the single (or missing) domain directly.

Great !

> 
> If we ever start controlling the individual domains ourselves from the
> kernel (rather than just ensuring they all come on and off in the
> correct sequence), we can add checks/handling for the no-name case then.
> 
> Cheers,
> Matt
> 
>>
>>>  - Come up with a new domain name to signal this explicitly (perhaps
>>>    simply "gpu")? Something that's unlikely to clash with the "real"
>>>    names that are going to start appearing in the Volcanic bindings
>>>    (where we finally ditched "a", "b", etc.).
>>>
>>> Cheers,
>>> Matt
>>>
>>> *Yes, I know that's what we said for the AXE-1-16M, but that tiny GPU is
>>> the exception to the rule; AFAIK it's the only one we've ever produced
>>> that truly has only one power domain.
>>>
>>>>
>>>>>
>>>>> This means a devicetree for the TH1520 can only ever provide one power
>>>>> domain for the GPU. However, a generic binding for img,bxm-4-64 should
>>>>
>>>> If this was a supply, you would have two supplies. Anyway internal
>>>> wirings of GPU do not matter in such case and more important what the
>>>> SoC has wired. And it has one power domain.
>>>>
>>>>
>>>>> account for a future SoC that might implement both power domains.
>>>>>
>>>>> That's why I proposed to relax the constraints on the img,bmx-4-64 GPU.
>>>>
>>>> This should be constrained per each device, so 1 for you and 2 for
>>>> everyone else.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>>
>>
>> Best regards,
> 
> 

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

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

Re: [PATCH v6 5/8] dt-bindings: gpu: img,powervr-rogue: Add TH1520 GPU compatible

[Krzysztof Kozlowski]

On 23/07/2025 20:25, Michal Wilczynski wrote:
> 
> 
> On 7/23/25 18:50, Matt Coster wrote:
>> On 23/07/2025 17:26, Michal Wilczynski wrote:
>>> On 7/23/25 11:45, Matt Coster wrote:
>>>> On 25/06/2025 15:41, Krzysztof Kozlowski wrote:
>>>>> On 25/06/2025 16:18, Michal Wilczynski wrote:
>>>>>>
>>>>>>
>>>>>> On 6/25/25 15:55, Krzysztof Kozlowski wrote:
>>>>>>> On 25/06/2025 14:45, Michal Wilczynski wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/24/25 15:53, Matt Coster wrote:
>>>>>>>>> On 23/06/2025 12:42, Michal Wilczynski wrote:
>>>>>>>>>> Update the img,powervr-rogue.yaml 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.
>>>>>>>>>>
>>>>>>>>>> The power-domains property requirement for img,img-bxm-4-64 is also
>>>>>>>>>> ensured by adding it to the relevant allOf condition.
>>>>>>>>>>
>>>>>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>>>>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>> 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..9b241a0c1f5941dc58a1e23970f6d3773d427c22 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>>>>>>>>>> @@ -21,6 +21,11 @@ properties:
>>>>>>>>>>            # work with newer dts.
>>>>>>>>>>            - const: img,img-axe
>>>>>>>>>>            - const: img,img-rogue
>>>>>>>>>> +      - items:
>>>>>>>>>> +          - enum:
>>>>>>>>>> +              - thead,th1520-gpu
>>>>>>>>>> +          - const: img,img-bxm-4-64
>>>>>>>>>> +          - const: img,img-rogue
>>>>>>>>>>        - items:
>>>>>>>>>>            - enum:
>>>>>>>>>>                - ti,j721s2-gpu
>>>>>>>>>> @@ -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
>>>>>>>>>
>>>>>>>>> This isn't right – BXM-4-64 has two power domains like BXS-4-64. I don't
>>>>>>>>> really know what the right way to handle that in devicetree is given the
>>>>>>>>> TH1520 appears to expose only a top-level domain for the entire GPU, but
>>>>>>>>> there are definitely two separate domains underneath that as far as the
>>>>>>>>> GPU is concerned (see the attached snippet from integration guide).
>>>>>>>>>
>>>>>>>>> Since power nodes are ref-counted anyway, do we just use the same node
>>>>>>>>> for both domains and let the driver up/down-count it twice?
>>>>>>>>
>>>>>>>> Hi Matt,
>>>>>>>>
>>>>>>>> Thanks for the very helpful insight. That's a great point, it seems the
>>>>>>>> SoC's design presents a tricky case for the bindings.
>>>>>>>>
>>>>>>>> I see what you mean about potentially using the same power domain node
>>>>>>>> twice. My only hesitation is that it might be a bit unclear for someone
>>>>>>>> reading the devicetree later. Perhaps another option could be to relax
>>>>>>>> the constraint for this compatible?
>>>>>>>>
>>>>>>>> Krzysztof, we'd be grateful for your thoughts on how to best model this
>>>>>>>> situation.
>>>>>>>
>>>>>>>
>>>>>>> It's your hardware, you should tell us, not me. I don't know how many
>>>>>>> power domains you have there, but for sure it is not one AND two domains
>>>>>>> the same time. It is either one or two, because power domains are not
>>>>>>> the same as regulator supplies.
>>>>>>
>>>>>> Hi Krzysztof, Matt,
>>>>>>
>>>>>> The img,bxm-4-64 GPU IP itself is designed with two separate power
>>>>>> domains. The TH1520 SoC, which integrates this GPU, wires both of these
>>>>>> to a single OS controllable power gate (controlled via mailbox and E902
>>>>>> co-processor).
>>>>>
>>>>> This helps... and also sounds a lot like regulator supplies, not power
>>>>> domains. :/
>>>>
>>>> Apologies for taking so long to get back to you with this, I wanted to
>>>> make sure I had the whole picture from our side before commenting again.
>>>>
>>>> From the GPU side, a "typical" integration of BXM-4-64 would use two
>>>> power domains.
>>>>
>>>> Typically, these domains exist in silicon, regardless of whether they
>>>> are exposed to the host OS, because the SoC's power controller must have
>>>> control over them. As part of normal operation, the GPU firmware (always
>>>> in domain "a" on Rogue) will request the power-up/down of the other
>>>> domains, including during the initial boot sequence. This all happens
>>>> transparently to the OS. The GPU block itself has no power gating at
>>>> that level, it relies entirely on the SoC integration.
>>>>
>>>> However, it turns out (unknown to me until very recently) that this
>>>> functionality is optional. The integrator can opt to forego the
>>>> power-saving functionality afforded by firmware-controlled power gating
>>>> and just throw everything into a single domain, which appears to be
>>>> what's happened here.
>>>>
>>>> My only remaining issue here, then, is the naming. Since this
>>>> integration doesn't use discrete domains, saying it has one domain
>>>> called "a" isn't correct*. We should either:
>>>>
>>>>  - Drop the name altogether for this integration (and others like it
>>>>    that don't use the low-power functionality, if there are any), or
>>>
>>> Hi Matt,
>>>
>>> Thanks for the detailed explanation, that clears things up perfectly.
>>
>> I'm glad I could get to the bottom of this one, it was bothering me too!
>>
>>>
>>> I agree with your assessment. Dropping the power-domain-names property
>>> for this integration seems like the cleanest solution. As you pointed
>>> out, since the OS sees only a single, undifferentiated power domain,
>>> giving it a name like "gpu" would be redundant. This approach correctly
>>> models the hardware without setting a potentially confusing precedent.
>>
>> That seems reasonable. I was very much on the fence for this one, so
>> I'll happily go along with dropping the name altogether.
>>
>> Just to make sure I understand correctly, the change here would be to
>> move "required: - power-domain-names" from "img,img-rogue" to every
>> conditional block that constrains the number of domains?
>>
>> Can we add negative constraints in conditionals? Then we could add
>> "power-domain-names: false" to the "thead,th1520-gpu" conditional block
>> alongside "power-domains: maxItems: 1".
> 
> Yeah the diff with v7 would look like so:
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index 263c40c8438e..338746f39cbb 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -89,6 +89,11 @@ allOf:
>          compatible:
>            contains:
>              const: img,img-rogue
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: thead,th1520-gpu

No, don't do that. Anyway, if you are going to rewrite patch, you MUST
drop all tags, document it and explicitly ask for re-review.

Best regards,
Krzysztof

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

[PATCH v6 6/8] riscv: dts: thead: th1520: Add GPU clkgen reset to AON node

[Michal Wilczynski]

Add the "gpu-clkgen" reset property to the AON device tree node. This
allows the AON power domain driver to detect the capability to power
sequence the GPU and spawn the necessary pwrseq-thead-gpu auxiliary
driver for managing the GPU's complex power sequence.

This commit also adds the prerequisite
dt-bindings/reset/thead,th1520-reset.h include to make the
TH1520_RESET_ID_GPU_CLKGEN available. This include was previously
dropped during a conflict resolution [1].

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

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Drew Fustini <drew@pdp7.com>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 1db0054c4e093400e9dbebcee5fcfa5b5cae6e32..f3f5db0201ab8c0306d4d63072a1573431e51893 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";
@@ -234,6 +235,8 @@ aon: aon {
 		compatible = "thead,th1520-aon";
 		mboxes = <&mbox_910t 1>;
 		mbox-names = "aon";
+		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
+		reset-names = "gpu-clkgen";
 		#power-domain-cells = <1>;
 	};
 

-- 
2.34.1


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

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

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Drew Fustini <drew@pdp7.com>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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 f3f5db0201ab8c0306d4d63072a1573431e51893..c8447eef36c3a6e92d768658b6b19dfeb59a47c4 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>;
@@ -500,6 +507,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


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

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

Add a dependency on MMU to fix a build warning on RISC-V configurations
without an MMU.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
index 5f9fff43d6baadc42ebf48d91729bfbf27e06caa..a7da858a5b301e8f088e3e22f5641feb2e078681 100644
--- a/drivers/gpu/drm/imagination/Kconfig
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -3,9 +3,10 @@
 
 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
+	depends on MMU
 	select DRM_EXEC
 	select DRM_GEM_SHMEM_HELPER
 	select DRM_SCHED

-- 
2.34.1


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

Re: [PATCH v6 8/8] drm/imagination: Enable PowerVR driver for RISC-V

[Matt Coster]

[-- Attachment #1.1.1: Type: text/plain, Size: 3553 bytes --]

On 23/06/2025 12:42, Michal Wilczynski wrote:
> 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.
> 
> Add a dependency on MMU to fix a build warning on RISC-V configurations
> without an MMU.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/gpu/drm/imagination/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
> index 5f9fff43d6baadc42ebf48d91729bfbf27e06caa..a7da858a5b301e8f088e3e22f5641feb2e078681 100644
> --- a/drivers/gpu/drm/imagination/Kconfig
> +++ b/drivers/gpu/drm/imagination/Kconfig
> @@ -3,9 +3,10 @@
>  
>  config DRM_POWERVR
>  	tristate "Imagination Technologies PowerVR (Series 6 and later) & IMG Graphics"
> -	depends on ARM64
> +	depends on (ARM64 || RISCV)

There were two issues you encountered when enabling COMPILE_TEST in v5,
both of which are somewhat simple to workaround but expose underlying
assumptions we made during early development.

The first [1] is due to us assuming a 64-bit platform, which was never a
problem with the ARM64 dependency, but may actually be a problem with
RISCV given this allows for 32-bit as well. You should probably make
this (RISCV && 64BIT) until the implicit 64-bit dependency can be worked
out.

Somewhat related, we also assume a little-endian host. Technically ARM64
can also be big-endian, you just don't encounter that in the wild too
often so it's never been a "real" issue. I do wonder if swapping out
(ARM64 || RISCV) for (64BIT && CPU_LITTLE_ENDIAN) entirely would be a
reasonable change, perhaps for another day though...

The other [2] is slightly more subtle. To keep things straightforward,
we currently map CPU pages to GPU pages 1:1, meaning we use the CPU page
size to define the GPU page size. That GPU page size is configurable,
but does not support every possible size the CPU could support on any
architecture. The failing test there was sparc64 with an 8K page size
causing no GPU page size to be defined. See the #if/#elif ladder at the
top of pvr_mmu.c for the supported sizes and the doc comment above
PVR_DEVICE_PAGE_SIZE in pvr_mmu.h for the acknowledgement of the page
size restrictions.

The "proper" fix here would be for us to make these two sizes
independent, but that's not a trivial change. The "quick" fix I suppose
would be to depend on one of the supported page sizes, so maybe
(PAGE_SIZE_4KB || PAGE_SIZE_16KB || PAGE_SIZE_64KB || PAGE_SIZE_256KB)
since the larger page sizes appear unsupported (probably for good
reason).

>  	depends on DRM
>  	depends on PM
> +	depends on MMU

Nit: can you keep this alphabetical?

Cheers,
Matt

[1]: https://lore.kernel.org/r/202506191323.zD1fszQb-lkp@intel.com/
[2]: https://lore.kernel.org/r/202506201103.GX6DA9Gx-lkp@intel.com/

>  	select DRM_EXEC
>  	select DRM_GEM_SHMEM_HELPER
>  	select DRM_SCHED
> 


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

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH v6 8/8] drm/imagination: Enable PowerVR driver for RISC-V

[Michal Wilczynski]

On 6/24/25 15:54, Matt Coster wrote:
> On 23/06/2025 12:42, Michal Wilczynski wrote:
>> 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.
>>
>> Add a dependency on MMU to fix a build warning on RISC-V configurations
>> without an MMU.
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/gpu/drm/imagination/Kconfig | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
>> index 5f9fff43d6baadc42ebf48d91729bfbf27e06caa..a7da858a5b301e8f088e3e22f5641feb2e078681 100644
>> --- a/drivers/gpu/drm/imagination/Kconfig
>> +++ b/drivers/gpu/drm/imagination/Kconfig
>> @@ -3,9 +3,10 @@
>>  
>>  config DRM_POWERVR
>>  	tristate "Imagination Technologies PowerVR (Series 6 and later) & IMG Graphics"
>> -	depends on ARM64
>> +	depends on (ARM64 || RISCV)
> 
> There were two issues you encountered when enabling COMPILE_TEST in v5,
> both of which are somewhat simple to workaround but expose underlying
> assumptions we made during early development.
> 
> The first [1] is due to us assuming a 64-bit platform, which was never a
> problem with the ARM64 dependency, but may actually be a problem with
> RISCV given this allows for 32-bit as well. You should probably make
> this (RISCV && 64BIT) until the implicit 64-bit dependency can be worked
> out.

Yeah will incude that in next revision.

> 
> Somewhat related, we also assume a little-endian host. Technically ARM64
> can also be big-endian, you just don't encounter that in the wild too
> often so it's never been a "real" issue. I do wonder if swapping out
> (ARM64 || RISCV) for (64BIT && CPU_LITTLE_ENDIAN) entirely would be a
> reasonable change, perhaps for another day though...
> 
> The other [2] is slightly more subtle. To keep things straightforward,
> we currently map CPU pages to GPU pages 1:1, meaning we use the CPU page
> size to define the GPU page size. That GPU page size is configurable,
> but does not support every possible size the CPU could support on any
> architecture. The failing test there was sparc64 with an 8K page size
> causing no GPU page size to be defined. See the #if/#elif ladder at the
> top of pvr_mmu.c for the supported sizes and the doc comment above
> PVR_DEVICE_PAGE_SIZE in pvr_mmu.h for the acknowledgement of the page
> size restrictions.
> 
> The "proper" fix here would be for us to make these two sizes
> independent, but that's not a trivial change. The "quick" fix I suppose
> would be to depend on one of the supported page sizes, so maybe
> (PAGE_SIZE_4KB || PAGE_SIZE_16KB || PAGE_SIZE_64KB || PAGE_SIZE_256KB)
> since the larger page sizes appear unsupported (probably for good
> reason).

Thanks for a great explanation !

> 
>>  	depends on DRM
>>  	depends on PM
>> +	depends on MMU
> 
> Nit: can you keep this alphabetical?
> 
> Cheers,
> Matt
> 
> [1]: https://lore.kernel.org/r/202506191323.zD1fszQb-lkp@intel.com/
> [2]: https://lore.kernel.org/r/202506201103.GX6DA9Gx-lkp@intel.com/
> 
>>  	select DRM_EXEC
>>  	select DRM_GEM_SHMEM_HELPER
>>  	select DRM_SCHED
>>
> 
> 

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

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

Re: (subset) [PATCH v6 0/8] Add TH1520 GPU support with power sequencing

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Mon, 23 Jun 2025 13:42:38 +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.
> 
> [...]

Applied, thanks!

[1/8] power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver
      https://git.kernel.org/brgl/linux/c/d4c2d9b5b7ceed14a3a835fd969bb0699b9608d3

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

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

[Ulf Hansson]

On Mon, 23 Jun 2025 at 13:44, 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.
>
> The series is structured as follows:
>
> Patch 1: Introduces the pwrseq-thead-gpu auxiliary driver to manage the
>          GPU's power-on/off sequence.
> Patch 2: Adds device tree bindings for the gpu-clkgen reset to the
>          existing thead,th1520-aon binding.
> Patch 3: Extends the pm-domains driver to detect the gpu-clkgen reset
>          and spawn the pwrseq-thead-gpu auxiliary driver.
> Patch 4: Updates the Imagination DRM driver to utilize the pwrseq
>          framework for TH1520 GPU power management.
> Patch 5: Adds the thead,th1520-gpu compatible string to the PowerVR GPU
>          device tree bindings.
> Patch 6: Adds the gpu-clkgen reset property to the aon node in the
>          TH1520 device tree source.
> Patch 7: Adds the device tree node for the IMG BXM-4-64 GPU and its
>          required fixed-clock.
> Patch 8: Enables compilation of the Imagination PowerVR driver on the
>          RISC-V architecture.
>
> This patchset finishes the work started in bigger series [2] by adding
> all 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.

I have applied patch2 and patch3 for next via the pmdomain tree, thanks!

Note that, the DT patch (patch2) is also available on the immutable dt
branch, if it needs to be pulled into some other tree.

[...]

Kind regards
Uffe

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