[PATCH v2 0/6] i.MX L2-cache code cleanups and performance tweaks

[PATCH v2 0/6] i.MX L2-cache code cleanups and performance tweaks

[Andrey Smirnov]

Hi everyone,

Here's the second version of i.MX L2-cache related patches. Here's
what's new from v1:

 - Already existing, spelling error free (shame on me for not noticing
   that in previous version!), symbolic constant defintions are used

 - Increased scope of magic number replacement (additional instance in
   cache-l2x0.c)

 - Added patch to remove unnecessary bit clearing in cache-l2x0.c

 - Added patch to implement a cleanup as suggested by Arnd

 - Added patch to tweak prefetch settings to improve performance. I
   was originally going to post this patch separately. But now that
   there was a need for v2, I decided to include it in the series.

All the feedback is much appreciated.

Thank you,
Andrey Smirnov


Andrey Smirnov (6):
  i.MX: system.c: Convert goto to if statement
  i.MX: system.c: Remove redundant errata 752271 code
  i.MX: system.c: Replace magic numbers
  ARM: cache-l2x0: Do not clear bit 23 in prefetch control register
  i.MX: system.c: Tweak prefetch settings for performance
  i.MX: Do not explicitly call l2x0_of_init()

 arch/arm/include/asm/hardware/cache-l2x0.h |  1 +
 arch/arm/mach-imx/imx35-dt.c               | 10 +++----
 arch/arm/mach-imx/mach-imx6q.c             |  2 ++
 arch/arm/mach-imx/mach-imx6sl.c            |  2 ++
 arch/arm/mach-imx/mach-imx6sx.c            |  2 ++
 arch/arm/mach-imx/system.c                 | 42 ++++++++++++------------------
 arch/arm/mm/cache-l2x0.c                   |  5 ++--
 7 files changed, 28 insertions(+), 36 deletions(-)

-- 
2.5.5

[PATCH v2 1/6] i.MX: system.c: Convert goto to if statement

[Andrey Smirnov]

Using goto here doesn't bring any advantages and only makes the code
flow less clear. No functional changes.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/system.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
index 105d1ce..d9f8b0e 100644
--- a/arch/arm/mach-imx/system.c
+++ b/arch/arm/mach-imx/system.c
@@ -106,26 +106,24 @@ void __init imx_init_l2cache(void)
 		goto out;
 	}
 
-	if (readl_relaxed(l2x0_base + L2X0_CTRL) & L2X0_CTRL_EN)
-		goto skip_if_enabled;
-
-	/* Configure the L2 PREFETCH and POWER registers */
-	val = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
-	val |= 0x70800000;
-	/*
-	 * The L2 cache controller(PL310) version on the i.MX6D/Q is r3p1-50rel0
-	 * The L2 cache controller(PL310) version on the i.MX6DL/SOLO/SL is r3p2
-	 * But according to ARM PL310 errata: 752271
-	 * ID: 752271: Double linefill feature can cause data corruption
-	 * Fault Status: Present in: r3p0, r3p1, r3p1-50rel0. Fixed in r3p2
-	 * Workaround: The only workaround to this erratum is to disable the
-	 * double linefill feature. This is the default behavior.
-	 */
-	if (cpu_is_imx6q())
-		val &= ~(1 << 30 | 1 << 23);
-	writel_relaxed(val, l2x0_base + L310_PREFETCH_CTRL);
+	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & L2X0_CTRL_EN)) {
+		/* Configure the L2 PREFETCH and POWER registers */
+		val = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
+		val |= 0x70800000;
+		/*
+		 * The L2 cache controller(PL310) version on the i.MX6D/Q is r3p1-50rel0
+		 * The L2 cache controller(PL310) version on the i.MX6DL/SOLO/SL is r3p2
+		 * But according to ARM PL310 errata: 752271
+		 * ID: 752271: Double linefill feature can cause data corruption
+		 * Fault Status: Present in: r3p0, r3p1, r3p1-50rel0. Fixed in r3p2
+		 * Workaround: The only workaround to this erratum is to disable the
+		 * double linefill feature. This is the default behavior.
+		 */
+		if (cpu_is_imx6q())
+			val &= ~(1 << 30 | 1 << 23);
+		writel_relaxed(val, l2x0_base + L310_PREFETCH_CTRL);
+	}
 
