[PATCH-V2 0/4] ARM: OMAP2+: Cleanup series in order to remove ARCH_OMAPx dependency

[PATCH-V2 0/4] ARM: OMAP2+: Cleanup series in order to remove ARCH_OMAPx dependency

[Vaibhav Hiremath]

In current implementation, some places we are still using
ARCH_OMAPx config option, making it difficult to add new devices;
for example, while adding am33xx device support I came across multiple
instances where I had to patch the existing code to make it work for
am33xx.

This patch tries to cleanup existing code for some of the
ARCH_OMAP2/3/4 dependency on the code.

NOTE: Patch series has been build tested for all possible
      combination of OMAP2, 3, 4 configurations.

History on Patch-series:
========================
Changes from V1:
   - Fixed comments from Tony
	- Config option name of SOC_HAS_SDRC changed to
	  SOC_HAS_OMAP2_SDRC
	- Config option name for SOC_HAS_DPLL_xxx flags to
	  SOC_HAS_OMAP3_DPLL_xxx.

Vaibhav Hiremath (4):
  ARM: OMAP2+: CLEANUP: All OMAP2PLUS uses omap-device.o target so add
    one entry
  ARM: OMAP2+: CLEANUP: Move omap3 dpll ops to dpll3xxx.c
  ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around
    __omap2_set_globals
  ARM: OMAP2+: CLEANUP: Add new config option for different DPLL
    features

 arch/arm/mach-omap2/Kconfig             |   42 +++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/clock.c             |   18 -------------
 arch/arm/mach-omap2/common.c            |    8 +-----
 arch/arm/mach-omap2/common.h            |    5 +++
 arch/arm/mach-omap2/dpll3xxx.c          |   14 ++++++++++
 arch/arm/plat-omap/Makefile             |    4 +--
 arch/arm/plat-omap/include/plat/clock.h |   14 ++++++++--
 7 files changed, 74 insertions(+), 31 deletions(-)

[PATCH-V2 1/4] ARM: OMAP2+: CLEANUP: All OMAP2PLUS uses omap-device.o target so add one entry

[Vaibhav Hiremath]

All OMAP2PLUS based devices, builds omap-device.o target;
so just add one entry so that there is no need to patch this file
for any future OMAP2+ devices.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/plat-omap/Makefile |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index ed8605f..c5b5f05 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -10,9 +10,7 @@ obj-n :=
 obj-  :=

 # omap_device support (OMAP2+ only at the moment)
-obj-$(CONFIG_ARCH_OMAP2) += omap_device.o
-obj-$(CONFIG_ARCH_OMAP3) += omap_device.o
-obj-$(CONFIG_ARCH_OMAP4) += omap_device.o
+obj-$(CONFIG_ARCH_OMAP2PLUS) += omap_device.o

 obj-$(CONFIG_OMAP_DM_TIMER) += dmtimer.o
 obj-$(CONFIG_OMAP_DEBUG_DEVICES) += debug-devices.o
--
1.7.0.4

[PATCH-V2 2/4] ARM: OMAP2+: CLEANUP: Move omap3 dpll ops to dpll3xxx.c

[Vaibhav Hiremath]

In order to remove unnecessary idefs, move noncore and core
dpll ops to dpll3xxx.c file (where it should have been already).

The clkops (clkops_omap3_core_dpll_ops & clkops_omap3_noncore_dpll_ops)
is used in clock data files, and dependency is already handled by
Makefile rule.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/clock.c    |   18 ------------------
 arch/arm/mach-omap2/dpll3xxx.c |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
index d9f4931..950a446 100644
--- a/arch/arm/mach-omap2/clock.c
+++ b/arch/arm/mach-omap2/clock.c
@@ -398,24 +398,6 @@ int omap2_clk_set_parent(struct clk *clk, struct clk *new_parent)
 	return omap2_clksel_set_parent(clk, new_parent);
 }
 
-/* OMAP3/4 non-CORE DPLL clkops */
-
-#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
-
-const struct clkops clkops_omap3_noncore_dpll_ops = {
-	.enable		= omap3_noncore_dpll_enable,
-	.disable	= omap3_noncore_dpll_disable,
-	.allow_idle	= omap3_dpll_allow_idle,
-	.deny_idle	= omap3_dpll_deny_idle,
-};
-
-const struct clkops clkops_omap3_core_dpll_ops = {
-	.allow_idle	= omap3_dpll_allow_idle,
-	.deny_idle	= omap3_dpll_deny_idle,
-};
-
-#endif
-
 /*
  * OMAP2+ clock reset and init functions
  */
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index fc56745..d15f2a2 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -615,3 +615,17 @@ unsigned long omap3_clkoutx2_recalc(struct clk *clk)
 		rate = clk->parent->rate * 2;
 	return rate;
 }
