[PATCH 0/2] arm/virt: fix machine poweroff

[PATCH 0/2] arm/virt: fix machine poweroff

[Maxim Uvarov]

Tested against the latest ecba223da6215d6f6ce2d343b70b2e9a19bfb90b
and ATF code. Patches fix 'reboot' and 'poweroff' commands issued
from linux.

Maxim Uvarov (2):
  arm/virt: fix irq mapping for gpio_pwr
  gpio_pwr: use shutdown function for reboot

 hw/arm/virt.c      | 4 ++--
 hw/gpio/gpio_pwr.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/2] arm/virt: fix irq mapping for gpio_pwr

[Maxim Uvarov]

Fix irq mapping for virt machine reboot and shutdown
interrupts triggered from ATF code.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 hw/arm/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4b96f06014..d83f4ebdd3 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -846,8 +846,8 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
                            "gpios", phandle, 3, 0);
 }
 
-#define SECURE_GPIO_POWEROFF 0
-#define SECURE_GPIO_RESET    1
+#define SECURE_GPIO_POWEROFF 1
+#define SECURE_GPIO_RESET    2
 
 static void create_secure_gpio_pwr(char *fdt, DeviceState *pl061_dev,
                                    uint32_t phandle)
-- 
2.17.1

Re: [PATCH 1/2] arm/virt: fix irq mapping for gpio_pwr

[Peter Maydell]

On Fri, 25 Jun 2021 at 12:18, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> Fix irq mapping for virt machine reboot and shutdown
> interrupts triggered from ATF code.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  hw/arm/virt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4b96f06014..d83f4ebdd3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -846,8 +846,8 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
>                             "gpios", phandle, 3, 0);
>  }
>
> -#define SECURE_GPIO_POWEROFF 0
> -#define SECURE_GPIO_RESET    1
> +#define SECURE_GPIO_POWEROFF 1
> +#define SECURE_GPIO_RESET    2

I don't understand why this is needed. QEMU counts its GPIO
lines from 0 for the PL061, so wiring up lines 1 and 2
should be no different from using lines 0 and 1 (either way,
we tell the guest what lines to use in the device tree).

-- PMM

Re: [PATCH 1/2] arm/virt: fix irq mapping for gpio_pwr

[Maxim Uvarov]

On Fri, 25 Jun 2021 at 14:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 25 Jun 2021 at 12:18, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> >
> > Fix irq mapping for virt machine reboot and shutdown
> > interrupts triggered from ATF code.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > ---
> >  hw/arm/virt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 4b96f06014..d83f4ebdd3 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -846,8 +846,8 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
> >                             "gpios", phandle, 3, 0);
> >  }
> >
> > -#define SECURE_GPIO_POWEROFF 0
> > -#define SECURE_GPIO_RESET    1
> > +#define SECURE_GPIO_POWEROFF 1
> > +#define SECURE_GPIO_RESET    2
>
> I don't understand why this is needed. QEMU counts its GPIO
> lines from 0 for the PL061, so wiring up lines 1 and 2
> should be no different from using lines 0 and 1 (either way,
> we tell the guest what lines to use in the device tree).
>
> -- PMM

For now we do not tell in the device tree. It's pin 0 for poweroff and
pin 1 for reboot;
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/qemu/qemu/include/platform_def.h#L94
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/qemu/common/qemu_pm.c#L211

But to answer the question why 0 and 1 do not work and 1 and 2 works I
need to do some debugging on pl061 or qemu interrupt code.

Regards,
Maxim.

[PATCH 2/2] gpio_pwr: use shutdown function for reboot

[Maxim Uvarov]

qemu has 2 type of functions: shutdown and reboot. Shutdown
function has to be used for machine shutdown.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 hw/gpio/gpio_pwr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
index 7714fa0dc4..dbaf1c70c8 100644
--- a/hw/gpio/gpio_pwr.c
+++ b/hw/gpio/gpio_pwr.c
@@ -43,7 +43,7 @@ static void gpio_pwr_reset(void *opaque, int n, int level)
 static void gpio_pwr_shutdown(void *opaque, int n, int level)
 {
     if (level) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
 }
 
-- 
2.17.1

Re: [PATCH 2/2] gpio_pwr: use shutdown function for reboot

[Peter Maydell]

On Fri, 25 Jun 2021 at 12:18, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> qemu has 2 type of functions: shutdown and reboot. Shutdown
> function has to be used for machine shutdown.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  hw/gpio/gpio_pwr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
> index 7714fa0dc4..dbaf1c70c8 100644
> --- a/hw/gpio/gpio_pwr.c
> +++ b/hw/gpio/gpio_pwr.c
> @@ -43,7 +43,7 @@ static void gpio_pwr_reset(void *opaque, int n, int level)
>  static void gpio_pwr_shutdown(void *opaque, int n, int level)
>  {
>      if (level) {
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>      }
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] gpio_pwr: use shutdown function for reboot

[Peter Maydell]

On Fri, 25 Jun 2021 at 12:56, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 25 Jun 2021 at 12:18, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> >
> > qemu has 2 type of functions: shutdown and reboot. Shutdown
> > function has to be used for machine shutdown.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > ---
> >  hw/gpio/gpio_pwr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
> > index 7714fa0dc4..dbaf1c70c8 100644
> > --- a/hw/gpio/gpio_pwr.c
> > +++ b/hw/gpio/gpio_pwr.c
> > @@ -43,7 +43,7 @@ static void gpio_pwr_reset(void *opaque, int n, int level)
> >  static void gpio_pwr_shutdown(void *opaque, int n, int level)
> >  {
> >      if (level) {
> > -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> >      }
> >  }
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

There are clearly more problems with this GPIO setup that we need
to get to the bottom of. But in the meantime this patch 2 is an
obvious bug fix, so I'm going to to apply it to target-arm.next.

thanks
-- PMM