| Message ID | 1470238731-32358-1-git-send-email-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)