[PATCHv2,3/4] ARM: mm: add support for HW coherent systems in PL310

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 separate
'arm,pl310-coherent-cache' compatible string, which identifies a PL310
cache in an I/O coherent configuration. This compatible string is
associated with 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 <thomas.petazzoni@free-electrons.com>
---
 Documentation/devicetree/bindings/arm/l2cc.txt |  2 ++
 arch/arm/mm/cache-l2x0.c                       | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

On Tue, May 13, 2014 at 11:10:38AM +0100, Thomas Petazzoni wrote:
> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> @@ -8,6 +8,8 @@ Required properties:
>  
>  - compatible : should be one of:
>    "arm,pl310-cache"
> +  "arm,pl310-coherent-cache", used for I/O coherent platforms using
> +     the PL310 cache
>    "arm,l220-cache"
>    "arm,l210-cache"
>    "bcm,bcm11351-a2-pl310-cache": DEPRECATED by "brcm,bcm11351-a2-pl310-cache"

The binding name kind of implies that we have a transparent PL310 cache
(at least to me), which is not the case. I would rather have a specific
dma-coherent property or something similar since it's not another type
of PL310 but rather a different SoC topology.

But I recall you mentioned that you can't make this decision at the DT
level since you don't know before whether the SoC is I/O coherent or
not.

Dear Catalin Marinas,

On Wed, 14 May 2014 15:34:48 +0100, Catalin Marinas wrote:
> On Tue, May 13, 2014 at 11:10:38AM +0100, Thomas Petazzoni wrote:
> > --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> > @@ -8,6 +8,8 @@ Required properties:
> >  
> >  - compatible : should be one of:
> >    "arm,pl310-cache"
> > +  "arm,pl310-coherent-cache", used for I/O coherent platforms using
> > +     the PL310 cache
> >    "arm,l220-cache"
> >    "arm,l210-cache"
> >    "bcm,bcm11351-a2-pl310-cache": DEPRECATED by "brcm,bcm11351-a2-pl310-cache"
> 
> The binding name kind of implies that we have a transparent PL310 cache
> (at least to me), which is not the case. I would rather have a specific
> dma-coherent property or something similar since it's not another type
> of PL310 but rather a different SoC topology.

Fine with me. A separate property or a different compatible string is
the same.

> But I recall you mentioned that you can't make this decision at the DT
> level since you don't know before whether the SoC is I/O coherent or
> not.

As of today, hardware I/O coherency can only be enabled if CONFIG_SMP
is enabled *and* the SoC is actually multi-core, due to limitations in
the ARM core code. So if you look at my PATCH 4/4 in this series, you
will see that I adjust the compatible string of the cache controller at
run time, depending on whether hardware I/O coherency is enabled or not:

+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 *new_compat;
+
+		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
+		new_compat->name = kstrdup("compatible", GFP_KERNEL);
+		new_compat->value = kstrdup("arm,pl310-coherent-cache",
+					    GFP_KERNEL);
+		new_compat->length = strlen(new_compat->value) + 1;
+		of_update_property(np, new_compat);
+	}
+}

So, this code can just as well be changed to add a property instead of
adjusting the compatible property. It's the same thing for me.

Also, I will send (hopefully today) a series that allows to enable
hardware I/O coherency even on !SMP configurations. But I believe it
might take a while to get merged.

Best regards,

Thomas