diff mbox

[v3,3/5] ARM: Exynos: switch to using generic cpufreq driver for Exynos4x12

Message ID 1438368557-2352-4-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz July 31, 2015, 6:49 p.m. UTC
The new CPU clock type allows the use of generic CPUfreq driver.
Switch Exynos4x12 to using generic cpufreq driver.

Also make CPUFREQ_DT config option select Exynos thermal driver
if Exynos platform support is enabled.

Please also note that the switch to use the generic cpufreq-dt
driver fixes the minor issue present with the old code (support
for 'boost' mode in the exynos-cpufreq driver was enabled for
all supported SoCs even though 'boost' frequency was provided
only for Exynos4x12 ones).

Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Thomas Abraham <thomas.ab@samsung.com>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/mach-exynos/exynos.c | 2 ++
 drivers/cpufreq/Kconfig       | 1 +
 2 files changed, 3 insertions(+)

Comments

Krzysztof Kozlowski Aug. 1, 2015, 7:43 a.m. UTC | #1
W dniu 01.08.2015 o 03:49, Bartlomiej Zolnierkiewicz pisze:
> The new CPU clock type allows the use of generic CPUfreq driver.
> Switch Exynos4x12 to using generic cpufreq driver.
> 
> Also make CPUFREQ_DT config option select Exynos thermal driver
> if Exynos platform support is enabled.

Why? I think this wasn't in your previous patch.

Best regards,
Krzysztof


> 
> Please also note that the switch to use the generic cpufreq-dt
> driver fixes the minor issue present with the old code (support
> for 'boost' mode in the exynos-cpufreq driver was enabled for
> all supported SoCs even though 'boost' frequency was provided
> only for Exynos4x12 ones).
> 
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Thomas Abraham <thomas.ab@samsung.com>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/mach-exynos/exynos.c | 2 ++
>  drivers/cpufreq/Kconfig       | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> index 77ac021..1c47aee 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -227,6 +227,8 @@ static void __init exynos_init_irq(void)
>  static const struct of_device_id exynos_cpufreq_matches[] = {
>  	{ .compatible = "samsung,exynos3250", .data = "cpufreq-dt" },
>  	{ .compatible = "samsung,exynos4210", .data = "cpufreq-dt" },
> +	{ .compatible = "samsung,exynos4212", .data = "cpufreq-dt" },
> +	{ .compatible = "samsung,exynos4412", .data = "cpufreq-dt" },
>  	{ .compatible = "samsung,exynos5250", .data = "cpufreq-dt" },
>  	{ /* sentinel */ }
>  };
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index 659879a..bf6d596 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -191,6 +191,7 @@ config CPUFREQ_DT
>  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
>  	depends on !CPU_THERMAL || THERMAL
>  	select PM_OPP
> +	select EXYNOS_THERMAL if ARCH_EXYNOS
>  	help
>  	  This adds a generic DT based cpufreq driver for frequency management.
>  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
>
Viresh Kumar Aug. 1, 2015, 11:17 a.m. UTC | #2
On 31-07-15, 20:49, Bartlomiej Zolnierkiewicz wrote:
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index 659879a..bf6d596 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -191,6 +191,7 @@ config CPUFREQ_DT
>  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
>  	depends on !CPU_THERMAL || THERMAL
>  	select PM_OPP
> +	select EXYNOS_THERMAL if ARCH_EXYNOS
>  	help
>  	  This adds a generic DT based cpufreq driver for frequency management.
>  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)

No, we shouldn't pollute generic Kconfig options with platform specific stuff.
Why don't you enable thermal in your .config?
Bartlomiej Zolnierkiewicz Aug. 3, 2015, 10:17 a.m. UTC | #3
Hi,

On Saturday, August 01, 2015 04:47:21 PM Viresh Kumar wrote:
> On 31-07-15, 20:49, Bartlomiej Zolnierkiewicz wrote:
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index 659879a..bf6d596 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -191,6 +191,7 @@ config CPUFREQ_DT
> >  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
> >  	depends on !CPU_THERMAL || THERMAL
> >  	select PM_OPP
> > +	select EXYNOS_THERMAL if ARCH_EXYNOS
> >  	help
> >  	  This adds a generic DT based cpufreq driver for frequency management.
> >  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> 
> No, we shouldn't pollute generic Kconfig options with platform specific stuff.

The old code depended on this.  You couldn't enable boost support
without enabling thermal support (ARM_EXYNOS_CPU_FREQ_BOOST_SW
config option selected EXYNOS_THERMAL).

> Why don't you enable thermal in your .config?

It is enabled in exynos_defconfig but without the above change it
can disabled manually which is something that we don't want.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Viresh Kumar Aug. 3, 2015, 10:29 a.m. UTC | #4
On 03-08-15, 12:17, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Saturday, August 01, 2015 04:47:21 PM Viresh Kumar wrote:
> > On 31-07-15, 20:49, Bartlomiej Zolnierkiewicz wrote:
> > > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > > index 659879a..bf6d596 100644
> > > --- a/drivers/cpufreq/Kconfig
> > > +++ b/drivers/cpufreq/Kconfig
> > > @@ -191,6 +191,7 @@ config CPUFREQ_DT
> > >  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
> > >  	depends on !CPU_THERMAL || THERMAL
> > >  	select PM_OPP
> > > +	select EXYNOS_THERMAL if ARCH_EXYNOS
> > >  	help
> > >  	  This adds a generic DT based cpufreq driver for frequency management.
> > >  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> > 
> > No, we shouldn't pollute generic Kconfig options with platform specific stuff.
> 
> The old code depended on this.  You couldn't enable boost support
> without enabling thermal support (ARM_EXYNOS_CPU_FREQ_BOOST_SW
> config option selected EXYNOS_THERMAL).
> 
> > Why don't you enable thermal in your .config?
> 
> It is enabled in exynos_defconfig but without the above change it
> can disabled manually which is something that we don't want.

You are not getting it. I am not asking you to not select thermal, but
to select it from within your architecture Kconfig option if you want.

Over that, thermal is really an option, not a dependency. So, if
someone manually disables it, its his problem not yours :)
Bartlomiej Zolnierkiewicz Aug. 3, 2015, 10:31 a.m. UTC | #5
[ added Zhang & Eduardo to Cc: ]

Hi,

On Saturday, August 01, 2015 04:43:58 PM Krzysztof Kozlowski wrote:
> W dniu 01.08.2015 o 03:49, Bartlomiej Zolnierkiewicz pisze:
> > The new CPU clock type allows the use of generic CPUfreq driver.
> > Switch Exynos4x12 to using generic cpufreq driver.
> > 
> > Also make CPUFREQ_DT config option select Exynos thermal driver
> > if Exynos platform support is enabled.
> 
> Why? I think this wasn't in your previous patch.

