Re: [PATCHv4 2/2] arm-virt: add secure pl061 for reset/power down

[Maxim Uvarov <maxim.uvarov@linaro.org>]

- the same size for secure and non secure gpio. Arm doc says that
secure memory is also split on 4k pages. So one page here has to be
ok.
- will add dtb.
- I think then less options is better. So I will remove
vmc->secure_gpio flag and keep only vmc flag.

Regards,
Maxim.

On Tue, 12 Jan 2021 at 19:28, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Jan 12, 2021 at 11:25:30AM -0500, Andrew Jones wrote:
> > On Tue, Jan 12, 2021 at 04:00:23PM +0000, Peter Maydell wrote:
> > > On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjones@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 ?
> > >
> >
> > 'its' is a property that can be changed on the command line. Unless
> > we want to be able to manage 'secure-gpio' separately from 'secure',
> > then I think vmc->its plus 'secure' should be sufficient. We don't
>
> I meant to write 'vmc->no_secure_gpio and vms->secure' here.
>
> Thanks,
> drew
>
> > always need both vmc and vms state, see 'no_ged'.
> >
> > Thanks,
> > drew
>