+
+/* OMAP3/4 non-CORE DPLL clkops */
+
+const struct clkops clkops_omap3_noncore_dpll_ops = {
+	.enable		= omap3_noncore_dpll_enable,
+	.disable	= omap3_noncore_dpll_disable,
+	.allow_idle	= omap3_dpll_allow_idle,
+	.deny_idle	= omap3_dpll_deny_idle,
+};
+
+const struct clkops clkops_omap3_core_dpll_ops = {
+	.allow_idle	= omap3_dpll_allow_idle,
+	.deny_idle	= omap3_dpll_deny_idle,
+};
-- 
1.7.0.4

[PATCH-V2 2/4] ARM: OMAP2+: CLEANUP: Move omap3 dpll ops to dpll3xxx.c

[Paul Walmsley]

On Wed, 9 May 2012, Vaibhav Hiremath wrote:

> In order to remove unnecessary idefs, move noncore and core
> dpll ops to dpll3xxx.c file (where it should have been already).
> 
> The clkops (clkops_omap3_core_dpll_ops & clkops_omap3_noncore_dpll_ops)
> is used in clock data files, and dependency is already handled by
> Makefile rule.
> 
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>

This is probably okay.

Acked-by: Paul Walmsley <paul@pwsan.com>


- Paul

[PATCH-V2 2/4] ARM: OMAP2+: CLEANUP: Move omap3 dpll ops to dpll3xxx.c

[Hiremath, Vaibhav]

On Fri, May 11, 2012 at 04:32:11, Paul Walmsley wrote:
> On Wed, 9 May 2012, Vaibhav Hiremath wrote:
> 
> > In order to remove unnecessary idefs, move noncore and core
> > dpll ops to dpll3xxx.c file (where it should have been already).
> > 
> > The clkops (clkops_omap3_core_dpll_ops & clkops_omap3_noncore_dpll_ops)
> > is used in clock data files, and dependency is already handled by
> > Makefile rule.
> > 
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Kevin Hilman <khilman@ti.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> 
> This is probably okay.
> 
> Acked-by: Paul Walmsley <paul@pwsan.com>
> 
> 

Thanks Paul,

Thanks,
Vaibhav

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Vaibhav Hiremath]

The function __omap2_set_globals() can be common across all
platforms/architectures, even in case of omap4, internally it
calls same set of functions as in __omap2_set_globals() function
(except for sdrc).
This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
so that we can reuse same function across omap2/3/4...

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/Kconfig  |    8 ++++++++
 arch/arm/mach-omap2/common.c |    8 +-------
 arch/arm/mach-omap2/common.h |    5 +++++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 0685dc8..a432f3e 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -20,12 +20,16 @@ config ARCH_OMAP2PLUS_TYPICAL
 	help
 	  Compile a kernel suitable for booting most boards

+config SOC_HAS_OMAP2_SDRC
+	bool "OMAP2 SDRAM Controller support"
+
 config ARCH_OMAP2
 	bool "TI OMAP2"
 	depends on ARCH_OMAP2PLUS
 	default y
 	select CPU_V6
 	select MULTI_IRQ_HANDLER
+	select SOC_HAS_OMAP2_SDRC

 config ARCH_OMAP3
 	bool "TI OMAP3"
@@ -37,6 +41,7 @@ config ARCH_OMAP3
 	select PM_OPP if PM
 	select ARM_CPU_SUSPEND if PM
 	select MULTI_IRQ_HANDLER
+	select SOC_HAS_OMAP2_SDRC

 config ARCH_OMAP4
 	bool "TI OMAP4"
@@ -64,18 +69,21 @@ config SOC_OMAP2420
 	default y
 	select OMAP_DM_TIMER
 	select ARCH_OMAP_OTG
+	select SOC_HAS_OMAP2_SDRC

 config SOC_OMAP2430
 	bool "OMAP2430 support"
 	depends on ARCH_OMAP2
 	default y
 	select ARCH_OMAP_OTG
+	select SOC_HAS_OMAP2_SDRC

 config SOC_OMAP3430
 	bool "OMAP3430 support"
 	depends on ARCH_OMAP3
 	default y
 	select ARCH_OMAP_OTG
+	select SOC_HAS_OMAP2_SDRC

 config SOC_OMAPTI81XX
 	bool "TI81XX support"
diff --git a/arch/arm/mach-omap2/common.c b/arch/arm/mach-omap2/common.c
index 1549c11..ad8626d 100644
--- a/arch/arm/mach-omap2/common.c
+++ b/arch/arm/mach-omap2/common.c
@@ -29,8 +29,6 @@

 /* Global address base setup code */

-#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
-
 static void __init __omap2_set_globals(struct omap_globals *omap2_globals)
 {
 	omap2_set_globals_tap(omap2_globals);
@@ -39,8 +37,6 @@ static void __init __omap2_set_globals(struct omap_globals *omap2_globals)
 	omap2_set_globals_prcm(omap2_globals);
 }

-#endif
-
 #if defined(CONFIG_SOC_OMAP2420)

 static struct omap_globals omap242x_globals = {
@@ -170,9 +166,7 @@ static struct omap_globals omap4_globals = {

 void __init omap2_set_globals_443x(void)
 {
-	omap2_set_globals_tap(&omap4_globals);
-	omap2_set_globals_control(&omap4_globals);
-	omap2_set_globals_prcm(&omap4_globals);
+	__omap2_set_globals(&omap4_globals);
 }

 void __init omap4_map_io(void)
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 0e95efc..87b132e 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -122,7 +122,12 @@ void omap2_set_globals_am33xx(void);

 /* These get called from omap2_set_globals_xxxx(), do not call these */
 void omap2_set_globals_tap(struct omap_globals *);
+#if defined(CONFIG_SOC_HAS_OMAP2_SDRC)
 void omap2_set_globals_sdrc(struct omap_globals *);
+#else
+static inline void omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
+{ }
+#endif
 void omap2_set_globals_control(struct omap_globals *);
 void omap2_set_globals_prcm(struct omap_globals *);

--
1.7.0.4

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Kevin Hilman]

Vaibhav Hiremath <hvaibhav@ti.com> writes:

> The function __omap2_set_globals() can be common across all
> platforms/architectures, even in case of omap4, internally it
> calls same set of functions as in __omap2_set_globals() function
> (except for sdrc).

OK so far.

> This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
> so that we can reuse same function across omap2/3/4...

But what happens when a single kernel is built that has support for an
SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?

In that case this new SOC_HAS_OMAP2_SDRC will be set, and
set_globals_sdrc() will be called even for the SoCs without SDRC.

So, rather than add a new Kconfig option for this, I would rather see 
you using the existing runtime feature check for the SDRC: omap_has_sdrc()

Kevin

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Hiremath, Vaibhav]

On Wed, May 09, 2012 at 04:08:09, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
> > The function __omap2_set_globals() can be common across all
> > platforms/architectures, even in case of omap4, internally it
> > calls same set of functions as in __omap2_set_globals() function
> > (except for sdrc).
> 
> OK so far.
> 
> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
> > so that we can reuse same function across omap2/3/4...
> 
> But what happens when a single kernel is built that has support for an
> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
> 

As such Nothing...I looking into this direction while implementing.

In that case, sdrc.c file will be compiled in and execution will jump to
omap2_set_globals_sdrc(). But inside this function, we are already checking 
whether the omap2_globals->sdrc and omap2_globals->sms for NULL and then use 
it.

And function omap2_sdrc_init() is also depends on machine, so in case of
Am33xx, it won't get into sdrc execution at all. And in case of omap4, it 
will.

> In that case this new SOC_HAS_OMAP2_SDRC will be set, and
> set_globals_sdrc() will be called even for the SoCs without SDRC.
> 
> So, rather than add a new Kconfig option for this, I would rather see 
> you using the existing runtime feature check for the SDRC: omap_has_sdrc()
> 

There is NO difference between runtime feature check Vs this patch, refer to the function implementation,

void __init omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
{
	if (omap2_globals->sdrc)
		omap2_sdrc_base = omap2_globals->sdrc;
	if (omap2_globals->sms)
		omap2_sms_base = omap2_globals->sms;
}

The initialization happens after checking for NULL, so even if you execute it or not, the variable are set to NULL in case of am33xx.

So I don't find any difference between runtime and this patch.

Thanks,
Vaibhav

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Kevin Hilman]

"Hiremath, Vaibhav" <hvaibhav@ti.com> writes:

> On Wed, May 09, 2012 at 04:08:09, Hilman, Kevin wrote:
>> Vaibhav Hiremath <hvaibhav@ti.com> writes:
>> 
>> > The function __omap2_set_globals() can be common across all
>> > platforms/architectures, even in case of omap4, internally it
>> > calls same set of functions as in __omap2_set_globals() function
>> > (except for sdrc).
>> 
>> OK so far.
>> 
>> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
>> > so that we can reuse same function across omap2/3/4...
>> 
>> But what happens when a single kernel is built that has support for an
>> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
>> 
>
> As such Nothing...I looking into this direction while implementing.
>
> In that case, sdrc.c file will be compiled in and execution will jump to
> omap2_set_globals_sdrc(). But inside this function, we are already checking 
> whether the omap2_globals->sdrc and omap2_globals->sms for NULL and then use 
> it.
>
> And function omap2_sdrc_init() is also depends on machine, so in case of
> Am33xx, it won't get into sdrc execution at all. And in case of omap4, it 
> will.

