from:"Andreas Kemnade"

Re: [PATCH RFC] ehci-omap: simple suspend implementation

2018-02-25 Thread Andreas Kemnade

Hi,

On Mon, 19 Feb 2018 11:41:36 +0200
Roger Quadros <rog...@ti.com> wrote:

> Andreas,
> 
> On 16/02/18 20:35, Andreas Kemnade wrote:
> > On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
> > Alan Stern <st...@rowland.harvard.edu> wrote:
> >   
> >> On Fri, 16 Feb 2018, Andreas Kemnade wrote:
> >>  
> >>> This powers down the phy and on a gta04 it reduces
> >>> suspend current by 13 mA.
> >>> For unknown reasons usb does not power on properly.
> >>> Also calling usb_phy_shutdown() here feels wrong
> >>> apparently the reset line has to be activated.
> >>> usb_phy_set_suspend is not enough here. The power
> >>> consumption still stays approximately the same as
> >>> without any patch.
> >>>
> >>> With a device connected the device does not enumerate
> >>> after resume. A rmmod ehci-omap ; modprobe ehci-omap
> >>> does not make it reenumerade.
> >>> So there is still something wrong here.
> >>>
> >>> Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
> >>> ---
> >>>  drivers/usb/host/ehci-omap.c | 59 
> >>> ++--
> >>>  1 file changed, 57 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
> >>> index 8d8bafc70c1f..0be2ccf8182a 100644
> >>> --- a/drivers/usb/host/ehci-omap.c
> >>> +++ b/drivers/usb/host/ehci-omap.c
> >>> @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct 
> >>> platform_device *pdev)
> >>>   return 0;
> >>>  }
> >>>  
> >>> +
> >>> +static int __maybe_unused ehci_omap_suspend(struct device *dev)
> >>> +{
> >>> + struct usb_hcd *hcd = dev_get_drvdata(dev);
> >>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
> >>> + int ret;
> >>> + int i;
> >>> +
> >>> + ret = ehci_suspend(hcd, false);
> >>> + if (ret) {
> >>> + dev_err(dev, "ehci suspend failed: %d\n", ret);
> >>> + return ret;
> >>> + }
> >>> + for (i = 0; i < omap->nports; i++) {
> >>> + if (omap->phy[i])
> >>> + usb_phy_shutdown(omap->phy[i]);
> >>> + }
> >>> + pm_runtime_put_sync(dev);
> >>
> >> Why do you include a runtime PM call here, given that the driver
> >> doesn't support runtime suspend or resume?
> >>  
> > Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM
> > calls here in the _probe/_remove functions, so it seems to be sane to do it
> > here, too.
> > 
> >   
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int __maybe_unused ehci_omap_resume(struct device *dev)
> >>> +{
> >>> + struct usb_hcd *hcd = dev_get_drvdata(dev);
> >>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
> >>> + int i;
> >>> +
> >>> + pm_runtime_get_sync(dev);
> >>> + /*
> >>> +  * An undocumented "feature" in the OMAP3 EHCI controller,
> >>> +  * causes suspended ports to be taken out of suspend when
> >>> +  * the USBCMD.Run/Stop bit is cleared (for example when
> >>> +  * we do ehci_bus_suspend).
> >>> +  * This breaks suspend-resume if the root-hub is allowed
> >>> +  * to suspend. Writing 1 to this undocumented register bit
> >>> +  * disables this feature and restores normal behavior.
> >>> +  */
> >>> + ehci_write(hcd->regs, EHCI_INSNREG04,
> >>> +EHCI_INSNREG04_DISABLE_UNSUSPEND);
> >>
> >> Doesn't this code belong in ehci_hcd_omap_probe()?  I assume you only
> >> need to set this undocumented bit once, not every time the controller
> >> is suspended.  And in any case, according to the comment, you would
> >> need to take care of this before the root hub is suspended -- by the
> >> time the controller is suspended, it is already too late.
> >>  
> >  
> > I am not really sure about this one. It is set in ehci_hcd_omap_probe()
> > so it is set before suspend. I set it here to ensure it is re-set when
> > the controller is powered down too much to keep register contents. I
> > do not know if that is the case. But I thought it would not harm.
> >   
> 
> If the Hardware SAR (Save and restore) functionality is enabled then
> everything will be restored by hardware after a sleep to wake transition.
> 
> But you will need this patch to enable SAR for the USB power domain.
> https://lkml.org/lkml/2013/7/10/356
> 
> Missing this might be the reason why things break for you after a system
> suspend/resume.
> 
Thanks for your hints.
There is a full patchset for suspend/resume and
runtime suspend support. Is that the latest one? Is it worth to
continue on that.

I have now a hacky working solution. I enabled offmode and inserted
a msleep(50) before ehci_resume, so the phy has more time to come up.

What I think is happening is that ehci does not put the phy into
lowpower mode via ulpi register access (Register 4, suspendM bit),
and if I reset the phy, it will make wrong assumptions about phy state
if not put in offmode.
I have to enable more debug there to see what is going on there.

Regards,
Andreas



pgpFQLciYD48z.pgp
Description: OpenPGP digital signature

Re: [PATCH RFC] ehci-omap: simple suspend implementation

2018-02-20 Thread Andreas Kemnade

On Mon, 19 Feb 2018 11:41:36 +0200
Roger Quadros  wrote:

[...]
> If the Hardware SAR (Save and restore) functionality is enabled then
> everything will be restored by hardware after a sleep to wake transition.
> 
> But you will need this patch to enable SAR for the USB power domain.
> https://lkml.org/lkml/2013/7/10/356
> 
> Missing this might be the reason why things break for you after a system
> suspend/resume.
> 
ehci_resume() has a force reset flag which should be enough to bring
the system to a known state. But I tried SAR. It did not help.

rmmod ehci_omap
modprobe ehci_omap
is enough to make the device disappear (with and without the patch).
rebooting is enough to make the device
appear again. That did once work, so I'll first check when did that
broke.

Regards,
Andreas


pgpfDFsvdAQ5n.pgp
Description: OpenPGP digital signature

Re: [PATCH RFC] ehci-omap: simple suspend implementation

2018-02-16 Thread Andreas Kemnade

On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
Alan Stern <st...@rowland.harvard.edu> wrote:

> On Fri, 16 Feb 2018, Andreas Kemnade wrote:
> 
> > This powers down the phy and on a gta04 it reduces
> > suspend current by 13 mA.
> > For unknown reasons usb does not power on properly.
> > Also calling usb_phy_shutdown() here feels wrong
> > apparently the reset line has to be activated.
> > usb_phy_set_suspend is not enough here. The power
> > consumption still stays approximately the same as
> > without any patch.
> > 
> > With a device connected the device does not enumerate
> > after resume. A rmmod ehci-omap ; modprobe ehci-omap
> > does not make it reenumerade.
> > So there is still something wrong here.
> > 
> > Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
> > ---
> >  drivers/usb/host/ehci-omap.c | 59 
> > ++--
> >  1 file changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
> > index 8d8bafc70c1f..0be2ccf8182a 100644
> > --- a/drivers/usb/host/ehci-omap.c
> > +++ b/drivers/usb/host/ehci-omap.c
> > @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device 
> > *pdev)
> > return 0;
> >  }
> >  
> > +
> > +static int __maybe_unused ehci_omap_suspend(struct device *dev)
> > +{
> > +   struct usb_hcd *hcd = dev_get_drvdata(dev);
> > +   struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
> > +   int ret;
> > +   int i;
> > +
> > +   ret = ehci_suspend(hcd, false);
> > +   if (ret) {
> > +   dev_err(dev, "ehci suspend failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +   for (i = 0; i < omap->nports; i++) {
> > +   if (omap->phy[i])
> > +   usb_phy_shutdown(omap->phy[i]);
> > +   }
> > +   pm_runtime_put_sync(dev);  
> 
> Why do you include a runtime PM call here, given that the driver
> doesn't support runtime suspend or resume?
> 
Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM
calls here in the _probe/_remove functions, so it seems to be sane to do it
here, too.


> > +
> > +   return 0;
> > +}
> > +
> > +static int __maybe_unused ehci_omap_resume(struct device *dev)
> > +{
> > +   struct usb_hcd *hcd = dev_get_drvdata(dev);
> > +   struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
> > +   int i;
> > +
> > +   pm_runtime_get_sync(dev);
> > +   /*
> > +* An undocumented "feature" in the OMAP3 EHCI controller,
> > +* causes suspended ports to be taken out of suspend when
> > +* the USBCMD.Run/Stop bit is cleared (for example when
> > +* we do ehci_bus_suspend).
> > +* This breaks suspend-resume if the root-hub is allowed
> > +* to suspend. Writing 1 to this undocumented register bit
> > +* disables this feature and restores normal behavior.
> > +*/
> > +   ehci_write(hcd->regs, EHCI_INSNREG04,
> > +  EHCI_INSNREG04_DISABLE_UNSUSPEND);  
> 
> Doesn't this code belong in ehci_hcd_omap_probe()?  I assume you only
> need to set this undocumented bit once, not every time the controller
> is suspended.  And in any case, according to the comment, you would
> need to take care of this before the root hub is suspended -- by the
> time the controller is suspended, it is already too late.
>
 
I am not really sure about this one. It is set in ehci_hcd_omap_probe()
so it is set before suspend. I set it here to ensure it is re-set when
the controller is powered down too much to keep register contents. I
do not know if that is the case. But I thought it would not harm.

Regards,
Andreas


pgpxAUipTYXaD.pgp
Description: OpenPGP digital signature

[PATCH RFC] ehci-omap: simple suspend implementation

2018-02-16 Thread Andreas Kemnade

This powers down the phy and on a gta04 it reduces
suspend current by 13 mA.
For unknown reasons usb does not power on properly.
Also calling usb_phy_shutdown() here feels wrong
apparently the reset line has to be activated.
usb_phy_set_suspend is not enough here. The power
consumption still stays approximately the same as
without any patch.

With a device connected the device does not enumerate
after resume. A rmmod ehci-omap ; modprobe ehci-omap
does not make it reenumerade.
So there is still something wrong here.

Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
---
 drivers/usb/host/ehci-omap.c | 59 ++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 8d8bafc70c1f..0be2ccf8182a 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device 
*pdev)
return 0;
 }
 
