ARM: implement workaround for Cortex-A9/PL310/PCIe deadlock

arch/arm/mm/ioremap.c int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr) · line 458

Quoted 9 lines · On Thursday 15 May 2014 11:18:37 Thomas Petazzoni wrote:

arch/arm/mm/ioremap.c +1 -1

@@ -445,7 +452,7 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
        return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
                                  PCI_IO_VIRT_BASE + offset + SZ_64K,
                                  phys_addr,
-                                 __pgprot(get_mem_type(MT_DEVICE)->prot_pte));
+                                 __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
 

As discussed on IRC, I think we'd be better off making this a strong-ordered mapping for all platforms unconditionally. The PCI I/O space semantics require non-posted writes, which is the main difference between device and SO mappings, so the same fix is required both for mvebu as a workaround for the deadlock as well as for everyone else as a fix for an incorrect PCI behavior.

Quoted 17 lines · Arnd Bergmann

arch/arm/mm/ioremap.c int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr) · line 458

Quoted 9 lines · On Thursday 15 May 2014 11:18:37 Thomas Petazzoni wrote:

arch/arm/mm/ioremap.c +1 -1

@@ -445,7 +452,7 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
        return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
                                  PCI_IO_VIRT_BASE + offset + SZ_64K,
                                  phys_addr,
-                                 __pgprot(get_mem_type(MT_DEVICE)->prot_pte));
+                                 __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
 

As discussed on IRC, I think we'd be better off making this a strong-ordered mapping for all platforms unconditionally. The PCI I/O space semantics require non-posted writes, which is the main difference between device and SO mappings, so the same fix is required both for mvebu as a workaround for the deadlock as well as for everyone else as a fix for an incorrect PCI behavior.

Ok, I'll take that into account when posting a v4.

Quoted 19 lines · Thomas Petazzoni

Quoted 17 lines · Arnd Bergmann

arch/arm/mm/ioremap.c int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr) · line 458

Quoted 9 lines · On Thursday 15 May 2014 11:18:37 Thomas Petazzoni wrote:

arch/arm/mm/ioremap.c +1 -1

@@ -445,7 +452,7 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
        return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
                                  PCI_IO_VIRT_BASE + offset + SZ_64K,
                                  phys_addr,
-                                 __pgprot(get_mem_type(MT_DEVICE)->prot_pte));
+                                 __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
 

As discussed on IRC, I think we'd be better off making this a strong-ordered mapping for all platforms unconditionally. The PCI I/O space semantics require non-posted writes, which is the main difference between device and SO mappings, so the same fix is required both for mvebu as a workaround for the deadlock as well as for everyone else as a fix for an incorrect PCI behavior.

Quoted 1 line

Ok, I'll take that into account when posting a v4.

Actually, I don't think this reasoning is correct. The memory type here applies to accesses mastered by the CPU onto AXI -- it is the job of the AXI/PCI bridge (i.e. the host controller) to ensure that writes are not posted, so this is irrelevant.

Of course, the erratum in question requires SO memory, but that's an entirely different problem. The default should remain as device memory.

Quoted 16 lines · On Thursday 15 May 2014 15:29:24 Will Deacon wrote:

Quoted 9 lines · Thomas Petazzoni

Quoted 6 lines · Arnd Bergmann

As discussed on IRC, I think we'd be better off making this a strong-ordered mapping for all platforms unconditionally. The PCI I/O space semantics require non-posted writes, which is the main difference between device and SO mappings, so the same fix is required both for mvebu as a workaround for the deadlock as well as for everyone else as a fix for an incorrect PCI behavior.

Ok, I'll take that into account when posting a v4.

Actually, I don't think this reasoning is correct. The memory type here applies to accesses mastered by the CPU onto AXI -- it is the job of the AXI/PCI bridge (i.e. the host controller) to ensure that writes are not posted, so this is irrelevant.

Quoted 2 lines

Of course, the erratum in question requires SO memory, but that's an entirely different problem. The default should remain as device memory.

How can a write be non-posted on the PCI bus if it's posted on AXI?

