Re: [PATCH v1 07/10] drm/panthor: Implement soft and fast reset via PWR_CONTROL

[Karunika Choo <karunika.choo@arm.com>]

On 20/10/2025 12:24, Steven Price wrote:
> On 14/10/2025 10:43, Karunika Choo wrote:
>> Add helpers to issue reset commands through the PWR_CONTROL interface
>> and wait for reset completion using IRQ signaling. This enables support
>> for both RESET_SOFT and RESET_FAST operations with timeout handling and
>> status verification.
>>
>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>> ---
>>  drivers/gpu/drm/panthor/panthor_pwr.c | 62 ++++++++++++++++++++++++++-
>>  drivers/gpu/drm/panthor/panthor_pwr.h |  2 +
>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
>> index 594181aba847..ecb278824d06 100644
>> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
>> @@ -3,6 +3,7 @@
>>  
>>  #include <linux/platform_device.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/cleanup.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/wait.h>
>>  
>> @@ -31,6 +32,8 @@
>>  
>>  #define PWR_RETRACT_TIMEOUT_US		2000
>>  
>> +#define PWR_RESET_TIMEOUT_MS		500
>> +
>>  /**
>>   * struct panthor_pwr - PWR_CONTROL block management data.
>>   */
>> @@ -80,6 +83,42 @@ static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command,
>>  	gpu_write(ptdev, PWR_COMMAND, command);
>>  }
>>  
>> +static bool reset_irq_raised(struct panthor_device *ptdev)
>> +{
>> +	return gpu_read(ptdev, PWR_INT_RAWSTAT) & PWR_IRQ_RESET_COMPLETED;
>> +}
>> +
>> +static bool reset_completed(struct panthor_device *ptdev)
>> +{
>> +	return (ptdev->pwr->pending_reqs & PWR_IRQ_RESET_COMPLETED);
>> +}
>> +
>> +static int panthor_pwr_reset(struct panthor_device *ptdev, u32 reset_cmd)
>> +{
>> +	scoped_guard(spinlock_irqsave, &ptdev->pwr->reqs_lock) {
>> +		if (!drm_WARN_ON(&ptdev->base, !reset_completed(ptdev))) {
>> +			ptdev->pwr->pending_reqs |= PWR_IRQ_RESET_COMPLETED;
>> +			gpu_write(ptdev, PWR_INT_CLEAR, PWR_IRQ_RESET_COMPLETED);
>> +			panthor_pwr_write_command(ptdev, reset_cmd, 0);
>> +		}
> 
> This would be easier to read as:
> 
> if (reset_completed(ptdev)) {
> 	....
> } else {
> 	drm_WARN(&ptdev->base, 1, "Hey, we're already resetting?");
> }
> 
> [Message modified to taste ;) ]
> 
> I'm also wondering if things would be easier to read if you switched
> from reset_completed() to reset_pending(). Certainly here it's the
> 'pending' test you are trying to do.
> 

Oops. I might have made a mistake with the logic. Let me fix this in v2.
Thanks for spotting it

>> +	}
>> +
>> +	if (!wait_event_timeout(ptdev->pwr->reqs_acked, reset_completed(ptdev),
>> +				msecs_to_jiffies(PWR_RESET_TIMEOUT_MS))) {
>> +		guard(spinlock_irqsave)(&ptdev->pwr->reqs_lock);
>> +
>> +		if (!reset_completed(ptdev) && !reset_irq_raised(ptdev)) {
>> +			drm_err(&ptdev->base, "RESET_%s timed out",
>> +				reset_cmd == PWR_COMMAND_RESET_SOFT ? "SOFT" : "FAST");
>> +			return -ETIMEDOUT;
>> +		}
>> +
>> +		ptdev->pwr->pending_reqs &= ~PWR_IRQ_RESET_COMPLETED;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static const char *get_domain_name(u8 domain)
>>  {
>>  	switch (domain) {
>> @@ -407,9 +446,30 @@ int panthor_pwr_init(struct panthor_device *ptdev)
>>  	return 0;
>>  }
>>  
>> +int panthor_pwr_reset_fast(struct panthor_device *ptdev)
>> +{
>> +	if (!ptdev->pwr)
>> +		return 0;
>> +
>> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_FAST_RESET)) {
>> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");
> 
> Copy/paste mistake on the error message.
> 
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_FAST);
>> +}
> 
> I can't actually find a caller of this function within the series.
> 

I will remove the fast reset option entirely in v2 as there is currently
no use for this function. We can reimplement this in the future, should
it be something that is desired.

>> +
>>  int panthor_pwr_reset_soft(struct panthor_device *ptdev)
>>  {
>> -	return 0;
>> +	if (!ptdev->pwr)
>> +		return 0;
> 
> When would this happen? Is this not a programming error?
> 

I will remove this. Thanks.

Kind regards,
Karunika Choo

> Thanks,
> Steve
> 
>> +
>> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_SOFT_RESET)) {
>> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_SOFT);
>>  }
>>  
>>  int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
>> index a4042c125448..2301c26dab86 100644
>> --- a/drivers/gpu/drm/panthor/panthor_pwr.h
>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.h
>> @@ -10,6 +10,8 @@ void panthor_pwr_unplug(struct panthor_device *ptdev);
>>  
>>  int panthor_pwr_init(struct panthor_device *ptdev);
>>  
>> +int panthor_pwr_reset_fast(struct panthor_device *ptdev);
>> +
>>  int panthor_pwr_reset_soft(struct panthor_device *ptdev);
>>  
>>  int panthor_pwr_l2_power_on(struct panthor_device *ptdev);
>