Re: [PATCH 1/3] MIPS: Introduce WAR_4KC_LLSC config option

[Jonas Gorski]

On Mon, 22 May 2023 at 22:38, Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx> wrote:
>
>
>
> > 2023年5月22日 19:40，Jonas Gorski <jonas.gorski@xxxxxxxxx> 写道：
> >
> > Hi,
> >
> > On Fri, 19 May 2023 at 18:49, Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx> wrote:
> >>
> >> WAR_4KC_LLSC is used to control workaround of 4KC LLSC issue
> >> that affects 4Kc up to version 0.9.
> >>
> >> Early ath25 chips are known to be affected.
> >>
> >> Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> >> ---
> >> arch/mips/Kconfig                                        | 6 ++++++
> >> arch/mips/include/asm/cpu.h                              | 1 +
> >> arch/mips/include/asm/mach-ath25/cpu-feature-overrides.h | 2 +-
> >> arch/mips/kernel/cpu-probe.c                             | 7 +++++++
> >> 4 files changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> >> index 30e90a2d53f4..354d033364ad 100644
> >> --- a/arch/mips/Kconfig
> >> +++ b/arch/mips/Kconfig
> >> @@ -230,6 +230,7 @@ config ATH25
> >>        select SYS_SUPPORTS_BIG_ENDIAN
> >>        select SYS_SUPPORTS_32BIT_KERNEL
> >>        select SYS_HAS_EARLY_PRINTK
> >> +       select WAR_4KC_LLSC if !SOC_AR5312
> >
> > Shouldn't this be "if SOC_AR5312"?
>
> Ah sorry, I misread the original code.
>
> >
> > Though since you are adding runtime detection/correction below, I
> > wonder if this would be really needed as an extra symbol, and rather
> > use the later introduced (CPU_MAY_HAVE_LLSC) directly.
>
> I bet it’s better to have a symbol just for tracking errata. So we can easily know
> if SoC is affected by a errata and have some extra documentation.
>
> >
> > Or rather have select "CPU_HAS_LLSC if !SOC_AR5312" in that case.
> >
> >>        help
> >>          Support for Atheros AR231x and Atheros AR531x based boards
> >>
> >> @@ -2544,6 +2545,11 @@ config WAR_ICACHE_REFILLS
> >> config WAR_R10000_LLSC
> >>        bool
> >>
> >> +# On 4Kc up to version 0.9 (PRID_REV < 1) there is a bug that may cause llsc
> >> +# sequences to deadlock.
> >> +config WAR_4KC_LLSC
> >> +       bool
> >> +
> >> # 34K core erratum: "Problems Executing the TLBR Instruction"
> >> config WAR_MIPS34K_MISSED_ITLB
> >>        bool
> >> diff --git a/arch/mips/include/asm/cpu.h b/arch/mips/include/asm/cpu.h
> >> index ecb9854cb432..84bb1931a8b4 100644
> >> --- a/arch/mips/include/asm/cpu.h
> >> +++ b/arch/mips/include/asm/cpu.h
> >> @@ -247,6 +247,7 @@
> >> #define PRID_REV_VR4122                        0x0070
> >> #define PRID_REV_VR4181A               0x0070  /* Same as VR4122 */
> >> #define PRID_REV_VR4130                        0x0080
> >> +#define PRID_REV_4KC_V1_0              0x0001
> >> #define PRID_REV_34K_V1_0_2            0x0022
> >> #define PRID_REV_LOONGSON1B            0x0020
> >> #define PRID_REV_LOONGSON1C            0x0020  /* Same as Loongson-1B */
> >> diff --git a/arch/mips/include/asm/mach-ath25/cpu-feature-overrides.h b/arch/mips/include/asm/mach-ath25/cpu-feature-overrides.h
> >> index ec3604c44ef2..5df292b1ff04 100644
> >> --- a/arch/mips/include/asm/mach-ath25/cpu-feature-overrides.h
> >> +++ b/arch/mips/include/asm/mach-ath25/cpu-feature-overrides.h
> >> @@ -24,7 +24,7 @@
> >> #define cpu_has_counter                        1
> >> #define cpu_has_ejtag                  1
> >>
> >> -#if !defined(CONFIG_SOC_AR5312)
> >> +#if !defined(WAR_4KC_LLSC)
> >> #  define cpu_has_llsc                 1
> >
> > since the #else path defines cpu_has_llsc as 0, it means that kernels
> > targeting both SoCs would force llsc to be unavailable (not introduced
> > by you).
>
> I’m a little bit confused.
> The logic seems very clear to me: If a SoC is not affected by WAR_4KC_LLSC,
> then wire  cpu_has_llsc to 1, else wire it to 0.

