| Message ID | 51E3FAE7.3090609@ti.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
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 <eduardo.valentin <at> 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 tempmon >>>>> driver to use this infrastructure. >>>>> >>>>> What I strongly dislike is the notion of the sensor drivers instantiating >>>>> the cooling devices and the resulting devicetree binding. This seems 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 plain 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 registered >>> thermal devices in the thermal framework together with their associated >>> 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 its >> 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. >> > > > > as simple as the following: > diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c > index 3ab8294..486881c 100644 > --- a/drivers/cpufreq/cpufreq-cpu0.c > +++ b/drivers/cpufreq/cpufreq-cpu0.c > @@ -20,6 +20,9 @@ > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > +#include <linux/thermal.h> > +#include <linux/cpu_cooling.h> > +#include <linux/cpumask.h> > > 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; > > static int cpu0_verify_speed(struct cpufreq_policy *policy) > { > @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_device > *pdev) > goto out_free_table; > } > > + if (!of_property_read_bool(np, "needs-cooling")) This is obviously supposed to be + if (of_property_read_bool(np, "needs-cooling")) > + cdev = cpufreq_cooling_register(cpu_present_mask); > + > of_node_put(np); > of_node_put(parent); > return 0; > @@ -269,6 +276,7 @@ out_put_node: > > 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); > >> 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 to >> 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. > > > Then you would define your cpu0 node as: > > cpu@0 { > /* OMAP443x variants OPP50-OPPNT */ > operating-points = < > /* kHz uV */ > 300000 1025000 > 600000 1200000 > 800000 1313000 > 1008000 1375000 > >; > clock-latency = <300000>; /* From legacy driver */ > needs-cooling; /* make sure we have cpufreq-cooling */ > }; > > Because in that system we actually need to take care of the cpu thermal. > > >> >> >>> >>>> I could try to push something following the same idea as the one I am >>>> 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 data >>>> into the thermal fw. Same could apply for instance for cpufreq cooling >>>> 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 stuff >>> 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 device >>> 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 not >> 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 setup >> 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 may >> >> 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 there. >> 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, referencing >>> 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. > > > Then you can keep everything about your thermal zone in one single > phandle, as follows, but remember, this is is the info about the thermal > zone, not about a cooling device. For instance, that is the zone built > on top of a bandgap sensor: > bandgap { > reg = <0x4a002260 0x4 0x4a00232C 0x4>; > compatible = "ti,omap4430-bandgap"; > thermal_zone { > type = "CPU"; > mask = <0x03>; /* trips writability */ > passive_delay = <250>; /* milliseconds */ > polling_delay = <1000>; /* milliseconds */ > governor = "step_wise"; > trips { > alert@100000{ > temperature = <100000>; > hysteresis = <0>; > type = <1>; > }; > crit@125000{ > temperature = <125000>; > hysteresis = <0>; > type = <3>; > }; > }; > bind_params { > action@0{ > cooling_device = "thermal-cpufreq"; > weight = <100>; /* percentage */ > mask = <0x01>; > }; > }; > }; > }; > > > 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. > > > >> >>> >>> Regards, >>> Lucas >>> >> >> > >
Am Montag, den 15.07.2013, 09:36 -0400 schrieb Eduardo Valentin: [...] > > > as simple as the following: > diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c > index 3ab8294..486881c 100644 > --- a/drivers/cpufreq/cpufreq-cpu0.c > +++ b/drivers/cpufreq/cpufreq-cpu0.c > @@ -20,6 +20,9 @@ > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > +#include <linux/thermal.h> > +#include <linux/cpu_cooling.h> > +#include <linux/cpumask.h> > > 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; > > static int cpu0_verify_speed(struct cpufreq_policy *policy) > { > @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_device > *pdev) > goto out_free_table; > } > > + if (!of_property_read_bool(np, "needs-cooling")) > + cdev = cpufreq_cooling_register(cpu_present_mask); > + > of_node_put(np); > of_node_put(parent); > return 0; > @@ -269,6 +276,7 @@ out_put_node: > > 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); > > > 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 to > > 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. > > > Then you would define your cpu0 node as: > > cpu@0 { > /* OMAP443x variants OPP50-OPPNT */ > operating-points = < > /* kHz uV */ > 300000 1025000 > 600000 1200000 > 800000 1313000 > 1008000 1375000 > >; > clock-latency = <300000>; /* From legacy driver */ > needs-cooling; /* make sure we have cpufreq-cooling */ > }; > > Because in that system we actually need to take care of the cpu thermal. > I don't see what not registering the cooling device is buying us (aside from a small resource saving). The cpu is one potential source of heat in a system, so we may want to reference it in a thermal zone, so to me it makes sense to always register the cooling device. > > > > > > >> > >>> I could try to push something following the same idea as the one I am > >>> 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 data > >>> into the thermal fw. Same could apply for instance for cpufreq cooling > >>> 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 stuff > >> 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 device > >> 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 not > > 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 setup > > 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 may > > > > 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 there. > > 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, referencing > >> 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. > > > Then you can keep everything about your thermal zone in one single > phandle, as follows, but remember, this is is the info about the thermal > zone, not about a cooling device. For instance, that is the zone built > on top of a bandgap sensor: > bandgap { > reg = <0x4a002260 0x4 0x4a00232C 0x4>; > compatible = "ti,omap4430-bandgap"; > thermal_zone { > type = "CPU"; > mask = <0x03>; /* trips writability */ > passive_delay = <250>; /* milliseconds */ > polling_delay = <1000>; /* milliseconds */ > governor = "step_wise"; > trips { > alert@100000{ > temperature = <100000>; > hysteresis = <0>; > type = <1>; > }; > crit@125000{ > temperature = <125000>; > hysteresis = <0>; > type = <3>; > }; > }; > bind_params { > action@0{ > cooling_device = "thermal-cpufreq"; > weight = <100>; /* percentage */ > mask = <0x01>; > }; > }; > }; > }; > > > 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. Yes, this makes perfect sense to me. What I would like is to have the links more specific in the devicetree, so to me this stringmatching thing doesn't look too appealing, as it makes it harder to follow the links just looking at the DT. That's why I would prefer them to be phandles, so I could do something like: bind_params { action@0 { cooling_device = <&cpu@0>; weight = <40>; /* percentage */ mask = <0x01>; }; action@1 { cooling_device = <&gpu3d>; weigth = <60>; mask = <0x01>; };
On 15-07-2013 10:05, Lucas Stach wrote: > Am Montag, den 15.07.2013, 09:36 -0400 schrieb Eduardo Valentin: > [...] >> >> >> as simple as the following: >> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c >> index 3ab8294..486881c 100644 >> --- a/drivers/cpufreq/cpufreq-cpu0.c >> +++ b/drivers/cpufreq/cpufreq-cpu0.c >> @@ -20,6 +20,9 @@ >> #include <linux/platform_device.h> >> #include <linux/regulator/consumer.h> >> #include <linux/slab.h> >> +#include <linux/thermal.h> >> +#include <linux/cpu_cooling.h> >> +#include <linux/cpumask.h> >> >> 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; >> >> static int cpu0_verify_speed(struct cpufreq_policy *policy) >> { >> @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_device >> *pdev) >> goto out_free_table; >> } >> >> + if (!of_property_read_bool(np, "needs-cooling")) >> + cdev = cpufreq_cooling_register(cpu_present_mask); >> + >> of_node_put(np); >> of_node_put(parent); >> return 0; >> @@ -269,6 +276,7 @@ out_put_node: >> >> 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); >> >>> 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 to >>> 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. >> >> >> Then you would define your cpu0 node as: >> >> cpu@0 { >> /* OMAP443x variants OPP50-OPPNT */ >> operating-points = < >> /* kHz uV */ >> 300000 1025000 >> 600000 1200000 >> 800000 1313000 >> 1008000 1375000 >> >; >> clock-latency = <300000>; /* From legacy driver */ >> needs-cooling; /* make sure we have cpufreq-cooling */ >> }; >> >> Because in that system we actually need to take care of the cpu thermal. >> > I don't see what not registering the cooling device is buying us (aside > from a small resource saving). The cpu is one potential source of heat > in a system, so we may want to reference it in a thermal zone, so to me > it makes sense to always register the cooling device. The 'aside from a small resource saving' that bugs me :-). And conceptually, to me it won't be correct to load stuff you don't need, specially the 'always' loading part of it. > >> >>> >>> >>>> >>>>> I could try to push something following the same idea as the one I am >>>>> 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 data >>>>> into the thermal fw. Same could apply for instance for cpufreq cooling >>>>> 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 stuff >>>> 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 device >>>> 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 not >>> 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 setup >>> 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 may >>> >>> 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 there. >>> 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, referencing >>>> 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. >> >> >> Then you can keep everything about your thermal zone in one single >> phandle, as follows, but remember, this is is the info about the thermal >> zone, not about a cooling device. For instance, that is the zone built >> on top of a bandgap sensor: >> bandgap { >> reg = <0x4a002260 0x4 0x4a00232C 0x4>; >> compatible = "ti,omap4430-bandgap"; >> thermal_zone { >> type = "CPU"; >> mask = <0x03>; /* trips writability */ >> passive_delay = <250>; /* milliseconds */ >> polling_delay = <1000>; /* milliseconds */ >> governor = "step_wise"; >> trips { >> alert@100000{ >> temperature = <100000>; >> hysteresis = <0>; >> type = <1>; >> }; >> crit@125000{ >> temperature = <125000>; >> hysteresis = <0>; >> type = <3>; >> }; >> }; >> bind_params { >> action@0{ >> cooling_device = "thermal-cpufreq"; >> weight = <100>; /* percentage */ >> mask = <0x01>; >> }; >> }; >> }; >> }; >> >> >> 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. > > Yes, this makes perfect sense to me. What I would like is to have the > links more specific in the devicetree, so to me this stringmatching > thing doesn't look too appealing, as it makes it harder to follow the > links just looking at the DT. That's why I would prefer them to be > phandles, so I could do something like: > > bind_params { > action@0 { > cooling_device = <&cpu@0>; > weight = <40>; /* percentage */ > mask = <0x01>; > }; > action@1 { > cooling_device = <&gpu3d>; > weigth = <60>; > mask = <0x01>; > }; > I see, but the matching won't work at device tree anyway. But I understand your point. However, those would need to be 'cooling device' phandles, not 'cpu' phandles or 'gpu3d' phandles, in order to this make really sense.
Hi Eduardo, Am Montag, den 15.07.2013, 10:14 -0400 schrieb Eduardo Valentin: > On 15-07-2013 10:05, Lucas Stach wrote: > > Am Montag, den 15.07.2013, 09:36 -0400 schrieb Eduardo Valentin: > > [...] > >> > >> > >> as simple as the following: > >> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c > >> index 3ab8294..486881c 100644 > >> --- a/drivers/cpufreq/cpufreq-cpu0.c > >> +++ b/drivers/cpufreq/cpufreq-cpu0.c > >> @@ -20,6 +20,9 @@ > >> #include <linux/platform_device.h> > >> #include <linux/regulator/consumer.h> > >> #include <linux/slab.h> > >> +#include <linux/thermal.h> > >> +#include <linux/cpu_cooling.h> > >> +#include <linux/cpumask.h> > >> > >> 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; > >> > >> static int cpu0_verify_speed(struct cpufreq_policy *policy) > >> { > >> @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_device > >> *pdev) > >> goto out_free_table; > >> } > >> > >> + if (!of_property_read_bool(np, "needs-cooling")) > >> + cdev = cpufreq_cooling_register(cpu_present_mask); > >> + > >> of_node_put(np); > >> of_node_put(parent); > >> return 0; > >> @@ -269,6 +276,7 @@ out_put_node: > >> > >> 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); > >> > >>> 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 to > >>> 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. > >> > >> > >> Then you would define your cpu0 node as: > >> > >> cpu@0 { > >> /* OMAP443x variants OPP50-OPPNT */ > >> operating-points = < > >> /* kHz uV */ > >> 300000 1025000 > >> 600000 1200000 > >> 800000 1313000 > >> 1008000 1375000 > >> >; > >> clock-latency = <300000>; /* From legacy driver */ > >> needs-cooling; /* make sure we have cpufreq-cooling */ > >> }; > >> > >> Because in that system we actually need to take care of the cpu thermal. > >> > > I don't see what not registering the cooling device is buying us (aside > > from a small resource saving). The cpu is one potential source of heat > > in a system, so we may want to reference it in a thermal zone, so to me > > it makes sense to always register the cooling device. > > The 'aside from a small resource saving' that bugs me :-). And > conceptually, to me it won't be correct to load stuff you don't need, > specially the 'always' loading part of it. > Hm, while thinking about this I agree. We should try to come up with some property that can be used across different devices. My feeling is that "needs-cooling" is a bit of a misnomer, as not the CPU/GPU/whatever node needs cooling, but the thermal zone. The devices themselves are just sources of heat in that thermal zone, so a name like "enable-thermal-cooling" or something might be more appropriate. > > > >> > >>> > >>> > >>>> > >>>>> I could try to push something following the same idea as the one I am > >>>>> 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 data > >>>>> into the thermal fw. Same could apply for instance for cpufreq cooling > >>>>> 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 stuff > >>>> 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 device > >>>> 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 not > >>> 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 setup > >>> 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 may > >>> > >>> 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 there. > >>> 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, referencing > >>>> 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. > >> > >> > >> Then you can keep everything about your thermal zone in one single > >> phandle, as follows, but remember, this is is the info about the thermal > >> zone, not about a cooling device. For instance, that is the zone built > >> on top of a bandgap sensor: > >> bandgap { > >> reg = <0x4a002260 0x4 0x4a00232C 0x4>; > >> compatible = "ti,omap4430-bandgap"; > >> thermal_zone { > >> type = "CPU"; > >> mask = <0x03>; /* trips writability */ > >> passive_delay = <250>; /* milliseconds */ > >> polling_delay = <1000>; /* milliseconds */ > >> governor = "step_wise"; > >> trips { > >> alert@100000{ > >> temperature = <100000>; > >> hysteresis = <0>; > >> type = <1>; > >> }; > >> crit@125000{ > >> temperature = <125000>; > >> hysteresis = <0>; > >> type = <3>; > >> }; > >> }; > >> bind_params { > >> action@0{ > >> cooling_device = "thermal-cpufreq"; > >> weight = <100>; /* percentage */ > >> mask = <0x01>; > >> }; > >> }; > >> }; > >> }; > >> > >> > >> 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. > > > > Yes, this makes perfect sense to me. What I would like is to have the > > links more specific in the devicetree, so to me this stringmatching > > thing doesn't look too appealing, as it makes it harder to follow the > > links just looking at the DT. That's why I would prefer them to be > > phandles, so I could do something like: > > > > bind_params { > > action@0 { > > cooling_device = <&cpu@0>; > > weight = <40>; /* percentage */ > > mask = <0x01>; > > }; > > action@1 { > > cooling_device = <&gpu3d>; > > weigth = <60>; > > mask = <0x01>; > > }; > > > > I see, but the matching won't work at device tree anyway. But I > understand your point. However, those would need to be 'cooling device' > phandles, not 'cpu' phandles or 'gpu3d' phandles, in order to this make > really sense. > I tend to disagree here. The devices themselves are the source of heat and need to be instructed to cool down, not some virtual cooling device on top of them. Also we should try to avoid to push implementation specific things into the devicetree. Cooling devices are just some arbitrary abstraction made up by the thermal framework, cpu and gpu nodes are real hardware and thus the thing that should be in the DT. Regards, Lucas
Hello Lucas, On 16-07-2013 05:54, Lucas Stach wrote: > Hi Eduardo, > > Am Montag, den 15.07.2013, 10:14 -0400 schrieb Eduardo Valentin: >> On 15-07-2013 10:05, Lucas Stach wrote: >>> Am Montag, den 15.07.2013, 09:36 -0400 schrieb Eduardo Valentin: >>> [...] >>>> >>>> >>>> as simple as the following: >>>> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c >>>> index 3ab8294..486881c 100644 >>>> --- a/drivers/cpufreq/cpufreq-cpu0.c >>>> +++ b/drivers/cpufreq/cpufreq-cpu0.c >>>> @@ -20,6 +20,9 @@ >>>> #include <linux/platform_device.h> >>>> #include <linux/regulator/consumer.h> >>>> #include <linux/slab.h> >>>> +#include <linux/thermal.h> >>>> +#include <linux/cpu_cooling.h> >>>> +#include <linux/cpumask.h> >>>> >>>> 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; >>>> >>>> static int cpu0_verify_speed(struct cpufreq_policy *policy) >>>> { >>>> @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_device >>>> *pdev) >>>> goto out_free_table; >>>> } >>>> >>>> + if (!of_property_read_bool(np, "needs-cooling")) >>>> + cdev = cpufreq_cooling_register(cpu_present_mask); >>>> + >>>> of_node_put(np); >>>> of_node_put(parent); >>>> return 0; >>>> @@ -269,6 +276,7 @@ out_put_node: >>>> >>>> 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); >>>> >>>>> 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 to >>>>> 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. >>>> >>>> >>>> Then you would define your cpu0 node as: >>>> >>>> cpu@0 { >>>> /* OMAP443x variants OPP50-OPPNT */ >>>> operating-points = < >>>> /* kHz uV */ >>>> 300000 1025000 >>>> 600000 1200000 >>>> 800000 1313000 >>>> 1008000 1375000 >>>> >; >>>> clock-latency = <300000>; /* From legacy driver */ >>>> needs-cooling; /* make sure we have cpufreq-cooling */ >>>> }; >>>> >>>> Because in that system we actually need to take care of the cpu thermal. >>>> >>> I don't see what not registering the cooling device is buying us (aside >>> from a small resource saving). The cpu is one potential source of heat >>> in a system, so we may want to reference it in a thermal zone, so to me >>> it makes sense to always register the cooling device. >> >> The 'aside from a small resource saving' that bugs me :-). And >> conceptually, to me it won't be correct to load stuff you don't need, >> specially the 'always' loading part of it. >> > Hm, while thinking about this I agree. We should try to come up with > some property that can be used across different devices. My feeling is > that "needs-cooling" is a bit of a misnomer, as not the CPU/GPU/whatever > node needs cooling, but the thermal zone. The devices themselves are > just sources of heat in that thermal zone, so a name like > "enable-thermal-cooling" or something might be more appropriate. They are heat sources, agreed, but they also need cooling. Again, they can generate heat, one can throttle their dissipated power, but they are not cooling devices. Cooling devices are virtual concepts, in this case, that modulates the power dissipated by these heat generating devices. "enable-thermal-cooling" looks like some switch/knob one would see in a cooling device. > >>> >>>> >>>>> >>>>> >>>>>> >>>>>>> I could try to push something following the same idea as the one I am >>>>>>> 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 data >>>>>>> into the thermal fw. Same could apply for instance for cpufreq cooling >>>>>>> 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 stuff >>>>>> 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 device >>>>>> 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 not >>>>> 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 setup >>>>> 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 may >>>>> >>>>> 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 there. >>>>> 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, referencing >>>>>> 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. >>>> >>>> >>>> Then you can keep everything about your thermal zone in one single >>>> phandle, as follows, but remember, this is is the info about the thermal >>>> zone, not about a cooling device. For instance, that is the zone built >>>> on top of a bandgap sensor: >>>> bandgap { >>>> reg = <0x4a002260 0x4 0x4a00232C 0x4>; >>>> compatible = "ti,omap4430-bandgap"; >>>> thermal_zone { >>>> type = "CPU"; >>>> mask = <0x03>; /* trips writability */ >>>> passive_delay = <250>; /* milliseconds */ >>>> polling_delay = <1000>; /* milliseconds */ >>>> governor = "step_wise"; >>>> trips { >>>> alert@100000{ >>>> temperature = <100000>; >>>> hysteresis = <0>; >>>> type = <1>; >>>> }; >>>> crit@125000{ >>>> temperature = <125000>; >>>> hysteresis = <0>; >>>> type = <3>; >>>> }; >>>> }; >>>> bind_params { >>>> action@0{ >>>> cooling_device = "thermal-cpufreq"; >>>> weight = <100>; /* percentage */ >>>> mask = <0x01>; >>>> }; >>>> }; >>>> }; >>>> }; >>>> >>>> >>>> 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. >>> >>> Yes, this makes perfect sense to me. What I would like is to have the >>> links more specific in the devicetree, so to me this stringmatching >>> thing doesn't look too appealing, as it makes it harder to follow the >>> links just looking at the DT. That's why I would prefer them to be >>> phandles, so I could do something like: >>> >>> bind_params { >>> action@0 { >>> cooling_device = <&cpu@0>; >>> weight = <40>; /* percentage */ >>> mask = <0x01>; >>> }; >>> action@1 { >>> cooling_device = <&gpu3d>; >>> weigth = <60>; >>> mask = <0x01>; >>> }; >>> >> >> I see, but the matching won't work at device tree anyway. But I >> understand your point. However, those would need to be 'cooling device' >> phandles, not 'cpu' phandles or 'gpu3d' phandles, in order to this make >> really sense. >> > I tend to disagree here. The devices themselves are the source of heat > and need to be instructed to cool down, not some virtual cooling device > on top of them. Also we should try to avoid to push implementation > specific things into the devicetree. Cooling devices are just some > arbitrary abstraction made up by the thermal framework, cpu and gpu > nodes are real hardware and thus the thing that should be in the DT. Well, that's exactly my point. We are again making the same points. We should not have phandles for things like cooling devices, as they are virtual devices. However, one disagreement here, when you construct something like: cooling_device = <&gpu3d>; To me, reading that, means gpu3d is a cooling device, which in fact is not. That is why I said, in order for the above really make sense, gpu3d would need to be a cooling device phandle, which to me is wrong to put inside device tree. Thus, I believe using the string matching in the C code is still a better way to go, because creating phandles for cooling devices is a bit of a stretching. > > Regards, > Lucas >
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 3ab8294..486881c 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -20,6 +20,9 @@ #include <linux/platform_device.h> #include <linux/regulator/consumer.h> #include <linux/slab.h> +#include <linux/thermal.h> +#include <linux/cpu_cooling.h> +#include <linux/cpumask.h> 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; static int cpu0_verify_speed(struct cpufreq_policy *policy) { @@ -256,6 +260,9 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev) goto out_free_table; } + if (!of_property_read_bool(np, "needs-cooling")) + cdev = cpufreq_cooling_register(cpu_present_mask); + of_node_put(np); of_node_put(parent);