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.stabell...@xilinx.com> >> --- >> 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, >
