| Message ID | 20210403061912.1012509-1-ilya.lipnitskiy@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: add support for buggy MT7621S core detection | expand |
On Fri, 2 Apr 2021, Ilya Lipnitskiy wrote: > diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h > index d72dc6e1cf3c..d32f0c4e61f7 100644 > --- a/arch/mips/include/asm/bugs.h > +++ b/arch/mips/include/asm/bugs.h > @@ -50,4 +51,21 @@ static inline int r4k_daddiu_bug(void) > return daddiu_bug != 0; > } > > +static inline void cm_gcr_pcores_bug(unsigned int *ncores) > +{ > + struct cpulaunch *launch; > + > + if (!IS_ENABLED(CONFIG_SOC_MT7621) || !ncores) > + return; > + > + /* > + * Ralink MT7621S SoC is single core, but GCR_CONFIG always reports 2 cores. Overlong line. > diff --git a/arch/mips/kernel/smp-cps.c b/arch/mips/kernel/smp-cps.c > index bcd6a944b839..e1e9c11e8a7c 100644 > --- a/arch/mips/kernel/smp-cps.c > +++ b/arch/mips/kernel/smp-cps.c > @@ -60,6 +61,7 @@ static void __init cps_smp_setup(void) > pr_cont("{"); > > ncores = mips_cps_numcores(cl); > + cm_gcr_pcores_bug(&ncores); > for (c = 0; c < ncores; c++) { > core_vpes = core_vpe_count(cl, c); > > @@ -170,6 +172,7 @@ static void __init cps_prepare_cpus(unsigned int max_cpus) > > /* Allocate core boot configuration structs */ > ncores = mips_cps_numcores(0); > + cm_gcr_pcores_bug(&ncores); Why called at each `mips_cps_numcores' call site rather than within the callee? Also weird inefficient interface: why isn't `ncores' passed by value for a new value to be returned? Maciej
On Mon, Apr 5, 2021 at 6:22 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > On Fri, 2 Apr 2021, Ilya Lipnitskiy wrote: > > > diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h > > index d72dc6e1cf3c..d32f0c4e61f7 100644 > > --- a/arch/mips/include/asm/bugs.h > > +++ b/arch/mips/include/asm/bugs.h > > @@ -50,4 +51,21 @@ static inline int r4k_daddiu_bug(void) > > return daddiu_bug != 0; > > } > > > > +static inline void cm_gcr_pcores_bug(unsigned int *ncores) > > +{ > > + struct cpulaunch *launch; > > + > > + if (!IS_ENABLED(CONFIG_SOC_MT7621) || !ncores) > > + return; > > + > > + /* > > + * Ralink MT7621S SoC is single core, but GCR_CONFIG always reports 2 cores. > > Overlong line. > > > diff --git a/arch/mips/kernel/smp-cps.c b/arch/mips/kernel/smp-cps.c > > index bcd6a944b839..e1e9c11e8a7c 100644 > > --- a/arch/mips/kernel/smp-cps.c > > +++ b/arch/mips/kernel/smp-cps.c > > @@ -60,6 +61,7 @@ static void __init cps_smp_setup(void) > > pr_cont("{"); > > > > ncores = mips_cps_numcores(cl); > > + cm_gcr_pcores_bug(&ncores); > > for (c = 0; c < ncores; c++) { > > core_vpes = core_vpe_count(cl, c); > > > > @@ -170,6 +172,7 @@ static void __init cps_prepare_cpus(unsigned int max_cpus) > > > > /* Allocate core boot configuration structs */ > > ncores = mips_cps_numcores(0); > > + cm_gcr_pcores_bug(&ncores); > > Why called at each `mips_cps_numcores' call site rather than within the > callee? Also weird inefficient interface: why isn't `ncores' passed by > value for a new value to be returned? Thanks for the comments. Including asm/bugs.h in asm/mips-cps.h led to some circular dependencies when I tried it, but I will try again based on your feedback - indeed it would be much cleaner to have this logic in mips_cps_numcores. The only wrinkle is that mips_cps_numcores may return a different value on MT7621 after the cores have started due to CPULAUNCH flags changing, but nobody calls mips_cps_numcores later anyway, so it's a moot point today. I will clean up the change and resend. Ilya
On Mon, 5 Apr 2021, Ilya Lipnitskiy wrote: > Thanks for the comments. Including asm/bugs.h in asm/mips-cps.h led to > some circular dependencies when I tried it, but I will try again based > on your feedback - indeed it would be much cleaner to have this logic > in mips_cps_numcores. The only wrinkle is that mips_cps_numcores may > return a different value on MT7621 after the cores have started due to > CPULAUNCH flags changing, but nobody calls mips_cps_numcores later > anyway, so it's a moot point today. I will clean up the change and > resend. Hmm, I don't know this system, but by the look of the code it queries launch[2], which I gather refers to the VPE #0 of an inexistent core #1, so why would the structure change given that there is no corresponding silicon? Maciej
On Wed, Apr 7, 2021 at 6:49 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > On Mon, 5 Apr 2021, Ilya Lipnitskiy wrote: > > > Thanks for the comments. Including asm/bugs.h in asm/mips-cps.h led to > > some circular dependencies when I tried it, but I will try again based > > on your feedback - indeed it would be much cleaner to have this logic > > in mips_cps_numcores. The only wrinkle is that mips_cps_numcores may > > return a different value on MT7621 after the cores have started due to > > CPULAUNCH flags changing, but nobody calls mips_cps_numcores later > > anyway, so it's a moot point today. I will clean up the change and > > resend. > > Hmm, I don't know this system, but by the look of the code it queries > launch[2], which I gather refers to the VPE #0 of an inexistent core #1, > so why would the structure change given that there is no corresponding > silicon? The structure would change only on the dual-core flavor of MT7621, the single-core would always report 1 core, but on the dual-core, if somebody were to call mips_cps_numcores after the second core had already started, mips_cps_numcores would return 1 instead of 2. I think it may be feasible to fix it by checking other flags, but there is no use case for that today, so I'd rather keep this hacky logic to a minimum. Ilya
On 8/4/21 1:49 am, Ilya Lipnitskiy wrote: > On Wed, Apr 7, 2021 at 6:49 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote: >> On Mon, 5 Apr 2021, Ilya Lipnitskiy wrote: >> >>> Thanks for the comments. Including asm/bugs.h in asm/mips-cps.h led to >>> some circular dependencies when I tried it, but I will try again based >>> on your feedback - indeed it would be much cleaner to have this logic >>> in mips_cps_numcores. The only wrinkle is that mips_cps_numcores may >>> return a different value on MT7621 after the cores have started due to >>> CPULAUNCH flags changing, but nobody calls mips_cps_numcores later >>> anyway, so it's a moot point today. I will clean up the change and >>> resend. >> Hmm, I don't know this system, but by the look of the code it queries >> launch[2], which I gather refers to the VPE #0 of an inexistent core #1, >> so why would the structure change given that there is no corresponding >> silicon? > The structure would change only on the dual-core flavor of MT7621, the > single-core would always report 1 core, but on the dual-core, if > somebody were to call mips_cps_numcores after the second core had > already started, mips_cps_numcores would return 1 instead of 2. I > think it may be feasible to fix it by checking other flags, but there > is no use case for that today, so I'd rather keep this hacky logic to > a minimum. > > Ilya > > Actually, I am currently struggling with a side effect of this approach in the original OpenWrt version of this method, although i think this version will suffer from the same effect. When you kexec the kernel from a previously running kernel, it only detects a single core. I am about to disable it entirely, as i really need to be able to run kexec on a MT7621 platform. I have instrumented the code with some debug to prove it is the case: Boot from u-boot: [ 0.000000] nclusters = 1 [ 0.000000] VPE topology [ 0.000000] cl = 0 [ 0.000000] { [ 0.000000] ncores = 2 [ 0.000000] cpulaunch.pc = 000000ff [ 0.000000] cpulaunch.gp = 0000ff00 [ 0.000000] cpulaunch.sp = 0000ffff [ 0.000000] cpulaunch.a0 = 08000800 [ 0.000000] cpulaunch.flags = 00000020 [ 0.000000] plat_cpu_core_present(0) = true [ 0.000000] core_vpes = 2 [ 0.000000] 2 [ 0.000000] cpulaunch.pc = 000000ff [ 0.000000] cpulaunch.gp = 0000ff00 [ 0.000000] cpulaunch.sp = 0000ffff [ 0.000000] cpulaunch.a0 = 08000800 [ 0.000000] cpulaunch.flags = 00000020 [ 0.000000] plat_cpu_core_present(1) = true [ 0.000000] core_vpes = 2 [ 0.000000] ,2} total 4 Boot from kexec: [ 0.000000] nclusters = 1 [ 0.000000] VPE topology [ 0.000000] cl = 0 [ 0.000000] { [ 0.000000] ncores = 2 [ 0.000000] cpulaunch.pc = 00000000 [ 0.000000] cpulaunch.gp = 00000000 [ 0.000000] cpulaunch.sp = 00000000 [ 0.000000] cpulaunch.a0 = 00000000 [ 0.000000] cpulaunch.flags = 00000000 [ 0.000000] plat_cpu_core_present(0) = true [ 0.000000] core_vpes = 2 [ 0.000000] 2 [ 0.000000] cpulaunch.pc = 00000000 [ 0.000000] cpulaunch.gp = 00000000 [ 0.000000] cpulaunch.sp = 00000000 [ 0.000000] cpulaunch.a0 = 00000000 [ 0.000000] cpulaunch.flags = 00000000} total 2 Steven
diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h index d72dc6e1cf3c..d32f0c4e61f7 100644 --- a/arch/mips/include/asm/bugs.h +++ b/arch/mips/include/asm/bugs.h @@ -16,6 +16,7 @@ #include <asm/cpu.h> #include <asm/cpu-info.h> +#include <asm/mips-boards/launch.h> extern int daddiu_bug; @@ -50,4 +51,21 @@ static inline int r4k_daddiu_bug(void) return daddiu_bug != 0; } +static inline void cm_gcr_pcores_bug(unsigned int *ncores) +{ + struct cpulaunch *launch; + + if (!IS_ENABLED(CONFIG_SOC_MT7621) || !ncores) + return; + + /* + * Ralink MT7621S SoC is single core, but GCR_CONFIG always reports 2 cores. + * Use legacy amon method to detect if the second core is missing. + */ + launch = (struct cpulaunch *)CKSEG0ADDR(CPULAUNCH); + launch += 2; /* MT7621 has 2 VPEs per core */ + if (!(launch->flags & LAUNCH_FREADY)) + *ncores = 1; +} + #endif /* _ASM_BUGS_H */ diff --git a/arch/mips/kernel/smp-cps.c b/arch/mips/kernel/smp-cps.c index bcd6a944b839..e1e9c11e8a7c 100644 --- a/arch/mips/kernel/smp-cps.c +++ b/arch/mips/kernel/smp-cps.c @@ -15,6 +15,7 @@ #include <linux/irq.h> #include <asm/bcache.h> +#include <asm/bugs.h> #include <asm/mips-cps.h> #include <asm/mips_mt.h> #include <asm/mipsregs.h> @@ -60,6 +61,7 @@ static void __init cps_smp_setup(void) pr_cont("{"); ncores = mips_cps_numcores(cl); + cm_gcr_pcores_bug(&ncores); for (c = 0; c < ncores; c++) { core_vpes = core_vpe_count(cl, c); @@ -170,6 +172,7 @@ static void __init cps_prepare_cpus(unsigned int max_cpus) /* Allocate core boot configuration structs */ ncores = mips_cps_numcores(0); + cm_gcr_pcores_bug(&ncores); mips_cps_core_bootcfg = kcalloc(ncores, sizeof(*mips_cps_core_bootcfg), GFP_KERNEL); if (!mips_cps_core_bootcfg) {
Most MT7621 SoCs have 2 cores, which is detected and supported properly by CPS. Unfortunately, MT7621 SoC has a less common S variant with only one core. On MT7621S, GCR_CONFIG still reports 2 cores, which leads to hangs when starting SMP. CPULAUNCH registers can be used in that case to detect the absence of the second core and override the GCR_CONFIG PCORES field. Rework a long-standing OpenWrt patch to override the value of mips_cps_numcores on single-core MT7621 systems. Tested on a dual-core MT7621 device (Ubiquiti ER-X) and a single-core MT7621 device (Netgear R6220). Original 4.14 OpenWrt patch: Link: https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=4cdbc90a376dd0555201c1434a2081e055e9ceb7 Current 5.10 OpenWrt patch: Link: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/ramips/patches-5.10/320-mt7621-core-detect-hack.patch;h=c63f0f4c1ec742e24d8480e80553863744b58f6a;hb=10267e17299806f9885d086147878f6c492cb904 Suggested-by: Felix Fietkau <nbd@nbd.name> Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com> --- arch/mips/include/asm/bugs.h | 18 ++++++++++++++++++ arch/mips/kernel/smp-cps.c | 3 +++ 2 files changed, 21 insertions(+)