| Message ID | 20210115101126.4259-4-maxim.uvarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm-virt: add secure pl061 for reset/power down | expand |
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
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
On Tue, 19 Jan 2021 at 13:47, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > 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: > > 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. Those are different GPIO lines on a different PL061 doing a different job. I don't think they need to be the same number. The power keys are on 3 and 4 because pins 0, 1 and 2 were reserved for PCI hotplug, CPU hotplug and memory hotplug. Unless you have some similar reason why you need to reserve pins on the secure PL061, I would just start from 0. > > > + 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. You can't use bindings you've just made up -- you have to get them accepted into the kernel's official devicetree documentation if the ones already there aren't sufficient, before you can add code to QEMU that generates them. > > 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". That's not how you specify that a node is only relevant to the secure world: you set the 'status' property to 'disabled' and the 'secure-status' property to 'okay': https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt thanks -- PMM
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 + +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)); + + 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); +} + static void create_gpio_devices(const VirtMachineState *vms, int gpio, MemoryRegion *mem) { @@ -888,6 +929,8 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio, /* Child gpio devices */ if (gpio == VIRT_GPIO) { create_gpio_keys(vms, pl061_dev, phandle); + } else { + create_gpio_pwr(vms, pl061_dev, phandle); } } @@ -2020,6 +2063,10 @@ static void machvirt_init(MachineState *machine) create_gpio_devices(vms, VIRT_GPIO, sysmem); } + if (vms->secure && !vmc->no_secure_gpio) { + create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem); + } + /* connect powerdown request */ vms->powerdown_notifier.notify = virt_powerdown_req; qemu_register_powerdown_notifier(&vms->powerdown_notifier); @@ -2635,8 +2682,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 0) static void virt_machine_5_2_options(MachineClass *mc) { + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); + virt_machine_6_0_options(mc); compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len); + vmc->no_secure_gpio = true; } DEFINE_VIRT_MACHINE(5, 2) diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index abf54fab49..6f6c85ffcf 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -81,6 +81,7 @@ enum { VIRT_GPIO, VIRT_SECURE_UART, VIRT_SECURE_MEM, + VIRT_SECURE_GPIO, VIRT_PCDIMM_ACPI, VIRT_ACPI_GED, VIRT_NVDIMM_ACPI, @@ -127,6 +128,7 @@ struct VirtMachineClass { bool kvm_no_adjvtime; bool no_kvm_steal_time; bool acpi_expose_flash; + bool no_secure_gpio; }; struct VirtMachineState {
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(+)