| Message ID | 20170623113923.32133-1-hegdevasant@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jun 23, 2017 at 9:09 PM, Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote: > Add IPMI sensor data under /bmc node. This doesn't add any tests. How did you test it? Which machines support this feature? > > CC: Joel Stanley <joel@jms.id.au> > Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > --- > hdata/fsp.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- > hdata/spira.c | 1 + > hdata/spira.h | 24 +++++++++++++++++++++++- > 3 files changed, 74 insertions(+), 2 deletions(-) > > diff --git a/hdata/fsp.c b/hdata/fsp.c > index 6953d97..b941ee2 100644 > --- a/hdata/fsp.c > +++ b/hdata/fsp.c > @@ -280,6 +280,53 @@ static void add_uart(const struct spss_iopath *iopath, struct dt_node *lpc) > be32_to_cpu(iopath->lpc.uart_baud)); > } > > +static void add_ipmi_sensors(struct dt_node *bmc_node) > +{ > + int i; > + const void *hdif_sensor; > + struct dt_node *sensors_node, *sensor_node; > + const struct ipmi_sensors *ipmi_sensors; > + > + hdif_sensor = get_hdif(&spira.ntuples.ipmi_sensor, IPMI_SENSORS_HDIF_SIG); What is hdif? (Oh, I just noticed that this is FSP stuff. I guess that's why it makes no sense to me?) > + if (!hdif_sensor) { > + prlog(PR_DEBUG, "SENSORS: Invalid data\n"); > + return; > + } > + > + ipmi_sensors = HDIF_get_idata(hdif_sensor, IPMI_SENSORS_IDATA_SENSORS, NULL); > + if (!ipmi_sensors) { > + prlog(PR_DEBUG, "SENSORS: bad data\n"); > + return; > + } > + > + sensors_node = dt_new(bmc_node, "sensors"); > + assert(sensors_node); > + > + dt_add_property_cells(sensors_node, "#address-cells", 1); > + dt_add_property_cells(sensors_node, "#size-cells", 0); > + > + for (i = 0; i < be32_to_cpu(ipmi_sensors->count); i++) { > + if(dt_find_by_name_addr(sensors_node, "sensor", if (condition) > + ipmi_sensors->data[i].id)) { > + prlog(PR_WARNING, "SENSORS: Duplicate sensor ID : %x\n", > + ipmi_sensors->data[i].id); What causes a duplicate sensor ID? What can the user do with this information? > + continue; > + } > + > + /* We support only < MAX_IPMI_SENSORS sensors */ > + if (ipmi_sensors->data[i].type > 0xfe) use MAX_IPMI_SENSORS. ie. if (!(ipmi_sensors->data[i].type < MAX_IPMI_SENSORS)) > + continue; > + > + sensor_node = dt_new_addr(sensors_node, "sensor", > + ipmi_sensors->data[i].id); > + assert(sensor_node); > + dt_add_property_string(sensor_node, "compatible", "ibm,ipmi-sensor"); > + dt_add_property_cells(sensor_node, "reg", ipmi_sensors->data[i].id); > + dt_add_property_cells(sensor_node, "ipmi-sensor-type", > + ipmi_sensors->data[i].type); > + } > +} > + > static void bmc_create_node(const struct HDIF_common_hdr *sp) > { > struct dt_node *bmc_node; > @@ -297,7 +344,9 @@ static void bmc_create_node(const struct HDIF_common_hdr *sp) > dt_add_property_cells(bmc_node, "#address-cells", 1); > dt_add_property_cells(bmc_node, "#size-cells", 0); > > - /* TODO: add sensor info under /bmc */ > + /* Add sensor info under /bmc */ > + add_ipmi_sensors(bmc_node); > + > sp_impl = HDIF_get_idata(sp, SPSS_IDATA_SP_IMPL, &size); > if (CHECK_SPPTR(sp_impl) && (size > 8)) { > dt_add_property_strings(bmc_node, "compatible", sp_impl->sp_family); > diff --git a/hdata/spira.c b/hdata/spira.c > index fd7f351..601c1b3 100644 > --- a/hdata/spira.c > +++ b/hdata/spira.c > @@ -1234,6 +1234,7 @@ static void fixup_spira(void) > spira.ntuples.pcia = spiras->ntuples.pcia; > spira.ntuples.proc_chip = spiras->ntuples.proc_chip; > spira.ntuples.hs_data = spiras->ntuples.hs_data; > + spira.ntuples.ipmi_sensor = spiras->ntuples.ipmi_sensor; > } > > int parse_hdat(bool is_opal) > diff --git a/hdata/spira.h b/hdata/spira.h > index bb7ad3e..f141b72 100644 > --- a/hdata/spira.h > +++ b/hdata/spira.h > @@ -68,6 +68,7 @@ struct spira_ntuples { > struct spira_ntuple pcia; /* 0x2e0 */ > struct spira_ntuple proc_chip; /* 0x300 */ > struct spira_ntuple hs_data; /* 0x320 */ > + struct spira_ntuple ipmi_sensor; /* 0x360 */ > }; > > struct spira { > @@ -81,7 +82,7 @@ struct spira { > * > * According to FSP engineers, this is an okay thing to do. > */ > - u8 reserved[0xc0]; > + u8 reserved[0xa0]; > } __packed __align(0x100); > > extern struct spira spira; > @@ -1063,6 +1064,27 @@ struct sppcrd_chip_tod { > /* Idata index 0 : System attribute data */ > #define HSERV_IDATA_SYS_ATTR 0 > > +/* IPMI sensors mapping data */ > +#define IPMI_SENSORS_HDIF_SIG "FRUSE " What does FRUSE mean? What is it used for? > + > +/* Idata index 0 : Sensor mapping data */ > +#define IPMI_SENSORS_IDATA_SENSORS 0 > + > +struct ipmi_sensors_data { > + __be32 slca_index; > + uint8_t type; > + uint8_t id; > + __be16 reserved; > +}; Perhaps do this so you don't get padding: struct ipmi_sensors_data { __be32 slca_index; uint8_t type; uint8_t id; uint8_t reserved[2]; }; And add a packed annotation so that we don't get holes. > + > +struct ipmi_sensors { > + __be32 count; > + struct ipmi_sensors_data data[]; > +}; > + > +/* Idata index 1 : LED - sensors ID mapping data */ > +#define IPMI_SENSORS_IDATA_LED 1 > + > static inline const char *cpu_state(u32 flags) > { > switch ((flags & CPU_ID_VERIFY_MASK) >> CPU_ID_VERIFY_SHIFT) { > -- > 2.9.3 >
On Mon, Jun 26, 2017 at 3:57 PM, Joel Stanley <joel@jms.id.au> wrote: > On Fri, Jun 23, 2017 at 9:09 PM, Vasant Hegde > <hegdevasant@linux.vnet.ibm.com> wrote: >> Add IPMI sensor data under /bmc node. > > This doesn't add any tests. > > How did you test it? > > Which machines support this feature? You grab a hdat dump from a system and feed it into hdata_to_dt and eyeball the result. The way we do hdat testing is pretty poor since it doesn't actually "test" anything, it just verifies that the output didn't change for a given input. We generally don't implement parsing of something until hostboot has implemented it so I've been resistant to adding a P9 HDAT blob to the repo since we would need to update it frequently and I don't want to be littering the repo with stale blobs. > >> >> CC: Joel Stanley <joel@jms.id.au> >> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >> --- >> hdata/fsp.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- >> hdata/spira.c | 1 + >> hdata/spira.h | 24 +++++++++++++++++++++++- >> 3 files changed, 74 insertions(+), 2 deletions(-) >> >> diff --git a/hdata/fsp.c b/hdata/fsp.c >> index 6953d97..b941ee2 100644 >> --- a/hdata/fsp.c >> +++ b/hdata/fsp.c >> @@ -280,6 +280,53 @@ static void add_uart(const struct spss_iopath *iopath, struct dt_node *lpc) >> be32_to_cpu(iopath->lpc.uart_baud)); >> } >> >> +static void add_ipmi_sensors(struct dt_node *bmc_node) >> +{ >> + int i; >> + const void *hdif_sensor; Shouldn't this be a struct HDIF_common_hdr *? >> + struct dt_node *sensors_node, *sensor_node; >> + const struct ipmi_sensors *ipmi_sensors; >> + >> + hdif_sensor = get_hdif(&spira.ntuples.ipmi_sensor, IPMI_SENSORS_HDIF_SIG); > > What is hdif? > > (Oh, I just noticed that this is FSP stuff. I guess that's why it > makes no sense to me?) Hypervisor Data Interface or something. It's just the name of the data structures that make up the HDAT. > >> + if (!hdif_sensor) { >> + prlog(PR_DEBUG, "SENSORS: Invalid data\n"); Can this say "missing IPMI sensor Mappings tuple" or something else that's a bit more descriptive? >> + return; >> + } >> + >> + ipmi_sensors = HDIF_get_idata(hdif_sensor, IPMI_SENSORS_IDATA_SENSORS, NULL); >> + if (!ipmi_sensors) { >> + prlog(PR_DEBUG, "SENSORS: bad data\n"); >> + return; >> + } >> + >> + sensors_node = dt_new(bmc_node, "sensors"); >> + assert(sensors_node); >> + >> + dt_add_property_cells(sensors_node, "#address-cells", 1); >> + dt_add_property_cells(sensors_node, "#size-cells", 0); >> + >> + for (i = 0; i < be32_to_cpu(ipmi_sensors->count); i++) { >> + if(dt_find_by_name_addr(sensors_node, "sensor", > > if (condition) > >> + ipmi_sensors->data[i].id)) { >> + prlog(PR_WARNING, "SENSORS: Duplicate sensor ID : %x\n", >> + ipmi_sensors->data[i].id); > > What causes a duplicate sensor ID? > > What can the user do with this information? Duplicate IDs indicate bad input data. I'm not exactly sure where the IPMI sensor list is sourced from, but I imagine it's the MRW. We want to be loud about it so vendors will see it and hopefully fix it since getting the MRW right isn't something that we (the OPAL team) shouldn't have to be involved in. > >> + continue; >> + } >> + >> + /* We support only < MAX_IPMI_SENSORS sensors */ >> + if (ipmi_sensors->data[i].type > 0xfe) > > use MAX_IPMI_SENSORS. > > ie. > > if (!(ipmi_sensors->data[i].type < MAX_IPMI_SENSORS)) > Given sensor type is a single byte field we should just check that it's not 0xff. Maybe #define that as IPMI_SENSOR_INVALID or some such. >> + continue; >> + >> + sensor_node = dt_new_addr(sensors_node, "sensor", >> + ipmi_sensors->data[i].id); >> + assert(sensor_node); >> + dt_add_property_string(sensor_node, "compatible", "ibm,ipmi-sensor"); >> + dt_add_property_cells(sensor_node, "reg", ipmi_sensors->data[i].id); >> + dt_add_property_cells(sensor_node, "ipmi-sensor-type", >> + ipmi_sensors->data[i].type); >> + } >> +} >> + >> static void bmc_create_node(const struct HDIF_common_hdr *sp) >> { >> struct dt_node *bmc_node; >> @@ -297,7 +344,9 @@ static void bmc_create_node(const struct HDIF_common_hdr *sp) >> dt_add_property_cells(bmc_node, "#address-cells", 1); >> dt_add_property_cells(bmc_node, "#size-cells", 0); >> >> - /* TODO: add sensor info under /bmc */ >> + /* Add sensor info under /bmc */ >> + add_ipmi_sensors(bmc_node); >> + >> sp_impl = HDIF_get_idata(sp, SPSS_IDATA_SP_IMPL, &size); >> if (CHECK_SPPTR(sp_impl) && (size > 8)) { >> dt_add_property_strings(bmc_node, "compatible", sp_impl->sp_family); >> diff --git a/hdata/spira.c b/hdata/spira.c >> index fd7f351..601c1b3 100644 >> --- a/hdata/spira.c >> +++ b/hdata/spira.c >> @@ -1234,6 +1234,7 @@ static void fixup_spira(void) >> spira.ntuples.pcia = spiras->ntuples.pcia; >> spira.ntuples.proc_chip = spiras->ntuples.proc_chip; >> spira.ntuples.hs_data = spiras->ntuples.hs_data; >> + spira.ntuples.ipmi_sensor = spiras->ntuples.ipmi_sensor; >> } >> >> int parse_hdat(bool is_opal) >> diff --git a/hdata/spira.h b/hdata/spira.h >> index bb7ad3e..f141b72 100644 >> --- a/hdata/spira.h >> +++ b/hdata/spira.h >> @@ -68,6 +68,7 @@ struct spira_ntuples { >> struct spira_ntuple pcia; /* 0x2e0 */ >> struct spira_ntuple proc_chip; /* 0x300 */ >> struct spira_ntuple hs_data; /* 0x320 */ >> + struct spira_ntuple ipmi_sensor; /* 0x360 */ >> }; >> >> struct spira { >> @@ -81,7 +82,7 @@ struct spira { >> * >> * According to FSP engineers, this is an okay thing to do. >> */ >> - u8 reserved[0xc0]; >> + u8 reserved[0xa0]; >> } __packed __align(0x100); >> >> extern struct spira spira; >> @@ -1063,6 +1064,27 @@ struct sppcrd_chip_tod { >> /* Idata index 0 : System attribute data */ >> #define HSERV_IDATA_SYS_ATTR 0 >> >> +/* IPMI sensors mapping data */ >> +#define IPMI_SENSORS_HDIF_SIG "FRUSE " > > What does FRUSE mean? It's probably "FRU Sensors" squashed into six characters. No idea why there's a space at the end though... > > What is it used for? It's the ID string for this type of HDIF structure. The ID strings are defined in the HDAT spec so there's not much we can do about them. 'FRUSE ' is pretty bad, but too late now. > >> + >> +/* Idata index 0 : Sensor mapping data */ >> +#define IPMI_SENSORS_IDATA_SENSORS 0 >> + >> +struct ipmi_sensors_data { >> + __be32 slca_index; >> + uint8_t type; >> + uint8_t id; >> + __be16 reserved; >> +}; > > Perhaps do this so you don't get padding: > > struct ipmi_sensors_data { > __be32 slca_index; > uint8_t type; > uint8_t id; > uint8_t reserved[2]; > }; > > And add a packed annotation so that we don't get holes. Just using the packed annotation should solve both problems. Looks fine otherwise. Thanks for implementing this Vasant. Oliver
On Fri, Jun 23, 2017 at 05:09:23PM +0530, Vasant Hegde wrote: > Add IPMI sensor data under /bmc node. > > CC: Joel Stanley <joel@jms.id.au> > Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> The firmware folks provided a test HB pnor on a system and I was able to see the sensors: ubuntu@w15l:/proc/device-tree/bmc/sensors$ ls -l total 0 -r--r--r-- 1 root root 4 Jun 28 08:41 #address-cells -r--r--r-- 1 root root 8 Jun 28 08:41 name -r--r--r-- 1 root root 4 Jun 28 08:41 phandle drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@1 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@10 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@2 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@3 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@4 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@5 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@59 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@5a drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@6 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@7 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@8 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@9 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@a drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@a5 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@a6 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@a7 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@a8 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@a9 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@aa drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@ab drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@ac drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@ad drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@ae drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@b drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@b3 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@b4 drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@c drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@d drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@e drwxr-xr-x 2 root root 0 Jun 28 08:41 sensor@f -r--r--r-- 1 root root 4 Jun 28 08:41 #size-cells ubuntu@w15l:/proc/device-tree/bmc/sensors$ hexdump -C sensor@1/ipmi-sensor-type 00000000 00 00 00 12 |....| 00000004 ubuntu@w15l:/proc/device-tree/bmc/sensors$ hexdump -C sensor@3/ipmi-sensor-type 00000000 00 00 00 0f |....| 00000004 ubuntu@w15l:/proc/device-tree/bmc/sensors$ hexdump -C sensor@5/ipmi-sensor-type 00000000 00 00 00 1f |....| 00000004
On 06/28/2017 07:17 PM, Ananth N Mavinakayanahalli wrote: > On Fri, Jun 23, 2017 at 05:09:23PM +0530, Vasant Hegde wrote: >> Add IPMI sensor data under /bmc node. >> >> CC: Joel Stanley <joel@jms.id.au> >> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > > The firmware folks provided a test HB pnor on a system and I was able to > see the sensors: DT output looks good. Thanks for the verification. -Vasant
On 06/26/2017 11:27 AM, Joel Stanley wrote: > On Fri, Jun 23, 2017 at 9:09 PM, Vasant Hegde > <hegdevasant@linux.vnet.ibm.com> wrote: >> Add IPMI sensor data under /bmc node. > > This doesn't add any tests. > > How did you test it? > > Which machines support this feature? Joel, In P8 BMC system, hostboot provided these sensors details. In P9 they will be passing this information via HDAT and we will build the device tree. I think hostboot side code merged recently. This feature is supported on all P9 BMC machines. > >> >> CC: Joel Stanley <joel@jms.id.au> >> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >> --- >> hdata/fsp.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- >> hdata/spira.c | 1 + >> hdata/spira.h | 24 +++++++++++++++++++++++- >> 3 files changed, 74 insertions(+), 2 deletions(-) >> >> diff --git a/hdata/fsp.c b/hdata/fsp.c >> index 6953d97..b941ee2 100644 >> --- a/hdata/fsp.c >> +++ b/hdata/fsp.c >> @@ -280,6 +280,53 @@ static void add_uart(const struct spss_iopath *iopath, struct dt_node *lpc) >> be32_to_cpu(iopath->lpc.uart_baud)); >> } >> >> +static void add_ipmi_sensors(struct dt_node *bmc_node) >> +{ >> + int i; >> + const void *hdif_sensor; >> + struct dt_node *sensors_node, *sensor_node; >> + const struct ipmi_sensors *ipmi_sensors; >> + >> + hdif_sensor = get_hdif(&spira.ntuples.ipmi_sensor, IPMI_SENSORS_HDIF_SIG); > > What is hdif? > > (Oh, I just noticed that this is FSP stuff. I guess that's why it > makes no sense to me?) Yes :-) HDIF is part of HDAT specification, not FSP specific anymore. > >> + if (!hdif_sensor) { >> + prlog(PR_DEBUG, "SENSORS: Invalid data\n"); >> + return; >> + } >> + >> + ipmi_sensors = HDIF_get_idata(hdif_sensor, IPMI_SENSORS_IDATA_SENSORS, NULL); >> + if (!ipmi_sensors) { >> + prlog(PR_DEBUG, "SENSORS: bad data\n"); >> + return; >> + } >> + >> + sensors_node = dt_new(bmc_node, "sensors"); >> + assert(sensors_node); >> + >> + dt_add_property_cells(sensors_node, "#address-cells", 1); >> + dt_add_property_cells(sensors_node, "#size-cells", 0); >> + >> + for (i = 0; i < be32_to_cpu(ipmi_sensors->count); i++) { >> + if(dt_find_by_name_addr(sensors_node, "sensor", > > if (condition) > >> + ipmi_sensors->data[i].id)) { >> + prlog(PR_WARNING, "SENSORS: Duplicate sensor ID : %x\n", >> + ipmi_sensors->data[i].id); > > What causes a duplicate sensor ID? FW ! > > What can the user do with this information? No use for end user. But it kind of tells us the we have a bug in FW . > >> + continue; >> + } >> + >> + /* We support only < MAX_IPMI_SENSORS sensors */ >> + if (ipmi_sensors->data[i].type > 0xfe) > > use MAX_IPMI_SENSORS. > > ie. > > if (!(ipmi_sensors->data[i].type < MAX_IPMI_SENSORS)) Agreed. I should have used that macro . Will fix in v2. > >> + continue; >> + >> + sensor_node = dt_new_addr(sensors_node, "sensor", >> + ipmi_sensors->data[i].id); >> + assert(sensor_node); >> + dt_add_property_string(sensor_node, "compatible", "ibm,ipmi-sensor"); >> + dt_add_property_cells(sensor_node, "reg", ipmi_sensors->data[i].id); >> + dt_add_property_cells(sensor_node, "ipmi-sensor-type", >> + ipmi_sensors->data[i].type); >> + } >> +} >> + >> static void bmc_create_node(const struct HDIF_common_hdr *sp) >> { >> struct dt_node *bmc_node; >> @@ -297,7 +344,9 @@ static void bmc_create_node(const struct HDIF_common_hdr *sp) >> dt_add_property_cells(bmc_node, "#address-cells", 1); >> dt_add_property_cells(bmc_node, "#size-cells", 0); >> >> - /* TODO: add sensor info under /bmc */ >> + /* Add sensor info under /bmc */ >> + add_ipmi_sensors(bmc_node); >> + >> sp_impl = HDIF_get_idata(sp, SPSS_IDATA_SP_IMPL, &size); >> if (CHECK_SPPTR(sp_impl) && (size > 8)) { >> dt_add_property_strings(bmc_node, "compatible", sp_impl->sp_family); >> diff --git a/hdata/spira.c b/hdata/spira.c >> index fd7f351..601c1b3 100644 >> --- a/hdata/spira.c >> +++ b/hdata/spira.c >> @@ -1234,6 +1234,7 @@ static void fixup_spira(void) >> spira.ntuples.pcia = spiras->ntuples.pcia; >> spira.ntuples.proc_chip = spiras->ntuples.proc_chip; >> spira.ntuples.hs_data = spiras->ntuples.hs_data; >> + spira.ntuples.ipmi_sensor = spiras->ntuples.ipmi_sensor; >> } >> >> int parse_hdat(bool is_opal) >> diff --git a/hdata/spira.h b/hdata/spira.h >> index bb7ad3e..f141b72 100644 >> --- a/hdata/spira.h >> +++ b/hdata/spira.h >> @@ -68,6 +68,7 @@ struct spira_ntuples { >> struct spira_ntuple pcia; /* 0x2e0 */ >> struct spira_ntuple proc_chip; /* 0x300 */ >> struct spira_ntuple hs_data; /* 0x320 */ >> + struct spira_ntuple ipmi_sensor; /* 0x360 */ >> }; >> >> struct spira { >> @@ -81,7 +82,7 @@ struct spira { >> * >> * According to FSP engineers, this is an okay thing to do. >> */ >> - u8 reserved[0xc0]; >> + u8 reserved[0xa0]; >> } __packed __align(0x100); >> >> extern struct spira spira; >> @@ -1063,6 +1064,27 @@ struct sppcrd_chip_tod { >> /* Idata index 0 : System attribute data */ >> #define HSERV_IDATA_SYS_ATTR 0 >> >> +/* IPMI sensors mapping data */ >> +#define IPMI_SENSORS_HDIF_SIG "FRUSE " > > What does FRUSE mean? > > What is it used for? Again its part of spec, we use FRUSE to identify the section. > >> + >> +/* Idata index 0 : Sensor mapping data */ >> +#define IPMI_SENSORS_IDATA_SENSORS 0 >> + >> +struct ipmi_sensors_data { >> + __be32 slca_index; >> + uint8_t type; >> + uint8_t id; >> + __be16 reserved; >> +}; > > Perhaps do this so you don't get padding: > > struct ipmi_sensors_data { > __be32 slca_index; > uint8_t type; > uint8_t id; > uint8_t reserved[2]; > }; > > And add a packed annotation so that we don't get holes. Agree. I missed to add packed annotation. Thank! -Vasant
On 06/26/2017 12:55 PM, Oliver wrote: > On Mon, Jun 26, 2017 at 3:57 PM, Joel Stanley <joel@jms.id.au> wrote: >> On Fri, Jun 23, 2017 at 9:09 PM, Vasant Hegde >> <hegdevasant@linux.vnet.ibm.com> wrote: >>> Add IPMI sensor data under /bmc node. .../... >>> >>> +static void add_ipmi_sensors(struct dt_node *bmc_node) >>> +{ >>> + int i; > >>> + const void *hdif_sensor; > > Shouldn't this be a struct HDIF_common_hdr *? Yes. .../... >> >>> + if (!hdif_sensor) { >>> + prlog(PR_DEBUG, "SENSORS: Invalid data\n"); > > Can this say "missing IPMI sensor Mappings tuple" or something else > that's a bit more descriptive? Sure. Will fix in v2. .../... >> >>> + continue; >>> + } >>> + >>> + /* We support only < MAX_IPMI_SENSORS sensors */ >>> + if (ipmi_sensors->data[i].type > 0xfe) >> >> use MAX_IPMI_SENSORS. >> >> ie. >> >> if (!(ipmi_sensors->data[i].type < MAX_IPMI_SENSORS)) >> > > Given sensor type is a single byte field we should just check that > it's not 0xff. Maybe #define that as IPMI_SENSOR_INVALID or some > such. Better to use MAX_IPMI_SENSORS so that any changes in count reflects everywhere. .../... >> >>> + >>> +/* Idata index 0 : Sensor mapping data */ >>> +#define IPMI_SENSORS_IDATA_SENSORS 0 >>> + >>> +struct ipmi_sensors_data { >>> + __be32 slca_index; >>> + uint8_t type; >>> + uint8_t id; >>> + __be16 reserved; >>> +}; >> >> Perhaps do this so you don't get padding: >> >> struct ipmi_sensors_data { >> __be32 slca_index; >> uint8_t type; >> uint8_t id; >> uint8_t reserved[2]; >> }; >> >> And add a packed annotation so that we don't get holes. > Just using the packed annotation should solve both problems. Yes. I missed to add it in v1. Will fix in v2. -Vasant
diff --git a/hdata/fsp.c b/hdata/fsp.c index 6953d97..b941ee2 100644 --- a/hdata/fsp.c +++ b/hdata/fsp.c @@ -280,6 +280,53 @@ static void add_uart(const struct spss_iopath *iopath, struct dt_node *lpc) be32_to_cpu(iopath->lpc.uart_baud)); } +static void add_ipmi_sensors(struct dt_node *bmc_node) +{ + int i; + const void *hdif_sensor; + struct dt_node *sensors_node, *sensor_node; + const struct ipmi_sensors *ipmi_sensors; + + hdif_sensor = get_hdif(&spira.ntuples.ipmi_sensor, IPMI_SENSORS_HDIF_SIG); + if (!hdif_sensor) { + prlog(PR_DEBUG, "SENSORS: Invalid data\n"); + return; + } + + ipmi_sensors = HDIF_get_idata(hdif_sensor, IPMI_SENSORS_IDATA_SENSORS, NULL); + if (!ipmi_sensors) { + prlog(PR_DEBUG, "SENSORS: bad data\n"); + return; + } + + sensors_node = dt_new(bmc_node, "sensors"); + assert(sensors_node); + + dt_add_property_cells(sensors_node, "#address-cells", 1); + dt_add_property_cells(sensors_node, "#size-cells", 0); + + for (i = 0; i < be32_to_cpu(ipmi_sensors->count); i++) { + if(dt_find_by_name_addr(sensors_node, "sensor", + ipmi_sensors->data[i].id)) { + prlog(PR_WARNING, "SENSORS: Duplicate sensor ID : %x\n", + ipmi_sensors->data[i].id); + continue; + } + + /* We support only < MAX_IPMI_SENSORS sensors */ + if (ipmi_sensors->data[i].type > 0xfe) + continue; + + sensor_node = dt_new_addr(sensors_node, "sensor", + ipmi_sensors->data[i].id); + assert(sensor_node); + dt_add_property_string(sensor_node, "compatible", "ibm,ipmi-sensor"); + dt_add_property_cells(sensor_node, "reg", ipmi_sensors->data[i].id); + dt_add_property_cells(sensor_node, "ipmi-sensor-type", + ipmi_sensors->data[i].type); + } +} + static void bmc_create_node(const struct HDIF_common_hdr *sp) { struct dt_node *bmc_node; @@ -297,7 +344,9 @@ static void bmc_create_node(const struct HDIF_common_hdr *sp) dt_add_property_cells(bmc_node, "#address-cells", 1); dt_add_property_cells(bmc_node, "#size-cells", 0); - /* TODO: add sensor info under /bmc */ + /* Add sensor info under /bmc */ + add_ipmi_sensors(bmc_node); + sp_impl = HDIF_get_idata(sp, SPSS_IDATA_SP_IMPL, &size); if (CHECK_SPPTR(sp_impl) && (size > 8)) { dt_add_property_strings(bmc_node, "compatible", sp_impl->sp_family); diff --git a/hdata/spira.c b/hdata/spira.c index fd7f351..601c1b3 100644 --- a/hdata/spira.c +++ b/hdata/spira.c @@ -1234,6 +1234,7 @@ static void fixup_spira(void) spira.ntuples.pcia = spiras->ntuples.pcia; spira.ntuples.proc_chip = spiras->ntuples.proc_chip; spira.ntuples.hs_data = spiras->ntuples.hs_data; + spira.ntuples.ipmi_sensor = spiras->ntuples.ipmi_sensor; } int parse_hdat(bool is_opal) diff --git a/hdata/spira.h b/hdata/spira.h index bb7ad3e..f141b72 100644 --- a/hdata/spira.h +++ b/hdata/spira.h @@ -68,6 +68,7 @@ struct spira_ntuples { struct spira_ntuple pcia; /* 0x2e0 */ struct spira_ntuple proc_chip; /* 0x300 */ struct spira_ntuple hs_data; /* 0x320 */ + struct spira_ntuple ipmi_sensor; /* 0x360 */ }; struct spira { @@ -81,7 +82,7 @@ struct spira { * * According to FSP engineers, this is an okay thing to do. */ - u8 reserved[0xc0]; + u8 reserved[0xa0]; } __packed __align(0x100); extern struct spira spira; @@ -1063,6 +1064,27 @@ struct sppcrd_chip_tod { /* Idata index 0 : System attribute data */ #define HSERV_IDATA_SYS_ATTR 0 +/* IPMI sensors mapping data */ +#define IPMI_SENSORS_HDIF_SIG "FRUSE " + +/* Idata index 0 : Sensor mapping data */ +#define IPMI_SENSORS_IDATA_SENSORS 0 + +struct ipmi_sensors_data { + __be32 slca_index; + uint8_t type; + uint8_t id; + __be16 reserved; +}; + +struct ipmi_sensors { + __be32 count; + struct ipmi_sensors_data data[]; +}; + +/* Idata index 1 : LED - sensors ID mapping data */ +#define IPMI_SENSORS_IDATA_LED 1 + static inline const char *cpu_state(u32 flags) { switch ((flags & CPU_ID_VERIFY_MASK) >> CPU_ID_VERIFY_SHIFT) {
Add IPMI sensor data under /bmc node. CC: Joel Stanley <joel@jms.id.au> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> --- hdata/fsp.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- hdata/spira.c | 1 + hdata/spira.h | 24 +++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-)