Previous patch kept ARM_EXYNOS_CPU_FREQ_BOOST_SW config option
which selected EXYNOS_THERMAL.  After recent changes (boost
support enabled in the cpufreq-dt driver when there are turbo
OPPs in board's dts file) ARM_EXYNOS_CPU_FREQ_BOOST_SW config
option become redundant and was removed.  However we still
would like to allow enabling boost support only if thermal
support is also enabled for Exynos platforms.

[ There may be a better way to do this in the future (runtime
  checking for thermal support being enabled) but currently
  there seems to be no thermal infrastructure to allow this. ]

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Best regards,
> Krzysztof
> 
> 
> > 
> > Please also note that the switch to use the generic cpufreq-dt
> > driver fixes the minor issue present with the old code (support
> > for 'boost' mode in the exynos-cpufreq driver was enabled for
> > all supported SoCs even though 'boost' frequency was provided
> > only for Exynos4x12 ones).
> > 
> > Cc: Tomasz Figa <tomasz.figa@gmail.com>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Thomas Abraham <thomas.ab@samsung.com>
> > Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > ---
> >  arch/arm/mach-exynos/exynos.c | 2 ++
> >  drivers/cpufreq/Kconfig       | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> > index 77ac021..1c47aee 100644
> > --- a/arch/arm/mach-exynos/exynos.c
> > +++ b/arch/arm/mach-exynos/exynos.c
> > @@ -227,6 +227,8 @@ static void __init exynos_init_irq(void)
> >  static const struct of_device_id exynos_cpufreq_matches[] = {
> >  	{ .compatible = "samsung,exynos3250", .data = "cpufreq-dt" },
> >  	{ .compatible = "samsung,exynos4210", .data = "cpufreq-dt" },
> > +	{ .compatible = "samsung,exynos4212", .data = "cpufreq-dt" },
> > +	{ .compatible = "samsung,exynos4412", .data = "cpufreq-dt" },
> >  	{ .compatible = "samsung,exynos5250", .data = "cpufreq-dt" },
> >  	{ /* sentinel */ }
> >  };
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index 659879a..bf6d596 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -191,6 +191,7 @@ config CPUFREQ_DT
> >  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
> >  	depends on !CPU_THERMAL || THERMAL
> >  	select PM_OPP
> > +	select EXYNOS_THERMAL if ARCH_EXYNOS
> >  	help
> >  	  This adds a generic DT based cpufreq driver for frequency management.
> >  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
Bartlomiej Zolnierkiewicz Aug. 3, 2015, 10:36 a.m. UTC | #6
On Monday, August 03, 2015 03:59:26 PM Viresh Kumar wrote:
> On 03-08-15, 12:17, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Saturday, August 01, 2015 04:47:21 PM Viresh Kumar wrote:
> > > On 31-07-15, 20:49, Bartlomiej Zolnierkiewicz wrote:
> > > > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > > > index 659879a..bf6d596 100644
> > > > --- a/drivers/cpufreq/Kconfig
> > > > +++ b/drivers/cpufreq/Kconfig
> > > > @@ -191,6 +191,7 @@ config CPUFREQ_DT
> > > >  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
> > > >  	depends on !CPU_THERMAL || THERMAL
> > > >  	select PM_OPP
> > > > +	select EXYNOS_THERMAL if ARCH_EXYNOS
> > > >  	help
> > > >  	  This adds a generic DT based cpufreq driver for frequency management.
> > > >  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> > > 
> > > No, we shouldn't pollute generic Kconfig options with platform specific stuff.
> > 
> > The old code depended on this.  You couldn't enable boost support
> > without enabling thermal support (ARM_EXYNOS_CPU_FREQ_BOOST_SW
> > config option selected EXYNOS_THERMAL).
> > 
> > > Why don't you enable thermal in your .config?
> > 
> > It is enabled in exynos_defconfig but without the above change it
> > can disabled manually which is something that we don't want.
> 
> You are not getting it. I am not asking you to not select thermal, but
> to select it from within your architecture Kconfig option if you want.

OK.  Krzysztof/Kukjin do you agree with selecting EXYNOS_THERMAL
from ARCH_EXYNOS in the platform code?

> Over that, thermal is really an option, not a dependency. So, if
> someone manually disables it, its his problem not yours :)

I would really like it to be dependency not an option (+ I think
that ideally it should be checked at runtime, IOW we should be
checking from cpufreq-dt driver if the thermal support is enabled
before enabling boost support).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Viresh Kumar Aug. 3, 2015, 10:40 a.m. UTC | #7
On 03-08-15, 12:36, Bartlomiej Zolnierkiewicz wrote:
> I would really like it to be dependency not an option (+ I think
> that ideally it should be checked at runtime, IOW we should be
> checking from cpufreq-dt driver if the thermal support is enabled
> before enabling boost support).

I don't think boost has any dependency on thermal support. Yeah, it
may be true for your platform but we can't force it. People might have
different algorithms to control boost modes, thermal is just one
option they may look at. For few, enabling boost may not be a thermal
issue, but power. So, they want to allow it only when they want, but
that wouldn't burn their chip.

So, a platform can choose how it wants to have it. :)
Bartlomiej Zolnierkiewicz Aug. 3, 2015, 10:47 a.m. UTC | #8
On Monday, August 03, 2015 04:10:44 PM Viresh Kumar wrote:
> On 03-08-15, 12:36, Bartlomiej Zolnierkiewicz wrote:
> > I would really like it to be dependency not an option (+ I think
> > that ideally it should be checked at runtime, IOW we should be
> > checking from cpufreq-dt driver if the thermal support is enabled
> > before enabling boost support).
> 
> I don't think boost has any dependency on thermal support. Yeah, it
> may be true for your platform but we can't force it. People might have
> different algorithms to control boost modes, thermal is just one
> option they may look at. For few, enabling boost may not be a thermal
> issue, but power. So, they want to allow it only when they want, but
> that wouldn't burn their chip.

