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

[Boris Brezillon <boris.brezillon@collabora.com>]

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.