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 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, 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);