From the point-of-view of the CPU it would be posted, but the PCI bus would

The way I understand it, the CPU would continue with the next instruction as soon as the write has made it out to the AXI fabric, i.e. before the PIO instruction is complete.

If this is used to synchronize with a DMA, there is no guarantee that the transaction from PCI will be visible in memory by then.

Quoted 3 lines

The way I understand it, the CPU would continue with the next instruction as soon as the write has made it out to the AXI fabric, i.e. before the PIO instruction is complete.

The CPU can continue regardless -- you'd need a DSB if you want to hold up the instruction stream based on completion of a memory access. With the posted write (device type), the write may complete as soon as it reaches an ordered bus.

Note that nGnRnE accesses in AArch64 (the equivalent to strongly-ordered) *can* still get an early write response -- that is simply a hint to the memory subsystem.

Quoted 2 lines

If this is used to synchronize with a DMA, there is no guarantee that the transaction from PCI will be visible in memory by then.

Can you elaborate on this scenario please? When would we use an I/O space write to synchronise with a DMA transfer from a PCI endpoint? You're definitely referring to I/O space as opposed to Configuration Space, right?

Quoted 13 lines · On Thursday 15 May 2014 16:34:30 Will Deacon wrote:

Quoted 3 lines

The way I understand it, the CPU would continue with the next instruction as soon as the write has made it out to the AXI fabric, i.e. before the PIO instruction is complete.

The CPU can continue regardless -- you'd need a DSB if you want to hold up the instruction stream based on completion of a memory access. With the posted write (device type), the write may complete as soon as it reaches an ordered bus.

Note that nGnRnE accesses in AArch64 (the equivalent to strongly-ordered) *can* still get an early write response -- that is simply a hint to the memory subsystem.

Quoted 6 lines

Quoted 2 lines

If this is used to synchronize with a DMA, there is no guarantee that the transaction from PCI will be visible in memory by then.

Can you elaborate on this scenario please? When would we use an I/O space write to synchronise with a DMA transfer from a PCI endpoint? You're definitely referring to I/O space as opposed to Configuration Space, right?

Correct. Assume a PCI device uses PIO and DMA. It sends a DMA to main memory and lets the CPU know about the data using a level (IntA as opposed to MSI) interrupt. The CPU performs an outl() operation to an I/O port to let the hardware know it has received the IRQ and the response of the outl() is guaranteed to flush the DMA transaction: by the time the outl() completes we know that the data in memory is valid because it is strongly ordered relative to the DMA.

outl() actually does a dsb() internally, but unfortunately that is before the store, not after, so I assume that a driver relying on the behavior above would still be racy.

Quoted 8 lines · Arnd Bergmann

Quoted 6 lines · On Thursday 15 May 2014 16:34:30 Will Deacon wrote:

Quoted 2 lines

If this is used to synchronize with a DMA, there is no guarantee that the transaction from PCI will be visible in memory by then.

Can you elaborate on this scenario please? When would we use an I/O space write to synchronise with a DMA transfer from a PCI endpoint? You're definitely referring to I/O space as opposed to Configuration Space, right?

Quoted 7 lines

Correct. Assume a PCI device uses PIO and DMA. It sends a DMA to main memory and lets the CPU know about the data using a level (IntA as opposed to MSI) interrupt. The CPU performs an outl() operation to an I/O port to let the hardware know it has received the IRQ and the response of the outl() is guaranteed to flush the DMA transaction: by the time the outl() completes we know that the data in memory is valid because it is strongly ordered relative to the DMA.

