Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1203177 > unrolled thread

Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect pm_runtime_get_sync() in probe function.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. Alexander Holler <holler@ahsoftware.de> - 2015-08-08 08:00 +0200
    Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect pm_runtime_get_sync()  in probe function. Kishon Vijay Abraham I <kishon@ti.com> - 2015-08-08 08:50 +0200
      Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. NeilBrown <neil@brown.name> - 2015-08-09 11:10 +0200
        Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. Alexander Holler <holler@ahsoftware.de> - 2015-08-09 12:50 +0200
          Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. NeilBrown <neil@brown.name> - 2015-08-11 10:30 +0200
            Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. Alexander Holler <holler@ahsoftware.de> - 2015-08-11 23:40 +0200
              Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. NeilBrown <neil@brown.name> - 2015-08-12 00:10 +0200
                Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect  pm_runtime_get_sync() in probe function. Felipe Balbi <balbi@ti.com> - 2015-08-12 02:30 +0200

#1203177 — Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect pm_runtime_get_sync() in probe function.

Hello,

this patch killed the musb-host functionality on my classic Beagleboard 
(rev c4). Symptom was that it there was a message I don't remember and 
the attached device didn't enumerate anymore (likely because of missing 
power, but I'm not sure).

A simple revert has fixed it, I haven't looked further into the problem.

Regards,

Alexander Holler

