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

[Peter Maydell <peter.maydell@linaro.org>]

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