Re: [PATCH] xen/rpi4: implement watchdog-based reset

[André Przywara <andre.przywara@xxxxxxx>]

On 04/06/2020 17:24, Stefano Stabellini wrote:
> On Thu, 4 Jun 2020, André Przywara wrote:
>> On 04/06/2020 09:48, Julien Grall wrote:
>>
>> Hi,
>>
>>> On 03/06/2020 23:31, Stefano Stabellini wrote:
>>>> Touching the watchdog is required to be able to reboot the board.
>>>
>>> In general the preferred method is PSCI. Does it mean RPI 4 doesn't
>>> support PSCI at all?
>>
>> There is mainline Trusted Firmware (TF-A) support for the RPi4 for a few
>> months now, which includes proper PSCI support (both for SMP bringup and
>> system reset/shutdown). At least that should work, if not, it's a bug.
>> An EDK-2 build for RPi4 bundles TF-A automatically, but you can use TF-A
>> without it, with or without U-Boot: It works as a drop-in replacement
>> for armstub.bin. Instruction for building it (one line!) are here:
>> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/plat/rpi4.rst
>>
>>>>
>>>> The implementation is based on
>>>> drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux.
>>>
>>> Can you give the baseline? This would allow us to track an issue and
>>> port them.
>>
>> Given the above I don't think it's a good idea to add extra platform
>> specific code to Xen.
> 
> The RPi4, at least the one I have, doesn't come with any TF, and it

My RPi4 didn't come with anything, actually ;-) It's just a matter of
what you put in the uSD card slot.

> doesn't come with PSCI in device tree.

TF-A patches the PSCI nodes in:
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/rpi/rpi4?id=f67fa69cb6937a7fc559bbec4a7acce5edefa888

> As a user, I would rather have
> this patch (even downstream) than having to introduce TF in my build and
> deployment just to be able to reboot.

I get your point, but would rather put more pressure on people using
TF-A. After all you run without CPU hotplug, A72 errata workarounds and
without Spectre/Meltdown fixes. What was the IP address of your board
again? ;-)

> 
> Do other RPi4 users on this thread agree?
> 
> 
> But fortunately this one of the few cases where we can have our cake and
> eat it too :-)
> 
> If PSCI is supported on the RPi4, Xen automatically uses the PSCI reboot
> method first. (We could even go one step further and check for PSCI
> support in rpi4_reset below.) See
> xen/arch/arm/shutdown.c:machine_restart:
> 
>     /* This is mainly for PSCI-0.2, which does not return if success. */
>     call_psci_system_reset();
> 
>     /* Alternative reset procedure */
>     while ( 1 )
>     {
>         platform_reset();
>         mdelay(100);
>     }
> 
> 
> In other words, this patch won't take anything away from the good work
> done in TF, and when/if available, Xen will use it.

Sure, it doesn't block anything. I won't be in your way, after all I
don't have much of a say anyway ;-)

But how do you actually run Xen on the board? I guess this involves
quite some hacks on the firmware side to get it running (bootloader?
EFI? grub? hack the DTB?). I wonder if adding bl31.bin is really your
biggest problem, then.

Cheers,
Andre

>>>>
>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>>>> ---
>>>>   xen/arch/arm/platforms/brcm-raspberry-pi.c | 60 ++++++++++++++++++++++
>>>>   1 file changed, 60 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> b/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> index f5ae58a7d5..0214ae2b3c 100644
>>>> --- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> +++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> @@ -18,6 +18,10 @@
>>>>    */
>>>>     #include <asm/platform.h>
>>>> +#include <xen/delay.h>
>>>> +#include <xen/mm.h>
>>>> +#include <xen/vmap.h>
>>>> +#include <asm/io.h>
>>>
>>> We are trying to keep the headers ordered alphabetically within each
>>> directory and then 'xen/' first following by 'asm/'.
>>>
>>>>     static const char *const rpi4_dt_compat[] __initconst =
>>>>   {
>>>> @@ -37,12 +41,68 @@ static const struct dt_device_match
>>>> rpi4_blacklist_dev[] __initconst =
>>>>        * The aux peripheral also shares a page with the aux UART.
>>>>        */
>>>>       DT_MATCH_COMPATIBLE("brcm,bcm2835-aux"),
>>>> +    /* Special device used for rebooting */
>>>> +    DT_MATCH_COMPATIBLE("brcm,bcm2835-pm"),
>>>>       { /* sentinel */ },
>>>>   };
>>>>   +
>>>> +#define PM_PASSWORD         0x5a000000
>>>> +#define PM_RSTC             0x1c
>>>> +#define PM_WDOG             0x24
>>>> +#define PM_RSTC_WRCFG_FULL_RESET    0x00000020
>>>> +#define PM_RSTC_WRCFG_CLR           0xffffffcf
>>>
>>> NIT: It is a bit odd you introduce the 5 define together but the first 3
>>> have a different indentation compare to the last 2.
>>>
>>>> +
>>>> +static void __iomem *rpi4_map_watchdog(void)
>>>> +{
>>>> +    void __iomem *base;
>>>> +    struct dt_device_node *node;
>>>> +    paddr_t start, len;
>>>> +    int ret;
>>>> +
>>>> +    node = dt_find_compatible_node(NULL, NULL, "brcm,bcm2835-pm");
>>>> +    if ( !node )
>>>> +        return NULL;
>>>> +
>>>> +    ret = dt_device_get_address(node, 0, &start, &len);
>>>> +    if ( ret )
>>>> +    {
>>>> +        dprintk(XENLOG_ERR, "Cannot read watchdog register address\n");
>>>
>>> I would suggest to use printk() rather than dprintk. It would be useful
>>> for a normal user to know that we didn't manage to reset the platform
>>> and why.
>>>
>>>
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    base = ioremap_nocache(start & PAGE_MASK, PAGE_SIZE);
>>>> +    if ( !base )
>>>> +    {
>>>> +        dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    return base;
>>>> +}
>>>> +
>>>> +static void rpi4_reset(void)
>>>> +{
>>>> +    u32 val;
>>>
>>> We are trying to get rid of any use of u32 in Xen code (the coding style
>>> used in this file). Please use uint32_t instead.
>>>
>>>> +    void __iomem *base = rpi4_map_watchdog();
>>>
>>> Newline here please.
>>>
>>>> +    if ( !base )
>>>> +        return;
>>>> +
>>>> +    /* use a timeout of 10 ticks (~150us) */
>>>> +    writel(10 | PM_PASSWORD, base + PM_WDOG);
>>>> +    val = readl(base + PM_RSTC);
>>>> +    val &= PM_RSTC_WRCFG_CLR;
>>>> +    val |= PM_PASSWORD | PM_RSTC_WRCFG_FULL_RESET;
>>>> +    writel(val, base + PM_RSTC);
>>>> +
>>>> +    /* No sleeping, possibly atomic. */
>>>> +    mdelay(1);
>>>> +}
>>>> +
>>>>   PLATFORM_START(rpi4, "Raspberry Pi 4")
>>>>       .compatible     = rpi4_dt_compat,
>>>>       .blacklist_dev  = rpi4_blacklist_dev,
>>>> +    .reset = rpi4_reset,
>>>>       .dma_bitsize    = 30,
>>>>   PLATFORM_END
>>>>  
>>>
>>> Cheers,
>>>