Re: tvp5150 regression after commit 9f924169c035

Re: tvp5150 regression after commit 9f924169c035

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Wed, Feb 03, 2016 at 10:46:51AM -0300, Javier Martinez Canillas wrote:
> Hello Wolfram,
> 
> I've a issue with a I2C video decoder driver (drivers/media/i2c/tvp5150.c).
> 
> In v4.5-rc1, the driver gets I2C read / writes timeouts when accessing the
> device I2C registers:
> 
> tvp5150 1-005c: i2c i/o error: rc == -110
> tvp5150: probe of 1-005c failed with error -110
> 
> The driver used to work up to v4.4 so this is a regression in v4.5-rc1:
> 
> tvp5150 1-005c: tvp5151 (1.0) chip found @ 0xb8 (OMAP I2C adapter)
> tvp5150 1-005c: tvp5151 detected.
> 
> I tracked down to commit 9f924169c035 ("i2c: always enable RuntimePM for
> the adapter device") and reverting that commit makes things to work again.
> 
> The tvp5150 driver doesn't have runtime PM support but the I2C controller
> driver does (drivers/i2c/busses/i2c-omap.c) FWIW.
> 
> I tried adding runtime PM support to tvp5150 (basically calling pm_runtime
> enable/get on probe before the first I2C read to resume the controller) but
> that it did not work.
> 
> Not filling the OMAP I2C driver's runtime PM callbacks does not help either.
> 
> Any hints about the proper way to fix this issue?

Asking linux-pm for help:

The commit in question enables RuntimePM for the logical adapter device
which sits between the HW I2C controller and the I2C client device. This
adapter device has been used with pm_runtime_no_callbacks before
enabling RPM. Now, Alan explained to me that "suspend events will
propagate from the I2C clients all the way up to the adapter's parent."
with RPM enabled for the adapter device. Which should be a no-op if the
client doesn't do any PM at all? What do I miss?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello,

On 02/08/2016 07:54 AM, Wolfram Sang wrote:
> On Wed, Feb 03, 2016 at 10:46:51AM -0300, Javier Martinez Canillas wrote:
>> Hello Wolfram,
>>
>> I've a issue with a I2C video decoder driver (drivers/media/i2c/tvp5150.c).
>>
>> In v4.5-rc1, the driver gets I2C read / writes timeouts when accessing the
>> device I2C registers:
>>
>> tvp5150 1-005c: i2c i/o error: rc == -110
>> tvp5150: probe of 1-005c failed with error -110
>>
>> The driver used to work up to v4.4 so this is a regression in v4.5-rc1:
>>
>> tvp5150 1-005c: tvp5151 (1.0) chip found @ 0xb8 (OMAP I2C adapter)
>> tvp5150 1-005c: tvp5151 detected.
>>
>> I tracked down to commit 9f924169c035 ("i2c: always enable RuntimePM for
>> the adapter device") and reverting that commit makes things to work again.
>>
>> The tvp5150 driver doesn't have runtime PM support but the I2C controller
>> driver does (drivers/i2c/busses/i2c-omap.c) FWIW.
>>
>> I tried adding runtime PM support to tvp5150 (basically calling pm_runtime
>> enable/get on probe before the first I2C read to resume the controller) but
>> that it did not work.
>>
>> Not filling the OMAP I2C driver's runtime PM callbacks does not help either.
>>
>> Any hints about the proper way to fix this issue?
>
> Asking linux-pm for help:
>
> The commit in question enables RuntimePM for the logical adapter device
> which sits between the HW I2C controller and the I2C client device. This
> adapter device has been used with pm_runtime_no_callbacks before
> enabling RPM. Now, Alan explained to me that "suspend events will
> propagate from the I2C clients all the way up to the adapter's parent."
> with RPM enabled for the adapter device. Which should be a no-op if the
> client doesn't do any PM at all? What do I miss?
>
> Thanks,
>
>     Wolfram
>

I'm adding Tony Lindgren to the cc list as well since he is the OMAP
maintainer and I see that has struggled lately with runtime PM issues
so maybe he has more ideas.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Tony Lindgren]

Hi,

* Javier Martinez Canillas <javier@osg.samsung.com> [160212 14:10]:
> Hello,
> 
> On 02/08/2016 07:54 AM, Wolfram Sang wrote:
> >On Wed, Feb 03, 2016 at 10:46:51AM -0300, Javier Martinez Canillas wrote:
> >>Hello Wolfram,
> >>
> >>I've a issue with a I2C video decoder driver (drivers/media/i2c/tvp5150.c).
> >>
> >>In v4.5-rc1, the driver gets I2C read / writes timeouts when accessing the
> >>device I2C registers:
> >>
> >>tvp5150 1-005c: i2c i/o error: rc == -110
> >>tvp5150: probe of 1-005c failed with error -110
> >>
> >>The driver used to work up to v4.4 so this is a regression in v4.5-rc1:
> >>
> >>tvp5150 1-005c: tvp5151 (1.0) chip found @ 0xb8 (OMAP I2C adapter)
> >>tvp5150 1-005c: tvp5151 detected.
> >>
> >>I tracked down to commit 9f924169c035 ("i2c: always enable RuntimePM for
> >>the adapter device") and reverting that commit makes things to work again.
> >>
> >>The tvp5150 driver doesn't have runtime PM support but the I2C controller
> >>driver does (drivers/i2c/busses/i2c-omap.c) FWIW.
> >>
> >>I tried adding runtime PM support to tvp5150 (basically calling pm_runtime
> >>enable/get on probe before the first I2C read to resume the controller) but
> >>that it did not work.
> >>
> >>Not filling the OMAP I2C driver's runtime PM callbacks does not help either.
> >>
> >>Any hints about the proper way to fix this issue?
> >
> >Asking linux-pm for help:
> >
> >The commit in question enables RuntimePM for the logical adapter device
> >which sits between the HW I2C controller and the I2C client device. This
> >adapter device has been used with pm_runtime_no_callbacks before
> >enabling RPM. Now, Alan explained to me that "suspend events will
> >propagate from the I2C clients all the way up to the adapter's parent."
> >with RPM enabled for the adapter device. Which should be a no-op if the
> >client doesn't do any PM at all? What do I miss?
> 
> I'm adding Tony Lindgren to the cc list as well since he is the OMAP
> maintainer and I see that has struggled lately with runtime PM issues
> so maybe he has more ideas.

Hmm yeah I wonder if this canned solution helps here too:

1. Check if the driver(s) are using pm_runtime_use_autosuspend()

2. If so, you must use pm_runtime_dont_use_autosuspend() before
   pm_runtime_put_sync() to make sure that pm_runtime_put_sync()
   works.

3. Or you can use pm_runtime_put_sync_suspend() instead of
   pm_runtime_put_sync() for sections of code where the clocks
   need to be stopped.

Regards,

Tony

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Tony,

On 02/12/2016 07:13 PM, Tony Lindgren wrote:
> Hi,
>
> * Javier Martinez Canillas <javier@osg.samsung.com> [160212 14:10]:
>> Hello,
>>
>> On 02/08/2016 07:54 AM, Wolfram Sang wrote:
>>> On Wed, Feb 03, 2016 at 10:46:51AM -0300, Javier Martinez Canillas wrote:
>>>> Hello Wolfram,
>>>>
>>>> I've a issue with a I2C video decoder driver (drivers/media/i2c/tvp5150.c).
>>>>
>>>> In v4.5-rc1, the driver gets I2C read / writes timeouts when accessing the
>>>> device I2C registers:
>>>>
>>>> tvp5150 1-005c: i2c i/o error: rc == -110
>>>> tvp5150: probe of 1-005c failed with error -110
>>>>
>>>> The driver used to work up to v4.4 so this is a regression in v4.5-rc1:
>>>>
>>>> tvp5150 1-005c: tvp5151 (1.0) chip found @ 0xb8 (OMAP I2C adapter)
>>>> tvp5150 1-005c: tvp5151 detected.
>>>>
>>>> I tracked down to commit 9f924169c035 ("i2c: always enable RuntimePM for
>>>> the adapter device") and reverting that commit makes things to work again.
>>>>
>>>> The tvp5150 driver doesn't have runtime PM support but the I2C controller
>>>> driver does (drivers/i2c/busses/i2c-omap.c) FWIW.
>>>>
>>>> I tried adding runtime PM support to tvp5150 (basically calling pm_runtime
>>>> enable/get on probe before the first I2C read to resume the controller) but
>>>> that it did not work.
>>>>
>>>> Not filling the OMAP I2C driver's runtime PM callbacks does not help either.
>>>>
>>>> Any hints about the proper way to fix this issue?
>>>
>>> Asking linux-pm for help:
>>>
>>> The commit in question enables RuntimePM for the logical adapter device
>>> which sits between the HW I2C controller and the I2C client device. This
>>> adapter device has been used with pm_runtime_no_callbacks before
>>> enabling RPM. Now, Alan explained to me that "suspend events will
>>> propagate from the I2C clients all the way up to the adapter's parent."
>>> with RPM enabled for the adapter device. Which should be a no-op if the
>>> client doesn't do any PM at all? What do I miss?
>>
>> I'm adding Tony Lindgren to the cc list as well since he is the OMAP
>> maintainer and I see that has struggled lately with runtime PM issues
>> so maybe he has more ideas.
>
> Hmm yeah I wonder if this canned solution helps here too:
>
> 1. Check if the driver(s) are using pm_runtime_use_autosuspend()
>

By driver do you mean the OMAP GPIO driver or the tvp5150 I2C driver?
The latter does not have runtime PM support.

> 2. If so, you must use pm_runtime_dont_use_autosuspend() before
>     pm_runtime_put_sync() to make sure that pm_runtime_put_sync()
>     works.
>
> 3. Or you can use pm_runtime_put_sync_suspend() instead of
>     pm_runtime_put_sync() for sections of code where the clocks
>     need to be stopped.
>

I can check if the OMAP GPIO is following these and give a try but
don't have access to the board right now so I'll do it on Monday.

> Regards,
>
> Tony
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Tony Lindgren]

* Javier Martinez Canillas <javier@osg.samsung.com> [160212 14:29]:
> On 02/12/2016 07:13 PM, Tony Lindgren wrote:
> >Hmm yeah I wonder if this canned solution helps here too:
> >
> >1. Check if the driver(s) are using pm_runtime_use_autosuspend()
> >
> 
> By driver do you mean the OMAP GPIO driver or the tvp5150 I2C driver?
> The latter does not have runtime PM support.

Sounds like OMAP GPIO then.

> >2. If so, you must use pm_runtime_dont_use_autosuspend() before
> >    pm_runtime_put_sync() to make sure that pm_runtime_put_sync()
> >    works.
> >
> >3. Or you can use pm_runtime_put_sync_suspend() instead of
> >    pm_runtime_put_sync() for sections of code where the clocks
> >    need to be stopped.
> >
> 
> I can check if the OMAP GPIO is following these and give a try but
> don't have access to the board right now so I'll do it on Monday.

It does not seem to be using pm_runtime_autosuspend(). Did you
try reverting commit de85b9d57ab ("PM / runtime: Re-init runtime
PM states at probe error and driver unbind") and see if that
helps?

If it does, then sounds like we may have some other regression
as well.

Regards,

Tony

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Tony,

On 02/12/2016 07:40 PM, Tony Lindgren wrote:
> * Javier Martinez Canillas <javier@osg.samsung.com> [160212 14:29]:
>> On 02/12/2016 07:13 PM, Tony Lindgren wrote:
>>> Hmm yeah I wonder if this canned solution helps here too:
>>>
>>> 1. Check if the driver(s) are using pm_runtime_use_autosuspend()
>>>
>>
>> By driver do you mean the OMAP GPIO driver or the tvp5150 I2C driver?
>> The latter does not have runtime PM support.
>
> Sounds like OMAP GPIO then.
>

Ok.
  
>>> 2. If so, you must use pm_runtime_dont_use_autosuspend() before
>>>     pm_runtime_put_sync() to make sure that pm_runtime_put_sync()
>>>     works.
>>>
>>> 3. Or you can use pm_runtime_put_sync_suspend() instead of
>>>     pm_runtime_put_sync() for sections of code where the clocks
>>>     need to be stopped.
>>>
>>
>> I can check if the OMAP GPIO is following these and give a try but
>> don't have access to the board right now so I'll do it on Monday.
>
> It does not seem to be using pm_runtime_autosuspend(). Did you
> try reverting commit de85b9d57ab ("PM / runtime: Re-init runtime
> PM states at probe error and driver unbind") and see if that
> helps?
>

Yes, that's the first thing I tried when I noticed your patch:

("i2c: omap: Fix PM regression with deferred probe for
pm_runtime_reinit")

But neither reverting commit de85b9d57ab nor your fix made a
difference.
  
> If it does, then sounds like we may have some other regression
> as well.
>

It seems that is not related but I hope that given you were
looking at the runtime PM core lately, maybe you can figure
out what we are missing.

I'm far from being familiar with the runtime PM framework
but I've looked and can't figure out why Wolfram's commit
make this driver to fail and reverting his commit make its
work again.

> Regards,
>
> Tony
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Tony Lindgren]

* Javier Martinez Canillas <javier@osg.samsung.com> [160212 15:09]:
> Hello Tony,
> 
> On 02/12/2016 07:40 PM, Tony Lindgren wrote:
> >* Javier Martinez Canillas <javier@osg.samsung.com> [160212 14:29]:
> >>On 02/12/2016 07:13 PM, Tony Lindgren wrote:
> >>>Hmm yeah I wonder if this canned solution helps here too:
> >>>
> >>>1. Check if the driver(s) are using pm_runtime_use_autosuspend()
> >>>
> >>
> >>By driver do you mean the OMAP GPIO driver or the tvp5150 I2C driver?
> >>The latter does not have runtime PM support.
> >
> >Sounds like OMAP GPIO then.
> >
> 
> Ok.
> >>>2. If so, you must use pm_runtime_dont_use_autosuspend() before
> >>>    pm_runtime_put_sync() to make sure that pm_runtime_put_sync()
> >>>    works.
> >>>
> >>>3. Or you can use pm_runtime_put_sync_suspend() instead of
> >>>    pm_runtime_put_sync() for sections of code where the clocks
> >>>    need to be stopped.
> >>>
> >>
> >>I can check if the OMAP GPIO is following these and give a try but
> >>don't have access to the board right now so I'll do it on Monday.
> >
> >It does not seem to be using pm_runtime_autosuspend(). Did you
> >try reverting commit de85b9d57ab ("PM / runtime: Re-init runtime
> >PM states at probe error and driver unbind") and see if that
> >helps?
> >
> 
> Yes, that's the first thing I tried when I noticed your patch:
> 
> ("i2c: omap: Fix PM regression with deferred probe for
> pm_runtime_reinit")
> 
> But neither reverting commit de85b9d57ab nor your fix made a
> difference.
> >If it does, then sounds like we may have some other regression
> >as well.

OK I doubt it's the GPIO driver if reverting 9f924169c035
helps.

I do see some GPIO regressions in current Linux next though,
but sounds like you're using v4.5-rc series.

> It seems that is not related but I hope that given you were
> looking at the runtime PM core lately, maybe you can figure
> out what we are missing.
> 
> I'm far from being familiar with the runtime PM framework
> but I've looked and can't figure out why Wolfram's commit
> make this driver to fail and reverting his commit make its
> work again.

No idea. What kind of PM runtime use case has that one been
tested with?

Regards,

Tony

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Tony,

On 02/12/2016 08:46 PM, Tony Lindgren wrote:

[snip]

>
> OK I doubt it's the GPIO driver if reverting 9f924169c035
> helps.
>

Right, sorry. I first thought the problem was with the OMAP GPIO
driver but then found that reverting 9f924169c035 caused the issue
to go away which indicates the problem is somehow related to I2C.

I forgot that and got confused again when answering to your email.

> I do see some GPIO regressions in current Linux next though,
> but sounds like you're using v4.5-rc series.
>

Yes, I'm testing with v4.5-rc and not linux-next.

>> It seems that is not related but I hope that given you were
>> looking at the runtime PM core lately, maybe you can figure
>> out what we are missing.
>>
>> I'm far from being familiar with the runtime PM framework
>> but I've looked and can't figure out why Wolfram's commit
>> make this driver to fail and reverting his commit make its
>> work again.
>
> No idea. What kind of PM runtime use case has that one been
> tested with?
>

Nothing special, the driver just fails to probe. What the driver
does is to toggle some pins (Power Down and Reset) in the tvp5150
chip to put it in an operational state and then attempts to read
an I2C register to detect the chip version and this is what fails.

> Regards,
>
> Tony
>

I'll try to find some time next week to dig deeper on this. Just
thought that may be related to the issue you found but it seems
that's not the case.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]


> I'll try to find some time next week to dig deeper on this. Just
> thought that may be related to the issue you found but it seems
> that's not the case.

Any updates on this?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Wolfram,

On 04/12/2016 06:32 PM, Wolfram Sang wrote:
> 
>> I'll try to find some time next week to dig deeper on this. Just
>> thought that may be related to the issue you found but it seems
>> that's not the case.
> 
> Any updates on this?
>

Sorry, I've been sick and then busy with other stuff so I didn't have time
to dig deeper on this.
 
> Thanks,
> 
>    Wolfram
> 

I'll write what I found so far in case someone with better knowledge about
the runtime PM API and the OMAP I2C controller driver can have an idea of
what could be causing this.

The problem is that after commit 9f924169c035 ("i2c: always enable RuntimePM
for the adapter device"), i2c_smbus_read_byte_data() fails and returns -110
(ETIMEDOUT) error in the tvp5150 I2C driver.

The call trace is the following:

i2c_device_probe
  tvp5150_probe
    tvp5150_read
      i2c_smbus_read_byte_data
        i2c_smbus_xfer
	  i2c_transfer
	    omap_i2c_xfer
              omap_i2c_xfer_msg

The fail in omap_i2c_xfer_msg() is due the wait_for_completion_timeout()
timeout, so it seems the omap_i2c_isr_thread() is not calling complete()
for the cmd_complete completion when runtime PM is enabled (no idea why).

If I revert commit 9f924169c035 then things works again or if I use the
following patch that basically disables runtime PM for the OMAP adapter
after it has been enabled by the I2C core due commit 9f924169c035: 

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 37a88f0ae179..0b72b21b379d 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1445,6 +1445,8 @@ omap_i2c_probe(struct platform_device *pdev)
 		goto err_unuse_clocks;
 	}
 
+	pm_runtime_disable(&adap->dev);
+
 	dev_info(omap->dev, "bus %d rev%d.%d at %d kHz\n", adap->nr,
 		 major, minor, omap->speed);

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]


> I'll write what I found so far in case someone with better knowledge about
> the runtime PM API and the OMAP I2C controller driver can have an idea of
> what could be causing this.

Thanks for the summary. I got no other reports like this, I wonder about
that. That being said, can you try this patch if it makes a change?

http://patchwork.ozlabs.org/patch/609280/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Wolfram,

On 04/14/2016 07:12 AM, Wolfram Sang wrote:
> 
>> I'll write what I found so far in case someone with better knowledge about
>> the runtime PM API and the OMAP I2C controller driver can have an idea of
>> what could be causing this.
> 
> Thanks for the summary. I got no other reports like this, I wonder about

Yes, I also wonder why I'm the only one facing this issue... maybe no one
else is using the tvp5150 driver on an OMAP board with mainline?

> that. That being said, can you try this patch if it makes a change?
> 
> http://patchwork.ozlabs.org/patch/609280/
> 

Unfortunately it doesn't help, I just tried today's -next (next-20160414)
that already has the mentioned patch but the issue is still present.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]


> Yes, I also wonder why I'm the only one facing this issue... maybe no one
> else is using the tvp5150 driver on an OMAP board with mainline?

I wonder why it only affects tvp5150. I don't see the connection yet.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Wofram,

On 04/14/2016 10:19 AM, Wolfram Sang wrote:
> 
>> Yes, I also wonder why I'm the only one facing this issue... maybe no one
>> else is using the tvp5150 driver on an OMAP board with mainline?
> 
> I wonder why it only affects tvp5150. I don't see the connection yet.
> 

Yes, me neither. All other I2C devices are working properly on this board.

The only thing I can think, is that the tvp5150 needs a reset sequence in
order to be operative. It basically toggles two pins in the chip, this is
done in tvp5150_init() [0] and is needed before accessing I2C registers.

Maybe runtime pm has an effect on this and the chip is not reset correctly?

[0]: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/media/i2c/tvp5150.c#n1311

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Tony Lindgren]

Hi,

* Javier Martinez Canillas <javier@osg.samsung.com> [160414 07:28]:
> Hello Wofram,
> 
> On 04/14/2016 10:19 AM, Wolfram Sang wrote:
> > 
> >> Yes, I also wonder why I'm the only one facing this issue... maybe no one
> >> else is using the tvp5150 driver on an OMAP board with mainline?
> > 
> > I wonder why it only affects tvp5150. I don't see the connection yet.
> > 
> 
> Yes, me neither. All other I2C devices are working properly on this board.
> 
> The only thing I can think, is that the tvp5150 needs a reset sequence in
> order to be operative. It basically toggles two pins in the chip, this is
> done in tvp5150_init() [0] and is needed before accessing I2C registers.
> 
> Maybe runtime pm has an effect on this and the chip is not reset correctly?

Is this with omap3 and does tvp5150 have a reset GPIO pin?

If so, you could be hitting the GPIO errata where a glitch can happen
when restoring the GPIO state coming back from off mode in idle. This
happes for GPIO pins that are not in GPIO bank1 and have an external
pull down resistor on the line.

The short term workaround is to mux the reset pin to use the internal
pulls by using PIN_INPUT_PULLUP | MUX_MODE7, or depending on the direction,
PIN_INPUT_PULLDOWN | MUX_MODE7.

The long term workaround is tho have gpio-omap.c do this dynamically
with pinctrl-single.c using gpio-ranges, but that's going to take a
while.. You can search for erratum 1.158 for more info.

Regards,

Tony

> [0]: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/media/i2c/tvp5150.c#n1311

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Tony,

On 04/14/2016 11:12 AM, Tony Lindgren wrote:
> Hi,
> 
> * Javier Martinez Canillas <javier@osg.samsung.com> [160414 07:28]:
>> Hello Wofram,
>>
>> On 04/14/2016 10:19 AM, Wolfram Sang wrote:
>>>
>>>> Yes, I also wonder why I'm the only one facing this issue... maybe no one
>>>> else is using the tvp5150 driver on an OMAP board with mainline?
>>>
>>> I wonder why it only affects tvp5150. I don't see the connection yet.
>>>
>>
>> Yes, me neither. All other I2C devices are working properly on this board.
>>
>> The only thing I can think, is that the tvp5150 needs a reset sequence in
>> order to be operative. It basically toggles two pins in the chip, this is
>> done in tvp5150_init() [0] and is needed before accessing I2C registers.
>>
>> Maybe runtime pm has an effect on this and the chip is not reset correctly?
> 
> Is this with omap3 and does tvp5150 have a reset GPIO pin?
>

Yes, it's a DM3730 (OMAP3) and yes the tvp5150 (actually it's a tvp5151) has
a reset pin that has to be toggled, along with a power-down pin for the chip
to be in an operative state before accessing the I2C registers. That is the
power/reset sequence I mentioned before.
 
> If so, you could be hitting the GPIO errata where a glitch can happen
> when restoring the GPIO state coming back from off mode in idle. This
> happes for GPIO pins that are not in GPIO bank1 and have an external
> pull down resistor on the line.
>

The GPIO lines connected to these pins are:

GPIO126 (bank4 pin 30) -> tvp5150 power-down pin
GPIO167 (bank6 pin 7)  -> tvp5150 reset pin

Neither are in GPIO bank1 so they could be affected by the errata you
mention but there isn't external pull down (or up) resistors on these
lines AFAICT by looking at the board schematics. I've added to the cc
list to other people that are familiar with the board in case I missed
something.

> The short term workaround is to mux the reset pin to use the internal
> pulls by using PIN_INPUT_PULLUP | MUX_MODE7, or depending on the direction,
> PIN_INPUT_PULLDOWN | MUX_MODE7.
>

I guess you meant MUX_MODE4 here since the pin has to be in GPIO mode?

Also, I wonder how the issue could be related to the GPIO controller
since is when enabling runtime PM for the I2C controller that things
fail. IOW, disabling runtime PM for the I2C adapter shouldn't make
things to work if the problem was caused by the mentioned GPIO errata.

In any case, I've tried to use the internal pulls as you suggested but
that didn't solve the issue.

> The long term workaround is tho have gpio-omap.c do this dynamically
> with pinctrl-single.c using gpio-ranges, but that's going to take a
> while.. You can search for erratum 1.158 for more info.
> 
> Regards,
> 
> Tony
> 
>> [0]: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/media/i2c/tvp5150.c#n1311

I'm really out of ideas on this, I don't think that Wolfram change has
to be reverted since it seems I'm the only one facing this issue and
also the DTS for the board I'm using is not even in mainline yet.

I'm waiting for some discussions about the Media Controller input
connectors DT bindings to settle before posting the board DTS patches.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Tony Lindgren]

* Javier Martinez Canillas <javier@osg.samsung.com> [160414 17:00]:
> On 04/14/2016 11:12 AM, Tony Lindgren wrote:
> > Is this with omap3 and does tvp5150 have a reset GPIO pin?
> 
> Yes, it's a DM3730 (OMAP3) and yes the tvp5150 (actually it's a tvp5151) has
> a reset pin that has to be toggled, along with a power-down pin for the chip
> to be in an operative state before accessing the I2C registers. That is the
> power/reset sequence I mentioned before.
>  
> > If so, you could be hitting the GPIO errata where a glitch can happen
> > when restoring the GPIO state coming back from off mode in idle. This
> > happes for GPIO pins that are not in GPIO bank1 and have an external
> > pull down resistor on the line.
> >
> 
> The GPIO lines connected to these pins are:
> 
> GPIO126 (bank4 pin 30) -> tvp5150 power-down pin
> GPIO167 (bank6 pin 7)  -> tvp5150 reset pin
> 
> Neither are in GPIO bank1 so they could be affected by the errata you
> mention but there isn't external pull down (or up) resistors on these
> lines AFAICT by looking at the board schematics. I've added to the cc
> list to other people that are familiar with the board in case I missed
> something.
> 
> > The short term workaround is to mux the reset pin to use the internal
> > pulls by using PIN_INPUT_PULLUP | MUX_MODE7, or depending on the direction,
> > PIN_INPUT_PULLDOWN | MUX_MODE7.
> 
> I guess you meant MUX_MODE4 here since the pin has to be in GPIO mode?

