Re: [RFC PATCH 13/20] powerpc/io: Remove unnecessary indirection

["Arnd Bergmann" <arnd@arndb.de>]

On Thu, Nov 14, 2024, at 13:51, Michael Ellerman wrote:
> Some of the __do_xxx() defines do nothing useful, they just existed to
> make the previous hooking macros work. So remove them.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> @@ -607,27 +600,27 @@ static inline u32 readl_be(const PCI_IO_ADDR addr)
> 
>  static inline void writeb(u8 val, PCI_IO_ADDR addr)
>  {
> -	__do_writeb(val, addr);
> +	out_8(addr, val);
>  }

I would open-code PCI_IO_ADDR here.

Also, at this point the writeb() etc functions are close enough
to the asm-generic/io.h version that we could try to go the next
few steps. One problem doing this is the definition of the
*_relaxed() accessors. Ideally the inline asm would go
into the __raw_*() helpers, with the "sync" and "twi;isync"
going into __io_br(), __io_bw() and __io_aw(), at which point
these can all use the generic versions, and out_*/in_*() can
be defined on top of those.

What I'm not sure about here is the __io_br() (sync before
readl()), since most other architectures don't do this.
Any idea why powerpc does it, and if the relaxed() version
needs it as well? Is this for spinlocks serialization?

I think the sync before write and twi;isync after read() is
needed mainly for synchronizing against concurrent DMA, so
it should be fine to leave those out of the relaxed
versions, but it's possible that there is something
more going on than I remember.

      Arnd