Am 31.07.2015 um 21:39 schrieb Greg Kroah-Hartman:
> 4.1-stable review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: NeilBrown <neil@brown.name>
>
> commit 4724e27114c4a7eceeee07db227a17fcab6f165c upstream.
>
> The USB phy should initialize with power-off, and will be powered on
> by the USB system when a cable connection is detected.
>
> Having this pm_runtime_get_sync() during probe causes the phy to
> *always* be powered on.
> Removing it returns to sensible power management.
>
> Fixes: 96be39ab34b77c6f6f5cd6ae03aac6c6449ee5c4
> Signed-off-by: NeilBrown <neil@brown.name>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>   drivers/phy/phy-twl4030-usb.c |    1 -
>   1 file changed, 1 deletion(-)
>
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -711,7 +711,6 @@ static int twl4030_usb_probe(struct plat
>   	pm_runtime_use_autosuspend(&pdev->dev);
>   	pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
>   	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(&pdev->dev);
>
>   	/* Our job is to use irqs and status from the power module
>   	 * to keep the transceiver disabled when nothing's connected.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1203184 — Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect pm_runtime_get_sync() in probe function.

On Saturday 08 August 2015 11:23 AM, Alexander Holler wrote:
> Hello,
> 
> this patch killed the musb-host functionality on my classic Beagleboard (rev
> c4). Symptom was that it there was a message I don't remember and the attached
> device didn't enumerate anymore (likely because of missing power, but I'm not
> sure).
> 
> A simple revert has fixed it, I haven't looked further into the problem.

Neil Brown, how was this tested?

-Kishon

> 
> Regards,
> 
> Alexander Holler
> 
> Am 31.07.2015 um 21:39 schrieb Greg Kroah-Hartman:
>> 4.1-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: NeilBrown <neil@brown.name>
>>
>> commit 4724e27114c4a7eceeee07db227a17fcab6f165c upstream.
>>
>> The USB phy should initialize with power-off, and will be powered on
>> by the USB system when a cable connection is detected.
>>
>> Having this pm_runtime_get_sync() during probe causes the phy to
>> *always* be powered on.
>> Removing it returns to sensible power management.
>>
>> Fixes: 96be39ab34b77c6f6f5cd6ae03aac6c6449ee5c4
>> Signed-off-by: NeilBrown <neil@brown.name>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> ---
>>   drivers/phy/phy-twl4030-usb.c |    1 -
>>   1 file changed, 1 deletion(-)
>>
>> --- a/drivers/phy/phy-twl4030-usb.c
>> +++ b/drivers/phy/phy-twl4030-usb.c
>> @@ -711,7 +711,6 @@ static int twl4030_usb_probe(struct plat
>>       pm_runtime_use_autosuspend(&pdev->dev);
>>       pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
>>       pm_runtime_enable(&pdev->dev);
>> -    pm_runtime_get_sync(&pdev->dev);
>>
>>       /* Our job is to use irqs and status from the power module
>>        * to keep the transceiver disabled when nothing's connected.
>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203609

On Sat, 8 Aug 2015 12:18:55 +0530 Kishon Vijay Abraham I
<kishon@ti.com> wrote:

> 
> 
> On Saturday 08 August 2015 11:23 AM, Alexander Holler wrote:
> > Hello,
> > 
> > this patch killed the musb-host functionality on my classic Beagleboard (rev
> > c4). Symptom was that it there was a message I don't remember and the attached
> > device didn't enumerate anymore (likely because of missing power, but I'm not
> > sure).
> > 
> > A simple revert has fixed it, I haven't looked further into the problem.
> 
> Neil Brown, how was this tested?


Well, I have a board with an OMAP3 connected to a twl4030 for USB and I
noted that it wasn't power-managed properly and when I made that change,
it was.  I don't recall the exact details

This is probably related to

Commit: 56301df6bcaa ("phy: twl4030-usb: make runtime pm more reliable.")

I certainly only tested with that patch in place.

Looking at the code again, I cannot see why I thought I needed the
4724e277 patch.
Devices are initialised as RPM_SUSPENDED with a usage count of zero (in
pm_runtime_init) so when twl4030_usb_probe() calls
pm_runtime_get_sync(), that will increase the usage_count to 1 and
resume the device.  Then it calls pm_runtime_put_autosuspend() which
will (ultimately) drop the usage_count back to zero and suspend the
device.

So it looks like I messed up, but I'm sure I must have had a reason for
that patch.  I'll try to do some testing later in the week and will
report what I find.

NeilBrown


> 
> -Kishon
> 
> > 
> > Regards,
> > 
> > Alexander Holler
> > 
> > Am 31.07.2015 um 21:39 schrieb Greg Kroah-Hartman:
> >> 4.1-stable review patch.  If anyone has any objections, please let me know.
> >>
> >> ------------------
> >>
> >> From: NeilBrown <neil@brown.name>
> >>
> >> commit 4724e27114c4a7eceeee07db227a17fcab6f165c upstream.
> >>
> >> The USB phy should initialize with power-off, and will be powered on
> >> by the USB system when a cable connection is detected.
> >>
> >> Having this pm_runtime_get_sync() during probe causes the phy to
> >> *always* be powered on.
> >> Removing it returns to sensible power management.
> >>
> >> Fixes: 96be39ab34b77c6f6f5cd6ae03aac6c6449ee5c4
> >> Signed-off-by: NeilBrown <neil@brown.name>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>
> >> ---
> >>   drivers/phy/phy-twl4030-usb.c |    1 -
> >>   1 file changed, 1 deletion(-)
> >>
> >> --- a/drivers/phy/phy-twl4030-usb.c
> >> +++ b/drivers/phy/phy-twl4030-usb.c
> >> @@ -711,7 +711,6 @@ static int twl4030_usb_probe(struct plat
> >>       pm_runtime_use_autosuspend(&pdev->dev);
> >>       pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
> >>       pm_runtime_enable(&pdev->dev);
> >> -    pm_runtime_get_sync(&pdev->dev);
> >>
> >>       /* Our job is to use irqs and status from the power module
> >>        * to keep the transceiver disabled when nothing's connected.
> >>
> >>
> >> -- 
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at  http://www.tux.org/lkml/
> >>
> > 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203629

Am 09.08.2015 um 11:00 schrieb NeilBrown:
> On Sat, 8 Aug 2015 12:18:55 +0530 Kishon Vijay Abraham I
> <kishon@ti.com> wrote:
>
>>
>>
>> On Saturday 08 August 2015 11:23 AM, Alexander Holler wrote:
>>> Hello,
>>>
>>> this patch killed the musb-host functionality on my classic Beagleboard (rev
>>> c4). Symptom was that it there was a message I don't remember and the attached
>>> device didn't enumerate anymore (likely because of missing power, but I'm not
>>> sure).
>>>
>>> A simple revert has fixed it, I haven't looked further into the problem.
>>
>> Neil Brown, how was this tested?
>
>
> Well, I have a board with an OMAP3 connected to a twl4030 for USB and I
> noted that it wasn't power-managed properly and when I made that change,
> it was.  I don't recall the exact details
>
> This is probably related to
>
> Commit: 56301df6bcaa ("phy: twl4030-usb: make runtime pm more reliable.")
>
> I certainly only tested with that patch in place.

Cherry-Picking 56301df6bcaa instead of reverting  d1221a608bd did the 
trick too. So it looks like 56301df6bcaa is indeed a prerequisit for 
d1221a608bd.

Therefor I suggest to feed 56301df6bcaa to the stable series (e.g. 
4.1.6) too.

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1204872

On Sun, 9 Aug 2015 12:45:20 +0200 Alexander Holler
<holler@ahsoftware.de> wrote:

> Am 09.08.2015 um 11:00 schrieb NeilBrown:
> > On Sat, 8 Aug 2015 12:18:55 +0530 Kishon Vijay Abraham I
> > <kishon@ti.com> wrote:
> >
> >>
> >>
> >> On Saturday 08 August 2015 11:23 AM, Alexander Holler wrote:
> >>> Hello,
> >>>
> >>> this patch killed the musb-host functionality on my classic Beagleboard (rev
> >>> c4). Symptom was that it there was a message I don't remember and the attached
> >>> device didn't enumerate anymore (likely because of missing power, but I'm not
> >>> sure).
> >>>
> >>> A simple revert has fixed it, I haven't looked further into the problem.
> >>
> >> Neil Brown, how was this tested?
> >
> >
> > Well, I have a board with an OMAP3 connected to a twl4030 for USB and I
> > noted that it wasn't power-managed properly and when I made that change,
> > it was.  I don't recall the exact details
> >
> > This is probably related to
> >
> > Commit: 56301df6bcaa ("phy: twl4030-usb: make runtime pm more reliable.")
> >
> > I certainly only tested with that patch in place.
> 
> Cherry-Picking 56301df6bcaa instead of reverting  d1221a608bd did the 
> trick too. So it looks like 56301df6bcaa is indeed a prerequisit for 
> d1221a608bd.
> 
> Therefor I suggest to feed 56301df6bcaa to the stable series (e.g. 
> 4.1.6) too.
> 

The reality is ... more complicated.

I had a close look at how refcounts are inc/dec for the twl4030 phy.

With the current mainline code (plus my twl4030 charger enhancements,
which are not deeply relevant), the refcount does go to zero when
nothing is plugged in, and goes to 2 when a regular USB cable is
plugged in.
The two counts come from twl4030_usb_irq and twl4030_charger_enable_usb,
which is what I would expect.

However at the end of twl4030_usb_probe, the count goes to -1 !!!
because of the pm_runtime_put_autosuspend, which no longer has a
balancing pm_runtime_get() -  which I really shouldn't have removed.

The extra refcount that I saw before and blamed on that
pm_runtime_get() actually comes from a phy_power_on() call in
omap2430_musb_init.

omap2430_musb_init() calls phy_power_on(), and doesn't call
phy_power_off() until omap2430_musb_exit().
So it tries to keep the phy on the entire time that the module is
loaded.

Do we want to just remove the phy_power_on() call from
omap2430_musb_init()?
That seems to work for me, but may well break on other boards.

I think the best thing to do for -stable it to leave 56301df6bcaa out
and revert the backport of d1221a608bd.
That will return to a state which, while not perfect, at least is not a
regression.

With that (older) code, the extra phy_power_on() call still increases
the usage_count, but the irq_handler in the twl4030 phy driver will
drop it down to zero without first increasing.  So things work for the
wrong reasons.

Felipe: you added the phy_power_on() call in 

 Commit: 3063a12be2b0 ("usb: musb: fix PHY power on/off")

Do we really want the phy to be on the whole time the modules is loaded?
If not, how/when should the phy be powered down?

Thanks,
NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205444

Am 11.08.2015 um 10:29 schrieb NeilBrown:

> With the current mainline code (plus my twl4030 charger enhancements,
> which are not deeply relevant), the refcount does go to zero when
> nothing is plugged in, and goes to 2 when a regular USB cable is
> plugged in.

Also I think it's just a little miswording (or extended typo), I'm not 
using a regular USB cable, but an OTG cable when using the host mode of 
the musb on the Beagleboard. Just to avoid confusion. Or do you talk 
about the client mode when it goes to 2?

Besides that, I'm sorry to not be of further help. My deeper knowledge 
about the musb sources in the kernel and u-boot (and the HW in 
question), has gone lost and I've currently no real reason to refresh 
that. ;)

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205457

On Tue, 11 Aug 2015 23:34:17 +0200 Alexander Holler
<holler@ahsoftware.de> wrote:

> Am 11.08.2015 um 10:29 schrieb NeilBrown:
> 
> > With the current mainline code (plus my twl4030 charger enhancements,
> > which are not deeply relevant), the refcount does go to zero when
> > nothing is plugged in, and goes to 2 when a regular USB cable is
> > plugged in.
> 
> Also I think it's just a little miswording (or extended typo), I'm not 
> using a regular USB cable, but an OTG cable when using the host mode of 
> the musb on the Beagleboard. Just to avoid confusion. Or do you talk 
> about the client mode when it goes to 2?

With an OTG cable, the usage_count would probably got to '1' because
the charger wouldn't be interested.

However the important point is that the usage_count goes to zero when
no cable is plugged in.

> 
> Besides that, I'm sorry to not be of further help. My deeper knowledge 
> about the musb sources in the kernel and u-boot (and the HW in 
> question), has gone lost and I've currently no real reason to refresh 
> that. ;)

Just reporting the problem is sometimes 90% for the bug fixed.
I'm confident I know what is happening.  What isn't clear is how best
to fix it without breaking something else... I suspect Felipe can help
with that.

Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205519

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Wed, Aug 12, 2015 at 08:09:27AM +1000, NeilBrown wrote:
> On Tue, 11 Aug 2015 23:34:17 +0200 Alexander Holler
> <holler@ahsoftware.de> wrote:
> 
> > Am 11.08.2015 um 10:29 schrieb NeilBrown:
> > 
> > > With the current mainline code (plus my twl4030 charger enhancements,
> > > which are not deeply relevant), the refcount does go to zero when
> > > nothing is plugged in, and goes to 2 when a regular USB cable is
> > > plugged in.
> > 
> > Also I think it's just a little miswording (or extended typo), I'm not 
> > using a regular USB cable, but an OTG cable when using the host mode of 
> > the musb on the Beagleboard. Just to avoid confusion. Or do you talk 
> > about the client mode when it goes to 2?
> 
> With an OTG cable, the usage_count would probably got to '1' because
> the charger wouldn't be interested.
> 
> However the important point is that the usage_count goes to zero when
> no cable is plugged in.
> 
> > 
> > Besides that, I'm sorry to not be of further help. My deeper knowledge 
> > about the musb sources in the kernel and u-boot (and the HW in 
> > question), has gone lost and I've currently no real reason to refresh 
> > that. ;)
> 
> Just reporting the problem is sometimes 90% for the bug fixed.
> I'm confident I know what is happening.  What isn't clear is how best
> to fix it without breaking something else... I suspect Felipe can help
> with that.

It'll take me a while to be able to look at this. I don't have any
platform with TWL4030 (IIRC, AM335x uses tps65218 instead). Besides, I
have a slightly bigger problem to look at (MUSB + some hubs + CP2108
fails sometimes).

If I get some time, I'll try to have a look, but it might take a few
weeks.

cheers

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web