| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140830194318.GA3025@developer>
Date: Sat, 30 Aug 2014 15:43:21 -0400
From: Eduardo Valentin <edubezval@...il.com>
To: Wei Ni <wni@...dia.com>
Cc: Jean Delvare <jdelvare@...e.de>,
Guenter Roeck <linux@...ck-us.net>,
Stephen Warren <swarren@...dotorg.org>,
lm-sensors@...sensors.org,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/4] thermal: add more description for thermal-zones
Hello Wei,
On Fri, Sep 05, 2014 at 05:41:12PM +0800, Wei Ni wrote:
> Hi, Eduardo
>
> On 09/01/2014 06:26 PM, Wei Ni wrote:
> > On 08/29/2014 07:32 PM, edubezval@...il.com wrote:
> >> Hello Wei,
> >>
> >> On Thu, Aug 28, 2014 at 11:03 PM, Wei Ni <wni@...dia.com> wrote:
> >>> On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
> >>>> Hello Wei,
> >>>>
> >>>> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
> >>>>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
> >>>>>> Hello Wei,
> >>>>>>
> >>>>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
> >>>>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
> >>>>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
> >>>>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> >>>>>>>>>> Hello Wei Ni,
> >>>>>>>>>>
> >>>>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> >>>>>>>>>>> Add more description for the "polling-delay" property.
> >>>>>>>>>>> Set "trips" and "cooling maps" as optional property, because
> >>>>>>>>>>> if missing these two sub-nodes, the thermal zone device still
> >>>>>>>>>>> work properly.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Wei Ni <wni@...dia.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>> Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
> >>>>>>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>>>>>>> index f5db6b7..e3d3ed9 100644
> >>>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
> >>>>>>>>>>>
> >>>>>>>>>>> Required properties:
> >>>>>>>>>>> - polling-delay: The maximum number of milliseconds to wait between polls
> >>>>>>>>>>> - Type: unsigned when checking this thermal zone.
> >>>>>>>>>>> - Size: one cell
> >>>>>>>>>>> + Type: unsigned when checking this thermal zone. If this value is 0, the
> >>>>>>>>>>> + Size: one cell driver will not run polling queue, but just cancel it.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> The description above is specific to Linux kernel implementation
> >>>>>>>>>> nomenclature. DT description needs to be OS agnostic.
> >>>>>>>>>>
> >>>>>>>>>>> - polling-delay-passive: The maximum number of milliseconds to wait
> >>>>>>>>>>> Type: unsigned between polls when performing passive cooling.
> >>>>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
> >>>>>>>>>>> phandles + sensor
> >>>>>>>>>>> specifier
> >>>>>>>>>>>
> >>>>>>>>>>> +Optional property:
> >>>>>>>>>>> - trips: A sub-node which is a container of only trip point nodes
> >>>>>>>>>>> Type: sub-node required to describe the thermal zone.
> >>>>>>>>>>>
> >>>>>>>>>>> - cooling-maps: A sub-node which is a container of only cooling device
> >>>>>>>>>>> Type: sub-node map nodes, used to describe the relation between trips
> >>>>>>>>>>> - and cooling devices.
> >>>>>>>>>>> + and cooling devices. If missing the "trips" property,
> >>>>>>>>>>> + This sub-node will not be parsed, because no trips can
> >>>>>>>>>>> + be bound to cooling devices.
> >>>>>>>>>>
> >>>>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
> >>>>>>>>>> the binding describes both, cooling-maps and trips, as required
> >>>>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
> >>>>>>>>>> as a proper described zone.
> >>>>>>>>>
> >>>>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
> >>>>>>>>> missing these two properties, the thermal zone devices still can be
> >>>>>>>>> registered, and the driver can work properly, it has the basic function,
> >>>>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
> >>>>>>>>> and bind with cooling devices.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> If a thermal zone is used only for monitoring, then I believe it lost
> >>>>>>>> its purpose. As Maybe a different framework shall be used, such as hwmon,
> >>>>>>>> for instance?
> >>>>>>>
> >>>>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
> >>>>>>> more functions base on these two thermal zone devices. We have a
> >>>>>>> skin-temperature driver, which used nct1008's remote and local
> >>>>>>> temperatures to estimator the skin temperature. As you know the thermal
> >>>>>>> framework is more powerful, the remote/local sensors can be register as
> >>>>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
> >>>>>>> to read their temperatures and then estimator skin's temp. We also will
> >>>>>>> set trips and cooling devices for this skin-temp.
> >>>>>>
> >>>>>> In this case, don't you think it would make sense to create a thermal
> >>>>>> zone for the skin temperature and add the required sensors (including
> >>>>>> the nct1008) in it?
> >>>>>
> >>>>> Hi, Eduardo
> >>>>> Yes, we will create a thermal zone for the skin-temp driver and add the
> >>>>> required sensors in it, but in here we want to register these required
> >>>>> sensors as thermal zone devices, then the thermal framework can export
> >>>>> functions to read these sensors temperature. If use hwmon framework, it
> >>>>> can't export nct1008's internal sensors, such as remote/local sensors,
> >>>>> and no exported functions to read these temperatures. If we doesn't use
> >>>>> the thermal/of-thermal framework, we have to develop a new framework to
> >>>>> parse those sensor nodes, and I think this new one may only have slight
> >>>>> difference with current thermal framework.
> >>>>> If we set those two properties as optional property, then we can use it
> >>>>> in this case simply.
> >>>>>
> >>>>> The skin-temp nodes will something like this:
> >>>>>
> >>>>> skin_temp: therm-est {
> >>>>> compatible = "nvidia,tegra124-therm-est";
> >>>>>
> >>>>> #thermal-sensor-cells = <0>;
> >>>>>
> >>>>> sub-devs {
> >>>>> dev@0 {
> >>>>> /* we need to register nct1008_remote as thermal zone devices, so that
> >>>>> it's easy to handle it by using thermal framework's exported functions. */
> >>>>> dev = <&nct1008_remote>;
> >>>>> };
> >>>>> dev@1 {
> >>>>> dev = <&nct1008_local>;
> >>>>> };
> >>>>> };
> >>>>> };
> >>>>>
> >>>>> thermal-zones {
> >>>>> skin-therm {
> >>>>> polling-delay-passive = <15000>
> >>>>> polling-delay = <0>;
> >>>>>
> >>>>> thermal-sensors = <&skin_temp>;
> >>>>
> >>>> The binding allows you to add several sensors in one thermal zone.
> >>>> Please have a look on the example (c) of the thermal.txt binding
> >>>> description. It might be that what you want to do is actually:
> >>>> thermal-sensors = <&ntc1008_local>,
> >>>> <&ntc1008_remote>;
> >>>
> >>>
> >>> Yes, we have considered it, but in current kernel, it only supports 1
> >>> sensor per zone. and according to the example (c), it's supposed to only
> >>> support simple algorithm, such as:
> >>> /* hotspot = 100 * bandgap - 120 * adc + 484 */
> >>> coefficients = <100 -120 484>;
> >>>
> >>> but in our skin-temp driver, we will recored 20 history temperatures for
> >>> every sensors, and set different coefficients for every history
> >>> temperatures, our HW team have private mathematical mode to calculate
> >>> these coeffs for different boards. And I think other users may use
> >>> different algorithm to calculate thermal zone temperature based on
> >>> several sensors.
> >>>
> >>> So I think it's better to register sensors to thermal zones, not set
> >>> trips and binding cooling devices, then user's thermal driver can use
> >>> them freely.
> >>>
> >>
> >> In your system, the used governor is userspace, then?
> >
> > No, we developed a new governor named as "pid governor", it manages
> > thermals based on output values of PID controller.
Have you tried the patch set from Javi? the power allocator schem
includes a PID controller governor. Might be worth having a common
solution. I think it is easier if I merge his changes into a branch and
you have try on it. His patch series have already gone through several
reviews. But you are free to comment too.
> >
> > Indeed, our thermal management includes skin-temp, soc-thermal,
> > cpu-throttle driver and pid governor. This pach set is prepared for the
> > skin-temp driver.
> > As you know the tegra soc-therm is in upstreaming, and we will post
> > other drivers step by step.
>
> Will you take this patch, if so i will post skin-temp driver later.
> Looking forward your comments :)
>
I am afraid moving the maps and trips properties to optional abuses the
purpose of the thermal DT. The primary goal is to describe hardware, and
in this specific case, the hardware proper thermal operating conditions.
DT has to be as agnostic to OS implementation as possible.
For the above reason, I do not think this patch is a good idea.
Cheers,
> Thanks.
> Wei.
>
> >
> > Thanks.
> > Wei.
> >
> >>
> >>> Thanks.
> >>> Wei.
> >>>
> >>>>
> >>>>
> >>>>>
> >>>>> trips {
> >>>>> };
> >>>>> cooling-maps {
> >>>>> };
> >>>>> };
> >>>>> };
> >>>>>
> >>>>> Wei.
> >>>>>>
> >>>>>>>
> >>>>>>> Wei.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
> >>>>>>>> hardware. As it is mentioned in the thermal.txt file.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>> Wei.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> -Optional property:
> >>>>>>>>>>> - coefficients: An array of integers (one signed cell) containing
> >>>>>>>>>>> Type: array coefficients to compose a linear relation between
> >>>>>>>>>>> Elem size: one cell the sensors listed in the thermal-sensors property.
> >>>>>>>>>>> --
> >>>>>>>>>>> 1.8.1.5
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>> --
> >>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> >>>>>>>> the body of a message to majordomo@...r.kernel.org
> >>>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>
> >>
> >>
> >>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists