Re: [Qemu-devel] [RFC PATCH 6/7] hw/arm/virt: Use PSCI v0.2 function IDs when kernel supports its

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Mar 14, 2014 at 05:10:34PM +0000, Rob Herring wrote:
> On Fri, Mar 14, 2014 at 5:23 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Mar 14, 2014 at 06:53:53AM +0000, Pranavkumar Sawargaonkar wrote:
> >> Hi Christoffer,
> >>
> >> On 14 March 2014 09:19, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> >> > On Thu, Feb 27, 2014 at 12:21:07PM +0530, Pranavkumar Sawargaonkar wrote:
> >> >> If we have in-kernel emulation of PSCI v0.2 for KVM ARM/ARM64 then
> >> >> we enable PSCI v0.2 for each VCPU at the time of VCPU init hence we
> >> >> need to provide PSCI v0.2 function IDs via generated DTB.
> >> >>
> >> >> This patch updates generated DTB to have PSCI v0.2 function IDs when
> >> >> we have in-kernel emulation PSCI v0.2 for KVM ARM/ARM64.
> >> >>
> >> >> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> >> >> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> >> >> ---
> >> >>  hw/arm/virt.c |   25 ++++++++++++++++++++-----
> >> >>  1 file changed, 20 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> >> index 517f2fe..a818a80 100644
> >> >> --- a/hw/arm/virt.c
> >> >> +++ b/hw/arm/virt.c
> >> >> @@ -187,11 +187,26 @@ static void create_fdt(VirtBoardInfo *vbi)
> >> >>          qemu_fdt_add_subnode(fdt, "/psci");
> >> >>          qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
> >> >
> >> > was there a decision on the format of the psci 0.2 bindings?
> >> >
> >> > I seem to recall that we should add arm,psci-0.2 or something like that.
> >>
> >> Yes there was a discussion related to that by Mark Rutland and Rob Herring :
> >> http://www.spinics.net/lists/arm-kernel/msg298509.html
> >>
> >> But there is no dt binding added related to psci 0.2 in kernel (I am
> >> not sure about final conclusion of the dt binding to be added). If the
> >> dt binding gets finalized I will definitely revise this patch.
> >> For now I have added old binding only to test the rfc patch (may now
> >> be the right way to do but do not have any option also).
> >
> > I believe Rob and I were happy with PSCI 0.2 IDs being implicit, though
> > for compatibility with existing kernels the IDs in the "arm,psci"
> > binding might also be listed.
> >
> > The only issue was Calxeda highbank/midway systems using pre-release
> > PSCI 0.2 IDs for functions not in the "arm,psci" binding. For those we
> > could allocate a compatilbe string like "calxeda,highbank-psci-0.2" and
> > allow them to be implicit, or just leave them in their current
> > (unsupported) state.
> >
> > Rob, thoughts?
> 
> We need to add "arm,psci-0.2" compatible since "arm,psci" defines all
> the function IDs as required and with 0.2 they are optional.
> 
> For highbank, since firmware and DTB is essentially frozen now, a
> compatible change is not going to happen. It probably means just
> leaving things as is.

Ok.

> 
> For reference, this is what highbank/midway's binding looks like:
> 
>         psci {
>                 compatible      = "arm,psci";
>                 method          = "smc";
>                 cpu_suspend     = <0x84000002>;
>                 cpu_off         = <0x84000004>;
>                 cpu_on          = <0x84000006>;
>                 system_off      = <0x84000100>;
>                 system_reset    = <0x84000101>;
>         };
> 
> I suppose I could just ignore the binding and make highbank reset/off
> functions do custom some PSCI calls. This would mean exposing
> invoke_psci_fn or creating some custom call wrapper.

Is there any good way to tell if a machine is highbank or midway (e.g.
the top-level compatible string)?

We could check for that and if so read the additional values from the
dt. That way highbank and midway get supported but new machines would
still have to follow the IDs from the final release.

This doesn't necessarily have to be documented as an official binding,
it's essentially a workaround for a quirk (though one with a good reason
for existing).

Does that sound reasonable?

Cheers,
Mark.