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

[Peter Maydell]

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.

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

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

> +    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 ?

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

thanks
-- PMM