[PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Michal Simek]

From: Thomas Betker <thomas.betker@rohde-schwarz.com>

This patch is based on the
commit 1a8e41cd672f ("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.

For Zynq, this fix avoids memory inconsistencies between Gigabit
Ethernet controller (GEM) and CPU when DMA_CMA is disabled.

Suggested-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 arch/arm/mach-zynq/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 39c1c7d43522..af36dc2545c1 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -197,8 +197,8 @@ static const char * const zynq_dt_match[] = {
 
 DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
 	/* 64KB way size, 8-way associativity, parity disabled */
-	.l2c_aux_val	= 0x00000000,
-	.l2c_aux_mask	= 0xffffffff,
+	.l2c_aux_val    = 0x00400000,
+	.l2c_aux_mask	= 0xffbfffff,
 	.smp		= smp_ops(zynq_smp_ops),
 	.map_io		= zynq_map_io,
 	.init_irq	= zynq_irq_init,
-- 
2.3.5

Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Dirk Behme]

On 12.05.2015 08:22, Michal Simek wrote:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
>
> This patch is based on the
> commit 1a8e41cd672f ("ARM: 6395/1: VExpress: Set bit 22 in the PL310
> (cache controller) AuxCtlr register")


I've been under the impression that this shouldn't be done in the 
kernel, but in the boot loader/firmware:

https://lkml.org/lkml/2015/2/20/199

http://lists.denx.de/pipermail/u-boot/2015-March/207803.html

Best regards

Dirk


> 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.
>
> For Zynq, this fix avoids memory inconsistencies between Gigabit
> Ethernet controller (GEM) and CPU when DMA_CMA is disabled.
>
> Suggested-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>   arch/arm/mach-zynq/common.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
> index 39c1c7d43522..af36dc2545c1 100644
> --- a/arch/arm/mach-zynq/common.c
> +++ b/arch/arm/mach-zynq/common.c
> @@ -197,8 +197,8 @@ static const char * const zynq_dt_match[] = {
>
>   DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
>   	/* 64KB way size, 8-way associativity, parity disabled */
> -	.l2c_aux_val	= 0x00000000,
> -	.l2c_aux_mask	= 0xffffffff,
> +	.l2c_aux_val    = 0x00400000,
> +	.l2c_aux_mask	= 0xffbfffff,
>   	.smp		= smp_ops(zynq_smp_ops),
>   	.map_io		= zynq_map_io,
>   	.init_irq	= zynq_irq_init,
>

Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Michal Simek]

On 05/12/2015 08:31 AM, Dirk Behme wrote:
> On 12.05.2015 08:22, Michal Simek wrote:
>> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
>>
>> This patch is based on the
>> commit 1a8e41cd672f ("ARM: 6395/1: VExpress: Set bit 22 in the PL310
>> (cache controller) AuxCtlr register")
> 
> 
> I've been under the impression that this shouldn't be done in the
> kernel, but in the boot loader/firmware:
> 
> https://lkml.org/lkml/2015/2/20/199
> 
> http://lists.denx.de/pipermail/u-boot/2015-March/207803.html

Tegra, Exynos, sti still have this bit set.
Does that mean that they should be just removed because fix should be in
bootloader?

Anyway it is normal that bootloader stay on system untouched and only OS
is updated. But OK - let's make this in bootloader if this is preferred
solution.

Thanks,
Michal

Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Thomas.Betker]

> >> This patch is based on the
> >> commit 1a8e41cd672f ("ARM: 6395/1: VExpress: Set bit 22 in the PL310
> >> (cache controller) AuxCtlr register")
> > 
> > 
> > I've been under the impression that this shouldn't be done in the
> > kernel, but in the boot loader/firmware:
> > 
> > https://lkml.org/lkml/2015/2/20/199
> > 
> > http://lists.denx.de/pipermail/u-boot/2015-March/207803.html
> 
> Tegra, Exynos, sti still have this bit set.

In 4.1-rc3, the bit is set by berlin, exynos, nomadik, omap2, sti, tegra, 
vexpress.

> Does that mean that they should be just removed because fix should be in
> bootloader?
> 
> Anyway it is normal that bootloader stay on system untouched and only OS
> is updated. But OK - let's make this in bootloader if this is preferred
> solution.

So the plan is to update each and every Zynq bootloader for a problem we 
have encountered in Linux (in our case, a problem we have been hunting for 
months)? My u-boot doesn't even use the cache; it's disabled until the 
kernel boots.

I do understand that the kernel should not overwrite hardwired settings 
such as cache size, but shouldn't we at least allow to fix things that 
definitely need to be fixed?

Best regards,
Thomas Betker

Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

Something tells me that Russell's patch system won't like to accept a
patch with a duplicate ID (although, I could be wrong).

On Tue, May 12, 2015 at 08:22:01AM +0200, Michal Simek wrote:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> 
> This patch is based on the
> commit 1a8e41cd672f ("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.
> 
> For Zynq, this fix avoids memory inconsistencies between Gigabit
> Ethernet controller (GEM) and CPU when DMA_CMA is disabled.

In practice, we've seen corruption not only with the GEM but also the
UDC (and likely other things as well).  So, this patch is welcome!

> Suggested-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

This feels like stable material as well, to me.  (Although, I'd expect a
bit of manual work to get it backported, with the fairly recent L2
reworking).

Thanks,
  Josh

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Catalin Marinas]

On Tue, May 12, 2015 at 08:31:35AM +0200, Dirk Behme wrote:
> On 12.05.2015 08:22, Michal Simek wrote:
> >From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> >
> >This patch is based on the
> >commit 1a8e41cd672f ("ARM: 6395/1: VExpress: Set bit 22 in the PL310
> >(cache controller) AuxCtlr register")
> 
> I've been under the impression that this shouldn't be done in the kernel,
> but in the boot loader/firmware:
> 
> https://lkml.org/lkml/2015/2/20/199
> 
> http://lists.denx.de/pipermail/u-boot/2015-March/207803.html

If you can fix it in the boot loader or firmware even better. If it's
not doable, this patch will help (if Russell takes it):

http://article.gmane.org/gmane.linux.ports.sh.devel/45685

-- 
Catalin

Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)

[Thomas.Betker]

Hello Catalin:

> > I've been under the impression that this shouldn't be done in the 
kernel,
> > but in the boot loader/firmware:
> > 
> > https://lkml.org/lkml/2015/2/20/199
> > 
> > http://lists.denx.de/pipermail/u-boot/2015-March/207803.html
> 
> If you can fix it in the boot loader or firmware even better. If it's
> not doable, this patch will help (if Russell takes it):
> 
> http://article.gmane.org/gmane.linux.ports.sh.devel/45685

Thanks for the info! Geert's patch looks good to me, and I sure hope it 
will be accepted.

Best regards,
Thomas Betker