[RFC] occ: Add support for OCC inband sensors

In P9, OCC copies its collection of sensors to OCC Common Area in main
memory. Each OCC writes three buffers which includes one names buffer
for sensor meta data and two buffers for sensor readings. The sensor
names buffers is written once and contains information like name,
units, frequency, sensor type, location, offset relative to reading
buffer and scale factor for each sensor. There are two buffers to
store sensor readings. While OCC writes to one buffer the sensor
values can be read from the other buffer. The sensor records stored in
sensor buffer can be of two types like counter sensor and full sensor.
Counter sensor record contains just the counter value. Full sensor
contains sample value, min, max, accumulator. It also maintains
min/max for three different clients which can be reset at runtime.

This patch exports the sensor buffer offsets in the below device-tree
node.
ibm,opal {
	occ-inband-sensors {
		compatible = "ibm,p9-occ-inband-sensor";
		sensor-reading-size-counter = <0x18>;
		#address-cells = <0x2>;
		sensor-reading-size-full = <0x30>;
		#size-cells = <0x1>;
		phandle = <0x10000138>;
		sensor-names-size = <0x30>;

		occ@30300000 {
			pong-offset = <0x18c00>;
			names-offset = <0x400>;
			ping-offset = <0xdc00>;
			nr-sensors = <0x10>;
			ibm,occ-id = <0x0>;
			ibm,chip-id = <0x0>;
			phandle = <0x10000139>;
			reg = <0x0 0x30300000 0x25800>;
		};
	};
}

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
 core/init.c       |   1 +
 hw/Makefile.inc   |   2 +-
 hw/occ-sensor.c   | 374 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/skiboot.h |   3 +
 4 files changed, 379 insertions(+), 1 deletion(-)
 create mode 100644 hw/occ-sensor.c

Hello Shilpasri,

Thanks for the write up on the OCC sensors. Nice and detailed.

On the overall design, I think this is exposing too much HW details 
to Linux. I am not sure Linux should even know about OCC or where
the sensors are coming from. Even if this is doable, I would not 
let the kernel loop on the OCC data to collect which sensors are
available. What is being done under :

	arch/powerpc/platforms/powernv/opal-occ-sensors.c

in the Linux driver should be in the firmware IHMO. 

skiboot should hide the gory details and expose in the device 
tree sensor nodes collected from the different available sources: 
FSP, BMC, OCC, DTS, etc. 

There is a framework in place in skiboot for that, which may be, 
needs some tuning to add OCC support. But the good thing about it, 
is that the Linux driver comes for free in *all* Linux distros now.

Cheers,

C.     

