[RFC PATCH linux v6 8/9] hwmon: occ: Add callbacks for parsing P9 OCC datastructures

Andrew Jeffery andrew at aj.id.au
Tue Dec 13 14:26:45 AEDT 2016


On Tue, 2016-11-29 at 17:04 -0600, eajames.ibm at gmail.com wrote:
> > From: "Edward A. James" <eajames at us.ibm.com>
> 

The usual here :) More words pointing to relevant documentation and
sections defining the returned data.

FWIW I don't actually know where this documentation is so can't verify
the behaviour.

The rest of the comments are mostly minor issues.

> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
>  drivers/hwmon/occ/occ_p9.c | 261 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_p9.h |  30 ++++++
>  2 files changed, 291 insertions(+)
>  create mode 100644 drivers/hwmon/occ/occ_p9.c
>  create mode 100644 drivers/hwmon/occ/occ_p9.h
> 
> diff --git a/drivers/hwmon/occ/occ_p9.c b/drivers/hwmon/occ/occ_p9.c
> new file mode 100644
> index 0000000..1109ab4
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_p9.c
> @@ -0,0 +1,261 @@
> +/*
> + * p9.c - OCC hwmon driver
> + *
> + * This file contains the Power9-specific methods and data structures for
> + * the OCC hwmon driver.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
> +#include "occ.h"
> +#include "occ_p9.h"
> +
> +/* P9 OCC sensor data format */
> +struct temp_sensor_p9 {
> +       u32 sensor_id;
> +       u8 fru_type;
> +       u8 value;
> +};

Whatever approach we apply for naming things in the P8 code we should
also apply here.

