| Message ID | 20220124171740.3285854-1-andre.przywara@arm.com |
|---|---|
| State | Accepted |
| Commit | 68f08966b0d07b5d9cd1abc8e14809a56e08e814 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | armv8: start.S: remove CONFIG_SYS_RESET_SCTRL code | expand |
Am 2022-01-24 18:17, schrieb Andre Przywara: > There is some code that tries to "reset" the SCTLR_ELx register early > in > the boot process. The idea seems to be to guarantee some sane settings > that U-Boot actually relies on, for instance running in little-endian > mode, with the MMU off initially. > However the current code has multiple problems: > - For a start, no platform or config defines the symbol that would > enable that code. > - The code itself really only works if the bits that it tries to clear > are already cleared: > - If we run in big-endian mode initially, any previous loads would > have > been wrong already. That applies to the (optional) relocation code, > but more prominently to the mask that it uses to clear those bits: > "ldr x1, =0xfdfffffa" looks innocent, but actually involves a > memory > access to the literal pool, using the current endianess. > - If we run with the MMU enabled, we are probably doomed already. We > *could* hope that we are running with an identity mapping, but > would > need to do some cache maintenance to avoid losing dirty cache > lines. > - The idea of doing a read-modify-write of SCTLR is somewhat > questionable to begin with, because as the owner of the current > exception level we should initialise all bits of this register with a > certain fixed value. > - The code is unnecessarily complicated, and the function name is > misspelled. > > While those problems *could* admittedly be fixed, the point that is > does > not seem to be used at all at the moment tells me we should just remove > this code, and be it to not give a bad example. > > If people care, I could introduce some proper SCTLR initialisation > code. > We are about to work this out for the boot-wrapper[1] as we speak, but > apparently we got away without doing this in U-Boot ever since, so it > might not be worth the potential trouble. > > [1] > https://lore.kernel.org/linux-arm-kernel/20220114105653.3003399-7-mark.rutland@arm.com/ > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Maybe its used in the cavium u-boot vendor tree; but in mainline it seems like it was always dead code. I'm fine with removing it. -michael
On Mon, Jan 24, 2022 at 05:17:40PM +0000, Andre Przywara wrote: > There is some code that tries to "reset" the SCTLR_ELx register early in > the boot process. The idea seems to be to guarantee some sane settings > that U-Boot actually relies on, for instance running in little-endian > mode, with the MMU off initially. > However the current code has multiple problems: > - For a start, no platform or config defines the symbol that would > enable that code. > - The code itself really only works if the bits that it tries to clear > are already cleared: > - If we run in big-endian mode initially, any previous loads would have > been wrong already. That applies to the (optional) relocation code, > but more prominently to the mask that it uses to clear those bits: > "ldr x1, =0xfdfffffa" looks innocent, but actually involves a memory > access to the literal pool, using the current endianess. > - If we run with the MMU enabled, we are probably doomed already. We > *could* hope that we are running with an identity mapping, but would > need to do some cache maintenance to avoid losing dirty cache lines. > - The idea of doing a read-modify-write of SCTLR is somewhat > questionable to begin with, because as the owner of the current > exception level we should initialise all bits of this register with a > certain fixed value. > - The code is unnecessarily complicated, and the function name is > misspelled. > > While those problems *could* admittedly be fixed, the point that is does > not seem to be used at all at the moment tells me we should just remove > this code, and be it to not give a bad example. > > If people care, I could introduce some proper SCTLR initialisation code. > We are about to work this out for the boot-wrapper[1] as we speak, but > apparently we got away without doing this in U-Boot ever since, so it > might not be worth the potential trouble. > > [1] https://lore.kernel.org/linux-arm-kernel/20220114105653.3003399-7-mark.rutland@arm.com/ > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Applied to u-boot/master, thanks!
diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S index b3eef705a5..91b00a46cc 100644 --- a/arch/arm/cpu/armv8/start.S +++ b/arch/arm/cpu/armv8/start.S @@ -104,10 +104,6 @@ pie_skip_reloc: pie_fixup_done: #endif -#ifdef CONFIG_SYS_RESET_SCTRL - bl reset_sctrl -#endif - #if defined(CONFIG_ARMV8_SPL_EXCEPTION_VECTORS) || !defined(CONFIG_SPL_BUILD) .macro set_vbar, regname, reg msr \regname, \reg @@ -195,39 +191,6 @@ slave_cpu: master_cpu: bl _main -#ifdef CONFIG_SYS_RESET_SCTRL -reset_sctrl: - switch_el x1, 3f, 2f, 1f -3: - mrs x0, sctlr_el3 - b 0f -2: - mrs x0, sctlr_el2 - b 0f -1: - mrs x0, sctlr_el1 - -0: - ldr x1, =0xfdfffffa - and x0, x0, x1 - - switch_el x1, 6f, 5f, 4f -6: - msr sctlr_el3, x0 - b 7f -5: - msr sctlr_el2, x0 - b 7f -4: - msr sctlr_el1, x0 - -7: - dsb sy - isb - b __asm_invalidate_tlb_all - ret -#endif - /*-----------------------------------------------------------------------*/ WEAK(apply_core_errata)
There is some code that tries to "reset" the SCTLR_ELx register early in the boot process. The idea seems to be to guarantee some sane settings that U-Boot actually relies on, for instance running in little-endian mode, with the MMU off initially. However the current code has multiple problems: - For a start, no platform or config defines the symbol that would enable that code. - The code itself really only works if the bits that it tries to clear are already cleared: - If we run in big-endian mode initially, any previous loads would have been wrong already. That applies to the (optional) relocation code, but more prominently to the mask that it uses to clear those bits: "ldr x1, =0xfdfffffa" looks innocent, but actually involves a memory access to the literal pool, using the current endianess. - If we run with the MMU enabled, we are probably doomed already. We *could* hope that we are running with an identity mapping, but would need to do some cache maintenance to avoid losing dirty cache lines. - The idea of doing a read-modify-write of SCTLR is somewhat questionable to begin with, because as the owner of the current exception level we should initialise all bits of this register with a certain fixed value. - The code is unnecessarily complicated, and the function name is misspelled. While those problems *could* admittedly be fixed, the point that is does not seem to be used at all at the moment tells me we should just remove this code, and be it to not give a bad example. If people care, I could introduce some proper SCTLR initialisation code. We are about to work this out for the boot-wrapper[1] as we speak, but apparently we got away without doing this in U-Boot ever since, so it might not be worth the potential trouble. [1] https://lore.kernel.org/linux-arm-kernel/20220114105653.3003399-7-mark.rutland@arm.com/ Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arch/arm/cpu/armv8/start.S | 37 ------------------------------------- 1 file changed, 37 deletions(-)