[PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Eduardo Valentin]

From: Eduardo Valentin <eduardo.valentin@nokia.com>

This patch removes the compilation error when compiling
kernel with CONFIG_PM=N. The problem was that omap3_pm_init_opp_table
was not defined if CONFIG_PM=N.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 arch/arm/mach-omap2/pm.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index d257225..5fc056f 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -69,7 +69,13 @@ static inline void omap3_pm_init_vc(struct prm_setup_vc *setup_vc)
  * Initialize the basic opp table here, board files could choose to modify opp
  * table after the basic initialization
  */
+#ifdef CONFIG_PM
 extern void omap3_pm_init_opp_table(void);
+#else
+static inline void omap3_pm_init_opp_table(void)
+{
+}
+#endif
 
 extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
 extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
-- 
1.6.5.7.g9ecb2

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Nishanth Menon]

Eduardo Valentin said the following on 01/19/2010 05:57 AM:
> From: Eduardo Valentin <eduardo.valentin@nokia.com>
>
> This patch removes the compilation error when compiling
> kernel with CONFIG_PM=N. The problem was that omap3_pm_init_opp_table
> was not defined if CONFIG_PM=N.
>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  arch/arm/mach-omap2/pm.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index d257225..5fc056f 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -69,7 +69,13 @@ static inline void omap3_pm_init_vc(struct prm_setup_vc *setup_vc)
>   * Initialize the basic opp table here, board files could choose to modify opp
>   * table after the basic initialization
>   */
> +#ifdef CONFIG_PM
>  extern void omap3_pm_init_opp_table(void);
> +#else
> +static inline void omap3_pm_init_opp_table(void)
> +{
> +}
> +#endif
>  
>  extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
>  extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
>   
Err... NAK.. I think you missed 
http://marc.info/?t=126356119700001&r=1&w=2 ?
there seems to be an issue else where, I have not dug at it yet..
Regards,
Nishanth Menon

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Eduardo Valentin]

On Tue, Jan 19, 2010 at 01:01:32PM +0100, ext Nishanth Menon wrote:
> Eduardo Valentin said the following on 01/19/2010 05:57 AM:
> > From: Eduardo Valentin <eduardo.valentin@nokia.com>
> >
> > This patch removes the compilation error when compiling
> > kernel with CONFIG_PM=N. The problem was that omap3_pm_init_opp_table
> > was not defined if CONFIG_PM=N.
> >
> > Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> > ---
> >  arch/arm/mach-omap2/pm.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> > index d257225..5fc056f 100644
> > --- a/arch/arm/mach-omap2/pm.h
> > +++ b/arch/arm/mach-omap2/pm.h
> > @@ -69,7 +69,13 @@ static inline void omap3_pm_init_vc(struct prm_setup_vc *setup_vc)
> >   * Initialize the basic opp table here, board files could choose to modify opp
> >   * table after the basic initialization
> >   */
> > +#ifdef CONFIG_PM
> >  extern void omap3_pm_init_opp_table(void);
> > +#else
> > +static inline void omap3_pm_init_opp_table(void)
> > +{
> > +}
> > +#endif
> >  
> >  extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
> >  extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
> >   
> Err... NAK.. I think you missed 
> http://marc.info/?t=126356119700001&r=1&w=2 ?
> there seems to be an issue else where, I have not dug at it yet..


Yeah. OK, I couldn't see the logs as the dumps has been removed already from that thread.
But if I got the problem correctly, the problem is when CONFIG_PM is not set but cpu freq is.
And if there is any call to new omap opp layer helper functions, then it will BUG the system.
Causing hangs.

I guess one way to solve this is to bind compilation of omap opp layer to CONFIG_PM and CONFIG_CPU_FREQ.
If either is disabled, then omap opp layer must be nops.

What do you think?

I am sending a patch to do the above.

> Regards,
> Nishanth Menon

-- 
Eduardo Valentin

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Romit Dasgupta]

>>>   
>> Err... NAK.. I think you missed 
>> http://marc.info/?t=126356119700001&r=1&w=2 ?
>> there seems to be an issue else where, I have not dug at it yet..
> 
> 
> Yeah. OK, I couldn't see the logs as the dumps has been removed already from that thread.
> But if I got the problem correctly, the problem is when CONFIG_PM is not set but cpu freq is.
> And if there is any call to new omap opp layer helper functions, then it will BUG the system.
> Causing hangs.
> 
> I guess one way to solve this is to bind compilation of omap opp layer to CONFIG_PM and CONFIG_CPU_FREQ.
> If either is disabled, then omap opp layer must be nops.
> 
> What do you think?
> 
> I am sending a patch to do the above.
No. That is incorrect. CONFIG_CPU_FREQ, CONFIG_CPU_IDLE and CONFIG_PM are
independent. None of the features should be dependent on the other two!
-Romit

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Nishanth Menon]

Romit Dasgupta had written, on 01/19/2010 08:09 AM, the following:
>>>>   
>>> Err... NAK.. I think you missed 
>>> http://marc.info/?t=126356119700001&r=1&w=2 ?
>>> there seems to be an issue else where, I have not dug at it yet..
>>
>> Yeah. OK, I couldn't see the logs as the dumps has been removed already from that thread.
>> But if I got the problem correctly, the problem is when CONFIG_PM is not set but cpu freq is.
>> And if there is any call to new omap opp layer helper functions, then it will BUG the system.
>> Causing hangs.
>>
>> I guess one way to solve this is to bind compilation of omap opp layer to CONFIG_PM and CONFIG_CPU_FREQ.
>> If either is disabled, then omap opp layer must be nops.
>>
>> What do you think?
>>
>> I am sending a patch to do the above.
> No. That is incorrect. CONFIG_CPU_FREQ, CONFIG_CPU_IDLE and CONFIG_PM are
> independent. None of the features should be dependent on the other two!
> -Romit
OPP layer is required by CPU_FREQ & CONFIG_PM, not CPU_IDLE.

if we modify Eduardo's patch from:

if defined(CONFIG_PM) && defined(CONFIG_CPU_FREQ)
To

if defined(CONFIG_PM) || defined(CONFIG_CPU_FREQ)

wont that ensure the independence is maintained for OPP layer? then, 
probably pm34xx.c maynot be the right place for opp registration for 
3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Romit Dasgupta]

Menon, Nishanth wrote:
> Romit Dasgupta had written, on 01/19/2010 08:09 AM, the following:
>>>>>   
>>>> Err... NAK.. I think you missed 
>>>> http://marc.info/?t=126356119700001&r=1&w=2 ?
>>>> there seems to be an issue else where, I have not dug at it yet..
>>> Yeah. OK, I couldn't see the logs as the dumps has been removed already from that thread.
>>> But if I got the problem correctly, the problem is when CONFIG_PM is not set but cpu freq is.
>>> And if there is any call to new omap opp layer helper functions, then it will BUG the system.
>>> Causing hangs.
>>>
>>> I guess one way to solve this is to bind compilation of omap opp layer to CONFIG_PM and CONFIG_CPU_FREQ.
>>> If either is disabled, then omap opp layer must be nops.
>>>
>>> What do you think?
>>>
>>> I am sending a patch to do the above.
>> No. That is incorrect. CONFIG_CPU_FREQ, CONFIG_CPU_IDLE and CONFIG_PM are
>> independent. None of the features should be dependent on the other two!
>> -Romit
> OPP layer is required by CPU_FREQ & CONFIG_PM, not CPU_IDLE.
> 
> if we modify Eduardo's patch from:
> 
> if defined(CONFIG_PM) && defined(CONFIG_CPU_FREQ)
> To
> 
> if defined(CONFIG_PM) || defined(CONFIG_CPU_FREQ)
> 
> wont that ensure the independence is maintained for OPP layer? then, 
> probably pm34xx.c maynot be the right place for opp registration for 
> 3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..
> 
It should be only
#ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.

Why do you need CPU_FREQ for suspend/resume??

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Nishanth Menon]