+
+static int __maybe_unused ehci_omap_suspend(struct device *dev)
+{
+   struct usb_hcd *hcd = dev_get_drvdata(dev);
+   struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+   int ret;
+   int i;
+
+   ret = ehci_suspend(hcd, false);
+   if (ret) {
+   dev_err(dev, "ehci suspend failed: %d\n", ret);
+   return ret;
+   }
+   for (i = 0; i < omap->nports; i++) {
+   if (omap->phy[i])
+   usb_phy_shutdown(omap->phy[i]);
+   }
+   pm_runtime_put_sync(dev);
+
+   return 0;
+}
+
+static int __maybe_unused ehci_omap_resume(struct device *dev)
+{
+   struct usb_hcd *hcd = dev_get_drvdata(dev);
+   struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+   int i;
+
+   pm_runtime_get_sync(dev);
+   /*
+* An undocumented "feature" in the OMAP3 EHCI controller,
+* causes suspended ports to be taken out of suspend when
+* the USBCMD.Run/Stop bit is cleared (for example when
+* we do ehci_bus_suspend).
+* This breaks suspend-resume if the root-hub is allowed
+* to suspend. Writing 1 to this undocumented register bit
+* disables this feature and restores normal behavior.
+*/
+   ehci_write(hcd->regs, EHCI_INSNREG04,
+  EHCI_INSNREG04_DISABLE_UNSUSPEND);
+
+   for (i = 0; i < omap->nports; i++) {
+   if (omap->phy[i]) {
+   usb_phy_init(omap->phy[i]);
+   usb_phy_set_suspend(omap->phy[i], false);
+   }
+   }
+
+   ehci_resume(hcd, true);
+   return 0;
+}
+
 static const struct of_device_id omap_ehci_dt_ids[] = {
{ .compatible = "ti,ehci-omap" },
{ }
@@ -273,14 +325,17 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
 
+static SIMPLE_DEV_PM_OPS(ehci_omap_pm_ops, ehci_omap_suspend,
+ehci_omap_resume);
+
+
 static struct platform_driver ehci_hcd_omap_driver = {
.probe  = ehci_hcd_omap_probe,
.remove = ehci_hcd_omap_remove,
.shutdown   = usb_hcd_platform_shutdown,
-   /*.suspend  = ehci_hcd_omap_suspend, */
-   /*.resume   = ehci_hcd_omap_resume, */
.driver = {
.name   = hcd_name,
+   .pm = _omap_pm_ops,
.of_match_table = omap_ehci_dt_ids,
}
 };
-- 
2.11.0

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

Re: [PATCH] usb: musb: fix enumeration after resume

2018-02-15 Thread Andreas Kemnade

On Wed, 7 Feb 2018 14:37:00 -0600
Bin Liu <b-...@ti.com> wrote:

> On Tue, Feb 06, 2018 at 08:00:24PM +0100, Andreas Kemnade wrote:
> > Hi,
> > 
> > On Tue, 6 Feb 2018 12:46:05 -0600
> > Bin Liu <b-...@ti.com> wrote:
> >   
> > > Hi,
> > > 
> > > On Sat, Jan 27, 2018 at 09:34:03AM +0100, Andreas Kemnade wrote:  
> > > > On dm3730 there are enumeration problems after resume.
> > > > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > > > bit is not set. If it was set before suspend (because it
> > > > was enabled via musb_pullup()), it is set in
> > > > musb_restore_context() so the pullup is enabled. But then
> > > > musb_start() is called which overwrites MUSB_POWER and
> > > > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > > > enabled and the device is not enumerated.
> > >  
> > > Do you see the issue with the v4.15?
> > >   
> > Yes. Tested without other patches. 
> > It was also there in earlier kernels but I had not had motivation enough
> > to debug.  
> 
> Applied to my tree. Thanks.
> 
> > So maybe it deserves a CC: Stable  
> 
> I would prefer it is first tested on each stable tree.
> 
Tested with 4.16-rc1 with and without that patch
The problem still exists and can be fixed by that patch.

Regards,
Andreas


pgpLQssM4piPD.pgp
Description: OpenPGP digital signature

Re: [PATCH] usb: musb: fix enumeration after resume

2018-02-06 Thread Andreas Kemnade

Hi,

On Tue, 6 Feb 2018 10:47:25 -0800
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [180127 08:34]:
> > On dm3730 there are enumeration problems after resume.
> > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > bit is not set. If it was set before suspend (because it
> > was enabled via musb_pullup()), it is set in
> > musb_restore_context() so the pullup is enabled. But then
> > musb_start() is called which overwrites MUSB_POWER and
> > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > enabled and the device is not enumerated.  
> 
> I just gave this patch a quick try and things seem to behave
> for me from PM point of view:
> 
> Tested-by: Tony Lindgren <t...@atomide.com>
> 
> Unrelated to this patch, I also noticed that we now somehow
> higher idle power consumption initially when musb modules are
> loaded. It used to idle after that but now to get things to
> idle I had to plug and unplug a USB device once to the musb
> port.
> 
Hmm, I have seen this effect with some earlier kernels but not with
4.15. My observation is that current consumption went down again after
a modprobe g_ether and ifconfig usb0 up

I was loading modules piece by piece and waited 10s after each and then
measured.

Regards,
Andreas


pgpeNyIHRI9k7.pgp
Description: OpenPGP digital signature

Re: [PATCH] usb: musb: fix enumeration after resume

2018-02-06 Thread Andreas Kemnade

Hi,

On Tue, 6 Feb 2018 12:46:05 -0600
Bin Liu <b-...@ti.com> wrote:

> Hi,
> 
> On Sat, Jan 27, 2018 at 09:34:03AM +0100, Andreas Kemnade wrote:
> > On dm3730 there are enumeration problems after resume.
> > Investigation led to the cause that the MUSB_POWER_SOFTCONN
> > bit is not set. If it was set before suspend (because it
> > was enabled via musb_pullup()), it is set in
> > musb_restore_context() so the pullup is enabled. But then
> > musb_start() is called which overwrites MUSB_POWER and
> > therefore disables MUSB_POWER_SOFTCONN, so no pullup is
> > enabled and the device is not enumerated.  
>  
> Do you see the issue with the v4.15?
> 
Yes. Tested without other patches. 
It was also there in earlier kernels but I had not had motivation enough
to debug.

So maybe it deserves a CC: Stable


> > So let's do a subset of what musb_start() does
> > in the same way as musb_suspend() does it. Platform-specific
> > stuff it still called as there might be some phy-related stuff
> > which needs to be enabled.
> > Also interrupts are enabled, as it was the original idea
> > of calling musb_start() in musb_resume() according to
> > Commit 6fc6f4b87cb3 ("usb: musb: Disable interrupts on suspend,
> > enable them on resume")  
> 
> The logic in the fix makes sense, and I do see the same problem with
> AM335x on v4.9 kernel, but it doesn't happen on v4.15. I haven't checked
> if there is anything after musb_start() which sets MUSB_POWER_SOFTCON
> bit.
> 
Well reconfiguring gadget from userspace helps.

Regards,
Andreas


pgpg045RDPWAb.pgp
Description: OpenPGP digital signature

Re: power management problems in ehci-omap

2018-02-06 Thread Andreas Kemnade

On Tue, 6 Feb 2018 10:16:23 -0800
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [180206 18:04]:
> > On Tue, 6 Feb 2018 09:17:37 -0800
> > Tony Lindgren <t...@atomide.com> wrote:  
> > > uarts=$(find /sys/class/tty/tty[SO]*/power/ -type d 2>/dev/null)
> > > for uart in $uarts; do
> > >   echo enabled > $uart/wakeup 2>&1
> > >   echo auto > $uart/control 2>&1
> > > done
> > >   
> > 
> > hmm, this looks a bit like runtime suspend.  
> 
> Not only that, it enables wakeup for UART also for suspend :)
> 
We are using the rtc for wakeup and measure discharge of battery
for a time frame of about 300 seconds.

> That is if your dts has it configured with interrupts-extended
> for the console UART like omap3-beagle-xm.dts has for example.
> Seems like the gta04 dts don't have these.. And you also want
> to have chosen with stdout-path =  or whatever the debug
> UART is for earlycon to work.
> 
> > I mean suspend aka echo mem >/sys/power/state
> >   
> > > echo -n 1 > /sys/kernel/debug/pm_debug/enable_off_mode  
> 
> And the above will enable SoC and PMIC off modes, which will also
> take the suspend power to some much much lower value :) You need
> to configure the PMIC too depending if the oscillator can be turned
> off, in that case set "ti,twl4030-power-idle-osc-off". That too
> seems to be missing in gta04 dts files..
> 
It was in our tree. It can be enabled for the gta04a5. We have even done
that. But then suspend while charging breaks. I have no idea how to do a
proper if-not-charging-power-idle-osc-off patch... 

