| Message ID | 1308745217-10883-1-git-send-email-dbaryshkov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: > Some platforms would like to disable D+ pullup on suspend, to drain as > low power, as possible. E.g. this was requested by mioa701 board > maintainers. I think this makes sense to many platforms, but by doing so, you would loose connection to the Host PC, so you need to make sure your device isn't been used before you go down this road. > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > --- > drivers/usb/otg/gpio_vbus.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/usb/gpio_vbus.h | 1 + > 2 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/otg/gpio_vbus.c b/drivers/usb/otg/gpio_vbus.c > index 52733d9..44527bd 100644 > --- a/drivers/usb/otg/gpio_vbus.c > +++ b/drivers/usb/otg/gpio_vbus.c > @@ -327,6 +327,34 @@ static int __exit gpio_vbus_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM > +static int gpio_vbus_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); > + struct gpio_vbus_mach_info *pdata = gpio_vbus->dev->platform_data; > + > + if (gpio_vbus->otg.gadget && pdata->disconnect_on_suspend) { > + /* optionally disable D+ pullup */ > + if (gpio_is_valid(pdata->gpio_pullup)) > + gpio_set_value(pdata->gpio_pullup, > + pdata->gpio_pullup_inverted); > + > + set_vbus_draw(gpio_vbus, 0); > + } > + return 0; > +} > + > +static int gpio_vbus_resume(struct platform_device *pdev) > +{ > + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); > + > + if (gpio_vbus->otg.gadget) > + schedule_work(&gpio_vbus->work); > + > + return 0; > +} actually, the correct way would be to use dev_pm_ops. > +#endif > + > /* NOTE: the gpio-vbus device may *NOT* be hotplugged */ > > MODULE_ALIAS("platform:gpio-vbus"); > @@ -337,6 +365,10 @@ static struct platform_driver gpio_vbus_driver = { > .owner = THIS_MODULE, > }, > .remove = __exit_p(gpio_vbus_remove), > +#ifdef CONFIG_PM > + .suspend = gpio_vbus_suspend, > + .resume = gpio_vbus_resume > +#endif also, avoid the ifdef on the driver structure.
On 6/22/11, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: >> Some platforms would like to disable D+ pullup on suspend, to drain as >> low power, as possible. E.g. this was requested by mioa701 board >> maintainers. > > I think this makes sense to many platforms, but by doing so, you would > loose connection to the Host PC, so you need to make sure your device > isn't been used before you go down this road. I've thought about this. Should UDC driver should somehow call into OTG layer on suspend? My understanding is that otg_set_suspend isn't the call that should be done here, is it true? My idea was that board can ask for D+ disabling, knowing itself the behaviour of the platform driver on suspend (e.g. PXA27x does disable UDC on suspend, but I dunno what effect this will cause on Host PC). >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >> --- >> drivers/usb/otg/gpio_vbus.c | 32 ++++++++++++++++++++++++++++++++ >> include/linux/usb/gpio_vbus.h | 1 + >> 2 files changed, 33 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/usb/otg/gpio_vbus.c b/drivers/usb/otg/gpio_vbus.c >> index 52733d9..44527bd 100644 >> --- a/drivers/usb/otg/gpio_vbus.c >> +++ b/drivers/usb/otg/gpio_vbus.c >> @@ -327,6 +327,34 @@ static int __exit gpio_vbus_remove(struct >> platform_device *pdev) >> return 0; >> } >> >> +#ifdef CONFIG_PM >> +static int gpio_vbus_suspend(struct platform_device *pdev, pm_message_t >> state) >> +{ >> + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); >> + struct gpio_vbus_mach_info *pdata = gpio_vbus->dev->platform_data; >> + >> + if (gpio_vbus->otg.gadget && pdata->disconnect_on_suspend) { >> + /* optionally disable D+ pullup */ >> + if (gpio_is_valid(pdata->gpio_pullup)) >> + gpio_set_value(pdata->gpio_pullup, >> + pdata->gpio_pullup_inverted); >> + >> + set_vbus_draw(gpio_vbus, 0); >> + } >> + return 0; >> +} >> + >> +static int gpio_vbus_resume(struct platform_device *pdev) >> +{ >> + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); >> + >> + if (gpio_vbus->otg.gadget) >> + schedule_work(&gpio_vbus->work); >> + >> + return 0; >> +} > > actually, the correct way would be to use dev_pm_ops. Could I use SIMPLE_DEV_PM_OPS here? > >> +#endif >> + >> /* NOTE: the gpio-vbus device may *NOT* be hotplugged */ >> >> MODULE_ALIAS("platform:gpio-vbus"); >> @@ -337,6 +365,10 @@ static struct platform_driver gpio_vbus_driver = { >> .owner = THIS_MODULE, >> }, >> .remove = __exit_p(gpio_vbus_remove), >> +#ifdef CONFIG_PM >> + .suspend = gpio_vbus_suspend, >> + .resume = gpio_vbus_resume >> +#endif > > also, avoid the ifdef on the driver structure. Ack. Was just C&P from gpio-vbus, but it's not an excuse to follow bad style.
Hi, On Wed, Jun 22, 2011 at 05:52:18PM +0400, Dmitry Eremin-Solenikov wrote: > On 6/22/11, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: > >> Some platforms would like to disable D+ pullup on suspend, to drain as > >> low power, as possible. E.g. this was requested by mioa701 board > >> maintainers. > > > > I think this makes sense to many platforms, but by doing so, you would > > loose connection to the Host PC, so you need to make sure your device > > isn't been used before you go down this road. > > I've thought about this. Should UDC driver should somehow call into OTG > layer on suspend? My understanding is that otg_set_suspend isn't the call > that should be done here, is it true? > > My idea was that board can ask for D+ disabling, knowing itself the behaviour > of the platform driver on suspend (e.g. PXA27x does disable UDC on suspend, > but I dunno what effect this will cause on Host PC). Host PC will only see the device disconnecting. So, what happens if the user has mounted file systems when you decide to go into suspend ? > > >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > >> --- > >> drivers/usb/otg/gpio_vbus.c | 32 ++++++++++++++++++++++++++++++++ > >> include/linux/usb/gpio_vbus.h | 1 + > >> 2 files changed, 33 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/usb/otg/gpio_vbus.c b/drivers/usb/otg/gpio_vbus.c > >> index 52733d9..44527bd 100644 > >> --- a/drivers/usb/otg/gpio_vbus.c > >> +++ b/drivers/usb/otg/gpio_vbus.c > >> @@ -327,6 +327,34 @@ static int __exit gpio_vbus_remove(struct > >> platform_device *pdev) > >> return 0; > >> } > >> > >> +#ifdef CONFIG_PM > >> +static int gpio_vbus_suspend(struct platform_device *pdev, pm_message_t > >> state) > >> +{ > >> + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); > >> + struct gpio_vbus_mach_info *pdata = gpio_vbus->dev->platform_data; > >> + > >> + if (gpio_vbus->otg.gadget && pdata->disconnect_on_suspend) { > >> + /* optionally disable D+ pullup */ > >> + if (gpio_is_valid(pdata->gpio_pullup)) > >> + gpio_set_value(pdata->gpio_pullup, > >> + pdata->gpio_pullup_inverted); > >> + > >> + set_vbus_draw(gpio_vbus, 0); > >> + } > >> + return 0; > >> +} > >> + > >> +static int gpio_vbus_resume(struct platform_device *pdev) > >> +{ > >> + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); > >> + > >> + if (gpio_vbus->otg.gadget) > >> + schedule_work(&gpio_vbus->work); > >> + > >> + return 0; > >> +} > > > > actually, the correct way would be to use dev_pm_ops. > > Could I use SIMPLE_DEV_PM_OPS here? for sure ;-) > >> +#endif > >> + > >> /* NOTE: the gpio-vbus device may *NOT* be hotplugged */ > >> > >> MODULE_ALIAS("platform:gpio-vbus"); > >> @@ -337,6 +365,10 @@ static struct platform_driver gpio_vbus_driver = { > >> .owner = THIS_MODULE, > >> }, > >> .remove = __exit_p(gpio_vbus_remove), > >> +#ifdef CONFIG_PM > >> + .suspend = gpio_vbus_suspend, > >> + .resume = gpio_vbus_resume > >> +#endif > > > > also, avoid the ifdef on the driver structure. > > Ack. Was just C&P from gpio-vbus, but it's not an excuse > to follow bad style. see what e.g the musb driver does to avoid the ifdef (drivers/usb/musb/musb_core.c)
On 6/22/11, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Jun 22, 2011 at 05:52:18PM +0400, Dmitry Eremin-Solenikov wrote: >> On 6/22/11, Felipe Balbi <balbi@ti.com> wrote: >> > Hi, >> > >> > On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: >> >> Some platforms would like to disable D+ pullup on suspend, to drain as >> >> low power, as possible. E.g. this was requested by mioa701 board >> >> maintainers. >> > >> > I think this makes sense to many platforms, but by doing so, you would >> > loose connection to the Host PC, so you need to make sure your device >> > isn't been used before you go down this road. >> >> I've thought about this. Should UDC driver should somehow call into OTG >> layer on suspend? My understanding is that otg_set_suspend isn't the call >> that should be done here, is it true? >> >> My idea was that board can ask for D+ disabling, knowing itself the >> behaviour >> of the platform driver on suspend (e.g. PXA27x does disable UDC on >> suspend, >> but I dunno what effect this will cause on Host PC). > > Host PC will only see the device disconnecting. So, what happens if the > user has mounted file systems when you decide to go into suspend ? What happens if user has mounted filesystems when I decide to pull out the cable? I agree with you generally, but I'd like to hear any suggestions.
Hi, On Wed, Jun 22, 2011 at 06:15:17PM +0400, Dmitry Eremin-Solenikov wrote: > On 6/22/11, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Wed, Jun 22, 2011 at 05:52:18PM +0400, Dmitry Eremin-Solenikov wrote: > >> On 6/22/11, Felipe Balbi <balbi@ti.com> wrote: > >> > Hi, > >> > > >> > On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: > >> >> Some platforms would like to disable D+ pullup on suspend, to drain as > >> >> low power, as possible. E.g. this was requested by mioa701 board > >> >> maintainers. > >> > > >> > I think this makes sense to many platforms, but by doing so, you would > >> > loose connection to the Host PC, so you need to make sure your device > >> > isn't been used before you go down this road. > >> > >> I've thought about this. Should UDC driver should somehow call into OTG > >> layer on suspend? My understanding is that otg_set_suspend isn't the call > >> that should be done here, is it true? > >> > >> My idea was that board can ask for D+ disabling, knowing itself the > >> behaviour > >> of the platform driver on suspend (e.g. PXA27x does disable UDC on > >> suspend, > >> but I dunno what effect this will cause on Host PC). > > > > Host PC will only see the device disconnecting. So, what happens if the > > user has mounted file systems when you decide to go into suspend ? > > What happens if user has mounted filesystems when I decide to pull out > the cable? for starters you could have filesystem corruption. In short, the best way would be to return -EBUSY in your suspend if the cable is still attached. You could use osme VBUS IRQ to toggle a driver flag which, if true, would return -EBUSY on suspend(). > I agree with you generally, but I'd like to hear any suggestions. I'm not sure how to solve this, but OTOH the original code already did this, just on a different way, right ?
On 22.06.2011 18:20, Felipe Balbi wrote: > Hi, > > On Wed, Jun 22, 2011 at 06:15:17PM +0400, Dmitry Eremin-Solenikov wrote: >> On 6/22/11, Felipe Balbi<balbi@ti.com> wrote: >>> Hi, >>> >>> On Wed, Jun 22, 2011 at 05:52:18PM +0400, Dmitry Eremin-Solenikov wrote: >>>> On 6/22/11, Felipe Balbi<balbi@ti.com> wrote: >>>>> Hi, >>>>> >>>>> On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: >>>>>> Some platforms would like to disable D+ pullup on suspend, to drain as >>>>>> low power, as possible. E.g. this was requested by mioa701 board >>>>>> maintainers. >>>>> >>>>> I think this makes sense to many platforms, but by doing so, you would >>>>> loose connection to the Host PC, so you need to make sure your device >>>>> isn't been used before you go down this road. >>>> >>>> I've thought about this. Should UDC driver should somehow call into OTG >>>> layer on suspend? My understanding is that otg_set_suspend isn't the call >>>> that should be done here, is it true? >>>> >>>> My idea was that board can ask for D+ disabling, knowing itself the >>>> behaviour >>>> of the platform driver on suspend (e.g. PXA27x does disable UDC on >>>> suspend, >>>> but I dunno what effect this will cause on Host PC). >>> >>> Host PC will only see the device disconnecting. So, what happens if the >>> user has mounted file systems when you decide to go into suspend ? >> >> What happens if user has mounted filesystems when I decide to pull out >> the cable? > > for starters you could have filesystem corruption. In short, the best > way would be to return -EBUSY in your suspend if the cable is still > attached. That was a rhetorical question. Basically there are plenty of situations and cases (cable is not attached; cable attached, but no gadget driver; cable attached, block gadget driver, but filesystems aren't mounted; cable attached, block gadget driver , filesystem mounted, but host is also suspended, etc.). > You could use osme VBUS IRQ to toggle a driver flag which, if true, > would return -EBUSY on suspend(). I'm more and more thinking that this handling this -EBUSY isn't a task of gpio-vbus, but rather of some higher level driver. I'd assume that if I hit this point, all previous drivers (which depend on this transceiver, so registered later) permit suspending at this moment, so everything is OK :) > >> I agree with you generally, but I'd like to hear any suggestions. > > I'm not sure how to solve this, but OTOH the original code already did > this, just on a different way, right ? Yes. pxa27x udc driver disables D+ pullup on suspend and that's the behaviour asked from me by Robert Jarzmik in comments to first cleanup patch serie for pxa27x UDC driver.
On Wed, 22 Jun 2011, Felipe Balbi wrote: > Hi, > > On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: > > Some platforms would like to disable D+ pullup on suspend, to drain as > > low power, as possible. E.g. this was requested by mioa701 board > > maintainers. > > I think this makes sense to many platforms, but by doing so, you would > loose connection to the Host PC, so you need to make sure your device > isn't been used before you go down this road. In fact, something like this is _necessary_ for all UDC/PHY drivers unless the device can guarantee that it will automatically wake up from suspend in time to service a USB packet (note that the window for responding to a packet is only a few microseconds). Otherwise the device would appear to the host to be unresponsive and broken -- better to do a clean disconnect. If suspending the device while it is in use would cause problems ... then don't suspend it when it is in use! Alan Stern
Hi, On Wed, Jun 22, 2011 at 06:26:36PM +0400, Dmitry Eremin-Solenikov wrote: > >You could use osme VBUS IRQ to toggle a driver flag which, if true, > >would return -EBUSY on suspend(). > > I'm more and more thinking that this handling this -EBUSY isn't a > task of gpio-vbus, but rather of some higher level driver. I'd assume > that > if I hit this point, all previous drivers (which depend on this > transceiver, so registered later) permit suspending at this moment, > so everything is OK :) the thing is that today we don't have the "higher level driver" see that the OTG/transceiver framework (if you can call it a framework) is just a static global pointer which people set. So, you need to have per-transceiver solutions, unfortunately :-( > >>I agree with you generally, but I'd like to hear any suggestions. > > > >I'm not sure how to solve this, but OTOH the original code already did > >this, just on a different way, right ? > > Yes. pxa27x udc driver disables D+ pullup on suspend and that's the > behaviour asked from me by Robert Jarzmik in comments to first > cleanup patch serie for pxa27x UDC driver. ok... so, go ahead but keep in mind you could end up in a bad situation ;-)
Hi, On Wed, Jun 22, 2011 at 10:30:18AM -0400, Alan Stern wrote: > On Wed, 22 Jun 2011, Felipe Balbi wrote: > > > Hi, > > > > On Wed, Jun 22, 2011 at 04:20:16PM +0400, Dmitry Eremin-Solenikov wrote: > > > Some platforms would like to disable D+ pullup on suspend, to drain as > > > low power, as possible. E.g. this was requested by mioa701 board > > > maintainers. > > > > I think this makes sense to many platforms, but by doing so, you would > > loose connection to the Host PC, so you need to make sure your device > > isn't been used before you go down this road. > > In fact, something like this is _necessary_ for all UDC/PHY drivers > unless the device can guarantee that it will automatically wake up from > suspend in time to service a USB packet (note that the window for > responding to a packet is only a few microseconds). Otherwise the > device would appear to the host to be unresponsive and broken -- better > to do a clean disconnect. > > If suspending the device while it is in use would cause problems ... > then don't suspend it when it is in use! I second your thoughts, but today we don't have enough infrastructure to communicate between PHY and Link, so a clean solution isn't possible, right ? Should we block this patch due to that ?
On Wed, 22 Jun 2011, Felipe Balbi wrote: > > In fact, something like this is _necessary_ for all UDC/PHY drivers > > unless the device can guarantee that it will automatically wake up from > > suspend in time to service a USB packet (note that the window for > > responding to a packet is only a few microseconds). Otherwise the > > device would appear to the host to be unresponsive and broken -- better > > to do a clean disconnect. > > > > If suspending the device while it is in use would cause problems ... > > then don't suspend it when it is in use! > > I second your thoughts, but today we don't have enough infrastructure to > communicate between PHY and Link, so a clean solution isn't possible, > right ? > > Should we block this patch due to that ? No, the patch is appropriate. We don't need better communication. If g_mass_storage (for example) knows that the device is in use, it can block suspends by returning -EBUSY from its own suspend callback. The UDC driver doesn't need to worry about these matters; it should assume that such things have already been handled elsewhere. That's what Dmitry meant when he was talking about a "higher level driver" -- maybe "lower level" would have been a better choice of words. :-) Until recently, ordering of the suspend callbacks didn't present any problem. The gadget device was a child of the UDC device and therefore its suspend callback would always be invoked first. With the new UDC framework, I don't know if this is true any more. It's something to consider. Alan Stern
Hi, On Wed, Jun 22, 2011 at 11:02:27AM -0400, Alan Stern wrote: > On Wed, 22 Jun 2011, Felipe Balbi wrote: > > > > In fact, something like this is _necessary_ for all UDC/PHY drivers > > > unless the device can guarantee that it will automatically wake up from > > > suspend in time to service a USB packet (note that the window for > > > responding to a packet is only a few microseconds). Otherwise the > > > device would appear to the host to be unresponsive and broken -- better > > > to do a clean disconnect. > > > > > > If suspending the device while it is in use would cause problems ... > > > then don't suspend it when it is in use! > > > > I second your thoughts, but today we don't have enough infrastructure to > > communicate between PHY and Link, so a clean solution isn't possible, > > right ? > > > > Should we block this patch due to that ? > > No, the patch is appropriate. > > We don't need better communication. If g_mass_storage (for example) > knows that the device is in use, it can block suspends by returning > -EBUSY from its own suspend callback. The UDC driver doesn't need to > worry about these matters; it should assume that such things have > already been handled elsewhere. That's what Dmitry meant when he was > talking about a "higher level driver" -- maybe "lower level" would have > been a better choice of words. :-) > > Until recently, ordering of the suspend callbacks didn't present any > problem. The gadget device was a child of the UDC device and therefore > its suspend callback would always be invoked first. > > With the new UDC framework, I don't know if this is true any more. > It's something to consider. it's still true, but with one extra device in the middle. e.g.: musb is parent to udc-core which is parent to g_zero.
On 06/22/2011 05:19 PM, Felipe Balbi wrote: > Hi, > > On Wed, Jun 22, 2011 at 11:02:27AM -0400, Alan Stern wrote: >> No, the patch is appropriate. Indeed. >> >> We don't need better communication. If g_mass_storage (for example) >> knows that the device is in use, it can block suspends by returning >> -EBUSY from its own suspend callback. The UDC driver doesn't need to >> worry about these matters; it should assume that such things have >> already been handled elsewhere. That's what Dmitry meant when he was >> talking about a "higher level driver" -- maybe "lower level" would have >> been a better choice of words. :-) The suspend at driver level should not care about filesystem in use, etc ... A disconnected cable cannot be prevented by the kernel, and the effect in fine is the same as the suspend. >> Until recently, ordering of the suspend callbacks didn't present any >> problem. The gadget device was a child of the UDC device and therefore >> its suspend callback would always be invoked first. >> >> With the new UDC framework, I don't know if this is true any more. >> It's something to consider. That true also for the UDC driver, which should be suspended before the transciever, so that it can complete its shutdown properly. That's what bothers me with the patch. And we should not confuse the 2 suspends : (1) driver suspend, which happens at suspendToRam or suspendToDisk (the one we're discussing here) (2) USB suspend (which if I remember correctly is a special USB command while the USB bus remains powered). The order of suspend in (1) requires : - gadget (gzero, gether, ...) suspended first - UDC suspended second - transceiver suspended last The order of suspend in (2) doesn't require anything AFAIR. For order in (1) : - gadget before UDC should be enforced by the framework, as the UDC usb_gadget_probe_driver() method calls device_add() on the gadget< - UDC before transceiver is enforced in some drivers by directly manipulating the gpio line. So Dimitry, do you have an idea as how to guarantee order of suspend in (1), between UDC driver and gpio_vbus ? Maybe an otg_*() call would do the trick ? Cheers. -- Robert
On 6/25/11, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > On 06/22/2011 05:19 PM, Felipe Balbi wrote: >> On Wed, Jun 22, 2011 at 11:02:27AM -0400, Alan Stern wrote: >>> Until recently, ordering of the suspend callbacks didn't present any >>> problem. The gadget device was a child of the UDC device and therefore >>> its suspend callback would always be invoked first. >>> >>> With the new UDC framework, I don't know if this is true any more. >>> It's something to consider. > That true also for the UDC driver, which should be suspended before the > transciever, so that it can complete its shutdown properly. > That's what bothers me with the patch. > > And we should not confuse the 2 suspends : > (1) driver suspend, which happens at suspendToRam or suspendToDisk > (the one we're discussing here) > (2) USB suspend (which if I remember correctly is a special USB > command while the USB bus remains powered). > > The order of suspend in (1) requires : > - gadget (gzero, gether, ...) suspended first > - UDC suspended second > - transceiver suspended last > > The order of suspend in (2) doesn't require anything AFAIR. > > For order in (1) : > - gadget before UDC should be enforced by the framework, as the UDC > usb_gadget_probe_driver() method calls device_add() on the gadget< > - UDC before transceiver is enforced in some drivers by directly > manipulating the gpio line. > > So Dimitry, do you have an idea as how to guarantee order of suspend in > (1), between UDC driver and gpio_vbus ? Maybe an otg_*() call would do > the trick ? That is simple: UDC driver acquires transceiver via otg_*() calls, so transceiver is already registered at the point of probing UDC. I'll check the order of calls during suspend though.
On Sat, 25 Jun 2011, Robert Jarzmik wrote: > On 06/22/2011 05:19 PM, Felipe Balbi wrote: > > Hi, > > > > On Wed, Jun 22, 2011 at 11:02:27AM -0400, Alan Stern wrote: > >> No, the patch is appropriate. > Indeed. > >> > >> We don't need better communication. If g_mass_storage (for example) > >> knows that the device is in use, it can block suspends by returning > >> -EBUSY from its own suspend callback. The UDC driver doesn't need to > >> worry about these matters; it should assume that such things have > >> already been handled elsewhere. That's what Dmitry meant when he was > >> talking about a "higher level driver" -- maybe "lower level" would have > >> been a better choice of words. :-) > The suspend at driver level should not care about filesystem in use, etc > ... A disconnected cable cannot be prevented by the kernel, and the > effect in fine is the same as the suspend. This is a little ironic, because the mass-storage gadget driver has no idea whether a filesystem is in use or not. All it knows is whether it is currently connected to a host. Besides, it's questionable whether the gadget driver should prevent the gadget from suspending. This is a policy matter which must be decided by the user, not by the kernel. Nevertheless... The fact that the kernel can do nothing about a disconnected cable shouldn't stop us from handling system sleep correctly. After all, the kernel _can_ do something about that. Alan Stern
On 06/25/2011 02:02 PM, Alan Stern wrote: > Nevertheless... The fact that the kernel can do nothing about a > disconnected cable shouldn't stop us from handling system sleep > correctly. After all, the kernel _can_ do something about that. Well, it depends of which level of driver we speek about. I was speeking about UDC driver, ie. a hardware driver. My point was that the hardware driver should only care about the hardware register manipulations, and check their success. If they succeed, then it should suspend itself without care of filesystem not in sync. I didn't meant that the filesystem driver shouldn't care about the suspend, or even the mass-storage gadget. I don't have a strong opinion on these higher levels. The question if the kernel can do something about it is much more complicated : - should userspace sync/umount and check filesystem before suspending (as umount usb storage before suspending) - should the kernel block a suspend because a usb key was "forgotten" in an USB slot ? For these questions, I'll let others battle. For the hardware related drivers, I'm pretty convinced all they should care about is the success or failure of hardware suspend manipulation, and the correct suspend order. Cheers. -- Robert
On Sat, Jun 25, 2011 at 11:26:09AM +0200, Robert Jarzmik wrote: > On 06/22/2011 05:19 PM, Felipe Balbi wrote: > >Hi, > > > >On Wed, Jun 22, 2011 at 11:02:27AM -0400, Alan Stern wrote: > >>No, the patch is appropriate. > Indeed. > >> > >>We don't need better communication. If g_mass_storage (for example) > >>knows that the device is in use, it can block suspends by returning > >>-EBUSY from its own suspend callback. The UDC driver doesn't need to > >>worry about these matters; it should assume that such things have > >>already been handled elsewhere. That's what Dmitry meant when he was > >>talking about a "higher level driver" -- maybe "lower level" would have > >>been a better choice of words. :-) > The suspend at driver level should not care about filesystem in use, > etc ... A disconnected cable cannot be prevented by the kernel, and > the effect in fine is the same as the suspend. > > >>Until recently, ordering of the suspend callbacks didn't present any > >>problem. The gadget device was a child of the UDC device and therefore > >>its suspend callback would always be invoked first. > >> > >>With the new UDC framework, I don't know if this is true any more. > >>It's something to consider. > That true also for the UDC driver, which should be suspended before > the transciever, so that it can complete its shutdown properly. > That's what bothers me with the patch. > > And we should not confuse the 2 suspends : > (1) driver suspend, which happens at suspendToRam or suspendToDisk > (the one we're discussing here) I think Dmitry's patch is for this system suspend, and less power consumption is very important for system design, esp for embedded device. > (2) USB suspend (which if I remember correctly is a special USB > command while the USB bus remains powered). This suspend is USB suspend in USB 2.0 spec, it is usually triggered by Usb controller after bus idle 3ms. It should not disable D+ pull-up, or the bus will not be J, and the host will think the device has disconnected. > > The order of suspend in (1) requires : > - gadget (gzero, gether, ...) suspended first > - UDC suspended second > - transceiver suspended last > > The order of suspend in (2) doesn't require anything AFAIR. > > For order in (1) : > - gadget before UDC should be enforced by the framework, as the UDC > usb_gadget_probe_driver() method calls device_add() on the gadget< > - UDC before transceiver is enforced in some drivers by directly > manipulating the gpio line. > > So Dimitry, do you have an idea as how to guarantee order of suspend > in (1), between UDC driver and gpio_vbus ? Maybe an otg_*() call > would do the trick ? > > Cheers. > > -- > Robert > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" 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/otg/gpio_vbus.c b/drivers/usb/otg/gpio_vbus.c index 52733d9..44527bd 100644 --- a/drivers/usb/otg/gpio_vbus.c +++ b/drivers/usb/otg/gpio_vbus.c @@ -327,6 +327,34 @@ static int __exit gpio_vbus_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM +static int gpio_vbus_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); + struct gpio_vbus_mach_info *pdata = gpio_vbus->dev->platform_data; + + if (gpio_vbus->otg.gadget && pdata->disconnect_on_suspend) { + /* optionally disable D+ pullup */ + if (gpio_is_valid(pdata->gpio_pullup)) + gpio_set_value(pdata->gpio_pullup, + pdata->gpio_pullup_inverted); + + set_vbus_draw(gpio_vbus, 0); + } + return 0; +} + +static int gpio_vbus_resume(struct platform_device *pdev) +{ + struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev); + + if (gpio_vbus->otg.gadget) + schedule_work(&gpio_vbus->work); + + return 0; +} +#endif + /* NOTE: the gpio-vbus device may *NOT* be hotplugged */ MODULE_ALIAS("platform:gpio-vbus"); @@ -337,6 +365,10 @@ static struct platform_driver gpio_vbus_driver = { .owner = THIS_MODULE, }, .remove = __exit_p(gpio_vbus_remove), +#ifdef CONFIG_PM + .suspend = gpio_vbus_suspend, + .resume = gpio_vbus_resume +#endif }; static int __init gpio_vbus_init(void) diff --git a/include/linux/usb/gpio_vbus.h b/include/linux/usb/gpio_vbus.h index d9f03cc..2faa38d 100644 --- a/include/linux/usb/gpio_vbus.h +++ b/include/linux/usb/gpio_vbus.h @@ -27,4 +27,5 @@ struct gpio_vbus_mach_info { int gpio_pullup; bool gpio_vbus_inverted; bool gpio_pullup_inverted; + bool disconnect_on_suspend; };
Some platforms would like to disable D+ pullup on suspend, to drain as low power, as possible. E.g. this was requested by mioa701 board maintainers. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- drivers/usb/otg/gpio_vbus.c | 32 ++++++++++++++++++++++++++++++++ include/linux/usb/gpio_vbus.h | 1 + 2 files changed, 33 insertions(+), 0 deletions(-)