Then why bother with the #ifdef at all?

If it already safe to call on all SoCs, just get rid of the #ifdef all
together.

>> In that case this new SOC_HAS_OMAP2_SDRC will be set, and
>> set_globals_sdrc() will be called even for the SoCs without SDRC.
>> 
>> So, rather than add a new Kconfig option for this, I would rather see 
>> you using the existing runtime feature check for the SDRC: omap_has_sdrc()
>> 
>
> There is NO difference between runtime feature check Vs this patch, refer to the function implementation,
>
> void __init omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
> {
> 	if (omap2_globals->sdrc)
> 		omap2_sdrc_base = omap2_globals->sdrc;
> 	if (omap2_globals->sms)
> 		omap2_sms_base = omap2_globals->sms;
> }
>
> The initialization happens after checking for NULL, so even if you execute it or not, the variable are set to NULL in case of am33xx.
>
> So I don't find any difference between runtime and this patch.

Except readability.

Kevin

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Hiremath, Vaibhav]

On Fri, May 11, 2012 at 03:09:39, Hilman, Kevin wrote:
> "Hiremath, Vaibhav" <hvaibhav@ti.com> writes:
> 
> > On Wed, May 09, 2012 at 04:08:09, Hilman, Kevin wrote:
> >> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> >> 
> >> > The function __omap2_set_globals() can be common across all
> >> > platforms/architectures, even in case of omap4, internally it
> >> > calls same set of functions as in __omap2_set_globals() function
> >> > (except for sdrc).
> >> 
> >> OK so far.
> >> 
> >> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
> >> > so that we can reuse same function across omap2/3/4...
> >> 
> >> But what happens when a single kernel is built that has support for an
> >> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
> >> 
> >
> > As such Nothing...I looking into this direction while implementing.
> >
> > In that case, sdrc.c file will be compiled in and execution will jump to
> > omap2_set_globals_sdrc(). But inside this function, we are already checking 
> > whether the omap2_globals->sdrc and omap2_globals->sms for NULL and then use 
> > it.
> >
> > And function omap2_sdrc_init() is also depends on machine, so in case of
> > Am33xx, it won't get into sdrc execution at all. And in case of omap4, it 
> > will.
> 
> Then why bother with the #ifdef at all?
> 
> If it already safe to call on all SoCs, just get rid of the #ifdef all
> together.
> 

Kevin,

sdrc.o target gets built only as "omap-2-3-common", this will not get built 
for omap4, am33xx, ti81xx, etc...
So in order to avoid build break, you have to have some mechanism, and that's where we need to create config option dependent on platform.

Another better way of handling this is adding __weak function.

What's your opinion on this?

Thanks,
Vaibhav

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Sricharan R]

Vaibhav,

> > >> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle
> sdrc,
> > >> > so that we can reuse same function across omap2/3/4...
> > >>
> > >> But what happens when a single kernel is built that has support
> for an
> > >> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
> > >>
> > >
> > > As such Nothing...I looking into this direction while implementing.
> > >
> > > In that case, sdrc.c file will be compiled in and execution will
> jump to
> > > omap2_set_globals_sdrc(). But inside this function, we are already
> checking
> > > whether the omap2_globals->sdrc and omap2_globals->sms for NULL and
> then use
> > > it.
> > >
> > > And function omap2_sdrc_init() is also depends on machine, so in
> case of
> > > Am33xx, it won't get into sdrc execution at all. And in case of
> omap4, it
> > > will.
> >
> > Then why bother with the #ifdef at all?
> >
> > If it already safe to call on all SoCs, just get rid of the #ifdef
> all
> > together.
> >
>
> Kevin,
>
> sdrc.o target gets built only as "omap-2-3-common", this will not get
> built
> for omap4, am33xx, ti81xx, etc...
> So in order to avoid build break, you have to have some mechanism, and
> that's where we need to create config option dependent on platform.
>
> Another better way of handling this is adding __weak function.
>
> What's your opinion on this?
>
  Then how about just allowing to compile for all omap2/3/4 ?

Thanks,
 Sricharan

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Hiremath, Vaibhav]

On Fri, May 11, 2012 at 12:07:59, R, Sricharan wrote:
> Vaibhav,
> 
> > > >> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle
> > sdrc,
> > > >> > so that we can reuse same function across omap2/3/4...
> > > >>
> > > >> But what happens when a single kernel is built that has support
> > for an
> > > >> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
> > > >>
> > > >
> > > > As such Nothing...I looking into this direction while implementing.
> > > >
> > > > In that case, sdrc.c file will be compiled in and execution will
> > jump to
> > > > omap2_set_globals_sdrc(). But inside this function, we are already
> > checking
> > > > whether the omap2_globals->sdrc and omap2_globals->sms for NULL and
> > then use
> > > > it.
> > > >
> > > > And function omap2_sdrc_init() is also depends on machine, so in
> > case of
> > > > Am33xx, it won't get into sdrc execution at all. And in case of
> > omap4, it
> > > > will.
> > >
> > > Then why bother with the #ifdef at all?
> > >
> > > If it already safe to call on all SoCs, just get rid of the #ifdef
> > all
> > > together.
> > >
> >
> > Kevin,
> >
> > sdrc.o target gets built only as "omap-2-3-common", this will not get
> > built
> > for omap4, am33xx, ti81xx, etc...
> > So in order to avoid build break, you have to have some mechanism, and
> > that's where we need to create config option dependent on platform.
> >
> > Another better way of handling this is adding __weak function.
> >
> > What's your opinion on this?
> >
>   Then how about just allowing to compile for all omap2/3/4 ?
> 

I wouldn't recommend that. 
Also, now we have almost all the devices coming in with EMIF (no sdrc).

Thanks,
Vaibhav

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Kevin Hilman]

"Hiremath, Vaibhav" <hvaibhav@ti.com> writes:

> On Fri, May 11, 2012 at 03:09:39, Hilman, Kevin wrote:
>> "Hiremath, Vaibhav" <hvaibhav@ti.com> writes:
>> 
>> > On Wed, May 09, 2012 at 04:08:09, Hilman, Kevin wrote:
>> >> Vaibhav Hiremath <hvaibhav@ti.com> writes:
>> >> 
>> >> > The function __omap2_set_globals() can be common across all
>> >> > platforms/architectures, even in case of omap4, internally it
>> >> > calls same set of functions as in __omap2_set_globals() function
>> >> > (except for sdrc).
>> >> 
>> >> OK so far.
>> >> 
>> >> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
>> >> > so that we can reuse same function across omap2/3/4...
>> >> 
>> >> But what happens when a single kernel is built that has support for an
>> >> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
>> >> 
>> >
>> > As such Nothing...I looking into this direction while implementing.
>> >
>> > In that case, sdrc.c file will be compiled in and execution will jump to
>> > omap2_set_globals_sdrc(). But inside this function, we are already checking 
>> > whether the omap2_globals->sdrc and omap2_globals->sms for NULL and then use 
>> > it.
>> >
>> > And function omap2_sdrc_init() is also depends on machine, so in case of
>> > Am33xx, it won't get into sdrc execution at all. And in case of omap4, it 
>> > will.
>> 
>> Then why bother with the #ifdef at all?
>> 
>> If it already safe to call on all SoCs, just get rid of the #ifdef all
>> together.
>> 
>
> Kevin,
>
> sdrc.o target gets built only as "omap-2-3-common", this will not get built 
> for omap4, am33xx, ti81xx, etc...

OK, I see what you mean now.  I was confusing because this patch doesn't
touch sdrc.c, or the Makefile for sdrc.c.

> So in order to avoid build break, you have to have some mechanism, and
> that's where we need to create config option dependent on platform.

That being the case, for readability sake, I suggest you change the
Makefile to use the new config option for sdrc.c as well.

Kevin

[PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Hiremath, Vaibhav]

On Sat, May 12, 2012 at 03:24:51, Hilman, Kevin wrote:
> "Hiremath, Vaibhav" <hvaibhav@ti.com> writes:
> 
> > On Fri, May 11, 2012 at 03:09:39, Hilman, Kevin wrote:
> >> "Hiremath, Vaibhav" <hvaibhav@ti.com> writes:
> >> 
> >> > On Wed, May 09, 2012 at 04:08:09, Hilman, Kevin wrote:
> >> >> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> >> >> 
> >> >> > The function __omap2_set_globals() can be common across all
> >> >> > platforms/architectures, even in case of omap4, internally it
> >> >> > calls same set of functions as in __omap2_set_globals() function
> >> >> > (except for sdrc).
> >> >> 
> >> >> OK so far.
> >> >> 
> >> >> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
> >> >> > so that we can reuse same function across omap2/3/4...
> >> >> 
> >> >> But what happens when a single kernel is built that has support for an
> >> >> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
> >> >> 
> >> >
> >> > As such Nothing...I looking into this direction while implementing.
> >> >
> >> > In that case, sdrc.c file will be compiled in and execution will jump to
> >> > omap2_set_globals_sdrc(). But inside this function, we are already checking 
> >> > whether the omap2_globals->sdrc and omap2_globals->sms for NULL and then use 
> >> > it.
> >> >
> >> > And function omap2_sdrc_init() is also depends on machine, so in case of
> >> > Am33xx, it won't get into sdrc execution at all. And in case of omap4, it 
> >> > will.
> >> 
> >> Then why bother with the #ifdef at all?
> >> 
> >> If it already safe to call on all SoCs, just get rid of the #ifdef all
> >> together.
> >> 
> >
> > Kevin,
> >
> > sdrc.o target gets built only as "omap-2-3-common", this will not get built 
> > for omap4, am33xx, ti81xx, etc...
> 
> OK, I see what you mean now.  I was confusing because this patch doesn't
> touch sdrc.c, or the Makefile for sdrc.c.
> 

I think I understand the source of confusion here, but now we are on same 
page...

> > So in order to avoid build break, you have to have some mechanism, and
> > that's where we need to create config option dependent on platform.
> 
> That being the case, for readability sake, I suggest you change the
> Makefile to use the new config option for sdrc.c as well.
> 

Will submit the patch for this today.

Thanks,
Vaibhav

[PATCH-V2 4/4] ARM: OMAP2+: CLEANUP: Add new config option for different DPLL features

[Vaibhav Hiremath]

This patch cleans up dpll_data structure, making structure
fields definition based on feature availability for given SoC,
managed using Kconfig rules.

SOC_HAS_OMAP3_DPLL_IDLE: idle state
SOC_HAS_OMAP3_DPLL_RECAL: recalibration capability
SOC_HAS_OMAP3_DPLL_DCO_SEL: dco selection
SOC_HAS_OMAP3_DPLL_SDDIV: sigma-delta div factor
SOC_HAS_OMAP3_DPLL_FREQSEL: frequency selection

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/Kconfig             |   34 +++++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/clock.h |   14 ++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index a432f3e..23999eb 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -20,6 +20,21 @@ config ARCH_OMAP2PLUS_TYPICAL
 	help
 	  Compile a kernel suitable for booting most boards

+config SOC_HAS_OMAP3_DPLL_IDLE
+	bool "OMAP3 DPLL idle state support"
+
+config SOC_HAS_OMAP3_DPLL_RECAL
+	bool "OMAP3 DPLL Recalibration support"
+
+config SOC_HAS_OMAP3_DPLL_DCO_SEL
+	bool "OMAP3 DPLL DCO (Digitally Controlled Oscilators) support"
+
+config SOC_HAS_OMAP3_DPLL_SDDIV
+	bool "OMAP3 DPLL Sigma-Delta Divider support"
+
+config SOC_HAS_OMAP3_DPLL_FREQSEL
+	bool "OMAP3 DPLL Freq Select support"
+
 config SOC_HAS_OMAP2_SDRC
 	bool "OMAP2 SDRAM Controller support"

@@ -42,6 +57,11 @@ config ARCH_OMAP3
 	select ARM_CPU_SUSPEND if PM
 	select MULTI_IRQ_HANDLER
 	select SOC_HAS_OMAP2_SDRC
+	select SOC_HAS_OMAP3_DPLL_IDLE
+	select SOC_HAS_OMAP3_DPLL_RECAL
+	select SOC_HAS_OMAP3_DPLL_DCO_SEL
+	select SOC_HAS_OMAP3_DPLL_SDDIV
+	select SOC_HAS_OMAP3_DPLL_FREQSEL

 config ARCH_OMAP4
 	bool "TI OMAP4"
@@ -59,6 +79,9 @@ config ARCH_OMAP4
 	select PM_OPP if PM
 	select USB_ARCH_HAS_EHCI if USB_SUPPORT
 	select ARM_CPU_SUSPEND if PM
+	select SOC_HAS_OMAP3_DPLL_IDLE
+	select SOC_HAS_OMAP3_DPLL_RECAL
+	select SOC_HAS_OMAP3_DPLL_SDDIV

 comment "OMAP Core Type"
 	depends on ARCH_OMAP2
@@ -84,16 +107,27 @@ config SOC_OMAP3430
 	default y
 	select ARCH_OMAP_OTG
 	select SOC_HAS_OMAP2_SDRC
+	select SOC_HAS_OMAP3_DPLL_IDLE
+	select SOC_HAS_OMAP3_DPLL_RECAL
+	select SOC_HAS_OMAP3_DPLL_DCO_SEL
+	select SOC_HAS_OMAP3_DPLL_SDDIV
+	select SOC_HAS_OMAP3_DPLL_FREQSEL

 config SOC_OMAPTI81XX
 	bool "TI81XX support"
 	depends on ARCH_OMAP3
 	default y
