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

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

[Stefano Stabellini]

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(+)

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

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

[Roman Shaposhnik]

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.

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

[Tamas K Lengyel]

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

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

[Corey Minyard]

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
> 

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Corey Minyard]

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

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

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

-- 
Julien Grall

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

[Julien Grall]

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

-- 
Julien Grall

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

[André Przywara]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5883 bytes --]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Roman Shaposhnik]

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.

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

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!

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2159 bytes --]

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

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

[André Przywara]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

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.

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

[André Przywara]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4134 bytes --]

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.