[Skiboot] [RFC V3] sensors: occ: Add support for OCC inband sensors

Cédric Le Goater clg at kaod.org
Fri Mar 31 00:54:35 AEDT 2017


Hello,

I think you can remove the RFC now. Looks good, some minor comments
below.

On 03/30/2017 02:13 PM, Shilpasri G Bhat wrote:
> Add support to parse and export OCC inband sensors which are copied
> by OCC to main memory in P9. Each OCC writes three buffers which
> includes one names buffer for sensor meta data and two buffers for
> sensor readings. While OCC writes to one buffer the sensor values
> can be read from the other buffer. The sensors are updated every
> 100ms.
> 
> This patch adds power, temperature, current and voltage sensors to
> /ibm,opal/sensors device-tree node which can be exported by the
> ibmpowernv-hwmon driver in Linux.
> 
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
> ---
> Changes from V2:
> - Added current and voltage sensors
> - Removed 'struct occ_sensor_info' which was used to store sensor
>   offset address values
> - Added sensor attributes like MIN and MAX
> - Moved the header sanity checks to a separate routine
>   'occ_sensor_sanity()'
> - Added a mapping for OCC sensor name to userspace label. If the
>   mapping is not found we fall back to OCC name.
> 
>  core/init.c       |   1 +
>  core/sensor.c     |   4 +
>  hw/Makefile.inc   |   2 +-
>  hw/occ-sensor.c   | 596 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/sensor.h  |   1 +
>  include/skiboot.h |   4 +
>  6 files changed, 607 insertions(+), 1 deletion(-)
>  create mode 100644 hw/occ-sensor.c
> 
> diff --git a/core/init.c b/core/init.c
> index 983ead5..dab76ea 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -501,6 +501,7 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
>  	 * as possible to avoid delay.
>  	 */
>  	occ_pstates_init();
> +	occ_sensors_init();
>  
>  	/* Use nvram bootargs over device tree */
>  	cmdline = nvram_query("bootargs");
> diff --git a/core/sensor.c b/core/sensor.c
> index cc5341c..b0d3c5e 100644
> --- a/core/sensor.c
> +++ b/core/sensor.c
> @@ -29,6 +29,10 @@ static int64_t opal_sensor_read(uint32_t sensor_hndl, int token,
>  	switch (sensor_get_family(sensor_hndl)) {
>  	case SENSOR_DTS:
>  		return dts_sensor_read(sensor_hndl, sensor_data);
> +	case SENSOR_OCC:
> +		return occ_sensor_read(sensor_hndl, sensor_data);
> +	default:
> +		break;
>  	}
>  
>  	if (platform.sensor_read)
> diff --git a/hw/Makefile.inc b/hw/Makefile.inc
> index d87f85e..194097f 100644
> --- a/hw/Makefile.inc
> +++ b/hw/Makefile.inc
> @@ -1,7 +1,7 @@
>  # -*-Makefile-*-
>  SUBDIRS += hw
>  HW_OBJS  = xscom.o chiptod.o gx.o cec.o lpc.o lpc-uart.o psi.o
> -HW_OBJS += homer.o slw.o occ.o fsi-master.o centaur.o
> +HW_OBJS += homer.o slw.o occ.o fsi-master.o centaur.o occ-sensor.o
>  HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-842.o
>  HW_OBJS += p7ioc.o p7ioc-inits.o p7ioc-phb.o
>  HW_OBJS += phb3.o sfc-ctrl.o fake-rtc.o bt.o p8-i2c.o prd.o
> diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c
> new file mode 100644
> index 0000000..d3d9ee4
> --- /dev/null
> +++ b/hw/occ-sensor.c
> @@ -0,0 +1,596 @@
> +/* Copyright 2017 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
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <skiboot.h>
> +#include <opal.h>
> +#include <chip.h>
> +#include <sensor.h>
> +#include <device.h>
> +
> +/*
> + * OCC Sensor Data
> + *
> + * OCC sensor data will use BAR2 (OCC Common is per physical drawer).
> + * Starting address is at offset 0x00580000 from BAR2 base address.
> + * Maximum size is 1.5MB.
> + *
> + * -------------------------------------------------------------------------
> + * | Start (Offset from |	End	| Size	   |Description		   |
> + * | BAR2 base address) |		|	   |			   |
> + * -------------------------------------------------------------------------
> + * |	0x00580000      |  0x005A57FF   |150kB     |OCC 0 Sensor Data Block|
> + * |	0x005A5800      |  0x005CAFFF   |150kB	   |OCC 1 Sensor Data Block|
> + * |	    :		|	:	|  :	   |		:          |
> + * |	0x00686800	|  0x006ABFFF   |150kB	   |OCC 7 Sensor Data Block|
> + * |	0x006AC000	|  0x006FFFFF   |336kB     |Reserved		   |
> + * -------------------------------------------------------------------------
> + *
> + *
> + * OCC N Sensor Data Block Layout (150kB)
> + *
> + * The sensor data block layout is the same for each OCC N. It contains
> + * sensor-header-block, sensor-names buffer, sensor-readings-ping buffer and
> + * sensor-readings-pong buffer.
> + *
> + * ----------------------------------------------------------------------------
> + * | Start (Offset from OCC |   End	   | Size |Description		      |
> + * | N Sensor Data Block)   |		   |	  |			      |
> + * ----------------------------------------------------------------------------
> + * |	0x00000000	    |  0x000003FF  |1kB   |Sensor Data Header Block   |
> + * |	0x00000400	    |  0x0000CBFF  |50kB  |Sensor Names		      |
> + * |	0x0000CC00	    |  0x0000DBFF  |4kB   |Reserved		      |
> + * |	0x0000DC00	    |  0x00017BFF  |40kB  |Sensor Readings ping buffer|
> + * |	0x00017C00	    |  0x00018BFF  |4kB   |Reserved		      |
> + * |	0x00018C00	    |  0x00022BFF  |40kB  |Sensor Readings pong buffer|
> + * |	0x00022C00	    |  0x000257FF  |11kB  |Reserved		      |
> + * ----------------------------------------------------------------------------
> + *
> + * Sensor Data Header Block : This is written once by the OCC during
> + * initialization after a load or reset. Layout is defined in 'struct
> + * occ_sensor_data_header'
> + *
> + * Sensor Names : This is written once by the OCC during initialization after a
> + * load or reset. It contains static information for each sensor. The number of
> + * sensors, format version and length of each sensor is defined in
> + * 'Sensor Data Header Block'. Format of each sensor name is defined in
> + * 'struct occ_sensor_name'. The first sensor starts at offset 0 followed
> + * immediately by the next sensor.
> + *
> + * Sensor Readings Ping/Pong Buffer:
> + * There are two 40kB buffers to store the sensor readings. One buffer that
> + * is currently being updated by the OCC and one that is available to be read.
> + * Each of these buffers will be of the same format. The number of sensors and
> + * the format version of the ping and pong buffers is defined in the
> + * 'Sensor Data Header Block'.
> + *
> + * Each sensor within the ping and pong buffers may be of a different format
> + * and length. For each sensor the length and format is determined by its
> + * 'struct occ_sensor_name.structure_type' in the Sensor Names buffer.
> + *
> + * --------------------------------------------------------------------------
> + * | Offset | Byte0 | Byte1 | Byte2 | Byte3 | Byte4 | Byte5 | Byte6 | Byte7 |
> + * --------------------------------------------------------------------------
> + * | 0x0000 |Valid  |		   Reserved				    |
> + * |        |(0x01) |							    |
> + * --------------------------------------------------------------------------
> + * | 0x0008 |			Sensor Readings				    |
> + * --------------------------------------------------------------------------
> + * |	:   |				:				    |
> + * --------------------------------------------------------------------------
> + * | 0xA000 |                     End of Data				    |
> + * --------------------------------------------------------------------------
> + *
> + */
> +
> +#define MAX_OCCS			8
> +#define MAX_CHARS_SENSOR_NAME		16
> +#define MAX_CHARS_SENSOR_UNIT		4
> +
> +#define OCC_SENSOR_DATA_BLOCK_OFFSET		0x00580000
> +#define OCC_SENSOR_DATA_BLOCK_SIZE		0x00025800
> +
> +enum occ_sensor_type {
> +	OCC_SENSOR_TYPE_GENERIC		= 0x0001,
> +	OCC_SENSOR_TYPE_CURRENT		= 0x0002,
> +	OCC_SENSOR_TYPE_VOLTAGE		= 0x0004,
> +	OCC_SENSOR_TYPE_TEMPERATURE	= 0x0008,
> +	OCC_SENSOR_TYPE_UTILIZATION	= 0x0010,
> +	OCC_SENSOR_TYPE_TIME		= 0x0020,
> +	OCC_SENSOR_TYPE_FREQUENCY	= 0x0040,
> +	OCC_SENSOR_TYPE_POWER		= 0x0080,
> +	OCC_SENSOR_TYPE_PERFORMANCE	= 0x0200,

0x200 ? just checking, I don't have the specs.

> +};
> +
> +enum occ_sensor_location {
> +	OCC_SENSOR_LOC_SYSTEM		= 0x0001,
> +	OCC_SENSOR_LOC_PROCESSOR	= 0x0002,
> +	OCC_SENSOR_LOC_PARTITION	= 0x0004,
> +	OCC_SENSOR_LOC_MEMORY		= 0x0008,
> +	OCC_SENSOR_LOC_VRM		= 0x0010,
> +	OCC_SENSOR_LOC_OCC		= 0x0020,
> +	OCC_SENSOR_LOC_CORE		= 0x0040,
> +	OCC_SENSOR_LOC_QUAD		= 0x0080,
> +	OCC_SENSOR_LOC_GPU		= 0x0100,
> +};
> +
> +enum sensor_struct_type {
> +	OCC_SENSOR_READING_FULL		= 0x01,
> +	OCC_SENSOR_READING_COUNTER	= 0x02,
> +};
> +
> +/**
> + * struct occ_sensor_data_header -	Sensor Data Header Block
> + * @valid:				When the value is 0x01 it indicates
> + *					that this header block and the sensor
> + *					names buffer are ready
> + * @version:				Format version of this block
> + * @nr_sensors:				Number of sensors in names, ping and
> + *					pong buffer
> + * @sensor_reading_version:		Format version of the Ping/Pong buffer
> + * @sensor_names_offset:		Offset to the location of names buffer
> + * @sensor_names_version:		Format version of names buffer
> + * @sensor_names_length:		Length of each sensor in names buffer
> + * @sensor_reading_ping_offset:		Offset to the location of Ping buffer
> + * @sensor_reading_pong_offset:		Offset to the location of Pong buffer
> + * @pad/reserved:			Unused data
> + */
> +struct occ_sensor_data_header {
> +	u8 valid;
> +	u8 version;
> +	u16 nr_sensors;
> +	u8 sensor_reading_version;
> +	u8 pad[3];
> +	u32 sensor_names_offset;
> +	u8 sensor_names_version;
> +	u8 sensor_name_length;
> +	u16 reserved;
> +	u32 sensor_reading_ping_offset;
> +	u32 sensor_reading_pong_offset;
> +} __packed;
>

minor: may be you can remove the 'sensor_' prefix in this struct. 
The field names are rather long.

> +/**
> + * struct occ_sensor_name -		Format of Sensor Name
> + * @name:				Sensor name
> + * @units:				Sensor units of measurement
> + * @gsid:				Global sensor id (OCC)
> + * @freq:				Update frequency
> + * @scale_factor:			Scaling factor
> + * @type:				Sensor type as defined in
> + *					'enum occ_sensor_type'
> + * @location:				Sensor location as defined in
> + *					'enum occ_sensor_location'
> + * @structure_type:			Indicates type of data structure used
> + *					for the sensor readings in the ping and
> + *					pong buffers for this sensor as defined
> + *					in 'enum sensor_struct_type'
> + * @reading_offset:			Offset from the start of the ping/pong
> + *					reading buffers for this sensor
> + * @sensor_data:			Sensor specific info
> + * @pad:				Padding to fit the size of 48 bytes.
> + */
> +struct occ_sensor_name {
> +	char name[MAX_CHARS_SENSOR_NAME];
> +	char units[MAX_CHARS_SENSOR_UNIT];
> +	u16 gsid;
> +	u32 freq;
> +	u32 scale_factor;
> +	u16 type;
> +	u16 location;
> +	u8 structure_type;
> +	u32 reading_offset;
> +	u8 sensor_data;
> +	u8 pad[8];
> +} __packed;
> +
> +/**
> + * struct occ_sensor_record -		Sensor Reading Full
> + * @gsid:				Global sensor id (OCC)
> + * @timestamp:				Time base counter value while updating
> + *					the sensor
> + * @sample:				Latest sample of this sensor
> + * @sample_min:				Minimum value since last OCC reset
> + * @sample_max:				Maximum value since last OCC reset
> + * @CSM_min:				Minimum value since last reset request
> + *					by CSM (CORAL)
> + * @CSM_max:				Maximum value since last reset request
> + *					by CSM (CORAL)
> + * @profiler_min:			Minimum value since last reset request
> + *					by profiler (CORAL)
> + * @profiler_max:			Maximum value since last reset request
> + *					by profiler (CORAL)
> + * @job_scheduler_min:			Minimum value since last reset request
> + *					by job scheduler(CORAL)
> + * @job_scheduler_max:			Maximum value since last reset request
> + *					by job scheduler (CORAL)
> + * @accumulator:			Accumulator for this sensor
> + * @update_tag:				Count of the number of ticks that have
> + *					passed between updates
> + * @pad:				Padding to fit the size of 48 bytes
> + */
> +struct occ_sensor_record {
> +	u16 gsid;
> +	u64 timestamp;
> +	u16 sample;
> +	u16 sample_min;
> +	u16 sample_max;
> +	u16 CSM_min;
> +	u16 CSM_max;
> +	u16 profiler_min;
> +	u16 profiler_max;
> +	u16 job_scheduler_min;
> +	u16 job_scheduler_max;
> +	u64 accumulator;
> +	u32 update_tag;
> +	u8 pad[8];
> +} __packed;
> +
> +/**
> + * struct occ_sensor_counter -		Sensor Reading Counter
> + * @gsid:				Global sensor id (OCC)
> + * @timestamp:				Time base counter value while updating
> + *					the sensor
> + * @accumulator:			Accumulator/Counter
> + * @sample:				Latest sample of this sensor (0/1)
> + * @pad:				Padding to fit the size of 24 bytes
> + */
> +struct occ_sensor_counter {
> +	u16 gsid;
> +	u64 timestamp;
> +	u64 accumulator;
> +	u8 sample;
> +	u8 pad[5];
> +} __packed;
> +
> +enum sensor_attr {
> +	SENSOR_SAMPLE,
> +	SENSOR_MAX,
> +	SENSOR_MIN,
> +	MAX_SENSOR_ATTR,
> +};
> +
> +static struct str_map {
> +	const char *occ_str;
> +	const char *opal_str;
> +} str_maps[] = {
> +	{"PWRSYS", "System"},
> +	{"PWRFAN", "Fan"},
> +	{"PWRIO", "IO"},
> +	{"PWRSTORE", "Storage"},
> +	{"PWRGPU", "GPU"},
> +	{"PWRAPSSCH", "APSS"},/
> +	{"PWRPROC", ""},
> +	{"PWRVDD", "Vdd"},
> +	{"CURVDD", "Vdd"},
> +	{"VOLTVDDSENSE", "Vdd Remote Sense"},
> +	{"VOLTVDD", "Vdd"},
> +	{"PWRVDN", "Vdn"},
> +	{"CURVDN", "Vdn"},
> +	{"VOLTVDNSENSE", "Vdn Remote Sense"},
> +	{"VOLTVDN", "Vdn"},
> +	{"PWRMEM", "Memory"},
> +	{"TEMPC", "Core"},
> +	{"TEMPQ", "Quad"},
> +	{"TEMPNEST", "Nest"},
> +	{"TEMPPROCTHRM", "Chip"},
> +	{"TEMPDIMM", "DIMM"},
> +	{"TEMPGPU", "GPU"},
> +};
> +
> +static u64 occ_sensor_base;
> +static int occ_to_chipid[MAX_CHIPS];

I am not sure this occ_to_chipid[] array is required.

> +
> +static inline
> +struct occ_sensor_data_header *get_sensor_header_block(int occ_num)
> +{
> +	return (struct occ_sensor_data_header *)
> +		(occ_sensor_base + occ_num * OCC_SENSOR_DATA_BLOCK_SIZE);
> +}
> +
> +static inline
> +struct occ_sensor_name *get_names_block(struct occ_sensor_data_header *hb)
> +{
> +	return ((struct occ_sensor_name *)((u64)hb + hb->sensor_names_offset));
> +}
> +
> +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)
> +{
> +	struct occ_sensor_data_header *hb;
> +	struct occ_sensor_name *md;
> +	struct occ_sensor_record *sping, *spong;
> +	struct occ_sensor_record *sensor;
> +	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;
> +
> +	hb = get_sensor_header_block(occ_num);
> +	md = get_names_block(hb);
> +
> +	if (hb->valid != 1)
> +		return OPAL_BUSY;
> +
> +	if (id > hb->nr_sensors)
> +		return OPAL_PARAMETER;
> +
> +	ping = (u8 *)((u64)hb + hb->sensor_reading_ping_offset);
> +	pong = (u8 *)((u64)hb + hb->sensor_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
> +	 *  0	0	Return with error
> +	 *  0	1	Read Pong
> +	 *  1	0	Read Ping
> +	 *  1	1	Read the buffer with latest timestamp
> +	 */
> +	if (*ping && *pong) {
> +		if (sping->timestamp > spong->timestamp)
> +			sensor = sping;
> +		else
> +			sensor = spong;
> +
> +	} else if (*ping && !*pong) {
> +		sensor = sping;
> +	} else if (!*ping && *pong) {
> +		sensor = spong;
> +	} else if (!*ping && !*pong) {
> +		prlog(PR_DEBUG, "OCC: Both ping and pong sensor buffers are invalid\n");
> +		return OPAL_BUSY;
> +	}
> +
> +	switch (attr) {
> +	case SENSOR_SAMPLE:
> +		*data = sensor->sample;
> +		break;
> +	case SENSOR_MAX:
> +		*data = sensor->sample_max;
> +		break;
> +	case SENSOR_MIN:
> +		*data = sensor->sample_min;
> +		break;
> +	default:
> +		*data = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool occ_sensor_sanity(int nr_occs)
> +{
> +	struct occ_sensor_data_header *hb;
> +	int occ_num;

you could loop on the chips and increment occ_num.

> +	for (occ_num = 0; occ_num < nr_occs; occ_num++) {
> +		hb = get_sensor_header_block(occ_num);
> +		if (hb->valid != 0x01) {
> +			prerror("OCC: Sensor data invalid\n");
> +			return false;
> +		}
> +
> +		if (hb->version != 0x01) {
> +			prerror("OCC: Unsupported sensor header block version %d\n",
> +				hb->version);
> +			return false;
> +		}
> +
> +		if (hb->sensor_reading_version != 0x01) {
> +			prerror("OCC: Unsupported sensor record format %d\n",
> +				hb->sensor_reading_version);
> +			return false;
> +		}
> +
> +		if (hb->sensor_names_version != 0x01) {
> +			prerror("OCC: Unsupported sensor info format %d\n",
> +				hb->sensor_names_version);
> +			return false;
> +		}
> +
> +		if (hb->sensor_name_length != sizeof(struct occ_sensor_name)) {
> +			prerror("OCC: Unsupported sensor info record length %d\n",
> +				hb->sensor_name_length);
> +			return false;
> +		}
> +
> +		if (!hb->nr_sensors) {
> +			prerror("OCC: No sensors found for chip %d\n",
> +				occ_to_chipid[occ_num]);
> +			continue;
> +		}
> +
> +		if (!hb->sensor_names_offset ||
> +		    !hb->sensor_reading_ping_offset ||
> +		    !hb->sensor_reading_pong_offset) {
> +			prerror("OCC: Invalid sensor buffer pointers\n");
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +static int parse_entity(const char *name, char **end)
> +{
> +	while (*name != '\0') {
> +		if (isdigit(*name))
> +			break;
> +		name++;
> +	}
> +
> +	if (*name)
> +		return strtol(name, end, 10);
> +	else
> +		return -1;
> +}

Please add some comments what this parsing does. What is the input format ?  

> +static void add_sensor_label(struct dt_node *node, struct occ_sensor_name *md,
> +			     int occ)
> +{
> +	char sname[30] = "";
> +	char prefix[30] = "";
> +	int i;
> +
> +	if (md->location != OCC_SENSOR_LOC_SYSTEM)
> +		snprintf(prefix, sizeof(prefix), "%s %d ", "Chip",
> +			 occ_to_chipid[occ]);

you could pass the chip->id instead of 'occ' ?
 
> +	for (i = 0; i < ARRAY_SIZE(str_maps); i++)
> +		if (!strncmp(str_maps[i].occ_str, md->name,
> +			     strlen(str_maps[i].occ_str))) {
> +			char *end;
> +			int num;
> +
> +			num = parse_entity(md->name, &end);
> +			if (num != -1) {
> +				snprintf(sname, sizeof(sname), "%s%s %d %s",
> +					 prefix, str_maps[i].opal_str, num,
> +					 end);
> +			} else {
> +				snprintf(sname, sizeof(sname), "%s%s", prefix,
> +					 str_maps[i].opal_str);
> +			}
> +			dt_add_property_string(node, "label", sname);
> +			return;
> +		}
> +
> +	if (md->location == OCC_SENSOR_LOC_SYSTEM) {
> +		dt_add_property_string(node, "label", md->name);
> +	} else {
> +		snprintf(sname, sizeof(sname), "P%d_%s", occ_to_chipid[occ],
> +			 md->name);
> +		dt_add_property_string(node, "label", sname);
> +	}
> +}
> +
> +void occ_sensors_init(void)
> +{
> +	struct proc_chip *chip;
> +	struct occ_sensor_name *md;
> +	struct occ_sensor_data_header *hb;
> +	int nr_occs = 0;
> +	int occ_num, i;
> +
> +	/* OCC inband sensors is only supported in P9 */
> +	if (proc_gen != proc_gen_p9)
> +		return;
> +
> +	/* Sensors are copied to BAR2 OCC Common Area */
> +	chip = next_chip(NULL);
> +	if (!chip->occ_common_base) {
> +		prerror("OCC: Unassigned OCC Common Area. No sensors found\n");
> +		return;
> +	}
> +
> +	occ_sensor_base = chip->occ_common_base + OCC_SENSOR_DATA_BLOCK_OFFSET;
> +
> +	for_each_chip(chip)
> +		occ_to_chipid[nr_occs++] = chip->id;
> +
> +	/* Sanity check of the Sensor Data Header Block */
> +	if (!occ_sensor_sanity(nr_occs))
> +		return;

so, if one occ has some bogus data we don't get the others' ? May be you
could do this sanity check per chip and move it under the loop below ? 

> +	for (occ_num = 0; occ_num < nr_occs; occ_num++) {

why not loop directly on the chips and increment occ_num ? 

> +		hb = get_sensor_header_block(occ_num);
> +		md = get_names_block(hb);
> +
> +		for (i = 0; i < hb->nr_sensors; i++) {
> +			char name[30];
> +			const char *type, *loc;
> +			struct dt_node *node;
> +			u32 handler;
> +
> +			if (md[i].type != OCC_SENSOR_TYPE_TEMPERATURE &&
> +			    md[i].type != OCC_SENSOR_TYPE_POWER &&
> +			    md[i].type != OCC_SENSOR_TYPE_CURRENT &&
> +			    md[i].type != OCC_SENSOR_TYPE_VOLTAGE)
> +				continue;

you could use a mask to filter out unwanted sensor type.

> +
> +			switch (md[i].type) {
> +			case OCC_SENSOR_TYPE_POWER:
> +				type = "power";
> +				break;
> +			case OCC_SENSOR_TYPE_TEMPERATURE:
> +				type = "temp";
> +				break;
> +			case OCC_SENSOR_TYPE_CURRENT:
> +				type = "curr";
> +				break;
> +			case OCC_SENSOR_TYPE_VOLTAGE:
> +				type = "in";
> +				break;
> +			default:
> +				return;
> +			}

May be we you could use a static const array for the above, with a 'type' BIT 
index ? or use a specific routine at least.

> +			switch (md[i].location) {
> +			case OCC_SENSOR_LOC_SYSTEM:
> +				loc = "sys";
> +				break;
> +			case OCC_SENSOR_LOC_PROCESSOR:
> +				loc = "proc";
> +				break;
> +			case OCC_SENSOR_LOC_MEMORY:
> +				loc = "mem";
> +				break;
> +			case OCC_SENSOR_LOC_VRM:
> +				loc = "vrm";
> +				break;
> +			case OCC_SENSOR_LOC_CORE:
> +				loc = "core";
> +				break;
> +			case OCC_SENSOR_LOC_QUAD:
> +				loc = "quad";
> +				break;
> +			case OCC_SENSOR_LOC_GPU:
> +				loc = "gpu";
> +				break;
> +			default:
> +				loc = "unknown";
> +				break;
> +			}

same here.

Thanks,

C.

> +			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_MAX);
> +			dt_add_property_cells(node, "sensor-data-max", handler);
> +
> +			handler = sensor_handler(occ_num, i, SENSOR_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], occ_num);
> +		}
> +	}
> +}
> diff --git a/include/sensor.h b/include/sensor.h
> index 7eb3fa5..445a6bc 100644
> --- a/include/sensor.h
> +++ b/include/sensor.h
> @@ -53,6 +53,7 @@
>   */
>  enum {
>  	SENSOR_FSP = 0,
> +	SENSOR_OCC = 6,
>  	SENSOR_DTS = 7,
>  };
>  
> diff --git a/include/skiboot.h b/include/skiboot.h
> index bb0a7b5..a8e1534 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -302,4 +302,8 @@ extern int fake_nvram_info(uint32_t *total_size);
>  extern int fake_nvram_start_read(void *dst, uint32_t src, uint32_t len);
>  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);
> +
>  #endif /* __SKIBOOT_H */
> 



More information about the Skiboot mailing list