Yes there are other places where we can optimize suspend current. But
lets first find out why ehci-omap seems to cause trouble here.
So we are looking for around 15mA of additional suspend current when the
module is loaded. 
Shouldn't the reset line of the phy (usb-nop-xceiv) be set to low when
going to suspend? I do not see code how to do it. I guess that is the
reason.

BTW:
root@letux:~# cat /sys/bus/platform/devices/48064800.ehci/power/runtime_status 
active

Regards,
Andreas


pgpnLxDAY7Lsv.pgp
Description: OpenPGP digital signature

Re: power management problems in ehci-omap

2018-02-06 Thread Andreas Kemnade

On Tue, 6 Feb 2018 09:17:37 -0800
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [180206 16:56]:
> > On Tue, 6 Feb 2018 08:04:52 -0800
> > Tony Lindgren <t...@atomide.com> wrote:
> >   
> > > * Andreas Kemnade <andr...@kemnade.info> [180206 06:42]:  
> > > > rechecked with a board with really nothing connected there
> > > > Same behaviour
> > > 
> > > I've just verified that my test board power consumption goes
> > > back to normal after rmmod ehci-omap with v4.15.
> > >   
> > yes, for me too, I initially used a test script which does an
> > echo rmmod ehci-omap
> > 
> > without a real
> > rmmod ehci-omap  
> 
> Ah OK :)
> 
> > It just seems to be consistent with my observations in a fully booted
> > system (where many things can play a role). Sorry for that confusion.  
> 
> Try with a minimal set of modules first, then modprobe and rmmod one
> at a time until you find the module breaking PM?
> 
Yes, that is basically what I do with this script:

https://misc.andi.de1.cc/measure4.sh

and

https://misc.andi.de1.cc/measure5.sh

I start from a bare kernel, check currents, load ehci-omap (I have also
debugged many other pm things that way)
check currents again. 
But since my rough observations with a fully loaded system
seem to match with the 
echo rmmod behaviour, I did not notice my mistake.

> You probably know this already, but just in case it helps..
> 
> First idle the UARTs and enable off mode with something like:
> 
> uarts=$(find /sys/class/tty/tty[SO]*/device/power/ -type d)
> for uart in $uarts; do
>   echo 3000 > $uart/autosuspend_delay_ms 2>&1
> done
> 
> uarts=$(find /sys/class/tty/tty[SO]*/power/ -type d 2>/dev/null)
> for uart in $uarts; do
>   echo enabled > $uart/wakeup 2>&1
>   echo auto > $uart/control 2>&1
> done
> 

