mbox

[PATCHv10,00/11] I2C fixes

Message ID 1338288983-15026-1-git-send-email-shubhrajyoti@ti.com
State New
Headers show

Pull-request

git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes

Message

Datta, Shubhrajyoti May 29, 2012, 10:56 a.m. UTC
The patch series does the following

- Warn fixes if CONFIG_PM_RUNTIME is not selected.
- In case of i2c remove register access was done without any
 get_sync fix the same.
- Folds a patch from Tasslehoff to prevent any merge conflicts.
- Prevents the XDUF flag to be set if the underflow condition is not met.
- As per discussion in [1] .Adds a patch to rename the 1p153 errata and
 use the unique id instead as the section number in the recent errata
 docs has changed.

v9:
Fix the comments from Wolfram Sang

v10:
Add a patch from Neil to the series.
Fix kevin comments
update the patches with comments.

[1] http://www.spinics.net/lists/linux-i2c/msg07607.html

Tested on omap4sdp and omap3sdp.
- Did functional tests read write on both the platforms.
- Off mode and retention on OMAP3
- On OMAP4 just did echo mem > /sys/power/state and wakeup.
Did see.
[ 1360.595855] Successfully put all powerdomains to target state
However didnt see the ret count for other than mpu_pwrdm increase.
I think power support for omap4 is minimal. 

The following changes since commit b48b2c3e50433ff6f7e46186daa7f986bd960215:

  openrisc: use generic strnlen_user() function (2012-05-27 21:00:32 -0700)

are available in the git repository at:
  git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes

Neil Brown (1):
      OMAP/I2C - Fix timeout problem during suspend.

Shubhrajyoti D (9):
      I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME
      I2C: OMAP: Fix the mismatch of pm_runtime enable and disable
      I2C: OMAP: Fix the interrupt clearing in OMAP4
      I2C: OMAP: Prevent the register access after pm_runtime_put in probe
      I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero
      I2C: OMAP: Fix the crash in i2c remove
      I2C: OMAP: Handle error check for pm runtime
      I2C: OMAP: Do not set the XUDF(Transmit underflow) if the underflow is not reached
      I2C: OMAP: Rename the 1p153 to the erratum id i462

Tasslehoff Kjappfot (1):
      I2C: OMAP: prevent the overwrite of the errata flags

 drivers/i2c/busses/i2c-omap.c |  129 ++++++++++++++++++++---------------------
 1 files changed, 63 insertions(+), 66 deletions(-)

Comments

Kevin Hilman May 31, 2012, 10:59 p.m. UTC | #1
Shubhrajyoti D <shubhrajyoti@ti.com> writes:

> The patch series does the following
>
> - Warn fixes if CONFIG_PM_RUNTIME is not selected.
> - In case of i2c remove register access was done without any
>  get_sync fix the same.
> - Folds a patch from Tasslehoff to prevent any merge conflicts.
> - Prevents the XDUF flag to be set if the underflow condition is not met.
> - As per discussion in [1] .Adds a patch to rename the 1p153 errata and
>  use the unique id instead as the section number in the recent errata
>  docs has changed.
>
> v9:
> Fix the comments from Wolfram Sang
>
> v10:
> Add a patch from Neil to the series.
> Fix kevin comments
> update the patches with comments.

Shubhrajyoti, thanks for the updates.

Wolfgang, with these updates and testing a bit better described, I'm OK
with you merging it.  Merging it now will give it plenty of time to
bake in linux-next and get more test exposure.

Thanks,

Kevin


