[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Stephen Warren]

On 11/09/2016 04:43 AM, Alexander Graf wrote:
>
>
> On 07/11/2016 22:26, Stephen Warren wrote:
>> On 11/06/2016 03:24 AM, Alexander Graf wrote:
>>>
>>>
>>> On 05/11/2016 23:01, Stephen Warren wrote:
>>>> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>>>>> The rpi has a pretty simple way of resetting the whole system. All it
>>>>> takes
>>>>> is to poke a few registers at a well defined location in MMIO space.
>>>>>
>>>>> This patch adds support for the EFI loader implementation to allow an
>>>>> OS to
>>>>> reset and power off the system when we're outside of boot time.
>>>>
>>>> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
>>>> complete EFI implementation such as TianoCore.)
>>>>
>>>>> diff --git a/arch/arm/mach-bcm283x/reset.c
>>>>> b/arch/arm/mach-bcm283x/reset.c
>>>>
>>>>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>>>>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>> +
>>>>> +void __efi_runtime reset_cpu(ulong addr)
>>>>>  {
>>>>> -    struct bcm2835_wdog_regs *regs =
>>>>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>
>>>> I'm not sure why that change is required. The value of the variable is
>>>> the same in both cases?
>>>
>>> Take a look a few lines down in the patch:
>>>
>>>> +void efi_reset_system_init(void)
>>>> +{
>>>> +    efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
>>>> +}
>>>
>>> What this does is register a *pointer* as run time service pointer. What
>>> does that mean?
>>>
>>> When we enter RTS, Linux can map any region in the EFI memory map into a
>>> different place in its own virtual memory map. So any pointers we use
>>> inside RTS have to be relocated to the new locations.
>>>
>>> For normal relocations, we move the relocations from linker time to run
>>> time, so that we can relocate ourselves when Linux does the switch-over
>>> to a new address space.
>>>
>>> However, for MMIO that's trickier. That's where the
>>> efi_add_runtime_mmio() function comes into play. It takes care of adding
>>> the page around the references address to the EFI memory map as RTS MMIO
>>> and relocates the pointer when Linux switches us into the new address
>>> space.
>>>
>>> Does that explain why we need to move from an inline address to an
>>> address stored in a memory location?
>>
>> So EFI RTS runs in the same exception level as the rich OS, and not in
>> EL3? I would have expected EFI to run in EL3 with a completely separate
>> MMU configuration. If that's not the case, then this part of the patch
>> does make sense.
>
> Right, it runs in EL2/EL1 with a virtual memory layout that is provided
> by the OS.
>
>>
>>>> Perhaps it's trying to ensure that if this gets compiled into an ldr
>>>> instruction, the referenced data value is in a linker section that's
>>>> still around when EFI runs? If so fine, but how is that ensured for all
>>>> the other constants that this code uses, and if that happens
>>>> automatically due to the __efi_runtime marker above, why doesn't it
>>>> work
>>>> for this one constant?
>>>>
>>>> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it
>>>> might
>>>> be nice to enable it as part of this series, since the code to perform
>>>> that operation is now present.
>>>
>>> That's what I originally wanted, yes :). Unfortunately due to the
>>> relocation explained above, it's basically impossible for any reset
>>> function that calls into MMIO space.
>>>
>>> However, we do have it now for PSCI. If you have a PSCI enabled system,
>>> we don't need to call into MMIO space and thus make the common reset
>>> function available as RTS.
>>
>> Can't the same U-Boot function be called both (a) during U-Boot runtime,
>> where wdog_regs are pre-initialized to match U-Boot's MMU configuration,
>> and (b) once the OS has booted, where wdog_regs has been modified
>> according to the new memory map?
>
> That's exactly what this patch does, no?

I assume not, since you said just a few lines above that doing so was 
impossible, hence why it doesn't implement any halt/poweroff/shutdown 
shell commands.