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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux