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

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

[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
and so the usb did get powered on.

The patch prevents phy_power_off() from being called when
it is already off.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
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

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 <andreas@kemnade.info>:
> 
> 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 <andreas@kemnade.info>
> ---
> 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-kernel@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 <andreas@kemnade.info>:
> 
> 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 <andreas@kemnade.info>
> ---
> 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-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

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

[Tony Lindgren]

Hi,

* H. Nikolaus Schaller <hns@goldelico.com> [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 <hns@goldelico.com> [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]

Hi Tony,

> Am 04.08.2016 um 16:29 schrieb Tony Lindgren <tony@atomide.com>:
> 
> Hi,
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [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 <tony@atomide.com>:
> 
> Hi,
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [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]

* H. Nikolaus Schaller <hns@goldelico.com> [160804 07:50]:
> > Am 04.08.2016 um 16:29 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > 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 <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org> [160804 07:50]:
> > Am 04.08.2016 um 16:29 schrieb Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>:
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <tony@atomide.com> wrote:

> Hi,
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [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 <tony@atomide.com> wrote:

> Hi,
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [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]

On Thu, 4 Aug 2016 18:31:29 +0200
Andreas Kemnade <andreas@kemnade.info> wrote:

> Hi,
> 
> On Thu, 4 Aug 2016 07:29:19 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > Hi,
> > 
> > * H. Nikolaus Schaller <hns@goldelico.com> [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 16:49:30 +0200
"H. Nikolaus Schaller" <hns@goldelico.com> 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" <hns@goldelico.com> 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()

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [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 Fri, 5 Aug 2016 06:55:01 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [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 <andreas@kemnade.info> [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]

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

On Fri, 5 Aug 2016 23:21:35 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [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

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

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

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [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: [v2] musb: omap2430: do not assume balanced enable()/disable()

[Laurent Pinchart]

Hi Andreas,

Thank you for the patch.

On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> 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
> and so the usb did get powered on.
> 
> The patch prevents phy_power_off() from being called when
> it is already off.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

This fixes USB gadget operation on the Panda board.

Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for 2430 
glue layer")
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 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);

-- 
Regards,

Laurent Pinchart

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

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 12:27]:
> Hi Andreas,
> 
> Thank you for the patch.
> 
> On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> > 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
> > and so the usb did get powered on.
> > 
> > The patch prevents phy_power_off() from being called when
> > it is already off.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> 
> This fixes USB gadget operation on the Panda board.
> 
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for 2430 
> glue layer")
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This patch has a side effect of fixing the issue by breaking PM
runtime, not a good fix as discussed.

Regards,

Tony

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

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [160909 12:27]:
> Hi Andreas,
> 
> Thank you for the patch.
> 
> On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> > 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
> > and so the usb did get powered on.
> > 
> > The patch prevents phy_power_off() from being called when
> > it is already off.
> > 
> > Signed-off-by: Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>
> 
> This fixes USB gadget operation on the Panda board.
> 
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for 2430 
> glue layer")
> Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

This patch has a side effect of fixing the issue by breaking PM
runtime, not a good fix as discussed.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

Hi Tony,

On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 12:27]:
> > On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> >> 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
> >> and so the usb did get powered on.
> >> 
> >> The patch prevents phy_power_off() from being called when
> >> it is already off.
> >> 
> >> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > 
> > This fixes USB gadget operation on the Panda board.
> > 
> > Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for
> > 2430 glue layer")
> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> This patch has a side effect of fixing the issue by breaking PM
> runtime, not a good fix as discussed.

How exactly is it worse breaking runtime PM than breaking USB gadget 
completely ? :-)

The issue here is that the .disable() platform operation is called by musb 
with the PHY already powered off, leading to the PHY power reference count 
becoming negative. The next call to the .enable() operation restores the 
reference count to 0 without enabling the PHY.

Feel free to send me a better fix and I will test it.

-- 
Regards,

Laurent Pinchart

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

[Andreas Kemnade]

On Fri, 09 Sep 2016 23:21:50 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Tony,
> 
> On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909
> > 12:27]:
> > > On Wednesday 03 Aug 2016 17:38:51 Andreas Kemnade wrote:
> > >> 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
> > >> and so the usb did get powered on.
> > >> 
> > >> The patch prevents phy_power_off() from being called when
> > >> it is already off.
> > >> 
> > >> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > 
> > > This fixes USB gadget operation on the Panda board.
> > > 
> > > Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy
> > > handling for 2430 glue layer")
> > > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > This patch has a side effect of fixing the issue by breaking PM
> > runtime, not a good fix as discussed.
> 
> How exactly is it worse breaking runtime PM than breaking USB gadget 
> completely ? :-)
> 
Does it still break with my phy-twl4030 fixes? At least on gta04,
they fix real problems and hide the musb problem I tried to fix with
this patch.
https://patchwork.kernel.org/patch/9292097/
https://patchwork.kernel.org/patch/9298447/

> The issue here is that the .disable() platform operation is called by
> musb with the PHY already powered off, leading to the PHY power
> reference count becoming negative. The next call to the .enable()
> operation restores the reference count to 0 without enabling the PHY.
> 
> Feel free to send me a better fix and I will test it.
> 
The patch has to be reworked on top of the patch series:
Implement PM runtime for musb-core based on session bit

Regards,
Andreas Kemnade

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

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 13:21]:
> On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > This patch has a side effect of fixing the issue by breaking PM
> > runtime, not a good fix as discussed.
> 
> How exactly is it worse breaking runtime PM than breaking USB gadget 
> completely ? :-)

Yeah sorry to break it, I obviously did not test it on all platforms :(
I'm mostly using omap3 with the 2430 glue layer and am335x for the
dsps glue layer and did not know that omap4 is broken. I guess I've
recently just used the EHCI ports on panda.

> The issue here is that the .disable() platform operation is called by musb 
> with the PHY already powered off, leading to the PHY power reference count 
> becoming negative. The next call to the .enable() operation restores the 
> reference count to 0 without enabling the PHY.

Well for the phy-twl4030-usb.c, AFAIK the right fix is to fix the PHY
driver as done in "[PATCH v2] phy-twl4030-usb: initialize charging-related
stuff via pm_runtime". I suspect something similar is happening here
also with the omap4 legacy phy.

> Feel free to send me a better fix and I will test it.

Yeah will do, hang on.

Tony

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

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [160909 13:40]:
> On Fri, 09 Sep 2016 23:21:50 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > 
> > How exactly is it worse breaking runtime PM than breaking USB gadget 
> > completely ? :-)
> > 
> Does it still break with my phy-twl4030 fixes? At least on gta04,
> they fix real problems and hide the musb problem I tried to fix with
> this patch.
> https://patchwork.kernel.org/patch/9292097/
> https://patchwork.kernel.org/patch/9298447/

Andreas, it's a different USB PHY on pandaboard, that's using
phy-twl6030-usb.c. Probably similar issue.

> > The issue here is that the .disable() platform operation is called by
> > musb with the PHY already powered off, leading to the PHY power
> > reference count becoming negative. The next call to the .enable()
> > operation restores the reference count to 0 without enabling the PHY.
> > 
> > Feel free to send me a better fix and I will test it.
> > 
> The patch has to be reworked on top of the patch series:
> Implement PM runtime for musb-core based on session bit

Yeah that leaves out all most of the trickery with the glue
specific PM runtime tinkering so tracking down any remaining
unbalanced calls should be easier :)