On 03/09/2017 12:34 PM, Shilpasri G Bhat wrote:
> In P9, OCC copies its collection of sensors to OCC Common Area in main
> memory. Each OCC writes three buffers which includes one names buffer
> for sensor meta data and two buffers for sensor readings. The sensor
> names buffers is written once and contains information like name,
> units, frequency, sensor type, location, offset relative to reading
> buffer and scale factor for each sensor. There are two buffers to
> store sensor readings. While OCC writes to one buffer the sensor
> values can be read from the other buffer. The sensor records stored in
> sensor buffer can be of two types like counter sensor and full sensor.
> Counter sensor record contains just the counter value. Full sensor
> contains sample value, min, max, accumulator. It also maintains
> min/max for three different clients which can be reset at runtime.
> 
> This patch exports the sensor buffer offsets in the below device-tree
> node.
> ibm,opal {
> 	occ-inband-sensors {
> 		compatible = "ibm,p9-occ-inband-sensor";
> 		sensor-reading-size-counter = <0x18>;
> 		#address-cells = <0x2>;
> 		sensor-reading-size-full = <0x30>;
> 		#size-cells = <0x1>;
> 		phandle = <0x10000138>;
> 		sensor-names-size = <0x30>;
> 
> 		occ@30300000 {
> 			pong-offset = <0x18c00>;
> 			names-offset = <0x400>;
> 			ping-offset = <0xdc00>;
> 			nr-sensors = <0x10>;
> 			ibm,occ-id = <0x0>;
> 			ibm,chip-id = <0x0>;
> 			phandle = <0x10000139>;
> 			reg = <0x0 0x30300000 0x25800>;
> 		};
> 	};
> }
> 
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
>  core/init.c       |   1 +
>  hw/Makefile.inc   |   2 +-
>  hw/occ-sensor.c   | 374 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/skiboot.h |   3 +
>  4 files changed, 379 insertions(+), 1 deletion(-)
>  create mode 100644 hw/occ-sensor.c
> 
> diff --git a/core/init.c b/core/init.c
> index 58f96f4..91e2d94 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/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..0788658
> --- /dev/null
> +++ b/hw/occ-sensor.c
> @@ -0,0 +1,374 @@
> +/* 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;
> +
> +static inline
> +struct occ_sensor_data_header *get_sensor_header_block(u64 occ_base,
> +						       int occ_num)
> +{
> +	return (struct occ_sensor_data_header *)
> +		(occ_base + occ_num * OCC_SENSOR_DATA_BLOCK_SIZE);
> +}
> +
> +void occ_sensors_init(void)
> +{
> +	struct proc_chip *chip;
> +	struct occ_sensor_data_header *hb;
> +	struct dt_node *sensor;
> +	u64 occ_sensor_base;
> +	u32 occ_to_chipid[MAX_OCCS];
> +	int nr_occs = 0, occ_num = 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;
> +
> +	/* Sanity check of the Sensor Data Header Block */
> +	for (occ_num = 0; occ_num < nr_occs; occ_num++) {
> +		hb = get_sensor_header_block(occ_sensor_base, occ_num);
> +		if (hb->valid != 0x01) {
> +			prerror("OCC: Sensor data invalid\n");
> +			return;
> +		}
> +
> +		if (hb->version != 0x01) {
> +			prerror("OCC: Unsupported sensor header block format version %d\n",
> +				hb->version);
> +			return;
> +		}
> +
> +		if (hb->sensor_reading_version != 0x01) {
> +			prerror("OCC: Unsupported sensor reading format version %d\n",
> +				hb->sensor_reading_version);
> +			return;
> +		}
> +
> +		if (hb->sensor_names_version != 0x01) {
> +			prerror("OCC: Unsupported sensor names format version %d\n",
> +				hb->sensor_names_version);
> +			return;
> +		}
> +
> +		if (hb->sensor_name_length != sizeof(struct occ_sensor_name)) {
> +			prerror("OCC: Unsupported sensor name 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 offsets\n");
> +			return;
> +		}
> +	}
> +
> +	sensor = dt_new(opal_node, "occ-inband-sensors");
> +	assert(sensor);
> +
> +	occ_num = 0;
> +	hb = get_sensor_header_block(occ_sensor_base, occ_num);
> +	dt_add_property_strings(sensor, "compatible",
> +				"ibm,p9-occ-inband-sensor");
> +	dt_add_property_cells(sensor, "sensor-names-size",
> +			      hb->sensor_name_length);
> +	dt_add_property_cells(sensor, "sensor-reading-size-full",
> +			      sizeof(struct occ_sensor_record));
> +	dt_add_property_cells(sensor, "sensor-reading-size-counter",
> +			      sizeof(struct occ_sensor_counter));
> +
> +	dt_add_property_cells(sensor, "#address-cells", 2);
> +	dt_add_property_cells(sensor, "#size-cells", 1);
> +
> +	for (occ_num = 0; occ_num < nr_occs; occ_num++) {
> +		struct dt_node *node;
> +
> +		hb = get_sensor_header_block(occ_sensor_base, occ_num);
> +		node = dt_new_addr(sensor, "occ", (uint64_t)hb);
> +		assert(node);
> +
> +		dt_add_property_cells(node, "ibm,occ-id", occ_num);
> +		dt_add_property_cells(node, "ibm,chip-id",
> +				      occ_to_chipid[occ_num]);
> +		dt_add_property_cells(node, "reg", hi32((uint64_t)hb),
> +				      lo32((uint64_t)hb),
> +				      OCC_SENSOR_DATA_BLOCK_SIZE);
> +		dt_add_property_cells(node, "nr-sensors", hb->nr_sensors);
> +		dt_add_property_cells(node, "ping-offset",
> +				      hb->sensor_reading_ping_offset);
> +		dt_add_property_cells(node, "pong-offset",
> +				      hb->sensor_reading_pong_offset);
> +		dt_add_property_cells(node, "names-offset",
> +				      hb->sensor_names_offset);
> +	}
> +}
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 7904eee..fbe5753 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -301,4 +301,7 @@ 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);
> +
>  #endif /* __SKIBOOT_H */
>

