| Message ID | 20110823163247.GM2796@pulham.picochip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 23, 2011 at 05:32:47PM +0100, Jamie Iles wrote: > Hi Will, Hi Jamie, > I'm trying to use the cpu_v6_reset that you added in "ARM: proc: add > definition of cpu_reset for ARMv6 and ARMv7 cores", but I've found that > on my 1176 platform, it never gets to the branch to the reset vector. Ok. How are you calling cpu_v6_reset? If you call it via arch_reset from arm_machine_restart then there should be an identity mapping in place, so you need to ensure that the reset code is called via this mapping in your implementation of arch_reset. Unfortunately, the current flat mapping only covers userspace, so it relies on the physical address of the reset code not aliasing with the kernel virtual addresses. I have some (experimental) patches to fix this in my kexec branch: http://www.linux-arm.org/git?p=linux-2.6-wd.git;a=shortlog;h=refs/heads/kexec-mmu-off > Removing the ISB allows the branch instruction to be in the pipeline by > the time the MMU is disabled, but I'm not sure if this is the correct > fix. Having said that, I don't see how this can work with an ISB in > there. With modern CPUs, you can't rely on characteristics of the pipeline to play tricks like this. Instead, you need to ensure that the reset code is executed with a 1:1 mapping. Will
On Tue, Aug 23, 2011 at 06:09:55PM +0100, Will Deacon wrote: > On Tue, Aug 23, 2011 at 05:32:47PM +0100, Jamie Iles wrote: > > Hi Will, > > Hi Jamie, > > > I'm trying to use the cpu_v6_reset that you added in "ARM: proc: add > > definition of cpu_reset for ARMv6 and ARMv7 cores", but I've found that > > on my 1176 platform, it never gets to the branch to the reset vector. > > Ok. How are you calling cpu_v6_reset? If you call it via arch_reset from > arm_machine_restart then there should be an identity mapping in place, so > you need to ensure that the reset code is called via this mapping in your > implementation of arch_reset. Yes, this is being called from the arch_reset hook. > Unfortunately, the current flat mapping only covers userspace, so it relies > on the physical address of the reset code not aliasing with the kernel virtual > addresses. The reset code is in our bootrom (at physical address 0xffff0000). > I have some (experimental) patches to fix this in my kexec branch: > > http://www.linux-arm.org/git?p=linux-2.6-wd.git;a=shortlog;h=refs/heads/kexec-mmu-off > > > Removing the ISB allows the branch instruction to be in the pipeline by > > the time the MMU is disabled, but I'm not sure if this is the correct > > fix. Having said that, I don't see how this can work with an ISB in > > there. > > With modern CPUs, you can't rely on characteristics of the pipeline to play > tricks like this. Instead, you need to ensure that the reset code is > executed with a 1:1 mapping. Hmm, I don't really understand this - the cpu_v6_reset code turns off the MMU, then issues an ISB. So for as long as cpu_v6_reset is executing in the kernel virtual address space I don't see how it can ever fetch the "mov pc, r0" instruction after the ISB without those instructions living in the 1:1 mapping? Jamie
On Tue, Aug 23, 2011 at 06:34:10PM +0100, Jamie Iles wrote: > On Tue, Aug 23, 2011 at 06:09:55PM +0100, Will Deacon wrote: > > Ok. How are you calling cpu_v6_reset? If you call it via arch_reset from > > arm_machine_restart then there should be an identity mapping in place, so > > you need to ensure that the reset code is called via this mapping in your > > implementation of arch_reset. > > Yes, this is being called from the arch_reset hook. Good, good. > > Unfortunately, the current flat mapping only covers userspace, so it relies > > on the physical address of the reset code not aliasing with the kernel virtual > > addresses. > > The reset code is in our bootrom (at physical address 0xffff0000). Sorry, I was referring to cpu_reset rather than the code that it then jumps to (which can be anywhere). > > With modern CPUs, you can't rely on characteristics of the pipeline to play > > tricks like this. Instead, you need to ensure that the reset code is > > executed with a 1:1 mapping. > > Hmm, I don't really understand this - the cpu_v6_reset code turns off > the MMU, then issues an ISB. So for as long as cpu_v6_reset is > executing in the kernel virtual address space I don't see how it can > ever fetch the "mov pc, r0" instruction after the ISB without those > instructions living in the 1:1 mapping? You need to make sure you call cpu_reset by jumping to its *physical* address. If that happens to alias with the virtual address of the kernel, it won't currently work but I have a solution to this in my kexec branch. You can do something like: typedef void (*phys_reset_t)(unsigned long); phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); phys_reset(0xffff0000); The problem with not having an ISB is that you can't guarantee at which point the MMU is no longer active, so you're really walking on a tightrope in that case. Will
On Tue, Aug 23, 2011 at 06:47:36PM +0100, Will Deacon wrote: > You need to make sure you call cpu_reset by jumping to its *physical* > address. If that happens to alias with the virtual address of the kernel, it > won't currently work but I have a solution to this in my kexec branch. > > You can do something like: > > typedef void (*phys_reset_t)(unsigned long); > > phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); > phys_reset(0xffff0000); OK, that makes perfect sense! I'll give it a go on my hardware tomorrow when I'm back in the office. All of the other mainline users of cpu_reset() just call it directly, so I guess they need to do the same thing? Jamie
On Tue, Aug 23, 2011 at 06:56:38PM +0100, Jamie Iles wrote: > On Tue, Aug 23, 2011 at 06:47:36PM +0100, Will Deacon wrote: > > You need to make sure you call cpu_reset by jumping to its *physical* > > address. If that happens to alias with the virtual address of the kernel, it > > won't currently work but I have a solution to this in my kexec branch. > > > > You can do something like: > > > > typedef void (*phys_reset_t)(unsigned long); > > > > phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); > > phys_reset(0xffff0000); > > OK, that makes perfect sense! I'll give it a go on my hardware tomorrow > when I'm back in the office. Great, let me know how you get on! I'll post my kexec series to the list again once I've got on top of my current patchsets. > All of the other mainline users of cpu_reset() just call it directly, so > I guess they need to do the same thing? For v6 and v7, yes. For older ARM cores you could actually code for the pipeline implementation and things were arguably easier that way. Will
On Tue, Aug 23, 2011 at 07:00:59PM +0100, Will Deacon wrote: > On Tue, Aug 23, 2011 at 06:56:38PM +0100, Jamie Iles wrote: > > On Tue, Aug 23, 2011 at 06:47:36PM +0100, Will Deacon wrote: > > > You need to make sure you call cpu_reset by jumping to its *physical* > > > address. If that happens to alias with the virtual address of the kernel, it > > > won't currently work but I have a solution to this in my kexec branch. > > > > > > You can do something like: > > > > > > typedef void (*phys_reset_t)(unsigned long); > > > > > > phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); > > > phys_reset(0xffff0000); > > > > OK, that makes perfect sense! I'll give it a go on my hardware tomorrow > > when I'm back in the office. > > Great, let me know how you get on! I'll post my kexec series to the list > again once I've got on top of my current patchsets. That works a treat, thanks Will! Jamie
diff --git a/arch/arm/mm/proc-v6.S b/arch/arm/mm/proc-v6.S index 219138d..3b6737a 100644 --- a/arch/arm/mm/proc-v6.S +++ b/arch/arm/mm/proc-v6.S @@ -59,8 +59,6 @@ ENTRY(cpu_v6_reset) mrc p15, 0, r1, c1, c0, 0 @ ctrl register bic r1, r1, #0x1 @ ...............m mcr p15, 0, r1, c1, c0, 0 @ disable MMU - mov r1, #0 - mcr p15, 0, r1, c7, c5, 4 @ ISB mov pc, r0 /*