Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230519164753.72065-1-jiaxun.yang@flygoat.com> <20230519164753.72065-2-jiaxun.yang@flygoat.com>
In-Reply-To: <20230519164753.72065-2-jiaxun.yang@flygoat.com>
From:   Jonas Gorski <jonas.gorski@gmail.com>
Date:   Mon, 22 May 2023 20:40:10 +0200
Message-ID: <CAOiHx==iku+duvBnAfu_3AXgNmY9aK+uO+t9Enzdf6qQN5m+iw@mail.gmail.com>
Subject: Re: [PATCH 1/3] MIPS: Introduce WAR_4KC_LLSC config option
To:     Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc:     linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        tsbogend@alpha.franken.de
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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