No, the glitch affects the GPIO mode, so that's why direction + pull +
safe mode is needed.

> Also, I wonder how the issue could be related to the GPIO controller
> since is when enabling runtime PM for the I2C controller that things
> fail. IOW, disabling runtime PM for the I2C adapter shouldn't make
> things to work if the problem was caused by the mentioned GPIO errata.

If you block PM runtime for I2C, then it blocks deeper idle states
for the whole device. Note that you can disable off mode during idle
and suspend with:

# echo 0 > /sys/kernel/debug/pm_debug/enable_off_mode

> In any case, I've tried to use the internal pulls as you suggested but
> that didn't solve the issue.

OK. Just to be sure.. Did you use the safe mode mux option instead
of the GPIO mux option?

Note that in some cases the internal pull is not strong enough to
keep the reset line up if there's an external pull down resistor.

Regards,

Tony

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Tony,

On 04/15/2016 10:58 AM, Tony Lindgren wrote:
>>
>>> The short term workaround is to mux the reset pin to use the internal
>>> pulls by using PIN_INPUT_PULLUP | MUX_MODE7, or depending on the direction,
>>> PIN_INPUT_PULLDOWN | MUX_MODE7.
>>
>> I guess you meant MUX_MODE4 here since the pin has to be in GPIO mode?
> 
> No, the glitch affects the GPIO mode, so that's why direction + pull +
> safe mode is needed.
>

