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

[Jiaxun Yang]

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

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