Re: [PATCH 2/3] OMAP3: PM: Configure PRM setup times from board files

[Kevin Hilman <khilman@deeprootsystems.com>]

"Nayak, Rajendra" <rnayak@ti.com> writes:

>>
>>[...]
>>
>>> diff --git a/arch/arm/mach-omap2/pm34xx.c 
>>b/arch/arm/mach-omap2/pm34xx.c
>>> index 2242d23..6f2fb51 100644
>>> --- a/arch/arm/mach-omap2/pm34xx.c
>>> +++ b/arch/arm/mach-omap2/pm34xx.c
>>> @@ -1084,6 +1084,9 @@ int omap3_pm_set_suspend_state(struct 
>>powerdomain *pwrdm, int state)
>>>  
>>>  void omap3_set_prm_setup_vc(struct prm_setup_vc *setup_vc)
>>>  {
>>> +	if (!setup_vc)
>>> +		return;
>>> +
>>>  	prm_setup.clksetup = setup_vc->clksetup;
>>>  	prm_setup.voltsetup_time1 = setup_vc->voltsetup_time1;
>>>  	prm_setup.voltsetup_time2 = setup_vc->voltsetup_time2;
>>> @@ -1285,13 +1288,15 @@ static void __init configure_vc(void)
>>>  
>>>  void omap3_pm_early_init(struct omap_opp *mpu_opps,
>>>  			 struct omap_opp *dsp_opps,
>>> -			 struct omap_opp *l3_opps)
>>> +			 struct omap_opp *l3_opps,
>>> +			 struct prm_setup_vc *setup_times)
>>>  {
>>>  	prm_clear_mod_reg_bits(OMAP3430_OFFMODE_POL, OMAP3430_GR_MOD,
>>>  				OMAP3_PRM_POLCTRL_OFFSET);
>>>  
>>>  	configure_vc();
>>>  	omap_pm_if_early_init(mpu_opps, dsp_opps, l3_opps);
>>> +	omap3_set_prm_setup_vc(setup_times);
>>
>>I think there's an ordering problem here since the configure_vc()
>>which uses the values is called before the setup_vc().
>
> Yes, the ordering seems to be certainly wrong. I will fix that in the
> updated patch-set.
>
>>
>>Also, if setup_times is passed as NULL, configure_vc() will be writing
>>wrong values with undefined results to the VC.
>
> I guess not, since it uses default values from the prm_setup table defined
> in pm34xx.c which are not optimal but certainally not wrong. No?
> Same is the case with cpuidle params as well.

Ah, you're right.  I saw the defaults in cpuidle, but not in pm34xx.c.
That should be fine.  Then, having these defaults will make the init
call optional and only needed in boards that want to override.

Thanks,

Kevin