But that's for v4.9, let's see what's the minimal fix for v4.8.

Regards,

Tony

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

[Tony Lindgren]

* Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160909 13:40]:
> On Fri, 09 Sep 2016 23:21:50 +0300
> Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> > 
> > How exactly is it worse breaking runtime PM than breaking USB gadget 
> > completely ? :-)
> > 
> Does it still break with my phy-twl4030 fixes? At least on gta04,
> they fix real problems and hide the musb problem I tried to fix with
> this patch.
> https://patchwork.kernel.org/patch/9292097/
> https://patchwork.kernel.org/patch/9298447/

Andreas, it's a different USB PHY on pandaboard, that's using
phy-twl6030-usb.c. Probably similar issue.

> > The issue here is that the .disable() platform operation is called by
> > musb with the PHY already powered off, leading to the PHY power
> > reference count becoming negative. The next call to the .enable()
> > operation restores the reference count to 0 without enabling the PHY.
> > 
> > Feel free to send me a better fix and I will test it.
> > 
> The patch has to be reworked on top of the patch series:
> Implement PM runtime for musb-core based on session bit

Yeah that leaves out all most of the trickery with the glue
specific PM runtime tinkering so tracking down any remaining
unbalanced calls should be easier :)

But that's for v4.9, let's see what's the minimal fix for v4.8.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andreas Kemnade]