OK, I see your point (I have not thought about power being the boost
limitation previously).

> So, a platform can choose how it wants to have it. :)

I'll re-do this patch.  Thank you.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Krzysztof Kozlowski Aug. 3, 2015, 11:15 a.m. UTC | #9
W dniu 03.08.2015 o 19:36, Bartlomiej Zolnierkiewicz pisze:
> On Monday, August 03, 2015 03:59:26 PM Viresh Kumar wrote:
>> On 03-08-15, 12:17, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> On Saturday, August 01, 2015 04:47:21 PM Viresh Kumar wrote:
>>>> On 31-07-15, 20:49, Bartlomiej Zolnierkiewicz wrote:
>>>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>>>>> index 659879a..bf6d596 100644
>>>>> --- a/drivers/cpufreq/Kconfig
>>>>> +++ b/drivers/cpufreq/Kconfig
>>>>> @@ -191,6 +191,7 @@ config CPUFREQ_DT
>>>>>  	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
>>>>>  	depends on !CPU_THERMAL || THERMAL
>>>>>  	select PM_OPP
>>>>> +	select EXYNOS_THERMAL if ARCH_EXYNOS
>>>>>  	help
>>>>>  	  This adds a generic DT based cpufreq driver for frequency management.
>>>>>  	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
>>>>
>>>> No, we shouldn't pollute generic Kconfig options with platform specific stuff.
>>>
>>> The old code depended on this.  You couldn't enable boost support
>>> without enabling thermal support (ARM_EXYNOS_CPU_FREQ_BOOST_SW
>>> config option selected EXYNOS_THERMAL).
>>>
>>>> Why don't you enable thermal in your .config?
>>>
>>> It is enabled in exynos_defconfig but without the above change it
>>> can disabled manually which is something that we don't want.
>>
>> You are not getting it. I am not asking you to not select thermal, but
>> to select it from within your architecture Kconfig option if you want.
> 
> OK.  Krzysztof/Kukjin do you agree with selecting EXYNOS_THERMAL
> from ARCH_EXYNOS in the platform code?

I agree, with your explanation it seems good. Can you just add this
justification to the commit message?

> 
>> Over that, thermal is really an option, not a dependency. So, if
>> someone manually disables it, its his problem not yours :)
> 
> I would really like it to be dependency not an option (+ I think
> that ideally it should be checked at runtime, IOW we should be
> checking from cpufreq-dt driver if the thermal support is enabled
> before enabling boost support).

That would be the best. It is fine with me if you want to do this in
consecutive patches (after applying patch selecting/depending on it in
mach-exynos code).

Best regards,
Krzysztof
diff mbox

Patch

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index 77ac021..1c47aee 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -227,6 +227,8 @@  static void __init exynos_init_irq(void)
 static const struct of_device_id exynos_cpufreq_matches[] = {
 	{ .compatible = "samsung,exynos3250", .data = "cpufreq-dt" },
 	{ .compatible = "samsung,exynos4210", .data = "cpufreq-dt" },
+	{ .compatible = "samsung,exynos4212", .data = "cpufreq-dt" },
+	{ .compatible = "samsung,exynos4412", .data = "cpufreq-dt" },
 	{ .compatible = "samsung,exynos5250", .data = "cpufreq-dt" },
 	{ /* sentinel */ }
 };
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 659879a..bf6d596 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -191,6 +191,7 @@  config CPUFREQ_DT
 	# if CPU_THERMAL is on and THERMAL=m, CPUFREQ_DT cannot be =y:
 	depends on !CPU_THERMAL || THERMAL
 	select PM_OPP
+	select EXYNOS_THERMAL if ARCH_EXYNOS
 	help
 	  This adds a generic DT based cpufreq driver for frequency management.
 	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)