+	select SOC_HAS_OMAP3_DPLL_IDLE
+	select SOC_HAS_OMAP3_DPLL_RECAL
+	select SOC_HAS_OMAP3_DPLL_SDDIV

 config SOC_OMAPAM33XX
 	bool "AM33XX support"
 	depends on ARCH_OMAP3
 	default y
+	select SOC_HAS_OMAP3_DPLL_IDLE
+	select SOC_HAS_OMAP3_DPLL_RECAL
+	select SOC_HAS_OMAP3_DPLL_SDDIV

 config OMAP_PACKAGE_ZAF
        bool
diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat-omap/include/plat/clock.h
index d0ef57c..8d40de8 100644
--- a/arch/arm/plat-omap/include/plat/clock.h
+++ b/arch/arm/plat-omap/include/plat/clock.h
@@ -156,18 +156,26 @@ struct dpll_data {
 	u8			min_divider;
 	u16			max_divider;
 	u8			modes;
-#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
+#if defined(CONFIG_SOC_HAS_OMAP3_DPLL_IDLE)
 	void __iomem		*autoidle_reg;
 	void __iomem		*idlest_reg;
 	u32			autoidle_mask;
-	u32			freqsel_mask;
 	u32			idlest_mask;
+#endif
+#if defined(CONFIG_SOC_HAS_OMAP3_DPLL_FREQSEL)
+	u32			freqsel_mask;
+#endif
+#if defined(CONFIG_SOC_HAS_OMAP3_DPLL_DCO_SEL)
 	u32			dco_mask;
+#endif
+#if defined(CONFIG_SOC_HAS_OMAP3_DPLL_SDDIV)
 	u32			sddiv_mask;
+#endif
+#if defined(CONFIG_SOC_HAS_OMAP3_DPLL_RECAL)
 	u8			auto_recal_bit;
 	u8			recal_en_bit;
 	u8			recal_st_bit;
-#  endif
+#endif
 	u8			flags;
 };

--
1.7.0.4

[PATCH-V2 4/4] ARM: OMAP2+: CLEANUP: Add new config option for different DPLL features

[Kevin Hilman]

Vaibhav Hiremath <hvaibhav@ti.com> writes:

> This patch cleans up dpll_data structure, making structure
> fields definition based on feature availability for given SoC,
> managed using Kconfig rules.
>
> SOC_HAS_OMAP3_DPLL_IDLE: idle state
> SOC_HAS_OMAP3_DPLL_RECAL: recalibration capability
> SOC_HAS_OMAP3_DPLL_DCO_SEL: dco selection
> SOC_HAS_OMAP3_DPLL_SDDIV: sigma-delta div factor
> SOC_HAS_OMAP3_DPLL_FREQSEL: frequency selection
>
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>

Paul is the one to make the call here, but personally I'd much prefer to
just see the existing ifdefs go away[1] instead of adding a bunch of new
ones.  

Yes, doing so would add a few fields to a struct that may be unused, but
IMO, the improvement in readabilitly and maintainability is improved.

Note that the users of these fields are not changed and are still based
on Kconfig/Makefile values as before (e.g. CONFIG_ARCH_OMAP3), so to me
this creates another layer of obfuscation.

Kevin


