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

[Steven Price]

On 24/04/2026 12:03, Boris Brezillon wrote:
> On Fri, 24 Apr 2026 11:38:24 +0100
> Steven Price <steven.price@xxxxxxx> wrote:
> 
>> 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@xxxxxxx>  
>>>>
>>>> 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.
> 
> I don't really mind if some intermediate patches use ugly tricks to get
> there,

Sorry, I should have been clearer on that - the intermediate patches can
be a big ugly if necessary (it's clearly better to split things up
rather than change everything in one go). I picked this patch to comment
on because it's the one changing the IRQ handling.

> but in the final state, I'd actually prefer if each block was>
initializing its own iomem, and then the interrupt iomem was calculated
> off this block-iomem region.
> 
> So, in patch, I'd like to see PWR_INT_BASE changed from 0x800 to 0 and
> 
> 	panthor_request_pwr_irq(ptdev, &pwr->irq, irq,
> 				PWR_INTERRUPTS_MASK,
> 				ptdev->pwr->iomem + PWR_INT_BASE);

Yes, using the relevant iomem pointer for each IRQ request also makes
sense. What I disagreed with Sashiko on was picking on just the GPU
instance ;)

Thanks,
Steve