hmm, this looks a bit like runtime suspend.

I mean suspend aka echo mem >/sys/power/state

> echo -n 1 > /sys/kernel/debug/pm_debug/enable_off_mode
> 
In earlier times this just caused trouble and a little lower current,
maybe it is time to check again.

> Then if using omap3, the attached debug hack patch can be used to
> see which devices are not idling:
> 
Yes, I will try out. It it about detecting whether the soc itself went
into low power mode. But it does not check e.g. whether the phy is put
into low power mode correctly. So ehci-usb in soc might be powered off
and phy is still on.

When I go to suspend, I get this message:
"Successfully put all powerdomains to target state"

@Nikolaus: Can you check IR at the end of the suspend time in
https://misc.andi.de1.cc/measure5.sh
the second suspend compared with the first whether the phy (the USB
2233) stays on. 

Regards,
Andreas


pgp3UOYw4KlKp.pgp
Description: OpenPGP digital signature

Re: power management problems in ehci-omap

2018-02-06 Thread Andreas Kemnade

On Tue, 6 Feb 2018 08:04:52 -0800
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [180206 06:42]:
> > rechecked with a board with really nothing connected there
> > Same behaviour  
> 
> I've just verified that my test board power consumption goes
> back to normal after rmmod ehci-omap with v4.15.
> 
yes, for me too, I initially used a test script which does an
echo rmmod ehci-omap

without a real
rmmod ehci-omap

It just seems to be consistent with my observations in a fully booted
system (where many things can play a role). Sorry for that confusion.

But suspend current is a problem. I have repeated the measurement with
another board, so it is not a board problem. 

Regards,
Andreas

pgp3FPeMxsI6B.pgp
Description: OpenPGP digital signature

Re: power management problems in ehci-omap

2018-02-05 Thread Andreas Kemnade

On Sun, 4 Feb 2018 09:43:45 +0100
Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:

> Hi Andreas
> 
> On Sun, Feb 4, 2018 at 9:38 AM, Andreas Kemnade <andr...@kemnade.info> wrote:
> > On Sun, 4 Feb 2018 00:10:50 +0100
> > Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:
> >  
> >> Hi
> >>
> >> On Sun, Feb 4, 2018 at 12:03 AM, Andreas Kemnade <andr...@kemnade.info> 
> >> wrote:  
> >> > Hi,
> >> >
> >> > I booted a 4.15 kernel without udev and loaded modules piece by piece to 
> >> > analyze
> >> > pm problems. modprobe ehci-omap increases current by around 35mA and
> >> > also rmmod ehci-omap does not let it go down at all.
> >> >
> >> > I expect that removing hardware does the same thing  
> > nonsense sentence from me, was to tired. I would expect that removing the 
> > modules
> > properly powers down the device.  
> >> >
> >> > Also suspend current increases by around 15mA if that module is loaded.
> >> > I tested with having everything disabled which is attached to that usb 
> >> > bus.
> >> >  
> >>
> >> Do you have an LTE connected to the usb?
> >>  
> > Yes, there is a UMTS modem attached, but it was off during the tests.
> > It did not enumerate on the modem.
> >  
> 
> Just to understand if the suspend current drop was connected to the
> suspend of lte modem on your side.
> So you don't have anything connected on usb bus?
>
rechecked with a board with really nothing connected there
Same behaviour

Regards,
Andreas


pgp88x2cyZoxE.pgp
Description: OpenPGP digital signature

Re: power management problems in ehci-omap

2018-02-05 Thread Andreas Kemnade

On Sun, 4 Feb 2018 11:55:02 +0100
Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:

> Hi Andreas
> 
> On Sun, Feb 4, 2018 at 11:50 AM, Andreas Kemnade <andr...@kemnade.info> wrote:
> > Hi,
> >
> > On Sun, 4 Feb 2018 09:43:45 +0100
> > Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:
> >  
> >> Hi Andreas
> >>
> >> On Sun, Feb 4, 2018 at 9:38 AM, Andreas Kemnade <andr...@kemnade.info> 
> >> wrote:  
> >> > On Sun, 4 Feb 2018 00:10:50 +0100
> >> > Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:
> >> >  
> >> >> Hi
> >> >>
> >> >> On Sun, Feb 4, 2018 at 12:03 AM, Andreas Kemnade <andr...@kemnade.info> 
> >> >> wrote:  
> >> >> > Hi,
> >> >> >
> >> >> > I booted a 4.15 kernel without udev and loaded modules piece by piece 
> >> >> > to analyze
> >> >> > pm problems. modprobe ehci-omap increases current by around 35mA and
> >> >> > also rmmod ehci-omap does not let it go down at all.
> >> >> >
> >> >> > I expect that removing hardware does the same thing  
> >> > nonsense sentence from me, was to tired. I would expect that removing 
> >> > the modules
> >> > properly powers down the device.  
> >> >> >
> >> >> > Also suspend current increases by around 15mA if that module is 
> >> >> > loaded.
> >> >> > I tested with having everything disabled which is attached to that 
> >> >> > usb bus.
> >> >> >  
> >> >>
> >> >> Do you have an LTE connected to the usb?
> >> >>  
> >> > Yes, there is a UMTS modem attached, but it was off during the tests.
> >> > It did not enumerate on the modem.
> >> >  
> >>
> >> Just to understand if the suspend current drop was connected to the
> >> suspend of lte modem on your side.
> >> So you don't have anything connected on usb bus?
> >>  
> > Suspend current is increased when the ehci-omap module is loaded
> > in comparison to the state. I tested with the modem disabled, so there
> > is nothing on the bus. Increased suspend current is one thing,
> > current_before_modprobe_ehci_omap != current_after_rmmod_ehci_omap.
> >
> > I am testing with init=some_testscript.sh, so no userspace
> > is doing strange things. No module autoload or something.
> >  

Ok, there is some heavy EBCAK involved. I just did an
echo rmmod
without a real rmmod. But the suspend thing is still valid.

Sorry for the confusion.

To avoid further confusion I have uploaded these two scripts
I have given to the kernel.
http://misc.andi.de1.cc/measure4.sh

output from that:
no modules: cur: 61047 delta: 61047
before: 423462
after: 421326
average 25632 uA over 300 seconds
 cur: 60333 delta: -714
+ehci-omap cur: 93712 delta: 33379
-ehci-omap cur: 60511 delta: -33201
before: 420792
after: 418656
average 25632 uA over 300 seconds


http://misc.andi.de1.cc/measure5.sh
output from that:

no modules: cur: 61225 delta: 61225
before: 427734
after: 425598
average 25717 uA over 299 seconds
 cur: 59797 delta: -1428
+ehci-omap cur: 93712 delta: 33915
before: 425242
after: 421860
average 40719 uA over 299 seconds

The 40mA is too high. We have had measurements below 30mA even with
modem enabled with some pre-dt setup (Kernel 3.7)

Regards,
Andreas


pgpRVX4H8D9yA.pgp
Description: OpenPGP digital signature