Dasgupta, Romit had written, on 01/19/2010 08:41 AM, the following:
> Menon, Nishanth wrote:
>> Romit Dasgupta had written, on 01/19/2010 08:09 AM, the following:
>>>>>>   
>>>>> Err... NAK.. I think you missed 
>>>>> http://marc.info/?t=126356119700001&r=1&w=2 ?
>>>>> there seems to be an issue else where, I have not dug at it yet..
>>>> Yeah. OK, I couldn't see the logs as the dumps has been removed already from that thread.
>>>> But if I got the problem correctly, the problem is when CONFIG_PM is not set but cpu freq is.
>>>> And if there is any call to new omap opp layer helper functions, then it will BUG the system.
>>>> Causing hangs.
>>>>
>>>> I guess one way to solve this is to bind compilation of omap opp layer to CONFIG_PM and CONFIG_CPU_FREQ.
>>>> If either is disabled, then omap opp layer must be nops.
>>>>
>>>> What do you think?
>>>>
>>>> I am sending a patch to do the above.
>>> No. That is incorrect. CONFIG_CPU_FREQ, CONFIG_CPU_IDLE and CONFIG_PM are
>>> independent. None of the features should be dependent on the other two!
>>> -Romit
>> OPP layer is required by CPU_FREQ & CONFIG_PM, not CPU_IDLE.
>>
>> if we modify Eduardo's patch from:
>>
>> if defined(CONFIG_PM) && defined(CONFIG_CPU_FREQ)
>> To
>>
>> if defined(CONFIG_PM) || defined(CONFIG_CPU_FREQ)
>>
>> wont that ensure the independence is maintained for OPP layer? then, 
>> probably pm34xx.c maynot be the right place for opp registration for 
>> 3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..
>>
> It should be only
> #ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.
> 
> Why do you need CPU_FREQ for suspend/resume??
> 
voltage control - SR needs to query for voltage?

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Romit Dasgupta]

>> 3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..
>>>
>> It should be only
>> #ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.
>>
>> Why do you need CPU_FREQ for suspend/resume??
>>
> voltage control - SR needs to query for voltage?
> 
Why should suspend/resume be dependent on SR?

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Nishanth Menon]

Dasgupta, Romit had written, on 01/19/2010 08:43 AM, the following:
>>> 3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..
>>> It should be only
>>> #ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.
>>>
>>> Why do you need CPU_FREQ for suspend/resume??
>>>
>> voltage control - SR needs to query for voltage?
>>
> Why should suspend/resume be dependent on SR?
please see the code logic -> when SR is enabled and OFF/RET happens, you 
need the voltage for the current frequency so that you can disable SR, 
set the nominal voltage for the current OPP then go to WFI.

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Romit Dasgupta]

Menon, Nishanth wrote:
> Dasgupta, Romit had written, on 01/19/2010 08:43 AM, the following:
>>>> 3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..
>>>> It should be only
>>>> #ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.
>>>>
>>>> Why do you need CPU_FREQ for suspend/resume??
>>>>
>>> voltage control - SR needs to query for voltage?
>>>
>> Why should suspend/resume be dependent on SR?
> please see the code logic -> when SR is enabled and OFF/RET happens, you 
> need the voltage for the current frequency so that you can disable SR, 
> set the nominal voltage for the current OPP then go to WFI.
> 
You do not need the OPP table for querying voltage alone. You can read that from
the OMAP chip registers directly. So OPP layer is not necessary when we do not
use cpufreq!

Re: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Nishanth Menon]

Dasgupta, Romit had written, on 01/19/2010 08:50 AM, the following:
> Menon, Nishanth wrote:
>> Dasgupta, Romit had written, on 01/19/2010 08:43 AM, the following:
>>>>> 3430 opps, and we should move it to opp34xx.c(I hate having new files :( )..
>>>>> It should be only
>>>>> #ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.
>>>>>
>>>>> Why do you need CPU_FREQ for suspend/resume??
>>>>>
>>>> voltage control - SR needs to query for voltage?
>>>>
>>> Why should suspend/resume be dependent on SR?
>> please see the code logic -> when SR is enabled and OFF/RET happens, you 
>> need the voltage for the current frequency so that you can disable SR, 
>> set the nominal voltage for the current OPP then go to WFI.
>>
> You do not need the OPP table for querying voltage alone. You can read that from
> the OMAP chip registers directly. So OPP layer is not necessary when we do not
> use cpufreq!
there are multiple ways to use SR -> Not all of them can use OMAP chip 
registers. certain class of devices would need to use PMICs, further SRF 
or it's equivalent will need to query the table for knowing what voltage 
corresponds to frequency, DSPBRidge needs to report to the baseimage 
what voltage corresponds to the frequency for the baseimage's load 
balancing/power decision logic etc.. in short when dynamic voltage 
scaling is needed, we need OPP layer a.k.a CONFIG_PM needs opp layer - 
not just cpufreq.

-- 
Regards,
Nishanth Menon

RE: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Dasgupta, Romit]

________________________________________

>there are multiple ways to use SR -> Not all of them can use OMAP chip
>registers. certain class of devices would need to use PMICs,
Even then you do not need OPP layer. For PMIC based solutions you need
to use I2C to get the voltage. So this should be abstracted. OPP layer is just for
CPU FREQ.
> further SRF
>or it's equivalent will need to query the table for knowing what voltage
>corresponds to frequency, 
Why should you know the voltage for a frequency other than your current
frequency when you do not scale the frequency of your CPU? Clock framework
will give you the current freq. For voltage, we already discussed above.


>DSPBRidge needs to report to the baseimage
>what voltage corresponds to the frequency for the baseimage's load
>balancing/power decision logic etc.. in short when dynamic voltage
>scaling is needed, we need OPP layer a.k.a CONFIG_PM needs opp layer -
>not just cpufreq.
Please understand CONFIG_PM is not for any kind of __cpufreq or dspfreq__.
It is for suspend resume and few other stuff. So why should CONFIG_PM need
OPP layer?



--
Regards,
Nishanth Menon

RE: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Cousson, Benoit]

Hi Romit,

>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>
>Menon, Nishanth wrote:
>> Dasgupta, Romit had written, on 01/19/2010 08:43 AM, the following:
>>>>> 3430 opps, and we should move it to opp34xx.c(I hate having new
>files :( )..
>>>>> It should be only
>>>>> #ifdef CONFIG_CPU_FREQ. OPP has nothing to do with CONFIG_PM.
>>>>>
>>>>> Why do you need CPU_FREQ for suspend/resume??
>>>>>
>>>> voltage control - SR needs to query for voltage?
>>>>
>>> Why should suspend/resume be dependent on SR?
>> please see the code logic -> when SR is enabled and OFF/RET happens, you
>> need the voltage for the current frequency so that you can disable SR,
>> set the nominal voltage for the current OPP then go to WFI.
>>
>You do not need the OPP table for querying voltage alone. You can read that
>from the OMAP chip registers directly. So OPP layer is not necessary when
>we do not use cpufreq!

I do agree that we should not need CPUfreq, but cannot use the voltage from the VP either; you need to set the nominal voltage to be safe not the one after SR correction.
Eventually it will have to be handled by the SR layer.

Regards,
Benoit

Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

RE: [PATCH 1/1] OMAP3: PM: Fix compilation issue of omap3_pm_init_opp_table

[Dasgupta, Romit]

Hello Benoit,
>>You do not need the OPP table for querying voltage alone. You can read that
>>from the OMAP chip registers directly. So OPP layer is not necessary when
>>we do not use cpufreq!

>I do agree that we should not need CPUfreq, but cannot use the voltage from the VP either; you need to set >the nominal voltage to be safe not the one after SR correction.
>Eventually it will have to be handled by the SR layer.
Yes it can be handled by SR layer but this does not need one to query OPP. All you need is to know current freq and voltage. This can and should be done without taking help of OPP layer (when CONFIG_CPU_FREQ is absent).

Thanks,
-Romit