On Fri, 9 Sep 2016 13:51:04 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160909 13:21]:
> > On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > > This patch has a side effect of fixing the issue by breaking PM
> > > runtime, not a good fix as discussed.
> > 
> > How exactly is it worse breaking runtime PM than breaking USB
> > gadget completely ? :-)
> 
> Yeah sorry to break it, I obviously did not test it on all
> platforms :( I'm mostly using omap3 with the 2430 glue layer and
> am335x for the dsps glue layer and did not know that omap4 is broken.
> I guess I've recently just used the EHCI ports on panda.
> 
> > The issue here is that the .disable() platform operation is called
> > by musb with the PHY already powered off, leading to the PHY power
> > reference count becoming negative. The next call to the .enable()
> > operation restores the reference count to 0 without enabling the
> > PHY.
> 
> Well for the phy-twl4030-usb.c, AFAIK the right fix is to fix the PHY
> driver as done in "[PATCH v2] phy-twl4030-usb: initialize
> charging-related stuff via pm_runtime". I suspect something similar
> is happening here also with the omap4 legacy phy.
> 
No, the fix is for making charging work independant of musb.
Gadget is working because charging is enabled and enables all parts in
the phy needed for it. And you can charge without musb (only musb_hdrc
for the mailbox but not the omap2430 glue module).

We have two independant things:
1. phy-twl4030-usb (and perhaps others) do not enable
  the phy enough to allow charging on pm_runtime_get().
  That is fixed by my phy-related patches.

2. phy_power_off/on() in called in an unbalanced way if
   it is called behind musb_platform_enable()/disable()
   as it happens in omap2430.c. Two ways to fix it:
   a) prevent phy_power_off()/on() to be called in
      an unbalanced way in omap240.c
   b) prevent musb_platform_enable()
	      musb_platform_disable() to be called in an
	      unbalanced way by fixing musb_core.c

Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
have gadget working for the most common usecases. (not using
twl4030-charger would not work yet)
But in the longer term 2. has to be fixed too.

Regards,
Andreas Kemnade

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

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> On Fri, 9 Sep 2016 13:51:04 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> > Well for the phy-twl4030-usb.c, AFAIK the right fix is to fix the PHY
> > driver as done in "[PATCH v2] phy-twl4030-usb: initialize
> > charging-related stuff via pm_runtime". I suspect something similar
> > is happening here also with the omap4 legacy phy.
> > 
> No, the fix is for making charging work independant of musb.
> Gadget is working because charging is enabled and enables all parts in
> the phy needed for it. And you can charge without musb (only musb_hdrc
> for the mailbox but not the omap2430 glue module).

Oh right.

> We have two independant things:
> 1. phy-twl4030-usb (and perhaps others) do not enable
>   the phy enough to allow charging on pm_runtime_get().
>   That is fixed by my phy-related patches.

OK

> 2. phy_power_off/on() in called in an unbalanced way if
>    it is called behind musb_platform_enable()/disable()
>    as it happens in omap2430.c. Two ways to fix it:
>    a) prevent phy_power_off()/on() to be called in
>       an unbalanced way in omap240.c
>    b) prevent musb_platform_enable()
> 	      musb_platform_disable() to be called in an
> 	      unbalanced way by fixing musb_core.c
> 
> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> have gadget working for the most common usecases. (not using
> twl4030-charger would not work yet)
> But in the longer term 2. has to be fixed too.

Sounds like option 2b here is the real fix.

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [160909 14:33]:
> * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > We have two independant things:
> > 1. phy-twl4030-usb (and perhaps others) do not enable
> >   the phy enough to allow charging on pm_runtime_get().
> >   That is fixed by my phy-related patches.
> 
> OK
>
> > 2. phy_power_off/on() in called in an unbalanced way if
> >    it is called behind musb_platform_enable()/disable()
> >    as it happens in omap2430.c. Two ways to fix it:
> >    a) prevent phy_power_off()/on() to be called in
> >       an unbalanced way in omap240.c
> >    b) prevent musb_platform_enable()
> > 	      musb_platform_disable() to be called in an
> > 	      unbalanced way by fixing musb_core.c
> > 
> > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > have gadget working for the most common usecases. (not using
> > twl4030-charger would not work yet)
> > But in the longer term 2. has to be fixed too.
> 
> Sounds like option 2b here is the real fix.

And doing full option 2b would be intrusive because of musb_stop
also calling musb_platform_disable. Here's a suggested fix for
v4.8-rc cycle.

Seems to work for me for omap3 torpedo using phy-twl4030-usb,
omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
black using dsps glue. Also PM runtime works on omap3.

This patch causes a slight merge conlict with Andrea's patches,
as listed in #1 above, but we should probably merge this first
as a fix. That is assuming it does not cause side effects to
Andrea's phy-twl4030-usb charger test case.