-skip_if_enabled:
 	iounmap(l2x0_base);
 	of_node_put(np);
 
-- 
2.5.5

[PATCH v2 2/6] i.MX: system.c: Remove redundant errata 752271 code

[Andrey Smirnov]

Applying a fix for ARM errata 752271 would already be taken care by a
call to a 'fixup' hook as a part of l2x0_of_init() -> __l2c_init() call
chain. Moreso the code in 'fixup' function would do that based on the
PL310's revsion information, whereas removde code does so based on SoC
version which does not work very well on i.MX6Q+ which identifies itself
as i.MX6Q as well but is not affected by 752271.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/system.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
index d9f8b0e..b153376 100644
--- a/arch/arm/mach-imx/system.c
+++ b/arch/arm/mach-imx/system.c
@@ -110,17 +110,6 @@ void __init imx_init_l2cache(void)
 		/* Configure the L2 PREFETCH and POWER registers */
 		val = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
 		val |= 0x70800000;
-		/*
-		 * The L2 cache controller(PL310) version on the i.MX6D/Q is r3p1-50rel0
-		 * The L2 cache controller(PL310) version on the i.MX6DL/SOLO/SL is r3p2
-		 * But according to ARM PL310 errata: 752271
-		 * ID: 752271: Double linefill feature can cause data corruption
-		 * Fault Status: Present in: r3p0, r3p1, r3p1-50rel0. Fixed in r3p2
-		 * Workaround: The only workaround to this erratum is to disable the
-		 * double linefill feature. This is the default behavior.
-		 */
-		if (cpu_is_imx6q())
-			val &= ~(1 << 30 | 1 << 23);
 		writel_relaxed(val, l2x0_base + L310_PREFETCH_CTRL);
 	}
 
-- 
2.5.5

[PATCH v2 3/6] i.MX: system.c: Replace magic numbers

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/include/asm/hardware/cache-l2x0.h | 1 +
 arch/arm/mach-imx/system.c                 | 5 ++++-
 arch/arm/mm/cache-l2x0.c                   | 6 ++++--
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index 3a5ec1c..e0ce59c 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -121,6 +121,7 @@
 #define L310_AUX_CTRL_STORE_LIMITATION		BIT(11)	/* R2P0+ */
 #define L310_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
 #define L310_AUX_CTRL_ASSOCIATIVITY_16		BIT(16)
+
 #define L310_AUX_CTRL_CACHE_REPLACE_RR		BIT(25)	/* R2P0+ */
 #define L310_AUX_CTRL_NS_LOCKDOWN		BIT(26)
 #define L310_AUX_CTRL_NS_INT_CTRL		BIT(27)
