Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* Andreas Kemnade  [160808 22:36]:
> Calls to musb_platform_enable() occur at only 1 place.
> musb_platform_disable() is called at 4 places.
> 
> about balancing:
> There is musb_start() and musb_stop(). They are called from
> musb_gadget_start/stop()
> These call musb_platform_enable() and musb_platform_disable().
> Looks ok.
> 
> There is musb_suspend() and musb_resume():
> 
> musb_suspend() calls musb_platform_disable()
> musb_resume() calls musb_plaform_enable() via musb_start()
> looks balanced but why don't we use musb_stop() in musb_suspend()?

Hmm let's try adding musb_stop() to musb_suspend() too.

> Now the odd things:
> musb_platform_disable() in musb_remove() called upon module removal
> musb_platform_disable() in musb_init_controller() called from
> musb_probe()
> 
> This looks clearly unbalanced.

Sure would be nice to get those balanced. I think the only
reason why musb_platform_disable() is called is to disable
interrupts.

Care to post a patch and let's see what happens? I can now
easily test the PM with musb.

Regards,

TOny

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* Andreas Kemnade  [160808 22:36]:
> Calls to musb_platform_enable() occur at only 1 place.
> musb_platform_disable() is called at 4 places.
> 
> about balancing:
> There is musb_start() and musb_stop(). They are called from
> musb_gadget_start/stop()
> These call musb_platform_enable() and musb_platform_disable().
> Looks ok.
> 
> There is musb_suspend() and musb_resume():
> 
> musb_suspend() calls musb_platform_disable()
> musb_resume() calls musb_plaform_enable() via musb_start()
> looks balanced but why don't we use musb_stop() in musb_suspend()?

Hmm let's try adding musb_stop() to musb_suspend() too.

> Now the odd things:
> musb_platform_disable() in musb_remove() called upon module removal
> musb_platform_disable() in musb_init_controller() called from
> musb_probe()
> 
> This looks clearly unbalanced.

Sure would be nice to get those balanced. I think the only
reason why musb_platform_disable() is called is to disable
interrupts.

Care to post a patch and let's see what happens? I can now
easily test the PM with musb.

Regards,

TOny

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Fri, 5 Aug 2016 23:21:35 -0700
Tony Lindgren  wrote:

> * Andreas Kemnade  [160805 08:35]:
> > I repeat the subject line of the patch:
> > [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
> > It is *not* fix charging.
> > 
> > So you mean that the phy should magically know at which refcounter
> > it should power on and off if power on/off is not called in the
> > balanced way?
> 
> No, I mean we need to figure out why things can be called in
> unbalanced way. With your patch I end up with unbalanced calls
> leaving the phy on after all the modules have been removed :)
> 
Well, causing trouble when modules are removed was not the intention.
I was just happy to have things working a bit again.
The phy is powered on/off via
musb_platform_enable()/musb_platform_disable().

Calls to musb_platform_enable() occur at only 1 place.
musb_platform_disable() is called at 4 places.

about balancing:
There is musb_start() and musb_stop(). They are called from
musb_gadget_start/stop()
These call musb_platform_enable() and musb_platform_disable().
Looks ok.

There is musb_suspend() and musb_resume():

musb_suspend() calls musb_platform_disable()
musb_resume() calls musb_plaform_enable() via musb_start()
looks balanced but why don't we use musb_stop() in musb_suspend()?

Now the odd things:
musb_platform_disable() in musb_remove() called upon module removal
musb_platform_disable() in musb_init_controller() called from
musb_probe()

This looks clearly unbalanced.


> > Maybe the phy-twl4030 uses the phy layer wrong. 
> > Now the relevant part of power on/off in phy-twl4030 is done via
> > struct phy_ops.power_off/power_on (*not* via pm_runtime). Maybe
> > that is wrong and more parts should be moved to the pm_runtime
> > stuff (which is also present). 
> 
> We should use phy power_off/power_on for the USB related parts.
> The parts needed by other components, like VBUS detection, should
> be handled by PM runtime. We should get phy-twl4030-usb and the
> charger driver working also when no musb modules are loaded.
> 
> > Then the phy subsystem has its own power refcounter in struct
> > phy.power_count. It it handled via phy_power_off()/phy_power_on().
> > And that is called from musb/omap2430.c 
> > But that is another story. 
> 
> Yes that's what the USB driver is expected to do. But obviously
> there are issues remaining also in the phy-twl4030-usb.c driver.
> And it seems that we should have some OTG parts always enabled
> when VBUS is detected when twl4030-charger is configured?
> 
Seems so. I am writing a patch for it.

> > > If there are MUSB specific PM runtime issues then let's fix
> > > those separately.
> > > 
> > And that exactly tries my patch to do. For that task it does not
> > even use the PM runtime system. Again please read the subject line
> > of the patch. Maybe it unveils some other pm issues in musb
> > which should first be fixed in a complete patch series.
> 
> Certainly that needs to be fixed, but let's do it in a way where
> things work for other test cases also. Care to describe how to
> to reproduce the issue you're seeing? It seems that you are
> seeing devices not being enmerated leading to the charger not
> working? Is this with built in MUSB and phy modules?
> 
Both as modules. I added some debug output to the driver/phy/phy-core.c
and have seen the phy->power_count sticking at -1 or 0. 
g_ether is also loaded.
Gadget stops for me (device not showing up at the other side) at 4.7rc4.
But I remember Nikolaus having the situation on the same type of device
that it was important on which side you replug the usb cable
(probably causing some timing differences) with 4.7rc1.