Can you guys please test? If things work I'll resend the
patch with proper tested-bys and acks.

Regards,

Tony

8< --------------------------
From: Tony Lindgren <tony@atomide.com>
Date: Fri, 9 Sep 2016 15:04:53 -0700
Subject: [PATCH] usb: musb: Fix unbalanced platform_disable

Commit a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
for 2430 glue layer") moved PHY enable/disable calls to happen from
omap2430_musb_enable/disable(). That broke enumeration for several
devices as PM runtime in the PHY will never enable it.

The root cause of the problem is unpaired calls from musb_core.c to
musb_platform_enable/disable in musb_core.c as reported by
Andreas Kemnade <andreas@kemnade.info>.

As musb_platform_enable/disable are being called from various functions,
let's not attempt to make them paiered immediately. This would require
fixing also musb_stop as it currently calls musb_platform_disable.

Instead, let's first fix the regression in a minimal way by removing
the initial call to musb_platform_disable.

AFAIK the initial musb_platform_disable call has always been just an
attempted workaround for the 2430 glue layer announcing itself too
early before the gadgets are configured. And that issue finally
got fixed with commit a118df07f5b1 ("usb: musb: Don't set d+ high
before enable for 2430 glue layer").

We now also need to fix the twl4030-phy accordingly making it's
PM runtime call only needed in twl4030_phy_power_on and have it
autosuspend. The cable state will keep the phy active when connected.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Reported-by: Andreas Kemnade <andreas@kemnade.info>
Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
for 2430 glue layer")
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -447,8 +447,6 @@ static int twl4030_phy_power_off(struct phy *phy)
 	struct twl4030_usb *twl = phy_get_drvdata(phy);
 
 	dev_dbg(twl->dev, "%s\n", __func__);
-	pm_runtime_mark_last_busy(twl->dev);
-	pm_runtime_put_autosuspend(twl->dev);
 
 	return 0;
 }
@@ -465,6 +463,8 @@ static int twl4030_phy_power_on(struct phy *phy)
 		twl4030_i2c_access(twl, 0);
 	twl->linkstat = MUSB_UNKNOWN;
 	schedule_delayed_work(&twl->id_workaround_work, HZ);
+	pm_runtime_mark_last_busy(twl->dev);
+	pm_runtime_put_autosuspend(twl->dev);
 
 	return 0;
 }
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2142,7 +2142,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	/* be sure interrupts are disabled before connecting ISR */
-	musb_platform_disable(musb);
 	musb_generic_disable(musb);
 
 	/* Init IRQ workqueue before request_irq */

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

[Andreas Kemnade]

On Fri, 9 Sep 2016 16:40:15 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> > * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > > We have two independant things:
> > > 1. phy-twl4030-usb (and perhaps others) do not enable
> > >   the phy enough to allow charging on pm_runtime_get().
> > >   That is fixed by my phy-related patches.
> > 
> > OK
> >
> > > 2. phy_power_off/on() in called in an unbalanced way if
> > >    it is called behind musb_platform_enable()/disable()
> > >    as it happens in omap2430.c. Two ways to fix it:
> > >    a) prevent phy_power_off()/on() to be called in
> > >       an unbalanced way in omap240.c
> > >    b) prevent musb_platform_enable()
> > > 	      musb_platform_disable() to be called in an
> > > 	      unbalanced way by fixing musb_core.c
> > > 
> > > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > > have gadget working for the most common usecases. (not using
> > > twl4030-charger would not work yet)
> > > But in the longer term 2. has to be fixed too.
> > 
> > Sounds like option 2b here is the real fix.
> 
> And doing full option 2b would be intrusive because of musb_stop
> also calling musb_platform_disable. Here's a suggested fix for
> v4.8-rc cycle.
> 
musb_platform_disable() in musb_stop() seems to be balanced.

from my list in an earlier mail:
musb_platform_disable() in musb_remove() called upon module removal

To my analysis this is odd because musb_stop() is also called indirectly
upon removal.
But topic that can be left for later.

> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> black using dsps glue. Also PM runtime works on omap3.
> 
> This patch causes a slight merge conlict with Andrea's patches,
> as listed in #1 above, but we should probably merge this first
> as a fix. That is assuming it does not cause side effects to
> Andrea's phy-twl4030-usb charger test case.
> 
Hmm, if the patch will gender-change me, then a clear NAK ;-)