Hmm, when you say `guaranteed to flush the DMA transaction', is that a PCI requirement? If so, whether or not that DMA data is then visible to the CPU is really specific to the host-controller implementation. It could easily be buffered somewhere between the host controller and memory, for example.

Quoted 3 lines

outl() actually does a dsb() internally, but unfortunately that is before the store, not after, so I assume that a driver relying on the behavior above would still be racy.

Yup, we'd need an additional dsb. I think we're confusing what the PCI specification says about ordering and what the inb/outb instructions provide on x86. It may well be that we want to emulate the x86 behaviour on ARM, but that's not going to come cheap and I don't think it's a decision we should take lightly.

Quoted 17 lines · On Friday 16 May 2014 10:53:33 Will Deacon wrote:

Quoted 15 lines · Arnd Bergmann

Quoted 6 lines · On Thursday 15 May 2014 16:34:30 Will Deacon wrote:

Quoted 2 lines

If this is used to synchronize with a DMA, there is no guarantee that the transaction from PCI will be visible in memory by then.

Can you elaborate on this scenario please? When would we use an I/O space write to synchronise with a DMA transfer from a PCI endpoint? You're definitely referring to I/O space as opposed to Configuration Space, right?

Correct. Assume a PCI device uses PIO and DMA. It sends a DMA to main memory and lets the CPU know about the data using a level (IntA as opposed to MSI) interrupt. The CPU performs an outl() operation to an I/O port to let the hardware know it has received the IRQ and the response of the outl() is guaranteed to flush the DMA transaction: by the time the outl() completes we know that the data in memory is valid because it is strongly ordered relative to the DMA.

Quoted 4 lines

Hmm, when you say `guaranteed to flush the DMA transaction', is that a PCI requirement? If so, whether or not that DMA data is then visible to the CPU is really specific to the host-controller implementation. It could easily be buffered somewhere between the host controller and memory, for example.

It's something that drivers are supposed to rely on, and the PCI host already has to make the same guarantee about ordering of MSI, MMIO-read and DMA transactions, all of which we definitely rely on in the kernel.

Quoted 9 lines

Quoted 3 lines

outl() actually does a dsb() internally, but unfortunately that is before the store, not after, so I assume that a driver relying on the behavior above would still be racy.

Yup, we'd need an additional dsb. I think we're confusing what the PCI specification says about ordering and what the inb/outb instructions provide on x86. It may well be that we want to emulate the x86 behaviour on ARM, but that's not going to come cheap and I don't think it's a decision we should take lightly.

outb() is expected to be an extremely heavyweight operation, that's why nobody uses it. Why would you care about whether it's one or two microsecond latency on a MIDI port or an ISDN adapter?

We can decide that we don't care about correctness here, but the performance argument doesn't seem overly important.

Quoted 23 lines · Arnd Bergmann

Quoted 21 lines · On Friday 16 May 2014 10:53:33 Will Deacon wrote:

Quoted 8 lines · Arnd Bergmann

Quoted 6 lines · On Thursday 15 May 2014 16:34:30 Will Deacon wrote:

Quoted 2 lines

If this is used to synchronize with a DMA, there is no guarantee that the transaction from PCI will be visible in memory by then.

Can you elaborate on this scenario please? When would we use an I/O space write to synchronise with a DMA transfer from a PCI endpoint? You're definitely referring to I/O space as opposed to Configuration Space, right?

Quoted 7 lines

Correct. Assume a PCI device uses PIO and DMA. It sends a DMA to main memory and lets the CPU know about the data using a level (IntA as opposed to MSI) interrupt. The CPU performs an outl() operation to an I/O port to let the hardware know it has received the IRQ and the response of the outl() is guaranteed to flush the DMA transaction: by the time the outl() completes we know that the data in memory is valid because it is strongly ordered relative to the DMA.

