| Message ID | 20200603223156.12767-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/rpi4: implement watchdog-based reset | expand |
On Wed, Jun 3, 2020 at 3:31 PM Stefano Stabellini <sstabellini@kernel.org> wrote: > > Touching the watchdog is required to be able to reboot the board. > > The implementation is based on > drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> Tested-by: Roman Shaposhnik <roman@zededa.com> Thanks, Roman.
On Wed, Jun 3, 2020 at 4:32 PM Stefano Stabellini <sstabellini@kernel.org> wrote: > > Touching the watchdog is required to be able to reboot the board. > > The implementation is based on > drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> Ah, fantastic, it's been very annoying not being able to reboot the board via ssh. Thanks, Tamas
On Wed, Jun 03, 2020 at 03:31:56PM -0700, Stefano Stabellini wrote: > Touching the watchdog is required to be able to reboot the board. > > The implementation is based on > drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. Ah, I was looking at this just today, as it had been annoying me greatly. This works for me, so: Tested-by: Corey Minyard <cminyard@mvista.com> However, I was wondering if it might be better to handle this by failing the operation in xen and passing it back to dom0 to do. On the Pi you send a firmware message to reboot, and that seems like too much to do in Xen, but it would seem possible to send this back to dom0. Just a thought, as it might be a more general fix for other devices in the same situation. Thanks, -corey > > Signed-off-by: Stefano Stabellini <stefano.stabellini@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> > > 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 > + > +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"); > + 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; > + void __iomem *base = rpi4_map_watchdog(); > + 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 > > -- > 2.17.1 >
Hi, On 04/06/2020 01:15, Corey Minyard wrote: > On Wed, Jun 03, 2020 at 03:31:56PM -0700, Stefano Stabellini wrote: >> Touching the watchdog is required to be able to reboot the board. >> >> The implementation is based on >> drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. > > Ah, I was looking at this just today, as it had been annoying me > greatly. This works for me, so: > > Tested-by: Corey Minyard <cminyard@mvista.com> > > However, I was wondering if it might be better to handle this by failing > the operation in xen and passing it back to dom0 to do. On the Pi you > send a firmware message to reboot, and that seems like too much to do in > Xen, but it would seem possible to send this back to dom0. I don't think this is possible in the current setup. Xen will usually restart the platform if Dom0 requested a clean reboot or crashed. So the domain wouldn't be in state to service such call. > Just a > thought, as it might be a more general fix for other devices in the same > situation. What are the devices you have in mind? Cheers,
(+ 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@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,
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@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, >
On Thu, Jun 04, 2020 at 09:15:33AM +0100, Julien Grall wrote: > Hi, > > On 04/06/2020 01:15, Corey Minyard wrote: > > On Wed, Jun 03, 2020 at 03:31:56PM -0700, Stefano Stabellini wrote: > > > Touching the watchdog is required to be able to reboot the board. > > > > > > The implementation is based on > > > drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. > > > > Ah, I was looking at this just today, as it had been annoying me > > greatly. This works for me, so: > > > > Tested-by: Corey Minyard <cminyard@mvista.com> > > > > However, I was wondering if it might be better to handle this by failing > > the operation in xen and passing it back to dom0 to do. On the Pi you > > send a firmware message to reboot, and that seems like too much to do in > > Xen, but it would seem possible to send this back to dom0. > I don't think this is possible in the current setup. Xen will usually > restart the platform if Dom0 requested a clean reboot or crashed. So the > domain wouldn't be in state to service such call. Ok, I hadn't looked at Xen yet, I didn't know how much shutdown of dom0 happens on a reset. > > > Just a > > thought, as it might be a more general fix for other devices in the same > > situation. > > What are the devices you have in mind? Nothing in particular, but other systems might have the same issue. I guess you have ACPI implemented on x86 already. It just seemed that Linux already has to be able to do this, and passing the buck back there might be a more general solution. Thanks, -corey > > Cheers, > > -- > Julien Grall
On 04/06/2020 12:59, Corey Minyard wrote: > On Thu, Jun 04, 2020 at 09:15:33AM +0100, Julien Grall wrote: >> Hi, >> >> On 04/06/2020 01:15, Corey Minyard wrote: >>> On Wed, Jun 03, 2020 at 03:31:56PM -0700, Stefano Stabellini wrote: >>>> Touching the watchdog is required to be able to reboot the board. >>>> >>>> The implementation is based on >>>> drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. >>> >>> Ah, I was looking at this just today, as it had been annoying me >>> greatly. This works for me, so: >>> >>> Tested-by: Corey Minyard <cminyard@mvista.com> >>> >>> However, I was wondering if it might be better to handle this by failing >>> the operation in xen and passing it back to dom0 to do. On the Pi you >>> send a firmware message to reboot, and that seems like too much to do in >>> Xen, but it would seem possible to send this back to dom0. >> I don't think this is possible in the current setup. Xen will usually >> restart the platform if Dom0 requested a clean reboot or crashed. So the >> domain wouldn't be in state to service such call. > > Ok, I hadn't looked at Xen yet, I didn't know how much shutdown of dom0 > happens on a reset. > >> >>> Just a >>> thought, as it might be a more general fix for other devices in the same >>> situation. >> >> What are the devices you have in mind? > > Nothing in particular, but other systems might have the same issue. I > guess you have ACPI implemented on x86 already. It just seemed that > Linux already has to be able to do this, and passing the buck back there > might be a more general solution. I don't really see how you can make a generic solution here. Each devices may have a different way to act. For anything related to power control, the right solution is to implement PSCI/SCMI in your firmware so you don't need to rely on Dom0 (which may not exist) for such thing. Cheers,
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@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, > > >
Hi, 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 > 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. So what are you using for the firmware? Do you boot Xen directly? Cheers,
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.stabellini@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, >>>
On Thu, 4 Jun 2020, André Przywara wrote: > 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? ;-) Please send a pull request to remove __bcm2835_restart from the Linux kernel, once it is removed from there I'd be happy to take it away from Xen too ;-) I know I am being cheeky but we have enough battles to fight and enough problems with Xen -- I don't think we should use the hypervisor as a leverage to get people to use or upgrade TF. We just need to get good functionalities to our users with the less amount of friction possible. Everything you mentioned are good reason to use TF, and this patch does not take anything away from it. My suggestion would be to work with raspberrypi.org to have TF installed by default by the Raspberry Pi Imager.
On Thu, Jun 4, 2020 at 9:36 AM Julien Grall <julien@xen.org> wrote: > > Hi, > > 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 > > 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. > > So what are you using for the firmware? Do you boot Xen directly? You've got 3 options: 1. booting directly (see Dornernerworks build: https://github.com/dornerworks/xen-rpi4-builder/blob/master/rpixen.sh#L143) 2. booting via u-boot (with efiboot) 3. booting via honest, upstream UEFI (https://github.com/pftf/RPi4/releases/tag/v1.5) So far we've been mostly doing #2 since it is the most flexible one. Thanks, Roman.
On Thu, 4 Jun 2020, Julien Grall wrote: > Hi, > > 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 > > 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. > > So what are you using for the firmware? Do you boot Xen directly? The raspberry pi comes with its own firmware/bootloader. It is possible to boot Linux from it directly, that's how it comes configured by default. For Xen, I am booting uboot from the RPi bootloader first, mostly because uboot is very convenient and adds tftp support which I use to load Xen and Linux. (I think it would be possible to boot Xen directly from the RPi firmware/bootloader but it would probably require some work to get dom0 to load too.)
On Thu, 4 Jun 2020, Roman Shaposhnik wrote: > On Thu, Jun 4, 2020 at 9:36 AM Julien Grall <julien@xen.org> wrote: > > > > Hi, > > > > 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 > > > 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. > > > > So what are you using for the firmware? Do you boot Xen directly? > > You've got 3 options: > 1. booting directly (see Dornernerworks build: > https://github.com/dornerworks/xen-rpi4-builder/blob/master/rpixen.sh#L143) Ah! I didn't realize they were booting directly, nice!
On 04/06/2020 17:46, Stefano Stabellini wrote: > On Thu, 4 Jun 2020, André Przywara wrote: >> 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? ;-) > > Please send a pull request to remove __bcm2835_restart from the Linux > kernel, once it is removed from there I'd be happy to take it away from > Xen too ;-) The kernel needs to support all RPi models, so we definitely need this. Also it's already in there, so removing it is more churn. The reason I am bringing this up is that we should get away from those platform specific files in Xen at all. The only reason we have it for the RPi4 is the non-page-aligned MMIO regions and overlaps, which could actually be determined much better at runtime ... > I know I am being cheeky but we have enough battles to fight and enough > problems with Xen -- I don't think we should use the hypervisor as a > leverage to get people to use or upgrade TF. We just need to get good > functionalities to our users with the less amount of friction possible. As I said: it's not my call, just pointing that out. It's just sad that people everywhere work around the limited firmware instead of doing it properly. > Everything you mentioned are good reason to use TF, and this patch does > not take anything away from it. My suggestion would be to work with > raspberrypi.org to have TF installed by default by the Raspberry Pi > Imager. As far as I know there are (were?) efforts underway. For years ;-) Cheers, Andre
On 04/06/2020 17:46, Stefano Stabellini wrote: > On Thu, 4 Jun 2020, André Przywara wrote: >> 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? ;-) > > Please send a pull request to remove __bcm2835_restart from the Linux > kernel, once it is removed from there I'd be happy to take it away from > Xen too ;-) Xen is not a slave of Linux. We make our own informed decision ;). > > I know I am being cheeky but we have enough battles to fight and enough > problems with Xen -- I don't think we should use the hypervisor as a > leverage to get people to use or upgrade TF. We just need to get good > functionalities to our users with the less amount of friction possible. Well it is nice to have functionality but you also need to have Xen running reliably and safely. No-one wants to drive in car with no brake on a windy road. Or maybe I am wrong? ;) > > Everything you mentioned are good reason to use TF, and this patch does > not take anything away from it. My suggestion would be to work with > raspberrypi.org to have TF installed by default by the . We actually did use the hypervisor as a leverage in the past. A pretty good example is RPI 3. Anyway, the patch is pretty simple and limited to the platform. So I would be inclined to accept it. Although this is just sweeping stability concern under the carpet and hoping for the best. What are the odds this is going to be used in production like that? Cheers,
On Thu, 4 Jun 2020, Julien Grall wrote: > On 04/06/2020 17:46, Stefano Stabellini wrote: > > On Thu, 4 Jun 2020, André Przywara wrote: > > > 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? ;-) > > > > Please send a pull request to remove __bcm2835_restart from the Linux > > kernel, once it is removed from there I'd be happy to take it away from > > Xen too ;-) > > Xen is not a slave of Linux. We make our own informed decision ;). > > > > > I know I am being cheeky but we have enough battles to fight and enough > > problems with Xen -- I don't think we should use the hypervisor as a > > leverage to get people to use or upgrade TF. We just need to get good > > functionalities to our users with the less amount of friction possible. > > Well it is nice to have functionality but you also need to have Xen running > reliably and safely. No-one wants to drive in car with no brake on a windy > road. Or maybe I am wrong? ;) > > > > > Everything you mentioned are good reason to use TF, and this patch does > > not take anything away from it. My suggestion would be to work with > > raspberrypi.org to have TF installed by default by the . > > We actually did use the hypervisor as a leverage in the past. A pretty good > example is RPI 3. > > Anyway, the patch is pretty simple and limited to the platform. So I would be > inclined to accept it. OK, thank you, that it also what I think. > Although this is just sweeping stability concern under the carpet and hoping > for the best. What are the odds this is going to be used in production like > that? We are writing a wikipage to explain how to boot Xen on RPi4, because of the unique firmware/bootloader and the non-upstream patches. We can add a recommandation to use TF for production to the wikipage.
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> 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 + +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"); + 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; + void __iomem *base = rpi4_map_watchdog(); + 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
Touching the watchdog is required to be able to reboot the board. The implementation is based on drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- xen/arch/arm/platforms/brcm-raspberry-pi.c | 60 ++++++++++++++++++++++ 1 file changed, 60 insertions(+)