Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq
Premi, Sanjeev had written, on 07/26/2010 10:35 AM, the following: -Original Message- From: Nishanth Menon [mailto:menon.nisha...@gmail.com] Sent: Wednesday, June 02, 2010 10:07 AM To: Premi, Sanjeev Cc: linux-omap@vger.kernel.org Subject: Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq On 06/01/2010 03:01 PM, Premi, Sanjeev wrote: -Original Message- From: Nishanth Menon [mailto:menon.nisha...@gmail.com] Sent: Monday, May 31, 2010 10:59 PM To: Premi, Sanjeev Cc: linux-omap@vger.kernel.org Subject: Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq On 05/31/2010 03:39 PM, Sanjeev Premi wrote: The OPP layer was contained in the CONFIG_CPU_FREQ. This patch removes this containment relation. Signed-off-by: Sanjeev Premi --- arch/arm/mach-omap2/Makefile |6 +- arch/arm/mach-omap2/board-omap3evm.c |2 +- [snip]--[snip] you sure this is the only board file having "omap3-opp.h" ? anyway.. the need for board files to use opp_init is gone with my patch http://marc.info/?l=linux-omap&m=127507237109393&w=2 so I wont harp on it, I would rather post a cleanup patch for all board files once the patch is in..(or mebbe kevin could drop the patch that adds opp_init_table to board files ;) ).. [sp] You didn't reead the 0/1 of the patch series, where I have clearly mentioned that I will make changes to the other board specific files once there rest of the changes are well discussed. There may be, possibly, more changes in the board specific files and we can review them in the context of this file and then same can be repeated for other board files. ok arch/arm/mach-omap2/cpufreq34xx.c | 164 arch/arm/mach-omap2/omap3-opp.h | 20 arch/arm/mach-omap2/opp34xx_data.c| 166 + arch/arm/mach-omap2/pm34xx.c |1 - arch/arm/plat-omap/Makefile |7 +- arch/arm/plat-omap/cpu-omap.c | 47 + arch/arm/plat-omap/include/plat/opp.h | 82 +--- arch/arm/plat-omap/opp.c | 46 - 10 files changed, 225 insertions(+), 316 deletions(-) delete mode 100644 arch/arm/mach-omap2/cpufreq34xx.c delete mode 100644 arch/arm/mach-omap2/omap3-opp.h create mode 100644 arch/arm/mach-omap2/opp34xx_data.c [sp] finding it difficult to align with this change, you introduce omap3_pm_init_opp_table later into plat/opp.h which defeats generic nature of opp.h - as it was supposed to be used for other omaps as well.. In that case the function omap3_pm_init_opp_table() can be made generic by renaming to omap_pm_init_opp_table and can be implemented for each omap family. Do you intend to handle multiomap case by calling each omap[1234]_pm_init_opp_table() if cpu_is_omap34xx() etc? you will still need a custom omap family specific init_opp_table - that is what this header provides. If opp table has to be implementyed for each family then why have different funtion with family specific prefixes? opp table contents will be different for each family and they should all build and co-exist in a single uImage. Also, what this headerf ile is/was doing? only defining the function to return -EINVAL when CONFIG_CPU_FREQ is not selected; which is not required. For OPP layer to be used this table needs to be populated. Now, there is only one place this function is used, so why do/should be need a header for the same. To allow the the external function that triggers it to be able to use it.. :) [snip]--[snip] +obj-y += opp.o +obj-$(CONFIG_TWL4030_POWER)+= opp_twl_tps.o NAK. you just need TWL4030_CORE not power here. any reason to retain power? it has no dependency on power.. [sp] Isn't this purpose of this opp to TWL linkage is to define the voltages in terms the PMIC connected; and later make sure that correct voltages are set via the PMIC? This is very much related no it does not. these are just translation functions, as long as _CORE exists, it means that the system uses twl and we should be good to go. to POWER. We could also do it on CORE; but I don't see this as a big issue. ok. But TWL4030 has more feature beyond PMIC but this is not the case with other simpler PMICs and I wanted to use CONFIG option that can be easier for someone else make the port easy to spot as a necessary change. i am aligned with the change, except that I believe you should not be using POWER as the prefix for the config build dependency. [snip]--[snip] + freq_table[i].index = i; + freq_table[i].frequency = CPUFREQ_TABLE_END; + + *table =&freq_table[0]; +} +#endif + e why? it used to be here and was moved to opp.c - see http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-p m.git;a=commit;h=9a6b00f70e9f4bce30ad4f8fab41a24bd3706dbd you are essentially reverting that patch! [sp] May be
RE: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq
> -Original Message- > From: Nishanth Menon [mailto:menon.nisha...@gmail.com] > Sent: Wednesday, June 02, 2010 10:07 AM > To: Premi, Sanjeev > Cc: linux-omap@vger.kernel.org > Subject: Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq > > On 06/01/2010 03:01 PM, Premi, Sanjeev wrote: > > > >> -Original Message- > >> From: Nishanth Menon [mailto:menon.nisha...@gmail.com] > >> Sent: Monday, May 31, 2010 10:59 PM > >> To: Premi, Sanjeev > >> Cc: linux-omap@vger.kernel.org > >> Subject: Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq > >> > >> On 05/31/2010 03:39 PM, Sanjeev Premi wrote: > >>> The OPP layer was contained in the CONFIG_CPU_FREQ. > >>> This patch removes this containment relation. > >>> > >>> Signed-off-by: Sanjeev Premi > >>> --- > >>>arch/arm/mach-omap2/Makefile |6 +- > >>>arch/arm/mach-omap2/board-omap3evm.c |2 +- > > > > [snip]--[snip] > > > >> you sure this is the only board file having "omap3-opp.h" ? > >> anyway.. the > >> need for board files to use opp_init is gone with my patch > >> http://marc.info/?l=linux-omap&m=127507237109393&w=2 > >> so I wont harp on it, I would rather post a cleanup patch for > >> all board > >> files once the patch is in..(or mebbe kevin could drop the > patch that > >> adds opp_init_table to board files ;) ).. > >> > > > > [sp] You didn't reead the 0/1 of the patch series, where I > have clearly > > mentioned that I will make changes to the other board specific files > > once there rest of the changes are well discussed. There > may be, possibly, > > more changes in the board specific files and we can review > them in the > > context of this file and then same can be repeated for > other board files. > ok > > > > >>>arch/arm/mach-omap2/cpufreq34xx.c | 164 > >> > >>>arch/arm/mach-omap2/omap3-opp.h | 20 > >>>arch/arm/mach-omap2/opp34xx_data.c| 166 > >> + > >>>arch/arm/mach-omap2/pm34xx.c |1 - > >>>arch/arm/plat-omap/Makefile |7 +- > >>>arch/arm/plat-omap/cpu-omap.c | 47 + > >>>arch/arm/plat-omap/include/plat/opp.h | 82 +--- > >>>arch/arm/plat-omap/opp.c | 46 - > >>>10 files changed, 225 insertions(+), 316 deletions(-) > >>>delete mode 100644 arch/arm/mach-omap2/cpufreq34xx.c > >>>delete mode 100644 arch/arm/mach-omap2/omap3-opp.h > >>>create mode 100644 arch/arm/mach-omap2/opp34xx_data.c > >>> > > > > [sp] > >> finding it difficult to align with this change, you introduce > >> omap3_pm_init_opp_table later into plat/opp.h which defeats generic > >> nature of opp.h - as it was supposed to be used for other > >> omaps as well.. > > > > In that case the function omap3_pm_init_opp_table() can be made > > generic by renaming to omap_pm_init_opp_table and can be implemented > > for each omap family. > Do you intend to handle multiomap case by calling > each omap[1234]_pm_init_opp_table() if cpu_is_omap34xx() etc? > you will > still need a custom omap family specific init_opp_table - > that is what > this header provides. > > > > > If opp table has to be implementyed for each family then why have > > different funtion with family specific prefixes? > opp table contents will be different for each family and they > should all > build and co-exist in a single uImage. > > > > > Also, what this headerf ile is/was doing? only defining the > > function to return -EINVAL when CONFIG_CPU_FREQ is not selected; > > which is not required. For OPP layer to be used this table needs > > to be populated. Now, there is only one place this function is > > used, so why do/should be need a header for the same. > > To allow the the external function that triggers it to be able to use > it.. :) > > > > > [snip]--[snip] > > > >> +obj-y += opp.o > >> +obj-$(CONFIG_TWL4030_POWER) += opp_twl_tps.o > > NAK. you just need TWL4030_CORE not power here. any reason to retain > > power? it has no dependency on power.. > > > > [sp] Isn't this purpose of this opp to TWL linkage is to define >
Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq
On 06/01/2010 03:01 PM, Premi, Sanjeev wrote: -Original Message- From: Nishanth Menon [mailto:menon.nisha...@gmail.com] Sent: Monday, May 31, 2010 10:59 PM To: Premi, Sanjeev Cc: linux-omap@vger.kernel.org Subject: Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq On 05/31/2010 03:39 PM, Sanjeev Premi wrote: The OPP layer was contained in the CONFIG_CPU_FREQ. This patch removes this containment relation. Signed-off-by: Sanjeev Premi --- arch/arm/mach-omap2/Makefile |6 +- arch/arm/mach-omap2/board-omap3evm.c |2 +- [snip]--[snip] you sure this is the only board file having "omap3-opp.h" ? anyway.. the need for board files to use opp_init is gone with my patch http://marc.info/?l=linux-omap&m=127507237109393&w=2 so I wont harp on it, I would rather post a cleanup patch for all board files once the patch is in..(or mebbe kevin could drop the patch that adds opp_init_table to board files ;) ).. [sp] You didn't reead the 0/1 of the patch series, where I have clearly mentioned that I will make changes to the other board specific files once there rest of the changes are well discussed. There may be, possibly, more changes in the board specific files and we can review them in the context of this file and then same can be repeated for other board files. ok arch/arm/mach-omap2/cpufreq34xx.c | 164 arch/arm/mach-omap2/omap3-opp.h | 20 arch/arm/mach-omap2/opp34xx_data.c| 166 + arch/arm/mach-omap2/pm34xx.c |1 - arch/arm/plat-omap/Makefile |7 +- arch/arm/plat-omap/cpu-omap.c | 47 + arch/arm/plat-omap/include/plat/opp.h | 82 +--- arch/arm/plat-omap/opp.c | 46 - 10 files changed, 225 insertions(+), 316 deletions(-) delete mode 100644 arch/arm/mach-omap2/cpufreq34xx.c delete mode 100644 arch/arm/mach-omap2/omap3-opp.h create mode 100644 arch/arm/mach-omap2/opp34xx_data.c [sp] finding it difficult to align with this change, you introduce omap3_pm_init_opp_table later into plat/opp.h which defeats generic nature of opp.h - as it was supposed to be used for other omaps as well.. In that case the function omap3_pm_init_opp_table() can be made generic by renaming to omap_pm_init_opp_table and can be implemented for each omap family. Do you intend to handle multiomap case by calling each omap[1234]_pm_init_opp_table() if cpu_is_omap34xx() etc? you will still need a custom omap family specific init_opp_table - that is what this header provides. If opp table has to be implementyed for each family then why have different funtion with family specific prefixes? opp table contents will be different for each family and they should all build and co-exist in a single uImage. Also, what this headerf ile is/was doing? only defining the function to return -EINVAL when CONFIG_CPU_FREQ is not selected; which is not required. For OPP layer to be used this table needs to be populated. Now, there is only one place this function is used, so why do/should be need a header for the same. To allow the the external function that triggers it to be able to use it.. :) [snip]--[snip] +obj-y += opp.o +obj-$(CONFIG_TWL4030_POWER)+= opp_twl_tps.o NAK. you just need TWL4030_CORE not power here. any reason to retain power? it has no dependency on power.. [sp] Isn't this purpose of this opp to TWL linkage is to define the voltages in terms the PMIC connected; and later make sure that correct voltages are set via the PMIC? This is very much related no it does not. these are just translation functions, as long as _CORE exists, it means that the system uses twl and we should be good to go. to POWER. We could also do it on CORE; but I don't see this as a big issue. ok. But TWL4030 has more feature beyond PMIC but this is not the case with other simpler PMICs and I wanted to use CONFIG option that can be easier for someone else make the port easy to spot as a necessary change. i am aligned with the change, except that I believe you should not be using POWER as the prefix for the config build dependency. [snip]--[snip] + freq_table[i].index = i; + freq_table[i].frequency = CPUFREQ_TABLE_END; + + *table =&freq_table[0]; +} +#endif + e why? it used to be here and was moved to opp.c - see http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-p m.git;a=commit;h=9a6b00f70e9f4bce30ad4f8fab41a24bd3706dbd you are essentially reverting that patch! [sp] May be I am reverting the patch, but I don't see this function being used anywhere else. Most of the other cpufreq related initialization is happening at this place. Only the function omap_cpu_init() calls this function and it is in the same file. It also helps in need of an addit
RE: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq
> -Original Message- > From: Nishanth Menon [mailto:menon.nisha...@gmail.com] > Sent: Monday, May 31, 2010 10:59 PM > To: Premi, Sanjeev > Cc: linux-omap@vger.kernel.org > Subject: Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq > > On 05/31/2010 03:39 PM, Sanjeev Premi wrote: > > The OPP layer was contained in the CONFIG_CPU_FREQ. > > This patch removes this containment relation. > > > > Signed-off-by: Sanjeev Premi > > --- > > arch/arm/mach-omap2/Makefile |6 +- > > arch/arm/mach-omap2/board-omap3evm.c |2 +- [snip]--[snip] > you sure this is the only board file having "omap3-opp.h" ? > anyway.. the > need for board files to use opp_init is gone with my patch > http://marc.info/?l=linux-omap&m=127507237109393&w=2 > so I wont harp on it, I would rather post a cleanup patch for > all board > files once the patch is in..(or mebbe kevin could drop the patch that > adds opp_init_table to board files ;) ).. > [sp] You didn't reead the 0/1 of the patch series, where I have clearly mentioned that I will make changes to the other board specific files once there rest of the changes are well discussed. There may be, possibly, more changes in the board specific files and we can review them in the context of this file and then same can be repeated for other board files. > > arch/arm/mach-omap2/cpufreq34xx.c | 164 > > > arch/arm/mach-omap2/omap3-opp.h | 20 > > arch/arm/mach-omap2/opp34xx_data.c| 166 > + > > arch/arm/mach-omap2/pm34xx.c |1 - > > arch/arm/plat-omap/Makefile |7 +- > > arch/arm/plat-omap/cpu-omap.c | 47 + > > arch/arm/plat-omap/include/plat/opp.h | 82 +--- > > arch/arm/plat-omap/opp.c | 46 - > > 10 files changed, 225 insertions(+), 316 deletions(-) > > delete mode 100644 arch/arm/mach-omap2/cpufreq34xx.c > > delete mode 100644 arch/arm/mach-omap2/omap3-opp.h > > create mode 100644 arch/arm/mach-omap2/opp34xx_data.c > > [sp] > finding it difficult to align with this change, you introduce > omap3_pm_init_opp_table later into plat/opp.h which defeats generic > nature of opp.h - as it was supposed to be used for other > omaps as well.. In that case the function omap3_pm_init_opp_table() can be made generic by renaming to omap_pm_init_opp_table and can be implemented for each omap family. If opp table has to be implementyed for each family then why have different funtion with family specific prefixes? Also, what this headerf ile is/was doing? only defining the function to return -EINVAL when CONFIG_CPU_FREQ is not selected; which is not required. For OPP layer to be used this table needs to be populated. Now, there is only one place this function is used, so why do/should be need a header for the same. [snip]--[snip] > +obj-y+= opp.o > +obj-$(CONFIG_TWL4030_POWER) += opp_twl_tps.o NAK. you just need TWL4030_CORE not power here. any reason to retain power? it has no dependency on power.. [sp] Isn't this purpose of this opp to TWL linkage is to define the voltages in terms the PMIC connected; and later make sure that correct voltages are set via the PMIC? This is very much related to POWER. We could also do it on CORE; but I don't see this as a big issue. But TWL4030 has more feature beyond PMIC but this is not the case with other simpler PMICs and I wanted to use CONFIG option that can be easier for someone else make the port easy to spot as a necessary change. [snip]--[snip] > > + freq_table[i].index = i; > > + freq_table[i].frequency = CPUFREQ_TABLE_END; > > + > > + *table =&freq_table[0]; > > +} > > +#endif > > + > e why? it used to be here and was moved to opp.c - see > http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-p m.git;a=commit;h=9a6b00f70e9f4bce30ad4f8fab41a24bd3706dbd > you are essentially reverting that patch! [sp] May be I am reverting the patch, but I don't see this function being used anywhere else. Most of the other cpufreq related initialization is happening at this place. Only the function omap_cpu_init() calls this function and it is in the same file. It also helps in need of an additional header; which seem to make "de-linking" more complex - in terms of #ifdefs. [snip]--[snip] > > > +/** > > + * omap3_pm_init_opp_table() - Initialize the OPP table > for OMAP3 devices. > > + * > > + * Initializes the OPP table for the current OMAP3 device. > > + */ > > +int __init omap3_pm_init_opp_table(void); > NAK.
Re: [PATCHv1 1/1] omap3: pm: Delink opp layer and cpufreq
On 05/31/2010 03:39 PM, Sanjeev Premi wrote:
The OPP layer was contained in the CONFIG_CPU_FREQ.
This patch removes this containment relation.
Signed-off-by: Sanjeev Premi
---
arch/arm/mach-omap2/Makefile |6 +-
arch/arm/mach-omap2/board-omap3evm.c |2 +-
you sure this is the only board file having "omap3-opp.h" ? anyway.. the
need for board files to use opp_init is gone with my patch
http://marc.info/?l=linux-omap&m=127507237109393&w=2
so I wont harp on it, I would rather post a cleanup patch for all board
files once the patch is in..(or mebbe kevin could drop the patch that
adds opp_init_table to board files ;) )..
arch/arm/mach-omap2/cpufreq34xx.c | 164
arch/arm/mach-omap2/omap3-opp.h | 20
arch/arm/mach-omap2/opp34xx_data.c| 166 +
arch/arm/mach-omap2/pm34xx.c |1 -
arch/arm/plat-omap/Makefile |7 +-
arch/arm/plat-omap/cpu-omap.c | 47 +
arch/arm/plat-omap/include/plat/opp.h | 82 +---
arch/arm/plat-omap/opp.c | 46 -
10 files changed, 225 insertions(+), 316 deletions(-)
delete mode 100644 arch/arm/mach-omap2/cpufreq34xx.c
delete mode 100644 arch/arm/mach-omap2/omap3-opp.h
create mode 100644 arch/arm/mach-omap2/opp34xx_data.c
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index ac08f99..4e51a99 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -55,10 +55,8 @@ AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a
endif
-# CPU Frequency
-ifeq ($(CONFIG_CPU_FREQ),y)
-obj-$(CONFIG_ARCH_OMAP3) += cpufreq34xx.o
-endif
+# OPP definitions for OMAP3
+obj-$(CONFIG_ARCH_OMAP3) += opp34xx_data.o
# PRCM
obj-$(CONFIG_ARCH_OMAP2) += cm.o
diff --git a/arch/arm/mach-omap2/board-omap3evm.c
b/arch/arm/mach-omap2/board-omap3evm.c
index 0a3f5b6..cb97c5d 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -50,11 +50,11 @@
#include
#include
#include
+#include
#include "mux.h"
#include "sdram-micron-mt46h32m32lf-6.h"
#include "hsmmc.h"
-#include "omap3-opp.h"
#define GPMC_CS0_BASE 0x60
#define GPMC_CS_SIZE 0x30
diff --git a/arch/arm/mach-omap2/cpufreq34xx.c
b/arch/arm/mach-omap2/cpufreq34xx.c
deleted file mode 100644
index 8747dd6..000
--- a/arch/arm/mach-omap2/cpufreq34xx.c
+++ /dev/null
@@ -1,164 +0,0 @@
-/*
- * arch/arm/mach-omap2/cpufreq34xx.c
- * OMAP3 resource init/change_level/validate_level functions
- *
- * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
- * Nishanth Menon
- * Copyright (C) 2009 - 2010 Deep Root Systems, LLC.
- * Kevin Hilman
- * Copyright (C) 2010 Nokia Corporation.
- * Eduardo Valentin
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
- * History:
- *
- */
-
-#include
-#include
-
-#include
-#include
-#include "omap3-opp.h"
-
-static struct omap_opp_def __initdata omap34xx_mpu_rate_table[] = {
- /* OPP1 */
- OMAP_OPP_DEF(true, 12500, 975000),
- /* OPP2 */
- OMAP_OPP_DEF(true, 25000, 1075000),
- /* OPP3 */
- OMAP_OPP_DEF(true, 5, 120),
- /* OPP4 */
- OMAP_OPP_DEF(true, 55000, 127),
- /* OPP5 */
- OMAP_OPP_DEF(true, 6, 135),
- /* Terminator */
- OMAP_OPP_DEF(0, 0, 0)
-};
-
-static struct omap_opp_def __initdata omap34xx_l3_rate_table[] = {
- /*
-* OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
-* almost the same than the one at 83MHz thus providing very little
-* gain for the power point of view. In term of energy it will even
-* increase the consumption due to the very negative performance
-* impact that frequency will do to the MPU and the whole system in
-* general.
-*/
- OMAP_OPP_DEF(false, 4150, 975000),
- /* OPP2 */
- OMAP_OPP_DEF(true, 8300, 105),
- /* OPP3 */
- OMAP_OPP_DEF(true, 16600, 115),
- /* Terminator */
- OMAP_OPP_DEF(0, 0, 0)
-};
-
-static struct omap_opp_def __initdata omap34xx_dsp_rate_table[] = {
- /* OPP1 */
- OMAP_OPP_DEF(true, 9000, 975000),
- /* OPP2 */
- OMAP_OPP_DEF(true, 18000, 1075000),
- /* OPP3 */
- OMAP_OPP_DEF(true, 36000, 120),
- /* OPP4 */
- OMAP_OPP_DEF(true, 4, 127),
- /* OPP5 */
- OMAP_OPP_DEF(true, 43000, 135),
- /* Terminator */
- OMAP_OPP_DEF(0, 0