Ah ok, thanks for the explanation and sorry for the confusion.
 
>> Also, I wonder how the issue could be related to the GPIO controller
>> since is when enabling runtime PM for the I2C controller that things
>> fail. IOW, disabling runtime PM for the I2C adapter shouldn't make
>> things to work if the problem was caused by the mentioned GPIO errata.
> 
> If you block PM runtime for I2C, then it blocks deeper idle states
> for the whole device. Note that you can disable off mode during idle

Thanks again for this clarification.

> and suspend with:
> 
> # echo 0 > /sys/kernel/debug/pm_debug/enable_off_mode
>

I see thought that enable_off_mode is 0 by default when booting the board:

# cat /sys/kernel/debug/pm_debug/enable_off_mode 
0

So if I understood your explanation correctly, that means that the glitch
should not happen for the GPIO pins since the machine doesn't enter into
deeper idle states that could cause the glitch from erratum 1.158?
 
>> In any case, I've tried to use the internal pulls as you suggested but
>> that didn't solve the issue.
> 
> OK. Just to be sure.. Did you use the safe mode mux option instead
> of the GPIO mux option?
>

No sorry, I tested with the GPIO mux mode before since I misunderstood
your previous email. But now I've tested with safe mode mux and didn't
make a difference (which was expected since off mode is disabled AFAIU). 
 