> [1] http://www.spinics.net/lists/linux-i2c/msg07607.html
>
> Tested on omap4sdp and omap3sdp.
> - Did functional tests read write on both the platforms.
> - Off mode and retention on OMAP3
> - On OMAP4 just did echo mem > /sys/power/state and wakeup.
> Did see.
> [ 1360.595855] Successfully put all powerdomains to target state
> However didnt see the ret count for other than mpu_pwrdm increase.
> I think power support for omap4 is minimal. 
>
> The following changes since commit b48b2c3e50433ff6f7e46186daa7f986bd960215:
>
>   openrisc: use generic strnlen_user() function (2012-05-27 21:00:32 -0700)
>
> are available in the git repository at:
>   git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes
>
> Neil Brown (1):
>       OMAP/I2C - Fix timeout problem during suspend.
>
> Shubhrajyoti D (9):
>       I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME
>       I2C: OMAP: Fix the mismatch of pm_runtime enable and disable
>       I2C: OMAP: Fix the interrupt clearing in OMAP4
>       I2C: OMAP: Prevent the register access after pm_runtime_put in probe
>       I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero
>       I2C: OMAP: Fix the crash in i2c remove
>       I2C: OMAP: Handle error check for pm runtime
>       I2C: OMAP: Do not set the XUDF(Transmit underflow) if the underflow is not reached
>       I2C: OMAP: Rename the 1p153 to the erratum id i462
>
> Tasslehoff Kjappfot (1):
>       I2C: OMAP: prevent the overwrite of the errata flags
>
>  drivers/i2c/busses/i2c-omap.c |  129 ++++++++++++++++++++---------------------
>  1 files changed, 63 insertions(+), 66 deletions(-)
Shubhrajyoti Datta June 10, 2012, 5:40 a.m. UTC | #2
On Fri, Jun 1, 2012 at 4:29 AM, Kevin Hilman <khilman@ti.com> wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
>> The patch series does the following
>>
>> - Warn fixes if CONFIG_PM_RUNTIME is not selected.
>> - In case of i2c remove register access was done without any
>>  get_sync fix the same.
>> - Folds a patch from Tasslehoff to prevent any merge conflicts.
>> - Prevents the XDUF flag to be set if the underflow condition is not met.
>> - As per discussion in [1] .Adds a patch to rename the 1p153 errata and
>>  use the unique id instead as the section number in the recent errata
>>  docs has changed.
>>
>> v9:
>> Fix the comments from Wolfram Sang
>>
>> v10:
>> Add a patch from Neil to the series.
>> Fix kevin comments
>> update the patches with comments.
>
> Shubhrajyoti, thanks for the updates.
>
> Wolfgang, with these updates and testing a bit better described, I'm OK
> with you merging it.  Merging it now will give it plenty of time to
> bake in linux-next and get more test exposure.

Agree,
These are only fixes can it be considered for rc3?

Thanks and Regards,
Shubhrajyoti
>
> Thanks,
>
> Kevin
>
>
>> [1] http://www.spinics.net/lists/linux-i2c/msg07607.html
>>
>> Tested on omap4sdp and omap3sdp.
>> - Did functional tests read write on both the platforms.
>> - Off mode and retention on OMAP3
>> - On OMAP4 just did echo mem > /sys/power/state and wakeup.
>> Did see.
>> [ 1360.595855] Successfully put all powerdomains to target state
>> However didnt see the ret count for other than mpu_pwrdm increase.
>> I think power support for omap4 is minimal.
>>
>> The following changes since commit b48b2c3e50433ff6f7e46186daa7f986bd960215:
>>
>>   openrisc: use generic strnlen_user() function (2012-05-27 21:00:32 -0700)
>>
>> are available in the git repository at:
>>   git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes
>>
>> Neil Brown (1):
>>       OMAP/I2C - Fix timeout problem during suspend.
>>
>> Shubhrajyoti D (9):
>>       I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME
>>       I2C: OMAP: Fix the mismatch of pm_runtime enable and disable
>>       I2C: OMAP: Fix the interrupt clearing in OMAP4
>>       I2C: OMAP: Prevent the register access after pm_runtime_put in probe
>>       I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero
>>       I2C: OMAP: Fix the crash in i2c remove
>>       I2C: OMAP: Handle error check for pm runtime
>>       I2C: OMAP: Do not set the XUDF(Transmit underflow) if the underflow is not reached
>>       I2C: OMAP: Rename the 1p153 to the erratum id i462
>>
>> Tasslehoff Kjappfot (1):
>>       I2C: OMAP: prevent the overwrite of the errata flags
>>
>>  drivers/i2c/busses/i2c-omap.c |  129 ++++++++++++++++++++---------------------
>>  1 files changed, 63 insertions(+), 66 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang June 11, 2012, 4 p.m. UTC | #3
On Sun, Jun 10, 2012 at 11:10:47AM +0530, Shubhrajyoti Datta wrote:
> On Fri, Jun 1, 2012 at 4:29 AM, Kevin Hilman <khilman@ti.com> wrote:
> > Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> >
> >> The patch series does the following
> >>
> >> - Warn fixes if CONFIG_PM_RUNTIME is not selected.
> >> - In case of i2c remove register access was done without any
> >>  get_sync fix the same.
> >> - Folds a patch from Tasslehoff to prevent any merge conflicts.
> >> - Prevents the XDUF flag to be set if the underflow condition is not met.
> >> - As per discussion in [1] .Adds a patch to rename the 1p153 errata and
> >>  use the unique id instead as the section number in the recent errata
> >>  docs has changed.
> >>
> >> v9:
> >> Fix the comments from Wolfram Sang
> >>
> >> v10:
> >> Add a patch from Neil to the series.
> >> Fix kevin comments
> >> update the patches with comments.
> >
> > Shubhrajyoti, thanks for the updates.
> >
> > Wolfgang, with these updates and testing a bit better described, I'm OK
> > with you merging it.  Merging it now will give it plenty of time to
> > bake in linux-next and get more test exposure.
> 
> Agree,
> These are only fixes can it be considered for rc3?

