HDAT: Add IPMI sensor data under /bmc node

Message ID 20170623113923.32133-1-hegdevasant@linux.vnet.ibm.com
State New
Headers show

Commit Message

Vasant Hegde June 23, 2017, 11:39 a.m.
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(-)

Comments

Joel Stanley June 26, 2017, 5:57 a.m. | #1
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
>
Oliver June 26, 2017, 7:25 a.m. | #2
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
Ananth N Mavinakayanahalli June 28, 2017, 1:47 p.m. | #3
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
Vasant Hegde July 3, 2017, 9:46 a.m. | #4
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
Vasant Hegde July 3, 2017, 9:57 a.m. | #5
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
Vasant Hegde July 3, 2017, 10:03 a.m. | #6
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

Patch

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) {