| Message ID | 1361354272-18089-3-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, 20 Feb 2013, Santosh Shilimkar wrote: > From: Rajendra Nayak <rnayak@ti.com> > > _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()? > and unnecessary. So here's part of the reason why the module wakeup control functions should remain separate. When the kernel boots, all the PM features should be disabled. Then mach-omap2/pm*.c should enables PM features where they're needed. Right now, mach-omap2/pm34xx.c sets module WKEN bits. (These direct register accesses need to be moved as part of the cleanup work, of moving the PM/PRM/CM code into drivers.) But the list of IP blocks that should be allowed to wake the system is board-dependent. So really, what mach-omap2/pm34xx.c should do is to call into the hwmod enable-wakeups code to enable MPU wakeups for all the IP blocks that have some DT property set, something like 'enable-wakeups'. Then the hwmod code should ensure that the PRM wakeup-enable and GRPSEL bits are programmed (by calling into the PRM driver code) and should then either set the ENAWAKEUP bits or put the module into smart_wkup MSTANDBY/MIDLE. Similarly, when the PM driver is unloaded, it should set no-idle on all the IP blocks that were marked as wakeup-capable and disable the PRCM wakeup control bits, by calling some hwmod disable-wakeups code. > Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes > correctly. So fix them so they also handle the midle modes correctly If there's a fix here, please split that out into a separate patch. - Paul
Paul, >> _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. regards, Rajendra > >> and unnecessary. > > So here's part of the reason why the module wakeup control functions > should remain separate. When the kernel boots, all the PM features should > be disabled. Then mach-omap2/pm*.c should enables PM features where > they're needed. > > Right now, mach-omap2/pm34xx.c sets module WKEN bits. (These direct > register accesses need to be moved as part of the cleanup work, of moving > the PM/PRM/CM code into drivers.) But the list of IP blocks that > should be allowed to wake the system is board-dependent. > > So really, what mach-omap2/pm34xx.c should do is to call into the hwmod > enable-wakeups code to enable MPU wakeups for all the IP blocks that have > some DT property set, something like 'enable-wakeups'. Then the hwmod > code should ensure that the PRM wakeup-enable and GRPSEL bits are > programmed (by calling into the PRM driver code) and should then either > set the ENAWAKEUP bits or put the module into smart_wkup MSTANDBY/MIDLE. > > Similarly, when the PM driver is unloaded, it should set no-idle on all > the IP blocks that were marked as wakeup-capable and disable the PRCM > wakeup control bits, by calling some hwmod disable-wakeups code. > >> Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes >> correctly. So fix them so they also handle the midle modes correctly > > If there's a fix here, please split that out into a separate patch. > > > - Paul >
Hi Paul, >>> _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? regards, Rajendra > > regards, > Rajendra > >> >>> and unnecessary. >> >> So here's part of the reason why the module wakeup control functions >> should remain separate. When the kernel boots, all the PM features should >> be disabled. Then mach-omap2/pm*.c should enables PM features where >> they're needed. >> >> Right now, mach-omap2/pm34xx.c sets module WKEN bits. (These direct >> register accesses need to be moved as part of the cleanup work, of moving >> the PM/PRM/CM code into drivers.) But the list of IP blocks that >> should be allowed to wake the system is board-dependent. >> >> So really, what mach-omap2/pm34xx.c should do is to call into the hwmod >> enable-wakeups code to enable MPU wakeups for all the IP blocks that have >> some DT property set, something like 'enable-wakeups'. Then the hwmod >> code should ensure that the PRM wakeup-enable and GRPSEL bits are >> programmed (by calling into the PRM driver code) and should then either >> set the ENAWAKEUP bits or put the module into smart_wkup MSTANDBY/MIDLE. >> >> Similarly, when the PM driver is unloaded, it should set no-idle on all >> the IP blocks that were marked as wakeup-capable and disable the PRCM >> wakeup control bits, by calling some hwmod disable-wakeups code. >> >>> Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes >>> correctly. So fix them so they also handle the midle modes correctly >> >> If there's a fix here, please split that out into a separate patch. >> >> >> - Paul >> >
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. - Paul
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 >
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index b45c8fb..fdd55cd 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -588,9 +588,7 @@ static void _set_idle_ioring_wakeup(struct omap_hwmod *oh, bool set_wake) static int _enable_wakeup(struct omap_hwmod *oh, u32 *v) { if (!oh->class->sysc || - !((oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP) || - (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) || - (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP))) + !(oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) return -EINVAL; if (!oh->class->sysc->sysc_fields) { @@ -601,11 +599,6 @@ static int _enable_wakeup(struct omap_hwmod *oh, u32 *v) if (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP) *v |= 0x1 << oh->class->sysc->sysc_fields->enwkup_shift; - if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) - _set_slave_idlemode(oh, HWMOD_IDLEMODE_SMART_WKUP, v); - if (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP) - _set_master_standbymode(oh, HWMOD_IDLEMODE_SMART_WKUP, v); - /* XXX test pwrdm_get_wken for this hwmod's subsystem */ return 0; @@ -621,9 +614,7 @@ static int _enable_wakeup(struct omap_hwmod *oh, u32 *v) static int _disable_wakeup(struct omap_hwmod *oh, u32 *v) { if (!oh->class->sysc || - !((oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP) || - (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) || - (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP))) + !(oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) return -EINVAL; if (!oh->class->sysc->sysc_fields) { @@ -634,11 +625,6 @@ static int _disable_wakeup(struct omap_hwmod *oh, u32 *v) if (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP) *v &= ~(0x1 << oh->class->sysc->sysc_fields->enwkup_shift); - if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) - _set_slave_idlemode(oh, HWMOD_IDLEMODE_SMART, v); - if (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP) - _set_master_standbymode(oh, HWMOD_IDLEMODE_SMART, v); - /* XXX test pwrdm_get_wken for this hwmod's subsystem */ return 0; @@ -1351,13 +1337,24 @@ static void _enable_sysc(struct omap_hwmod *oh) clkdm = _get_clkdm(oh); if (sf & SYSC_HAS_SIDLEMODE) { + if (oh->flags & HWMOD_SWSUP_SIDLE) { + idlemode = HWMOD_IDLEMODE_NO; + } else { + if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) + idlemode = HWMOD_IDLEMODE_SMART_WKUP; + else + idlemode = HWMOD_IDLEMODE_SMART; + } + + /* + * This is special handling for some IPs like + * 32k sync timer. Force them to idle! + */ clkdm_act = (clkdm && clkdm->flags & CLKDM_ACTIVE_WITH_MPU); if (clkdm_act && !(oh->class->sysc->idlemodes & (SIDLE_SMART | SIDLE_SMART_WKUP))) idlemode = HWMOD_IDLEMODE_FORCE; - else - idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ? - HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART; + _set_slave_idlemode(oh, idlemode, &v); } @@ -1423,13 +1420,14 @@ static void _idle_sysc(struct omap_hwmod *oh) sf = oh->class->sysc->sysc_flags; if (sf & SYSC_HAS_SIDLEMODE) { - /* XXX What about HWMOD_IDLEMODE_SMART_WKUP? */ - if (oh->flags & HWMOD_SWSUP_SIDLE || - !(oh->class->sysc->idlemodes & - (SIDLE_SMART | SIDLE_SMART_WKUP))) + if (oh->flags & HWMOD_SWSUP_SIDLE) { idlemode = HWMOD_IDLEMODE_FORCE; - else - idlemode = HWMOD_IDLEMODE_SMART; + } else { + if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) + idlemode = HWMOD_IDLEMODE_SMART_WKUP; + else + idlemode = HWMOD_IDLEMODE_SMART; + } _set_slave_idlemode(oh, idlemode, &v); }