> Can you guys please test? If things work I'll resend the
> patch with proper tested-bys and acks.
> 
I tested the patch without any other musb/phy-fixes.
No regressions. It fixes Gadget to be usable. Charging seems
not to be totally stable. I will check my phy-patches
on top of that again. 
PM runtime probably still desires some work on it.
But I give a clear Ack for merging this one first.

Regards,
Andreas Kemnade

> Regards,
> 
> Tony
> 
> 8< --------------------------
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 9 Sep 2016 15:04:53 -0700
> Subject: [PATCH] usb: musb: Fix unbalanced platform_disable
> 
> Commit a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer") moved PHY enable/disable calls to happen from
> omap2430_musb_enable/disable(). That broke enumeration for several
> devices as PM runtime in the PHY will never enable it.
> 
> The root cause of the problem is unpaired calls from musb_core.c to
> musb_platform_enable/disable in musb_core.c as reported by
> Andreas Kemnade <andreas@kemnade.info>.
> 
> As musb_platform_enable/disable are being called from various
> functions, let's not attempt to make them paiered immediately. This
> would require fixing also musb_stop as it currently calls
> musb_platform_disable.
> 
> Instead, let's first fix the regression in a minimal way by removing
> the initial call to musb_platform_disable.
> 
> AFAIK the initial musb_platform_disable call has always been just an
> attempted workaround for the 2430 glue layer announcing itself too
> early before the gadgets are configured. And that issue finally
> got fixed with commit a118df07f5b1 ("usb: musb: Don't set d+ high
> before enable for 2430 glue layer").
> 
> We now also need to fix the twl4030-phy accordingly making it's
> PM runtime call only needed in twl4030_phy_power_on and have it
> autosuspend. The cable state will keep the phy active when connected.
> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Reported-by: Andreas Kemnade <andreas@kemnade.info>
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -447,8 +447,6 @@ static int twl4030_phy_power_off(struct phy *phy)
>  	struct twl4030_usb *twl = phy_get_drvdata(phy);
>  
>  	dev_dbg(twl->dev, "%s\n", __func__);
> -	pm_runtime_mark_last_busy(twl->dev);
> -	pm_runtime_put_autosuspend(twl->dev);
>  
>  	return 0;
>  }
> @@ -465,6 +463,8 @@ static int twl4030_phy_power_on(struct phy *phy)
>  		twl4030_i2c_access(twl, 0);
>  	twl->linkstat = MUSB_UNKNOWN;
>  	schedule_delayed_work(&twl->id_workaround_work, HZ);
> +	pm_runtime_mark_last_busy(twl->dev);
> +	pm_runtime_put_autosuspend(twl->dev);
>  
>  	return 0;
>  }
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2142,7 +2142,6 @@ musb_init_controller(struct device *dev, int
> nIrq, void __iomem *ctrl) }
>  
>  	/* be sure interrupts are disabled before connecting ISR */
> -	musb_platform_disable(musb);
>  	musb_generic_disable(musb);
>  
>  	/* Init IRQ workqueue before request_irq */
> 
> 

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

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [160910 04:27]:
> On Fri, 9 Sep 2016 16:40:15 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> > > * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > > > We have two independant things:
> > > > 1. phy-twl4030-usb (and perhaps others) do not enable
> > > >   the phy enough to allow charging on pm_runtime_get().
> > > >   That is fixed by my phy-related patches.
> > > 
> > > OK
> > >
> > > > 2. phy_power_off/on() in called in an unbalanced way if
> > > >    it is called behind musb_platform_enable()/disable()
> > > >    as it happens in omap2430.c. Two ways to fix it:
> > > >    a) prevent phy_power_off()/on() to be called in
> > > >       an unbalanced way in omap240.c
> > > >    b) prevent musb_platform_enable()
> > > > 	      musb_platform_disable() to be called in an
> > > > 	      unbalanced way by fixing musb_core.c
> > > > 
> > > > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > > > have gadget working for the most common usecases. (not using
> > > > twl4030-charger would not work yet)
> > > > But in the longer term 2. has to be fixed too.
> > > 
> > > Sounds like option 2b here is the real fix.
> > 
> > And doing full option 2b would be intrusive because of musb_stop
> > also calling musb_platform_disable. Here's a suggested fix for
> > v4.8-rc cycle.
> > 
> musb_platform_disable() in musb_stop() seems to be balanced.
> 
> from my list in an earlier mail:
> musb_platform_disable() in musb_remove() called upon module removal
> 
> To my analysis this is odd because musb_stop() is also called indirectly
> upon removal.
> But topic that can be left for later.

OK will update the patch description. You probaby should attempt
to do a patch to make the calls paired as you already know what
needs to be done there..

> > Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> > omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> > black using dsps glue. Also PM runtime works on omap3.
> > 
> > This patch causes a slight merge conlict with Andrea's patches,
> > as listed in #1 above, but we should probably merge this first
> > as a fix. That is assuming it does not cause side effects to
> > Andrea's phy-twl4030-usb charger test case.
> > 
> Hmm, if the patch will gender-change me, then a clear NAK ;-)

Sorry typo there, s/Andrea/Andreas/, no need for other radical
changes :)

> > Can you guys please test? If things work I'll resend the
> > patch with proper tested-bys and acks.
> > 
> I tested the patch without any other musb/phy-fixes.
> No regressions. It fixes Gadget to be usable. Charging seems
> not to be totally stable. I will check my phy-patches
> on top of that again. 
> PM runtime probably still desires some work on it.
> But I give a clear Ack for merging this one first.

OK good to hear & thanks for testing. Let's see what Laurent
says.

Regards,

Tony

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

[Tony Lindgren]

* Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160910 04:27]:
> On Fri, 9 Sep 2016 16:40:15 -0700
> Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160909 14:33]:
> > > * Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160909 14:22]:
> > > > We have two independant things:
> > > > 1. phy-twl4030-usb (and perhaps others) do not enable
> > > >   the phy enough to allow charging on pm_runtime_get().
> > > >   That is fixed by my phy-related patches.
> > > 
> > > OK
> > >
> > > > 2. phy_power_off/on() in called in an unbalanced way if
> > > >    it is called behind musb_platform_enable()/disable()
> > > >    as it happens in omap2430.c. Two ways to fix it:
> > > >    a) prevent phy_power_off()/on() to be called in
> > > >       an unbalanced way in omap240.c
> > > >    b) prevent musb_platform_enable()
> > > > 	      musb_platform_disable() to be called in an
> > > > 	      unbalanced way by fixing musb_core.c
> > > > 
> > > > Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > > > have gadget working for the most common usecases. (not using
> > > > twl4030-charger would not work yet)
> > > > But in the longer term 2. has to be fixed too.
> > > 
> > > Sounds like option 2b here is the real fix.
> > 
> > And doing full option 2b would be intrusive because of musb_stop
> > also calling musb_platform_disable. Here's a suggested fix for
> > v4.8-rc cycle.
> > 
> musb_platform_disable() in musb_stop() seems to be balanced.
> 
> from my list in an earlier mail:
> musb_platform_disable() in musb_remove() called upon module removal
> 
> To my analysis this is odd because musb_stop() is also called indirectly
> upon removal.
> But topic that can be left for later.

OK will update the patch description. You probaby should attempt
to do a patch to make the calls paired as you already know what
needs to be done there..

> > Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> > omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> > black using dsps glue. Also PM runtime works on omap3.
> > 
> > This patch causes a slight merge conlict with Andrea's patches,
> > as listed in #1 above, but we should probably merge this first
> > as a fix. That is assuming it does not cause side effects to
> > Andrea's phy-twl4030-usb charger test case.
> > 
> Hmm, if the patch will gender-change me, then a clear NAK ;-)

Sorry typo there, s/Andrea/Andreas/, no need for other radical
changes :)

> > Can you guys please test? If things work I'll resend the
> > patch with proper tested-bys and acks.
> > 
> I tested the patch without any other musb/phy-fixes.
> No regressions. It fixes Gadget to be usable. Charging seems
> not to be totally stable. I will check my phy-patches
> on top of that again. 
> PM runtime probably still desires some work on it.
> But I give a clear Ack for merging this one first.

OK good to hear & thanks for testing. Let's see what Laurent
says.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

Hi Tony,

