| Message ID | 1430753072-17145-2-git-send-email-geert+renesas@glider.be |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 1 warnings, 0 lines checked |
| robh/patch-applied | success |
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. > 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 > - 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 Russell has refused to take patches setting this bit by default (I have yet to see a valid argument), it's still better than nothing.
Hi Catalin, On Tue, May 5, 2015 at 1:21 PM, Catalin Marinas <catalin.marinas@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... >> 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. >> - 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). > Russell has refused to take patches setting this bit by default (I have > yet to see a valid argument), it's still better than nothing. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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.
Hi Catalin, On Tue, May 5, 2015 at 4:22 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > 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@arm.com> wrote: >> > On Mon, May 04, 2015 at 05:24:27PM +0200, Geert Uytterhoeven wrote: >> >> 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. Of course. > BTW, your patch mentions r2p0. My reading of the PL310 TRM shows this > bit as default from r0p0. Arnd told me he had read the documentation for r0p0 and couldn't find it. The r3p2 manual lists the following changes between r1p0-r2p0: - new behavior linked to the Shared attribute. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 05, 2015 at 04:55:02PM +0200, Geert Uytterhoeven wrote: > On Tue, May 5, 2015 at 4:22 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > BTW, your patch mentions r2p0. My reading of the PL310 TRM shows this > > bit as default from r0p0. > > Arnd told me he had read the documentation for r0p0 and couldn't find it. > The r3p2 manual lists the following changes between r1p0-r2p0: > - new behavior linked to the Shared attribute. Bit 22 is present since r0p0 with the same description as in r2p0 and later: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246a/Ceggcfcj.html I guess the new behaviour is about bit 13, Shared Attribute Invalidate Enable: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246c/Ceggcfcj.html But that's outside the scope of this patch.
On Tue, May 5, 2015 at 9:22 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > 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@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"). IIRC, it still had to be set on highbank even when we had coherent i/o. > 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. +1 > BTW, your patch mentions r2p0. My reading of the PL310 TRM shows this > bit as default from r0p0. I believe the change in behavior was from the L2x0 to PL310. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt index 0dbabe9a6b0abb91..f6f81b263a9cf56d 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt +++ b/Documentation/devicetree/bindings/arm/l2cc.txt @@ -67,6 +67,12 @@ Optional properties: disable if zero. - arm,prefetch-offset : Override prefetch offset value. Valid values are 0-7, 15, 23, and 31. +- arm,shared-override : As of r2p0, the default behavior of the pl310 cache + controller with respect to the shareable attribute is to transform "normal + memory non-cacheable transactions" into "cacheable no allocate" (for reads) + or "write through no write allocate" (for writes). + On systems where this may cause DMA buffer corruption, this property must be + specified to indicate that such transforms are precluded. Example: diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index e309c8f35af5af61..535b575ac3d7a52f 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -17,6 +17,7 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ #include <linux/cpu.h> +#include <linux/dma-contiguous.h> #include <linux/err.h> #include <linux/init.h> #include <linux/smp.h> @@ -1149,6 +1150,15 @@ static void __init l2c310_of_parse(const struct device_node *np, } } + if (of_property_read_bool(np, "arm,shared-override")) { + if (dev_get_cma_area(NULL)) { + pr_info("L2C-310 ignoring arm,shared-override when CMA is enabled\n"); + } else { + *aux_val |= L2C_AUX_CTRL_SHARED_OVERRIDE; + *aux_mask &= ~L2C_AUX_CTRL_SHARED_OVERRIDE; + } + } + prefetch = l2x0_saved_regs.prefetch_ctrl; ret = of_property_read_u32(np, "arm,double-linefill", &val);
"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. 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, - If CMA is not available, the "shared attribute override enable" bit will be set. See also commit 1a8e41cd672f894b ("ARM: 6395/1: VExpress: Set bit 22 in the PL310 (cache controller) AuxCtlr register"): "Clearing bit 22 in the PL310 Auxiliary Control register (shared attribute override enable) has the side effect of transforming Normal Shared Non-cacheable reads into Cacheable no-allocate reads. Coherent DMA buffers in Linux always have a Cacheable alias via the kernel linear mapping and the processor can speculatively load cache lines into the PL310 controller. With bit 22 cleared, Non-cacheable reads would unexpectedly hit such cache lines leading to buffer corruption." Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3: - New. --- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++++ arch/arm/mm/cache-l2x0.c | 10 ++++++++++ 2 files changed, 16 insertions(+)