diff mbox series

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

Message ID 20250530-apr_14_for_sending-v3-3-83d5744d997c@samsung.com
State New
Headers show
Series Add TH1520 GPU support with power sequencing | expand

Commit Message

Michal Wilczynski May 29, 2025, 10:23 p.m. UTC 
Update the Imagination PVR DRM driver to leverage the pwrseq framework
for managing the power sequence of the GPU on the T-HEAD TH1520 SoC.

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

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

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

Comments

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

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

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

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

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

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

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

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

> 
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..737ace77c4f1247c687cc1fde2f139fc2e118c50 100644
--- a/drivers/gpu/drm/imagination/Kconfig
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -11,6 +11,7 @@  config DRM_POWERVR
 	select DRM_SCHED
 	select DRM_GPUVM
 	select FW_LOADER
+  select POWER_SEQUENCING
 	help
 	  Choose this option if you have a system that has an Imagination
 	  Technologies PowerVR (Series 6 or later) or IMG GPU.
diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..19d48bbc828cf2b8dbead602e90ff88780152124 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -25,6 +25,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pwrseq/consumer.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/stddef.h>
@@ -631,10 +632,34 @@  pvr_device_init(struct pvr_device *pvr_dev)
 	if (err)
 		return err;
 
-	/* Get the reset line for the GPU */
-	err = pvr_device_reset_init(pvr_dev);
-	if (err)
-		return err;
+	/*
+	 * Try to get a power sequencer. If successful, it will handle clocks
+	 * and resets. Otherwise, we fall back to managing them ourselves.
+	 */
+	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
+	if (IS_ERR(pvr_dev->pwrseq)) {
+		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
+
+		/*
+		 * If the error is -EPROBE_DEFER, it's because the
+		 * optional sequencer provider is not present
+		 * and it's safe to fall back on manual power-up.
+		 */
+		if (pwrseq_err == -EPROBE_DEFER)
+			pvr_dev->pwrseq = NULL;
+		else
+			return dev_err_probe(dev, pwrseq_err,
+					     "Failed to get power sequencer\n");
+	}
+
+	/* Get the reset line for the GPU, but since it's exclusive only
+	 * get it if the pwerseq is NULL.
+	 */
+	if (!pvr_dev->pwrseq) {
+		err = pvr_device_reset_init(pvr_dev);
+		if (err)
+			return err;
+	}
 
 	/* Explicitly power the GPU so we can access control registers before the FW is booted. */
 	err = pm_runtime_resume_and_get(dev);
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index 7cb01c38d2a9c3fc71effe789d4dfe54eddd93ee..0b6d53994d25abe32a2f1b133a4efa574e150da9 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -37,6 +37,9 @@  struct clk;
 /* Forward declaration from <linux/firmware.h>. */
 struct firmware;
 
+/* Forward declaration from <linux/pwrseq/consumer.h */
+struct pwrseq_desc;
+
 /**
  * struct pvr_gpu_id - Hardware GPU ID information for a PowerVR device
  * @b: Branch ID.
@@ -148,6 +151,9 @@  struct pvr_device {
 	 */
 	struct reset_control *reset;
 
+	/** @pwrseq: Pointer to a power sequencer, if one is used. */
+	struct pwrseq_desc *pwrseq;
+
 	/** @irq: IRQ number. */
 	int irq;
 
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index 41f5d89e78b854cf6993838868a4416a220b490a..b7dcc03d7d9e06f03f059124fbe21dd844de713e 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -21,6 +21,7 @@ 
 #include <linux/reset.h>
 #include <linux/timer.h>
 #include <linux/types.h>
+#include <linux/pwrseq/consumer.h>
 #include <linux/workqueue.h>
 
 #define POWER_SYNC_TIMEOUT_US (1000000) /* 1s */
@@ -234,6 +235,19 @@  pvr_watchdog_init(struct pvr_device *pvr_dev)
 	return 0;
 }
 