[1]
diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat-omap/include/plat/clock.h
index d0ef57c..656b986 100644
--- a/arch/arm/plat-omap/include/plat/clock.h
+++ b/arch/arm/plat-omap/include/plat/clock.h
@@ -156,7 +156,6 @@ struct dpll_data {
 	u8			min_divider;
 	u16			max_divider;
 	u8			modes;
-#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
 	void __iomem		*autoidle_reg;
 	void __iomem		*idlest_reg;
 	u32			autoidle_mask;
@@ -167,7 +166,6 @@ struct dpll_data {
 	u8			auto_recal_bit;
 	u8			recal_en_bit;
 	u8			recal_st_bit;
-#  endif
 	u8			flags;
 };
 

[PATCH-V2 4/4] ARM: OMAP2+: CLEANUP: Add new config option for different DPLL features

[Hiremath, Vaibhav]

On Wed, May 09, 2012 at 04:20:06, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
> > This patch cleans up dpll_data structure, making structure
> > fields definition based on feature availability for given SoC,
> > managed using Kconfig rules.
> >
> > SOC_HAS_OMAP3_DPLL_IDLE: idle state
> > SOC_HAS_OMAP3_DPLL_RECAL: recalibration capability
> > SOC_HAS_OMAP3_DPLL_DCO_SEL: dco selection
> > SOC_HAS_OMAP3_DPLL_SDDIV: sigma-delta div factor
> > SOC_HAS_OMAP3_DPLL_FREQSEL: frequency selection
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Kevin Hilman <khilman@ti.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> 
> Paul is the one to make the call here, but personally I'd much prefer to
> just see the existing ifdefs go away[1] instead of adding a bunch of new
> ones.  
> 
> Yes, doing so would add a few fields to a struct that may be unused, but
> IMO, the improvement in readabilitly and maintainability is improved.
> 
> Note that the users of these fields are not changed and are still based
> on Kconfig/Makefile values as before (e.g. CONFIG_ARCH_OMAP3), so to me
> this creates another layer of obfuscation.
> 

This will bring in difference on omap2 alone build OR omap4, am33xx alone 
builds.
But again, how much it makes sense, and how much benefits we bring-in by 
adding config options for every bit-fields (like this) needs to be looked at.
And that's the reason, I submitted first version as a RFC.


> 
> [1]
> diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat-omap/include/plat/clock.h
> index d0ef57c..656b986 100644
> --- a/arch/arm/plat-omap/include/plat/clock.h
> +++ b/arch/arm/plat-omap/include/plat/clock.h
> @@ -156,7 +156,6 @@ struct dpll_data {
>  	u8			min_divider;
>  	u16			max_divider;
>  	u8			modes;
> -#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
>  	void __iomem		*autoidle_reg;
>  	void __iomem		*idlest_reg;
>  	u32			autoidle_mask;
> @@ -167,7 +166,6 @@ struct dpll_data {
>  	u8			auto_recal_bit;
>  	u8			recal_en_bit;
>  	u8			recal_st_bit;
> -#  endif
>  	u8			flags;
>  };
>  

Honestly, I wanted to propose this first. 
I am OK, as long as we all agree on this.

Paul, can you conform on this?

Thanks,
Vaibhav

[PATCH-V2 4/4] ARM: OMAP2+: CLEANUP: Add new config option for different DPLL features

[Paul Walmsley]

On Wed, 9 May 2012, Hiremath, Vaibhav wrote:

> On Wed, May 09, 2012 at 04:20:06, Hilman, Kevin wrote:
> 
> > 
> > [1]
> > diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat-omap/include/plat/clock.h
> > index d0ef57c..656b986 100644
> > --- a/arch/arm/plat-omap/include/plat/clock.h
> > +++ b/arch/arm/plat-omap/include/plat/clock.h
> > @@ -156,7 +156,6 @@ struct dpll_data {
> >  	u8			min_divider;
> >  	u16			max_divider;
> >  	u8			modes;
> > -#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
> >  	void __iomem		*autoidle_reg;
> >  	void __iomem		*idlest_reg;
> >  	u32			autoidle_mask;
> > @@ -167,7 +166,6 @@ struct dpll_data {
> >  	u8			auto_recal_bit;
> >  	u8			recal_en_bit;
> >  	u8			recal_st_bit;
> > -#  endif
> >  	u8			flags;
> >  };
> >  
> 
> Honestly, I wanted to propose this first. 
> I am OK, as long as we all agree on this.
> 
> Paul, can you conform on this?

Yes that's a better approach then defining CONFIG_* options for all the 
bitfields, so please go ahead and just send a patch to drop that #ifdef 
and I'll ack it.


- Paul

[PATCH-V2 4/4] ARM: OMAP2+: CLEANUP: Add new config option for different DPLL features

[Hiremath, Vaibhav]

On Fri, May 11, 2012 at 04:05:56, Paul Walmsley wrote:
> On Wed, 9 May 2012, Hiremath, Vaibhav wrote:
> 
> > On Wed, May 09, 2012 at 04:20:06, Hilman, Kevin wrote:
> > 
> > > 
> > > [1]
> > > diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat-omap/include/plat/clock.h
> > > index d0ef57c..656b986 100644
> > > --- a/arch/arm/plat-omap/include/plat/clock.h
> > > +++ b/arch/arm/plat-omap/include/plat/clock.h
> > > @@ -156,7 +156,6 @@ struct dpll_data {
> > >  	u8			min_divider;
> > >  	u16			max_divider;
> > >  	u8			modes;
> > > -#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
> > >  	void __iomem		*autoidle_reg;
> > >  	void __iomem		*idlest_reg;
> > >  	u32			autoidle_mask;
> > > @@ -167,7 +166,6 @@ struct dpll_data {
> > >  	u8			auto_recal_bit;
> > >  	u8			recal_en_bit;
> > >  	u8			recal_st_bit;
> > > -#  endif
> > >  	u8			flags;
> > >  };
> > >  
> > 
> > Honestly, I wanted to propose this first. 
> > I am OK, as long as we all agree on this.
> > 
> > Paul, can you conform on this?
> 
> Yes that's a better approach then defining CONFIG_* options for all the 
> bitfields, so please go ahead and just send a patch to drop that #ifdef 
> and I'll ack it.
> 
> 

Thanks Paul,

I will submit it shortly.

Thanks,
Vaibhav