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