Premi, Sanjeev had written, on 07/26/2010 10:35 AM, the following:
-----Original Message-----
From: Nishanth Menon [mailto:menon.nishanth@xxxxxxxxx]
Sent: Wednesday, June 02, 2010 10:07 AM
To: Premi, Sanjeev
Cc: linux-omap@xxxxxxxxxxxxxxx
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.nishanth@xxxxxxxxx]
Sent: Monday, May 31, 2010 10:59 PM
To: Premi, Sanjeev
Cc: linux-omap@xxxxxxxxxxxxxxx
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<premi@xxxxxx>
---
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
+
errrr.... 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.
the idea was to have a common function for ALL omaps to
create the table
and reuse it where ever needed, if you look beyond omap3 into omap1,2
and 4, the ability to do this is invaluable. does it matter if a
function exists in the library even if not used?
[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. opp. is meant to be used by omap2, OMAP4 etc..
when you removed from omap3-opp.h, it kinda needed you to
have it here,
which breaks the generic nature of this header.
[sp] See my comment earlier. The function for init-ing the OPP
table can be made generic. Then (after rename) this is a
generic function itself.
Rather than making sweelping changes, I am only delinking
the OPP layer and CPU freq. These changes can be done
separately.
replied above.
-#endif /* CONFIG_CPU_FREQ */
#endif /* __ASM_ARM_OMAP_OPP_H */
diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index 13da451..3ed3ec1 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -351,49 +351,3 @@ int opp_disable(struct omap_opp *opp)
opp->enabled = false;
return 0;
}
-
-/* XXX document */
-void opp_init_cpufreq_table(enum opp_t opp_type,
- struct cpufreq_frequency_table **table)
-{
- int i = 0, j;
- int opp_num;
- struct cpufreq_frequency_table *freq_table;
- struct omap_opp *opp;
-
- if (opp_type>= OPP_TYPES_MAX) {
- pr_warning("%s: failed to initialize frequency"
- "table\n", __func__);
- return;
- }
-
- opp_num = opp_get_opp_count(opp_type);
- if (opp_num< 0) {
- pr_err("%s: no opp table?\n", __func__);
- return;
- }
-
- freq_table = kmalloc(sizeof(struct cpufreq_frequency_table) *
- (opp_num + 1), GFP_ATOMIC);
- if (!freq_table) {
- pr_warning("%s: failed to allocate frequency"
- "table\n", __func__);
- return;
- }
-
- opp = _opp_list[opp_type];
- opp += opp_num;
- for (j = opp_num; j>= 0; j--) {
- if (opp->enabled) {
- freq_table[i].index = i;
- freq_table[i].frequency = opp->rate / 1000;
- i++;
- }
- opp--;
- }
-
- freq_table[i].index = i;
- freq_table[i].frequency = CPUFREQ_TABLE_END;
-
- *table =&freq_table[0];
-}
not sure why you removed this..
[sp] It isn't removed but simply moved to the only file in
the code where it
is being used... along with rest of the code related
to CPU_FREQ.
The way I see, the OPP layer has been mixed with CPUFREQ
usage in the
cureent code; but if you look at OPP layer as as "real"
layer then the
CPUFREQ implementation should be the "client" to this later
and use its
services - not get 'mingled' into the layer itself. If you
go by this
reasoning, this init function belong outside the OPP layer.
I have explained on top, further, the only set of dependencies i see:
a) naming of the the files
b) build and #ifdef CPUFREQ dependencies
these are changes I liked from your patch.
[sp] Picking this thread after long time (vacation and other digressions)
I haven't understood the last part in your comments. The premise of
the patch your comments seems to apparent "reversal" done. But if it
is helping achieve desired independence, then shouldn't it be done?
Regarding filenames, what eaxctly is the issue?
I believe it was omap3-opp.h - which is not relevant anymore since we
moved to hwmods for opp layer as well.. overall, do leave the cpufreq
api alone, rebase, rename to opp34xx_data.c, decouple from CPUFREQ seems
to be the main items left to do here..
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