Re: power management problems in ehci-omap

2018-02-04 Thread Andreas Kemnade

Hi,

On Sun, 4 Feb 2018 09:43:45 +0100
Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:

> Hi Andreas
> 
> On Sun, Feb 4, 2018 at 9:38 AM, Andreas Kemnade <andr...@kemnade.info> wrote:
> > On Sun, 4 Feb 2018 00:10:50 +0100
> > Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:
> >  
> >> Hi
> >>
> >> On Sun, Feb 4, 2018 at 12:03 AM, Andreas Kemnade <andr...@kemnade.info> 
> >> wrote:  
> >> > Hi,
> >> >
> >> > I booted a 4.15 kernel without udev and loaded modules piece by piece to 
> >> > analyze
> >> > pm problems. modprobe ehci-omap increases current by around 35mA and
> >> > also rmmod ehci-omap does not let it go down at all.
> >> >
> >> > I expect that removing hardware does the same thing  
> > nonsense sentence from me, was to tired. I would expect that removing the 
> > modules
> > properly powers down the device.  
> >> >
> >> > Also suspend current increases by around 15mA if that module is loaded.
> >> > I tested with having everything disabled which is attached to that usb 
> >> > bus.
> >> >  
> >>
> >> Do you have an LTE connected to the usb?
> >>  
> > Yes, there is a UMTS modem attached, but it was off during the tests.
> > It did not enumerate on the modem.
> >  
> 
> Just to understand if the suspend current drop was connected to the
> suspend of lte modem on your side.
> So you don't have anything connected on usb bus?
> 
Suspend current is increased when the ehci-omap module is loaded
in comparison to the state. I tested with the modem disabled, so there
is nothing on the bus. Increased suspend current is one thing,
current_before_modprobe_ehci_omap != current_after_rmmod_ehci_omap.

I am testing with init=some_testscript.sh, so no userspace
is doing strange things. No module autoload or something.

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

Re: power management problems in ehci-omap

2018-02-04 Thread Andreas Kemnade

On Sun, 4 Feb 2018 00:10:50 +0100
Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:

> Hi
> 
> On Sun, Feb 4, 2018 at 12:03 AM, Andreas Kemnade <andr...@kemnade.info> wrote:
> > Hi,
> >
> > I booted a 4.15 kernel without udev and loaded modules piece by piece to 
> > analyze
> > pm problems. modprobe ehci-omap increases current by around 35mA and
> > also rmmod ehci-omap does not let it go down at all.
> >
> > I expect that removing hardware does the same thing
nonsense sentence from me, was to tired. I would expect that removing the 
modules
properly powers down the device.
> >
> > Also suspend current increases by around 15mA if that module is loaded.
> > I tested with having everything disabled which is attached to that usb bus.
> >  
> 
> Do you have an LTE connected to the usb?
> 
Yes, there is a UMTS modem attached, but it was off during the tests.
It did not enumerate on the modem.

> Michael
> 
> > System was
> > GTA04A5 (with dm3730 processor and usb3322 phy)
> >
> > I know it has worked once, but I do not remember the version.
> >
> > The kernel config used can be found here:
> > http://misc.andi.de1.cc/config-4.15.gz
> >
Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

power management problems in ehci-omap

2018-02-03 Thread Andreas Kemnade

Hi,

I booted a 4.15 kernel without udev and loaded modules piece by piece to analyze
pm problems. modprobe ehci-omap increases current by around 35mA and
also rmmod ehci-omap does not let it go down at all.

I expect that removing hardware does the same thing

Also suspend current increases by around 15mA if that module is loaded.
I tested with having everything disabled which is attached to that usb bus.

System was 
GTA04A5 (with dm3730 processor and usb3322 phy)

I know it has worked once, but I do not remember the version.

The kernel config used can be found here:
http://misc.andi.de1.cc/config-4.15.gz

Regards
Andreas
BTW: I am at the FOSDEM, will probably spend a lot of time in the
hw enablement devroom. So maybe ideas could be discussed directly.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: musb: fix enumeration after resume

2018-01-27 Thread Andreas Kemnade

On dm3730 there are enumeration problems after resume.
Investigation led to the cause that the MUSB_POWER_SOFTCONN
bit is not set. If it was set before suspend (because it
was enabled via musb_pullup()), it is set in
musb_restore_context() so the pullup is enabled. But then
musb_start() is called which overwrites MUSB_POWER and
therefore disables MUSB_POWER_SOFTCONN, so no pullup is
enabled and the device is not enumerated.