ATH25 allows you building for multiple SoCs at the same time, and if
you do so, you don't know in advance on which SoC you boot. So you
need to have third path here where cpu_has_llsc isn't wired to
anything.

This is wrong in the current code already, so should be fixed there.

>
> It matches my intention.
>
> >
> > So this probably should be rather this:
> >
> > #if !defined(CONFIG_SOC_AR5312)
> > #define cpu_has_llsc 1
> > #else if !defined(CONFIG_SOC_AR5312)
> > #define cpu_has_llsc 0
> > #endif
>
> The condition on if leg seems same to the else leg, I’m not sure if it can ever work.

Sorry, I typo'd. I wanted to write

#if !defined(CONFIG_SOC_AR5312)
#define cpu_has_llsc 1
#else if !defined(CONFIG_SOC_AR2315)
#define cpu_has_llsc 0
(#else /* don't define it */)
#endif

Only SOC_AR2315 selected => cpu_has_llsc = 1
Only SOC_AR5312 selected => cpu_has_llsc = 0
both selected => cpu_has_llsc will need to be determined at runtime
(don't set it to anything)

>
> >
> > (so if only one is enabled, set it accordingly, else let runtime
> > detection handle it).
> >
> >> #else
> >> /*
> >> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> >> index 6d15a398d389..fd452e68cd90 100644
> >> --- a/arch/mips/kernel/cpu-probe.c
> >> +++ b/arch/mips/kernel/cpu-probe.c
> >> @@ -152,6 +152,13 @@ static inline void check_errata(void)
> >>        struct cpuinfo_mips *c = &current_cpu_data;
> >>
> >>        switch (current_cpu_type()) {
> >> +       case CPU_4KC:
> >> +               if ((c->processor_id & PRID_REV_MASK) < PRID_REV_4KC_V1_0) {
> >> +                       c->options &= ~MIPS_CPU_LLSC;
> >> +                       if (!IS_ENABLED(CONFIG_WAR_4K_LLSC))
> >> +                               pr_err("CPU have LLSC errata, please enable CONFIG_WAR_4K_LLSC");
> >> +               }
> >
> > And then you don't need this error message at all, since then
> > cpu_has_llsc is 0 or follows MIPS_CPU_LLSC, unless you disabled
> > support for the relevant SoC, and then you'll have bigger problems
> > anyway.
>
> The problem is as per MIPS the affected IP core was shipped to multiple customers
> This error message can cover other SoCs that potentially using this core.

AFAICT the core issue is if the platform hardcodes cpu_has_llsc to 1.

So the error/warning this should be then something like this

 if ((c->processor_id & PRID_REV_MASK) < PRID_REV_4KC_V1_0) {
   c->options &= ~MIPS_CPU_LLSC;
   if (cpu_has_llsc) { // <- should now be false, unless the platform
defines it as 1
      pr_err("CPU has LLSC erratum, but cpu_has_llsc is force enabled!\n");
   }

because clearing MIPS_CPU_LLSC does nothing if cpu_has_llsc is
#defined as 1, regardless if it selected WAR_4K_LLSC or not.

(also your error print is missing a \n at the end)

Regards,
Jonas