[Skiboot] [PATCH] sensors: occ: Support reading u64 sensor values
Stewart Smith
stewart at linux.vnet.ibm.com
Mon Nov 20 16:21:04 AEDT 2017
Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com> writes:
> This patch adds new OPAL call to read sensor values of type u64. It
> also exports energy sensors in device-tree.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
> ---
> core/sensor.c | 14 ++++
> hw/occ-sensor.c | 210 ++++++++++++++++++++++++++++++++++++++---------------
> include/opal-api.h | 3 +-
> include/skiboot.h | 2 +-
> 4 files changed, 169 insertions(+), 60 deletions(-)
>
> diff --git a/core/sensor.c b/core/sensor.c
> index 57b21bc..4eeed20 100644
> --- a/core/sensor.c
> +++ b/core/sensor.c
> @@ -41,6 +41,19 @@ static int64_t opal_sensor_read(uint32_t sensor_hndl, int token,
> return OPAL_UNSUPPORTED;
> }
>
> +static int64_t opal_sensor_read_long(u32 sensor_hndl, int token __unused,
> + u64 *sensor_data)
I think opal_sensor_read_u64 would be better name.
Also, can you instead of only adding this, which only works for a
limited number of sensors, have opal_sensor_read_u64 be the base
implementation and have the current opal_sensor_read call it and just
discard the top 32bits.
That way, we're now just supplying an API to read counters up to 64bits
in size and everything current "just works" with 32bits, but future, if
kernels call it instead, they will get the larger counters.
Or is there a use case for counters only showing up with one of those
calls that I can't think of?
> +{
> + switch (sensor_get_family(sensor_hndl)) {
> + case SENSOR_OCC:
> + return occ_sensor_read(sensor_hndl, sensor_data);
> + default:
> + break;
> + }
> +
> + return OPAL_UNSUPPORTED;
> +}
> +
> static int opal_sensor_group_clear(u32 group_hndl, int token)
> {
> switch (sensor_get_family(group_hndl)) {
> @@ -63,4 +76,5 @@ void sensor_init(void)
> /* Register OPAL interface */
> opal_register(OPAL_SENSOR_READ, opal_sensor_read, 3);
> opal_register(OPAL_SENSOR_GROUP_CLEAR, opal_sensor_group_clear, 2);
> + opal_register(OPAL_SENSOR_READ64, opal_sensor_read_long, 3);
> }
> diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c
> index 1042c11..4ed0e6a 100644
> --- a/hw/occ-sensor.c
> +++ b/hw/occ-sensor.c
> @@ -261,6 +261,7 @@ enum sensor_attr {
> SENSOR_SAMPLE_MAX,
> SENSOR_CSM_MIN, /* CSM's min/max */
> SENSOR_CSM_MAX,
> + SENSOR_ACCUMULATOR,
> MAX_SENSOR_ATTR,
> };
This patch as it stands does two things, please split out the new API
from the implementation of the new sensor attribute.
>
> @@ -317,22 +318,48 @@ static inline u32 sensor_handler(int occ_num, int sensor_id, int attr)
> return sensor_make_handler(SENSOR_OCC, occ_num, sensor_id, attr);
> }
>
> -int occ_sensor_read(u32 handle, u32 *data)
> +static u64 read_sensor(struct occ_sensor_record *sensor, int attr)
> +{
> + switch (attr) {
> + case SENSOR_SAMPLE:
> + return sensor->sample;
> + case SENSOR_SAMPLE_MIN:
> + return sensor->sample_min;
> + case SENSOR_SAMPLE_MAX:
> + return sensor->sample_max;
> + case SENSOR_CSM_MIN:
> + return sensor->csm_min;
> + case SENSOR_CSM_MAX:
> + return sensor->csm_max;
> + case SENSOR_ACCUMULATOR:
> + return sensor->accumulator;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static u64 read_counter(struct occ_sensor_counter *ctr, int attr)
> +{
> + switch (attr) {
> + case SENSOR_SAMPLE:
> + return ctr->sample;
> + case SENSOR_ACCUMULATOR:
> + return ctr->accumulator;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int select_buffer_and_read(int occ_num, int id, int attr, u64 *val)
> {
> struct occ_sensor_data_header *hb;
> struct occ_sensor_name *md;
> - struct occ_sensor_record *sping, *spong;
> - struct occ_sensor_record *sensor = NULL;
> u8 *ping, *pong;
> - u16 id = sensor_get_rid(handle);
> - u8 occ_num = sensor_get_frc(handle);
> - u8 attr = sensor_get_attr(handle);
> -
> - if (occ_num > MAX_OCCS)
> - return OPAL_PARAMETER;
> -
> - if (attr > MAX_SENSOR_ATTR)
> - return OPAL_PARAMETER;
> + void *buffer;
>
> hb = get_sensor_header_block(occ_num);
> md = get_names_block(hb);
> @@ -345,8 +372,6 @@ int occ_sensor_read(u32 handle, u32 *data)
>
> ping = (u8 *)((u64)hb + hb->reading_ping_offset);
> pong = (u8 *)((u64)hb + hb->reading_pong_offset);
> - sping = (struct occ_sensor_record *)((u64)ping + md[id].reading_offset);
> - spong = (struct occ_sensor_record *)((u64)pong + md[id].reading_offset);
>
> /* Check which buffer is valid and read the data from that.
> * Ping Pong Action
> @@ -355,41 +380,73 @@ int occ_sensor_read(u32 handle, u32 *data)
> * 1 0 Read Ping
> * 1 1 Read the buffer with latest timestamp
> */
> +
> if (*ping && *pong) {
> - if (sping->timestamp > spong->timestamp)
> - sensor = sping;
> + u64 tping, tpong;
> +
> + if (md[id].structure_type == OCC_SENSOR_READING_FULL) {
> + tping = ((struct occ_sensor_record *)((u64)ping +
> + md[id].reading_offset))->timestamp;
> + tpong = ((struct occ_sensor_record *)((u64)pong +
> + md[id].reading_offset))->timestamp;
> + } else {
> + tping = ((struct occ_sensor_counter *)((u64)ping +
> + md[id].reading_offset))->timestamp;
> + tpong = ((struct occ_sensor_counter *)((u64)pong +
> + md[id].reading_offset))->timestamp;
> + }
> + if (tping > tpong)
> + buffer = ping;
> else
> - sensor = spong;
> -
> + buffer = pong;
> } else if (*ping && !*pong) {
> - sensor = sping;
> + buffer = ping;
> } else if (!*ping && *pong) {
> - sensor = spong;
> + buffer = pong;
> } else if (!*ping && !*pong) {
> prlog(PR_DEBUG, "OCC: Both ping and pong sensor buffers are invalid\n");
> return OPAL_HARDWARE;
> }
>
> - switch (attr) {
> - case SENSOR_SAMPLE:
> - *data = sensor->sample;
> - break;
> - case SENSOR_SAMPLE_MIN:
> - *data = sensor->sample_min;
> - break;
> - case SENSOR_SAMPLE_MAX:
> - *data = sensor->sample_max;
> - break;
> - case SENSOR_CSM_MIN:
> - *data = sensor->csm_min;
> + buffer = (void *)((u64)buffer + md[id].reading_offset);
> +
> + switch (md[id].structure_type) {
> + case OCC_SENSOR_READING_FULL:
> + *val = read_sensor(buffer, attr);
> break;
> - case SENSOR_CSM_MAX:
> - *data = sensor->csm_max;
> + case OCC_SENSOR_READING_COUNTER:
> + *val = read_counter(buffer, attr);
> break;
> default:
> - *data = 0;
> + return OPAL_UNSUPPORTED;
> }
>
> + return 0;
> +}
> +
> +int occ_sensor_read(u32 handle, void *data)
why not just u64* ?
> +{
> + u64 val;
> + u16 id = sensor_get_rid(handle);
> + u8 occ_num = sensor_get_frc(handle);
> + u8 attr = sensor_get_attr(handle);
> + int rc;
> +
> + if (occ_num > MAX_OCCS)
> + return OPAL_PARAMETER;
> +
> + if (attr > MAX_SENSOR_ATTR)
> + return OPAL_PARAMETER;
> +
> + rc = select_buffer_and_read(occ_num, id, attr, &val);
> + if (rc)
> + return rc;
> +
> + if (attr == SENSOR_ACCUMULATOR)
> + *(u64 *)data = val;
> + else
> + *(u32 *)data = val;
> +
> return OPAL_SUCCESS;
> }
>
> @@ -543,6 +600,41 @@ static const char *get_sensor_loc_string(enum occ_sensor_location loc)
> return "unknown";
> }
>
> +static void add_sensor_node(const char *loc, const char *type, int i, int attr,
> + struct occ_sensor_name *md, u32 *phandle, u32 pir,
> + u32 occ_num, u32 chipid)
> +{
> + char name[30];
> + struct dt_node *node;
> + u32 handler;
> +
> + snprintf(name, sizeof(name), "%s-%s", loc, type);
> + handler = sensor_handler(occ_num, i, attr);
> + node = dt_new_addr(sensor_node, name, handler);
> + dt_add_property_string(node, "sensor-type", type);
> + dt_add_property_cells(node, "sensor-data", handler);
> + dt_add_property_string(node, "occ_label", md->name);
> + add_sensor_label(node, md, chipid);
> +
> + if (md->location == OCC_SENSOR_LOC_CORE)
> + dt_add_property_cells(node, "ibm,pir", pir);
> +
> + if (attr == SENSOR_SAMPLE) {
> + dt_add_property_string(node, "compatible", "ibm,opal-sensor");
> +
> + handler = sensor_handler(occ_num, i, SENSOR_CSM_MAX);
> + dt_add_property_cells(node, "sensor-data-max", handler);
> +
> + handler = sensor_handler(occ_num, i, SENSOR_CSM_MIN);
> + dt_add_property_cells(node, "sensor-data-min", handler);
> + } else if (attr == SENSOR_ACCUMULATOR) {
> + dt_add_property_string(node, "compatible",
> + "ibm,opal-sensor-counter");
This is adding a new device tree binding. You will need to document it.
Also, I don't see why this is needed, when all sensors could be read by
either opal_sensor_read or opal_sensor_read_u64 just at different precision.
> + }
> +
> + *phandle = node->phandle;
> +}
> +
> void occ_sensors_init(void)
> {
> struct proc_chip *chip;
> @@ -585,11 +677,9 @@ void occ_sensors_init(void)
> assert(phandles);
>
> for (i = 0; i < hb->nr_sensors; i++) {
> - char name[30];
> const char *type, *loc;
> - struct dt_node *node;
> struct cpu_thread *c = NULL;
> - u32 handler;
> + int attr;
>
> if (!(md[i].type & HWMON_SENSORS_MASK))
> continue;
> @@ -606,28 +696,32 @@ void occ_sensors_init(void)
>
> type = get_sensor_type_string(md[i].type);
> loc = get_sensor_loc_string(md[i].location);
> - snprintf(name, sizeof(name), "%s-%s", loc, type);
> -
> - handler = sensor_handler(occ_num, i, SENSOR_SAMPLE);
> - node = dt_new_addr(sensor_node, name, handler);
> -
> - dt_add_property_string(node, "sensor-type", type);
> - dt_add_property_cells(node, "sensor-data", handler);
> -
> - handler = sensor_handler(occ_num, i, SENSOR_CSM_MAX);
> - dt_add_property_cells(node, "sensor-data-max", handler);
>
> - handler = sensor_handler(occ_num, i, SENSOR_CSM_MIN);
> - dt_add_property_cells(node, "sensor-data-min", handler);
> -
> - dt_add_property_string(node, "compatible",
> - "ibm,opal-sensor");
> - dt_add_property_string(node, "occ_label", md[i].name);
> - add_sensor_label(node, &md[i], chip->id);
> + switch (md[i].structure_type) {
> + case OCC_SENSOR_READING_FULL:
> + attr = SENSOR_SAMPLE;
> + break;
> + case OCC_SENSOR_READING_COUNTER:
> + attr = SENSOR_ACCUMULATOR;
> + break;
> + default:
> + continue;
> + }
>
> - if (md[i].location == OCC_SENSOR_LOC_CORE)
> - dt_add_property_cells(node, "ibm,pir", c->pir);
> - phandles[phcount++] = node->phandle;
> + add_sensor_node(loc, type, i, attr, &md[i],
> + &phandles[phcount], c->pir, occ_num,
> + chip->id);
> + phcount++;
> +
> + /* Add energy sensors */
> + if (md[i].type == OCC_SENSOR_TYPE_POWER &&
> + md[i].structure_type == OCC_SENSOR_READING_FULL) {
> + add_sensor_node(loc, "energy", i,
> + SENSOR_ACCUMULATOR, &md[i],
> + &phandles[phcount], c->pir,
> + occ_num, chip->id);
> + phcount++;
> + }
> }
> occ_num++;
> occ_add_sensor_groups(sg, phandles, phcount, chip->id);
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 0bc036e..3f3dbff 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -214,7 +214,8 @@
> #define OPAL_SET_POWER_SHIFT_RATIO 155
> #define OPAL_SENSOR_GROUP_CLEAR 156
> #define OPAL_PCI_SET_P2P 157
> -#define OPAL_LAST 157
> +#define OPAL_SENSOR_READ64 158
> +#define OPAL_LAST 158
For any new OPAL API you need to add applicable documentation.
>
> /* Device tree flags */
>
> diff --git a/include/skiboot.h b/include/skiboot.h
> index db91325..893d673 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -328,7 +328,7 @@ extern int fake_nvram_write(uint32_t offset, void *src, uint32_t size);
>
> /* OCC Inband Sensors */
> extern void occ_sensors_init(void);
> -extern int occ_sensor_read(u32 handle, u32 *data);
> +extern int occ_sensor_read(u32 handle, void *data);
> extern int occ_sensor_group_clear(u32 group_hndl, int token);
> extern void occ_add_sensor_groups(struct dt_node *sg, u32 *phandles,
> int nr_phandles, int chipid);
> --
> 1.8.3.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
--
Stewart Smith
OPAL Architect, IBM.
More information about the Skiboot
mailing list