Hmm, when you say `guaranteed to flush the DMA transaction', is that a PCI requirement? If so, whether or not that DMA data is then visible to the CPU is really specific to the host-controller implementation. It could easily be buffered somewhere between the host controller and memory, for example.

Quoted 3 lines

It's something that drivers are supposed to rely on, and the PCI host already has to make the same guarantee about ordering of MSI, MMIO-read and DMA transactions, all of which we definitely rely on in the kernel.

Supposed to rely on for x86, sure. I don't think we can even give you this guarantee for arm64, where nE is nothing more than a *hint* to the memory subsystem.

For the MSI case, I thought we had to go and poke the SCU for the Marvell SoC?

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

outl() actually does a dsb() internally, but unfortunately that is before the store, not after, so I assume that a driver relying on the behavior above would still be racy.

Yup, we'd need an additional dsb. I think we're confusing what the PCI specification says about ordering and what the inb/outb instructions provide on x86. It may well be that we want to emulate the x86 behaviour on ARM, but that's not going to come cheap and I don't think it's a decision we should take lightly.

Quoted 3 lines

outb() is expected to be an extremely heavyweight operation, that's why nobody uses it. Why would you care about whether it's one or two microsecond latency on a MIDI port or an ISDN adapter?

Fair enough -- I just don't think we should dress up an erratum workaround as a bug fix, especially when it's adding a new user of strongly-ordered memory to the kernel which we can't honour for arm64.

Quoted 3 lines · On Monday 19 May 2014 15:23:55 Will Deacon wrote:

Fair enough -- I just don't think we should dress up an erratum workaround as a bug fix, especially when it's adding a new user of strongly-ordered memory to the kernel which we can't honour for arm64.

Makes sense. How about a patch then that just changes the memory type for the I/O space and adds a comment explaining all we found out? From what I can tell, strict ordering is required for Armada 3xx, may or may not be required elsewhere but should never hurt noticeably. We may want to restrict it to ARMv6/v7 so we don't have to pick the right domain.

Quoted 5 lines · Arnd Bergmann

Quoted 3 lines · On Monday 19 May 2014 15:23:55 Will Deacon wrote:

Fair enough -- I just don't think we should dress up an erratum workaround as a bug fix, especially when it's adding a new user of strongly-ordered memory to the kernel which we can't honour for arm64.

Quoted 5 lines

Makes sense. How about a patch then that just changes the memory type for the I/O space and adds a comment explaining all we found out? From what I can tell, strict ordering is required for Armada 3xx, may or may not be required elsewhere but should never hurt noticeably. We may want to restrict it to ARMv6/v7 so we don't have to pick the right domain.

Ok, and we don't bother with the extra dsb in outb? I'm alright with that if the comment goes into enough detail.

Quoted 7 lines

Correct. Assume a PCI device uses PIO and DMA. It sends a DMA to main memory and lets the CPU know about the data using a level (IntA as opposed to MSI) interrupt. The CPU performs an outl() operation to an I/O port to let the hardware know it has received the IRQ and the response of the outl() is guaranteed to flush the DMA transaction: by the time the outl() completes we know that the data in memory is valid because it is strongly ordered relative to the DMA.

Hmm, when you say `guaranteed to flush the DMA transaction', is that a PCI requirement? If so, whether or not that DMA data is then visible to the CPU is really specific to the host-controller implementation. It could easily be buffered somewhere between the host controller and memory, for example.

Quoted 7 lines · Will Deacon

Quoted 7 lines

Correct. Assume a PCI device uses PIO and DMA. It sends a DMA to main memory and lets the CPU know about the data using a level (IntA as opposed to MSI) interrupt. The CPU performs an outl() operation to an I/O port to let the hardware know it has received the IRQ and the response of the outl() is guaranteed to flush the DMA transaction: by the time the outl() completes we know that the data in memory is valid because it is strongly ordered relative to the DMA.

Keep in mind that the IntA message itself is going to flush the DMA, no sane host bridge implementation should process the IntA until all prior DMA writes are completed, just like MSI.

Also, legacy non-MSI interrupts are always sharable, so the ISR must always start with a read of a device specific status reguster, which will also flush any DMA writes.

The simplest common scenario to show synchronous outl is this:

void pci_isr()
{
if (inl(status_reg) & INT_PENDING)

      outl(ACK_INT,status_reg);
}

Where the outl is not expected to complete at the CPU until the device has lowered the level triggered interrupt line.

If outl is not synchronous then a spurious interrupt will be caused.

When converting a driver to MMIO you'd often have to do this:

void pci_isr()

{
   if (readl(status_reg) & INT_PENDING) {
      writel(ACK_INT,status_reg);
      readl(status_reg); // Synchronizing read, flushes write.
   }
}

Which is one of the software visible impacts of io vs mmio.

Quoted 4 lines

Hmm, when you say `guaranteed to flush the DMA transaction', is that a PCI requirement? If so, whether or not that DMA data is then visible to the CPU is really specific to the host-controller implementation. It could easily be buffered somewhere between the host controller and memory, for example.

PCI has the producer/consumer ordering model as part of the driving concept in the spec. Basically it wants to see the ordering model preserved right to the driver code itself.

Realistically, way back, archs that couldn't do the synchronous IO (like my old MIPS design) had to convert their drivers to MMIO and run that way. It never worked 100% properly, or made sense to try an use an async outl, even though some systems provided it

Quoted 1 line

How can a write be non-posted on the PCI bus if it's posted on AXI?

From the point-of-view of the CPU it would be posted, but the PCI bus would see an unposted write (so I imagine there would be write buffering at the host controller). However, I worry that I'm missing your point :)

Quoted 5 lines · Will Deacon

Quoted 1 line

How can a write be non-posted on the PCI bus if it's posted on AXI?

From the point-of-view of the CPU it would be posted, but the PCI bus would see an unposted write (so I imagine there would be write buffering at the host controller). However, I worry that I'm missing your point :)

It is worth being a bit careful with language here, from an AXI perspective there is not really such thing as a posted write.

All writes are explicitly ack'd upon 'completion', however the memory type influences when that is allowed to happen.

For PCI IO writes the AXI memory type from the CPU must be 'Device Non-bufferable' (AWCACHE = 0), which will require the AXI ACK to be generated only once the PCI target returns an IOWr completion TLP.

For PCI Memory writes the AXI memory type from the CPU could be 'Device Non-bufferable' but it would be best if it is 'Device Bufferable' (AWCACHE = 1).

The latter allows more performance by permitting any AXI bridge in the path to ack the write early. This is as close as AXI gets to 'posted writes'

It is very important that the page tables in the CPU properly select the right AXI Memory Type for each space.

AFAIK, to duplicate x86 semantics an outl/inl must spin the CPU until it completes at the target, and the CPU must not pipeline outl/inl
operations: outl(); outl(); produces 1 IOWr TLP, waits for completion, then produces another.

Quoted 4 lines

AFAIK, to duplicate x86 semantics an outl/inl must spin the CPU until it completes at the target, and the CPU must not pipeline outl/inl
operations: outl(); outl(); produces 1 IOWr TLP, waits for completion, then produces another.

So that's the real question: Do we really need to duplicate x86 semantics for IO space accesses? If we do, then we need both strongly-ordered memory *and* a dsb in our accessors. That's not going to be much fun.

When a PL310 cache is used on a system that provides hardware coherency, the outer cache sync operation is useless, and can be skipped. Moreover, on some systems, it is harmful as it causes deadlocks between the Marvell coherency mechanism, the Marvell PCIe controller and the Cortex-A9.

To avoid this, this commit introduces a new Device Tree property 'dma-coherent' for the L2 cache controller node, valid only for the PL310 cache. It identifies the usage of the PL310 cache in an I/O coherent configuration. Internally, it makes the driver use a different set of l2x0_of_data, in which the ->sync operation is NULL.

Note that technically speaking, a fully coherent system wouldn't require any of the other .outer_cache operations. However, in practice, when booting secondary CPUs, these are not yet coherent, and therefore a set of cache maintenance operations are necessary at this point. This explains why we keep the other .outer_cache operations and only ->sync is disabled.

While in theory any write to a PL310 register could cause the deadlock, in practice, disabling ->sync is sufficient to workaround the deadlock, since the other cache maintenance operations are only used in very specific situations.

Signed-off-by Thomas Petazzoni <[email protected]>

Quoted 6 lines

While in theory any write to a PL310 register could cause the deadlock, in practice, disabling ->sync is sufficient to workaround the deadlock, since the other cache maintenance operations are only used in very specific situations.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by Catalin Marinas <[email protected]>

When a PL310 cache is used on a system that provides hardware coherency, the outer cache sync operation is useless, and can be skipped. Moreover, on some systems, it is harmful as it causes deadlocks between the Marvell coherency mechanism, the Marvell PCIe controller and the Cortex-A9.

To avoid this, this commit introduces a new Device Tree property 'dma-coherent' for the L2 cache controller node, valid only for the PL310 cache. It identifies the usage of the PL310 cache in an I/O coherent configuration. Internally, it makes the driver use a different set of l2x0_of_data, in which the ->sync operation is NULL.

Note that technically speaking, a fully coherent system wouldn't require any of the other .outer_cache operations. However, in practice, when booting secondary CPUs, these are not yet coherent, and therefore a set of cache maintenance operations are necessary at this point. This explains why we keep the other .outer_cache operations and only ->sync is disabled.

While in theory any write to a PL310 register could cause the deadlock, in practice, disabling ->sync is sufficient to workaround the deadlock, since the other cache maintenance operations are only used in very specific situations.

Signed-off-by Thomas Petazzoni <[email protected]>

When a PL310 cache is used on a system that provides hardware coherency, the outer cache sync operation is useless, and can be skipped. Moreover, on some systems, it is harmful as it causes deadlocks between the Marvell coherency mechanism, the Marvell PCIe controller and the Cortex-A9.

To avoid this, this commit introduces a new Device Tree property 'dma-coherent' for the L2 cache controller node, valid only for the PL310 cache. It identifies the usage of the PL310 cache in an I/O coherent configuration. Internally, it makes the driver use a different set of l2x0_of_data, in which the ->sync operation is NULL.

Note that technically speaking, a fully coherent system wouldn't require any of the other .outer_cache operations. However, in practice, when booting secondary CPUs, these are not yet coherent, and therefore a set of cache maintenance operations are necessary at this point. This explains why we keep the other .outer_cache operations and only ->sync is disabled.

While in theory any write to a PL310 register could cause the deadlock, in practice, disabling ->sync is sufficient to workaround the deadlock, since the other cache maintenance operations are only used in very specific situations.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Documentation/devicetree/bindings/arm/l2cc.txt |  3 +++
 arch/arm/mm/cache-l2x0.c                       | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

Documentation/devicetree/bindings/arm/l2cc.txt +0 -0

index b513cb8..077d837 100644

Documentation/devicetree/bindings/arm/l2cc.txt +0 -0

 - interrupts : 1 combined interrupt.
 - cache-id-part: cache id part number to be used if it is not present
   on hardware

arch/arm/mm/cache-l2x0.c +9 -0

index 7abde2ce..cf0c037 100644
@@ -889,6 +889,26 @@ static const struct l2x0_of_data pl310_data = {
        },
 };
 
+/*
+ * PL310 operations used on I/O coherent systems. Theoretically, no
+ * outer cache operations would be needed, except that for secondary
+ * processors bring up, a few cache maintenance operations are needed
+ * because secondary processors are not directly coherent with the L2
+ * cache when they start up.
+ */
+static const struct l2x0_of_data pl310_coherent_data = {
+       .setup = pl310_of_setup,

Quoted 8 lines

Documentation/devicetree/bindings/arm/l2cc.txt +0 -0

index b513cb8..077d837 100644
@@ -40,6 +40,9 @@ Optional properties:
 - arm,filter-ranges : <start length> Starting address and length of window to
   filter. Addresses in the filter window are directed to the M1 port. Other
   addresses will go to the M0 port.

Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 45

Quoted 3 lines

Documentation/devicetree/bindings/arm/l2cc.txt +3 -0

+- dma-coherent : indicates that the system is operating in an hardware
+  I/O coherent mode. Valid only when the arm,pl310-cache compatible
+  string is used.

I don't like this because it creates 2 different meanings for dma-coherent. dma-coherent is meant to be a property of DMA masters and that is not really what the L2 is. Perhaps "arm,io-coherent" or "pl310-io-coherent" instead.

Quoted 13 lines

Documentation/devicetree/bindings/arm/l2cc.txt +13 -0

+/*
+ * PL310 operations used on I/O coherent systems. Theoretically, no
+ * outer cache operations would be needed, except that for secondary
+ * processors bring up, a few cache maintenance operations are needed
+ * because secondary processors are not directly coherent with the L2
+ * cache when they start up.
+ */
+static const struct l2x0_of_data pl310_coherent_data = {
+       .setup = pl310_of_setup,
+       .save  = pl310_save,
+       .outer_cache = {
+               .resume      = pl310_resume,
+               .inv_range   = l2x0_inv_range,

Documentation/devicetree/bindings/arm/l2cc.txt

Quoted 6 lines

Documentation/devicetree/bindings/arm/l2cc.txt +6 -0

+               .clean_range = l2x0_clean_range,
+               .flush_range = l2x0_flush_range,
+               .flush_all   = l2x0_flush_all,
+               .inv_all     = l2x0_inv_all,
+       },
+};

Why do you need a whole new struct. Can't you just null out the sync ptr?

The Marvell Armada 375 and Armada 38x SOCs, which use the Cortex-A9 CPU core, the PL310 cache and the Marvell PCIe hardware block are affected a L2/PCIe deadlock caused by a system erratum when hardware I/O coherency is used.

This deadlock can be avoided by mapping the PCIe memory areas as strongly-ordered (note: MT_UNCACHED is strongly-ordered), and by removing the outer cache sync done in software. This is done in this patch, thanks to the new bits of infrastructure added in 'ARM: mm:
allow sub-architectures to override PCI I/O memory type' and 'ARM: mm:
add support for HW coherent systems in PL310' respectively.

Signed-off-by Thomas Petazzoni <[email protected]>

The Marvell Armada 375 and Armada 38x SOCs, which use the Cortex-A9 CPU core, the PL310 cache and the Marvell PCIe hardware block are affected a L2/PCIe deadlock caused by a system erratum when hardware I/O coherency is used.

This deadlock can be avoided by mapping the PCIe memory areas as strongly-ordered (note: MT_UNCACHED is strongly-ordered), and by removing the outer cache sync done in software. This is done in this patch, thanks to the new bits of infrastructure added in 'ARM: mm:
allow sub-architectures to override PCI I/O memory type' and 'ARM: mm:
add support for HW coherent systems in PL310' respectively.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/mach-mvebu/board-v7.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

arch/arm/mach-mvebu/board-v7.c +43 -0

index 01cfce6..5016c58 100644
@@ -71,6 +71,49 @@ static int armada_375_external_abort_wa(unsigned long addr, unsigned int fsr,
 	return 1;
 }
 
+/*
+ * This ioremap hook is used on Armada 375/38x to ensure that PCIe
+ * memory areas are mapped as MT_MEMORY_RW_SO instead of
+ * MT_DEVICE. This is needed as a workaround for a deadlock issue
+ * between the PCIe interface and the cache controller.
+ */
+static void __iomem *
+armada_pcie_wa_ioremap_caller(phys_addr_t phys_addr, size_t size,
+			      unsigned int mtype, void *caller)
+{
+	struct resource pcie_mem;
+
+	mvebu_mbus_get_pcie_mem_aperture(&pcie_mem);
+
+	if (pcie_mem.start <= phys_addr && (phys_addr + size) <= pcie_mem.end)
+		mtype = MT_UNCACHED;
+
+	return __arm_ioremap_caller(phys_addr, size, mtype, caller);
+}
+
+/*
+ * When we are I/O coherent and we use the PL310 cache controller, we
+ * add the PL310 property "dma-coherent". This makes sure the outer
+ * sync operation is not used, which allows to workaround the system
+ * erratum that causes deadlocks when doing PCIe in an SMP situation
+ * on Armada 375 and Armada 38x.
+ */
+static void __init mvebu_l2x0_pl310_coherent(void)
+{
+	struct device_node *np;
+
+	if (!coherency_available())
+		return;
+
+	for_each_compatible_node(np, NULL, "arm,pl310-cache") {
+		struct property *p;
+
+		p = kzalloc(sizeof(*p), GFP_KERNEL);
+		p->name = kstrdup("dma-coherent", GFP_KERNEL);
+		of_add_property(np, p);
+	}
+}
+
 static void __init mvebu_timer_and_clk_init(void)
 {
 	of_clk_init(NULL);

arch/arm/mach-mvebu/board-v7.c static void __init mvebu_timer_and_clk_init(void) · line 128

Quoted 5 lines

arch/arm/mach-mvebu/board-v7.c +5 -0

+
+	if (of_machine_is_compatible("marvell,armada375") ||
+	    of_machine_is_compatible("marvell,armada38x")) {
+		arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
+		pci_ioremap_set_mem_type(MT_UNCACHED);

iiuc, this patch depends on 1/3. So how would you like to handle it?

From my cover letter:

Dear Jason Cooper,

Quoted 7 lines · Jason Cooper

arch/arm/mach-mvebu/board-v7.c static void __init mvebu_timer_and_clk_init(void) · line 128

Quoted 5 lines

arch/arm/mach-mvebu/board-v7.c +5 -0

+
+	if (of_machine_is_compatible("marvell,armada375") ||
+	    of_machine_is_compatible("marvell,armada38x")) {
+		arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
+		pci_ioremap_set_mem_type(MT_UNCACHED);

iiuc, this patch depends on 1/3. So how would you like to handle it?

Seems like my long cover letters are not always read in their entirety :-)

From my cover letter:

* PATCH 3/3 uses both of the added infrastructures, as well as the existing infrastructure to customize the behavior of ioremap() on a per-platform basis, to implement the workaround for the Armada 375 and 38x SOCs. This patch should go through the mvebu maintainers tree. However, it has a build dependency on PATCH 1/3 that needs to be taken into account.

The last two sentences are the most important ones here :-)

Joke aside, I'm really open to your suggestions as to what the appropriate merging patch is. I know Russell prefers to have the ARM core code flow through his tree, which obviously make sense.

However, routing PATCH 3/3 through Russell tree is going to be a mess of conflicts when things will get merged by Linus, due to the numerous other changes we've been doing in mach-mvebu/board-v7.c.

So the only course of action I see right now is to work with Russell to have him pull patches 1 and 2, and then provide a stable branch/tag that you can merge as a dependency in whatever branch you decide to merge patch 3/3. Would that work?

Quoted 4 lines

Quoted 1 line

iiuc, this patch depends on 1/3. So how would you like to handle it?

Seems like my long cover letters are not always read in their entirety :-)

It was, the 'u' comes from 'u'nderstanding the coverletter _and_ the code. ;-)

One a side note, I should mention I find your detailed coverletters extremely helpful for getting up to speed quickly on what the intention of a series is. Even better, the Link tag that's added to the commit message allows future devs to be three clicks away from that information. Thanks for setting that standard.

Which makes me think, in a lot of cases, it's better to start at the top of the thread when researching a patch. The Message-Id of the coverletter is always the first address in the References header. Perhaps I should link to that instead, or add both.

Quoted 4 lines

So the only course of action I see right now is to work with Russell to have him pull patches 1 and 2, and then provide a stable branch/tag that you can merge as a dependency in whatever branch you decide to merge patch 3/3. Would that work?

Dunno. I have a 100% failure rate getting Russell to create a stable topic branch to resolve dependencies like these. Unfortunately, I think the answer is going to be resubmit 3/3 once 3.16-rc1 lands with 1/3 and 2/3.

Quoted 6 lines

arch/arm/mach-mvebu/board-v7.c +6 -0

+
+       if (of_machine_is_compatible("marvell,armada375") ||
+           of_machine_is_compatible("marvell,armada38x")) {
+               arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
+               pci_ioremap_set_mem_type(MT_UNCACHED);
+               mvebu_l2x0_pl310_coherent();

arch/arm/mach-mvebu/board-v7.c static void __init mvebu_timer_and_clk_init(void) · line 132

Quoted 3 lines

arch/arm/mach-mvebu/board-v7.c +2 -0

+       }
+
        l2x0_of_init(0, ~0UL);

I wonder whether this should just be move to the existing armada_375_380_coherency_init() function to avoid the of_machine_is_compatible() check. Other than that,