Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425885 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] i.MX L2-cache code cleanups and performance tweaks Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 02:40 +0200
    [PATCH v3 2/5] i.MX: system.c: Remove redundant errata 752271 code Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 02:40 +0200
    [PATCH 1/2] ARM: cache-l2x0.c: Replace magic numbers Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 02:40 +0200
    [PATCH 2/2] ARM: cache-l2x0.c: Do not clear bit 23 in prefetch control register Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 02:40 +0200
    Re: [PATCH v3 0/5] i.MX L2-cache code cleanups and performance tweaks Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 03:10 +0200
    Re: [PATCH v3 0/5] i.MX L2-cache code cleanups and performance tweaks Arnd Bergmann <arnd@arndb.de> - 2016-06-19 16:50 +0200
      Re: [PATCH v3 0/5] i.MX L2-cache code cleanups and performance tweaks Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 20:00 +0200
        Re: [PATCH v3 0/5] i.MX L2-cache code cleanups and performance tweaks Arnd Bergmann <arnd@arndb.de> - 2016-06-21 01:50 +0200

#1425885 — [PATCH v3 0/5] i.MX L2-cache code cleanups and performance tweaks

Hi everyone,

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

       - Generic ARM patches were split inot a separte series

       - Typo and whitespace fixes (as per Uwe's suggestions)

       - Re-ordered l2c_aux_val, and l2c_aux_mask are initializers are placed
         in the order of their declaration

Let me know if any more changes to the series are needed.

Andrey Smirnov

Andrey Smirnov (5):
  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
  i.MX: system.c: Tweak prefetch settings for performance
  i.MX: Do not explicitly call l2x0_of_init()

 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 ++++++++++++++++-------------------------
 5 files changed, 25 insertions(+), 33 deletions(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1425886 — [PATCH v3 2/5] i.MX: system.c: Remove redundant errata 752271 code

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 removed 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.

Acked-by: Arnd Bergmann <arnd@arndb.de>
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

[toc] | [prev] | [next] | [standalone]

#1425887 — [PATCH 1/2] ARM: cache-l2x0.c: Replace magic numbers

Replace magic numbers used for L310 Prefetch Control Register

Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mm/cache-l2x0.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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

[toc] | [prev] | [next] | [standalone]

#1425888 — [PATCH 2/2] ARM: cache-l2x0.c: Do not clear bit 23 in prefetch control register

As per L2C-310 TRM[1]:

"... You can control this feature using bits 30,27 and 23 of the
Prefetch Control Register. Bit 23 and 27 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

Acked-by: Arnd Bergmann <arnd@arndb.de>
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

[toc] | [prev] | [next] | [standalone]

#1425898

Please ignore this version of the patch set, I just realized that I
screwed up patch 3/5. I'll be sending fixed v4 shortly.

Sorry for the noise,
Andrey Smirnov

On Sat, Jun 18, 2016 at 5:31 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Hi everyone,
>
> Here's the second version of i.MX L2-cache related patches. Here's
> what's new from v2:
>
>        - Generic ARM patches were split inot a separte series
>
>        - Typo and whitespace fixes (as per Uwe's suggestions)
>
>        - Re-ordered l2c_aux_val, and l2c_aux_mask are initializers are placed
>          in the order of their declaration
>
> Let me know if any more changes to the series are needed.
>
> Andrey Smirnov
>
> Andrey Smirnov (5):
>   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
>   i.MX: system.c: Tweak prefetch settings for performance
>   i.MX: Do not explicitly call l2x0_of_init()
>
>  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 ++++++++++++++++-------------------------
>  5 files changed, 25 insertions(+), 33 deletions(-)
>
> --
> 2.5.5
>

[toc] | [prev] | [next] | [standalone]

#1425983

On Saturday, June 18, 2016 5:31:01 PM CEST Andrey Smirnov wrote:
> Hi everyone,
> 
> Here's the second version of i.MX L2-cache related patches. Here's
> what's new from v2:
> 
>        - Generic ARM patches were split inot a separte series
> 
>        - Typo and whitespace fixes (as per Uwe's suggestions)
> 
>        - Re-ordered l2c_aux_val, and l2c_aux_mask are initializers are placed
>          in the order of their declaration
> 
> Let me know if any more changes to the series are needed.

One more question, less about your patches than the existing code:

After going through the current users of l2x0_init for board files,
I noticed that imx35 enables l2x0 for both dts and all the board files
and has an arm,l210-cache node in its dts, but imx31 only enables
it for the board file and in the dt case doesn't init the cache nor
does it have the dts node.

I'm guessing this is a bug on i.mx31 dt support, right?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1426014

 One more question, less about your patches than the existing code:
>
> After going through the current users of l2x0_init for board files,
> I noticed that imx35 enables l2x0 for both dts and all the board files
> and has an arm,l210-cache node in its dts, but imx31 only enables
> it for the board file and in the dt case doesn't init the cache nor
> does it have the dts node.
>
> I'm guessing this is a bug on i.mx31 dt support, right?

I would agree that it is. OTOH, I can't find any i.MX31 boards that
use device tree. The only file that references imx31.dtsi is
imx31-bug.dts, but at the same time that board has a dedicated board
file (mach-bug.c) so I wonder if it was ever used.

It looks like Sacha was the author of i.MX31, Sascha do you have any
comment on this?

There's also another small cleanup opportunity in collapsing
imx31_dt_timer_init() and mx31_clocks_init_dt() into a single function
given how the latter always returns 0 and can be converted to void.

I am more than happy to make both changes and include them in the set,
but I only have i.MX6 HW, and would only be able to do a compile-test,
so I am not sure if I should.

Thanks,
Andrey Smirnov

[toc] | [prev] | [next] | [standalone]

#1427132

On Sunday, June 19, 2016 10:53:13 AM CEST Andrey Smirnov wrote:
>  One more question, less about your patches than the existing code:
> >
> > After going through the current users of l2x0_init for board files,
> > I noticed that imx35 enables l2x0 for both dts and all the board files
> > and has an arm,l210-cache node in its dts, but imx31 only enables
> > it for the board file and in the dt case doesn't init the cache nor
> > does it have the dts node.
> >
> > I'm guessing this is a bug on i.mx31 dt support, right?
> 
> I would agree that it is. OTOH, I can't find any i.MX31 boards that
> use device tree. The only file that references imx31.dtsi is
> imx31-bug.dts, but at the same time that board has a dedicated board
> file (mach-bug.c) so I wonder if it was ever used.

My understanding is that many of the i.mx dts files were introduced
a few years ago as conversions of the board files, and we are now
in the (slow) process of removing the board files, assuming that
everyone has had time for migration if they still run that hardware
on new kernels, or that it doesn't hurt if nobody uses it.

This one was added with the comment

    arm/dts: Add support for i.MX31 bug 1.x board from buglabs.
    
    Only the main UART and the memory node information are added.

and it's unclear if that has ever been tested. A few device nodes
were added to the imx31.dtsi file later, indicating that it probably
had /some/ testing, but no other board file was ever added as you
say. The only difference I see between mach-bug.c and imx31-bug.dts
is the configuration of the uart pins. I don't know if that is
required in the dts, because I'd assume that the boot loader would
leave the pinmux in a state in which the console uart works.

> It looks like Sacha was the author of i.MX31, Sascha do you have any
> comment on this?
> 
> There's also another small cleanup opportunity in collapsing
> imx31_dt_timer_init() and mx31_clocks_init_dt() into a single function
> given how the latter always returns 0 and can be converted to void.

Or we could go one step further and use CLK_OF_DECLARE() to remove
that init function entirely. Same thing for mx31_init_irq and
IRQCHIP_DECLARE().

> I am more than happy to make both changes and include them in the set,
> but I only have i.MX6 HW, and would only be able to do a compile-test,
> so I am not sure if I should.

Let's wait for Sascha to reply.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web