| Message ID | 1373920943-18308-1-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 15, 2013 at 1:42 PM, Rob Herring <robherring2@gmail.com> wrote: > diff --git a/arch/arm/kernel/smp_tlb.c b/arch/arm/kernel/smp_tlb.c > index 9a52a07..7fd64b7 100644 > --- a/arch/arm/kernel/smp_tlb.c > +++ b/arch/arm/kernel/smp_tlb.c > @@ -74,10 +74,21 @@ static inline void ipi_flush_bp_all(void *ignored) > static int erratum_a15_798181(void) > { > unsigned int midr = read_cpuid_id(); > + unsigned int revidr; > > /* Cortex-A15 r0p0..r3p2 affected */ > if ((midr & 0xff0ffff0) != 0x410fc0f0 || midr > 0x413fc0f2) > return 0; > + > + /* Check for Cortex A15 <= r3p2 with ECO fix */ > + revidr = read_cpuid(CPUID_REVIDR); > + if ((revidr & 0x210) == 0x210) > + return 0; Reading and evaluating all this on every invalidate seems suboptimal. It should be possible to read just once and cache the result. I would have had the same comment about the original patch, but ARM pushes those to Russell in secret so it wasn't on the list. :-) -Olof
On 07/15/2013 03:59 PM, Olof Johansson wrote: > On Mon, Jul 15, 2013 at 1:42 PM, Rob Herring <robherring2@gmail.com> wrote: >> diff --git a/arch/arm/kernel/smp_tlb.c b/arch/arm/kernel/smp_tlb.c >> index 9a52a07..7fd64b7 100644 >> --- a/arch/arm/kernel/smp_tlb.c >> +++ b/arch/arm/kernel/smp_tlb.c >> @@ -74,10 +74,21 @@ static inline void ipi_flush_bp_all(void *ignored) >> static int erratum_a15_798181(void) >> { >> unsigned int midr = read_cpuid_id(); >> + unsigned int revidr; >> >> /* Cortex-A15 r0p0..r3p2 affected */ >> if ((midr & 0xff0ffff0) != 0x410fc0f0 || midr > 0x413fc0f2) >> return 0; >> + >> + /* Check for Cortex A15 <= r3p2 with ECO fix */ >> + revidr = read_cpuid(CPUID_REVIDR); >> + if ((revidr & 0x210) == 0x210) >> + return 0; > > Reading and evaluating all this on every invalidate seems suboptimal. > It should be possible to read just once and cache the result. Yes, but I'm not sure it is so clear. 2 coproc reads may be faster than a load potentially from memory. But then how much more code do we have to fetch. How about something like this: static int erratum_a15_798181(void) { static int errata_fix_needed = -1; if (unlikely(errata_fix_needed == -1)) { unsigned int midr = read_cpuid_id(); unsigned int revidr = read_cpuid(CPUID_REVIDR); /* Cortex-A15 r0p0..r3p2 w/o ECO fix affected */ if ((midr & 0xff0ffff0) != 0x410fc0f0 || midr > 0x413fc0f2 || (revidr & 0x210) == 0x210) { errata_fix_needed = 0; return 0; } if (revidr & 0x10) errata_fix_needed = 1; errata_fix_needed = 2; } if (errata_fix_needed) dummy_flush_tlb_a15_erratum(); return (errata_fix_needed == 2) ? 1: 0; } > I would have had the same comment about the original patch, but ARM > pushes those to Russell in secret so it wasn't on the list. :-) And I would have commented that we need to handle ECO fixes... Rob
On Mon, Jul 15, 2013 at 01:59:24PM -0700, Olof Johansson wrote: > On Mon, Jul 15, 2013 at 1:42 PM, Rob Herring <robherring2@gmail.com> wrote: > > diff --git a/arch/arm/kernel/smp_tlb.c b/arch/arm/kernel/smp_tlb.c > > index 9a52a07..7fd64b7 100644 > > --- a/arch/arm/kernel/smp_tlb.c > > +++ b/arch/arm/kernel/smp_tlb.c > > @@ -74,10 +74,21 @@ static inline void ipi_flush_bp_all(void *ignored) > > static int erratum_a15_798181(void) > > { > > unsigned int midr = read_cpuid_id(); > > + unsigned int revidr; > > > > /* Cortex-A15 r0p0..r3p2 affected */ > > if ((midr & 0xff0ffff0) != 0x410fc0f0 || midr > 0x413fc0f2) > > return 0; > > + > > + /* Check for Cortex A15 <= r3p2 with ECO fix */ > > + revidr = read_cpuid(CPUID_REVIDR); > > + if ((revidr & 0x210) == 0x210) > > + return 0; > > Reading and evaluating all this on every invalidate seems suboptimal. > It should be possible to read just once and cache the result. > > I would have had the same comment about the original patch, but ARM > pushes those to Russell in secret so it wasn't on the list. :-) For a single read, there's probably not much between reading the CP15 register and doing the tests, vs reading a bool from memory to decide whether we need to do it. For more than that (as the above patch is growing) it probably makes sense to think about using a bool in memory now.
diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h index ec635ff..ac1c46a 100644 --- a/arch/arm/include/asm/cputype.h +++ b/arch/arm/include/asm/cputype.h @@ -9,6 +9,7 @@ #define CPUID_TCM 2 #define CPUID_TLBTYPE 3 #define CPUID_MPIDR 5 +#define CPUID_REVIDR 6 #ifdef CONFIG_CPU_V7M #define CPUID_EXT_PFR0 0x40 diff --git a/arch/arm/kernel/smp_tlb.c b/arch/arm/kernel/smp_tlb.c index 9a52a07..7fd64b7 100644 --- a/arch/arm/kernel/smp_tlb.c +++ b/arch/arm/kernel/smp_tlb.c @@ -74,10 +74,21 @@ static inline void ipi_flush_bp_all(void *ignored) static int erratum_a15_798181(void) { unsigned int midr = read_cpuid_id(); + unsigned int revidr; /* Cortex-A15 r0p0..r3p2 affected */ if ((midr & 0xff0ffff0) != 0x410fc0f0 || midr > 0x413fc0f2) return 0; + + /* Check for Cortex A15 <= r3p2 with ECO fix */ + revidr = read_cpuid(CPUID_REVIDR); + if ((revidr & 0x210) == 0x210) + return 0; + + dummy_flush_tlb_a15_erratum(); + if (revidr & 0x10) + return 0; + return 1; } #else @@ -97,7 +108,6 @@ static void broadcast_tlb_a15_erratum(void) if (!erratum_a15_798181()) return; - dummy_flush_tlb_a15_erratum(); smp_call_function(ipi_flush_tlb_a15_erratum, NULL, 1); } @@ -109,7 +119,6 @@ static void broadcast_tlb_mm_a15_erratum(struct mm_struct *mm) if (!erratum_a15_798181()) return; - dummy_flush_tlb_a15_erratum(); this_cpu = get_cpu(); for_each_online_cpu(cpu) { if (cpu == this_cpu)