On Thu, 2017-03-09 at 13:44 +0100, Cédric Le Goater wrote:
> Hello Shilpasri,
> 
> Thanks for the write up on the OCC sensors. Nice and detailed.
> 
> On the overall design, I think this is exposing too much HW details 
> to Linux. I am not sure Linux should even know about OCC or where
> the sensors are coming from. Even if this is doable, I would not 
> let the kernel loop on the OCC data to collect which sensors are
> available. What is being done under :
> 
> 	arch/powerpc/platforms/powernv/opal-occ-sensors.c
> 
> in the Linux driver should be in the firmware IHMO. 
> 
> skiboot should hide the gory details and expose in the device 
> tree sensor nodes collected from the different available sources: 
> FSP, BMC, OCC, DTS, etc. 
> 
> There is a framework in place in skiboot for that, which may be, 
> needs some tuning to add OCC support. But the good thing about it, 
> is that the Linux driver comes for free in *all* Linux distros now.

I spoke with Stewart a while back about how we should expose sensors to
Linux and we reached the same conclusions as Cedric. Lets keep the OCC
details contained in the firmware.

Cheers,

Andrew

Andrew Jeffery <andrew@aj.id.au> writes:
> On Thu, 2017-03-09 at 13:44 +0100, Cédric Le Goater wrote:
>> Hello Shilpasri,
>> 
>> Thanks for the write up on the OCC sensors. Nice and detailed.
>> 
>> On the overall design, I think this is exposing too much HW details 
>> to Linux. I am not sure Linux should even know about OCC or where
>> the sensors are coming from. Even if this is doable, I would not 
>> let the kernel loop on the OCC data to collect which sensors are
>> available. What is being done under :
>> 
>> 	arch/powerpc/platforms/powernv/opal-occ-sensors.c
>> 
>> in the Linux driver should be in the firmware IHMO. 
>> 
>> skiboot should hide the gory details and expose in the device 
>> tree sensor nodes collected from the different available sources: 
>> FSP, BMC, OCC, DTS, etc. 
>> 
>> There is a framework in place in skiboot for that, which may be, 
>> needs some tuning to add OCC support. But the good thing about it, 
>> is that the Linux driver comes for free in *all* Linux distros now.
>
> I spoke with Stewart a while back about how we should expose sensors to
> Linux and we reached the same conclusions as Cedric. Lets keep the OCC
> details contained in the firmware.

Yep. OCC is merely an implementation detail.

I'd argue that all sensors should be readable through the OPAL calls. As
an optimisation, we could add the ability for some opal sensors to be
read directly from an address by the kernel.

But yeah, for these sensors, existing kernels should "just work" by
using the existing OPAL calls.

Hi,

On 03/10/2017 05:54 AM, Stewart Smith wrote:
> Andrew Jeffery <andrew@aj.id.au> writes:
>> On Thu, 2017-03-09 at 13:44 +0100, Cédric Le Goater wrote:
>>> Hello Shilpasri,
>>>
>>> Thanks for the write up on the OCC sensors. Nice and detailed.
>>>
>>> On the overall design, I think this is exposing too much HW details 
>>> to Linux. I am not sure Linux should even know about OCC or where
>>> the sensors are coming from. Even if this is doable, I would not 
>>> let the kernel loop on the OCC data to collect which sensors are
>>> available. What is being done under :
>>>
>>> 	arch/powerpc/platforms/powernv/opal-occ-sensors.c
>>>
>>> in the Linux driver should be in the firmware IHMO. 
>>>
>>> skiboot should hide the gory details and expose in the device 
>>> tree sensor nodes collected from the different available sources: 
>>> FSP, BMC, OCC, DTS, etc. 
>>>
>>> There is a framework in place in skiboot for that, which may be, 
>>> needs some tuning to add OCC support. But the good thing about it, 
>>> is that the Linux driver comes for free in *all* Linux distros now.
>>
>> I spoke with Stewart a while back about how we should expose sensors to
>> Linux and we reached the same conclusions as Cedric. Lets keep the OCC
>> details contained in the firmware.
> 
> Yep. OCC is merely an implementation detail.
> 
> I'd argue that all sensors should be readable through the OPAL calls. As
> an optimisation, we could add the ability for some opal sensors to be
> read directly from an address by the kernel.
> 
> But yeah, for these sensors, existing kernels should "just work" by
> using the existing OPAL calls.
> 

Yes. I completely agree that the sensors should be exported in device tree. I
will send out a patch for the same in the next iteration.

But there are couple of concerns which this patch is trying to address:

1) If we end up exporting all sensors as device tree nodes then we will bloat
the device tree with 450 sensors per OCC. And it will also increase the OPAL
boot time.

