Le 22/12/2023 à 09:48, Matthias Schiffer a écrit : > [Vous ne recevez pas souvent de courriers de > matthias.schif...@ew.tq-group.com. Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > On Thu, 2023-12-21 at 13:57 +0000, Christophe Leroy wrote: >> >> >> Le 21/12/2023 à 13:45, Matthias Schiffer a écrit : >>> MMU_FTR_USE_HIGH_BATS is set for G2-based cores (G2_LE, e300cX), but the >>> high BATs need to be enabled in HID2 to work. Add register definitions >>> and introduce a G2 variant of __setup_cpu_603. >> >> Well spotted. >> >> I have a mpc8321, hence e300c2. I never had the problem you had. >> >> But ... looks like U-boot configuration has CONFIG_HID2_HBE so that's >> set by U-boot indeed, that's the reason why I never had that problem. > > I'll extend the commit message to mention that U-Boot setting in v2. > > >> >>> >>> This fixes boot on CPUs like the MPC5200B with STRICT_KERNEL_RWX enabled. >>> >>> Fixes: e4d6654ebe6e ("powerpc/mm/32s: rework mmu_mapin_ram()") >>> Signed-off-by: Matthias Schiffer <matthias.schif...@ew.tq-group.com> >>> --- >>> arch/powerpc/include/asm/cpu_setup.h | 1 + >>> arch/powerpc/include/asm/reg.h | 2 ++ >>> arch/powerpc/kernel/cpu_setup_6xx.S | 25 ++++++++++++++++++++++- >>> arch/powerpc/kernel/cpu_specs_book3s_32.h | 10 ++++----- >>> 4 files changed, 32 insertions(+), 6 deletions(-) >>> >>> >>> I have only tested this on the MPC5200B (G2_LE), but according to the >>> e300 manual, e300cX cores should behave the same. >>> >>> The Fixes tag is the best I could come up with - I believe that the >>> underlying issue of setting USE_HIGH_BATS without actually enabling them >>> is as old as Linux's PowerPC implementation, but the specific code >>> causing the boot failure was added in the mentioned commit. >> >> Agreed, before that only BATs 0 to 3 were used anyway. >> There was also BAT 4 used by platforms/embedded6xx/wii.c , but that's >> probably not a G2 ? >> >>> >>> Another issue I found in the code is that >>> arch/powerpc/platforms/52xx/lite5200_sleep.S uses the SPRN_HID2 definition >>> which does not refer to HID2 on the 5200... but that will be for someone >>> else to fix, if there is still anyone left using that platform. >> >> Maybe file an issue for it at https://github.com/linuxppc/issues/issues >> if you don't plan to fix it ? >> >> By the way, it looks like the SPRN_HID2 definition we have is very >> specific to the IBM 750. >> > > IBM 750GX even - googling for IBM 750 came up with several other cores that > either don't have HID2, > or have it at a different SPR. > > >> MPC 750 has SPRN_HID2 as 1011 == 0x3f3 like others. > >>> >>> >>> diff --git a/arch/powerpc/include/asm/cpu_setup.h >>> b/arch/powerpc/include/asm/cpu_setup.h >>> index 30e2fe3895024..68d804e74d221 100644 >>> --- a/arch/powerpc/include/asm/cpu_setup.h >>> +++ b/arch/powerpc/include/asm/cpu_setup.h >>> @@ -35,6 +35,7 @@ void __setup_cpu_750fx(unsigned long offset, struct >>> cpu_spec *spec); >>> void __setup_cpu_7400(unsigned long offset, struct cpu_spec *spec); >>> void __setup_cpu_7410(unsigned long offset, struct cpu_spec *spec); >>> void __setup_cpu_745x(unsigned long offset, struct cpu_spec *spec); >>> +void __setup_cpu_g2(unsigned long offset, struct cpu_spec *spec); >>> >>> void __setup_cpu_ppc970(unsigned long offset, struct cpu_spec *spec); >>> void __setup_cpu_ppc970MP(unsigned long offset, struct cpu_spec *spec); >>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h >>> index 4ae4ab9090a2d..f5641fcd1da85 100644 >>> --- a/arch/powerpc/include/asm/reg.h >>> +++ b/arch/powerpc/include/asm/reg.h >>> @@ -617,6 +617,8 @@ >>> #endif >>> #define SPRN_HID2 0x3F8 /* Hardware Implementation >>> Register 2 */ >> >> Should that HID2 be renamed to SPRN_HID2_750 to avoid confusion ? > > Makes sense (should the suffix be "750GX"?). I can also add a FIXME comment > to lite5200_sleep.S as > part of the rename. > > >> >>> #define SPRN_HID2_GEKKO 0x398 /* Gekko HID2 Register */ >>> +#define SPRN_HID2_G2 0x3F3 /* G2 HID2 Register */ >>> +#define HID2_HBE_G2 (1<<18) /* High BAT Enable (G2) */ >>> #define SPRN_IABR 0x3F2 /* Instruction Address Breakpoint >>> Register */ >>> #define SPRN_IABR2 0x3FA /* 83xx */ >>> #define SPRN_IBCR 0x135 /* 83xx Insn Breakpoint Control >>> Reg */ >>> diff --git a/arch/powerpc/kernel/cpu_setup_6xx.S >>> b/arch/powerpc/kernel/cpu_setup_6xx.S >>> index f29ce3dd6140f..c67d32e04df9c 100644 >>> --- a/arch/powerpc/kernel/cpu_setup_6xx.S >>> +++ b/arch/powerpc/kernel/cpu_setup_6xx.S >>> @@ -81,6 +81,20 @@ _GLOBAL(__setup_cpu_745x) >>> bl setup_745x_specifics >>> mtlr r5 >>> blr >>> +_GLOBAL(__setup_cpu_g2) >>> + mflr r5 >>> +BEGIN_MMU_FTR_SECTION >>> + li r10,0 >>> + mtspr SPRN_SPRG_603_LRU,r10 /* init SW LRU tracking */ >>> +END_MMU_FTR_SECTION_IFSET(MMU_FTR_NEED_DTLB_SW_LRU) >> >> MMU_FTR_NEED_DTLB_SW_LRU is dedicated to e300 core. You should also >> remove it from __setup_cpu_603 > > Will do, thanks.
Thinking about it once more, can we do even more simple ? Why do we need that __setup_cpu_g2() at all ? You could just add the following into __set_cpu_603() diff --git a/arch/powerpc/kernel/cpu_setup_6xx.S b/arch/powerpc/kernel/cpu_setup_6xx.S index c67d32e04df9..7b41e3884866 100644 --- a/arch/powerpc/kernel/cpu_setup_6xx.S +++ b/arch/powerpc/kernel/cpu_setup_6xx.S @@ -21,6 +21,11 @@ BEGIN_MMU_FTR_SECTION li r10,0 mtspr SPRN_SPRG_603_LRU,r10 /* init SW LRU tracking */ END_MMU_FTR_SECTION_IFSET(MMU_FTR_NEED_DTLB_SW_LRU) +BEGIN_MMU_FTR_SECTION + mfspr r11,SPRN_HID2_G2 + oris r11,r11,HID2_HBE_G2@h + mtspr SPRN_HID2_G2,r11 +END_MMU_FTR_SECTION_IFSET(MMU_FTR_USE_HIGH_BATS) BEGIN_FTR_SECTION bl __init_fpu_registers --- By the way, as your register is named SPRN_HID2_G2, the bit would better be named HID2_G2_HBE instead of HID2_HBE_G2 I think. Christophe > >> >>> + >>> +BEGIN_FTR_SECTION >>> + bl __init_fpu_registers >>> +END_FTR_SECTION_IFCLR(CPU_FTR_FPU_UNAVAILABLE) >>> + bl setup_common_caches >>> + bl setup_g2_hid2 >>> + mtlr r5 >>> + blr >>> >>> /* Enable caches for 603's, 604, 750 & 7400 */ >>> SYM_FUNC_START_LOCAL(setup_common_caches) >>> @@ -115,6 +129,16 @@ SYM_FUNC_START_LOCAL(setup_604_hid0) >>> blr >>> SYM_FUNC_END(setup_604_hid0) >>> >>> +/* Enable high BATs for G2 (G2_LE, e300cX) */ >>> +SYM_FUNC_START_LOCAL(setup_g2_hid2) >>> + mfspr r11,SPRN_HID2_G2 >>> + oris r11,r11,HID2_HBE_G2@h >>> + mtspr SPRN_HID2_G2,r11 >>> + sync >>> + isync >>> + blr >>> +SYM_FUNC_END(setup_g2_hid2) >>> + >>> /* 7400 <= rev 2.7 and 7410 rev = 1.0 suffer from some >>> * erratas we work around here. >>> * Moto MPC710CE.pdf describes them, those are errata >>> @@ -495,4 +519,3 @@ _GLOBAL(__restore_cpu_setup) >>> mtcr r7 >>> blr >>> _ASM_NOKPROBE_SYMBOL(__restore_cpu_setup) >>> - >>> diff --git a/arch/powerpc/kernel/cpu_specs_book3s_32.h >>> b/arch/powerpc/kernel/cpu_specs_book3s_32.h >>> index 3714634d194a1..83f054fcf837c 100644 >>> --- a/arch/powerpc/kernel/cpu_specs_book3s_32.h >>> +++ b/arch/powerpc/kernel/cpu_specs_book3s_32.h >>> @@ -69,7 +69,7 @@ static struct cpu_spec cpu_specs[] __initdata = { >>> .mmu_features = MMU_FTR_USE_HIGH_BATS, >>> .icache_bsize = 32, >>> .dcache_bsize = 32, >>> - .cpu_setup = __setup_cpu_603, >>> + .cpu_setup = __setup_cpu_g2, >>> .machine_check = machine_check_generic, >>> .platform = "ppc603", >>> }, >>> @@ -83,7 +83,7 @@ static struct cpu_spec cpu_specs[] __initdata = { >>> .mmu_features = MMU_FTR_USE_HIGH_BATS, >>> .icache_bsize = 32, >>> .dcache_bsize = 32, >>> - .cpu_setup = __setup_cpu_603, >>> + .cpu_setup = __setup_cpu_g2, >>> .machine_check = machine_check_83xx, >>> .platform = "ppc603", >>> }, >>> @@ -96,7 +96,7 @@ static struct cpu_spec cpu_specs[] __initdata = { >>> .mmu_features = MMU_FTR_USE_HIGH_BATS | >>> MMU_FTR_NEED_DTLB_SW_LRU, >>> .icache_bsize = 32, >>> .dcache_bsize = 32, >>> - .cpu_setup = __setup_cpu_603, >>> + .cpu_setup = __setup_cpu_g2, >>> .machine_check = machine_check_83xx, >>> .platform = "ppc603", >>> }, >>> @@ -109,7 +109,7 @@ static struct cpu_spec cpu_specs[] __initdata = { >>> .mmu_features = MMU_FTR_USE_HIGH_BATS | >>> MMU_FTR_NEED_DTLB_SW_LRU, >>> .icache_bsize = 32, >>> .dcache_bsize = 32, >>> - .cpu_setup = __setup_cpu_603, >>> + .cpu_setup = __setup_cpu_g2, >>> .machine_check = machine_check_83xx, >>> .num_pmcs = 4, >>> .platform = "ppc603", >>> @@ -123,7 +123,7 @@ static struct cpu_spec cpu_specs[] __initdata = { >>> .mmu_features = MMU_FTR_USE_HIGH_BATS | >>> MMU_FTR_NEED_DTLB_SW_LRU, >>> .icache_bsize = 32, >>> .dcache_bsize = 32, >>> - .cpu_setup = __setup_cpu_603, >>> + .cpu_setup = __setup_cpu_g2, >>> .machine_check = machine_check_83xx, >>> .num_pmcs = 4, >>> .platform = "ppc603", >>> -- >>> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany >>> Amtsgericht München, HRB 105018 >>> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider >>> https://www.tq-group.com/ > > -- > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany > Amtsgericht München, HRB 105018 > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider > https://www.tq-group.com/