[Skiboot] [PATCH v2 1/2] FSP/EPOW: Add support for FSP EPOW events in OPAL
Vipin K Parashar
vipin at linux.vnet.ibm.com
Tue May 12 15:34:35 AEST 2015
Hi Stewart,
Thanks for review.
Responses below
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.
* 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.
* 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.
Suggestions ?
>
> Vipin K Parashar <vipin at linux.vnet.ibm.com> writes:
>> diff --git a/hw/fsp/fsp-epow.c b/hw/fsp/fsp-epow.c
>> index eaba8bb..97b082a 100644
>> --- a/hw/fsp/fsp-epow.c
>> +++ b/hw/fsp/fsp-epow.c
>> @@ -1,10 +1,11 @@
>> -/* Copyright 2013-2014 IBM Corp.
>> +/*
>> + * Copyright 2013-2014 IBM Corp.
>> *
>> * Licensed under the Apache License, Version 2.0 (the "License");
>> * you may not use this file except in compliance with the License.
>> * You may obtain a copy of the License at
>> *
>> - * http://www.apache.org/licenses/LICENSE-2.0
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> *
>> * Unless required by applicable law or agreed to in writing, software
>> * distributed under the License is distributed on an "AS IS" BASIS,
> If this is different to other files, please submit separate patch that
> fixes everywhere to be consistent.
ok
>
>> @@ -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.
>
>> #include <fsp.h>
>> #include <device.h>
>> -#include <stdio.h>
>> -#include <spcn.h>
>> -#include <opal.h>
>> +#include <lock.h>
>> #include <opal-msg.h>
>> +#include <opal-api.h>
>>
>> #include "fsp-epow.h"
>>
>> -#define PREFIX "FSPEPOW: "
>> +#define PREFIX "FSP-EPOW: "
> I've finally gotten sick enough of seeing #define PREFIX around that
> I've gone and implemented pr_fmt for our printf/prlog function, so when
> you resubmit this patch, please use it.
Will do.
>
>> /*
>> * System EPOW status
>> *
>> - * This value is exported to the host. Each individual element in this array
>> - * [0..(OPAL_SYSEPOW_MAX -1)] contains detailed status (in it's bit positions)
>> - * corresponding to a particular defined EPOW sub class. For example.
>> - *
>> - * epow_status[OPAL_SYSEPOW_POWER] will reflect whether the system has one or
>> - * more of power subsystem specific EPOW events like OPAL_SYSPOWER_UPS,
>> - * OPAL_SYSPOWER_CHNG, OPAL_SYSPOWER_FAIL or OPAL_SYSPOWER_INCL.
>> + * This value is exported to the host. Each individual element in this
>> + * array [0...(OPAL_MAX_EPOW_CLASSES-1)] contains bitwise EPOW event info
>> + * corresponding to particular defined EPOW sub class. For example.
>> + * opal_epow_status[OPAL_EPOW_POWER] will reflect power related EPOW events.
>> */
>> -static int16_t epow_status[OPAL_SYSEPOW_MAX];
>> +static u32 opal_epow_status[OPAL_MAX_EPOW_CLASSES];
> Please also add a doc/opal-api/ file describing the OPAL calls and data
> structures.
>
> Is this an API change though? Going from 16 to 32bits looks like it.
Each bit in epow_status tells one EPOW event. Having u16 to present all
EPOWs in a class doesn't
seem to be sufficient and might get exhausted.
>
>> -/* Process FSP sent SPCN based information */
>> -static void epow_process_base_event(u8 *epow)
>> -{
>> +/* Utilty Failure (UF) bit status */
>> +static u8 utility_fail_mask;
>>
>> - epow_status[OPAL_SYSEPOW_POWER] &= ~(OPAL_SYSPOWER_CHNG |
>> - OPAL_SYSPOWER_FAIL | OPAL_SYSPOWER_INCL);
>> - /*
>> - * FIXME: As of now, SPCN_FAULT_LOG event is not being used
>> - * as it does not map to any generic defined OPAL EPOW event.
>> - */
>> - if (epow[3] & SPCN_CNF_CHNG) {
>> - /*
>> - * The frequency of the SPCN_CNF_CHNG message is very
>> - * high on POWER7 and POWER8 systems which will fill
>> - * up the Sapphire log buffer. SPCN configuration
>> - * change does not take down the system, hence the
>> - * logging of these type of messages can be avoided to
>> - * save precious log buffer space.
>> - */
>> - epow_status[OPAL_SYSEPOW_POWER] |= OPAL_SYSPOWER_CHNG;
>> - }
>> +/* Battery Low (BL) bit status */
>> +static u8 battery_low_mask;
>>
>> - if (epow[3] & SPCN_POWR_FAIL) {
>> - prlog(PR_TRACE, PREFIX "FSP message with SPCN_POWR_FAIL\n");
>> - epow_status[OPAL_SYSEPOW_POWER] |= OPAL_SYSPOWER_FAIL;
>> - }
>> +/* Process EPOW event notification */
>> +static bool process_epow_event(struct fsp_msg *msg)
> I don't think this comment adds anything that cannot be inferred from
> the name of the function.
Will remove it.
>
>> +{
>> + u8 ups_status;
>> + int epow_reason;
>> + bool new_epow_event = false;
>> + bool unknown_epow = false;
>>
>> - if (epow[3] & SPCN_INCL_POWR) {
>> - prlog(PR_TRACE, PREFIX "FSP message with SPCN_INCL_POWR\n");
>> - epow_status[OPAL_SYSEPOW_POWER] |= OPAL_SYSPOWER_INCL;
>> - }
>> -}
>> + prlog(PR_DEBUG, PREFIX "UPS byte = 0x%x, SPCN byte = 0x%x\n",
>> + msg->data.bytes[1], msg->data.bytes[2]);
>>
>> -/* Process FSP sent EPOW based information */
>> -static void epow_process_ex1_event(u8 *epow)
>> -{
>> - epow_status[OPAL_SYSEPOW_POWER] &= ~OPAL_SYSPOWER_UPS;
>> - epow_status[OPAL_SYSEPOW_TEMP] &= ~(OPAL_SYSTEMP_AMB | OPAL_SYSTEMP_INT);
>> + /* UPS status bits information. */
>> + ups_status = msg->data.bytes[1];
>>
>> - if (epow[4] == EPOW_ON_UPS) {
>> - prlog(PR_TRACE, PREFIX "FSP message with EPOW_ON_UPS\n");
>> - epow_status[OPAL_SYSEPOW_POWER] |= OPAL_SYSPOWER_UPS;
>> + /*
>> + * Check if there is any change in UF and BL bits,
>> + * signifying new EPOW event.
>> + */
>> + lock(&epow_lock);
>> + if (utility_fail_mask != (ups_status & UPS_UTILITY_FAIL)) {
>> + /* Update new utilty fail mask */
>> + utility_fail_mask = (ups_status & UPS_UTILITY_FAIL);
>> + new_epow_event = true;
>> }
>>
>> - if (epow[4] == EPOW_TMP_AMB) {
>> - prlog(PR_TRACE, PREFIX "FSP message with EPOW_TMP_AMB\n");
>> - epow_status[OPAL_SYSEPOW_TEMP] |= OPAL_SYSTEMP_AMB;
>> + /*
>> + * With non-zero utility fail mask, check if there is any change in
>> + * battery low mask value . With zero utility fail mask value, battery
>> + * low mask change doesn't trigger EPOW event.
>> + */
>> + if (utility_fail_mask) {
>> + if (battery_low_mask != (ups_status & UPS_BATTERY_LOW)) {
>> + /* Update new battery low mask */
>> + battery_low_mask = (ups_status & UPS_BATTERY_LOW);
>> + new_epow_event = true;
>> + }
>> }
>>
>> - if (epow[4] == EPOW_TMP_INT) {
>> - prlog(PR_TRACE, PREFIX "FSP message with EPOW_TMP_INT\n");
>> - epow_status[OPAL_SYSEPOW_TEMP] |= OPAL_SYSTEMP_INT;
>> + prlog(PR_DEBUG, PREFIX "UF Mask = 0x%x, BL Mask = 0x%x. "
>> + "New EPOW event = %d\n", utility_fail_mask,
>> + battery_low_mask, new_epow_event);
>> +
>> + if (new_epow_event == false) {
>> + unlock(&epow_lock);
>> + return false;
>> }
>> -}
>>
>> -/* Update the system EPOW status */
>> -static void fsp_epow_update(u8 *epow, int epow_type)
>> -{
>> - int16_t old_epow_status[OPAL_SYSEPOW_MAX];
>> - bool epow_changed = false;
>> - int rc;
>> + /* Collect EPOW reason code information */
>> + epow_reason = msg->data.bytes[4];
>>
>> - lock(&epow_lock);
>> + /* Proceed with new EPOW event processing */
>> + switch (epow_reason) {
>> + case EPOW_NONE:
>> + if (!utility_fail_mask && !battery_low_mask) {
>> + memset(opal_epow_status, 0, sizeof(opal_epow_status));
>> + prlog(PR_INFO,
>> + PREFIX "EPOW condition returned to normal\n");
>> + } else
>> + unknown_epow = true;
>> + break;
>>
>> - /* Copy over and clear system EPOW status */
>> - memcpy(old_epow_status, epow_status, sizeof(old_epow_status));
>> - switch(epow_type) {
>> - case EPOW_NORMAL:
>> - epow_process_base_event(epow);
>> - /* FIXME: IPL mode information present but not used */
>> + case EPOW_ON_UPS:
>> + if (utility_fail_mask && !battery_low_mask) {
>> + opal_epow_status[OPAL_EPOW_POWER] = OPAL_EPOW_POWER_UPS;
>> + prlog(PR_INFO,
>> + PREFIX "EPOW due to system on UPS power\n");
>> + } else if (utility_fail_mask && battery_low_mask) {
>> + opal_epow_status[OPAL_EPOW_POWER] =
>> + OPAL_EPOW_POWER_UPS_LOW;
>> + prlog(PR_INFO,
>> + PREFIX "EPOW due to system on UPS power "
>> + "with UPS battery low.\n");
>> + } else
>> + unknown_epow = true;
>> break;
>> - case EPOW_EX1:
>> - epow_process_base_event(epow);
>> - epow_process_ex1_event(epow);
>> - /* FIXME: IPL mode information present but not used */
>> - /* FIXME: Key position information present but not used */
>> +
>> + case EPOW_AMB_TEMP:
>> + if (utility_fail_mask && !battery_low_mask) {
>> + opal_epow_status[OPAL_EPOW_TEMP] =
>> + OPAL_EPOW_TEMP_HIGH_AMB;
>> + prlog(PR_INFO,
>> + PREFIX "EPOW due to high ambient temp\n");
>> + } else if (utility_fail_mask && battery_low_mask) {
>> + opal_epow_status[OPAL_EPOW_TEMP] =
>> + OPAL_EPOW_TEMP_CRIT_AMB;
>> + prlog(PR_INFO,
>> + PREFIX "EPOW due to critical ambient temp\n");
>> + } else
>> + unknown_epow = true;
>> break;
>> - case EPOW_EX2:
>> - /*FIXME: IPL mode information present but not used */
>> - /*FIXME: Key position information present but not used */
>> +
>> + case EPOW_INT_TEMP:
>> + if (utility_fail_mask && !battery_low_mask) {
>> + opal_epow_status[OPAL_EPOW_TEMP] =
>> + OPAL_EPOW_TEMP_HIGH_INT;
>> + prlog(PR_INFO,
>> + PREFIX "EPOW due to high internal temp\n");
>> + } else if (utility_fail_mask && battery_low_mask) {
>> + opal_epow_status[OPAL_EPOW_TEMP] =
>> + OPAL_EPOW_TEMP_CRIT_INT;
>> + prlog(PR_INFO,
>> + PREFIX "EPOW due to critical internal temp\n");
>> + } else
>> + unknown_epow = true;
>> break;
>> +
>> default:
>> - prlog(PR_WARNING, PREFIX "Unknown EPOW event notification\n");
>> - break;
>> + unknown_epow = true;
>> }
>> unlock(&epow_lock);
>>
>> - if (memcmp(epow_status, old_epow_status, sizeof(epow_status)))
>> - epow_changed = true;
>> + prlog(PR_DEBUG, PREFIX "Supported EPOW classes = %d. "
>> + "EPOW Status = 0x%x 0x%x 0x%x\n", OPAL_MAX_EPOW_CLASSES,
>> + opal_epow_status[0], opal_epow_status[1],
>> opal_epow_status[2]);
> Not quite sure why there's this prlog here about what EPOW classes are
> supported, isn't this determined at startup?
Main purpose of this message is to report EPOW status obtained for each
class,
Will drop EPOW classes supported info here as its fixed as can be logged
in beginning
itself.
>
>> - /* Send OPAL message notification */
>> - if (epow_changed) {
>> - rc = opal_queue_msg(OPAL_MSG_EPOW, NULL, NULL);
>> - if (rc) {
>> - prlog(PR_ERR, PREFIX "OPAL EPOW message queuing failed\n");
>> - return;
>> - }
>> + if (unknown_epow) {
>> + prerror(PREFIX "Unknown EPOW. UF Mask = 0x%x, BL Mask = 0x%x, "
>> + "Reason = %d\n", utility_fail_mask,
>> + battery_low_mask, epow_reason);
>> + return false;
>> }
>> +
>> + return true;
>> }
>>
>> -/* Process captured EPOW event notification */
>> -static void fsp_process_epow(struct fsp_msg *msg, int epow_type)
>> +/*
>> + * Notify host about EPOW event. Host should make a OPAL call
>> + * subsequently to obtain EPOW event information.
>> + */
>> +static void notify_epow_event(void)
>> +{
>> + int rc;
>> +
>> + /* Send OPAL message notification */
>> + rc = opal_queue_msg(OPAL_MSG_EPOW, NULL, NULL);
>> + if (rc)
>> + prerror(PREFIX "OPAL message queuing failed for EPOW event. "
>> + "rc = %d\n", rc);
>> + else
>> + prlog(PR_INFO, PREFIX "Notified host about EPOW event.\n");
>> +}
>> +
>> +/* Process panel status notification */
>> +static void process_panel_status(struct fsp_msg *msg, int panel_type)
>> {
>> - struct fsp_msg *resp;
>> - u8 epow[8];
>> + bool new_epow_event;
>>
>> - /* Basic EPOW signature */
>> + /* Match panel status notification signature */
>> if (msg->data.bytes[0] != 0xF2) {
>> - prlog(PR_ERR, PREFIX "Signature mismatch\n");
>> + prerror(PREFIX "Signature mismatch\n");
>> return;
>> }
>>
>> - /* Common to all EPOW event types */
>> - epow[0] = msg->data.bytes[0];
>> - epow[1] = msg->data.bytes[1];
>> - epow[2] = msg->data.bytes[2];
>> - epow[3] = msg->data.bytes[3];
>> -
>> - /*
>> - * After receiving the FSP async message, HV needs to
>> - * ask for the detailed panel status through corresponding
>> - * mbox command. HV need not use the received details status
>> - * as it does not have any thing more or new than what came
>> - * along with the original FSP async message. But requesting
>> - * for the detailed panel status exclussively is necessary as
>> - * it forms a kind of handshaking with the FSP. Without this
>> - * step, FSP wont be sending any new panel status messages.
>> - */
>> - switch(epow_type) {
>> - case EPOW_NORMAL:
>> - resp = fsp_mkmsg(FSP_CMD_STATUS_REQ, 0);
>> - if (resp == NULL) {
>> - prerror(PREFIX "%s : Message allocation failed\n",
>> - __func__);
>> - break;
>> - }
>> - if (fsp_queue_msg(resp, fsp_freemsg)) {
>> - fsp_freemsg(resp);
>> - prerror(PREFIX "%s : Failed to queue response "
>> - "message\n", __func__);
>> - }
>> - break;
>> - case EPOW_EX1:
>> - /* EPOW_EX1 specific extra event data */
>> - epow[4] = msg->data.bytes[4];
>> - resp = fsp_mkmsg(FSP_CMD_STATUS_EX1_REQ, 0);
>> - if (resp == NULL) {
>> - prerror(PREFIX "%s : Message allocation failed\n",
>> - __func__);
>> - break;
>> - }
>> - if (fsp_queue_msg(resp, fsp_freemsg)) {
>> - fsp_freemsg(resp);
>> - prerror(PREFIX "%s : Failed to queue response "
>> - "message\n", __func__);
>> - }
>> - break;
>> - case EPOW_EX2:
>> - resp = fsp_mkmsg(FSP_CMD_STATUS_EX2_REQ, 0);
>> - if (resp == NULL) {
>> - prerror(PREFIX "%s : Message allocation failed\n",
>> - __func__);
>> - break;
>> - }
>> - if (fsp_queue_msg(resp, fsp_freemsg)) {
>> - fsp_freemsg(resp);
>> - prerror(PREFIX "%s : Failed to queue response "
>> - "message\n", __func__);
>> - }
>> - break;
>> - default:
>> - prlog(PR_WARNING, PREFIX "Unknown EPOW event notification\n");
>> + /* EPOW event information is encoded in extended panel 1 status */
>> + if (panel_type != PANEL_EX1)
>> return;
>> - }
>> - fsp_epow_update(epow, epow_type);
>> +
>> + /* Process EPOW event information and */
>> + new_epow_event = process_epow_event(msg);
>> +
>> + /* Notify host if new EPOW event detected */
>> + if (new_epow_event)
>> + notify_epow_event();
>> }
>>
>> /*
>> * EPOW OPAL interface
>> *
>> * The host requests for the system EPOW status through this
>> - * OPAl call, where it passes a buffer with a give length.
>> - * Sapphire fills the buffer with updated system EPOW status
>> - * and then updates the length variable back to reflect the
>> - * number of EPOW sub classes it has updated the buffer with.
>> + * OPAl call. Host passes an buffer address as first argument
> nitpick: a buffer, not an buffer.
Agreed.
>
>> + * while second argument contains number of EPOW classes known
>> + * by it. OPAL populates passed buffer with its EPOW status
>> + * and updates variable having number of classes with number of
>> + * EPOW classes for which it has returned status.
>> */
>> -static int64_t fsp_opal_get_epow_status(int16_t *out_epow,
>> - int16_t *length)
>> +static int32_t opal_get_epow_status(u32 *epow_status,
>> + int32_t
>> *num_epow_classes)
> This does look like a API change.
>
> If you *must* change OPAL API, you *MUST* do it in a forward and
> backwards compatible way and very carefully document old and new in
> doc/opal-api/ (and in commit message)
Please see first note.
>
>> @@ -274,13 +261,13 @@ static bool fsp_epow_message(u32 cmd_sub_mod, struct fsp_msg *msg)
>> {
>> switch(cmd_sub_mod) {
>> case FSP_CMD_PANELSTATUS:
>> - fsp_process_epow(msg, EPOW_NORMAL);
>> + process_panel_status(msg, PANEL_NORMAL);
>> return true;
>> case FSP_CMD_PANELSTATUS_EX1:
>> - fsp_process_epow(msg, EPOW_EX1);
>> + process_panel_status(msg, PANEL_EX1);
>> return true;
>> case FSP_CMD_PANELSTATUS_EX2:
>> - fsp_process_epow(msg, EPOW_EX2);
>> + process_panel_status(msg, PANEL_EX2);
>> return true;
> 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.
>
>> }
>> return false;
>> @@ -292,12 +279,25 @@ static struct fsp_client fsp_epow_client = {
>>
>> void fsp_epow_init(void)
>> {
>> - struct dt_node *np;
>> + struct dt_node *node_epow;
>>
>> + /* Register FSP EPOW notifications */
>> fsp_register_client(&fsp_epow_client, FSP_MCLASS_SERVICE);
>> - opal_register(OPAL_GET_EPOW_STATUS, fsp_opal_get_epow_status, 2);
>> - np = dt_new(opal_node, "epow");
>> - dt_add_property_strings(np, "compatible", "ibm,opal-v3-epow");
>> - dt_add_property_strings(np, "epow-classes", "power", "temperature", "cooling");
>> - prlog(PR_TRACE, PREFIX "FSP EPOW support initialized\n");
>> +
>> + /* Register host OPAL EPOW interface */
>> + opal_register(OPAL_GET_EPOW_STATUS, opal_get_epow_status, 2);
> Having fsp in function name makes more sense as this is the FSP handler
> for this API.
ok
>
>> +
>> + /* 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 ?
EPOW API is generic enough to take care of EPOW classes passed and thus
versioning
isn't needed.
Changing version helps kvm driver work with modified EPOW driver,
so both scenarios
addressed with one change.
>
>> + dt_add_property_strings(node_epow, "epow-classes", "power", "temp",
>> + "cooling");
>> +
>> + prlog(PR_INFO, PREFIX "FSP EPOW support initialized\n");
>> }
>> diff --git a/hw/fsp/fsp-epow.h b/hw/fsp/fsp-epow.h
>> index 24a0ae9..7dfd8c6 100644
>> --- a/hw/fsp/fsp-epow.h
>> +++ b/hw/fsp/fsp-epow.h
>> @@ -14,27 +15,34 @@
>> * limitations under the License.
>> */
>>
>> -/*
>> - * Handle FSP EPOW event notifications
>> - */
>> +/* FSP EPOW (Early Power Off Warning) support */
>>
>> #ifndef __FSP_EPOW_H
>> #define __FSP_EPOW_H
>>
>> -/* FSP based EPOW event notifications */
>> -#define EPOW_NORMAL 0x00 /* panel status normal */
>> -#define EPOW_EX1 0x01 /* panel status extended 1 */
>> -#define EPOW_EX2 0x02 /* Panel status extended 2 */
>> +/* Timeout values for various EPOW events */
>> +#define TIMEOUT_EPOW_POWER_UPS 900
>> +#define TIMEOUT_EPOW_POWER_UPS_LOW 20
>> +#define TIMEOUT_EPOW_TEMP_HIGH_AMB 900
>> +#define TIMEOUT_EPOW_TEMP_CRIT_AMB 20
>> +#define TIMEOUT_EPOW_TEMP_HIGH_INT 900
>> +#define TIMEOUT_EPOW_TEMP_CRIT_INT 20
> Please specify units somewhere. These are just from FSP spec? We can't
> interrogate FSP at all for these?
Timeout values in seconds. Mentioning timeout unit in comment above
macros seems ok ?
As below
/* Timeout values (in seconds) for various EPOW events. */
#define TIMEOUT_EPOW_POWER_UPS 900
>
>> -/* SPCN notifications */
>> -#define SPCN_CNF_CHNG 0x08 /* SPCN configuration change */
>> -#define SPCN_FAULT_LOG 0x04 /* SPCN fault to log */
>> -#define SPCN_POWR_FAIL 0x02 /* SPCN impending power failure */
>> -#define SPCN_INCL_POWR 0x01 /* SPCN incomplete power */
>> +/* FSP Panel Status type */
>> +#define PANEL_NORMAL 0 /* Normal Panel */
>> +#define PANEL_EX1 1 /* Extended Panel 1 */
>> +#define PANEL_EX2 2 /* Extended Panel 2 */
>>
>> -/* EPOW reason code notifications */
>> +/* EPOW reason codes */
>> +#define EPOW_NONE 0 /* EPOW normal/reset */
>> #define EPOW_ON_UPS 1 /* System on UPS */
>> -#define EPOW_TMP_AMB 2 /* Over ambient temperature */
>> -#define EPOW_TMP_INT 3 /* Over internal temperature */
>> +#define EPOW_AMB_TEMP 2 /* Over ambient temperature */
>> +#define EPOW_INT_TEMP 3 /* Over internal temperature */
>> +
>> +/* UPS bits mask used in EPOW notifications */
>> +#define UPS_PRESENT 0x80 /* UPS present */
>> +#define UPS_UTILITY_FAIL 0x40 /* UPS utility fail */
>> +#define UPS_BYPASED 0x20 /* UPS bypassed */
>> +#define UPS_BATTERY_LOW 0x10 /* UPS battery low */
>>
>> #endif
>> diff --git a/include/opal-api.h b/include/opal-api.h
>> index 1698311..12e427c 100644
>> --- a/include/opal-api.h
>> +++ b/include/opal-api.h
>> @@ -1,10 +1,11 @@
>> -/* Copyright 2013-2014 IBM Corp.
>> +/*
>> + * Copyright 2013-2014 IBM Corp.
>> *
>> * Licensed under the Apache License, Version 2.0 (the "License");
>> * you may not use this file except in compliance with the License.
>> * You may obtain a copy of the License at
>> *
>> - * http://www.apache.org/licenses/LICENSE-2.0
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> *
>> * Unless required by applicable law or agreed to in writing, software
>> * distributed under the License is distributed on an "AS IS" BASIS,
> Only change if making consistent with other files, and even then, as a
> separate patch.
Agreed.
>
>> @@ -384,13 +385,6 @@ enum OpalSlotLedState {
>> OPAL_SLOT_LED_STATE_ON = 1 /* LED is ON */
>> };
>>
>> -enum OpalEpowStatus {
>> - OPAL_EPOW_NONE = 0,
>> - OPAL_EPOW_UPS = 1,
>> - OPAL_EPOW_OVER_AMBIENT_TEMP = 2,
>> - OPAL_EPOW_OVER_INTERNAL_TEMP = 3
>> -};
>> -
> So... is the API changing or not? I'm a bit unclear now and it seems
> non-obvious from the changes to opal-api.h
Cleanup done. Not used and not useful.
>
>> enum OpalCheckTokenStatus {
>> OPAL_TOKEN_ABSENT = 0,
>> OPAL_TOKEN_PRESENT = 1
>> @@ -444,46 +438,6 @@ struct opal_ipmi_msg {
>> uint8_t data[];
>> };
>>
>> -/*
>> - * EPOW status sharing (OPAL and the host)
>> - *
>> - * The host will pass on OPAL, a buffer of length OPAL_SYSEPOW_MAX
>> - * with individual elements being 16 bits wide to fetch the system
>> - * wide EPOW status. Each element in the buffer will contain the
>> - * EPOW status in it's bit representation for a particular EPOW sub
>> - * class as defiend here. So multiple detailed EPOW status bits
>> - * specific for any sub class can be represented in a single buffer
>> - * element as it's bit representation.
>> - */
>> -
>> -/* System EPOW type */
>> -enum OpalSysEpow {
>> - OPAL_SYSEPOW_POWER = 0, /* Power EPOW */
>> - OPAL_SYSEPOW_TEMP = 1, /* Temperature EPOW */
>> - OPAL_SYSEPOW_COOLING = 2, /* Cooling EPOW */
>> - OPAL_SYSEPOW_MAX = 3, /* Max EPOW categories */
>> -};
>> -
>> -/* Power EPOW */
>> -enum OpalSysPower {
>> - OPAL_SYSPOWER_UPS = 0x0001, /* System on UPS power */
>> - OPAL_SYSPOWER_CHNG = 0x0002, /* System power configuration change */
>> - OPAL_SYSPOWER_FAIL = 0x0004, /* System impending power failure */
>> - OPAL_SYSPOWER_INCL = 0x0008, /* System incomplete power */
>> -};
>> -
>> -/* Temperature EPOW */
>> -enum OpalSysTemp {
>> - OPAL_SYSTEMP_AMB = 0x0001, /* System over ambient temperature */
>> - OPAL_SYSTEMP_INT = 0x0002, /* System over internal temperature */
>> - OPAL_SYSTEMP_HMD = 0x0004, /* System over ambient humidity */
>> -};
>> -
>> -/* Cooling EPOW */
>> -enum OpalSysCooling {
>> - OPAL_SYSCOOL_INSF = 0x0001, /* System insufficient cooling */
>> -};
>> -
>> /* FSP memory errors handling */
>> enum OpalMemErr_Version {
>> OpalMemErr_V1 = 1,
>> @@ -941,6 +895,36 @@ struct opal_i2c_request {
>> __be64 buffer_ra; /* Buffer real address */
>> };
>>
>> +/*
>> + * EPOW status sharing (OPAL and the host)
>> + *
>> + * The host will pass on OPAL, a buffer of length OPAL_EPOW_MAX_CLASSES
>> + * to fetch system wide EPOW status. Each element in the returned buffer
>> + * will contain bitwise EPOW status for each EPOW sub class.
>> + */
>> +
>> +/* 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.
Remoed ones like OpalSysPower OpalSysTemp as they contain events that don't
cause any EPOW with some of events values not available in HW or not
directly match.
Like OPAL_SYSPOWER_INCL tells that there is imcomplete power to system
and likely that
out of two power supply sources one is disconnected but that doesn't
cause any EPOW.
OPAL_SYSTEMP_HM is not available in HW. OPAL_SYSTEMP_AMB doesn't
match with any
EPOW event. We have Temp EPOW for High Ambient temp or Critical temp but
it doesn't tell
which one, and will have to be forcefully used.
Naming of these enums also suggests that they are representing
Generic System Power
or temp state and not just EPOW scenarios which are Power Off events.
Added new enums representing Power, Temp EPOWs cases and matching with
HW details
available on EPOW.
More information about the Skiboot
mailing list