"Baking in linux-next" and "considering rc3" don't match; baking needs
time, rc3 is soon. I've put the patches now into my -next branch for
more exposure. I am still uncertain if they should be in 3.5 already;
there seem to be worhty fixes in there, but they do depend on stuff
which don't really qualify as bugfixes...

Thanks,

   Wolfram
Datta, Shubhrajyoti June 11, 2012, 5:39 p.m. UTC | #4
On Monday 11 June 2012 09:30 PM, Wolfram Sang wrote:
>> Agree,
>> > These are only fixes can it be considered for rc3?
> "Baking in linux-next" and "considering rc3" don't match; baking needs
> time, rc3 is soon. I've put the patches now into my -next branch for
> more exposure.
Thanks.
>  I am still uncertain if they should be in 3.5 already;
> there seem to be worhty fixes in there, but they do depend on stuff
> which don't really qualify as bugfixes...
Kevin Hilman June 27, 2012, 1:43 a.m. UTC | #5
Shubhrajyoti D <shubhrajyoti@ti.com> writes:

> If PM runtime get_sync fails return with the error
> so that no further reads/writes goes through the interface.
> This will avoid possible abort. Add a error message in case
> of failure with the cause of the failure.
>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

This patch introduced a regression where the runtime PM usecount will
never reach zero and so CORE retention is prevented after any xfer
failures...

> ---
> -v10 Use IS_ERR_VALUE
> -v9 Fix the error handling
>
>  drivers/i2c/busses/i2c-omap.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 44e8cfa..c39b72f 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -585,7 +585,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int i;
>  	int r;
>  
> -	pm_runtime_get_sync(dev->dev);
> +	r = pm_runtime_get_sync(dev->dev);<
> +	if (IS_ERR_VALUE(r))
> +		return r;

This return should be 'goto out' so the runtime PM usecount is
decremented by the 'put'.  Otherwise, after failure, the usecount
remains non-zero, so the device is considered 'active' and keeps the
containing power domain active.

I found this on a 3730/OveroSTORM where the suspend/resume of MMC fails
because I2C is already suspended.  After the suspend though, the CORE
powerdomain never again hits retention, and I tracked it down to this.

I'll send a separate patch to fix this shortly.

Kevin
Shubhrajyoti Datta June 27, 2012, 9:01 a.m. UTC | #6
On Wed, Jun 27, 2012 at 7:13 AM, Kevin Hilman <khilman@ti.com> wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
>> If PM runtime get_sync fails return with the error
>> so that no further reads/writes goes through the interface.
>> This will avoid possible abort. Add a error message in case
>> of failure with the cause of the failure.
>>
>> Reviewed-by: Kevin Hilman <khilman@ti.com>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>
> This patch introduced a regression where the runtime PM usecount will
> never reach zero and so CORE retention is prevented after any xfer
> failures...
>
>> ---
>> -v10 Use IS_ERR_VALUE
>> -v9 Fix the error handling
>>
>>  drivers/i2c/busses/i2c-omap.c |   14 +++++++++++---
>>  1 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 44e8cfa..c39b72f 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -585,7 +585,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>       int i;
>>       int r;
>>
>> -     pm_runtime_get_sync(dev->dev);
>> +     r = pm_runtime_get_sync(dev->dev);<
>> +     if (IS_ERR_VALUE(r))
>> +             return r;
>
> This return should be 'goto out' so the runtime PM usecount is
> decremented by the 'put'.  Otherwise, after failure, the usecount
> remains non-zero, so the device is considered 'active' and keeps the
> containing power domain active.
>
> I found this on a 3730/OveroSTORM where the suspend/resume of MMC fails
> because I2C is already suspended.  After the suspend though, the CORE
> powerdomain never again hits retention, and I tracked it down to this.
>
> I'll send a separate patch to fix this shortly.

Thanks,
>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html