Re: [PATCH] powerpc/6xx: set High BAT Enable flag on G2 cores

From: Christophe Leroy
Date: Fri Dec 22 2023 - 13:42:06 EST



Le 22/12/2023 à 09:48, Matthias Schiffer a écrit :
> [Vous ne recevez pas souvent de courriers de matthias.schiffer@xxxxxxxxxxxxxxx. 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.schiffer@xxxxxxxxxxxxxxx>
>>> ---
>>> 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/