Regards,
Andreas


pgpz33vMleAJR.pgp
Description: OpenPGP digital signature

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Fri, 5 Aug 2016 23:21:35 -0700
Tony Lindgren  wrote:

> * Andreas Kemnade  [160805 08:35]:
> > I repeat the subject line of the patch:
> > [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
> > It is *not* fix charging.
> > 
> > So you mean that the phy should magically know at which refcounter
> > it should power on and off if power on/off is not called in the
> > balanced way?
> 
> No, I mean we need to figure out why things can be called in
> unbalanced way. With your patch I end up with unbalanced calls
> leaving the phy on after all the modules have been removed :)
> 
Well, causing trouble when modules are removed was not the intention.
I was just happy to have things working a bit again.
The phy is powered on/off via
musb_platform_enable()/musb_platform_disable().

Calls to musb_platform_enable() occur at only 1 place.
musb_platform_disable() is called at 4 places.

about balancing:
There is musb_start() and musb_stop(). They are called from
musb_gadget_start/stop()
These call musb_platform_enable() and musb_platform_disable().
Looks ok.

There is musb_suspend() and musb_resume():

musb_suspend() calls musb_platform_disable()
musb_resume() calls musb_plaform_enable() via musb_start()
looks balanced but why don't we use musb_stop() in musb_suspend()?

Now the odd things:
musb_platform_disable() in musb_remove() called upon module removal
musb_platform_disable() in musb_init_controller() called from
musb_probe()

This looks clearly unbalanced.


> > Maybe the phy-twl4030 uses the phy layer wrong. 
> > Now the relevant part of power on/off in phy-twl4030 is done via
> > struct phy_ops.power_off/power_on (*not* via pm_runtime). Maybe
> > that is wrong and more parts should be moved to the pm_runtime
> > stuff (which is also present). 
> 
> We should use phy power_off/power_on for the USB related parts.
> The parts needed by other components, like VBUS detection, should
> be handled by PM runtime. We should get phy-twl4030-usb and the
> charger driver working also when no musb modules are loaded.
> 
> > Then the phy subsystem has its own power refcounter in struct
> > phy.power_count. It it handled via phy_power_off()/phy_power_on().
> > And that is called from musb/omap2430.c 
> > But that is another story. 
> 
> Yes that's what the USB driver is expected to do. But obviously
> there are issues remaining also in the phy-twl4030-usb.c driver.
> And it seems that we should have some OTG parts always enabled
> when VBUS is detected when twl4030-charger is configured?
> 
Seems so. I am writing a patch for it.

> > > If there are MUSB specific PM runtime issues then let's fix
> > > those separately.
> > > 
> > And that exactly tries my patch to do. For that task it does not
> > even use the PM runtime system. Again please read the subject line
> > of the patch. Maybe it unveils some other pm issues in musb
> > which should first be fixed in a complete patch series.
> 
> Certainly that needs to be fixed, but let's do it in a way where
> things work for other test cases also. Care to describe how to
> to reproduce the issue you're seeing? It seems that you are
> seeing devices not being enmerated leading to the charger not
> working? Is this with built in MUSB and phy modules?
> 
Both as modules. I added some debug output to the driver/phy/phy-core.c
and have seen the phy->power_count sticking at -1 or 0. 
g_ether is also loaded.
Gadget stops for me (device not showing up at the other side) at 4.7rc4.
But I remember Nikolaus having the situation on the same type of device
that it was important on which side you replug the usb cable
(probably causing some timing differences) with 4.7rc1.

Regards,
Andreas


pgpz33vMleAJR.pgp
Description: OpenPGP digital signature

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* Andreas Kemnade  [160805 08:35]:
> I repeat the subject line of the patch:
> [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
> It is *not* fix charging.
> 
> So you mean that the phy should magically know at which refcounter
> it should power on and off if power on/off is not called in the
> balanced way?

No, I mean we need to figure out why things can be called in
unbalanced way. With your patch I end up with unbalanced calls
leaving the phy on after all the modules have been removed :)

> Maybe the phy-twl4030 uses the phy layer wrong. 
> Now the relevant part of power on/off in phy-twl4030 is done via struct
> phy_ops.power_off/power_on (*not* via pm_runtime). Maybe that is wrong
> and more parts should be moved to the pm_runtime stuff (which is also
> present). 

We should use phy power_off/power_on for the USB related parts.
The parts needed by other components, like VBUS detection, should
be handled by PM runtime. We should get phy-twl4030-usb and the
charger driver working also when no musb modules are loaded.

> Then the phy subsystem has its own power refcounter in struct
> phy.power_count. It it handled via phy_power_off()/phy_power_on().
> And that is called from musb/omap2430.c 
> But that is another story. 

