pl310: arm: fix up define typo for the share override bit

PATCH New dinguyen at opensource.altera.com 4 participants 10 messages Oct 12, 2015 – Oct 15, 2015

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape Arm/socfpga architecture

lore

Share & tools

lore

dinguyen at opensource.altera.com

author

2015-10-12 14:59 UTC #0 lore raw

PATCH 1/2 pl310: arm: fix up define typo for the share override bit

From: Dinh Nguyen <[email protected]>

s/L310_SHARED_ATT_OVERRIDE_ENABLE/PL310_SHARED_ATT_OVERRIDE_ENABLE

Signed-off-by Dinh Nguyen <[email protected]>

---
 arch/arm/imx-common/cache.c  | 2 +-
 arch/arm/include/asm/pl310.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/imx-common/cache.c +1 -1

index 54b021c..d6b1955 100644
@@ -47,7 +47,7 @@ void v7_outer_cache_enable(void)
 	 * is cleared, PL310 treats Normal Shared Non-cacheable
 	 * accesses as Cacheable no-allocate.
 	 */
-	setbits_le32(&pl310->pl310_aux_ctrl, L310_SHARED_ATT_OVERRIDE_ENABLE);
+	setbits_le32(&pl310->pl310_aux_ctrl, PL310_SHARED_ATT_OVERRIDE_ENABLE);
 
 #if defined CONFIG_MX6SL
 	struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR;

arch/arm/include/asm/pl310.h +1 -1

index de7650e..18b90b7 100644
@@ -16,7 +16,7 @@
 #define L2X0_STNDBY_MODE_EN			(1 << 0)
 #define L2X0_CTRL_EN				1
 
-#define L310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
+#define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
 
 struct pl310_regs {
 	u32 pl310_cache_id;

Signature · 1 line

2.4.5

dinguyen at opensource.altera.com

author

2015-10-12 14:59 UTC #1 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

From: Dinh Nguyen <[email protected]>

Update the L2 AUX CTRL settings for the SoCFPGA.

Enabling D and I prefetch bits helps improve SDRAM performance on the platform.

Also, we need to enable bit 22 of the L2. By not having bit 22 set 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 Dinh Nguyen <[email protected]>

---
 arch/arm/include/asm/pl310.h |  2 ++
 arch/arm/mach-socfpga/misc.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

arch/arm/include/asm/pl310.h +2 -0

index 18b90b7..7a11405 100644
@@ -17,6 +17,8 @@
 #define L2X0_CTRL_EN				1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK	(1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK	(1 << 29)
 
 struct pl310_regs {
 	u32 pl310_cache_id;

arch/arm/mach-socfpga/misc.c +12 -0

index 0940cc5..a134760 100644
@@ -52,6 +52,18 @@ void enable_caches(void)
 #endif
 }
 
+void v7_outer_cache_enable(void)
+{
+	/* disable the L2 cache */
+	writel(0, &pl310->pl310_ctrl);
+
+	/* enable BRESP, instruction and data prefetch, full line of zeroes */
+	setbits_le32(&pl310->pl310_aux_ctrl,
+		PL310_AUX_CTRL_DATA_PREFETCH_MASK |
+		PL310_AUX_CTRL_INST_PREFETCH_MASK |
+		PL310_SHARED_ATT_OVERRIDE_ENABLE);
+}
+
 /*
  * DesignWare Ethernet initialization
  */

Signature · 1 line

2.4.5

Pavel Machek

2015-10-14 16:32 UTC #2 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

Quoted 29 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>
Update the L2 AUX CTRL settings for the SoCFPGA.
Enabling D and I prefetch bits helps improve SDRAM performance on the platform.
Also, we need to enable bit 22 of the L2. By not having bit 22 set 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 Dinh Nguyen <[email protected]>
---
 arch/arm/include/asm/pl310.h |  2 ++
 arch/arm/mach-socfpga/misc.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)
arch/arm/include/asm/pl310.h +0 -0
index 18b90b7..7a11405 100644

arch/arm/include/asm/pl310.h line 21

Quoted 5 lines

arch/arm/include/asm/pl310.h +2 -0

@@ -17,6 +17,8 @@
 #define L2X0_CTRL_EN				1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK	(1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK	(1 << 29)

These would be

arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_DATA_PREFETCH BIT(28)
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_INSTR_PREFETCH BIT(29)

...in kernel. So maybe staying with L310_ prefix makes sense? Otherwise it looks ok.

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Vasut

2015-10-15 14:32 UTC #3 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

Quoted 41 lines · Pavel Machek

Quoted 30 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Update the L2 AUX CTRL settings for the SoCFPGA.
Enabling D and I prefetch bits helps improve SDRAM performance on the platform.
Also, we need to enable bit 22 of the L2. By not having bit 22 set 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 Dinh Nguyen <[email protected]>
---
 arch/arm/include/asm/pl310.h |  2 ++
 arch/arm/mach-socfpga/misc.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)
arch/arm/include/asm/pl310.h +0 -0
index 18b90b7..7a11405 100644
arch/arm/include/asm/pl310.h line 23
Quoted 7 lines
arch/arm/include/asm/pl310.h +2 -0
@@ -17,6 +17,8 @@
 
 #define L2X0_CTRL_EN				1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
 
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK	(1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK	(1 << 29)
These would be

Quoted 7 lines

arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_DATA_PREFETCH BIT(28)
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_INSTR_PREFETCH BIT(29)
...in kernel. So maybe staying with L310_ prefix makes sense? Otherwise it looks ok.

Why is it L... in one and PL... in the other one ? What does the "PL" prefix stand for anyway ?

Best regards,
Marek Vasut

Dinh Nguyen

author

2015-10-15 15:04 UTC #4 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

Quoted 47 lines · Marek Vasut

Quoted 46 lines · Pavel Machek
Quoted 30 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Update the L2 AUX CTRL settings for the SoCFPGA.
Enabling D and I prefetch bits helps improve SDRAM performance on the platform.
Also, we need to enable bit 22 of the L2. By not having bit 22 set 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 Dinh Nguyen <[email protected]>
---
 arch/arm/include/asm/pl310.h |  2 ++
 arch/arm/mach-socfpga/misc.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)
arch/arm/include/asm/pl310.h +0 -0
index 18b90b7..7a11405 100644
arch/arm/include/asm/pl310.h line 23
Quoted 7 lines
arch/arm/include/asm/pl310.h +2 -0
@@ -17,6 +17,8 @@
 
 #define L2X0_CTRL_EN				1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
 
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK	(1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK	(1 << 29)
These would be
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_DATA_PREFETCH BIT(28)
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_INSTR_PREFETCH BIT(29)

Quoted 6 lines

Quoted 2 lines
...in kernel. So maybe staying with L310_ prefix makes sense? Otherwise it looks ok.
Why is it L... in one and PL... in the other one ? What does the "PL" prefix stand for anyway ?

As Pavel pointed out, it should be L310_x as this is how the Linux kernel is defining it. It's my mistake in the previous patch to change the define to PL310.

I'll respin this patch with L310_PREFETCH_CTRL_DATA_PREFETCH and L310_PREFETCH_CTRL_INSTR_PREFETCH.

Dinh

Marek Vasut

2015-10-15 18:08 UTC #5 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

On Thursday, October 15, 2015 at 05:04:38 PM, Dinh Nguyen wrote:

Hi!

Quoted 26 lines

Quoted 20 lines
Quoted 20 lines
arch/arm/include/asm/pl310.h line 23
Quoted 12 lines
(quoted diff) +0 -0
b/arch/arm/include/asm/pl310.h index 18b90b7..7a11405 100644
arch/arm/include/asm/pl310.h +2 -0
@@ -17,6 +17,8 @@
 
 #define L2X0_CTRL_EN				1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
 
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK	(1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK	(1 << 29)
These would be
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_DATA_PREFETCH BIT(28)
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_INSTR_PREFETCH BIT(29)
Quoted 5 lines
Quoted 2 lines
...in kernel. So maybe staying with L310_ prefix makes sense? Otherwise it looks ok.
Why is it L... in one and PL... in the other one ? What does the "PL" prefix stand for anyway ?

Quoted 6 lines

As Pavel pointed out, it should be L310_x as this is how the Linux kernel is defining it. It's my mistake in the previous patch to change the define to PL310.
I'll respin this patch with L310_PREFETCH_CTRL_DATA_PREFETCH and L310_PREFETCH_CTRL_INSTR_PREFETCH.

Well that didn't answer either of my questions ;-)

Best regards,
Marek Vasut

Dinh Nguyen

2015-10-15 18:18 UTC #6 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

Quoted 30 lines · Marek Vasut

On Thursday, October 15, 2015 at 05:04:38 PM, Dinh Nguyen wrote:
Hi!
Quoted 26 lines
Quoted 25 lines
Quoted 15 lines
arch/arm/include/asm/pl310.h line 23
Quoted 12 lines
(quoted diff) +0 -0
b/arch/arm/include/asm/pl310.h index 18b90b7..7a11405 100644
arch/arm/include/asm/pl310.h +2 -0
@@ -17,6 +17,8 @@
 
 #define L2X0_CTRL_EN                             1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE         (1 << 22)
 
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK        (1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK        (1 << 29)
These would be
Quoted 7 lines
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_DATA_PREFETCH BIT(28)
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_INSTR_PREFETCH BIT(29)
...in kernel. So maybe staying with L310_ prefix makes sense? Otherwise it looks ok.
Why is it L... in one and PL... in the other one ? What does the "PL" prefix stand for anyway ?

Quoted 8 lines

Quoted 6 lines
As Pavel pointed out, it should be L310_x as this is how the Linux kernel is defining it. It's my mistake in the previous patch to change the define to PL310.
I'll respin this patch with L310_PREFETCH_CTRL_DATA_PREFETCH and L310_PREFETCH_CTRL_INSTR_PREFETCH.
Well that didn't answer either of my questions ;-)

It should have been just 'L" and not "PL" at all.

Dinh

Marek Vasut

2015-10-15 22:33 UTC #7 lore raw

PATCH 2/2 arm: socfpga: enable data/inst prefetch and shared override in the L2

Quoted 35 lines · Dinh Nguyen

Quoted 34 lines · Marek Vasut
On Thursday, October 15, 2015 at 05:04:38 PM, Dinh Nguyen wrote:
Hi!
Quoted 30 lines
Quoted 20 lines
Quoted 20 lines
arch/arm/include/asm/pl310.h line 23
Quoted 12 lines
(quoted diff) +0 -0
b/arch/arm/include/asm/pl310.h index 18b90b7..7a11405 100644
arch/arm/include/asm/pl310.h +2 -0
@@ -17,6 +17,8 @@
 
 #define L2X0_CTRL_EN                             1
 
 #define PL310_SHARED_ATT_OVERRIDE_ENABLE         (1 << 22)
 
+#define PL310_AUX_CTRL_DATA_PREFETCH_MASK        (1 << 28)
+#define PL310_AUX_CTRL_INST_PREFETCH_MASK        (1 << 29)
These would be
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_DATA_PREFETCH BIT(28)
arch/arm/include/asm/hardware/cache-l2x0.h:#define
L310_PREFETCH_CTRL_INSTR_PREFETCH BIT(29)
Quoted 5 lines
Quoted 2 lines
...in kernel. So maybe staying with L310_ prefix makes sense? Otherwise it looks ok.
Why is it L... in one and PL... in the other one ? What does the "PL" prefix stand for anyway ?
As Pavel pointed out, it should be L310_x as this is how the Linux kernel is defining it. It's my mistake in the previous patch to change the define to PL310.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
I'll respin this patch with L310_PREFETCH_CTRL_DATA_PREFETCH and L310_PREFETCH_CTRL_INSTR_PREFETCH.
Well that didn't answer either of my questions ;-)
It should have been just 'L" and not "PL" at all.

Why ? What does that "PL" stand for and why can we omit the P ?

I don't mean to grind you, don't get me wrong, I'm gonna pick the V2 tomorrow, I just want to understand why it's enough to omit the P .

Best regards,
Marek Vasut

Pavel Machek

2015-10-14 16:31 UTC #8 lore raw

PATCH 1/2 pl310: arm: fix up define typo for the share override bit

In reply to dinguyen at opensource.altera.com

Quoted 5 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>
s/L310_SHARED_ATT_OVERRIDE_ENABLE/PL310_SHARED_ATT_OVERRIDE_ENABLE
Signed-off-by Dinh Nguyen <[email protected]>

Well, in kernel, pl310 -related registers also have L310 prefix (no PL), so I'm not sure it was a typo...

Pavel

Quoted 12 lines

index de7650e..18b90b7 100644

arch/arm/include/asm/pl310.h +1 -1

@@ -16,7 +16,7 @@
 #define L2X0_STNDBY_MODE_EN			(1 << 0)
 #define L2X0_CTRL_EN				1
 
-#define L310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
+#define PL310_SHARED_ATT_OVERRIDE_ENABLE		(1 << 22)
 
 struct pl310_regs {
 	u32 pl310_cache_id;

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Dinh Nguyen

2015-10-15 15:21 UTC #9 lore raw

PATCH 1/2 pl310: arm: fix up define typo for the share override bit

Quoted 10 lines · Pavel Machek

Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
s/L310_SHARED_ATT_OVERRIDE_ENABLE/PL310_SHARED_ATT_OVERRIDE_ENABLE
Signed-off-by Dinh Nguyen <[email protected]>
Well, in kernel, pl310 -related registers also have L310 prefix (no PL), so I'm not sure it was a typo...

You're right. Sorry for the noise.

Dinh