On Saturday 10 Sep 2016 06:07:50 Tony Lindgren wrote:
> * Andreas Kemnade <andreas@kemnade.info> [160910 04:27]:
> > On Fri, 9 Sep 2016 16:40:15 -0700 Tony Lindgren wrote:
> >> * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> >>> * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> >>>> We have two independant things:
> >>>> 1. phy-twl4030-usb (and perhaps others) do not enable
> >>>> 
> >>>>   the phy enough to allow charging on pm_runtime_get().
> >>>>   That is fixed by my phy-related patches.
> >>> 
> >>> OK
> >>> 
> >>>> 2. phy_power_off/on() in called in an unbalanced way if
> >>>>    it is called behind musb_platform_enable()/disable()
> >>>>    as it happens in omap2430.c. Two ways to fix it:
> >>>>    a) prevent phy_power_off()/on() to be called in
> >>>>       an unbalanced way in omap240.c
> >>>>    
> >>>>    b) prevent musb_platform_enable()
> >>>> 	      musb_platform_disable() to be called in an
> >>>> 	      unbalanced way by fixing musb_core.c
> >>>> 
> >>>> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> >>>> have gadget working for the most common usecases. (not using
> >>>> twl4030-charger would not work yet)
> >>>> But in the longer term 2. has to be fixed too.
> >>> 
> >>> Sounds like option 2b here is the real fix.
> >> 
> >> And doing full option 2b would be intrusive because of musb_stop
> >> also calling musb_platform_disable. Here's a suggested fix for
> >> v4.8-rc cycle.
> > 
> > musb_platform_disable() in musb_stop() seems to be balanced.
> > 
> > from my list in an earlier mail:
> > musb_platform_disable() in musb_remove() called upon module removal
> > 
> > To my analysis this is odd because musb_stop() is also called indirectly
> > upon removal.
> > But topic that can be left for later.
> 
> OK will update the patch description. You probaby should attempt
> to do a patch to make the calls paired as you already know what
> needs to be done there..
> 
> >> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> >> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> >> black using dsps glue. Also PM runtime works on omap3.
> >> 
> >> This patch causes a slight merge conlict with Andrea's patches,
> >> as listed in #1 above, but we should probably merge this first
> >> as a fix. That is assuming it does not cause side effects to
> >> Andrea's phy-twl4030-usb charger test case.
> > 
> > Hmm, if the patch will gender-change me, then a clear NAK ;-)
> 
> Sorry typo there, s/Andrea/Andreas/, no need for other radical
> changes :)
> 
> >> Can you guys please test? If things work I'll resend the
> >> patch with proper tested-bys and acks.
> > 
> > I tested the patch without any other musb/phy-fixes.
> > No regressions. It fixes Gadget to be usable. Charging seems
> > not to be totally stable. I will check my phy-patches
> > on top of that again.
> > PM runtime probably still desires some work on it.
> > But I give a clear Ack for merging this one first.
> 
> OK good to hear & thanks for testing. Let's see what Laurent
> says.

I say

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you for the fix.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Tony,

On Saturday 10 Sep 2016 06:07:50 Tony Lindgren wrote:
> * Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160910 04:27]:
> > On Fri, 9 Sep 2016 16:40:15 -0700 Tony Lindgren wrote:
> >> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160909 14:33]:
> >>> * Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org> [160909 14:22]:
> >>>> We have two independant things:
> >>>> 1. phy-twl4030-usb (and perhaps others) do not enable
> >>>> 
> >>>>   the phy enough to allow charging on pm_runtime_get().
> >>>>   That is fixed by my phy-related patches.
> >>> 
> >>> OK
> >>> 
> >>>> 2. phy_power_off/on() in called in an unbalanced way if
> >>>>    it is called behind musb_platform_enable()/disable()
> >>>>    as it happens in omap2430.c. Two ways to fix it:
> >>>>    a) prevent phy_power_off()/on() to be called in
> >>>>       an unbalanced way in omap240.c
> >>>>    
> >>>>    b) prevent musb_platform_enable()
> >>>> 	      musb_platform_disable() to be called in an
> >>>> 	      unbalanced way by fixing musb_core.c
> >>>> 
> >>>> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> >>>> have gadget working for the most common usecases. (not using
> >>>> twl4030-charger would not work yet)
> >>>> But in the longer term 2. has to be fixed too.
> >>> 
> >>> Sounds like option 2b here is the real fix.
> >> 
> >> And doing full option 2b would be intrusive because of musb_stop
> >> also calling musb_platform_disable. Here's a suggested fix for
> >> v4.8-rc cycle.
> > 
> > musb_platform_disable() in musb_stop() seems to be balanced.
> > 
> > from my list in an earlier mail:
> > musb_platform_disable() in musb_remove() called upon module removal
> > 
> > To my analysis this is odd because musb_stop() is also called indirectly
> > upon removal.
> > But topic that can be left for later.
> 
> OK will update the patch description. You probaby should attempt
> to do a patch to make the calls paired as you already know what
> needs to be done there..
> 
> >> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> >> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> >> black using dsps glue. Also PM runtime works on omap3.
> >> 
> >> This patch causes a slight merge conlict with Andrea's patches,
> >> as listed in #1 above, but we should probably merge this first
> >> as a fix. That is assuming it does not cause side effects to
> >> Andrea's phy-twl4030-usb charger test case.
> > 
> > Hmm, if the patch will gender-change me, then a clear NAK ;-)
> 
> Sorry typo there, s/Andrea/Andreas/, no need for other radical
> changes :)
> 
> >> Can you guys please test? If things work I'll resend the
> >> patch with proper tested-bys and acks.
> > 
> > I tested the patch without any other musb/phy-fixes.
> > No regressions. It fixes Gadget to be usable. Charging seems
> > not to be totally stable. I will check my phy-patches
> > on top of that again.
> > PM runtime probably still desires some work on it.
> > But I give a clear Ack for merging this one first.
> 
> OK good to hear & thanks for testing. Let's see what Laurent
> says.

