Re: [PATCH v2 4/8] drm/panthor: Store IRQ register base iomem pointer in panthor_irq

[Steven Price <steven.price@arm.com>]

On 22/04/2026 17:08, Karunika Choo wrote:
> On 22/04/2026 10:34, Steven Price wrote:
>> On 12/04/2026 15:29, Karunika Choo wrote:
>>> Update common IRQ handling code to work from an IRQ-local iomem base
>>> instead of referencing block-specific interrupt register offsets.
>>>
>>> Store the interrupt base address iomem pointer in struct panthor_irq and
>>> switch the shared IRQ helpers to use generic INT_* offsets from that
>>> local base. This removes the need for each caller to expose absolute IRQ
>>> register addresses while keeping the common IRQ flow unchanged.
>>>
>>> No functional change intended.
>>>
>>> v2:
>>> - Change IRQ request function to accept an iomem pointer instead of
>>>   computing it from an offset argument.
>>>
>>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>>
>> One minor comment below...
>>
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_device.h   | 32 ++++++++++++++--------
>>>  drivers/gpu/drm/panthor/panthor_fw.c       |  5 ++--
>>>  drivers/gpu/drm/panthor/panthor_fw_regs.h  |  2 ++
>>>  drivers/gpu/drm/panthor/panthor_gpu.c      |  6 ++--
>>>  drivers/gpu/drm/panthor/panthor_gpu_regs.h |  3 ++
>>>  drivers/gpu/drm/panthor/panthor_mmu.c      |  5 ++--
>>>  drivers/gpu/drm/panthor/panthor_mmu_regs.h |  3 ++
>>>  drivers/gpu/drm/panthor/panthor_pwr.c      |  6 ++--
>>>  8 files changed, 42 insertions(+), 20 deletions(-)
>>>
>> [...]
>>> @@ -1470,7 +1470,8 @@ int panthor_fw_init(struct panthor_device *ptdev)
>>>  	if (irq <= 0)
>>>  		return -ENODEV;
>>>  
>>> -	ret = panthor_request_job_irq(ptdev, &fw->irq, irq, 0);
>>> +	ret = panthor_request_job_irq(ptdev, &fw->irq, irq, 0,
>>> +				      ptdev->iomem + JOB_INT_BASE);
>>>  	if (ret) {
>>>  		drm_err(&ptdev->base, "failed to request job irq");
>>>  		return ret;
>> [..]
>>> @@ -162,7 +162,9 @@ int panthor_gpu_init(struct panthor_device *ptdev)
>>>  	if (irq < 0)
>>>  		return irq;
>>>  
>>> -	ret = panthor_request_gpu_irq(ptdev, &ptdev->gpu->irq, irq, GPU_INTERRUPTS_MASK);
>>> +	ret = panthor_request_gpu_irq(ptdev, &ptdev->gpu->irq, irq,
>>> +				      GPU_INTERRUPTS_MASK,
>>> +				      ptdev->iomem + GPU_INT_BASE);
>>>  	if (ret)
>>>  		return ret;
>>>  
>> [...]
>>> @@ -3229,7 +3229,8 @@ int panthor_mmu_init(struct panthor_device *ptdev)
>>>  		return -ENODEV;
>>>  
>>>  	ret = panthor_request_mmu_irq(ptdev, &mmu->irq, irq,
>>> -				      panthor_mmu_fault_mask(ptdev, ~0));
>>> +				      panthor_mmu_fault_mask(ptdev, ~0),
>>> +				      ptdev->iomem + MMU_INT_BASE);
>>>  	if (ret)
>>>  		return ret;
>>>  
>> [...]
>>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
>>> index aafb0c5c7d23..11c43de1ddd5 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
>>> @@ -70,7 +70,7 @@ static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status)
>>>  	}
>>>  	spin_unlock(&ptdev->pwr->reqs_lock);
>>>  }
>>> -PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler);
>>> +PANTHOR_IRQ_HANDLER(pwr, panthor_pwr_irq_handler);
>>>  
>>>  static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command, u64 args)
>>>  {
>>> @@ -464,7 +464,9 @@ int panthor_pwr_init(struct panthor_device *ptdev)
>>>  	if (irq < 0)
>>>  		return irq;
>>>  
>>> -	err = panthor_request_pwr_irq(ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK);
>>> +	err = panthor_request_pwr_irq(
>>> +		ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK,
>>> +		ptdev->iomem + GPU_CONTROL_BASE + PWR_CONTROL_BASE);
>>
>> This one is the odd one out because it adds GPU_CONTROL_BASE put the
>> other panthor_request_xxx_irq() calls don't. Sashiko also points out
>> that there's an argument these should all be using ptdev->gpu->iomem in
>> the final refactor.
>>
> 
> I understand that it is slightly different, it is only there to
> illustrate the fact that it is a child of the GPU_CONTROL register page.
> w.r.t using ptdev->gpu->iomem, that would mean that we need reach into
> the panthor_gpu component to access the memory, hence why we used a
> separate iomem for this one.

Why do you not add GPU_CONTROL_BASE on when accessing JOB_INT_BASE,
GPU_INT_BASE and MMU_INT_BASE though?

The obvious answer for JOB/MMU is because they're not architecturally in
GPU_CONTROL. But then why don't we have
JOB_CONTROL_BASE/MMU_CONTROL_BASE (my guess is because there isn't
really anything other than INT registers in those blocks).

So it looks like for JOB_INT_BASE/GPU_INT_BASE/MMU_INT_BASE these are
offsets from the beginning of the GPU registers iomem. But then for some
reason PWR_INT_BASE (introduced in patch 6) is treated as if it's a
relative offset within GPU_CONTROL_BASE. Which is just weirdly inconsistent.

I'm happy to disagree with Sashiko on the use of ptdev->gpu->iomem - I
think interrupts are special enough that we can use the top-level iomem.
But that only holds as a justification if all interrupts are treated in
the same way.

Thanks,
Steve

> Kind regards,
> Karunika
> 
> 
>> Thanks,
>> Steve
>>
>>>  	if (err)
>>>  		return err;
>>>  
>>
>