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

Stewart Smith stewart at linux.vnet.ibm.com
Wed May 13 18:05:12 AEST 2015


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.

> Suggestions ?

1) maintain compatibility with existing ABI (and document it)
2) extend (and document how it works) API (and document it) to encompass
   extra functionality needed

Big thing is documentation, and making it *obvious* that existing
kernels will continue to work.

>> Vipin K Parashar <vipin at linux.vnet.ibm.com> writes:
>>> @@ -13,229 +14,217 @@
>>>    * See the License for the specific language governing permissions and
>>>    * limitations under the License.
>>>    */
>>> -/*
>>> - * Handle FSP Environmental and Power Warning (EPOW) events notification
>>> - */
>>> -#include <skiboot.h>
>>> -#include <console.h>
>>> +
>>> +/* FSP Early Power Off Warning (EPOW) support */
>>> +
>> Please submit general re-org as sep patch.
>
> Part of cleanup. EPOW has different names in PAPR and FSP.
> Change done to match with HW/FSP docs.

Okay - but as separate patch would be good.

>> I don't know why FSP chooses to call it panel status, but probably
>> better to refer to it is epow here?
>
> There are no special notifications for EPOW as such in FSP.
> FSP implements Panel Status notifications and reports various info
> like SPCN info, generic UPS info. EPOW information is also encoded in 
> one of those
> Panel status notifications. So the issue is EPOW_EX1 seems to suggest 
> that EX1 is EPOW
> type though EX1 is Panel status (Extended1) and EPOW info is encoded
> there.

Let's attempt to abstract away brain-deadness of parts of FSP as much as
possible :)

>>> +
>>> +	/* Create EPOW device tree node */
>>> +	node_epow = dt_new(opal_node, "epow");
>>> +	if (!node_epow) {
>>> +		prerror(PREFIX "Failed to create epow device tree node\n");
>>> +		return;
>>> +	}
>>> +
>>> +	/* Add EPOW node properties */
>>> +	dt_add_property_strings(node_epow, "compatible",
>>> "ibm,opal-epow");
>> why change in compatible? This means we're breaking API, right?
>
> Not sure why we had "v3" added in compatible fileld ?

OPALv3 (rather than OPALv2 and OPALv1 which were internal).

>>> +/* EPOW types */
>>> +enum OpalEpow {
>>> +	OPAL_EPOW_POWER		= 0,	/* Power EPOW */
>>> +	OPAL_EPOW_TEMP		= 1,	/* Temperature EPOW */
>>> +	OPAL_EPOW_COOLING	= 2,	/* Cooling EPOW */
>>> +	OPAL_MAX_EPOW_CLASSES	= 3,	/* Max EPOW categories */
>>> +};
>>> +
>>> +/* Power EPOW events */
>>> +enum OpalEpowPower {
>>> +	OPAL_EPOW_POWER_UPS	= 0x1, /* System on UPS power */
>>> +	OPAL_EPOW_POWER_UPS_LOW	= 0x2, /* System on UPS power with low battery*/
>>> +};
>>> +
>>> +/* Temperature EPOW events */
>>> +enum OpalEpowTemp {
>>> +	OPAL_EPOW_TEMP_HIGH_AMB	= 0x1, /* High ambient temperature */
>>> +	OPAL_EPOW_TEMP_CRIT_AMB	= 0x2, /* Critical ambient temperature */
>>> +	OPAL_EPOW_TEMP_HIGH_INT	= 0x4, /* High internal temperature */
>>> +	OPAL_EPOW_TEMP_CRIT_INT	= 0x8, /* Critical internal temperature */
>>> +};
>>> +
>> Not sure why all these have changed
>
> Cleanup done. Removed ones aren't useful.

But have they been used anywhere and thus form part of API?



More information about the Skiboot mailing list