I say

Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Thank you for the fix.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [160911 02:06]:
> Hi Tony,
> 
> On Saturday 10 Sep 2016 06:07:50 Tony Lindgren wrote:
> > * Andreas Kemnade <andreas@kemnade.info> [160910 04:27]:
> > > On Fri, 9 Sep 2016 16:40:15 -0700 Tony Lindgren wrote:
> > >> * Tony Lindgren <tony@atomide.com> [160909 14:33]:
> > >>> * Andreas Kemnade <andreas@kemnade.info> [160909 14:22]:
> > >>>> We have two independant things:
> > >>>> 1. phy-twl4030-usb (and perhaps others) do not enable
> > >>>> 
> > >>>>   the phy enough to allow charging on pm_runtime_get().
> > >>>>   That is fixed by my phy-related patches.
> > >>> 
> > >>> OK
> > >>> 
> > >>>> 2. phy_power_off/on() in called in an unbalanced way if
> > >>>>    it is called behind musb_platform_enable()/disable()
> > >>>>    as it happens in omap2430.c. Two ways to fix it:
> > >>>>    a) prevent phy_power_off()/on() to be called in
> > >>>>       an unbalanced way in omap240.c
> > >>>>    
> > >>>>    b) prevent musb_platform_enable()
> > >>>> 	      musb_platform_disable() to be called in an
> > >>>> 	      unbalanced way by fixing musb_core.c
> > >>>> 
> > >>>> Fixing 1. is enough on gta04 to fix charging and hide 2. enough to
> > >>>> have gadget working for the most common usecases. (not using
> > >>>> twl4030-charger would not work yet)
> > >>>> But in the longer term 2. has to be fixed too.
> > >>> 
> > >>> Sounds like option 2b here is the real fix.
> > >> 
> > >> And doing full option 2b would be intrusive because of musb_stop
> > >> also calling musb_platform_disable. Here's a suggested fix for
> > >> v4.8-rc cycle.
> > > 
> > > musb_platform_disable() in musb_stop() seems to be balanced.
> > > 
> > > from my list in an earlier mail:
> > > musb_platform_disable() in musb_remove() called upon module removal
> > > 
> > > To my analysis this is odd because musb_stop() is also called indirectly
> > > upon removal.
> > > But topic that can be left for later.
> > 
> > OK will update the patch description. You probaby should attempt
> > to do a patch to make the calls paired as you already know what
> > needs to be done there..
> > 
> > >> Seems to work for me for omap3 torpedo using phy-twl4030-usb,
> > >> omap4 pandaboard es using phy-twl6030-usb and am335x beaglebone
> > >> black using dsps glue. Also PM runtime works on omap3.
> > >> 
> > >> This patch causes a slight merge conlict with Andrea's patches,
> > >> as listed in #1 above, but we should probably merge this first
> > >> as a fix. That is assuming it does not cause side effects to
> > >> Andrea's phy-twl4030-usb charger test case.
> > > 
> > > Hmm, if the patch will gender-change me, then a clear NAK ;-)
> > 
> > Sorry typo there, s/Andrea/Andreas/, no need for other radical
> > changes :)
> > 
> > >> Can you guys please test? If things work I'll resend the
> > >> patch with proper tested-bys and acks.
> > > 
> > > I tested the patch without any other musb/phy-fixes.
> > > No regressions. It fixes Gadget to be usable. Charging seems
> > > not to be totally stable. I will check my phy-patches
> > > on top of that again.
> > > PM runtime probably still desires some work on it.
> > > But I give a clear Ack for merging this one first.
> > 
> > OK good to hear & thanks for testing. Let's see what Laurent
> > says.
> 
> I say
> 
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thank you for the fix.

OK great, will send a proper patch with acks shortly.

Thanks,

Tony