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

[Julien Grall <julien@xxxxxxx>]

(+ Andre)

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

--
Julien Grall