> Note that in some cases the internal pull is not strong enough to
> keep the reset line up if there's an external pull down resistor.
>

Yes, I got that from your previous email but as I mentioned, the lines
in the board don't have external pull resistors.
 
> Regards,
> 
> Tony
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: tvp5150 regression after commit 9f924169c035

[Tony Lindgren]

* Javier Martinez Canillas <javier@osg.samsung.com> [160415 09:50]:
> On 04/15/2016 10:58 AM, Tony Lindgren wrote:
> > If you block PM runtime for I2C, then it blocks deeper idle states
> > for the whole device. Note that you can disable off mode during idle
> 
> Thanks again for this clarification.
> 
> > and suspend with:
> > 
> > # echo 0 > /sys/kernel/debug/pm_debug/enable_off_mode
> >
> 
> I see thought that enable_off_mode is 0 by default when booting the board:
> 
> # cat /sys/kernel/debug/pm_debug/enable_off_mode 
> 0

OK so you're not hitting off mode then.

> So if I understood your explanation correctly, that means that the glitch
> should not happen for the GPIO pins since the machine doesn't enter into
> deeper idle states that could cause the glitch from erratum 1.158?

Correct. But you could still have a dependency to some other
device driver that stays active if I2C keeps the whole system
from hitting retention mode during idle.

Regards,

Tony

Re: tvp5150 regression after commit 9f924169c035

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]


> I'm adding Tony Lindgren to the cc list as well since he is the OMAP
> maintainer and I see that has struggled lately with runtime PM issues
> so maybe he has more ideas.

Good idea. Did you try his patch which is in my for-current branch
("i2c: omap: Fix PM regression with deferred probe for
pm_runtime_reinit")?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: tvp5150 regression after commit 9f924169c035

[Javier Martinez Canillas]

Hello Wolfram,

On 02/12/2016 07:22 PM, Wolfram Sang wrote:
>
>> I'm adding Tony Lindgren to the cc list as well since he is the OMAP
>> maintainer and I see that has struggled lately with runtime PM issues
>> so maybe he has more ideas.
>
> Good idea. Did you try his patch which is in my for-current branch
> ("i2c: omap: Fix PM regression with deferred probe for
> pm_runtime_reinit")?
>

Yes I did but unfortunately it did not help in this case.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America