> +
> +struct freq_sensor_p9 {
> +       u32 sensor_id;
> +       u16 value;
> +};
> +
> +struct power_sensor_p9 {
> +       u32 sensor_id;
> +       u8 function_id;
> +       u8 apss_channel;
> +       u16 reserved;
> +       u32 update_tag;
> +       u64 accumulator;
> +       u16 value;
> +};
> +
> +struct caps_sensor_p9 {
> +       u16 curr_powercap;
> +       u16 curr_powerreading;
> +       u16 norm_powercap;
> +       u16 max_powercap;
> +       u16 min_powercap;
> +       u16 user_powerlimit;
> +       u8 user_powerlimit_source;
> +};
> +
> +/*
> +static char *caps_sensor_names[] = {
> +       "curr_powercap",
> +       "curr_powerreading",
> +       "norm_powercap",
> +       "max_powercap",
> +       "min_powercap",
> +       "user_powerlimit",
> +       "user_powerlimit_source"
> +};
> +*/
> +
> +void p9_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
> +                    int snum)
> +{
> +       switch (sensor_type) {
> +               case FREQ:
> +               {
> +                       struct freq_sensor_p9 *fs =
> +                               &(((struct freq_sensor_p9 *)sensor)[snum]);
> +                       fs->sensor_id =
> +                               be32_to_cpup((const __be32 *)&data[off]);
> +                       fs->value =
> +                               be16_to_cpup((const __be16 *)&data[off + 4]);
> +               }
> +                       break;
> +               case TEMP:
> +               {
> +                       struct temp_sensor_p9 *ts =
> +                               &(((struct temp_sensor_p9 *)sensor)[snum]);
> +                       ts->sensor_id =
> +                               be32_to_cpup((const __be32 *)&data[off]);
> +                       ts->fru_type = data[off + 4];
> +                       ts->value = data[off + 5];
> +               }
> +                       break;
> +               case POWER:
> +               {
> +                       struct power_sensor_p9 *ps =
> +                               &(((struct power_sensor_p9 *)sensor)[snum]);
> +                       ps->sensor_id =
> +                               be32_to_cpup((const __be32 *)&data[off]);
> +                       ps->function_id = data[off + 4];
> +                       ps->apss_channel = data[off + 5];
> +                       /* two bytes reserved */
> +                       ps->update_tag =
> +                               be32_to_cpup((const __be32 *)&data[off + 8]);
> +                       ps->accumulator =
> +                               be64_to_cpup((const __be64 *)&data[off + 12]);

This looks to be unaligned. Please fix it.

> +                       ps->value =
> +                               be16_to_cpup((const __be16 *)&data[off + 20]);
> +               }
> +                       break;
> +               case CAPS:
> +               {
> +                       struct caps_sensor_p9 *cs =
> +                               &(((struct caps_sensor_p9 *)sensor)[snum]);
> +                       cs->curr_powercap =
> +                               be16_to_cpup((const __be16 *)&data[off]);
> +                       cs->curr_powerreading =
> +                               be16_to_cpup((const __be16 *)&data[off + 2]);
> +                       cs->norm_powercap =
> +                               be16_to_cpup((const __be16 *)&data[off + 4]);
> +                       cs->max_powercap =
> +                               be16_to_cpup((const __be16 *)&data[off + 6]);
> +                       cs->min_powercap =
> +                               be16_to_cpup((const __be16 *)&data[off + 8]);
> +                       cs->user_powerlimit =
> +                               be16_to_cpup((const __be16 *)&data[off + 10]);
> +                       cs->user_powerlimit_source = data[off + 12];
> +               }
> +                       break;
> +       };
> +}
> +
> +void * p9_alloc_sensor(int sensor_type, int num_sensors)
> +{
> +       switch (sensor_type) {
> +               case FREQ:
> +                       return kcalloc(num_sensors,
> +                                      sizeof(struct freq_sensor_p9),
> +                                      GFP_KERNEL);
> +               case TEMP:
> +                       return kcalloc(num_sensors,
> +                                      sizeof(struct temp_sensor_p9),
> +                                      GFP_KERNEL);
> +               case POWER:
> +                       return kcalloc(num_sensors,
> +                                      sizeof(struct power_sensor_p9),
> +                                      GFP_KERNEL);
> +               case CAPS:
> +                       return kcalloc(num_sensors,
> +                                      sizeof(struct caps_sensor_p9),
> +                                      GFP_KERNEL);
> +               default:
> +                       return NULL;
> +       }
> +}
> +
> +int p9_get_sensor_value(struct occ *driver, int sensor_type, int snum)
> +{
> +       void *sensor;
> +
> +       if (sensor_type == CAPS)
> +               return -1;
> +
> +       sensor = occ_get_sensor(driver, sensor_type);
> +       if (!sensor)
> +               return -1;
> +
> +       switch (sensor_type) {
> +               case FREQ:
> +                       return ((struct freq_sensor_p9 *)sensor)[snum].value;
> +               case TEMP:
> +                       return ((struct temp_sensor_p9 *)sensor)[snum].value;
> +               case POWER:
> +                       return ((struct power_sensor_p9 *)sensor)[snum].value;
> +               default:
> +                       return -1;
> +       }
> +}
> +
> +int p9_get_sensor_id(struct occ *driver, int sensor_type, int snum)
> +{
> +       void *sensor;
> +       int i = snum;
> +
> +       if (sensor_type == CAPS)
> +               return -1;
> +
> +       sensor = occ_get_sensor(driver, sensor_type);
> +       if (!sensor)
> +               return -1;
> +
> +       switch (sensor_type) {
> +               case FREQ:
> +                       return ((struct freq_sensor_p9 *)sensor)[i].sensor_id;
> +               case TEMP:
> +                       return ((struct temp_sensor_p9 *)sensor)[i].sensor_id;
> +               case POWER:
> +                       return ((struct power_sensor_p9 *)sensor)[i].sensor_id;
> +               default:
> +                       return -1;
> +       }
> +}
> +
> +int p9_get_caps_value(void *sensor, int snum, int caps_field)
> +{
> +       struct caps_sensor_p9 *caps_sensor = sensor;
> +
> +       switch (caps_field) {
> +       case 0:
> +               return caps_sensor[snum].curr_powercap;
> +       case 1:
> +               return caps_sensor[snum].curr_powerreading;
> +       case 2:
> +               return caps_sensor[snum].norm_powercap;
> +       case 3:
> +               return caps_sensor[snum].max_powercap;
> +       case 4:
> +               return caps_sensor[snum].min_powercap;
> +       case 5:
> +               return caps_sensor[snum].user_powerlimit;
> +       case 6:
> +               return caps_sensor[snum].user_powerlimit_source;
> +       default:
> +               return -1;
> +       }
> +}
> +static const struct occ_ops p9_ops = {
> +       .parse_sensor = p9_parse_sensor,
> +       .alloc_sensor = p9_alloc_sensor,
> +       .get_sensor_value = p9_get_sensor_value,
> +       .get_sensor_id = p9_get_sensor_id,
> +       .get_caps_value = p9_get_caps_value,
> +};
> +
> +static const struct occ_config p9_config = {
> +       .command_addr = 0xFFFBE000,
> +       .response_addr = 0xFFFBF000,
> +};
> +
> +struct occ *occ_p9_start(struct device *dev, void *bus,
> +                        struct occ_bus_ops bus_ops)

Again we're copying bus_ops here. Can/should we take a pointer instead?

> +{
> +       return occ_start(dev, bus, bus_ops, p9_ops, p9_config);
> +}
> +EXPORT_SYMBOL(occ_p9_start);
> +
> +int occ_p9_stop(struct occ *occ)
> +{
> +       return occ_stop(occ);
> +}
> +EXPORT_SYMBOL(occ_p9_stop);
> +
> > +MODULE_AUTHOR("Eddie James <eajames at us.ibm.com>");
> +MODULE_DESCRIPTION("P9 OCC sensors");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_p9.h b/drivers/hwmon/occ/occ_p9.h
> new file mode 100644
> index 0000000..52b3827
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_p9.h
> @@ -0,0 +1,30 @@
> +/*
> + * p9.h - OCC hwmon driver
> + *
> + * This file contains Power9-specific function prototypes
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __P9_H__
> +#define __P9_H__

Picking on my own guard/file naming problem again :)

> +
> +#include "scom.h"
> +#include "occ.h"
> +
> +struct device;
> +
> +struct occ *occ_p9_start(struct device *dev, void *bus, struct occ_bus_ops bus_ops);
> +int occ_p9_stop(struct occ *occ);
> +
> +#endif /* __P9_H__ */

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161213/443825df/attachment-0001.sig>


More information about the openbmc mailing list