+static int pvr_power_off_sequence_manual(struct pvr_device *pvr_dev)
+{
+	int err;
+
+	err = reset_control_assert(pvr_dev->reset);
+
+	clk_disable_unprepare(pvr_dev->mem_clk);
+	clk_disable_unprepare(pvr_dev->sys_clk);
+	clk_disable_unprepare(pvr_dev->core_clk);
+
+	return err;
+}
+
 int
 pvr_power_device_suspend(struct device *dev)
 {
@@ -252,11 +266,10 @@  pvr_power_device_suspend(struct device *dev)
 			goto err_drm_dev_exit;
 	}
 
-	clk_disable_unprepare(pvr_dev->mem_clk);
-	clk_disable_unprepare(pvr_dev->sys_clk);
-	clk_disable_unprepare(pvr_dev->core_clk);
-
-	err = reset_control_assert(pvr_dev->reset);
+	if (pvr_dev->pwrseq)
+		err = pwrseq_power_off(pvr_dev->pwrseq);
+	else
+		err = pvr_power_off_sequence_manual(pvr_dev);
 
 err_drm_dev_exit:
 	drm_dev_exit(idx);
@@ -276,44 +289,55 @@  pvr_power_device_resume(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
-	err = clk_prepare_enable(pvr_dev->core_clk);
-	if (err)
-		goto err_drm_dev_exit;
+	if (pvr_dev->pwrseq) {
+		err = pwrseq_power_on(pvr_dev->pwrseq);
+		if (err)
+			goto err_drm_dev_exit;
+	} else {
+		err = clk_prepare_enable(pvr_dev->core_clk);
+		if (err)
+			goto err_drm_dev_exit;
 
-	err = clk_prepare_enable(pvr_dev->sys_clk);
-	if (err)
-		goto err_core_clk_disable;
+		err = clk_prepare_enable(pvr_dev->sys_clk);
+		if (err)
+			goto err_core_clk_disable;
 
-	err = clk_prepare_enable(pvr_dev->mem_clk);
-	if (err)
-		goto err_sys_clk_disable;
+		err = clk_prepare_enable(pvr_dev->mem_clk);
+		if (err)
+			goto err_sys_clk_disable;
 
-	/*
-	 * According to the hardware manual, a delay of at least 32 clock
-	 * cycles is required between de-asserting the clkgen reset and
-	 * de-asserting the GPU reset. Assuming a worst-case scenario with
-	 * a very high GPU clock frequency, a delay of 1 microsecond is
-	 * sufficient to ensure this requirement is met across all
-	 * feasible GPU clock speeds.
-	 */
-	udelay(1);
+		/*
+		 * According to the hardware manual, a delay of at least 32 clock
+		 * cycles is required between de-asserting the clkgen reset and
+		 * de-asserting the GPU reset. Assuming a worst-case scenario with
+		 * a very high GPU clock frequency, a delay of 1 microsecond is
+		 * sufficient to ensure this requirement is met across all
+		 * feasible GPU clock speeds.
+		 */
+		udelay(1);
 
-	err = reset_control_deassert(pvr_dev->reset);
-	if (err)
-		goto err_mem_clk_disable;
+		err = reset_control_deassert(pvr_dev->reset);
+		if (err)
+			goto err_mem_clk_disable;
+	}
 
 	if (pvr_dev->fw_dev.booted) {
 		err = pvr_power_fw_enable(pvr_dev);
 		if (err)
-			goto err_reset_assert;
+			goto err_power_off;
 	}
 
 	drm_dev_exit(idx);
 
 	return 0;
 
-err_reset_assert:
-	reset_control_assert(pvr_dev->reset);
+err_power_off:
+	if (pvr_dev->pwrseq)
+		pwrseq_power_off(pvr_dev->pwrseq);
+	else
+		pvr_power_off_sequence_manual(pvr_dev);
+
+	goto err_drm_dev_exit;
 
 err_mem_clk_disable:
 	clk_disable_unprepare(pvr_dev->mem_clk);