[1/3] MIPS: Introduce WAR_4KC_LLSC config option

  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@flygoat.com>
---
 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(-)
  

On Fri, 19 May 2023, Jiaxun Yang wrote:

> 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");

 Given the circumstances I think this should be `panic'.  You don't want 
to continue with a system that can randomly lock up.

 Also "CPU has LLSC erratum, [...]" as both are singular.

  Maciej
  

> 2023年5月19日 21:23，Maciej W. Rozycki <macro@orcam.me.uk> 写道：
> 
> On Fri, 19 May 2023, Jiaxun Yang wrote:
> 
>> 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");
> 
> Given the circumstances I think this should be `panic'.  You don't want 
> to continue with a system that can randomly lock up.

I just checked how other architectures handle such situation, it seems like
TAINT_CPU_OUT_OF_SPEC is a better option.

Panic in cpu_probe can be frustrating for users as it is earlier than initialisation
of consoles so the panic message can never be read by users.

> 
> Also "CPU has LLSC erratum, [...]" as both are singular.

Thanks for the tip :-)
- Jiaxun

> 
>  Maciej
  

On Fri, 19 May 2023, Jiaxun Yang wrote:

> > Given the circumstances I think this should be `panic'.  You don't want 
> > to continue with a system that can randomly lock up.
> 
> I just checked how other architectures handle such situation, it seems like
> TAINT_CPU_OUT_OF_SPEC is a better option.

 That can be easily missed, just as a random message in the kernel log, 
even at a high priority, and LOGLEVEL_ERR is not particularly high even.  
For a system configuration that qualifies as not usable for any practical 
purpose I find this approach leaving something to desire.

> Panic in cpu_probe can be frustrating for users as it is earlier than initialisation
> of consoles so the panic message can never be read by users.

 Is there no early console support for the systems affected?

  Maciej
  

Hi,

On Fri, 19 May 2023 at 18:49, Jiaxun Yang <jiaxun.yang@flygoat.com> 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@flygoat.com>
> ---
>  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"?

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.

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

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

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

Regards,
Jonas
  

> 2023年5月22日 19:40，Jonas Gorski <jonas.gorski@gmail.com> 写道：
> 
> Hi,
> 
> On Fri, 19 May 2023 at 18:49, Jiaxun Yang <jiaxun.yang@flygoat.com> 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@flygoat.com>
>> ---
>> 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.

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.

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

Thanks
- Jiaxun

> 
> Regards,
> Jonas
  

> 2023年5月19日 23:51，Maciej W. Rozycki <macro@orcam.me.uk> 写道：
> 
> On Fri, 19 May 2023, Jiaxun Yang wrote:
> 
>>> Given the circumstances I think this should be `panic'.  You don't want 
>>> to continue with a system that can randomly lock up.
>> 
>> I just checked how other architectures handle such situation, it seems like
>> TAINT_CPU_OUT_OF_SPEC is a better option.
> 
> That can be easily missed, just as a random message in the kernel log, 
> even at a high priority, and LOGLEVEL_ERR is not particularly high even.  
> For a system configuration that qualifies as not usable for any practical 
> purpose I find this approach leaving something to desire.

Just found another reason that not to panic in such situation when I’m playing
around QEMU.

Emulation software like QEMU, mipssim, OVPSim, IASIM tends use base PRID
of a core. It can be more than annoying for people to find kernel suddenly won’t
boot on their simulator.

> 
>> Panic in cpu_probe can be frustrating for users as it is earlier than initialisation
>> of consoles so the panic message can never be read by users.
> 
> Is there no early console support for the systems affected?

For routers it’s very common that serial port available to user is not the same one
as we hooked for earlyprintk.

For QEMU users potentially using graphic interface, initialisation of DRM/FBDEV
comes very later.

Thanks
- Jiaxun

> 
>  Maciej
  

On Mon, 22 May 2023 at 22:38, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
>
>
> > 2023年5月22日 19:40，Jonas Gorski <jonas.gorski@gmail.com> 写道：
> >
> > Hi,
> >
> > On Fri, 19 May 2023 at 18:49, Jiaxun Yang <jiaxun.yang@flygoat.com> 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@flygoat.com>
> >> ---
> >> 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
  

> 2023年5月22日 23:03，Jonas Gorski <jonas.gorski@gmail.com> 写道：
> 
> On Mon, 22 May 2023 at 22:38, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>> 
>> 
>> 
>>> 2023年5月22日 19:40，Jonas Gorski <jonas.gorski@gmail.com> 写道：
>>> 
>>> Hi,
>>> 
>>> On Fri, 19 May 2023 at 18:49, Jiaxun Yang <jiaxun.yang@flygoat.com> 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@flygoat.com>
>>>> ---
>>>> 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.

Thanks for pointing out the missing piece, I thought ATH25 can only be
built for a single SoC :-)

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

[...]

> 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)

Ah, thanks.
I’m planning to replace this pr_err with WARN_TAINT_ONCE.

Thanks
- Jiaxun

> 
> Regards,
> Jonas