| Message ID | 1415898165-27406-6-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> One potential issue I can see is that if the cpufreq driver fails to probe then you'll never get the thermal driver either. For example, Tegra124 currently has no cpufreq driver, so if CONFIG_CPU_THERMAL was enabled, then the soctherm driver would never be able to probe. But I don't really have a solution for this either. Cheers, Mikko On 11/13/2014 07:02 PM, Lukasz Majewski wrote: > When CPU freq is used as a thermal zone cooling device, one needs to wait > until cpufreq subsystem is properly initialized. > > This code is similar to the one already available in imx_thermal.c file. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > drivers/thermal/tegra_soctherm.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c > index 70f7e9e..9c5aaa4 100644 > --- a/drivers/thermal/tegra_soctherm.c > +++ b/drivers/thermal/tegra_soctherm.c > @@ -26,6 +26,7 @@ > #include <linux/platform_device.h> > #include <linux/reset.h> > #include <linux/thermal.h> > +#include <linux/cpufreq.h> > > #include <soc/tegra/fuse.h> > > @@ -346,6 +347,12 @@ static int tegra_soctherm_probe(struct platform_device *pdev) > > const struct tegra_tsensor *tsensors = t124_tsensors; > > +#ifdef CONFIG_CPU_THERMAL > + if (!cpufreq_get_current_driver()) { > + dev_dbg(&pdev->dev, "no cpufreq driver!"); > + return -EPROBE_DEFER; > + } > +#endif > tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL); > if (!tegra) > return -ENOMEM; > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mikko, > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> Thanks for testing. > > One potential issue I can see is that if the cpufreq driver fails to > probe then you'll never get the thermal driver either. For example, > Tegra124 currently has no cpufreq driver, so if CONFIG_CPU_THERMAL > was enabled, then the soctherm driver would never be able to probe. Yes, this is a potential issue. However, this option in tegra_defconfig is by default disabled when thermal is enabled. > But I don't really have a solution for this either. Ok. I see. > > Cheers, > Mikko > > On 11/13/2014 07:02 PM, Lukasz Majewski wrote: > > When CPU freq is used as a thermal zone cooling device, one needs > > to wait until cpufreq subsystem is properly initialized. > > > > This code is similar to the one already available in imx_thermal.c > > file. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > drivers/thermal/tegra_soctherm.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/thermal/tegra_soctherm.c > > b/drivers/thermal/tegra_soctherm.c index 70f7e9e..9c5aaa4 100644 > > --- a/drivers/thermal/tegra_soctherm.c > > +++ b/drivers/thermal/tegra_soctherm.c > > @@ -26,6 +26,7 @@ > > #include <linux/platform_device.h> > > #include <linux/reset.h> > > #include <linux/thermal.h> > > +#include <linux/cpufreq.h> > > > > #include <soc/tegra/fuse.h> > > > > @@ -346,6 +347,12 @@ static int tegra_soctherm_probe(struct > > platform_device *pdev) > > > > const struct tegra_tsensor *tsensors = t124_tsensors; > > > > +#ifdef CONFIG_CPU_THERMAL > > + if (!cpufreq_get_current_driver()) { > > + dev_dbg(&pdev->dev, "no cpufreq driver!"); > > + return -EPROBE_DEFER; > > + } > > +#endif > > tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), > > GFP_KERNEL); if (!tegra) > > return -ENOMEM; > > >
On Thu, Nov 13, 2014 at 06:02:42PM +0100, Lukasz Majewski wrote: > When CPU freq is used as a thermal zone cooling device, one needs to wait > until cpufreq subsystem is properly initialized. > > This code is similar to the one already available in imx_thermal.c file. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > drivers/thermal/tegra_soctherm.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c > index 70f7e9e..9c5aaa4 100644 > --- a/drivers/thermal/tegra_soctherm.c > +++ b/drivers/thermal/tegra_soctherm.c > @@ -26,6 +26,7 @@ > #include <linux/platform_device.h> > #include <linux/reset.h> > #include <linux/thermal.h> > +#include <linux/cpufreq.h> > > #include <soc/tegra/fuse.h> > > @@ -346,6 +347,12 @@ static int tegra_soctherm_probe(struct platform_device *pdev) > > const struct tegra_tsensor *tsensors = t124_tsensors; > > +#ifdef CONFIG_CPU_THERMAL > + if (!cpufreq_get_current_driver()) { > + dev_dbg(&pdev->dev, "no cpufreq driver!"); Shouldn't this rather be dev_err() or dev_warn() to give at least some clue as to the cause? Thierry
On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > One potential issue I can see is that if the cpufreq driver fails to probe > then you'll never get the thermal driver either. For example, Tegra124 > currently has no cpufreq driver, so if CONFIG_CPU_THERMAL was enabled, then > the soctherm driver would never be able to probe. But I don't really have a > solution for this either. It doesn't seem like there's any code whatsoever to deal with cpufreq within the soctherm driver, so deferring probe based on something we're not using anyway seems rather useless. Thierry
Hi Thierry, > On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: > > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > > > One potential issue I can see is that if the cpufreq driver fails > > to probe then you'll never get the thermal driver either. For > > example, Tegra124 currently has no cpufreq driver, so if > > CONFIG_CPU_THERMAL was enabled, then the soctherm driver would > > never be able to probe. But I don't really have a solution for this > > either. > > It doesn't seem like there's any code whatsoever to deal with cpufreq > within the soctherm driver, so deferring probe based on something > we're not using anyway seems rather useless. So, If I understood you correctly - this patch is not needed in the /tegra_soctherm.c:[tegra_defconfig] driver and can be safely omitted in v2 of this driver. Please note that I'm not aware of Tegra specific use cases and hence I've prepared fixes for all potential candidates. > > Thierry
On Fri, Nov 14, 2014 at 12:24:37PM +0100, Lukasz Majewski wrote: > Hi Mikko, > > > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > Thanks for testing. > > > > > One potential issue I can see is that if the cpufreq driver fails to > > probe then you'll never get the thermal driver either. For example, > > Tegra124 currently has no cpufreq driver, so if CONFIG_CPU_THERMAL > > was enabled, then the soctherm driver would never be able to probe. > > Yes, this is a potential issue. However, this option in tegra_defconfig > is by default disabled when thermal is enabled. Not everybody uses tegra_defconfig for their kernel builds. In fact I'd imagine that the majority of kernels use a variant of multi_v7_defconfig and therefore CPU_THERMAL might get enabled irrespective of any Tegra support. I think a better solution would be to add this check only when proper support for CPU frequency based cooling is added. That is, when a call to cpufreq_cooling_register() (or a variant thereof) is added. But while at it, why not make it so that cpufreq_cooling_register() detects if a cpufreq driver has been registered yet and propagate -EPROBE_DEFER if necessary? Thierry
On Mon, Nov 17, 2014 at 12:50:13PM +0100, Lukasz Majewski wrote: > Hi Thierry, > > > On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: > > > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > > > > > One potential issue I can see is that if the cpufreq driver fails > > > to probe then you'll never get the thermal driver either. For > > > example, Tegra124 currently has no cpufreq driver, so if > > > CONFIG_CPU_THERMAL was enabled, then the soctherm driver would > > > never be able to probe. But I don't really have a solution for this > > > either. > > > > It doesn't seem like there's any code whatsoever to deal with cpufreq > > within the soctherm driver, so deferring probe based on something > > we're not using anyway seems rather useless. > > So, If I understood you correctly - this patch is not needed in the > /tegra_soctherm.c:[tegra_defconfig] driver and can be safely omitted in > v2 of this driver. What I'm saying is that I don't think doing this mass conversion wholesale is useful since none of the drivers register a cooling device based on cpufreq. In other words: if you're not going to use a feature there's no use testing for it. Thierry
On 11/17/2014 01:43 PM, Thierry Reding wrote: > On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: >> Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> >> >> One potential issue I can see is that if the cpufreq driver fails to probe >> then you'll never get the thermal driver either. For example, Tegra124 >> currently has no cpufreq driver, so if CONFIG_CPU_THERMAL was enabled, then >> the soctherm driver would never be able to probe. But I don't really have a >> solution for this either. > > It doesn't seem like there's any code whatsoever to deal with cpufreq > within the soctherm driver, so deferring probe based on something we're > not using anyway seems rather useless. > > Thierry > My understanding is that there needs to be no code inside soctherm to handle it, as the cpufreq driver (cpufreq-dt) will register a cooling device that will then be bound to the soctherm sensors using the of-thermal device tree properties. At this moment, however, we don't have that cpufreq driver so this patch is still useless for Tegra. Mikko -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thierry, > On Fri, Nov 14, 2014 at 12:24:37PM +0100, Lukasz Majewski wrote: > > Hi Mikko, > > > > > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > > > Thanks for testing. > > > > > > > > One potential issue I can see is that if the cpufreq driver fails > > > to probe then you'll never get the thermal driver either. For > > > example, Tegra124 currently has no cpufreq driver, so if > > > CONFIG_CPU_THERMAL was enabled, then the soctherm driver would > > > never be able to probe. > > > > Yes, this is a potential issue. However, this option in > > tegra_defconfig is by default disabled when thermal is enabled. > > Not everybody uses tegra_defconfig for their kernel builds. In fact > I'd imagine that the majority of kernels use a variant of > multi_v7_defconfig and therefore CPU_THERMAL might get enabled > irrespective of any Tegra support. I see your point. > > I think a better solution would be to add this check only when proper > support for CPU frequency based cooling is added. That is, when a call > to cpufreq_cooling_register() (or a variant thereof) is added. For the above reason the code is only compiled in when user enable CONFIG_CPU_THERMAL. > > But while at it, why not make it so that cpufreq_cooling_register() > detects if a cpufreq driver has been registered yet and propagate > -EPROBE_DEFER if necessary? cpufreq_cooling_register() may be a good place to add check for deferred probe. The problem with cpufreq_cooling_register() is that it may be called late in the probe function (as it is done now in Exynos). > > Thierry
Hi Thierry, > On Mon, Nov 17, 2014 at 12:50:13PM +0100, Lukasz Majewski wrote: > > Hi Thierry, > > > > > On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: > > > > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > > > > > > > One potential issue I can see is that if the cpufreq driver > > > > fails to probe then you'll never get the thermal driver either. > > > > For example, Tegra124 currently has no cpufreq driver, so if > > > > CONFIG_CPU_THERMAL was enabled, then the soctherm driver would > > > > never be able to probe. But I don't really have a solution for > > > > this either. > > > > > > It doesn't seem like there's any code whatsoever to deal with > > > cpufreq within the soctherm driver, so deferring probe based on > > > something we're not using anyway seems rather useless. > > > > So, If I understood you correctly - this patch is not needed in the > > /tegra_soctherm.c:[tegra_defconfig] driver and can be safely > > omitted in v2 of this driver. > > What I'm saying is that I don't think doing this mass conversion > wholesale is useful since none of the drivers register a cooling > device based on cpufreq. In other words: if you're not going to use a > feature there's no use testing for it. > It seems, like one option here would be to add deferred proble to cpufreq_cooling_register() or check which driver in its thermal probe is calling cpufreq_cooling_register() function. The latter option explains why in the imx_thermal.c file we check for deferred probe without #ifdefs for CONFIG_CPU_THERMAL. If no objections, I would like to stick to the code already available in imx_thermal.c. > Thierry
On Mon, Nov 17, 2014 at 02:51:24PM +0200, Mikko Perttunen wrote: > On 11/17/2014 01:43 PM, Thierry Reding wrote: > >On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: > >>Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > >> > >>One potential issue I can see is that if the cpufreq driver fails to probe > >>then you'll never get the thermal driver either. For example, Tegra124 > >>currently has no cpufreq driver, so if CONFIG_CPU_THERMAL was enabled, then > >>the soctherm driver would never be able to probe. But I don't really have a > >>solution for this either. > > > >It doesn't seem like there's any code whatsoever to deal with cpufreq > >within the soctherm driver, so deferring probe based on something we're > >not using anyway seems rather useless. > > > >Thierry > > > > My understanding is that there needs to be no code inside soctherm to handle > it, as the cpufreq driver (cpufreq-dt) will register a cooling device that > will then be bound to the soctherm sensors using the of-thermal device tree > properties. At this moment, however, we don't have that cpufreq driver so > this patch is still useless for Tegra. But if the cpufreq driver will automatically do this already, why do we even need to check for it in the soctherm driver? Thierry
On Mon, Nov 17, 2014 at 01:51:42PM +0100, Lukasz Majewski wrote: > Hi Thierry, > > > On Fri, Nov 14, 2014 at 12:24:37PM +0100, Lukasz Majewski wrote: > > > Hi Mikko, > > > > > > > Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> > > > > > > Thanks for testing. > > > > > > > > > > > One potential issue I can see is that if the cpufreq driver fails > > > > to probe then you'll never get the thermal driver either. For > > > > example, Tegra124 currently has no cpufreq driver, so if > > > > CONFIG_CPU_THERMAL was enabled, then the soctherm driver would > > > > never be able to probe. > > > > > > Yes, this is a potential issue. However, this option in > > > tegra_defconfig is by default disabled when thermal is enabled. > > > > Not everybody uses tegra_defconfig for their kernel builds. In fact > > I'd imagine that the majority of kernels use a variant of > > multi_v7_defconfig and therefore CPU_THERMAL might get enabled > > irrespective of any Tegra support. > > I see your point. > > > > > I think a better solution would be to add this check only when proper > > support for CPU frequency based cooling is added. That is, when a call > > to cpufreq_cooling_register() (or a variant thereof) is added. > > For the above reason the code is only compiled in when user enable > CONFIG_CPU_THERMAL. Like I said, CPU_THERMAL is a kernel-wide configuration option and not tied to the specific SoC. Therefore if an SoC, say Exynos, gains full support for cooling via cpufreq then somebody may decide to enable the CPU_THERMAL option in multi_v7_defconfig. At that point every thermal driver that you're patching in this way but for which no cpufreq driver is registered will indefinitely defer probing. So I see two options: 1) make sure that we defer probing only on devices where a cpufreq driver is available 2) not rely on deferred probing at all but allow a cooling device to be registered after the thermal driver has finished probing 1) seems impossible to do because cpufreq simply doesn't provide enough infrastructure to deal with that situation. Thierry
On 11/17/2014 03:08 PM, Thierry Reding wrote: > On Mon, Nov 17, 2014 at 02:51:24PM +0200, Mikko Perttunen wrote: >> On 11/17/2014 01:43 PM, Thierry Reding wrote: >>> On Fri, Nov 14, 2014 at 12:47:33PM +0200, Mikko Perttunen wrote: >>>> Tested-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> >>>> >>>> One potential issue I can see is that if the cpufreq driver fails to probe >>>> then you'll never get the thermal driver either. For example, Tegra124 >>>> currently has no cpufreq driver, so if CONFIG_CPU_THERMAL was enabled, then >>>> the soctherm driver would never be able to probe. But I don't really have a >>>> solution for this either. >>> >>> It doesn't seem like there's any code whatsoever to deal with cpufreq >>> within the soctherm driver, so deferring probe based on something we're >>> not using anyway seems rather useless. >>> >>> Thierry >>> >> >> My understanding is that there needs to be no code inside soctherm to handle >> it, as the cpufreq driver (cpufreq-dt) will register a cooling device that >> will then be bound to the soctherm sensors using the of-thermal device tree >> properties. At this moment, however, we don't have that cpufreq driver so >> this patch is still useless for Tegra. > > But if the cpufreq driver will automatically do this already, why do we > even need to check for it in the soctherm driver? > > Thierry > Indeed, we shouldn't. Unless I am mistaken, the issue is then that the cpufreq cooling device calls thermal_cooling_device_register before being ready to handle callbacks, which clearly would be an issue in the cpufreq driver. The thermal core seems to able to handle registrations of thermal zones and cooling devices in any order; AFAICT it defers binding the tz<->cdev mapping until both have registered themselves to the thermal core. Mikko -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c index 70f7e9e..9c5aaa4 100644 --- a/drivers/thermal/tegra_soctherm.c +++ b/drivers/thermal/tegra_soctherm.c @@ -26,6 +26,7 @@ #include <linux/platform_device.h> #include <linux/reset.h> #include <linux/thermal.h> +#include <linux/cpufreq.h> #include <soc/tegra/fuse.h> @@ -346,6 +347,12 @@ static int tegra_soctherm_probe(struct platform_device *pdev) const struct tegra_tsensor *tsensors = t124_tsensors; +#ifdef CONFIG_CPU_THERMAL + if (!cpufreq_get_current_driver()) { + dev_dbg(&pdev->dev, "no cpufreq driver!"); + return -EPROBE_DEFER; + } +#endif tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL); if (!tegra) return -ENOMEM;
When CPU freq is used as a thermal zone cooling device, one needs to wait until cpufreq subsystem is properly initialized. This code is similar to the one already available in imx_thermal.c file. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- drivers/thermal/tegra_soctherm.c | 7 +++++++ 1 file changed, 7 insertions(+)