reset on mpc83xx
Hi,
I'm wondering why the sysreset_mpc83xx driver (as well as the similar code in arch/powerpc/cpu/mpc83xx/cpu.c) removes the MSR_IR and MSR_DR bits from MSR.
On my mpc8309, that seems to simply make the board hang right after writing the new MSR value - i.e., it never gets to write the magic value 0x52535445 to RPR and of course not to then actually do the reset. Doing
- msr &= ~(MSR_EE | MSR_IR | MSR_DR); + msr &= ~(MSR_EE);
(whether using the sysreset driver or the cpu.c code) makes the board reset as expected.
The linux kernel's mpc83xx_restart() doesn't seem to remove those bits, and rebooting from linux works as expected - it does local_irq_disable() which amounts to clearing the EE bit.
But since the arch/powerpc/cpu/mpc83xx/cpu.c code in U-Boot has been there forever, I assume I'm missing something fundamental.
[The board happens to have an external watchdog circuit that performs a power cycle after a while, but there are cases, e.g. bootstrapping, where that circuitry is disabled, and moreover, we'd like to be able to make use of the reset status register to distinguish a reset from a cold boot].
Rasmus
Le 30/04/2020 à 16:44, Rasmus Villemoes a écrit :
Hi,
I'm wondering why the sysreset_mpc83xx driver (as well as the similar code in arch/powerpc/cpu/mpc83xx/cpu.c) removes the MSR_IR and MSR_DR bits from MSR.
On my mpc8309, that seems to simply make the board hang right after writing the new MSR value - i.e., it never gets to write the magic value 0x52535445 to RPR and of course not to then actually do the reset. Doing
msr &= ~(MSR_EE | MSR_IR | MSR_DR);
msr &= ~(MSR_EE);(whether using the sysreset driver or the cpu.c code) makes the board reset as expected.
The linux kernel's mpc83xx_restart() doesn't seem to remove those bits, and rebooting from linux works as expected - it does local_irq_disable() which amounts to clearing the EE bit.
But since the arch/powerpc/cpu/mpc83xx/cpu.c code in U-Boot has been there forever, I assume I'm missing something fundamental.
[The board happens to have an external watchdog circuit that performs a power cycle after a while, but there are cases, e.g. bootstrapping, where that circuitry is disabled, and moreover, we'd like to be able to make use of the reset status register to distinguish a reset from a cold boot].
On my mpc8321 it works.
However I agree it looks odd.
I think it works by chance because we have a 1:1 MMU mapping, but if you have a difference mapping I can't see how you can disable Instruction MMU without going through a write into SRR0/SRR1 and an rfi.
Christophe
On 30/04/2020 17.03, Christophe Leroy wrote:
Le 30/04/2020 à 16:44, Rasmus Villemoes a écrit :
Hi,
I'm wondering why the sysreset_mpc83xx driver (as well as the similar code in arch/powerpc/cpu/mpc83xx/cpu.c) removes the MSR_IR and MSR_DR bits from MSR.
[snip]
But since the arch/powerpc/cpu/mpc83xx/cpu.c code in U-Boot has been there forever, I assume I'm missing something fundamental.
On my mpc8321 it works.
However I agree it looks odd.
I think it works by chance because we have a 1:1 MMU mapping, but if you have a difference mapping I can't see how you can disable Instruction MMU without going through a write into SRR0/SRR1 and an rfi.
OK, thanks. Can you test if it still works if you change the code to not clear the MSR_IR and MSR_DR bits? (I expect it will, since I assume you'd have noticed if reboot from linux didn't work).
I'm actually wondering what the core does after the MSR_IR bit gets cleared. If address translation is disabled, what instruction does it then do immediately after the mtmsr? I mean, the pc has some value, but how does that get interpreted without MSR_IR set?
And yes, the (unused, it seems) disable_addr_trans routine in start.S does seem to be somewhat more complicated than a simple RMW of MSR, suggesting that if disabling address translation was really needed before reset, it should be done using that rather than by just clearing the bits directly.
Rasmus
Le 30/04/2020 à 21:06, Rasmus Villemoes a écrit :
On 30/04/2020 17.03, Christophe Leroy wrote:
Le 30/04/2020 à 16:44, Rasmus Villemoes a écrit :
Hi,
I'm wondering why the sysreset_mpc83xx driver (as well as the similar code in arch/powerpc/cpu/mpc83xx/cpu.c) removes the MSR_IR and MSR_DR bits from MSR.
[snip]
But since the arch/powerpc/cpu/mpc83xx/cpu.c code in U-Boot has been there forever, I assume I'm missing something fundamental.
On my mpc8321 it works.
However I agree it looks odd.
I think it works by chance because we have a 1:1 MMU mapping, but if you have a difference mapping I can't see how you can disable Instruction MMU without going through a write into SRR0/SRR1 and an rfi.
OK, thanks. Can you test if it still works if you change the code to not clear the MSR_IR and MSR_DR bits? (I expect it will, since I assume you'd have noticed if reboot from linux didn't work).
Due to the COVID-19 lockdown I'm working from home and don't have access to HW tools to recover by boards in case I flash a bad Uboot, so I won't do that for now.
However I confirm reboot works properly in Linux.
I'm actually wondering what the core does after the MSR_IR bit gets cleared. If address translation is disabled, what instruction does it then do immediately after the mtmsr? I mean, the pc has some value, but how does that get interpreted without MSR_IR set?
After the mtmsr it probably continues with a few instructions that are already in the pipeline, until the isync. Then after the isync it will try to fetch from the physical address having the value of the pc. If it doesn't have an existing address, you'll get a machine check.
And yes, the (unused, it seems) disable_addr_trans routine in start.S does seem to be somewhat more complicated than a simple RMW of MSR, suggesting that if disabling address translation was really needed before reset, it should be done using that rather than by just clearing the bits directly.
Yes disable_addr_trans() is exactly what has to be done for disabling instruction address translation.
For disabling only data address translation can be done with an RMW of MSR + isync.
Christophe
participants (2)
-
Christophe Leroy -
Rasmus Villemoes