diff --git a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
index b153376..f0a9787 100644
--- a/arch/arm/mach-imx/system.c
+++ b/arch/arm/mach-imx/system.c
@@ -109,7 +109,10 @@ void __init imx_init_l2cache(void)
 	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & L2X0_CTRL_EN)) {
 		/* Configure the L2 PREFETCH and POWER registers */
 		val = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
-		val |= 0x70800000;
+		val |=  L310_PREFETCH_CTRL_DBL_LINEFILL |
+		        L310_PREFETCH_CTRL_INSTR_PREFETCH |
+			L310_PREFETCH_CTRL_DATA_PREFETCH |
+			L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
 		writel_relaxed(val, l2x0_base + L310_PREFETCH_CTRL);
 	}
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 9f9d542..30e2012 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -716,8 +716,10 @@ static void __init l2c310_fixup(void __iomem *base, u32 cache_id,
 	    revision < L310_CACHE_ID_RTL_R3P2) {
 		u32 val = l2x0_saved_regs.prefetch_ctrl;
 		/* I don't think bit23 is required here... but iMX6 does so */
-		if (val & (BIT(30) | BIT(23))) {
-			val &= ~(BIT(30) | BIT(23));
+		if (val & (L310_PREFETCH_CTRL_DBL_LINEFILL |
+			   L310_PREFETCH_CTRL_DBL_LINEFILL_INCR)) {
+			val &= ~(L310_PREFETCH_CTRL_DBL_LINEFILL |
+				 L310_PREFETCH_CTRL_DBL_LINEFILL_INCR);
 			l2x0_saved_regs.prefetch_ctrl = val;
 			errata[n++] = "752271";
 		}
-- 
2.5.5

[PATCH v2 4/6] ARM: cache-l2x0: Do not clear bit 23 in prefetch control register

[Andrey Smirnov]

As per L2C-310 TRM[1]:

"... You can control this feature using bits 30,27, and 23 of the
Prefetch Control Register. Bit 23 and27 are only used if you set bit 30
HIGH..."

which means there is no need to clear bit 23 if bit 30 is being cleared.

[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246e/CJAJACBJ.html

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mm/cache-l2x0.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 30e2012..12c1ba7 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -715,11 +715,8 @@ static void __init l2c310_fixup(void __iomem *base, u32 cache_id,
 	if (revision >= L310_CACHE_ID_RTL_R3P0 &&
 	    revision < L310_CACHE_ID_RTL_R3P2) {
 		u32 val = l2x0_saved_regs.prefetch_ctrl;
-		/* I don't think bit23 is required here... but iMX6 does so */
-		if (val & (L310_PREFETCH_CTRL_DBL_LINEFILL |
-			   L310_PREFETCH_CTRL_DBL_LINEFILL_INCR)) {
-			val &= ~(L310_PREFETCH_CTRL_DBL_LINEFILL |
-				 L310_PREFETCH_CTRL_DBL_LINEFILL_INCR);
+		if (val & L310_PREFETCH_CTRL_DBL_LINEFILL) {
+			val &= ~L310_PREFETCH_CTRL_DBL_LINEFILL;
 			l2x0_saved_regs.prefetch_ctrl = val;
 			errata[n++] = "752271";
 		}
-- 
2.5.5

[PATCH v2 5/6] i.MX: system.c: Tweak prefetch settings for performance

[Andrey Smirnov]

Update Prefetch Control Register settings to match that of Freescale's
Linux tree. As the commit e3addf1b773964eac7f797e8538c69481be4279c
states (author Nitin Garg):

"... set Prefetch offset to 15, since it improves memcpy performance by
35%. Don't enable Incr double Linefill enable since it adversely affects
memcpy performance by about 32MB/s and reads by 90MB/s. Tested with 4K
to 16MB sized src and dst aligned buffer..."

Those results are also corroborated by our own testing.

Tested-by: Chris Healy <cphealy@gmail.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/system.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
index f0a9787..9ea01e3 100644
--- a/arch/arm/mach-imx/system.c
+++ b/arch/arm/mach-imx/system.c
@@ -111,8 +111,12 @@ void __init imx_init_l2cache(void)
 		val = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
 		val |=  L310_PREFETCH_CTRL_DBL_LINEFILL |
 		        L310_PREFETCH_CTRL_INSTR_PREFETCH |
-			L310_PREFETCH_CTRL_DATA_PREFETCH |
-			L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+			L310_PREFETCH_CTRL_DATA_PREFETCH;
+
+		/* Set perfetch offset to improve performance */
+		val &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
+		val |= 15;
+
 		writel_relaxed(val, l2x0_base + L310_PREFETCH_CTRL);
 	}
 
-- 
2.5.5

[PATCH v2 6/6] i.MX: Do not explicitly call l2x0_of_init()

[Andrey Smirnov]

There's no need to explicitly call l2x0_of_init() since it will be
called as a part of init_IRQ() (see arch/arm/kernel/irq.c for
details). This way we can simplify imx_init_l2cache() and ditch the call
to it on i.MX35 (which does not claim compatibility with
"arm,pl310-cache") alltogether.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/imx35-dt.c    | 10 +++-------
 arch/arm/mach-imx/mach-imx6q.c  |  2 ++
 arch/arm/mach-imx/mach-imx6sl.c |  2 ++
 arch/arm/mach-imx/mach-imx6sx.c |  2 ++
 arch/arm/mach-imx/system.c      | 12 ++++--------
 5 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/imx35-dt.c b/arch/arm/mach-imx/imx35-dt.c
index e939603..52ab2c2 100644
--- a/arch/arm/mach-imx/imx35-dt.c
+++ b/arch/arm/mach-imx/imx35-dt.c
@@ -20,12 +20,6 @@
 #include "common.h"
 #include "mx35.h"
 
-static void __init imx35_irq_init(void)
-{
-	imx_init_l2cache();
-	mx35_init_irq();
-}
-
 static const char * const imx35_dt_board_compat[] __initconst = {
 	"fsl,imx35",
 	NULL
@@ -34,6 +28,8 @@ static const char * const imx35_dt_board_compat[] __initconst = {
 DT_MACHINE_START(IMX35_DT, "Freescale i.MX35 (Device Tree Support)")
 	.map_io		= mx35_map_io,
 	.init_early	= imx35_init_early,
-	.init_irq	= imx35_irq_init,
+	.init_irq	= mx35_init_irq,
 	.dt_compat	= imx35_dt_board_compat,
+	.l2c_aux_val 	= 0,
+	.l2c_aux_mask	= ~0,
 MACHINE_END
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index cb27d56..9360a8a 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -413,4 +413,6 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad/DualLite (Device Tree)")
 	.init_machine	= imx6q_init_machine,
 	.init_late      = imx6q_init_late,
 	.dt_compat	= imx6q_dt_compat,
+	.l2c_aux_val 	= 0,
+	.l2c_aux_mask	= ~0,
 MACHINE_END
diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
index 3003263..c665551 100644
--- a/arch/arm/mach-imx/mach-imx6sl.c
+++ b/arch/arm/mach-imx/mach-imx6sl.c
@@ -79,4 +79,6 @@ DT_MACHINE_START(IMX6SL, "Freescale i.MX6 SoloLite (Device Tree)")
 	.init_machine	= imx6sl_init_machine,
 	.init_late      = imx6sl_init_late,
 	.dt_compat	= imx6sl_dt_compat,
+	.l2c_aux_val 	= 0,
+	.l2c_aux_mask	= ~0,
 MACHINE_END
diff --git a/arch/arm/mach-imx/mach-imx6sx.c b/arch/arm/mach-imx/mach-imx6sx.c
index 6a0b061..2c3f927 100644
--- a/arch/arm/mach-imx/mach-imx6sx.c
+++ b/arch/arm/mach-imx/mach-imx6sx.c
@@ -107,4 +107,6 @@ DT_MACHINE_START(IMX6SX, "Freescale i.MX6 SoloX (Device Tree)")
 	.init_machine	= imx6sx_init_machine,
 	.dt_compat	= imx6sx_dt_compat,
 	.init_late	= imx6sx_init_late,
+	.l2c_aux_val 	= 0,
+	.l2c_aux_mask	= ~0,
 MACHINE_END
diff --git a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
index 9ea01e3..a89df44 100644
--- a/arch/arm/mach-imx/system.c
+++ b/arch/arm/mach-imx/system.c
@@ -98,13 +98,11 @@ void __init imx_init_l2cache(void)
 
 	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
 	if (!np)
-		goto out;
+		return;
 
 	l2x0_base = of_iomap(np, 0);
-	if (!l2x0_base) {
-		of_node_put(np);
-		goto out;
-	}
+	if (!l2x0_base)
+		goto put_node;
 
 	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & L2X0_CTRL_EN)) {
 		/* Configure the L2 PREFETCH and POWER registers */
@@ -121,9 +119,7 @@ void __init imx_init_l2cache(void)
 	}
 
 	iounmap(l2x0_base);
+put_node:
 	of_node_put(np);
-
-out:
-	l2x0_of_init(0, ~0);
 }
 #endif
-- 
2.5.5

Re: [PATCH v2 3/6] i.MX: system.c: Replace magic numbers

[Uwe Kleine-König]

Hello,

On Tue, Jun 14, 2016 at 06:29:50PM -0700, Andrey Smirnov wrote:
> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> index 3a5ec1c..e0ce59c 100644
> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> @@ -121,6 +121,7 @@
>  #define L310_AUX_CTRL_STORE_LIMITATION		BIT(11)	/* R2P0+ */
>  #define L310_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
>  #define L310_AUX_CTRL_ASSOCIATIVITY_16		BIT(16)
> +
>  #define L310_AUX_CTRL_CACHE_REPLACE_RR		BIT(25)	/* R2P0+ */
>  #define L310_AUX_CTRL_NS_LOCKDOWN		BIT(26)
>  #define L310_AUX_CTRL_NS_INT_CTRL		BIT(27)

I'd not include this hunk in this patch.

> diff --git a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
> index b153376..f0a9787 100644
> --- a/arch/arm/mach-imx/system.c
> +++ b/arch/arm/mach-imx/system.c
> @@ -109,7 +109,10 @@ void __init imx_init_l2cache(void)
>  	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & L2X0_CTRL_EN)) {
>  		/* Configure the L2 PREFETCH and POWER registers */
>  		val = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
> -		val |= 0x70800000;
> +		val |=  L310_PREFETCH_CTRL_DBL_LINEFILL |

s/  / / ?

> +		        L310_PREFETCH_CTRL_INSTR_PREFETCH |
> +			L310_PREFETCH_CTRL_DATA_PREFETCH |
> +			L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
>  		writel_relaxed(val, l2x0_base + L310_PREFETCH_CTRL);
>  	}
>  

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 6/6] i.MX: Do not explicitly call l2x0_of_init()

[Arnd Bergmann]

On Tuesday, June 14, 2016 6:29:53 PM CEST Andrey Smirnov wrote:
>  DT_MACHINE_START(IMX35_DT, "Freescale i.MX35 (Device Tree Support)")
>         .map_io         = mx35_map_io,
>         .init_early     = imx35_init_early,
> -       .init_irq       = imx35_irq_init,
> +       .init_irq       = mx35_init_irq,
>         .dt_compat      = imx35_dt_board_compat,
> +       .l2c_aux_val    = 0,
> +       .l2c_aux_mask   = ~0,
>  MACHINE_END
> 

Please keep the fields sorted the same way that they are defined in
struct machine_desc, i.e. add the new fields at the top of this list.

Otherwise the whole series looks good to me now,

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v2 3/6] i.MX: system.c: Replace magic numbers

[Shawn Guo]

On Tue, Jun 14, 2016 at 06:29:50PM -0700, Andrey Smirnov wrote:
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/include/asm/hardware/cache-l2x0.h | 1 +

As Uwe already pointed out, it's not necessary to change this file.

>  arch/arm/mach-imx/system.c                 | 5 ++++-
>  arch/arm/mm/cache-l2x0.c                   | 6 ++++--

I suggest you put the changes on cache-l2x0.c into a separate patch and
send it to Russell.

Shawn

>  3 files changed, 9 insertions(+), 3 deletions(-)