Yes that's what the USB driver is expected to do. But obviously
there are issues remaining also in the phy-twl4030-usb.c driver.
And it seems that we should have some OTG parts always enabled
when VBUS is detected when twl4030-charger is configured?

> > If there are MUSB specific PM runtime issues then let's fix
> > those separately.
> > 
> And that exactly tries my patch to do. For that task it does not
> even use the PM runtime system. Again please read the subject line of
> the patch. Maybe it unveils some other pm issues in musb
> which should first be fixed in a complete patch series.

Certainly that needs to be fixed, but let's do it in a way where
things work for other test cases also. Care to describe how to
to reproduce the issue you're seeing? It seems that you are
seeing devices not being enmerated leading to the charger not
working? Is this with built in MUSB and phy modules?

Regrds,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* Andreas Kemnade  [160805 08:35]:
> I repeat the subject line of the patch:
> [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
> It is *not* fix charging.
> 
> So you mean that the phy should magically know at which refcounter
> it should power on and off if power on/off is not called in the
> balanced way?

No, I mean we need to figure out why things can be called in
unbalanced way. With your patch I end up with unbalanced calls
leaving the phy on after all the modules have been removed :)

> Maybe the phy-twl4030 uses the phy layer wrong. 
> Now the relevant part of power on/off in phy-twl4030 is done via struct
> phy_ops.power_off/power_on (*not* via pm_runtime). Maybe that is wrong
> and more parts should be moved to the pm_runtime stuff (which is also
> present). 

We should use phy power_off/power_on for the USB related parts.
The parts needed by other components, like VBUS detection, should
be handled by PM runtime. We should get phy-twl4030-usb and the
charger driver working also when no musb modules are loaded.

> Then the phy subsystem has its own power refcounter in struct
> phy.power_count. It it handled via phy_power_off()/phy_power_on().
> And that is called from musb/omap2430.c 
> But that is another story. 

Yes that's what the USB driver is expected to do. But obviously
there are issues remaining also in the phy-twl4030-usb.c driver.
And it seems that we should have some OTG parts always enabled
when VBUS is detected when twl4030-charger is configured?

> > If there are MUSB specific PM runtime issues then let's fix
> > those separately.
> > 
> And that exactly tries my patch to do. For that task it does not
> even use the PM runtime system. Again please read the subject line of
> the patch. Maybe it unveils some other pm issues in musb
> which should first be fixed in a complete patch series.

Certainly that needs to be fixed, but let's do it in a way where
things work for other test cases also. Care to describe how to
to reproduce the issue you're seeing? It seems that you are
seeing devices not being enmerated leading to the charger not
working? Is this with built in MUSB and phy modules?

Regrds,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Fri, 5 Aug 2016 06:55:01 -0700
Tony Lindgren  wrote:

> * Andreas Kemnade  [160804 09:44]:
> > Nothing happens here, so the previous state of the phy remains.
> > It would be disabled by the generic phy layer in
> > drivers/phy/phy-core.c
> >
> > > gadget driver is loaded.
> > > musb_start() is called
> > > omap2430_musb_enable() is called
> > >   calls phy_power_on(),
> > >   phy->power_count goes to 0,
> > >   phy is not powered on because power_count != 1
> > > -> no gadget working, no charging.
> > > 
> > ... if not configured by u-boot before. USB gadget might work when
> > initialized earlier in the boot process (u-boot/x-loader/mlo ...)
> > and phy-twl4030 cannot do anything about it besides if we change
> > drivers/phy/phy-core.c
> 
> Ssounds like an issue in the phy-twl4030-usb.c. Let's try to figure
> out what all we need set there for the various components there.
> 
> PM runtime for phy-twl4030-usb.c should be for the whole PHY
> device including all it's components. Then the charger and
> MUSB should separately increment the PM runtime count and then
> enable the component specific things. And then we have the
> errata to deal with when VBUS is enabled.

I repeat the subject line of the patch:
[PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
It is *not* fix charging.

So you mean that the phy should magically know at which refcounter
it should power on and off if power on/off is not called in the
balanced way?

Maybe the phy-twl4030 uses the phy layer wrong. 
Now the relevant part of power on/off in phy-twl4030 is done via struct
phy_ops.power_off/power_on (*not* via pm_runtime). Maybe that is wrong
and more parts should be moved to the pm_runtime stuff (which is also
present). 
Then the phy subsystem has its own power refcounter in struct
phy.power_count. It it handled via phy_power_off()/phy_power_on().
And that is called from musb/omap2430.c 
But that is another story. 

> 
> If there are MUSB specific PM runtime issues then let's fix
> those separately.
> 
And that exactly tries my patch to do. For that task it does not
even use the PM runtime system. Again please read the subject line of
the patch. Maybe it unveils some other pm issues in musb
which should first be fixed in a complete patch series.

Regards,
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Fri, 5 Aug 2016 06:55:01 -0700
Tony Lindgren  wrote:

> * Andreas Kemnade  [160804 09:44]:
> > Nothing happens here, so the previous state of the phy remains.
> > It would be disabled by the generic phy layer in
> > drivers/phy/phy-core.c
> >
> > > gadget driver is loaded.
> > > musb_start() is called
> > > omap2430_musb_enable() is called
> > >   calls phy_power_on(),
> > >   phy->power_count goes to 0,
> > >   phy is not powered on because power_count != 1
> > > -> no gadget working, no charging.
> > > 
> > ... if not configured by u-boot before. USB gadget might work when
> > initialized earlier in the boot process (u-boot/x-loader/mlo ...)
> > and phy-twl4030 cannot do anything about it besides if we change
> > drivers/phy/phy-core.c
> 
> Ssounds like an issue in the phy-twl4030-usb.c. Let's try to figure
> out what all we need set there for the various components there.
> 
> PM runtime for phy-twl4030-usb.c should be for the whole PHY
> device including all it's components. Then the charger and
> MUSB should separately increment the PM runtime count and then
> enable the component specific things. And then we have the
> errata to deal with when VBUS is enabled.

I repeat the subject line of the patch:
[PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
It is *not* fix charging.

So you mean that the phy should magically know at which refcounter
it should power on and off if power on/off is not called in the
balanced way?

Maybe the phy-twl4030 uses the phy layer wrong. 
Now the relevant part of power on/off in phy-twl4030 is done via struct
phy_ops.power_off/power_on (*not* via pm_runtime). Maybe that is wrong
and more parts should be moved to the pm_runtime stuff (which is also
present). 
Then the phy subsystem has its own power refcounter in struct
phy.power_count. It it handled via phy_power_off()/phy_power_on().
And that is called from musb/omap2430.c 
But that is another story. 

> 
> If there are MUSB specific PM runtime issues then let's fix
> those separately.
> 
And that exactly tries my patch to do. For that task it does not
even use the PM runtime system. Again please read the subject line of
the patch. Maybe it unveils some other pm issues in musb
which should first be fixed in a complete patch series.

Regards,
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* Andreas Kemnade  [160804 09:44]:
> Nothing happens here, so the previous state of the phy remains.
> It would be disabled by the generic phy layer in drivers/phy/phy-core.c
>
> > gadget driver is loaded.
> > musb_start() is called
> > omap2430_musb_enable() is called
> > calls phy_power_on(),
> > phy->power_count goes to 0,
> > phy is not powered on because power_count != 1
> > -> no gadget working, no charging.
> > 
> ... if not configured by u-boot before. USB gadget might work when
> initialized earlier in the boot process (u-boot/x-loader/mlo ...)
> and phy-twl4030 cannot do anything about it besides if we change
> drivers/phy/phy-core.c

Ssounds like an issue in the phy-twl4030-usb.c. Let's try to figure
out what all we need set there for the various components there.

PM runtime for phy-twl4030-usb.c should be for the whole PHY
device including all it's components. Then the charger and
MUSB should separately increment the PM runtime count and then
enable the component specific things. And then we have the
errata to deal with when VBUS is enabled.

If there are MUSB specific PM runtime issues then let's fix
those separately.

Regards,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* Andreas Kemnade  [160804 09:44]:
> Nothing happens here, so the previous state of the phy remains.
> It would be disabled by the generic phy layer in drivers/phy/phy-core.c
>
> > gadget driver is loaded.
> > musb_start() is called
> > omap2430_musb_enable() is called
> > calls phy_power_on(),
> > phy->power_count goes to 0,
> > phy is not powered on because power_count != 1
> > -> no gadget working, no charging.
> > 
> ... if not configured by u-boot before. USB gadget might work when
> initialized earlier in the boot process (u-boot/x-loader/mlo ...)
> and phy-twl4030 cannot do anything about it besides if we change
> drivers/phy/phy-core.c

Ssounds like an issue in the phy-twl4030-usb.c. Let's try to figure
out what all we need set there for the various components there.

PM runtime for phy-twl4030-usb.c should be for the whole PHY
device including all it's components. Then the charger and
MUSB should separately increment the PM runtime count and then
enable the component specific things. And then we have the
errata to deal with when VBUS is enabled.

If there are MUSB specific PM runtime issues then let's fix
those separately.

Regards,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Thu, 4 Aug 2016 16:49:30 +0200
"H. Nikolaus Schaller"  wrote:

> > Rationale:
> > 
> > The charger driver calls pm_runtime_get_sync(bci->transceiver->dev);
> > which should indirectly call twl4030_usb_set_mode to set the
> > POWER_CTRL_OTG_ENAB bit. This enables the prescaler hardware
> > for ADC8 (VBUS) channel. But this does not happen for reasons
> > outside the charger driver.
> > 
how should that work?
I only have seen the call trace from the musb/omap2430.c glue though
the phy subsystem (via phy_ops) to  twl4030_phy_power_on (which sets
the important bit).
And the phy subsystem has its own power refcounting system which
is brought into disorder by unbalanced calls from the musb system...

Regards,
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Thu, 4 Aug 2016 16:49:30 +0200
"H. Nikolaus Schaller"  wrote:

> > Rationale:
> > 
> > The charger driver calls pm_runtime_get_sync(bci->transceiver->dev);
> > which should indirectly call twl4030_usb_set_mode to set the
> > POWER_CTRL_OTG_ENAB bit. This enables the prescaler hardware
> > for ADC8 (VBUS) channel. But this does not happen for reasons
> > outside the charger driver.
> > 
how should that work?
I only have seen the call trace from the musb/omap2430.c glue though
the phy subsystem (via phy_ops) to  twl4030_phy_power_on (which sets
the important bit).
And the phy subsystem has its own power refcounting system which
is brought into disorder by unbalanced calls from the musb system...

Regards,
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Thu, 4 Aug 2016 18:31:29 +0200
Andreas Kemnade  wrote:

> Hi,
> 
> On Thu, 4 Aug 2016 07:29:19 -0700
> Tony Lindgren  wrote:
> 
> > Hi,
> > 
> > * H. Nikolaus Schaller  [160803 10:07]:
> > > All this prevents detection of cable plugin-events and VBUS
> > > measurement and setting OTG_EN before charging is attempted.
> > 
> > So I gave this patch a try but it now blocks all deeper SoC idle
> > states as the PHY stays active. I think the real fix is to make
> > sure the charger behaves independent of the USB PHY state. So
> > probably this needs to be fixed in phy-twl4030-usb.c and
> > twl4030_charger.c instead. Now it sounds like we're also shutting
> > down the charger with the USB PHY.
> > 
> Then there is another power management issue. The patch is not about
> fixing every pm issue in musb. That is not only about charging, it is
> about enabling/disabling() the phy unbalanced:
> Again what happens here without the patch:
> 
> musb will be initialized:
> omap2430_musb_disable()
>calls phy_power_off(), phy will be disabled,
>   phy->power_count goes to -1.
> 
sorry mixed something up.
Nothing happens here, so the previous state of the phy remains.
It would be disabled by the generic phy layer in drivers/phy/phy-core.c

> gadget driver is loaded.
> musb_start() is called
> omap2430_musb_enable() is called
>   calls phy_power_on(),
>   phy->power_count goes to 0,
>   phy is not powered on because power_count != 1
> -> no gadget working, no charging.
> 
... if not configured by u-boot before. USB gadget might work when
initialized earlier in the boot process (u-boot/x-loader/mlo ...)
and phy-twl4030 cannot do anything about it besides if we change
drivers/phy/phy-core.c

Regards,
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Thu, 4 Aug 2016 18:31:29 +0200
Andreas Kemnade  wrote:

> Hi,
> 
> On Thu, 4 Aug 2016 07:29:19 -0700
> Tony Lindgren  wrote:
> 
> > Hi,
> > 
> > * H. Nikolaus Schaller  [160803 10:07]:
> > > All this prevents detection of cable plugin-events and VBUS
> > > measurement and setting OTG_EN before charging is attempted.
> > 
> > So I gave this patch a try but it now blocks all deeper SoC idle
> > states as the PHY stays active. I think the real fix is to make
> > sure the charger behaves independent of the USB PHY state. So
> > probably this needs to be fixed in phy-twl4030-usb.c and
> > twl4030_charger.c instead. Now it sounds like we're also shutting
> > down the charger with the USB PHY.
> > 
> Then there is another power management issue. The patch is not about
> fixing every pm issue in musb. That is not only about charging, it is
> about enabling/disabling() the phy unbalanced:
> Again what happens here without the patch:
> 
> musb will be initialized:
> omap2430_musb_disable()
>calls phy_power_off(), phy will be disabled,
>   phy->power_count goes to -1.
> 
sorry mixed something up.
Nothing happens here, so the previous state of the phy remains.
It would be disabled by the generic phy layer in drivers/phy/phy-core.c

> gadget driver is loaded.
> musb_start() is called
> omap2430_musb_enable() is called
>   calls phy_power_on(),
>   phy->power_count goes to 0,
>   phy is not powered on because power_count != 1
> -> no gadget working, no charging.
> 
... if not configured by u-boot before. USB gadget might work when
initialized earlier in the boot process (u-boot/x-loader/mlo ...)
and phy-twl4030 cannot do anything about it besides if we change
drivers/phy/phy-core.c

Regards,
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

Hi,

On Thu, 4 Aug 2016 07:29:19 -0700
Tony Lindgren  wrote:

> Hi,
> 
> * H. Nikolaus Schaller  [160803 10:07]:
> > All this prevents detection of cable plugin-events and VBUS
> > measurement and setting OTG_EN before charging is attempted.
> 
> So I gave this patch a try but it now blocks all deeper SoC idle
> states as the PHY stays active. I think the real fix is to make
> sure the charger behaves independent of the USB PHY state. So
> probably this needs to be fixed in phy-twl4030-usb.c and
> twl4030_charger.c instead. Now it sounds like we're also shutting
> down the charger with the USB PHY.
> 
Then there is another power management issue. The patch is not about
fixing every pm issue in musb. That is not only about charging, it is
about enabling/disabling() the phy unbalanced:
Again what happens here without the patch:

musb will be initialized:
omap2430_musb_disable()
   calls phy_power_off(), phy will be disabled,
phy->power_count goes to -1.

gadget driver is loaded.
musb_start() is called
omap2430_musb_enable() is called
calls phy_power_on(),
phy->power_count goes to 0,
phy is not powered on because power_count != 1
-> no gadget working, no charging.

Regards
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

Hi,

On Thu, 4 Aug 2016 07:29:19 -0700
Tony Lindgren  wrote:

> Hi,
> 
> * H. Nikolaus Schaller  [160803 10:07]:
> > All this prevents detection of cable plugin-events and VBUS
> > measurement and setting OTG_EN before charging is attempted.
> 
> So I gave this patch a try but it now blocks all deeper SoC idle
> states as the PHY stays active. I think the real fix is to make
> sure the charger behaves independent of the USB PHY state. So
> probably this needs to be fixed in phy-twl4030-usb.c and
> twl4030_charger.c instead. Now it sounds like we're also shutting
> down the charger with the USB PHY.
> 
Then there is another power management issue. The patch is not about
fixing every pm issue in musb. That is not only about charging, it is
about enabling/disabling() the phy unbalanced:
Again what happens here without the patch:

musb will be initialized:
omap2430_musb_disable()
   calls phy_power_off(), phy will be disabled,
phy->power_count goes to -1.

gadget driver is loaded.
musb_start() is called
omap2430_musb_enable() is called
calls phy_power_on(),
phy->power_count goes to 0,
phy is not powered on because power_count != 1
-> no gadget working, no charging.

Regards
Andreas

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* H. Nikolaus Schaller  [160804 07:50]:
> > Am 04.08.2016 um 16:29 schrieb Tony Lindgren :
> > 
> > So I gave this patch a try but it now blocks all deeper SoC idle
> > states as the PHY stays active. I think the real fix is to make
> > sure the charger behaves independent of the USB PHY state.
> 
> IMHO, plugin detection of the cable is a phy task and then it tells
> the charger to start. This part works.

OK

> Charging did work up to kernel 4.3. It started to fail with 4.4-rc1
> without obvious changes to the charger but many patches to phy
> and musb. We had even backported the 4.7 charger driver
> to 4.3 and it failed as well.

OK

> > So
> > probably this needs to be fixed in phy-twl4030-usb.c and
> > twl4030_charger.c instead. Now it sounds like we're also shutting
> > down the charger with the USB PHY.
> 
> As a very deeply hidden side-effect the charger is shut down immediately
> after being started. Because phy-twl4030-usb.c does not do what it is expected
> and told to do.
> 
> I have developed a workaround for the charger driver but I do not consider it
> as the solution.
> 
> http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=b8c538e75c6dd034889bdb0d66e00ca6e128e616
...
> With that we have a workaround in the charger, but not a correct solution.
> That is what Andreas is trying to fix. The charger driver seems to be ok to
> me.

OK. So does the charger work with just phy-twl4030-usb and charger
modules loaded when cable is inserted?

Regards,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* H. Nikolaus Schaller  [160804 07:50]:
> > Am 04.08.2016 um 16:29 schrieb Tony Lindgren :
> > 
> > So I gave this patch a try but it now blocks all deeper SoC idle
> > states as the PHY stays active. I think the real fix is to make
> > sure the charger behaves independent of the USB PHY state.
> 
> IMHO, plugin detection of the cable is a phy task and then it tells
> the charger to start. This part works.

OK

> Charging did work up to kernel 4.3. It started to fail with 4.4-rc1
> without obvious changes to the charger but many patches to phy
> and musb. We had even backported the 4.7 charger driver
> to 4.3 and it failed as well.

OK

> > So
> > probably this needs to be fixed in phy-twl4030-usb.c and
> > twl4030_charger.c instead. Now it sounds like we're also shutting
> > down the charger with the USB PHY.
> 
> As a very deeply hidden side-effect the charger is shut down immediately
> after being started. Because phy-twl4030-usb.c does not do what it is expected
> and told to do.
> 
> I have developed a workaround for the charger driver but I do not consider it
> as the solution.
> 
> http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=b8c538e75c6dd034889bdb0d66e00ca6e128e616
...
> With that we have a workaround in the charger, but not a correct solution.
> That is what Andreas is trying to fix. The charger driver seems to be ok to
> me.

OK. So does the charger work with just phy-twl4030-usb and charger
modules loaded when cable is inserted?

Regards,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[H. Nikolaus Schaller]

Hi Tony,

> Am 04.08.2016 um 16:29 schrieb Tony Lindgren :
> 
> Hi,
> 
> * H. Nikolaus Schaller  [160803 10:07]:
>> All this prevents detection of cable plugin-events and VBUS measurement
>> and setting OTG_EN before charging is attempted.
> 
> So I gave this patch a try but it now blocks all deeper SoC idle
> states as the PHY stays active. I think the real fix is to make
> sure the charger behaves independent of the USB PHY state.

IMHO, plugin detection of the cable is a phy task and then it tells
the charger to start. This part works.

Charging did work up to kernel 4.3. It started to fail with 4.4-rc1
without obvious changes to the charger but many patches to phy
and musb. We had even backported the 4.7 charger driver
to 4.3 and it failed as well.

> So
> probably this needs to be fixed in phy-twl4030-usb.c and
> twl4030_charger.c instead. Now it sounds like we're also shutting
> down the charger with the USB PHY.

As a very deeply hidden side-effect the charger is shut down immediately
after being started. Because phy-twl4030-usb.c does not do what it is expected
and told to do.

I have developed a workaround for the charger driver but I do not consider it
as the solution.

http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=b8c538e75c6dd034889bdb0d66e00ca6e128e616

> power:twl4030_charger: hack to set POWER_CTRL_OTG_ENAB what twl4030-phy does 
> not do
> 
> This hack is a workaround in the charger driver to do what the  twl4030-usb
> driver is expected to have done. It is designed in a way that it can be
> removed after the twl4030-usb issue is solved, but it does not harm if it
> isn't removed immediately.
> 
> Rationale:
> 
> The charger driver calls pm_runtime_get_sync(bci->transceiver->dev);
> which should indirectly call twl4030_usb_set_mode to set the
> POWER_CTRL_OTG_ENAB bit. This enables the prescaler hardware
> for ADC8 (VBUS) channel. But this does not happen for reasons outside
> the charger driver.
> 
> If this bit is not set there are strange effects:
> * VBUS reports 0mV through MADC
> * the automatic charging stops after some ms
> * ITHEN is disabled automatically and the temperature
>   reports 56°C through MADC
> 
> The TPS65950 TRM says:
> "The prescaler for the ADCIN8 channel is in the On-The-Go (OTG) module of
> the USB subchip. This prescaler is enabled when the OTG is enabled by
> writing 1 to the OTG_EN bit of the POWER_CTRL register of the USB subchip."
> 
> and
> 
> "The software must set the POWER_CTRL[5] OTG_EN bit to 1 at least 50 ms
> before forcing the BCIMFSTS4[2] USBFASTMCHG bit to 1."
> 
> For unknown reasons this does not happen with current phy-twl4030-usb code.
> 
> Therefore we add a hack that ensures that this bit is set and the 50ms
> delay is done.
> 
> It appears as if this problem occurred for the first time in linux 4.4-rc1.


With that we have a workaround in the charger, but not a correct solution.
That is what Andreas is trying to fix. The charger driver seems to be ok to
me.

Hope this helps to better understand what is going wrong in phy4030 or musb.

BR,
Nikolaus

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[H. Nikolaus Schaller]

Hi Tony,

> Am 04.08.2016 um 16:29 schrieb Tony Lindgren :
> 
> Hi,
> 
> * H. Nikolaus Schaller  [160803 10:07]:
>> All this prevents detection of cable plugin-events and VBUS measurement
>> and setting OTG_EN before charging is attempted.
> 
> So I gave this patch a try but it now blocks all deeper SoC idle
> states as the PHY stays active. I think the real fix is to make
> sure the charger behaves independent of the USB PHY state.

IMHO, plugin detection of the cable is a phy task and then it tells
the charger to start. This part works.

Charging did work up to kernel 4.3. It started to fail with 4.4-rc1
without obvious changes to the charger but many patches to phy
and musb. We had even backported the 4.7 charger driver
to 4.3 and it failed as well.

> So
> probably this needs to be fixed in phy-twl4030-usb.c and
> twl4030_charger.c instead. Now it sounds like we're also shutting
> down the charger with the USB PHY.

As a very deeply hidden side-effect the charger is shut down immediately
after being started. Because phy-twl4030-usb.c does not do what it is expected
and told to do.

I have developed a workaround for the charger driver but I do not consider it
as the solution.

http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=b8c538e75c6dd034889bdb0d66e00ca6e128e616

> power:twl4030_charger: hack to set POWER_CTRL_OTG_ENAB what twl4030-phy does 
> not do
> 
> This hack is a workaround in the charger driver to do what the  twl4030-usb
> driver is expected to have done. It is designed in a way that it can be
> removed after the twl4030-usb issue is solved, but it does not harm if it
> isn't removed immediately.
> 
> Rationale:
> 
> The charger driver calls pm_runtime_get_sync(bci->transceiver->dev);
> which should indirectly call twl4030_usb_set_mode to set the
> POWER_CTRL_OTG_ENAB bit. This enables the prescaler hardware
> for ADC8 (VBUS) channel. But this does not happen for reasons outside
> the charger driver.
> 
> If this bit is not set there are strange effects:
> * VBUS reports 0mV through MADC
> * the automatic charging stops after some ms
> * ITHEN is disabled automatically and the temperature
>   reports 56°C through MADC
> 
> The TPS65950 TRM says:
> "The prescaler for the ADCIN8 channel is in the On-The-Go (OTG) module of
> the USB subchip. This prescaler is enabled when the OTG is enabled by
> writing 1 to the OTG_EN bit of the POWER_CTRL register of the USB subchip."
> 
> and
> 
> "The software must set the POWER_CTRL[5] OTG_EN bit to 1 at least 50 ms
> before forcing the BCIMFSTS4[2] USBFASTMCHG bit to 1."
> 
> For unknown reasons this does not happen with current phy-twl4030-usb code.
> 
> Therefore we add a hack that ensures that this bit is set and the 50ms
> delay is done.
> 
> It appears as if this problem occurred for the first time in linux 4.4-rc1.


With that we have a workaround in the charger, but not a correct solution.
That is what Andreas is trying to fix. The charger driver seems to be ok to
me.

Hope this helps to better understand what is going wrong in phy4030 or musb.

BR,
Nikolaus

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

Hi,

* H. Nikolaus Schaller  [160803 10:07]:
> All this prevents detection of cable plugin-events and VBUS measurement
> and setting OTG_EN before charging is attempted.

So I gave this patch a try but it now blocks all deeper SoC idle
states as the PHY stays active. I think the real fix is to make
sure the charger behaves independent of the USB PHY state. So
probably this needs to be fixed in phy-twl4030-usb.c and
twl4030_charger.c instead. Now it sounds like we're also shutting
down the charger with the USB PHY.

Regards,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

Hi,

* H. Nikolaus Schaller  [160803 10:07]:
> All this prevents detection of cable plugin-events and VBUS measurement
> and setting OTG_EN before charging is attempted.

So I gave this patch a try but it now blocks all deeper SoC idle
states as the PHY stays active. I think the real fix is to make
sure the charger behaves independent of the USB PHY state. So
probably this needs to be fixed in phy-twl4030-usb.c and
twl4030_charger.c instead. Now it sounds like we're also shutting
down the charger with the USB PHY.

Regards,

Tony

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[H. Nikolaus Schaller]

> Am 03.08.2016 um 17:38 schrieb Andreas Kemnade :
> 
> The code assumes that omap2430_musb_enable() and
> omap2430_musb_disable() are called in a balanced way.
> That fact is broken by the fact that musb_init_controller() calls
> musb_platform_disable() to switch from unknown state to off state
> on initialisation.
> 
> That means that phy_power_off() is called first so that
> phy->power_count gets -1 and the phy is not enabled on phy_power_on().
> So when usb gadget is started the phy is not powered on.
> Depending on the phy used that caused various problems.
> Besides of causing usb problems, that can also have side effects.
> 
> In the case of using the phy_twl4030, that prevents also charging
> the battery via usb (using twl4030_charger) and so makes further
> kernel debugging hard.
> The problem was seen with 4.7 on an openphoenux gta04. It has a DM3730
> SoC and a TPS65950 companion.  phy->power never became 1

s/TPS65950/TPS65950 (twl4030)/

> and so the usb did get powered on.

s/did get/did not get/

maybe add:

All this prevents detection of cable plugin-events and VBUS measurement
and setting OTG_EN before charging is attempted.

> 
> The patch prevents phy_power_off() from being called when
> it is already off.
> 
> Signed-off-by: Andreas Kemnade 
> ---
> changes in v2:
> improved commit message
> 
> drivers/usb/musb/omap2430.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 0b4cec9..c1a2b7b 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -413,9 +413,10 @@ static void omap2430_musb_disable(struct musb *musb)
>   struct device *dev = musb->controller;
>   struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
> 
> - if (!WARN_ON(!musb->phy))
> - phy_power_off(musb->phy);
> -
> + if (glue->enabled) {
> + if (!WARN_ON(!musb->phy))
> + phy_power_off(musb->phy);
> + }
>   if (glue->status != MUSB_UNKNOWN)
>   omap_control_usb_set_mode(glue->control_otghs,
>   USB_MODE_DISCONNECT);
> -- 
> 2.1.4
> 
> ___
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> letux-ker...@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[H. Nikolaus Schaller]

> Am 03.08.2016 um 17:38 schrieb Andreas Kemnade :
> 
> The code assumes that omap2430_musb_enable() and
> omap2430_musb_disable() are called in a balanced way.
> That fact is broken by the fact that musb_init_controller() calls
> musb_platform_disable() to switch from unknown state to off state
> on initialisation.
> 
> That means that phy_power_off() is called first so that
> phy->power_count gets -1 and the phy is not enabled on phy_power_on().
> So when usb gadget is started the phy is not powered on.
> Depending on the phy used that caused various problems.
> Besides of causing usb problems, that can also have side effects.
> 
> In the case of using the phy_twl4030, that prevents also charging
> the battery via usb (using twl4030_charger) and so makes further
> kernel debugging hard.
> The problem was seen with 4.7 on an openphoenux gta04. It has a DM3730
> SoC and a TPS65950 companion.  phy->power never became 1

s/TPS65950/TPS65950 (twl4030)/

> and so the usb did get powered on.

s/did get/did not get/

maybe add:

All this prevents detection of cable plugin-events and VBUS measurement
and setting OTG_EN before charging is attempted.

> 
> The patch prevents phy_power_off() from being called when
> it is already off.
> 
> Signed-off-by: Andreas Kemnade 
> ---
> changes in v2:
> improved commit message
> 
> drivers/usb/musb/omap2430.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 0b4cec9..c1a2b7b 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -413,9 +413,10 @@ static void omap2430_musb_disable(struct musb *musb)
>   struct device *dev = musb->controller;
>   struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
> 
> - if (!WARN_ON(!musb->phy))
> - phy_power_off(musb->phy);
> -
> + if (glue->enabled) {
> + if (!WARN_ON(!musb->phy))
> + phy_power_off(musb->phy);
> + }
>   if (glue->status != MUSB_UNKNOWN)
>   omap_control_usb_set_mode(glue->control_otghs,
>   USB_MODE_DISCONNECT);
> -- 
> 2.1.4
> 
> ___
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> letux-ker...@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel