[PATCH v3 1/6] ARM: l2c: Add DT support for Shared Override

[Catalin Marinas]

On Tue, May 05, 2015 at 01:42:21PM +0200, Geert Uytterhoeven wrote:
> On Tue, May 5, 2015 at 1:21 PM, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > On Mon, May 04, 2015 at 05:24:27PM +0200, Geert Uytterhoeven wrote:
> >> "CoreLink Level 2 Cache Controller L2C-310", p. 2-15, section 2.3.2
> >> Shareable attribute" states:
> >>
> >>     "The default behavior of the cache controller with respect to the
> >>      shareable attribute is to transform Normal Memory Non-cacheable
> >>      transactions into:
> >>         - cacheable no allocate for reads
> >>         - write through no write allocate for writes."
> >>
> >> Depending on the system architecture, this may cause memory corruption
> >> in the presence of bus mastering devices (e.g. OHCI). To avoid such
> >> corruption, the default behavior can be disabled by setting the Shared
> >> Override bit in the Auxiliary Control register.
> >>
> >> Currently the Shared Override bit can be set only using C code:
> >>   - by calling l2x0_init() directly, which is deprecated,
> >>   - by setting/clearing the bit in the machine_desc.l2c_aux_val/mask
> >>     fields, but using values differing from 0/~0 is also deprecated.
> >
> > Or you can set it in firmware/boot-loader before Linux starts.
> 
> ... together with all other additional settings cache-l2x0.c does?
> 
> I'm afraid this is not going to happen...

It's not that bad but, well, it requires changes elsewhere (U-Boot
already does this for some SoCs).

> >> Hence add support for an "arm,shared-override" device tree property for
> >> the l2c device node. By specifying this property, affected systems can
> >> indicate that non-cacheable transactions must not be transformed.
> >>
> >> If specified, the actual behavior of the kernel depends on whether CMA
> >> is available or not:
> >>   - If CMA is available, nothing needs to be done, as there won't be a
> >>     kernel linear mappings and cacheable aliases for the DMA buffers,
> >
> > I don't think this is true. See this thread:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/329492.html
> 
> Doh, and I had hoped to please Russell...
> 
> To bad, will drop this.

You should only drop the "if (dev_get_cma_area(NULL))" check.

> >>   - If CMA is not available, the "shared attribute override enable" bit
> >>     will be set.
> >
> > IMO, this should always be done, independent of any DT or kernel
> > configuration. I stated it several times already that I don't see why we
> > would ever need such bit cleared:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330573.html
> >
> > Personally I don't think this configuration belongs to the DT,
> > especially as it may depend on how the OS is configured. But since
> 
> That's why I worded the bindings like "[...] this property must be specified to
> indicate that such transforms are precluded.", not "[...] this property must be
> specified if the Shared Override bit must be set by the OS.".
> It's still up to the OS to decide (e.g. if CMA will get fixed).

We are kind of stretching this definition if the CMA check is removed.
The check could added back if DMA allocations are ever fixed to avoid
memory attributes aliases (right now they still exist, even though
temporarily). A situation where this bit probably does not need to be
set (though it's harmless) is I/O coherent systems ("arm,io-coherent").

My view is that we should always set this bit without any additional DT
properties but if it's easier to get it accepted this way, I'm fine as
well.

BTW, your patch mentions r2p0. My reading of the PL310 TRM shows this
bit as default from r0p0.

-- 
Catalin