On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjo...@redhat.com> wrote: > > On Tue, Jan 12, 2021 at 05:30:58PM +0300, Maxim Uvarov wrote: > > Add secure pl061 for reset/power down machine from > > the secure world (Arm Trusted Firmware). Connect it > > with gpio-pwr driver.
> > + /* 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)); > > I don't know anything about secure world, but it seems odd that we don't > need to add anything to the DTB. We should be adding something to the DTB, yes. Look at how create_uart() does this -- you set the 'status' and 'secure-status' properties to indicate that the device is secure-world only. > > + if (vmc->no_secure_gpio) { > > + vms->secure_gpio = false; > > + } else { > > + vms->secure_gpio = true; > > + } > > nit: vms->secure_gpio = !vmc->no_secure_gpio > > But do we even need vms->secure_gpio? Why not just do > > if (vms->secure && !vmc->no_secure_gpio) { > create_gpio_secure(vms, secure_sysmem); > } > > in machvirt_init() ? We're just following the same pattern as vmc->no_its/vms->its, aren't we ? thanks -- PMM