2) Apart from the min/max for each sensor OCC provides three different min/max
which can be reset at runtime. I am not sure if it is a right way to create new
nodes for each min/max per sensor.

Thanks and Regards,
Shilpa

On 03/10/2017 01:24 AM, Stewart Smith wrote:
> Andrew Jeffery <andrew@aj.id.au> writes:
>> On Thu, 2017-03-09 at 13:44 +0100, Cédric Le Goater wrote:
>>> Hello Shilpasri,
>>>
>>> Thanks for the write up on the OCC sensors. Nice and detailed.
>>>
>>> On the overall design, I think this is exposing too much HW details 
>>> to Linux. I am not sure Linux should even know about OCC or where
>>> the sensors are coming from. Even if this is doable, I would not 
>>> let the kernel loop on the OCC data to collect which sensors are
>>> available. What is being done under :
>>>
>>> 	arch/powerpc/platforms/powernv/opal-occ-sensors.c
>>>
>>> in the Linux driver should be in the firmware IHMO. 
>>>
>>> skiboot should hide the gory details and expose in the device 
>>> tree sensor nodes collected from the different available sources: 
>>> FSP, BMC, OCC, DTS, etc. 
>>>
>>> There is a framework in place in skiboot for that, which may be, 
>>> needs some tuning to add OCC support. But the good thing about it, 
>>> is that the Linux driver comes for free in *all* Linux distros now.
>>
>> I spoke with Stewart a while back about how we should expose sensors to
>> Linux and we reached the same conclusions as Cedric. Lets keep the OCC
>> details contained in the firmware.
> 
> Yep. OCC is merely an implementation detail.
> 
> I'd argue that all sensors should be readable through the OPAL calls. As
> an optimisation, we could add the ability for some opal sensors to be
> read directly from an address by the kernel.

And  we would expose in the device tree an address to read the 
value from ? we would need a new reserved region I suppose.  

C.

 
> But yeah, for these sensors, existing kernels should "just work" by
> using the existing OPAL calls.
>

On 03/10/2017 08:20 AM, Shilpasri G Bhat wrote:
> Hi,
> 
> On 03/10/2017 05:54 AM, Stewart Smith wrote:
>> Andrew Jeffery <andrew@aj.id.au> writes:
>>> On Thu, 2017-03-09 at 13:44 +0100, Cédric Le Goater wrote:
>>>> Hello Shilpasri,
>>>>
>>>> Thanks for the write up on the OCC sensors. Nice and detailed.
>>>>
>>>> On the overall design, I think this is exposing too much HW details 
>>>> to Linux. I am not sure Linux should even know about OCC or where
>>>> the sensors are coming from. Even if this is doable, I would not 
>>>> let the kernel loop on the OCC data to collect which sensors are
>>>> available. What is being done under :
>>>>
>>>> 	arch/powerpc/platforms/powernv/opal-occ-sensors.c
>>>>
>>>> in the Linux driver should be in the firmware IHMO. 
>>>>
>>>> skiboot should hide the gory details and expose in the device 
>>>> tree sensor nodes collected from the different available sources: 
>>>> FSP, BMC, OCC, DTS, etc. 
>>>>
>>>> There is a framework in place in skiboot for that, which may be, 
>>>> needs some tuning to add OCC support. But the good thing about it, 
>>>> is that the Linux driver comes for free in *all* Linux distros now.
>>>
>>> I spoke with Stewart a while back about how we should expose sensors to
>>> Linux and we reached the same conclusions as Cedric. Lets keep the OCC
>>> details contained in the firmware.
>>
>> Yep. OCC is merely an implementation detail.
>>
>> I'd argue that all sensors should be readable through the OPAL calls. As
>> an optimisation, we could add the ability for some opal sensors to be
>> read directly from an address by the kernel.
>>
>> But yeah, for these sensors, existing kernels should "just work" by
>> using the existing OPAL calls.
>>
> 
> Yes. I completely agree that the sensors should be exported in device tree. I
> will send out a patch for the same in the next iteration.
> 
> But there are couple of concerns which this patch is trying to address:
> 
> 1) If we end up exporting all sensors as device tree nodes then we will bloat
> the device tree with 450 sensors per OCC. And it will also increase the OPAL
> boot time.

Let's see how time it takes first and may be we can filter some of them.

> 
> 2) Apart from the min/max for each sensor OCC provides three different min/max
> which can be reset at runtime. I am not sure if it is a right way to create new
> nodes for each min/max per sensor.

I would say that the 'Latest sample' should be enough.

Thanks,

C.