So let's do a subset of what musb_start() does
in the same way as musb_suspend() does it. Platform-specific
stuff it still called as there might be some phy-related stuff
which needs to be enabled.
Also interrupts are enabled, as it was the original idea
of calling musb_start() in musb_resume() according to
Commit 6fc6f4b87cb3 ("usb: musb: Disable interrupts on suspend,
enable them on resume")

Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
---
 drivers/usb/musb/musb_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index ea5013aa69e2..255424aae513 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2709,7 +2709,8 @@ static int musb_resume(struct device *dev)
if ((devctl & mask) != (musb->context.devctl & mask))
musb->port1_status = 0;
 
-   musb_start(musb);
+   musb_enable_interrupts(musb);
+   musb_platform_enable(musb);
 
spin_lock_irqsave(>lock, flags);
error = musb_run_resume_work(musb);
-- 
2.11.0

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

Re: [PATCH] net: hso: register netdev later to avoid a race condition

2017-04-28 Thread Andreas Kemnade

On Thu, 27 Apr 2017 10:44:01 +0200
Johan Hovold <jo...@kernel.org> wrote:

> On Wed, Apr 26, 2017 at 07:26:40PM +0200, Andreas Kemnade wrote:
> > If the netdev is accessed before the urbs are initialized,
> > there will be NULL pointer dereferences. That is avoided by
> > registering it when it is fully initialized.
> 
> > Reported-by: H. Nikolaus Schaller <h...@goldelico.com>
> > Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
> > ---
> >  drivers/net/usb/hso.c | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 93411a3..00067a0 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -2534,13 +2534,6 @@ static struct hso_device 
> > *hso_create_net_device(struct usb_interface *interface,
> > SET_NETDEV_DEV(net, >dev);
> > SET_NETDEV_DEVTYPE(net, _type);
> >  
> > -   /* registering our net device */
> > -   result = register_netdev(net);
> > -   if (result) {
> > -   dev_err(>dev, "Failed to register device\n");
> > -   goto exit;
> > -   }
> > -
> > /* start allocating */
> > for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
> > hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
> > @@ -2560,6 +2553,13 @@ static struct hso_device 
> > *hso_create_net_device(struct usb_interface *interface,
> >  
> > add_net_device(hso_dev);
> >  
> > +   /* registering our net device */
> > +   result = register_netdev(net);
> > +   if (result) {
> > +   dev_err(>dev, "Failed to register device\n");
> > +   goto exit;
> 
> This all looks good, but you should consider cleaning up the error
> handling of this function as a follow-up as we should not be
> deregistering netdevs that have never been registered (e.g. if a
> required endpoint is missing or if registration fails for some reason).
> 
> But just to be clear, this problem existed also before this change.
> 
Just to check wether I am understanding this correctly. In your opinion
this patch is good for now. And later when it is applied, there should
be an additional error handling cleanup patch.

Regards,
Andreas


pgpAgLpQ_Ldqj.pgp
Description: OpenPGP digital signature

[PATCH] net: hso: register netdev later to avoid a race condition

2017-04-26 Thread Andreas Kemnade

) from [] 
(devinet_ioctl+0x348/0x714)
[ 1514.322540] [] (devinet_ioctl) from [] 
(sock_ioctl+0x2b0/0x308)
[ 1514.330627] [] (sock_ioctl) from [] (vfs_ioctl+0x20/0x34)
[ 1514.338165] [] (vfs_ioctl) from [] 
(do_vfs_ioctl+0x82c/0x93c)
[ 1514.346038] [] (do_vfs_ioctl) from [] 
(SyS_ioctl+0x4c/0x74)
[ 1514.353759] [] (SyS_ioctl) from [] 
(ret_fast_syscall+0x0/0x1c)
[ 1514.361755] Code: e3822103 e3822080 e1822781 e5981014 (e5832030)
[ 1514.510833] ---[ end trace dfb3e53c657f34a0 ]---

Reported-by: H. Nikolaus Schaller <h...@goldelico.com>
Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
---
 drivers/net/usb/hso.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 93411a3..00067a0 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2534,13 +2534,6 @@ static struct hso_device *hso_create_net_device(struct 
usb_interface *interface,
SET_NETDEV_DEV(net, >dev);
SET_NETDEV_DEVTYPE(net, _type);
 
-   /* registering our net device */
-   result = register_netdev(net);
-   if (result) {
-   dev_err(>dev, "Failed to register device\n");
-   goto exit;
-   }
-
/* start allocating */
for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
@@ -2560,6 +2553,13 @@ static struct hso_device *hso_create_net_device(struct 
usb_interface *interface,
 
add_net_device(hso_dev);
 
+   /* registering our net device */
+   result = register_netdev(net);
+   if (result) {
+   dev_err(>dev, "Failed to register device\n");
+   goto exit;
+   }
+
hso_log_port(hso_dev);
 
hso_create_rfkill(hso_dev, interface);
-- 
2.1.4

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

[PATCH] net: hso: fix module unloading

2017-04-24 Thread Andreas Kemnade

keep tty driver until usb driver is unregistered
rmmod hso
produces traces like this without that:

[40261.645904] usb 2-2: new high-speed USB device number 2 using ehci-omap
[40261.854644] usb 2-2: New USB device found, idVendor=0af0, idProduct=8800
[40261.862609] usb 2-2: New USB device strings: Mfr=3, Product=2, SerialNumber=0
[40261.872772] usb 2-2: Product: Globetrotter HSUPA Modem
[40261.880279] usb 2-2: Manufacturer: Option N.V.
[40262.021270] hso 2-2:1.5: Not our interface
[40265.556945] hso: unloaded
[40265.559875] usbcore: deregistering interface driver hso
[40265.595947] Unable to handle kernel NULL pointer dereference at virtual 
address 0033
[40265.604522] pgd = ecb14000
[40265.611877] [0033] *pgd=
[40265.617034] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[40265.622650] Modules linked in: hso(-) bnep bluetooth ipv6 arc4 
twl4030_madc_hwmon wl18xx wlcore mac80211 cfg80211 snd_soc_simple_card 
snd_soc_simple_card_utils snd_soc_omap_twl4030 snd_soc_gtm601 
generic_adc_battery extcon_gpio omap3_isp videobuf2_dma_contig videobuf2_memops 
wlcore_sdio videobuf2_v4l2 videobuf2_core ov9650 bmp280_i2c v4l2_common bmp280 
bmg160_i2c bmg160_core at24 nvmem_core videodev bmc150_accel_i2c 
bmc150_magn_i2c media bmc150_accel_core tsc2007 bmc150_magn leds_tca6507 bno055 
snd_soc_omap_mcbsp industrialio_triggered_buffer snd_soc_omap kfifo_buf 
snd_pcm_dmaengine gpio_twl4030 snd_soc_twl4030 twl4030_vibra twl4030_madc 
wwan_on_off ehci_omap pwm_bl pwm_omap_dmtimer panel_tpo_td028ttec1 
encoder_opa362 connector_analog_tv omapdrm drm_kms_helper cfbfillrect 
syscopyarea cfbimgblt sysfillrect
[40265.698211]  sysimgblt fb_sys_fops cfbcopyarea drm omapdss usb_f_ecm g_ether 
usb_f_rndis u_ether libcomposite configfs omap2430 phy_twl4030_usb musb_hdrc 
twl4030_charger industrialio w2sg0004 twl4030_pwrbutton bq27xxx_battery 
w1_bq27000 omap_hdq [last unloaded: hso]
[40265.723175] CPU: 0 PID: 2701 Comm: rmmod Not tainted 4.11.0-rc6-letux+ #6
[40265.730346] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[40265.736938] task: ecb81100 task.stack: ecb82000
[40265.741729] PC is at cdev_del+0xc/0x2c
[40265.745666] LR is at tty_unregister_device+0x40/0x50
[40265.750915] pc : []lr : []psr: 600b0113
sp : ecb83ea8  ip : eca4f898  fp : 
[40265.763000] r10:   r9 :   r8 : 0001
[40265.768493] r7 : eca4f800  r6 : 0003  r5 :   r4 : 
[40265.775360] r3 : c1458d54  r2 :   r1 : 0004  r0 : 
[40265.782257] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[40265.789764] Control: 10c5387d  Table: acb14019  DAC: 0051
[40265.795806] Process rmmod (pid: 2701, stack limit = 0xecb82218)
[40265.802062] Stack: (0xecb83ea8 to 0xecb84000)
[40265.806640] 3ea0:   ec9e8100 c04b3ecc bf737378 ed5b7c00 
0003 bf7327ec
[40265.815277] 3ec0: eca4f800  ec9fd800 eca4f800 bf737070 bf7328bc 
eca4f820 c05a9a04
[40265.823883] 3ee0: eca4f820  0001 eca4f820 ec9fd870 bf737070 
eca4f854 ec9fd8a4
[40265.832519] 3f00: ecb82000   c04e6960 eca4f820 bf737070 
bf737048 0081
[40265.841125] 3f20: c01071e4 c04e6a60 ecb81100 bf737070 bf737070 c04e5d94 
bf737020 c05a8f88
[40265.849731] 3f40: bf737100 0800 7f5fa254 0081 c01071e4 c01c4afc 
 006f7368
[40265.858367] 3f60: ecb815f4  c0cac9c4 c01071e4 ecb82000  
 c01512f4
[40265.866973] 3f80: ed5b3200 c01071e4 7f5fa220 7f5fa220 bea78ec9 0010711c 
7f5fa220 7f5fa220
[40265.875579] 3fa0: bea78ec9 c0107040 7f5fa220 7f5fa220 7f5fa254 0800 
dd35b800 dd35b800
[40265.884216] 3fc0: 7f5fa220 7f5fa220 bea78ec9 0081 bea78dcc  
bea78bd8 
[40265.892822] 3fe0: b6f70521 bea78b6c 7f5dd613 b6f70526 80070030 7f5fa254 
 
[40265.901458] [] (cdev_del) from [] 
(tty_unregister_device+0x40/0x50)
[40265.909942] [] (tty_unregister_device) from [] 
(hso_free_interface+0x80/0x144 [hso])
[40265.919982] [] (hso_free_interface [hso]) from [] 
(hso_disconnect+0xc/0x18 [hso])
[40265.929718] [] (hso_disconnect [hso]) from [] 
(usb_unbind_interface+0x84/0x200)
[40265.939239] [] (usb_unbind_interface) from [] 
(device_release_driver_internal+0x138/0x1cc)
[40265.949798] [] (device_release_driver_internal) from [] 
(driver_detach+0x60/0x6c)
[40265.959503] [] (driver_detach) from [] 
(bus_remove_driver+0x64/0x8c)
[40265.968017] [] (bus_remove_driver) from [] 
(usb_deregister+0x5c/0xb8)
[40265.976654] [] (usb_deregister) from [] 
(SyS_delete_module+0x160/0x1dc)
[40265.985443] [] (SyS_delete_module) from [] 
(ret_fast_syscall+0x0/0x1c)
[40265.994171] Code: c1458d54 e59f3020 e92d4010 e1a04000 (e5941034)
[40266.016693] ---[ end trace 9d5ac43c7e41075c ]---

Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
---
 drivers/net/usb/hso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 2e66340..b75e23f 100644
--- a/drivers/net/usb/hso.c
+++ b/drive

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

2016-09-19 Thread Andreas Kemnade

On Mon, 19 Sep 2016 09:02:50 -0700
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [160918 23:00]:
> > On Sun, 18 Sep 2016 08:19:02 -0700
> > Tony Lindgren <t...@atomide.com> wrote:
> > 
> > > * Laurent Pinchart <laurent.pinch...@ideasonboard.com> [160918
> > > 05:13]:
> > > > 
> > > > FYI, while this patch allows me to boot my Panda board with NFS
> > > > over usbnet, it only works with cold boots. A warm reboot
> > > > results in the following warning, and no ethernet traffic going
> > > > through. The USB device is detected by the host though.
> > > 
> > > Yeah I noticed too that we still have issues. For example doing
> > > rmmod of omap2430 with gadget configured and connected will
> > > produce a hardirq-safe hardirq-unsafe lock order error. That also
> > > happens with reboot with gadget configured and connected.
> > > 
> > hmm, well, there is a musb_platform_disable() in musb_remove()
> > which is simply superfluous...
> > Some days ago we had a locking problem with musb_start() and moved
> > it out of the locked area. Maybe we could do also something similar
> > here.
> 
> Well I don't think we can do that safely at this point because we
> have unpaired calls to musb_start() and musb_stop(). So trying to
> make musb_platform_enable/disable() paired right now will just lead
> into mystery breakage in various use cases.
> 
I am primarily talking about 
doing things like the patch
  usb: musb: Fix locking errors for host only mode
2c5575401e34de3d2f

did for musb_start(), for musb_stop() also. 


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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

2016-09-19 Thread Andreas Kemnade

On Sun, 18 Sep 2016 08:19:02 -0700
Tony Lindgren  wrote:

> * Laurent Pinchart  [160918 05:13]:
> > 
> > FYI, while this patch allows me to boot my Panda board with NFS
> > over usbnet, it only works with cold boots. A warm reboot results
> > in the following warning, and no ethernet traffic going through.
> > The USB device is detected by the host though.
> 
> Yeah I noticed too that we still have issues. For example doing rmmod
> of omap2430 with gadget configured and connected will produce a
> hardirq-safe hardirq-unsafe lock order error. That also happens with
> reboot with gadget configured and connected.
> 
hmm, well, there is a musb_platform_disable() in musb_remove() which is
simply superfluous...
Some days ago we had a locking problem with musb_start() and moved
it out of the locked area. Maybe we could do also something similar
here.
 
> > I'm not sure whether this is a regression introduced by commit
> > a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling for
> > 2430 glue layer") or an entirely different issue;
> ...
> > [5.711303] [] (_raw_spin_unlock_irqrestore) from
> > [] (musb_gadget_queue+0x128/0x4ac)
> > [5.711303] [] (musb_gadget_queue) from [] 
> > (usb_ep_queue+0x38/0x1d4)
> > [5.729766] [] (usb_ep_queue) from [] 
> > (rx_submit+0xc8/0x19c)
> > [5.737548] [] (rx_submit) from []
> > (rx_fill+0x7c/0xa0) [5.737548] [] (rx_fill) from
> > [] (eth_start+0x28/0x48) [5.751983] []
> > (eth_start) from [] (eth_open+0x6c/0x7c) [5.751983]
> > [] (eth_open) from [] (__dev_open+0x9c/0x104)
> 
> This could be something else though. Care to email me your .config,
> maybe this is related to legacy g_ether being built in?
> 
> Anyways, please also give the following patch a try. The reason why we
> currently have no chance of getting musb_platform_enable/disable
> balanced is because we need to try to set the musb devctl session bit
> in various places and possibly retry too. So musb_start should really
> be called musb_try_start_session() or something.
> 
> So I think the only sane thing to do at this point is to revert the
> changes trying to enable/disable USB PHY from
> omap2430_musb_enable/disable. The other fixes are OK too as they get
> us a bit closer to making the platform glue calls balanced.
>
or to balance it there (in a better way as done by my first patch).

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

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

2016-09-12 Thread Andreas Kemnade

On Mon, 12 Sep 2016 10:34:08 -0700
Tony Lindgren <t...@atomide.com> wrote:

> * Tony Lindgren <t...@atomide.com> [160912 10:26]:
> > * Bin Liu <b-...@ti.com> [160912 09:55]:
> > > Hi Tony,
> > > 
> > > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > > 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 <andr...@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 all the callers like musb_remove.
> > > > 
> > > > 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
> > > 
> > > Many glue layers rely on musb_platform_diable to disable
> > > interrupts in musb_init_controller() before registering ISR, is
> > > it safe to assume the interrupts will be masked when musb is
> > > out-of-reset so that we don't have to call
> > > musb_platform_disable() in musb_init_controller()?
> > 
> > It should be, we do request_irq only later on after this in
> > musb_core.c. And the glue layers don't do request_irq except for
> > the separate DMA interrupts in two cases.
> > 
> > And as the platform glue layer are the ones doing the probing, they
> > should initialize things into sane state :)
> > 
> > We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> > request_irq. But I'm guessing there's no need to.. Do you have some
> > example in mind that should be tested?
> 
> Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
> clear interrupts. I'll try to check on am3517 here today, don't have
> any davinci boards configured here.
> 
Hmm, then the question is: Couldn't the X_musb_disable simply be called
from X_probe if needed to be an the safe side?

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

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

2016-09-10 Thread Andreas Kemnade

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

> * Tony Lindgren <t...@atomide.com> [160909 14:33]:
> > * Andreas Kemnade <andr...@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 <t...@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 <andr...@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 <kis...@ti.com>
> Reported-by: Andreas Kemnade <andr...@kemnade.info>
> Reported-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer")
> Signed-off-by: Tony Lindgren <t...@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_dr

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

2016-09-09 Thread Andreas Kemnade

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

> * Laurent Pinchart <laurent.pinch...@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-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-09-09 Thread Andreas Kemnade

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

> Hi Tony,
> 
> On Friday 09 Sep 2016 13:08:03 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinch...@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 <andr...@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.pinch...@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-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Implement PM runtime for musb-core based on session bit

2016-08-21 Thread Andreas Kemnade

Hi,

On Thu, 18 Aug 2016 15:46:46 -0700
Tony Lindgren  wrote:

> Hi all,
> 
> Here's a series of patches to simplify musb PM runtime support
> further.
> 
> I finally figured out that we can get rid of most of the glue layer
> specific workarounds by following the devctl session bit in musb core.
> 
> The series also adds PM runtime support to the musb dsps glue layer
> for the PIO mode.
> 
> I've tested basic plugging of cables in peripheral and host mode
> on omap3 and am335x using omap2plus_defconfig. Please test if you
> can.
> 
Testing on GTA04 hardware (dm3730 + twl4030 as usb-phy), device mode,
charging: it does not make things worse than it is without it. Same
problems,
phy->power_count goes down to -1, so no gadget, no charging.
Fixing that (I have some nearly-ready things in my queue) causes
device mode and charging to work (as it does without
this patch-series).

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.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()

2016-08-08 Thread Andreas Kemnade

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

> * Andreas Kemnade <andr...@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


pgpxcl3A3FXFO.pgp
Description: OpenPGP digital signature

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

2016-08-05 Thread Andreas Kemnade

On Fri, 5 Aug 2016 06:55:01 -0700
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@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-usb" in
the body of a message to majord...@vger.kernel.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()

2016-08-04 Thread Andreas Kemnade

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

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

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.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()

2016-08-04 Thread Andreas Kemnade

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

> Hi,
> 
> On Thu, 4 Aug 2016 07:29:19 -0700
> Tony Lindgren <t...@atomide.com> wrote:
> 
> > Hi,
> > 
> > * H. Nikolaus Schaller <h...@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-usb" in
the body of a message to majord...@vger.kernel.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()

2016-08-04 Thread Andreas Kemnade

Hi,

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

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

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

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

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

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

2016-08-03 Thread 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 <andr...@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

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

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

2016-08-03 Thread Andreas Kemnade

On Tue, 2 Aug 2016 23:30:16 -0700
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [160802 08:14]:
> > On Tue, 2 Aug 2016 03:33:34 -0700
> > Tony Lindgren <t...@atomide.com> wrote:
> > 
> > > * Andreas Kemnade <andr...@kemnade.info> [160729 11:14]:
> > > > The code assumes that omap2430_musb_enable() and
> > > > omap2430_musb_disable() is called in a balanced way. The
> > > > That fact is broken by the fact that musb_init_controller()
> > > > calls musb_platform_disable() to switch from unknown state to
> > > > off state.
> > > 
> > > OK, some spelling issues with the above paragraph though :)
> > > 
> > > > 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(). In the probably common case of using the
> > > > phy_twl4030, that prevents also charging the battery and so
> > > > makes further kernel debugging hard.
> > > 
> > > Is this with v4.7 kernel? Also, care to describe how you hit this
> > > and on which hardware? Just wondering..
> > 
> > I got this error on the Openphoenux GTA04 phone. It has a DM3730
> > SoC and a TPS65950 companion. Severe charging problems were already
> > observed with the 4.4rc1. I do not know if that already was exactly
> > *this* problem. I have debugged and patched the v4.7 kernel.
> 
> OK thanks for the info.
> 
> > How I hit the problem: Just boot an that device and try to charge
> > via usb.
> 
> OK so it's the twl4030 charger then I guess.
> 
yes, besides from causing usb gadget problems, it has side effect
towards the twl4030 charger.

> > Should I resubmit the patch with an extended commit message?
> 
> Well yeah it might be worth describing that it's the twl4030
> charger that otherwise does not work properly.
> 
Ok, will resend a better commit message.

Regards,
Andreas Kemnade


pgp37vW0A_rBI.pgp
Description: OpenPGP digital signature

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

2016-08-02 Thread Andreas Kemnade

On Tue, 2 Aug 2016 03:33:34 -0700
Tony Lindgren <t...@atomide.com> wrote:

> * Andreas Kemnade <andr...@kemnade.info> [160729 11:14]:
> > The code assumes that omap2430_musb_enable() and
> > omap2430_musb_disable() is called in a balanced way. The
> > That fact is broken by the fact that musb_init_controller() calls
> > musb_platform_disable() to switch from unknown state to off state.
> 
> OK, some spelling issues with the above paragraph though :)
> 
> > 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(). In the probably common case of using the
> > phy_twl4030, that prevents also charging the battery and so makes
> > further kernel debugging hard.
> 
> Is this with v4.7 kernel? Also, care to describe how you hit this
> and on which hardware? Just wondering..

I got this error on the Openphoenux GTA04 phone. It has a DM3730
SoC and a TPS65950 companion. Severe charging problems were already
observed with the 4.4rc1. I do not know if that already was exactly
*this* problem. I have debugged and patched the v4.7 kernel.
How I hit the problem: Just boot an that device and try to charge
via usb.
Should I resubmit the patch with an extended commit message?

Regards,
Andreas Kemnade


pgpx1rKf1rrwD.pgp
Description: OpenPGP digital signature

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

2016-07-29 Thread Andreas Kemnade

The code assumes that omap2430_musb_enable() and
omap2430_musb_disable() is called in a balanced way. The
That fact is broken by the fact that musb_init_controller() calls
musb_platform_disable() to switch from unknown state to off state.

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().
In the probably common case of using the phy_twl4030, that
prevents also charging the battery and so makes further
kernel debugging hard.

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

Signed-off-by: Andreas Kemnade <andr...@kemnade.info>
---
 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 aecb934..c7ae117 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -415,9 +415,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

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

Re: [PATCH RFC] ehci-omap: simple suspend implementation

Re: [PATCH RFC] ehci-omap: simple suspend implementation

Re: [PATCH RFC] ehci-omap: simple suspend implementation

[PATCH RFC] ehci-omap: simple suspend implementation

Re: [PATCH] usb: musb: fix enumeration after resume

Re: [PATCH] usb: musb: fix enumeration after resume

Re: [PATCH] usb: musb: fix enumeration after resume

Re: power management problems in ehci-omap

Re: power management problems in ehci-omap

Re: power management problems in ehci-omap

Re: power management problems in ehci-omap

Re: power management problems in ehci-omap

Re: power management problems in ehci-omap

Re: power management problems in ehci-omap

power management problems in ehci-omap

[PATCH] usb: musb: fix enumeration after resume

Re: [PATCH] net: hso: register netdev later to avoid a race condition

[PATCH] net: hso: register netdev later to avoid a race condition

[PATCH] net: hso: fix module unloading

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

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

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

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

Re: [PATCH 0/4] Implement PM runtime for musb-core based on session bit

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

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

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

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

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

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

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

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

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

35 matches

Site Navigation

Mail list logo

Footer information