Re: [RFC] Support stop state version quirk and firmware enabled stop

[Pratik Sampat]

Hello Gautham,

On 08/04/20 3:20 pm, Gautham R Shenoy wrote:
> Hi Pratik,
>
> On Wed, Mar 04, 2020 at 09:26:48PM +0530, Pratik Rajesh Sampat wrote:
> > A concept patch in Skiboot to illustrate the case wherein handling of
> > stop states for different DD versions of a CPU can be achieved by a
> > simple modification in the list of cpu_features.
> > As an example idle-stop1 is defined which uses P9_CPU_DD1 to define the
> > cpu feature.
> >
> > Along with that, an implementation is being worked upon the LE OPAL
> > series which helps OPAL handle the stop state entry and exit.
> >
> > This patch advertises this capability of the firmware which can be
> > availed if the quirk-version-setting is not cognizable.
> >
> > The firmware-enabled stop is being worked by Abhishek Goel
> > <huntbag@xxxxxxxxxxxxxxxxxx> building upon the LE OPAL series.
> >
> > Signed-off-by: Pratik Rajesh Sampat <psampat@xxxxxxxxxxxxx>
> > ---
> >   core/cpufeatures.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/core/cpufeatures.c b/core/cpufeatures.c
> > index ec30c975..b9875e7b 100644
> > --- a/core/cpufeatures.c
> > +++ b/core/cpufeatures.c
> > @@ -510,6 +510,25 @@ static const struct cpu_feature cpu_features_table[] = {
> >   	-1, -1, -1,
> >   	NULL, },
> >
> > +	/*
> > +	 * QUIRK for ISAv3.0B stop idle instructions and registers
> > +	 * Helps us determine if there are any quirks
> > +	 * XXX: Same of idle-stop
> > +	 */
> > +	{ "idle-stop-v1",
> > +	CPU_P9_DD1,
> > +	ISA_V3_0B, USABLE_HV|USABLE_OS,
> > +	HV_CUSTOM, OS_CUSTOM,
> > +	-1, -1, -1,
> > +	NULL, },
>
> So, at this point, we don't need any such quirk for any of the DD
> version right ? This is to demonstrate that if say P9_DD1 had a quirk
> w.r.t stop-state handling, then this is how we would advertise it to
> the kernel.

Absolutely, A dummy property has been added to show how quirk handling
with stop-states.

> > +
> > +	{ "firmware-stop-supported",
> > +	CPU_P9,
> > +	ISA_V3_0B, USABLE_HV|USABLE_OS,
> > +	HV_CUSTOM, OS_CUSTOM,
> > +	-1, -1, -1,
> > +	NULL, },
> > +
>
> I suppose this is for the opal-cpuidle driver support posted here:
> https://lists.ozlabs.org/pipermail/skiboot/2020-April/016726.html

Right, this complements in usage of the opal-cpuidle driver

> >   	/*
> >   	 * ISAv3.0B Hypervisor Virtualization Interrupt
> >   	 * Also associated system registers, LPCR EE, HEIC, HVICE,
> > @@ -883,6 +902,9 @@ static void add_cpufeatures(struct dt_node *cpus,
> >   		const struct cpu_feature *f = &cpu_features_table[i];
> >
> >   		if (f->cpus_supported & cpu_feature_cpu) {
> > +			if (!strcmp(f->name, "firmware-stop-supported") &&
> > +			    HAVE_BIG_ENDIAN)
> > +				continue;
> In OPAL do we have an macro defining BIG_ENDIAN ? If yes, you could
> wrap the "firmware-stop-supported" in cpu_features_table[] within
> #ifndef BIG_ENDIAN. That way you won't need a special case here.
HAVE_BIG_ENDIAN is actually a macro. Its a good idea to wrap it in
the declaration itself.

> >   			DBG("  '%s'\n", f->name);
> >   			add_cpu_feature_nodeps(features, f);
> >   		}
> > --
> > 2.24.1
> --
> Thanks and Regards
> gautham.