Patchwork [PATCHv8,00/23] I2C big cleanup

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date Sept. 12, 2012, 10:57 a.m.
Message ID <1347447496-16793-1-git-send-email-shubhrajyoti@ti.com>
Download mbox
Permalink /patch/183323/
State New
Headers show

Pull-request

git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups

Comments

Datta, Shubhrajyoti - Sept. 12, 2012, 10:57 a.m.
Changes since v1:
       - removed tabification on patch 6/17
       - removed dev_err() which was introduced on patch 09/17
Changes since v2:
        - do not set full fifo depth in the RDR interrupt.
        - some changelog updates.
        - rebase to the Wolfram's tree.
Changes since v3:
        - Remove a redundant read of status register
        - Read the dev->buf_len variable instead of the register
        as the information of the remaining bytes is there.
Changes since v4:
        - Ack the arbitration lost.
        - Rebase to the i2c-embedded/for-next branch.
Changes since v5:
        - Rebase to latest mainline
        - Added some more cleanup patches so as have a consolidated series.
Changes since v6:
        - Fix comments on setting the pdev to NULL.
        - Trivial changelog update
Changes since v7:
	- Remove a patch as the code is getting changed anyways
	- Changelogs update.

Previous discussions can be found here 
http://www.spinics.net/lists/linux-i2c/msg09764.html

This is the cleanup only series.
  
Tested on omap4sdp and 3430sdp.

The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:

  Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)

are available in the git repository at:
  git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups


Felipe Balbi (22):
  i2c: omap: switch to devm_* API
  i2c: omap: simplify num_bytes handling
  i2c: omap: decrease indentation level on data handling
  i2c: omap: add blank lines
  i2c: omap: simplify omap_i2c_ack_stat()
  i2c: omap: split out [XR]DR and [XR]RDY
  i2c: omap: improve i462 errata handling
  i2c: omap: re-factor receive/transmit data loop
  i2c: omap: switch over to do {} while loop
  i2c: omap: ack IRQ in parts
  i2c: omap: switch to platform_get_irq()
  i2c: omap: bus: add a receiver flag
  i2c: omap: simplify errata check
  i2c: omap: always return IRQ_HANDLED
  i2c: omap: simplify IRQ exit path
  i2c: omap: resize fifos before each message
  i2c: omap: get rid of the "complete" label
  i2c: omap: always return IRQ_HANDLED
  i2c: omap: switch to threaded IRQ support
  i2c: omap: remove unnecessary pm_runtime_suspended check
  i2c: omap: switch over to autosuspend API
  i2c: omap: sanitize exit path

Shubhrajyoti D (1):
  i2c: omap: remove redundant status read

 drivers/i2c/busses/i2c-omap.c |  442 +++++++++++++++++++++++++----------------
 1 files changed, 271 insertions(+), 171 deletions(-)
Wolfram Sang - Sept. 12, 2012, 1:16 p.m.
On Wed, Sep 12, 2012 at 04:27:54PM +0530, Shubhrajyoti D wrote:
> Changes since v1:
>        - removed tabification on patch 6/17
>        - removed dev_err() which was introduced on patch 09/17
> Changes since v2:
>         - do not set full fifo depth in the RDR interrupt.
>         - some changelog updates.
>         - rebase to the Wolfram's tree.
> Changes since v3:
>         - Remove a redundant read of status register
>         - Read the dev->buf_len variable instead of the register
>         as the information of the remaining bytes is there.
> Changes since v4:
>         - Ack the arbitration lost.
>         - Rebase to the i2c-embedded/for-next branch.
> Changes since v5:
>         - Rebase to latest mainline
>         - Added some more cleanup patches so as have a consolidated series.
> Changes since v6:
>         - Fix comments on setting the pdev to NULL.
>         - Trivial changelog update
> Changes since v7:
> 	- Remove a patch as the code is getting changed anyways
> 	- Changelogs update.
> 
> Previous discussions can be found here 
> http://www.spinics.net/lists/linux-i2c/msg09764.html
> 
> This is the cleanup only series.
>   
> Tested on omap4sdp and 3430sdp.
> 
> The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
> 
>   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
> 
> are available in the git repository at:
>   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups

Pushed to -next. Thanks to all involved!
Datta, Shubhrajyoti - Sept. 12, 2012, 1:25 p.m.
On Wednesday 12 September 2012 06:46 PM, Wolfram Sang wrote:
>> Tested on omap4sdp and 3430sdp.
>> > 
>> > The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
>> > 
>> >   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
>> > 
>> > are available in the git repository at:
>> >   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups
> Pushed to -next. Thanks to all involved!
Thanks.
Kevin Hilman - Sept. 12, 2012, 10:27 p.m.
Shubhrajyoti D <shubhrajyoti@ti.com> writes:

[...]

>
> This is the cleanup only series.
>   
> Tested on omap4sdp and 3430sdp.

It would be extremely helpful if you would describe how this was tested.
And for me, it would be a source of extreme joy if you described any PM
testing.

I gave this some additional PM testing on 3430/n900, 3530/Overo,
3730/OveroSTORM, 3730/Beagle-xM and 4430/Panda.

I tested v3.6-rc5 which passed all PM tests and then added this series
(by merging the i2c-embedded/i2c-next branch.)  PM tests then fail.

At least on 3530/Overo and 3730/Overo, CORE no longer hits retenion (or
off) during idle.  

The easy way to notice this is to see that even when no i2c transactions
are happening, the runtime PM status for omap_i2c.1 is remains 'active':

# cat omap_i2c.*/power/runtime_status 
active
suspended

Of course that means that clocks are never gated during idle, and CORE
will never hit retention.

I noticed it because my PM tests detected that the CORE powerdomain was
not transitioning to retention (or off) during idle.

To reproduce, simply enable UART timeouts so CORE will hit retention:

echo 3000 > /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
echo 3000 > /sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
echo 3000 > /sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
echo 3000 > /sys/devices/platform/omap_uart.3/power/autosuspend_delay_ms

and check the core_pwrm state counters:

cat /debug/pm_debug/count

wait > 3 seconds for the UART autosuspend timers to kick in.  At this
point, CORE should be transitioning to retenion in idle (determined by
noticing that the RET counter for core_pwrdm is increasing.)

With $SUBJECT series applied, you'll notice that CORE is not
transitioning.

Kevin

> The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
>
>   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
>
> are available in the git repository at:
>   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups
>
>
> Felipe Balbi (22):
>   i2c: omap: switch to devm_* API
>   i2c: omap: simplify num_bytes handling
>   i2c: omap: decrease indentation level on data handling
>   i2c: omap: add blank lines
>   i2c: omap: simplify omap_i2c_ack_stat()
>   i2c: omap: split out [XR]DR and [XR]RDY
>   i2c: omap: improve i462 errata handling
>   i2c: omap: re-factor receive/transmit data loop
>   i2c: omap: switch over to do {} while loop
>   i2c: omap: ack IRQ in parts
>   i2c: omap: switch to platform_get_irq()
>   i2c: omap: bus: add a receiver flag
>   i2c: omap: simplify errata check
>   i2c: omap: always return IRQ_HANDLED
>   i2c: omap: simplify IRQ exit path
>   i2c: omap: resize fifos before each message
>   i2c: omap: get rid of the "complete" label
>   i2c: omap: always return IRQ_HANDLED
>   i2c: omap: switch to threaded IRQ support
>   i2c: omap: remove unnecessary pm_runtime_suspended check
>   i2c: omap: switch over to autosuspend API
>   i2c: omap: sanitize exit path
>
> Shubhrajyoti D (1):
>   i2c: omap: remove redundant status read
>
>  drivers/i2c/busses/i2c-omap.c |  442 +++++++++++++++++++++++++----------------
>  1 files changed, 271 insertions(+), 171 deletions(-)
Kevin Hilman - Sept. 12, 2012, 10:39 p.m.
Wolfram Sang <w.sang@pengutronix.de> writes:

