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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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
doesn't come with PSCI in device tree. 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.

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.



> >>
> >> 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,
> > 
>