RE: RFC: hwmod, iclks, auto-idle and autodeps

2010-12-09 Thread Paul Walmsley
Hello Partha

On Tue, 23 Nov 2010, Basak, Partha wrote:

  -Original Message-
  From: Paul Walmsley [mailto:p...@pwsan.com]
  Sent: Tuesday, November 23, 2010 9:31 AM
  
  The rest of the time, if the GPIO driver wants to use idle-mode 
  wakeups (presumably higher wakeup latency, but lower power 
  consumption), it should just clk_disable() its clocks, but not call 
  pm_runtime_put*().  After
 
 This would require that clk_disable() be called directly by the driver
 outside of pm_runtime_putsync(). 

Yes.

 Isn't this not in line with runtime PM paradigm?

Not really sure what you mean.  If you're asking whether I'd prefer to 
have this hidden behind some other API, the answer is probably yes.  In 
the medium- to long-term, I do think we should try to figure out a 
coherent way to handle this case without calling 
clk_enable()/clk_disable() directly from the driver.  Maybe by adding some 
omap_device API calls to allow the driver to tell the 
omap_device/omap_hwmod code what level of 'idle' to enter.

But to get there, we have to understand the problem first that we're 
trying to solve.  I'm not sure we (at least I) fully understand that now.  
To the extent our current device drivers rely on asynchronous wakeup or 
autonomous operation, those parts of the drivers are definitely not 
documented.  GPIO seems to tacitly rely on it.  Maybe UART.  (Essentially 
any device that has special hacks in the pm34xx.c idle loop for 
enabling/disabling it.)  And McBSP also has some tricky twists here.  I 
suspect that there are lots of hidden dependencies, like assuming 
interface clock autoidle, or certain powerdomain sleep dependencies, etc.

  If idle-mode wakeup is used, then the PRCM ISR code that notes the 
  GPIO wakeup event either needs to enable the GPIO main clock before 
  calling its ISR, or the GPIO ISR needs to enable its main clock first 
  thing.  If active-mode interrupts are used, then the GPIO ISR doesn't 
  need to enable its main clock since it is already running at that 
  point.
 
 Enabling main clock via pm_runtime_getsync() in interrupt context will 
 be a problem.  Can we use pm_runtime_get() instead? This will lead to
 some delay in the interrupts getting processed.

If the GPIO interface/functional clock is off, and the GPIO debounce clock
is off, then according to the TRM, the GPIO block will never assert an 
interrupt, so we don't need to worry about the ISR in that case.  This 
state is entered after the last pm_runtime_put_sync() on the GPIO block.

If however, the GPIO driver wants to support idle-mode wakeups, then the 
GPIO debounce clock needs to be running.  pm_runtime_put_sync() cannot 
currently be called in this situation - instead, the driver has to call 
clk_disable() on its ifclk -- or rely on interface clock autoidle -- if it 
wants the GPIO IP block to be able to wake the system from idle mode.  In 
this case, the GPIO ISR _can_ successfully call clk_enable() on the GPIO 
IFCLK, and then clk_disable() once it is done accessing the GPIO block 
registers.  The GPIO ISR should not use pm_runtime_get*() right now, if 
for no other reason than because the driver did not previously call 
pm_runtime_put*().

...

So for the time being, I'd suggest calling clk_enable() on the GPIO ifclk 
as the first thing in the GPIO ISR, and ensuring that clk_disable() is 
called on the ifclk before exiting the ISR.  That should be safe for both 
the idle-mode wakeup and active-mode interrupt GPIO use cases, and should 
be safe for both interface clock auto-idle and interface clock manual 
idle.  And since the last pm_runtime_put*() on the device will prevent the 
GPIO from sending any kind of wakeup or interrupt (since all GPIO clocks 
will be off), pm_runtime_put*() should only be called when the GPIO block 
is not in use and not expected to generate wakeups nor interrupts.

Does this sound reasonable?


- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: RFC: hwmod, iclks, auto-idle and autodeps

2010-11-23 Thread Basak, Partha


 -Original Message-
 From: Paul Walmsley [mailto:p...@pwsan.com]
 Sent: Tuesday, November 23, 2010 9:31 AM
 To: Kevin Hilman
 Cc: Cousson, Benoit; Basak, Partha; Varadarajan, Charulatha; linux-
 o...@vger.kernel.org
 Subject: Re: RFC: hwmod, iclks, auto-idle and autodeps
 
 Hi
 
 Kevin and I discussed this privately, this is just to summarize for
 everyone else.
 
 On Wed, 17 Nov 2010, Kevin Hilman wrote:
 
  There have been a few discussions over the few months about how to
  properly use omap_hwmod to manage certain IPs that have the interface
  clock as the functional clock (e.g. OMAP3 GPIOs.)  The goal of this
 RFC
  is to come to a conclusion about what should be done, and how to go
  about implementing it.
 
  In the initial conversion of the GPIO core to omap_hwmod, main_clk
 was
  left NULL so that hwmod was not managing the clock on every hwmod
  enable/disable.
 
 Since GPIO has a register target, main_clk cannot be null.  There's an
 erroneous comment in plat/omap_hwmod.h about this; I'll add it to my
 list
 of things to fix.
 
  This behavior matched current mainline GPIO code, which does not
  dynamically disable/enable GPIO iclks after init time. Instead, it
  relies on the module-level auto idle feature in HW.
 
  However, without dynamically managing the clock in hwmod, this meant
  that there were no autodeps tracked for GPIO and thus the PER
  powerdomain could transition independently of MPU/CORE.
 
 The current GPIO driver works only because it exploits some incidental
 properties of the OMAP core code.  It implicitly relies on CM_AUTOIDLE
 = 1
 on its iclk for power management to work, since the driver never
 disables
 its iclk.  The driver also relies on the addition of MPU/IVA2 autodeps
 to
 avoid losing logic context once all devices in PER go idle.  And by
 never
 explicitly disabling its iclk, the driver avoids dealing with the
 various
 GPIO wakeup/interrupt modes, causing its context save/restore to be
 implemented as a weird hack in pm34xx.c.
 
 That said, there definitely is one real limitation/bug in the OMAP PM
 core
 code that the GPIO driver has to work around.  The current core code
 doesn't try to keep a powerdomain powered when an IP block inside the
 powerdomain is still considered to be in use but all its system-sourced
 clocks are cut.  This can result in unexpected context loss and
 malfunction in some GPIO and McBSP cases, possibly some other modules
 also
 that can be externally clocked and contain FIFOs/buffers, or that can
 generate asynchronous wakeups.  There's a patch in the works here that
 will require a powerdomain to stay on as long as a hwmod in that
 powerdomain is enabled.  Once omap_hwmod_idle() is called for that
 hwmod,
 the lower-bound on the power state of the powerdomain is removed.
 
 So in the context of the PM runtime conversion of the GPIO driver, the
 thing to do is to only do a pm_runtime_put*() once the GPIO module is
 really ready to be powered down.  After that call, the GPIO block may
 lose
 its logic context, and it may not be able to generate interrupts or
 wakeups.  We may enforce the latter in the hwmod code for debugging
 purposes by forcing the module into idle and instructing the PRCM to
 ignore wakeups from the module in omap_hwmod_idle().  IO ring/chain
 wakeups may still occur, but these are independent of the GPIO block.
 
 The rest of the time, if the GPIO driver wants to use idle-mode wakeups
 (presumably higher wakeup latency, but lower power consumption), it
 should
 just clk_disable() its clocks, but not call pm_runtime_put*().  After

This would require that clk_disable() be called directly by the driver
outside of pm_runtime_putsync(). 
Isn't this not in line with runtime PM paradigm?

 the
 previously-mentioned improvement to the powerdomain/hwmod code is
 completed and applied, that should result in the powerdomain staying
 powered, but all clocks being disabled.  Otherwise, if the GPIO driver
 wants to use active-mode interrupts (presumably lower wakeup latency,
 but
 higher power consumption), then the driver should just leave its clocks
 enabled and never call pm_runtime_put*().

So, if I understood correctly, it is up to the driver to take the call.
It is perfectly OK if the driver decides to do a pm_runtime_get_sync()
when a GPIO module is requested and not call pm_runtime_put_sync()
until it is released. Now, in OMAP, because of the incidental availability
of AutoIdle feature, this will not prevent the clocks getting auto-gated.
 
 Both of these latter modes will block some low-power states.  At some
 point, the selection of the interrupt/wakeup mode -- GPIO active, GPIO
 idle, IO ring/chain -- should be made based on the required wakeup
 latency
 of the GPIO user.  Multiple modes may need to used simultaneously,
 since
 individual modes are restricted to certain power states (e.g. IO ring
 is
 only available in CORE off, IO chain is only available in CORE/PER
 retention)
 
  The initial solution

Re: RFC: hwmod, iclks, auto-idle and autodeps

2010-11-22 Thread Paul Walmsley
Hi

Kevin and I discussed this privately, this is just to summarize for 
everyone else.

On Wed, 17 Nov 2010, Kevin Hilman wrote:

 There have been a few discussions over the few months about how to
 properly use omap_hwmod to manage certain IPs that have the interface
 clock as the functional clock (e.g. OMAP3 GPIOs.)  The goal of this RFC
 is to come to a conclusion about what should be done, and how to go
 about implementing it.
 
 In the initial conversion of the GPIO core to omap_hwmod, main_clk was 
 left NULL so that hwmod was not managing the clock on every hwmod 
 enable/disable.

Since GPIO has a register target, main_clk cannot be null.  There's an 
erroneous comment in plat/omap_hwmod.h about this; I'll add it to my list 
of things to fix.

 This behavior matched current mainline GPIO code, which does not 
 dynamically disable/enable GPIO iclks after init time. Instead, it 
 relies on the module-level auto idle feature in HW.
 
 However, without dynamically managing the clock in hwmod, this meant
 that there were no autodeps tracked for GPIO and thus the PER
 powerdomain could transition independently of MPU/CORE.

The current GPIO driver works only because it exploits some incidental 
properties of the OMAP core code.  It implicitly relies on CM_AUTOIDLE = 1 
on its iclk for power management to work, since the driver never disables 
its iclk.  The driver also relies on the addition of MPU/IVA2 autodeps to 
avoid losing logic context once all devices in PER go idle.  And by never 
explicitly disabling its iclk, the driver avoids dealing with the various 
GPIO wakeup/interrupt modes, causing its context save/restore to be 
implemented as a weird hack in pm34xx.c.

That said, there definitely is one real limitation/bug in the OMAP PM core 
code that the GPIO driver has to work around.  The current core code 
doesn't try to keep a powerdomain powered when an IP block inside the 
powerdomain is still considered to be in use but all its system-sourced 
clocks are cut.  This can result in unexpected context loss and 
malfunction in some GPIO and McBSP cases, possibly some other modules also 
that can be externally clocked and contain FIFOs/buffers, or that can 
generate asynchronous wakeups.  There's a patch in the works here that 
will require a powerdomain to stay on as long as a hwmod in that 
powerdomain is enabled.  Once omap_hwmod_idle() is called for that hwmod, 
the lower-bound on the power state of the powerdomain is removed.

So in the context of the PM runtime conversion of the GPIO driver, the 
thing to do is to only do a pm_runtime_put*() once the GPIO module is 
really ready to be powered down.  After that call, the GPIO block may lose 
its logic context, and it may not be able to generate interrupts or 
wakeups.  We may enforce the latter in the hwmod code for debugging 
purposes by forcing the module into idle and instructing the PRCM to 
ignore wakeups from the module in omap_hwmod_idle().  IO ring/chain 
wakeups may still occur, but these are independent of the GPIO block.

The rest of the time, if the GPIO driver wants to use idle-mode wakeups 
(presumably higher wakeup latency, but lower power consumption), it should 
just clk_disable() its clocks, but not call pm_runtime_put*().  After the 
previously-mentioned improvement to the powerdomain/hwmod code is 
completed and applied, that should result in the powerdomain staying 
powered, but all clocks being disabled.  Otherwise, if the GPIO driver
wants to use active-mode interrupts (presumably lower wakeup latency, but 
higher power consumption), then the driver should just leave its clocks 
enabled and never call pm_runtime_put*().

Both of these latter modes will block some low-power states.  At some 
point, the selection of the interrupt/wakeup mode -- GPIO active, GPIO 
idle, IO ring/chain -- should be made based on the required wakeup latency 
of the GPIO user.  Multiple modes may need to used simultaneously, since 
individual modes are restricted to certain power states (e.g. IO ring is 
only available in CORE off, IO chain is only available in CORE/PER 
retention)

 The initial solution to this was to set the iclk to be the main_clk of 
 the hwmod, such that autodeps were managed dynamically as well.  This 
 led to a change in functionality in current code, since the iclk was now 
 being manually enabled/disabled at runtime instead of being left for HW 
 to manage by module autoidle.  It also led to problems in correctly 
 handling asynchronous GPIO wakeups.

If idle-mode wakeup is used, then the PRCM ISR code that notes the GPIO 
wakeup event either needs to enable the GPIO main clock before calling its 
ISR, or the GPIO ISR needs to enable its main clock first thing.  If 
active-mode interrupts are used, then the GPIO ISR doesn't need to enable 
its main clock since it is already running at that point.

 Alternatively, would it make sense to do something different with
 autodeps, such that modules like this that don't