Re: [PATCHv7 3/3] arm-virt: add secure pl061 for reset/power down

[Maxim Uvarov]

On Tue, 19 Jan 2021 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 15 Jan 2021 at 10:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> >
> > Add secure pl061 for reset/power down machine from
> > the secure world (Arm Trusted Firmware). Connect it
> > with gpio-pwr driver.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > ---
> >  hw/arm/Kconfig        |  1 +
> >  hw/arm/virt.c         | 50 +++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/arm/virt.h |  2 ++
> >  3 files changed, 53 insertions(+)
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 0a242e4c5d..13cc42dcc8 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -17,6 +17,7 @@ config ARM_VIRT
> >      select PL011 # UART
> >      select PL031 # RTC
> >      select PL061 # GPIO
> > +    select GPIO_PWR
> >      select PLATFORM_BUS
> >      select SMBIOS
> >      select VIRTIO_MMIO
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 26bb66e8e1..436ae894c9 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -153,6 +153,7 @@ static const MemMapEntry base_memmap[] = {
> >      [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> >      [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
> >      [VIRT_PVTIME] =             { 0x090a0000, 0x00010000 },
> > +    [VIRT_SECURE_GPIO] =        { 0x090b0000, 0x00001000 },
> >      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that 
> > size */
> >      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> > @@ -841,6 +842,46 @@ static void create_gpio_keys(const VirtMachineState 
> > *vms,
> >                             "gpios", phandle, 3, 0);
> >  }
> >
> > +#define ATF_GPIO_POWEROFF 3
> > +#define ATF_GPIO_REBOOT   4
>
> These aren't ATF specific, so you could name them SECURE_GPIO_POWEROFF
> and SECURE_GPIO_REBOOT.
>
OK.

> Remind me why we start with GPIO line number 3 and not 0 ?
>

Original gpio power key use 3 and 4 (non-secure). I just selected the
same to be consistent.


> > +
> > +static void create_gpio_pwr(const VirtMachineState *vms,
> > +                            DeviceState *pl061_dev,
> > +                            uint32_t phandle)
> > +{
> > +    DeviceState *gpio_pwr_dev;
> > +
> > +    /* gpio-pwr */
> > +    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
> > +
> > +    /* connect secure pl061 to gpio-pwr */
> > +    qdev_connect_gpio_out(pl061_dev, ATF_GPIO_POWEROFF,
> > +                          qdev_get_gpio_in_named(gpio_pwr_dev, "reset", 
> > 0));
> > +    qdev_connect_gpio_out(pl061_dev, ATF_GPIO_REBOOT,
> > +                          qdev_get_gpio_in_named(gpio_pwr_dev, "shutdown", 
> > 0));
>
> You've connected the POWEROFF gpio line to 'reset' and the
> REBOOT line to 'shutdown'. This looks like it's backwards.
>

Oh, yes. Thanks for finding that.

> > +    qemu_fdt_add_subnode(vms->fdt, "/gpio-pwr");
> > +    qemu_fdt_setprop_string(vms->fdt, "/gpio-pwr", "compatible", 
> > "gpio-pwr");
> > +    qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr", "#size-cells", 0);
> > +    qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr", "#address-cells", 1);
> > +
> > +    qemu_fdt_add_subnode(vms->fdt, "/gpio-pwr/poweroff");
> > +    qemu_fdt_setprop_string(vms->fdt, "/gpio-pwr/poweroff",
> > +                            "label", "GPIO PWR Poweroff");
> > +    qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr/poweroff", "code",
> > +                          ATF_GPIO_POWEROFF);
> > +    qemu_fdt_setprop_cells(vms->fdt, "/gpio-pwr/poweroff",
> > +                           "gpios", phandle, 3, 0);
> > +
> > +    qemu_fdt_add_subnode(vms->fdt, "/gpio-pwr/reboot");
> > +    qemu_fdt_setprop_string(vms->fdt, "/gpio-pwr/reboot",
> > +                            "label", "GPIO PWR Reboot");
> > +    qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr/reboot", "code",
> > +                          ATF_GPIO_REBOOT);
> > +    qemu_fdt_setprop_cells(vms->fdt, "/gpio-pwr/reboot",
> > +                           "gpios", phandle, 3, 0);
>
> There doesn't seem to be any documented 'gpio-pwr' devicetree
> binding. Where does this come from ?
>
gpio-pwr created from the first patch.  There are no bindings yet.

> I think the bindings you want to be using are
> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-restart.txt
> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt
>
These handles are from 'secure memory' where linux does not have
access.  But I think we can use that
binding with other compatible. Like compatible = "gpio-poweroff,secure".

Maxim.

> thanks
> -- PMM