From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Valentin Subject: Re: [RFC PATCH 3/4] thermal: ti-soc-thermal: use thermal DT infrastructure Date: Mon, 15 Jul 2013 09:38:42 -0400 Message-ID: <51E3FB62.1070106@ti.com> References: <1373378414-28086-1-git-send-email-eduardo.valentin@ti.com> <1373378414-28086-4-git-send-email-eduardo.valentin@ti.com> <51E3EC2C.2030803@ti.com> <1373893198.4172.28.camel@weser.hi.pengutronix.de> <51E3F864.1060000@ti.com> <51E3FAE7.3090609@ti.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2PWADFEKSPDJOSUIXETDA" Return-path: Received: from comal.ext.ti.com ([198.47.26.152]:46588 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755349Ab3GONiu (ORCPT ); Mon, 15 Jul 2013 09:38:50 -0400 In-Reply-To: <51E3FAE7.3090609@ti.com> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Eduardo Valentin Cc: Lucas Stach , linux-pm@vger.kernel.org ------enig2PWADFEKSPDJOSUIXETDA Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 15-07-2013 09:36, Eduardo Valentin wrote: > On 15-07-2013 09:25, Eduardo Valentin wrote: >> On 15-07-2013 08:59, Lucas Stach wrote: >>> Am Montag, den 15.07.2013, 08:33 -0400 schrieb Eduardo Valentin: >>>> On 15-07-2013 08:12, Lucas Stach wrote: >>>>> Hi Eduardo and others, >>>>> >>>>> Eduardo Valentin ti.com> writes: >>>>> >>>>>> >>>>>> This patch improves the ti-soc-thermal driver by adding the >>>>>> support to build the thermal zones based on DT nodes. >>>>>> >>>>>> The driver will have two options now to build the thermal >>>>>> zones. The first option is the zones originally coded >>>>>> in this driver. So, the driver behavior will be same >>>>>> if there is no DT node describing the zones. The second >>>>>> option, when it is found a DT node with thermal data, >>>>>> will used the common infrastructure to build the thermal >>>>>> zone and bind its cooling devices. >>>>>> >>>>>> In either case, this driver still adds to the system >>>>>> a cpufreq cooling device. >>>>>> >>>>> >>>>> I really like the idea to configure thermal zones using devicetree,= it's a >>>>> step in the right direction. We might follow suit with the i.MX6 te= mpmon >>>>> driver to use this infrastructure. >>>>> >>>>> What I strongly dislike is the notion of the sensor drivers instant= iating >>>>> the cooling devices and the resulting devicetree binding. This seem= s really >>>>> backward to me. >>>>> I would rather see the drivers associated with the cooling devices = (like >>>>> cpufreq and the respective gpu drivers) to instantiate the cooling = devices >>>>> and the thermal zone referring to them through phandles. I think it= >>>>> shouldn't be too much work to go in that direction. >>>>> The current method might be enough to work with the current thermal= platform >>>>> drivers, but if you want to go down the route to eventually use pla= in i2c >>>>> devices (likely with an existing hwmon driver) you have to get away= from the >>>>> sensor devices instantiating the cooling devices. >>>> >>>> I agree with you. While implementing the RFC, it looks awkward that = the >>>> ti-soc-thermal driver had to do the job to load the cpufreq-cooling >>>> device. Problem is that a cooling device is not really a real device= , >>>> and might get flamed while represented as a device tree node. >>>> >>> I don't think we even need to represent this into the device tree. We= >>> already have the CPU nodes there and the cpu-freq driver is already >>> linked to those. It should be easy to have a global list of registere= d >>> thermal devices in the thermal framework together with their associat= ed >>> devices, so one could look up cooling devices through the thermal >>> framework when we only have a phandle to the cpu node. >> >> Well, we do have a list of thermal cooling devices associated with its= >> corresponding struct device. That is private data to the thermal >> framework. However, one needs to load the cooling device in order to i= ts >> data structure appear in this list. The framework can not be proactive= >> here. Some other entity needs to see the need and inform the thermal >> framework of it. >> >=20 >=20 >=20 > as simple as the following: > diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-c= pu0.c > index 3ab8294..486881c 100644 > --- a/drivers/cpufreq/cpufreq-cpu0.c > +++ b/drivers/cpufreq/cpufreq-cpu0.c > @@ -20,6 +20,9 @@ > #include > #include > #include > +#include > +#include > +#include >=20 > static unsigned int transition_latency; > static unsigned int voltage_tolerance; /* in percentage */ > @@ -28,6 +31,7 @@ static struct device *cpu_dev; > static struct clk *cpu_clk; > static struct regulator *cpu_reg; > static struct cpufreq_frequency_table *freq_table; > +static struct thermal_cooling_device *cdev; >=20 > static int cpu0_verify_speed(struct cpufreq_policy *policy) > { > @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_devic= e > *pdev) > goto out_free_table; > } >=20 > + if (!of_property_read_bool(np, "needs-cooling")) This is obviously supposed to be + if (of_property_read_bool(np, "needs-cooling")) > + cdev =3D cpufreq_cooling_register(cpu_present_mask); > + > of_node_put(np); > of_node_put(parent); > return 0; > @@ -269,6 +276,7 @@ out_put_node: >=20 > static int cpu0_cpufreq_remove(struct platform_device *pdev) > { > + cpufreq_cooling_unregister(cdev); > cpufreq_unregister_driver(&cpu0_cpufreq_driver); > opp_free_cpufreq_table(cpu_dev, &freq_table); >=20 >> For instance, assuming that all systems will need a cpufreq cooling >> device is a flaw, because that is not the case. Thus, it makes sense t= o >> have a property, say at the cpu node, to determine that it needs >> cooling. However, that won't be saying how it would cool off. >=20 >=20 > Then you would define your cpu0 node as: >=20 > cpu@0 { > /* OMAP443x variants OPP50-OPPNT */ > operating-points =3D < > /* kHz uV */ > 300000 1025000 > 600000 1200000 > 800000 1313000 > 1008000 1375000 > >; > clock-latency =3D <300000>; /* From legacy driver */ > needs-cooling; /* make sure we have cpufreq-cooling */ > }; >=20 > Because in that system we actually need to take care of the cpu thermal= =2E >=20 >=20 >> >> >>> >>>> I could try to push something following the same idea as the one I a= m >>>> trying to sell with this series for sensor devices. For instance, in= a >>>> sensor node I am attaching a phandle to describe how thermal fw must= >>>> behave. Then the sensor driver it is supposed to load the thermal da= ta >>>> into the thermal fw. Same could apply for instance for cpufreq cooli= ng >>>> device. at the cpu node we could have a 'cooling_device' node at the= cpu >>>> node, while loading cpufreq-cpu0. >>> >>> I think a separate cooling_device node may be only necessary if we st= uff >>> additional info in there. If it's just a plain cooling device I think= it >>> is reasonable for the cpufreq driver to just register a cooling devic= e >>> if the thermal framework is there. >> >> no, I think this is not what we want, because, as I said, not all cpus= >> will need cooling. Just because the thermal framework is there does no= t >> mean your cpu needs cooling. As you can see, the thermal framework is >> not only for cpu cooling. It can be used for any other thermal need. >> Besides one needs to cover for the case where you are building for >> multiple platform support. Assuming system needs based on Kconfig setu= p >> is not very likely to scale in this case. >> >>> >>> I would really like the information about a thermal zone to hang off = one >>> dt node rather than being scattered over several nodes. This way it m= ay >> >> Again, thermal framework is not about only cpu(freq) cooling. Thermal >> zone info can (and will) be hanged off in one dt node. But please don'= t >> mix concepts. Just because a cooling device is part of a thermal zone,= >> it does not mean it is only used there and that it can be defined ther= e. >> One can use a cooling device in different thermal zones. >> >>> be easy to reference a cooling device in different thermal zones with= >>> different weight, etc. As long as we define a thermal zone to always = be >>> defined by a single sensor the right place seems to be the proposed >>> subnode to the sensor. If we want a zone to have more than one sensor= , >>> we might even want a separate dt node for the thermal zone, referenci= ng >>> both sensors and cooling devices through phandles. >> >> I still don't get why and how defining a thermal zone inside a sensor >> phandle can prevent us defining a cooling device in different device >> phandle. >=20 >=20 > Then you can keep everything about your thermal zone in one single > phandle, as follows, but remember, this is is the info about the therma= l > zone, not about a cooling device. For instance, that is the zone built > on top of a bandgap sensor: > bandgap { > reg =3D <0x4a002260 0x4 0x4a00232C 0x4>; > compatible =3D "ti,omap4430-bandgap"; > thermal_zone { > type =3D "CPU"; > mask =3D <0x03>; /* trips writability */ > passive_delay =3D <250>; /* milliseconds */ > polling_delay =3D <1000>; /* milliseconds */ > governor =3D "step_wise"; > trips { > alert@100000{ > temperature =3D <100000>; > hysteresis =3D <0>; > type =3D <1>; > }; > crit@125000{ > temperature =3D <125000>; > hysteresis =3D <0>; > type =3D <3>; > }; > }; > bind_params { > action@0{ > cooling_device =3D "thermal-cpufreq"; > weight =3D <100>; /* percentage */ > mask =3D <0x01>; > }; > }; > }; > }; >=20 >=20 > And you see that, in this case, the bandgap sensor driver does not need= > to worry about loading the cpufreq cooling device anymore. Who is > responsible of doing that is the cpufreq driver, with the above > proposal, when it makes sense and when there is a need. >=20 >=20 >=20 >> >>> >>> Regards, >>> Lucas >>> >> >> >=20 >=20 --=20 You have got to be excited about what you are doing. (L. Lamport) Eduardo Valentin ------enig2PWADFEKSPDJOSUIXETDA Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iF4EAREIAAYFAlHj+2IACgkQCXcVR3XQvP0FMAD/dM/fTWD45qOkS8FjpmkPHG4h Bbz9LUxa/Y1zffjy5MMBAKu6+X7Sw0L5ufiBhB3EYPSJ0M+fcIc9I/j3hzHbQeax =zT3c -----END PGP SIGNATURE----- ------enig2PWADFEKSPDJOSUIXETDA--