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

Vipin K Parashar vipin at linux.vnet.ibm.com
Thu May 14 06:20:10 AEST 2015


Thanks Stewart.
Will send out a cleanup patch removing unneeded stuff and keeping API 
intact.

Regards,
Vipin

On 05/13/2015 01:35 PM, 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.
>
>> 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