ARM: OMAP2+: Cleanup series in order to remove ARCH_OMAPx dependency

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 <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Paul Walmsley <[email protected]>

Quoted 7 lines · 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.

Quoted 4 lines

Signed-off-by Vaibhav Hiremath <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Paul Walmsley <[email protected]>

This is probably okay.

Acked-by Paul Walmsley <[email protected]>

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...

Quoted 4 lines · 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).

OK so far.

Quoted 2 lines

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()

Quoted 7 lines · Kevin

Quoted 4 lines · 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).

Quoted 8 lines

OK so far.

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 24 lines · Vaibhav

Quoted 7 lines · Kevin

Quoted 4 lines · 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).

Quoted 8 lines

OK so far.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle

sdrc,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

omap2_set_globals_sdrc(). But inside this function, we are already

checking

Quoted 1 line

Quoted 1 line

whether the omap2_globals->sdrc and omap2_globals->sms for NULL and

then use

Quoted 3 lines

Quoted 3 lines

it.

And function omap2_sdrc_init() is also depends on machine, so in

case of

Quoted 1 line

Quoted 1 line

Am33xx, it won't get into sdrc execution at all. And in case of

omap4, it

Quoted 5 lines

Quoted 1 line

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

Quoted 2 lines

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?

Vaibhav,

Quoted 40 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle

sdrc,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

omap2_set_globals_sdrc(). But inside this function, we are already

checking

Quoted 1 line

Quoted 1 line

whether the omap2_globals->sdrc and omap2_globals->sms for NULL and

then use

Quoted 3 lines

Quoted 3 lines

it.

And function omap2_sdrc_init() is also depends on machine, so in

case of

Quoted 1 line

Quoted 1 line

Am33xx, it won't get into sdrc execution at all. And in case of

omap4, it

Quoted 5 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 4 lines

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 ?

Quoted 32 lines · Kevin

Quoted 19 lines · Vaibhav

Quoted 7 lines · Kevin

Quoted 4 lines · 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).

Quoted 8 lines

OK so far.

Quoted 2 lines

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.

Quoted 8 lines

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?

Quoted 3 lines

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.

Quoted 37 lines · Vaibhav

Quoted 35 lines · Kevin

Quoted 19 lines · Vaibhav

Quoted 7 lines · Kevin

Quoted 4 lines · 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).

Quoted 8 lines

OK so far.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

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 <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Paul Walmsley <[email protected]>

Quoted 10 lines · 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

Quoted 4 lines

Signed-off-by Vaibhav Hiremath <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Paul Walmsley <[email protected]>

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.

[1]

arch/arm/plat-omap/include/plat/clock.h +0 -1

index d0ef57c..656b986 100644
@@ -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;

Quoted 15 lines · Kevin

[1]

arch/arm/plat-omap/include/plat/clock.h +0 -1

index d0ef57c..656b986 100644
@@ -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;

arch/arm/plat-omap/include/plat/clock.h struct dpll_data { · line 171

Quoted 7 lines

arch/arm/plat-omap/include/plat/clock.h +0 -1

@@ -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?

Quoted 28 lines · Vaibhav

Quoted 15 lines · Kevin

[1]

arch/arm/plat-omap/include/plat/clock.h +0 -1

index d0ef57c..656b986 100644
@@ -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;

arch/arm/plat-omap/include/plat/clock.h struct dpll_data { · line 171

Quoted 7 lines

arch/arm/plat-omap/include/plat/clock.h +0 -1

@@ -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.

Quoted 1 line

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.