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

Shilpasri G Bhat shilpa.bhat at linux.vnet.ibm.com
Tue Mar 21 21:27:00 AEDT 2017


Hi Cedric,

On 03/21/2017 01:20 PM, Cédric Le Goater wrote:
> Hello Shilpasri,
> 
> It looks good to me. I just have a few questions below.
> 
> 
> On 03/18/2017 07:54 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 and temperature sensors to /ibm,opal/sensors
>> device-tree node which can be exported by the ibmpowernv-hwmon driver
>> in Linux.
> 
> You could expose others sensors in the device tree also even if the 
> Linux driver does not support them yet and then work on the extension. 
> I suppose voltage would be interesting. What else ? 
> 

There are sensors of type current like CURVDD, CURVDN which are processor Vdd
and Vdn current values. These can be exposed as HWMON sensors.

Yes voltage sensors can be exposed.

>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
>> ---
>> Changes from V1:
>> - Populate sensors to device tree instead of parsing in kernel
>> - Read sensor value in OPAL instead of kernel via opal_sensor_read()
>>
>>  core/init.c       |   1 +
>>  core/sensor.c     |   4 +
>>  hw/Makefile.inc   |   2 +-
>>  hw/occ-sensor.c   | 445 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/sensor.h  |   1 +
>>  include/skiboot.h |   4 +
>>  6 files changed, 456 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..04aeb2a
>> --- /dev/null
>> +++ b/hw/occ-sensor.c
>> @@ -0,0 +1,445 @@
>> +/* 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,
>> +};
>> +
>> +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;
>> +
>> +/**
>> + * 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;
>> +
>> +struct occ_sensor_info {
>> +	u32 offset;
>> +	u8 occ_num;
>> +} *sdr;
> 
> Is this an array mapping OCC sensor addresses with the skiboot sensor
> handler ? 

Yes this array is used to map sensor handlers to sensor address offset which is
relative to the starting address of sensor reading buffer.

>   
>> +static u64 occ_sensor_base;
>> +
>> +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 u32 sensor_handler(int sensor_id)
>> +{
>> +	return sensor_make_handler(SENSOR_OCC, 0, sensor_id, 0);
>> +}
>>
> 
> So you don't plan to have attributes ? like fault ?  I guess you don't 
> need the sensor class but may be you could hijack it to store the OCC
> number. Just an idea. You don't have to unless this is a way to get rid
> of sdr[] 

Yes. I was planning to post next series with attributes like min, max for each
sensor.

Yup I can use class to store OCC number. That way I dont have to store them for
each sdr[handler].

> 
>> +int occ_sensor_read(u32 handle, u32 *data)
>> +{
>> +	struct occ_sensor_data_header *hb;
>> +	struct occ_sensor_record *sping, *spong;
>> +	struct occ_sensor_record *sensor;
>> +	u8 *ping, *pong;
>> +	u16 id = sensor_get_rid(handle);
>> +	u8 occ_num;
>> +
>> +	occ_num = sdr[id].occ_num;
>> +	hb = get_sensor_header_block(occ_num);
>> +	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 + sdr[id].offset);
>> +	spong = (struct occ_sensor_record *)((u64)pong + sdr[id].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;
>> +	}
>> +
>> +	*data = sensor->sample;
>> +	return 0;
>> +}
>> +
>> +void occ_sensors_init(void)
>> +{
>> +	struct proc_chip *chip;
>> +	struct occ_sensor_data_header *hb;
>> +	struct occ_sensor_name *md;
>> +	int occ_to_chipid[MAX_OCCS];
> 
> MAX_CHIPS may be ? because you loop on chips.

Yes will change.

> 
>> +	int nr_sensors = 0, nr_occs = 0;
>> +	int occ_num, i;
>> +	u16 id = 0;
>> +
>> +	/* 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;
> 
> I would put the section below in its own occ_sensor_sanity() routine. 

Okay will do.

> 
>> +	/* Sanity check of the Sensor Data Header Block */
>> +	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;
>> +		}
>> +
>> +		if (hb->version != 0x01) {
>> +			prerror("OCC: Unsupported sensor header block version %d\n",
>> +				hb->version);
>> +			return;
>> +		}
>> +
>> +		if (hb->sensor_reading_version != 0x01) {
>> +			prerror("OCC: Unsupported sensor record format %d\n",
>> +				hb->sensor_reading_version);
>> +			return;
>> +		}
>> +
>> +		if (hb->sensor_names_version != 0x01) {
>> +			prerror("OCC: Unsupported sensor info format %d\n",
>> +				hb->sensor_names_version);
>> +			return;
>> +		}
>> +
>> +		if (hb->sensor_name_length != sizeof(struct occ_sensor_name)) {
>> +			prerror("OCC: Unsupported sensor info record length %d\n",
>> +				hb->sensor_name_length);
>> +			return;
>> +		}
>> +
>> +		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;
>> +		}
>> +	}
>> +
>> +	for (occ_num = 0; occ_num < nr_occs; occ_num++) {
>> +		hb = get_sensor_header_block(occ_num);
>> +
>> +		md = (struct occ_sensor_name *)((uint64_t)hb +
>> +						hb->sensor_names_offset);
> 
> I would add an helper for the above cast.

Okay will do.

> 
>> +		for (i = 0; i < hb->nr_sensors; i++) {
>> +			if (md[i].type == OCC_SENSOR_TYPE_TEMPERATURE ||
>> +			    md[i].type == OCC_SENSOR_TYPE_POWER)
>> +				nr_sensors++;
>> +		}
>> +	}
>> +
>> +	sdr = malloc(sizeof(*sdr) * nr_sensors);
>> +	assert(sdr);
>> +
>> +	for (occ_num = 0; occ_num < nr_occs; occ_num++) {
>> +		hb = get_sensor_header_block(occ_num);
>> +
>> +		md = (struct occ_sensor_name *)((uint64_t)hb +
>> +						hb->sensor_names_offset);
>> +
>> +		for (i = 0; i < hb->nr_sensors; i++) {
>> +			char name[MAX_CHARS_SENSOR_NAME * 2];
> 
> why '* 2' ?

I am prefixing chip id in the sensor name for chip sensors.
char name[MAX_CHARS_SENSOR_NAME + 5] should do instead.

>> +			struct dt_node *node;
>> +			u32 handler;
>> +
>> +			if (md[i].type != OCC_SENSOR_TYPE_TEMPERATURE &&
>> +			    md[i].type != OCC_SENSOR_TYPE_POWER)
>> +				continue;
>> +
>> +			sdr[id].offset = md[i].reading_offset;
>> +			sdr[id].occ_num = occ_num;
> 
> could md[i].reading_offset fit in a u16 ? 

No both are defined u32.

> 
>> +			handler = sensor_handler(id);
>> +			if (md[i].location == OCC_SENSOR_LOC_SYSTEM)
>> +				snprintf(name, sizeof(name), "%s", md[i].name);
>> +			else
>> +				snprintf(name, sizeof(name), "P%d_%s",
>> +					 occ_to_chipid[occ_num], md[i].name);
> 
> could you give us a sample ouput of these device tree names please ? 

                        P0_TEMPC3 at c0000a {
                                label = "P0_TEMPC3";
                                compatible = "ibm,opal-sensor";
                                sensor-data = <0xc0000a>;
                                sensor-type = "temp";
                                phandle = <0x10000142>;
                        };

                        PWRSYS at c00000 {
                                label = "PWRSYS";
                                compatible = "ibm,opal-sensor";
                                sensor-data = <0xc00000>;
                                sensor-type = "power";
                                phandle = <0x10000138>;
                        };

                        PWRGPU at c00004 {
                                label = "PWRGPU";
                                compatible = "ibm,opal-sensor";
                                sensor-data = <0xc00004>;
                                sensor-type = "power";
                                phandle = <0x1000013c>;
                        };

                        P0_TEMPDIMM6 at c00006 {
                                label = "P0_TEMPDIMM6";
                                compatible = "ibm,opal-sensor";
                                sensor-data = <0xc00006>;
                                sensor-type = "temp";
                                phandle = <0x1000013e>;
                        };

                        PWRSTORE at c00003 {
                                label = "PWRSTORE";
                                compatible = "ibm,opal-sensor";
                                sensor-data = <0xc00003>;
                                sensor-type = "power";
                                phandle = <0x1000013b>;
                        };


> 
>> +			node = dt_new_addr(sensor_node, name, handler);
>> +			dt_add_property_cells(node, "sensor-data", handler);
>> +			dt_add_property_string(node, "compatible",
>> +					       "ibm,opal-sensor");
>> +			dt_add_property_string(node, "label", name);
> 
> This literal name will reach user space. It should be understandable and 
> not necessarily the same as the device tree node name.  

Yeah agree. Will add appropriate device-tree node names.

> 
>> +			if (md[i].type == OCC_SENSOR_TYPE_POWER)
> 
> use a switch even if there is only two supported types.
> 

Okay will do.

Thanks and Regards,
Shilpa

> Thanks,
> 
> C.
> 
>> +				dt_add_property_string(node, "sensor-type",
>> +						       "power");
>> +			else
>> +				dt_add_property_string(node, "sensor-type",
>> +						       "temp");
>> +			id++;
>> +		}
>> +	}
>> +}
>> 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