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

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

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.

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