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

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

On 24/04/2026 13:09, Boris Brezillon wrote:
> On Fri, 24 Apr 2026 12:20:07 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 24/04/2026 12:03, Boris Brezillon wrote:
>>> On Fri, 24 Apr 2026 11:38:24 +0100
>>> Steven Price <steven.price@arm.com> 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@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.  
>>>
>>> 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 ;)
> 
> No worries. That was more a comment addressed to Karunika actually,
> since I see:
> 
>  	err = panthor_request_pwr_irq(
>  		ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK,
> 		ptdev->iomem + GPU_CONTROL_BASE + PWR_INT_BASE);
> 
> in patch 6, which is not what I'd expect there.

Hello, thanks for the suggestions. I have tried addressing these in v3.

https://lore.kernel.org/all/20260427155934.416502-1-karunika.choo@arm.com/

Kind regards,
Karunika