| Message ID | 1438368557-2352-4-git-send-email-b.zolnierkie@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) >
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?
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
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 :)
[ 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)
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
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. :)
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
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 --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)
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(+)