Re: [RFT PATCH v3 08/27] asm-generic/io.h: Add a non-posted variant of ioremap()

[Hector Martin]

On 05/03/2021 23.45, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 11:40 PM Hector Martin <marcan@xxxxxxxxx> wrote:
> > ARM64 currently defaults to posted MMIO (nGnRnE), but some devices
> > require the use of non-posted MMIO (nGnRE). Introduce a new ioremap()
> > variant to handle this case. ioremap_np() is aliased to ioremap() by
> > default on arches that do not implement this variant.
>
> Hmm... But isn't it basically a requirement to those device drivers to
> use readX()/writeX() instead of readX_relaxed() / writeX_relaxed()?

No, the write ops used do not matter. It's just that on these Apple SoCs 
the fabric requires the mappings to be nGnRnE, else it just throws 
SErrors on all writes and ignores them.

The difference between _relaxed and not is barrier behavior with regards 
to DMA/memory accesses; this applies regardless of whether the writes 
are E or nE. You can have relaxed accesses with nGnRnE and then you 
would still have race conditions if you do not have a barrier between 
the MMIO and accessing DMA memory. What nGnRnE buys you (on 
platforms/buses where it works properly) is that you do need a dummy 
read after a write to ensure completion.

All of this is to some extent moot on these SoCs; it's not that we need 
the drivers to use nGnRnE for some correctness reason, it's that the 
SoCs force us to use it or else everything breaks, which was the 
motivation for this change. But since on most other SoCs both are valid 
options, this does allow some other drivers/platforms to opt into nGnRnE 
if they have a good reason to do so.

Though you just made me notice two mistakes in the commit description: 
first, it describes the old v2 version, for v3 I made ioremap_np() just 
return NULL on arches that don't implement it. Second, nGnRnE and nGnRE 
are backwards. Oops. I'll fix it for the next version.

> >   #define IORESOURCE_MEM_32BIT           (3<<3)
> >   #define IORESOURCE_MEM_SHADOWABLE      (1<<5)  /* dup: IORESOURCE_SHADOWABLE */
> >   #define IORESOURCE_MEM_EXPANSIONROM    (1<<6)
> > +#define IORESOURCE_MEM_NONPOSTED       (1<<7)
>
> Not sure it's the right location (in a bit field) for this flag.

Do you have a better suggestion? It seemed logical to put it here, as a 
flag on memory-type I/O resources.

--
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub