[Skiboot] [PATCH v2 1/2] FSP/EPOW: Add support for FSP EPOW events in OPAL

[Ananth N Mavinakayanahalli]

On Wed, May 13, 2015 at 06:05:12PM +1000, Stewart Smith wrote:
> Vipin K Parashar <vipin at linux.vnet.ibm.com> writes:
> > On 05/12/2015 08:33 AM, Stewart Smith wrote:
> >> couple of global comments:
> >> - generally this patch is pretty hard to read, there's a lot of change
> >>    going on, can you break it up into smaller chunks?
> >> - documentation on API is missing and API appears to change.
> > There are few issues with existing API needs to be resolved.
> >
> > Issues as below:
> > * u16 for each class with one bit per EPOW event. Is it sufficient for 
> > future expansions ?
> >     There could be more than 16 events falling under cases like Power or 
> > cooling EPOWs.
> >     u32 would have been a proper one.
> 
> Yes, it would have been and likely should have been... but it isn't, so
> we're stuck with it. We can always have extra classes though EPOW_POWER2
> or something if we need more bits?
> 
> > * Current EPOW OPAL API
> >     int64_t fsp_opal_get_epow_status(int16_t *out_epow, int16_t *length)
> > ^^ ^^
> >        |||| -- Return 
> > status                                                       |||| ------ 
> > Number of EPOW classes.
> >        |||| -- Why int64 ? int32 is default preferred                  
> > |||| ------ int32 is default preferred.
> >
> > Above API parameters need simplification. Currently it burdens 
> > programmers by deviating from default values.
> 
> Yeah, it's not identical to how other places do it, but I don't think
> that's reason enough to change the existing API.
> 
> >
> > * Implemented with understanding that all bits in panel status 
> > notification cause EPOW.
> >     FSP sends Panel Status notification with 3 Panel types. EPOW info is 
> > present in one panel status
> >     in encoded form.  So here only 2 bits cause EPOW, rest don't cause 
> > any EPOW and hence older
> >     EPOW implementation needs cleanup to remove unused cases.
> >
> > Most of places cleanup is necessary to match API implementation with HW 
> > behaviour.
> > Please do look below comments for cleanup needed which is necessary.
> >
> > Considering there are no users for EPOW API in kernel now so it will be 
> > good to make these small adjustments
> > now itself.  Since existing EPOW API is generic enough and thus is good 
> > to use with modifications suggested above.
> 
> So... hw/fsp/fsp-epow.c started in July 2014. Have we *really* had this
> unused? Is it in any PowerKVM release? It seems that PowerKVM 2.1
> shipped with 101a9273dc03f2384bb9ff2642c6dec3304ca50d which does call
> opal_get_epow_status - so this API is being used in the wild and we
> shouldn't break it without *SERIOUS* thought and deprecation over a long
> period of time.
> 
> Note:
> +       epow = of_find_node_by_path("/ibm,opal/epow");
> +       if (epow) {
> +               of_platform_device_create(epow, "opal_event", NULL);
> +               of_node_put(epow);
> +       }
> +
> <snip>
> +static struct of_device_id opal_event_match[] = {
> +       {
> +               .compatible     = "ibm,opal-v3-epow",
> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, opal_event_match);
> +
> +static struct platform_driver opal_event_driver = {
> +       .probe  = opal_event_probe,
> +       .remove = opal_event_remove,
> +       .driver = {
> +               .name = "opal-platform-event-driver",
> +               .owner = THIS_MODULE,
> +               .of_match_table = opal_event_match,
> +       },
> +};
> +
> 
> So while it seems your patch will ensure things will fail gracefully, we
> loose the functionality we previously had, and I'm not sure there's a
> good reason for that.
> 
> we do want to keep compatibility with PowerKVM versions with new/old
> firmware without losing functionality unless there is a *VERY* good
> reason. You haven't convinced me that breaking EPOW support in existing
> PowerKVM versions is something we *must* do.

FWIW, EPOW support in PowerKVM 2.1.1 was just Tech Preview AFAIR. Given
that nobody actually 'used' this interface from what I know, do we have
some leeway to change/correct/deprecate it? One simple change would be
to just do an orderly_poweroff() in the 2.1.1 driver (through a bugfix)
-- that is what current PowerVM Linux LPARs do.

Ananth