Message-ID: <51EFED19.5090900@ti.com>
Date:	Wed, 24 Jul 2013 11:04:57 -0400
From:	Eduardo Valentin <eduardo.valentin@...com>
To:	Pawel Moll <pawel.moll@....com>
CC:	Eduardo Valentin <eduardo.valentin@...com>,
	Stephen Warren <swarren@...dotorg.org>,
	Mark Rutland <Mark.Rutland@....com>,
	Ian Campbell <ian.campbell@...rix.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Guenter Roeck <linux@...ck-us.net>,
	Durgadoss R <durgadoss.r@...el.com>,
	"Zhang, Rui" <rui.zhang@...el.com>, Wei Ni <wni@...dia.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: RFC: device thermal limits represented in device tree nodes


Pawel,

On 24-07-2013 07:19, Pawel Moll wrote:
> Greeting,
> 
> Funnily enough I had this discussion couple a months ago and made my
> mind back then :-)
> 

:-)

>> On 07/22/2013 07:25 AM, Eduardo Valentin wrote:
>>> representing in device tree would not
>>> infringe the original purpose of this data structure  ("The Device
>>> Tree is a data structure for describing hardware."[2]).
> 
> I couldn't agree more, with a few remarks...
> 

Nice.

>>> In this current proposal, a 'thermal_zone' node would be embedded
>>> inside a temperature sensor node, for simplicity. But other
>>> possible builds could embedded them in the device with thermal
>>> limits (CPU nodes, for instance) or they could be not embedded in
>>> any specific node.
> 
> So this is the detail that actually caused most of the disagreement :-)
> 
> My position on this can be summarized as follows:
> 
> 1. As you have pointed out, the thermal limits are related to the
> *device being monitored*, not the sensor itself.
> 

Yeah, thinking of it now, this original proposal, it lacks a stronger
relationship mapping between monitored and monitoring devices. But it
does have it..

> 2. Therefore the tree should express relation between those two; a
> sensor mode should be connected (via phandle most likely) to the device

.. this is done, more or less, by means of the 'type' property (see
original RFC binding).

> being monitored, eg. (I'm not suggesting any property names, just trying
> to express the idea ;-) memory mapped PVT "sensor" monitoring a core
> (cpu) temperature could look like this:
> 	cpu0: cpu@0 {
> 	};
> 
> 	sensor@...x {
> 		reg = <xxx>;
> 		device_monitored = <&cpu0>;
> 	};

To, this is not a sensor property. It could be a property in the
thermal_zone node itself.

> while I2C or 1Wire sensor measuring temperature of the board (or even
> inside the device enclosure) would point at the root of the tree.
> 
> 3. Basing on the above, the thermal limits of the device could be
> described in the tree (again, don't pay attention to naming):
> 	cpu0: cpu@0 {
> 		thermal_limits {
> 			cooling {
> 			};
> 			critical {
> 			};
> 		};
> 	};
> or "hardcoded" in the device driver. After all, as you have pointed out,

For example:
cpu0: cpu@0 {
	/* ... cpu needed bindings  */

	thermal_zone {
		type = "CPU";

		monitoring_device = <&sensor@...x
				    &sensor@...y>;

		mask = <0x03>; /* trips writability */
		passive_delay = <250>; /* milliseconds */
		polling_delay = <1000>; /* milliseconds */
		policy = "step_wise";
		trips {
			alert@...000{
				temperature = <100000>; /* milliCelsius
				hysteresis = <2000>; /* milliCelsius */
				type = <THERMAL_TRIP_PASSIVE>;
			};
			crit@...000{
				temperature = <125000>; /* milliCelsius
				hysteresis = <2000>; /* milliCelsius */
				type = <THERMAL_TRIP_CRITICAL>;
			};
		};
		bind_params {
			action@0{
				cooling_device = "thermal-cpufreq";
				weight = <100>; /* percentage */
				mask = <0x01>;
				/* no limits, using defaults */
			};
		};
	};
};

/* .. other binding .. */

sensor@...x {
	reg = <xxx>;
	compatible = <xxx>;
};

sensor@...y {
	reg = <yyyy>;
	compatible = <yyyy>;
};


The above way, I think it is less intrusive to sensor bindings. Besides,
it gives the flexibility to have more that one sensor monitoring a
thermal zone. In Linux we don't have this support, but in practice, we
do have this requirement. Durgadoss has proposed an improvement of the
thermal framework as a RFC. So, the support to have several sensors per
zone will come.

Another way, as I mentioned in the original RFC, an option would be to
have the thermal_zone node not embedded in any device node. But them, we
would need to firmly link it to other device nodes, to describe what is
monitored and what is used for monitoring. Something like:
thermal_zone {
	type = "CPU";
	monitored_device = <&cpu0>;
	monitoring_device = <&sensor@...x
			    &sensor@...y>;
	mask = <0x03>; /* trips writability */
	passive_delay = <250>; /* milliseconds */
	polling_delay = <1000>; /* milliseconds */
	policy = "step_wise";
	trips {
		alert@...000{
			temperature = <100000>; /* milliCelsius
			hysteresis = <2000>; /* milliCelsius */
			type = <THERMAL_TRIP_PASSIVE>;
		};
		crit@...000{
			temperature = <125000>; /* milliCelsius
			hysteresis = <2000>; /* milliCelsius */
			type = <THERMAL_TRIP_CRITICAL>;
		};
	};
	bind_params {
		action@0{
			cooling_device = "thermal-cpufreq";
			weight = <100>; /* percentage */
			mask = <0x01>;
			/* no limits, using defaults */
		};
	};
};

With the above I believe we could have dts(i) files describing only
thermal, for instance.

> the limits are the device's characteristic, so I see no problem with the
> "SOC driver" registering the trip points on its own. I'm sure there will
> be situations when it's better to parametrize such data via Device Tree,
> so it makes perfect sense to have bindings defined for such occasion.
> Anyway, as long as the tree describes the relation between the monitored
> device, the sensor and the cooling device all should work.
> 
> Now, my personal opinion is that the tree should focus at "system level"
> data (ie. how is the device connected to others) and describe as little
> information that can be probed in runtime - a small (small!) array of
> silicon variants in the driver doesn't hurt (unless we're talking about
> hundreds of possibilities, where the device tree is obviously the place
> to have them). But this is just a rule of thumb I'm following.
> 
> Cheers!
> 
> Pawel
> 
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


Download attachment "signature.asc" of type "application/pgp-signature" (296 bytes)

Powered by blists - more mailing lists