> On Wed, Sep 12, 2012 at 04:27:54PM +0530, Shubhrajyoti D wrote:

[...]

>> This is the cleanup only series.
>>   
>> Tested on omap4sdp and 3430sdp.
>> 
>> The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
>> 
>>   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
>> 
>> are available in the git repository at:
>>   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups
>
> Pushed to -next. Thanks to all involved!

Sorry to be late to the party (again), but still catching up after some
time off.

Unfortunately, this series causes PM regressions on several OMAP
platforms.  I hope we can hold off on this until those issues are
addressed.

We *really* need to have some more thorough testing (especially PM)
before merging large series like this.

Kevin
Kevin Hilman - Sept. 12, 2012, 11:03 p.m.
Shubhrajyoti D <shubhrajyoti@ti.com> writes:

> From: Felipe Balbi <balbi@ti.com>
>
> this helps us reduce unnecessary pm transitions
> in case we have another i2c message starting soon.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

I tracked the PM regression down to this patch.

> ---
>  drivers/i2c/busses/i2c-omap.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 6d38a57..122f517 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -55,6 +55,9 @@
>  /* timeout waiting for the controller to respond */
>  #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
>  
> +/* timeout for pm runtime autosuspend */
> +#define OMAP_I2C_PM_TIMEOUT		1000	/* ms */
> +
>  /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
>  enum {
>  	OMAP_I2C_REV_REG = 0,
> @@ -645,7 +648,8 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  	omap_i2c_wait_for_bb(dev);
>  out:
> -	pm_runtime_put(dev->dev);
> +	pm_runtime_mark_last_busy(dev->dev);
> +	pm_runtime_put_autosuspend(dev->dev);

Reverting this change allows CORE to hit retention again.

I didn't debug this any further, so I'm not sure exactly why the async
suspend works but not the autosuspend.

Kevin
Kevin Hilman - Sept. 12, 2012, 11:08 p.m.
Kevin Hilman <khilman@deeprootsystems.com> writes:

> Wolfram Sang <w.sang@pengutronix.de> writes:
>
>> On Wed, Sep 12, 2012 at 04:27:54PM +0530, Shubhrajyoti D wrote:
>
> [...]
>
>>> This is the cleanup only series.
>>>   
>>> Tested on omap4sdp and 3430sdp.
>>> 
>>> The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
>>> 
>>>   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
>>> 
>>> are available in the git repository at:
>>>   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups
>>
>> Pushed to -next. Thanks to all involved!
>
> Sorry to be late to the party (again), but still catching up after some
> time off.
>
> Unfortunately, this series causes PM regressions on several OMAP
> platforms.  I hope we can hold off on this until those issues are
> addressed.

I tracked the regression down to [PATCHv8 21/22] (see reply there.)

Since this series is already merged, I suggest that the problem patch be
reverted, at least for v3.7 and until the problem is better understood
and tested.

With that patch reverted, all my PM tests are passing.  Feel free to
add:

Tested-by: Kevin Hilman <khilman@ti.com>

Kevin
Datta, Shubhrajyoti - Sept. 13, 2012, 6:04 a.m.
On Thursday 13 September 2012 03:57 AM, Kevin Hilman wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
> [...]
>
>> This is the cleanup only series.
>>   
>> Tested on omap4sdp and 3430sdp.
> It would be extremely helpful if you would describe how this was tested.
> And for me, it would be a source of extreme joy if you described any PM
> testing.
>
> I gave this some additional PM testing on 3430/n900, 3530/Overo,
> 3730/OveroSTORM, 3730/Beagle-xM and 4430/Panda.
>
> I tested v3.6-rc5 which passed all PM tests and then added this series
> (by merging the i2c-embedded/i2c-next branch.)  PM tests then fail.
>
> At least on 3530/Overo and 3730/Overo, CORE no longer hits retenion (or
> off) during idle.  
>
> The easy way to notice this is to see that even when no i2c transactions
> are happening, the runtime PM status for omap_i2c.1 is remains 'active':
>
> # cat omap_i2c.*/power/runtime_status 
> active
> suspended
>
> Of course that means that clocks are never gated during idle, and CORE
> will never hit retention.
>
> I noticed it because my PM tests detected that the CORE powerdomain was
> not transitioning to retention (or off) during idle.
>
> To reproduce, simply enable UART timeouts so CORE will hit retention:
>
> echo 3000 > /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
> echo 3000 > /sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
> echo 3000 > /sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
> echo 3000 > /sys/devices/platform/omap_uart.3/power/autosuspend_delay_ms
>
> and check the core_pwrm state counters:
>
> cat /debug/pm_debug/count
>
> wait > 3 seconds for the UART autosuspend timers to kick in.  At this
> point, CORE should be transitioning to retenion in idle (determined by
> noticing that the RET counter for core_pwrdm is increasing.)
>
> With $SUBJECT series applied, you'll notice that CORE is not
> transitioning.
However I do not see the issue,let me know if I missed something see below.

On my 3430sdp

/sys/bus/platform/devices # cat omap_i2c.*/power/runtime_status
suspended
suspended
suspended
/sys/bus/platform/devices #
/sys/bus/platform/devices # cat omap_i2c.*/power/autosuspend_delay_ms
1000
1000
1000
/sys/bus/platform/devices # echo 3000 >
/sys/devices/platform/omap_uart.0/power/
autosuspend_delay_ms
/sys/bus/platform/devices # echo 3000 >
/sys/devices/platform/omap_uart.1/power/
autosuspend_delay_ms
/sys/bus/platform/devices # echo 3000 >
/sys/devices/platform/omap_uart.2/power/
autosuspend_delay_ms
/sys/bus/platform/devices #
/sys/bus/platform/devices # cat /debug/pm_debug/count
usbhost_pwrdm
(ON),OFF:0,RET:1373,INA:0,ON:1374,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm
(ON),OFF:0,RET:1,INA:0,ON:2,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:79,INA:0,ON:80,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm
(ON),OFF:0,RET:1373,INA:0,ON:1374,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:1373,INA:0,ON:1374,RET-LOGIC-OFF:0
mpu_pwrdm
(ON),OFF:0,RET:1373,INA:0,ON:1374,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm
(RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RE0
usbhost_clkdm->usbhost_pwrdm (1)
sgx_clkdm->sgx_pwrdm (0)
per_clkdm->per_pwrdm (13)
cam_clkdm->cam_pwrdm (0)
dss_clkdm->dss_pwrdm (1)
d2d_clkdm->core_pwrdm (0)
iva2_clkdm->iva2_pwrdm (0)
mpu_clkdm->mpu_pwrdm (0)
core_l4_clkdm->core_pwrdm (23)
core_l3_clkdm->core_pwrdm (4)
neon_clkdm->neon_pwrdm (0)
/sys/bus/platform/devices #
/sys/bus/platform/devices #
/sys/bus/platform/devices # sleep 5
/sys/bus/platform/devices # cat /debug/pm_debug/count
usbhost_pwrdm
(ON),OFF:0,RET:1512,INA:0,ON:1513,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm
(ON),OFF:0,RET:12,INA:0,ON:13,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:218,INA:0,ON:219,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm
(ON),OFF:0,RET:1512,INA:0,ON:1513,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:1512,INA:0,ON:1513,RET-LOGIC-OFF:0
mpu_pwrdm
(ON),OFF:0,RET:1512,INA:0,ON:1513,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm
(RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RE0
usbhost_clkdm->usbhost_pwrdm (1)
sgx_clkdm->sgx_pwrdm (0)
per_clkdm->per_pwrdm (13)
cam_clkdm->cam_pwrdm (0)
dss_clkdm->dss_pwrdm (1)
d2d_clkdm->core_pwrdm (0)
iva2_clkdm->iva2_pwrdm (0)
mpu_clkdm->mpu_pwrdm (0)
core_l4_clkdm->core_pwrdm (23)
core_l3_clkdm->core_pwrdm (4)
neon_clkdm->neon_pwrdm (0)
/sys/bus/platform/devices # cat omap_i2c.*/power/runtime_status
suspended
suspended
suspended
/sys/bus/platform/devices #


>
> Kevin
>
>> The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
>>
>>   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
>>
>> are available in the git repository at:
>>   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups
>>
>>
>> Felipe Balbi (22):
>>   i2c: omap: switch to devm_* API
>>   i2c: omap: simplify num_bytes handling
>>   i2c: omap: decrease indentation level on data handling
>>   i2c: omap: add blank lines
>>   i2c: omap: simplify omap_i2c_ack_stat()
>>   i2c: omap: split out [XR]DR and [XR]RDY
>>   i2c: omap: improve i462 errata handling
>>   i2c: omap: re-factor receive/transmit data loop
>>   i2c: omap: switch over to do {} while loop
>>   i2c: omap: ack IRQ in parts
>>   i2c: omap: switch to platform_get_irq()
>>   i2c: omap: bus: add a receiver flag
>>   i2c: omap: simplify errata check
>>   i2c: omap: always return IRQ_HANDLED
>>   i2c: omap: simplify IRQ exit path
>>   i2c: omap: resize fifos before each message
>>   i2c: omap: get rid of the "complete" label
>>   i2c: omap: always return IRQ_HANDLED
>>   i2c: omap: switch to threaded IRQ support
>>   i2c: omap: remove unnecessary pm_runtime_suspended check
>>   i2c: omap: switch over to autosuspend API
>>   i2c: omap: sanitize exit path
>>
>> Shubhrajyoti D (1):
>>   i2c: omap: remove redundant status read
>>
>>  drivers/i2c/busses/i2c-omap.c |  442 +++++++++++++++++++++++++----------------
>>  1 files changed, 271 insertions(+), 171 deletions(-)
Felipe Balbi - Sept. 13, 2012, 6:36 a.m.
Hi,

On Thu, Sep 13, 2012 at 11:34:48AM +0530, Shubhrajyoti wrote:
> On Thursday 13 September 2012 03:57 AM, Kevin Hilman wrote:
> > Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> >
> > [...]
> >
> >> This is the cleanup only series.
> >>   
> >> Tested on omap4sdp and 3430sdp.
> > It would be extremely helpful if you would describe how this was tested.
> > And for me, it would be a source of extreme joy if you described any PM
> > testing.
> >
> > I gave this some additional PM testing on 3430/n900, 3530/Overo,
> > 3730/OveroSTORM, 3730/Beagle-xM and 4430/Panda.
> >
> > I tested v3.6-rc5 which passed all PM tests and then added this series
> > (by merging the i2c-embedded/i2c-next branch.)  PM tests then fail.
> >
> > At least on 3530/Overo and 3730/Overo, CORE no longer hits retenion (or
> > off) during idle.  
> >
> > The easy way to notice this is to see that even when no i2c transactions
> > are happening, the runtime PM status for omap_i2c.1 is remains 'active':
> >
> > # cat omap_i2c.*/power/runtime_status 
> > active
> > suspended
> >
> > Of course that means that clocks are never gated during idle, and CORE
> > will never hit retention.
> >
> > I noticed it because my PM tests detected that the CORE powerdomain was
> > not transitioning to retention (or off) during idle.
> >
> > To reproduce, simply enable UART timeouts so CORE will hit retention:
> >
> > echo 3000 > /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
> > echo 3000 > /sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
> > echo 3000 > /sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
> > echo 3000 > /sys/devices/platform/omap_uart.3/power/autosuspend_delay_ms
> >
> > and check the core_pwrm state counters:
> >
> > cat /debug/pm_debug/count
> >
> > wait > 3 seconds for the UART autosuspend timers to kick in.  At this
> > point, CORE should be transitioning to retenion in idle (determined by
> > noticing that the RET counter for core_pwrdm is increasing.)
> >
> > With $SUBJECT series applied, you'll notice that CORE is not
> > transitioning.
> However I do not see the issue,let me know if I missed something see below.
> 
> On my 3430sdp
> 
> /sys/bus/platform/devices # cat omap_i2c.*/power/runtime_status
> suspended
> suspended
> suspended
> /sys/bus/platform/devices #
> /sys/bus/platform/devices # cat omap_i2c.*/power/autosuspend_delay_ms
> 1000
> 1000
> 1000

I just revived my very, very old 3503 overo and while i2c isn't active,
I don't see transitions on core power domain:

# mount -t proc none /proc
# mount -t sysfs none /sys
# mount -t debugfs none /debug
# echo 3000 > /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
# echo 3000 > /sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
# echo 3000 > /sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
# cat /sys/devices/platform/omap_i2c.*/power/runtime_status
suspended
suspended
# grep ^core_pwrdm /debug/pm_debug/count && sleep 5 && grep ^core_pwrdm /debug/pm_debug/count
core_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
core_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0

Weird, but I don't think i2c is to blame here (unless I'm missing
something). I can see that after i2c transfers, my suspend function is
called and right after that i2c is suspended:

# cat /sys/class/regulator/regulator.*/status && sleep 1 && cat /sys/devices/platform/omap_i2c.*/power/runtime_status
[  411.072998] omap_i2c omap_i2c.1: omap_i2c_runtime_resume
normal
normal
normal
[  412.075347] omap_i2c omap_i2c.1: omap_i2c_runtime_suspend
suspended
suspended

What am I missing to get any pm transitions to happen with my overo ?

# cat  /debug/pm_debug/count && sleep 5 && cat /debug/pm_debug/count
usbhost_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:0,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:2379,INA:0,ON:2380,RET-LOGIC-OFF:0
mpu_pwrdm (ON),OFF:0,RET:2377,INA:0,ON:2378,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0
usbhost_clkdm->usbhost_pwrdm (1)
sgx_clkdm->sgx_pwrdm (0)
per_clkdm->per_pwrdm (19)
cam_clkdm->cam_pwrdm (0)
dss_clkdm->dss_pwrdm (1)
d2d_clkdm->core_pwrdm (0)
iva2_clkdm->iva2_pwrdm (0)
mpu_clkdm->mpu_pwrdm (0)
core_l4_clkdm->core_pwrdm (21)
core_l3_clkdm->core_pwrdm (4)
neon_clkdm->neon_pwrdm (0)
usbhost_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:0,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:2442,INA:0,ON:2443,RET-LOGIC-OFF:0
mpu_pwrdm (ON),OFF:0,RET:2440,INA:0,ON:2441,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0
usbhost_clkdm->usbhost_pwrdm (1)
sgx_clkdm->sgx_pwrdm (0)
per_clkdm->per_pwrdm (19)
cam_clkdm->cam_pwrdm (0)
dss_clkdm->dss_pwrdm (1)
d2d_clkdm->core_pwrdm (0)
iva2_clkdm->iva2_pwrdm (0)
mpu_clkdm->mpu_pwrdm (0)
core_l4_clkdm->core_pwrdm (21)
core_l3_clkdm->core_pwrdm (4)
neon_clkdm->neon_pwrdm (0)
Datta, Shubhrajyoti - Sept. 13, 2012, 6:55 a.m.
On Thursday 13 September 2012 11:34 AM, Shubhrajyoti wrote:
> > At least on 3530/Overo and 3730/Overo, CORE no longer hits retenion (or
> > off) during idle.  
I dont have an Overo, anyways will see if I can get one

However on beagleXm even off count increases

/ # grep "^core_pwrdm" /debug/pm_debug/count
core_pwrdm
(ON),OFF:48,RET:856,INA:0,ON:905,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
/ # cat /sys/devices/platform/omap_i2c.*/power/runtime_status
suspended
suspended
/ #
/ #
/ # cat /sys/devices/platform/omap_i2c.*/power/autosuspend_delay_ms
1000
1000
/ #
/ #
/ # grep "^core_pwrdm" /debug/pm_debug/count
core_pwrdm
(ON),OFF:218,RET:856,INA:0,ON:1075,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
/ #
Wolfram Sang - Sept. 13, 2012, 8:52 a.m.
Hi Kevin,

> Since this series is already merged, I suggest that the problem patch be
> reverted, at least for v3.7 and until the problem is better understood
> and tested.

I thought I'll give you guys some more days to fix the problem before
reverting.

> 
> With that patch reverted, all my PM tests are passing.  Feel free to
> add:
> 
> Tested-by: Kevin Hilman <khilman@ti.com>

Thanks. That indeed increases confidence in this series. I won't add the
tags to the tree, though, since that would mean rebasing.

All the best,

   Wolfram
Kevin Hilman - Sept. 13, 2012, 2:31 p.m.
Felipe Balbi <balbi@ti.com> writes:

[...]

> I just ran same tests on pandaboard and i2c is suspended actually,
> though no power domain transitions to RET. Do we not have retention
> while idle on pandaboard ?

Not for CORE.  Only CPUx and MPU domains will be transitioning on OMAP4,
and then, only with CPUidle enabled.

Until we have CORE retention support in mainline (we're almost there)
OMAP4 is not a useful test platform for device PM.

Kevin
Kevin Hilman - Sept. 13, 2012, 6:04 p.m.
Kevin Hilman <khilman@deeprootsystems.com> writes:

> Kevin Hilman <khilman@deeprootsystems.com> writes:
>
[...]

>> Sorry to be late to the party (again), but still catching up after some
>> time off.
>>
>> Unfortunately, this series causes PM regressions on several OMAP
>> platforms.  I hope we can hold off on this until those issues are
>> addressed.
>
> I tracked the regression down to [PATCHv8 21/22] (see reply there.)
>
> Since this series is already merged, I suggest that the problem patch be
> reverted, at least for v3.7 and until the problem is better understood
> and tested.
>
> With that patch reverted, all my PM tests are passing.  Feel free to
> add:

OK, the i2c series is off the hook.

Felipe and I spent a little time tracking this down.  Felipe suggested
that there might be a driver with periodic i2c activity keeping I2C
awake, and thus preventing CORE retention.  He was right.

On the boards where I'm seeing the failure, the RTC is firing every
second, and since the RTC is on the I2C connected PMIC, the PMIC IRQ
reads and the RTC reads are causing lots of I2C activity every second.  

With the new autosuspend feature, that is enough to keep the I2C active
continually and prevent CORE retention.

So, all that to say, from my PoV, this series can go in as is.  The PM
problem is caused by the RTC driver someplace.

Thanks Felipe,

Kevin
Felipe Balbi - Sept. 13, 2012, 6:37 p.m.
Hi,

On Thu, Sep 13, 2012 at 11:04:42AM -0700, Kevin Hilman wrote:
> Kevin Hilman <khilman@deeprootsystems.com> writes:
> 
> > Kevin Hilman <khilman@deeprootsystems.com> writes:
> >
> [...]
> 
> >> Sorry to be late to the party (again), but still catching up after some
> >> time off.
> >>
> >> Unfortunately, this series causes PM regressions on several OMAP
> >> platforms.  I hope we can hold off on this until those issues are
> >> addressed.
> >
> > I tracked the regression down to [PATCHv8 21/22] (see reply there.)
> >
> > Since this series is already merged, I suggest that the problem patch be
> > reverted, at least for v3.7 and until the problem is better understood
> > and tested.
> >
> > With that patch reverted, all my PM tests are passing.  Feel free to
> > add:
> 
> OK, the i2c series is off the hook.
> 
> Felipe and I spent a little time tracking this down.  Felipe suggested
> that there might be a driver with periodic i2c activity keeping I2C
> awake, and thus preventing CORE retention.  He was right.

FYI, the original idea came from Shubhro. We agreed that would be the
only way i2c would be prevented from idling.
Kevin Hilman - Sept. 13, 2012, 9:28 p.m.
Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Thu, Sep 13, 2012 at 11:04:42AM -0700, Kevin Hilman wrote:
>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>> 
>> > Kevin Hilman <khilman@deeprootsystems.com> writes:
>> >
>> [...]
>> 
>> >> Sorry to be late to the party (again), but still catching up after some
>> >> time off.
>> >>
>> >> Unfortunately, this series causes PM regressions on several OMAP
>> >> platforms.  I hope we can hold off on this until those issues are
>> >> addressed.
>> >
>> > I tracked the regression down to [PATCHv8 21/22] (see reply there.)
>> >
>> > Since this series is already merged, I suggest that the problem patch be
>> > reverted, at least for v3.7 and until the problem is better understood
>> > and tested.
>> >
>> > With that patch reverted, all my PM tests are passing.  Feel free to
>> > add:
>> 
>> OK, the i2c series is off the hook.
>> 
>> Felipe and I spent a little time tracking this down.  Felipe suggested
>> that there might be a driver with periodic i2c activity keeping I2C
>> awake, and thus preventing CORE retention.  He was right.
>
> FYI, the original idea came from Shubhro. We agreed that would be the
> only way i2c would be prevented from idling.

Great, thanks Shubhro!

Also, FYI, I just submitted a patch to the TWL RTC driver which was the
source of all the I2C activity since it's on the I2C-connected PMIC.

Thanks for the help and suggestions,

Kevin

[1] https://groups.google.com/forum/#!topic/rtc-linux/sFbYmAzCRLQ
Datta, Shubhrajyoti - Sept. 14, 2012, 10:40 a.m.
On Friday 14 September 2012 02:58 AM, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
>
>> Hi,
>>
>> On Thu, Sep 13, 2012 at 11:04:42AM -0700, Kevin Hilman wrote:
>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>>
>>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>>>
>>> [...]
>>>
>>>>> Sorry to be late to the party (again), but still catching up after some
>>>>> time off.
>>>>>
>>>>> Unfortunately, this series causes PM regressions on several OMAP
>>>>> platforms.  I hope we can hold off on this until those issues are
>>>>> addressed.
>>>> I tracked the regression down to [PATCHv8 21/22] (see reply there.)
>>>>
>>>> Since this series is already merged, I suggest that the problem patch be
>>>> reverted, at least for v3.7 and until the problem is better understood
>>>> and tested.
>>>>
>>>> With that patch reverted, all my PM tests are passing.  Feel free to
>>>> add:
>>> OK, the i2c series is off the hook.
>>>
>>> Felipe and I spent a little time tracking this down.  Felipe suggested
>>> that there might be a driver with periodic i2c activity keeping I2C
>>> awake, and thus preventing CORE retention.  He was right.
>> FYI, the original idea came from Shubhro. We agreed that would be the
>> only way i2c would be prevented from idling.
> Great, thanks Shubhro!
>
> Also, FYI, I just submitted a patch to the TWL RTC driver which was the
> source of all the I2C activity since it's on the I2C-connected PMIC.
>
> Thanks for the help and suggestions,
>
> Kevin
>
> [1] https://groups.google.com/forum/#!topic/rtc-linux/sFbYmAzCRLQ
Thanks for the testing and the patch.