Re: [PATCH] drm/panthor: Wrap register accessor helpers for type safety

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

On Mon, 11 May 2026 15:53:31 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> >   
> > >  {
> > >  	return readl_relaxed(iomem + reg);
> > >  }
> > >  
> > > -static inline void gpu_write64(void __iomem *iomem, u32 reg, u64 data)
> > > +/*
> > > + * The function signature of gpu_read/gpu_write/gpu_read_relaxed/... used to
> > > + * take a &struct panthor_device* as the first parameter. During the split of
> > > + * iomem ranges into individual sub-components, this was changed to take a
> > > + * void __iomem* instead. These wrappers exists Tto avoid situations wherein
> > > + * pre-refactor patches are applied in error, as they'd compile fine. That's
> > > + * because the old calling convention's first parameter implicitly casts to a
> > > + * void pointer.
> > > + */
> > > +
> > > +#define gpu_write(iomem, reg, data) ({					\
> > > +	static_assert(!__same_type((iomem), struct panthor_device *));	\  
> > 
> > Hm, this only covers ptdev being passed as an iomem pointer. I know it's
> > the only case we had so far, but if we're going to add type enforcement,
> > I think I'd prefer if we were covered for more than just ptdev.
> > 
> > One way of doing that would be to wrap the `void __iomem *iomem` in an
> > explicit type like:
> > 
> > struct panthor_reg_bank {
> > 	void __iomem *iomem;
> > };
> > 
> > which then gets passed to gpu_{read,write} helpers (see the diff below).  
> 
> Hm, okay, the diff below is smaller than I feared. Though it doesn't get
> us type checking for someone, say, trying to read GPU_STATUS with the
> iomem of panthor_fw.

Yep, that's annoying, though solving that would require connecting reg
definitions (in panthor_xxx_regs.h) to a specific reg_bank, which is
only doable if we provide per-component accessors like:

#define mmu_reg(_mmu, _name) ((_mmu)->iomem + MMU_ ##)

#define mmu_read(_mmu, _name) gpu_read_iomem(mmu_reg(_mmu, _name))
#define mmu_write(_mmu, _name, _val) \
	gpu_write_iomem(mmu_reg(_mmu, _name),_ val)

> But neither does my proposal below.
> 
> > 
> > The other way would be to pass the component, and have the macro
> > do the <component>->iomem deref, but there's a few places where reg banks
> > are accessed outside of the components that own them (panthor_hw.c).  
> 
> Yeah, I prototyped going down something along that route by having
> the register accessors be generics that are implemented by each
> component, and it's a bit messy. Either you expose the struct
> definitions of individual components so that this header has visibility
> into them (not great), or you add boilerplate "do this accessor
> operation for this component" helpers for every component, which is both
> verbose and possibly causes the inlining to no longer work, though I have
> yet to verify that.
> 
> If we do want to go down this route (though I'm not sure, since your
> reg bank solution seems to get us the same guarantees but without bringing
> generics into this), then the following may be an okay idea:
> 
> I think having just the iomem deref genericised may be a good middle
> ground. If instead of making it a deref, we make it return the pointer
> to the member into the component that it can then deref, then the
> component-specific part can be pure (since offset of the iomem member
> is constant so for a particular pointer to a component, the pointer to
> the iomem member only depends on the passed-in pointer to component.)
> 
> This should make sure that when the compiler gets
> 
> panthor_gpu_write(ptdev->gpu, foo, bar);
> val = panthor_gpu_read(ptdev->gpu, baz);
> 
> it can optimise the expanded
> 
> iomem = *panthor_get_iomem_ptr(ptdev->gpu);
> panthor_actual_write(iomem, foo, bar);
> iomem = *panthor_get_iomem_ptr(ptdev->gpu);
> val = panthor_actual_read(iomem, baz);
> 
> to the simplified
> 
> iomem = *panthor_get_iomem_ptr(ptdev->gpu);
> panthor_actual_write(iomem, foo, bar);
> val = panthor_actual_read(iomem, baz);
> 
> because panthor_get_iomem_ptr will be known to return the same value
> when called with the same input param.

Right, as long as the compiler sees the definition of
panthor_get_<component>_iomem_ptr() (which should be the case any time a
read/write happens inside the panthor_<component>.c compilation unit),
it hopefully inlines the whole thing and you get the iomem pointer from
a direct deref rather than a function call. LTO might even give us link
time optim for the bits in panthor_hw.c where the compiler can't see
through struct panthor_{gpu,pwr}.

This being said, there's still no guarantee that one would mix regs and
banks randomly, like

	gpu_read(ptdev->mmu, GPU_ID);

> 
> Anway, I think it's probably best if I abandon this and you just send
> your patch to the list with a real base. I only have one comment on it,
> which I've included inline.

Let's wait for Liviu's and Steve's feedback before taking any action,
cause that's still quite a lot of changes, and it's not clear it will
help much once we've got all the pending patchset rebased on misc-next
(that's a mistake you do once at rebase time, once you've got bitten,
you tend to be more careful ;-)).