Re: [PATCH 2/8] ARM: OMAP2+: hwmod: Cleanup sidle/mstandby programming

[Rajendra Nayak <rnayak@ti.com>]

On Tuesday 23 April 2013 01:49 PM, Paul Walmsley wrote:
> Hi Rajendra,
> 
> On Thu, 18 Apr 2013, Rajendra Nayak wrote:
> 
>>>>> _enable_wakeup() and _disable_wakeup() are expected to program the
>>>>> OCP_SYSCONFIG.ENAWAKEUP bit.
>>>>
>>>> These functions were originally intended to take care of everything needed 
>>>> for the IP block to wake up the chip, including the PRCM WKEN programming.  
>>>> ENAWAKEUP is simply one part of that.
>>>>
>>>>> Get rid of the additional sidle/mstandby programming in them, as its 
>>>>> confusing (this is expected to be handled elsewhere as part of 
>>>>> _enable_sysc()/__idle_sysc())
>>>>
>>>> Sorry, why does the expectation exist for the code to enable and disable 
>>>> device wakeup to be part of _enable_sysc()/_idle_sysc(), rather than in 
>>>> functions called by _enable_sysc()/_idle_sysc()?
>>>
>>> It all comes down to if SIDLE_SMART_WKUP/MSTANDBY_SMART_WKUP programming
>>> be considered as 'idlemode' programming or 'enwakeup' programming.
>>> If you consider these are being part of 'enwakeup' progrmming, these should
>>> certainly be handled as part of _enable_wakeup() and _disable_wakeup().
>>>
>>> Today, in some cases, these are *also* handled as part of _enable_sysc()
>>> and _idle_sysc(). The way _enable_wakeup() is invoked from _enable_sysc()
>>> is also very inconsistent. For instance, for any IP which supports
>>> SYSC_HAS_MIDLEMODE and SYSC_HAS_ENAWAKEUP, we invoke _enable_wakeup()
>>> regardless of the MIDLEMODE programmmed.
>>> While in case of the IP supporting SYSC_HAS_SIDLEMODE, _enable_wakeup() is
>>> invoked only when the SIDLEMODE programmed is SMART or SMART_WKUP.
>>>
>>> I understand the cleanups you are suggesting below as part of the movement
>>> of some of these things outside of mach-omap2.
>>> I was more looking at fixing the existing piece so its readable and does
>>> things more consistently.
>>
>> Do you have any further thoughts on how we should do about this?
> 
> Is it possible to implement a solution that preserves _enable_wakeup() and 
> _disable_wakeup() as distinct functions, that can be called by separate 
> wakeup control entry points?
> 
> If it makes sense to change _enable_sysc() so that it doesn't call 
> _enable_wakeup(), but does similar work itself, that's fine with me, as 
> long as there's not too much code duplication.
> 
> It will be good to have the inconsistencies fixed.

Sure, I'll post something on those lines shortly.

regards,
Rajendra
> 
> 
> - Paul
>