[U-Boot] [PATCH 2/2] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

On 12.07.16 21:35, Simon Glass wrote:
> Hi Alexander,
> 
> 
> On 5 June 2016 at 15:17, Alexander Graf <agraf at suse.de> 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.
>>
>> Signed-off-by: Alexander Graf <agraf at suse.de>
>> ---
>>  arch/arm/mach-bcm283x/include/mach/wdog.h |  2 +-
>>  arch/arm/mach-bcm283x/reset.c             | 59 +++++++++++++++++++++++++++----
>>  2 files changed, 54 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/mach-bcm283x/include/mach/wdog.h b/arch/arm/mach-bcm283x/include/mach/wdog.h
>> index 7741d7b..b4caca1 100644
>> --- a/arch/arm/mach-bcm283x/include/mach/wdog.h
>> +++ b/arch/arm/mach-bcm283x/include/mach/wdog.h
>> @@ -16,7 +16,7 @@
>>  struct bcm2835_wdog_regs {
>>         u32 unknown0[7];
>>         u32 rstc;
>> -       u32 unknown1;
>> +       u32 rsts;
>>         u32 wdog;
>>  };
>>
>> diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c
>> index 72cdc31..e87241d 100644
>> --- a/arch/arm/mach-bcm283x/reset.c
>> +++ b/arch/arm/mach-bcm283x/reset.c
>> @@ -10,19 +10,66 @@
>>  #include <common.h>
>>  #include <asm/io.h>
>>  #include <asm/arch/wdog.h>
>> +#include <efi_loader.h>
>>
>>  #define RESET_TIMEOUT 10
>>
>> -void reset_cpu(ulong addr)
>> +/*
>> + * The Raspberry Pi firmware uses the RSTS register to know which partiton
>> + * to boot from. The partiton value is spread into bits 0, 2, 4, 6, 8, 10.
>> + * Partiton 63 is a special partition used by the firmware to indicate halt.
>> + */
>> +#define BCM2835_WDOG_RSTS_RASPBERRYPI_HALT     0x555
>> +
>> +EFI_RUNTIME_DATA struct bcm2835_wdog_regs *wdog_regs =
>> +       (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>> +
>> +void EFI_RUNTIME_TEXT reset_cpu(ulong addr)
>>  {
>> -       struct bcm2835_wdog_regs *regs =
>> -               (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>         uint32_t rstc;
>>
>> -       rstc = readl(&regs->rstc);
>> +       rstc = readl(&wdog_regs->rstc);
>>         rstc &= ~BCM2835_WDOG_RSTC_WRCFG_MASK;
>>         rstc |= BCM2835_WDOG_RSTC_WRCFG_FULL_RESET;
>>
>> -       writel(BCM2835_WDOG_PASSWORD | RESET_TIMEOUT, &regs->wdog);
>> -       writel(BCM2835_WDOG_PASSWORD | rstc, &regs->rstc);
>> +       writel(BCM2835_WDOG_PASSWORD | RESET_TIMEOUT, &wdog_regs->wdog);
>> +       writel(BCM2835_WDOG_PASSWORD | rstc, &wdog_regs->rstc);
> 
> This is rpi-specific code, and presumably U-Boot itself would want to
> provide this reset facility.
> 
> But this code is marked with EFI_RUNTIME_TEXT. I think this means that
> it can still be called as a normal U-Boot function, right?

Exactly. EFI_RUNTIME_TEXT just moves it into a section that allows the
code to exist in parallel with Linux as something called "runtime services".

> Is there some enforcement at link-time that all the function called by
> EFI_RUNTIME_TEXT functions are also EFI_RUNTIME_TEXT?

There's a runtime check in the efi_loader linker. I couldn't find a good
way to check it at compile time.

> Also I'd like to suggest a better name. How about __efi_runtime
> instead of EFI_RUNTIME_TEXT? All those capital letters in function
> declarations will give me a headache...we use __packed, __weak, etc.

Uh, sure. The reason it's upper case is that it can either be defined to
a section identifier (if EFI is enabled) or to nothing. Since I'm used
to the very simple convention that "#define means upper case" I've made
the macro upper case too.

But if you think it'd be more readable to treat it like an unconditional
attribute marker, I'm certainly not against it.


Alex