[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Dmitry Eremin-Solenikov]

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

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;
 };
-- 
1.7.5.3

[PATCH 2/2] mioa701: move gpio-pullup functionality to gpio-vbus

[Dmitry Eremin-Solenikov]

gpio-vbus can (and should?) handle D+ pullup gpio for us. Move gpio-pullup handling
to gpio-vbus and stop providing udc_info as it's empty now.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 arch/arm/mach-pxa/mioa701.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
index bcbd359..e712c9d 100644
--- a/arch/arm/mach-pxa/mioa701.c
+++ b/arch/arm/mach-pxa/mioa701.c
@@ -402,14 +402,10 @@ static void gsm_exit(void)
 /*
  * USB UDC
  */
-static struct pxa2xx_udc_mach_info mioa701_udc_info = {
-	.gpio_pullup	  = GPIO22_USB_ENABLE,
-};
-
 struct gpio_vbus_mach_info gpio_vbus_data = {
 	.gpio_vbus = GPIO13_nUSB_DETECT,
 	.gpio_vbus_inverted = 1,
-	.gpio_pullup = -1,
+	.gpio_pullup = GPIO22_USB_ENABLE,
 };
 
 /*
@@ -770,7 +766,6 @@ static void __init mioa701_machine_init(void)
 	pxa_set_fb_info(NULL, &mioa701_pxafb_info);
 	pxa_set_mci_info(&mioa701_mci_info);
 	pxa_set_keypad_info(&mioa701_keypad_info);
-	pxa_set_udc_info(&mioa701_udc_info);
 	pxa_set_ac97_info(&mioa701_ac97_info);
 	pm_power_off = mioa701_poweroff;
 	arm_pm_restart = mioa701_restart;
-- 
1.7.5.3

[PATCH 2/2] mioa701: move gpio-pullup functionality to gpio-vbus

[Dmitry Eremin-Solenikov]

Oops... See bellow,

On 6/22/11, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote:
> gpio-vbus can (and should?) handle D+ pullup gpio for us. Move gpio-pullup
> handling
> to gpio-vbus and stop providing udc_info as it's empty now.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  arch/arm/mach-pxa/mioa701.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
> index bcbd359..e712c9d 100644
> --- a/arch/arm/mach-pxa/mioa701.c
> +++ b/arch/arm/mach-pxa/mioa701.c
> @@ -402,14 +402,10 @@ static void gsm_exit(void)
>  /*
>   * USB UDC
>   */
> -static struct pxa2xx_udc_mach_info mioa701_udc_info = {
> -	.gpio_pullup	  = GPIO22_USB_ENABLE,
> -};
> -
>  struct gpio_vbus_mach_info gpio_vbus_data = {
>  	.gpio_vbus = GPIO13_nUSB_DETECT,
>  	.gpio_vbus_inverted = 1,
> -	.gpio_pullup = -1,
> +	.gpio_pullup = GPIO22_USB_ENABLE,

This should be also
+	.disconnect_on_suspend = 1,

>  };
>
>  /*
> @@ -770,7 +766,6 @@ static void __init mioa701_machine_init(void)
>  	pxa_set_fb_info(NULL, &mioa701_pxafb_info);
>  	pxa_set_mci_info(&mioa701_mci_info);
>  	pxa_set_keypad_info(&mioa701_keypad_info);
> -	pxa_set_udc_info(&mioa701_udc_info);
>  	pxa_set_ac97_info(&mioa701_ac97_info);
>  	pm_power_off = mioa701_poweroff;
>  	arm_pm_restart = mioa701_restart;
> --
> 1.7.5.3
>
>


-- 
With best wishes
Dmitry

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Felipe Balbi]

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.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110622/caa12764/attachment.sig>

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Dmitry Eremin-Solenikov]

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.


-- 
With best wishes
Dmitry

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Felipe Balbi]

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)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110622/53a18614/attachment.sig>

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Dmitry Eremin-Solenikov]

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.


-- 
With best wishes
Dmitry

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Felipe Balbi]

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 ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110622/7a844ca7/attachment.sig>

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Dmitry Eremin-Solenikov]

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.

-- 
With best wishes
Dmitry

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Alan Stern]

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

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Felipe Balbi]

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

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110622/50aed419/attachment.sig>

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Felipe Balbi]

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 ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110622/555e71af/attachment.sig>

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Alan Stern]

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

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Felipe Balbi]

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.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110622/4af441b7/attachment.sig>

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Robert Jarzmik]

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

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Dmitry Eremin-Solenikov]

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.

-- 
With best wishes
Dmitry

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Alan Stern]

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

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Robert Jarzmik]

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

[PATCH 1/2] gpio-vbus: support disabling D+ pullup on suspend

[Peter Chen]

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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Best Regards,
Peter Chen