All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Add vbus draw support to DWC3
@ 2020-12-29 23:03 Wesley Cheng
  2020-12-29 23:03 ` [PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback Wesley Cheng
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Wesley Cheng @ 2020-12-29 23:03 UTC (permalink / raw)
  To: peter.chen, balbi, gregkh; +Cc: linux-kernel, linux-usb, Wesley Cheng

Some devices are connected to standard downstream ports (SDP) and draw current
from them.  The current rates are defined in the BC1.2 specification, and
highlights the different charge rates depending on the device state.  The DWC3
gadget does not currently have a mechanism to notify external drivers about
how much current can be drawn.

The current rates are notified by the USB gadget layer, and the DWC3 gadget will
propagate this potentially to external charger drivers.  Also, the USB gadget
needs to be fixed to only allow 100mA current draw when receiving a bus reset
from the host, as the BC1.2 specification states that this is the max current
draw possible when in the connected and unconfigured state.

Wesley Cheng (3):
  usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback
  usb: gadget: composite: Split composite reset and disconnect
  usb: gadget: configfs: Add a specific configFS reset callback

 drivers/usb/dwc3/gadget.c      | 11 +++++++++++
 drivers/usb/gadget/composite.c | 21 +++++++++++++++++++--
 drivers/usb/gadget/configfs.c  | 24 +++++++++++++++++++++++-
 include/linux/usb/composite.h  |  2 ++
 4 files changed, 55 insertions(+), 3 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback
  2020-12-29 23:03 [PATCH 0/3] Add vbus draw support to DWC3 Wesley Cheng
@ 2020-12-29 23:03 ` Wesley Cheng
  2020-12-29 23:03 ` [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect Wesley Cheng
  2020-12-29 23:03 ` [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback Wesley Cheng
  2 siblings, 0 replies; 14+ messages in thread
From: Wesley Cheng @ 2020-12-29 23:03 UTC (permalink / raw)
  To: peter.chen, balbi, gregkh; +Cc: linux-kernel, linux-usb, Wesley Cheng

Some devices support charging while in device mode.  In these situations,
the USB gadget will notify the DWC3 gadget driver to modify the current
based on the enumeration and device state.  The usb_phy_set_power() API
will allow external charger entities to adjust the charge current through
the notifier block.

Reviewed-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/dwc3/gadget.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c145da1d8ba5..69122f66978e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2387,6 +2387,16 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 
+static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
+{
+	struct dwc3		*dwc = gadget_to_dwc(g);
+
+	if (dwc->usb2_phy)
+		return usb_phy_set_power(dwc->usb2_phy, mA);
+
+	return 0;
+}
+
 static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.get_frame		= dwc3_gadget_get_frame,
 	.wakeup			= dwc3_gadget_wakeup,
@@ -2396,6 +2406,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.udc_stop		= dwc3_gadget_stop,
 	.udc_set_speed		= dwc3_gadget_set_speed,
 	.get_config_params	= dwc3_gadget_config_params,
+	.vbus_draw		= dwc3_gadget_vbus_draw,
 };
 
 /* -------------------------------------------------------------------------- */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2020-12-29 23:03 [PATCH 0/3] Add vbus draw support to DWC3 Wesley Cheng
  2020-12-29 23:03 ` [PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback Wesley Cheng
@ 2020-12-29 23:03 ` Wesley Cheng
  2021-01-05 13:14   ` Felipe Balbi
  2020-12-29 23:03 ` [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback Wesley Cheng
  2 siblings, 1 reply; 14+ messages in thread
From: Wesley Cheng @ 2020-12-29 23:03 UTC (permalink / raw)
  To: peter.chen, balbi, gregkh; +Cc: linux-kernel, linux-usb, Wesley Cheng

Add a specific composite reset API to differentiate between disconnect and
reset events.  This is needed for adjusting the current draw accordingly
based on the USB battery charging specification.  The device is only allowed
to draw the 500/900 mA (HS/SS) while in the CONFIGURED state, and only 100 mA
in the connected and UNCONFIGURED state.

Reviewed-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/gadget/composite.c | 21 +++++++++++++++++++--
 include/linux/usb/composite.h  |  2 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 05b176c82cc5..a41f7fe4b518 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2036,7 +2036,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 	return value;
 }
 
-void composite_disconnect(struct usb_gadget *gadget)
+static void __composite_disconnect(struct usb_gadget *gadget)
 {
 	struct usb_composite_dev	*cdev = get_gadget_data(gadget);
 	unsigned long			flags;
@@ -2053,6 +2053,23 @@ void composite_disconnect(struct usb_gadget *gadget)
 	spin_unlock_irqrestore(&cdev->lock, flags);
 }
 
+void composite_disconnect(struct usb_gadget *gadget)
+{
+	usb_gadget_vbus_draw(gadget, 0);
+	__composite_disconnect(gadget);
+}
+
+void composite_reset(struct usb_gadget *gadget)
+{
+	/*
+	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
+	 * specification v1.2 states that a device connected on a SDP shall only
+	 * draw at max 100mA while in a connected, but unconfigured state.
+	 */
+	usb_gadget_vbus_draw(gadget, 100);
+	__composite_disconnect(gadget);
+}
+
 /*-------------------------------------------------------------------------*/
 
 static ssize_t suspended_show(struct device *dev, struct device_attribute *attr,
@@ -2373,7 +2390,7 @@ static const struct usb_gadget_driver composite_driver_template = {
 	.unbind		= composite_unbind,
 
 	.setup		= composite_setup,
-	.reset		= composite_disconnect,
+	.reset		= composite_reset,
 	.disconnect	= composite_disconnect,
 
 	.suspend	= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2040696d75b6..0d8a71471512 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -525,6 +525,8 @@ extern struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev,
 extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
 
 extern void composite_disconnect(struct usb_gadget *gadget);
+extern void composite_reset(struct usb_gadget *gadget);
+
 extern int composite_setup(struct usb_gadget *gadget,
 		const struct usb_ctrlrequest *ctrl);
 extern void composite_suspend(struct usb_gadget *gadget);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
  2020-12-29 23:03 [PATCH 0/3] Add vbus draw support to DWC3 Wesley Cheng
  2020-12-29 23:03 ` [PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback Wesley Cheng
  2020-12-29 23:03 ` [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect Wesley Cheng
@ 2020-12-29 23:03 ` Wesley Cheng
  2021-01-04 15:45   ` Greg KH
  2 siblings, 1 reply; 14+ messages in thread
From: Wesley Cheng @ 2020-12-29 23:03 UTC (permalink / raw)
  To: peter.chen, balbi, gregkh; +Cc: linux-kernel, linux-usb, Wesley Cheng

In order for configFS based USB gadgets to set the proper charge current
for bus reset scenarios, expose a separate reset callback to set the
current to 100mA based on the USB battery charging specification.

Reviewed-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/gadget/configfs.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 56051bb97349..80ca7ff2fb97 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct usb_gadget *gadget)
 	spin_unlock_irqrestore(&gi->spinlock, flags);
 }
 
+static void configfs_composite_reset(struct usb_gadget *gadget)
+{
+	struct usb_composite_dev *cdev;
+	struct gadget_info *gi;
+	unsigned long flags;
+
+	cdev = get_gadget_data(gadget);
+	if (!cdev)
+		return;
+
+	gi = container_of(cdev, struct gadget_info, cdev);
+	spin_lock_irqsave(&gi->spinlock, flags);
+	cdev = get_gadget_data(gadget);
+	if (!cdev || gi->unbind) {
+		spin_unlock_irqrestore(&gi->spinlock, flags);
+		return;
+	}
+
+	composite_reset(gadget);
+	spin_unlock_irqrestore(&gi->spinlock, flags);
+}
+
 static void configfs_composite_suspend(struct usb_gadget *gadget)
 {
 	struct usb_composite_dev *cdev;
@@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver configfs_driver_template = {
 	.unbind         = configfs_composite_unbind,
 
 	.setup          = configfs_composite_setup,
-	.reset          = configfs_composite_disconnect,
+	.reset          = configfs_composite_reset,
 	.disconnect     = configfs_composite_disconnect,
 
 	.suspend	= configfs_composite_suspend,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
  2020-12-29 23:03 ` [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback Wesley Cheng
@ 2021-01-04 15:45   ` Greg KH
  2021-01-04 19:03     ` Wesley Cheng
  0 siblings, 1 reply; 14+ messages in thread
From: Greg KH @ 2021-01-04 15:45 UTC (permalink / raw)
  To: Wesley Cheng; +Cc: peter.chen, balbi, linux-kernel, linux-usb

On Tue, Dec 29, 2020 at 03:03:31PM -0800, Wesley Cheng wrote:
> In order for configFS based USB gadgets to set the proper charge current
> for bus reset scenarios, expose a separate reset callback to set the
> current to 100mA based on the USB battery charging specification.
> 
> Reviewed-by: Peter Chen <peter.chen@nxp.com>
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> ---
>  drivers/usb/gadget/configfs.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 56051bb97349..80ca7ff2fb97 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct usb_gadget *gadget)
>  	spin_unlock_irqrestore(&gi->spinlock, flags);
>  }
>  
> +static void configfs_composite_reset(struct usb_gadget *gadget)
> +{
> +	struct usb_composite_dev *cdev;
> +	struct gadget_info *gi;
> +	unsigned long flags;
> +
> +	cdev = get_gadget_data(gadget);
> +	if (!cdev)
> +		return;
> +
> +	gi = container_of(cdev, struct gadget_info, cdev);
> +	spin_lock_irqsave(&gi->spinlock, flags);
> +	cdev = get_gadget_data(gadget);
> +	if (!cdev || gi->unbind) {
> +		spin_unlock_irqrestore(&gi->spinlock, flags);
> +		return;
> +	}
> +
> +	composite_reset(gadget);
> +	spin_unlock_irqrestore(&gi->spinlock, flags);
> +}
> +
>  static void configfs_composite_suspend(struct usb_gadget *gadget)
>  {
>  	struct usb_composite_dev *cdev;
> @@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver configfs_driver_template = {
>  	.unbind         = configfs_composite_unbind,
>  
>  	.setup          = configfs_composite_setup,
> -	.reset          = configfs_composite_disconnect,
> +	.reset          = configfs_composite_reset,
>  	.disconnect     = configfs_composite_disconnect,
>  
>  	.suspend	= configfs_composite_suspend,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

So this changes the existing userspace functionality?  What will break
because of this now unexpected change?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
  2021-01-04 15:45   ` Greg KH
@ 2021-01-04 19:03     ` Wesley Cheng
  0 siblings, 0 replies; 14+ messages in thread
From: Wesley Cheng @ 2021-01-04 19:03 UTC (permalink / raw)
  To: Greg KH; +Cc: peter.chen, balbi, linux-kernel, linux-usb



On 1/4/2021 7:45 AM, Greg KH wrote:
> On Tue, Dec 29, 2020 at 03:03:31PM -0800, Wesley Cheng wrote:
>> In order for configFS based USB gadgets to set the proper charge current
>> for bus reset scenarios, expose a separate reset callback to set the
>> current to 100mA based on the USB battery charging specification.
>>
>> Reviewed-by: Peter Chen <peter.chen@nxp.com>
>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>> ---
>>  drivers/usb/gadget/configfs.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 56051bb97349..80ca7ff2fb97 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct usb_gadget *gadget)
>>  	spin_unlock_irqrestore(&gi->spinlock, flags);
>>  }
>>  
>> +static void configfs_composite_reset(struct usb_gadget *gadget)
>> +{
>> +	struct usb_composite_dev *cdev;
>> +	struct gadget_info *gi;
>> +	unsigned long flags;
>> +
>> +	cdev = get_gadget_data(gadget);
>> +	if (!cdev)
>> +		return;
>> +
>> +	gi = container_of(cdev, struct gadget_info, cdev);
>> +	spin_lock_irqsave(&gi->spinlock, flags);
>> +	cdev = get_gadget_data(gadget);
>> +	if (!cdev || gi->unbind) {
>> +		spin_unlock_irqrestore(&gi->spinlock, flags);
>> +		return;
>> +	}
>> +
>> +	composite_reset(gadget);
>> +	spin_unlock_irqrestore(&gi->spinlock, flags);
>> +}
>> +
>>  static void configfs_composite_suspend(struct usb_gadget *gadget)
>>  {
>>  	struct usb_composite_dev *cdev;
>> @@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver configfs_driver_template = {
>>  	.unbind         = configfs_composite_unbind,
>>  
>>  	.setup          = configfs_composite_setup,
>> -	.reset          = configfs_composite_disconnect,
>> +	.reset          = configfs_composite_reset,
>>  	.disconnect     = configfs_composite_disconnect,
>>  
>>  	.suspend	= configfs_composite_suspend,
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> 
> So this changes the existing userspace functionality?  What will break
> because of this now unexpected change?
> 
> thanks,
> 
> greg k-h
> 

Hi Greg,

Happy new years!  This wouldn't affect the userspace interaction with
configFS, as this is modifying the reset callback for the UDC core.  The
reset callback is only executed during usb_gadget_udc_reset(), which is
specifically run when vendor UDC drivers (i.e. DWC3 gadget) receive a
USB bus reset interrupt.  This is similar to the composite.c patch,
because for configFS based gadgets, they do not directly register the
USB composite ops and have their own routines.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2020-12-29 23:03 ` [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect Wesley Cheng
@ 2021-01-05 13:14   ` Felipe Balbi
  2021-01-08  2:19     ` Thinh Nguyen
  0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2021-01-05 13:14 UTC (permalink / raw)
  To: Wesley Cheng, peter.chen, gregkh; +Cc: linux-kernel, linux-usb, Wesley Cheng


Hi,

Wesley Cheng <wcheng@codeaurora.org> writes:
> +void composite_reset(struct usb_gadget *gadget)
> +{
> +	/*
> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
> +	 * specification v1.2 states that a device connected on a SDP shall only
> +	 * draw at max 100mA while in a connected, but unconfigured state.

The requirements are different, though. I think OTG spec has some extra
requirements where only 8mA can be drawn max. You need to check for the
otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
can't draw 100mA (IIRC).

-- 
balbi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2021-01-05 13:14   ` Felipe Balbi
@ 2021-01-08  2:19     ` Thinh Nguyen
  2021-01-08  8:35       ` Jack Pham
  2021-01-08  9:13       ` gregkh
  0 siblings, 2 replies; 14+ messages in thread
From: Thinh Nguyen @ 2021-01-08  2:19 UTC (permalink / raw)
  To: Felipe Balbi, Wesley Cheng, peter.chen, gregkh; +Cc: linux-kernel, linux-usb

Hi Wesley,

Felipe Balbi wrote:
> Hi,
>
> Wesley Cheng <wcheng@codeaurora.org> writes:
>> +void composite_reset(struct usb_gadget *gadget)
>> +{
>> +	/*
>> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
>> +	 * specification v1.2 states that a device connected on a SDP shall only
>> +	 * draw at max 100mA while in a connected, but unconfigured state.
> The requirements are different, though. I think OTG spec has some extra
> requirements where only 8mA can be drawn max. You need to check for the
> otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
> can't draw 100mA (IIRC).
>

We see issue with this patch series. For our device running at SSP, the
device couldn't recover from a port reset and remained in eSS.Inactive
state.

This patch series is already in Greg's usb-testing. Please review and
help fix it.

We can see the failure once the patch "usb: gadget: configfs: Add a
specific configFS reset callback" is introduced.

Thanks,
Thinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2021-01-08  2:19     ` Thinh Nguyen
@ 2021-01-08  8:35       ` Jack Pham
  2021-01-08 22:04         ` Thinh Nguyen
  2021-01-08  9:13       ` gregkh
  1 sibling, 1 reply; 14+ messages in thread
From: Jack Pham @ 2021-01-08  8:35 UTC (permalink / raw)
  To: Thinh Nguyen
  Cc: Felipe Balbi, Wesley Cheng, peter.chen, gregkh, linux-kernel, linux-usb

Hi Thinh,

On Fri, Jan 08, 2021 at 02:19:30AM +0000, Thinh Nguyen wrote:
> Hi Wesley,
> 
> Felipe Balbi wrote:
> > Hi,
> >
> > Wesley Cheng <wcheng@codeaurora.org> writes:
> >> +void composite_reset(struct usb_gadget *gadget)
> >> +{
> >> +	/*
> >> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
> >> +	 * specification v1.2 states that a device connected on a SDP shall only
> >> +	 * draw at max 100mA while in a connected, but unconfigured state.
> > The requirements are different, though. I think OTG spec has some extra
> > requirements where only 8mA can be drawn max. You need to check for the
> > otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
> > can't draw 100mA (IIRC).
> >
> 
> We see issue with this patch series. For our device running at SSP, the
> device couldn't recover from a port reset and remained in eSS.Inactive
> state.
> 
> This patch series is already in Greg's usb-testing. Please review and
> help fix it.
> 
> We can see the failure once the patch "usb: gadget: configfs: Add a
> specific configFS reset callback" is introduced.

Hmm. Does your device use a legacy USB2 PHY (not generic PHY) driver,
and does it implement the usb_phy_set_power() callback? Because
otherwise this new configfs_composite_reset() callback would not have
changed from the original behavior since the newly introduced (in patch
1/3) dwc3_gadget_vbus_draw() callback would simply be a no-op if
dwc->usb2_phy is not present.

If it does turn out to be something with your PHY driver's set_power(),
it's still puzzling since it's directed to only the usb2_phy, so I'm
curious how telling it to draw 100mA could affect SSP operation at all.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2021-01-08  2:19     ` Thinh Nguyen
  2021-01-08  8:35       ` Jack Pham
@ 2021-01-08  9:13       ` gregkh
  2021-01-08 22:13         ` Thinh Nguyen
  1 sibling, 1 reply; 14+ messages in thread
From: gregkh @ 2021-01-08  9:13 UTC (permalink / raw)
  To: Thinh Nguyen
  Cc: Felipe Balbi, Wesley Cheng, peter.chen, linux-kernel, linux-usb

On Fri, Jan 08, 2021 at 02:19:30AM +0000, Thinh Nguyen wrote:
> Hi Wesley,
> 
> Felipe Balbi wrote:
> > Hi,
> >
> > Wesley Cheng <wcheng@codeaurora.org> writes:
> >> +void composite_reset(struct usb_gadget *gadget)
> >> +{
> >> +	/*
> >> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
> >> +	 * specification v1.2 states that a device connected on a SDP shall only
> >> +	 * draw at max 100mA while in a connected, but unconfigured state.
> > The requirements are different, though. I think OTG spec has some extra
> > requirements where only 8mA can be drawn max. You need to check for the
> > otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
> > can't draw 100mA (IIRC).
> >
> 
> We see issue with this patch series. For our device running at SSP, the
> device couldn't recover from a port reset and remained in eSS.Inactive
> state.
> 
> This patch series is already in Greg's usb-testing. Please review and
> help fix it.

Should I just revert this?  I'll be glad to drop it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2021-01-08  8:35       ` Jack Pham
@ 2021-01-08 22:04         ` Thinh Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Thinh Nguyen @ 2021-01-08 22:04 UTC (permalink / raw)
  To: Jack Pham, Thinh Nguyen
  Cc: Felipe Balbi, Wesley Cheng, peter.chen, gregkh, linux-kernel, linux-usb

Jack Pham wrote:
> Hi Thinh,
>
> On Fri, Jan 08, 2021 at 02:19:30AM +0000, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Wesley Cheng <wcheng@codeaurora.org> writes:
>>>> +void composite_reset(struct usb_gadget *gadget)
>>>> +{
>>>> +	/*
>>>> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
>>>> +	 * specification v1.2 states that a device connected on a SDP shall only
>>>> +	 * draw at max 100mA while in a connected, but unconfigured state.
>>> The requirements are different, though. I think OTG spec has some extra
>>> requirements where only 8mA can be drawn max. You need to check for the
>>> otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
>>> can't draw 100mA (IIRC).
>>>
>> We see issue with this patch series. For our device running at SSP, the
>> device couldn't recover from a port reset and remained in eSS.Inactive
>> state.
>>
>> This patch series is already in Greg's usb-testing. Please review and
>> help fix it.
>>
>> We can see the failure once the patch "usb: gadget: configfs: Add a
>> specific configFS reset callback" is introduced.
> Hmm. Does your device use a legacy USB2 PHY (not generic PHY) driver,
> and does it implement the usb_phy_set_power() callback? Because
> otherwise this new configfs_composite_reset() callback would not have
> changed from the original behavior since the newly introduced (in patch
> 1/3) dwc3_gadget_vbus_draw() callback would simply be a no-op if
> dwc->usb2_phy is not present.
>
> If it does turn out to be something with your PHY driver's set_power(),
> it's still puzzling since it's directed to only the usb2_phy, so I'm
> curious how telling it to draw 100mA could affect SSP operation at all.
>
> Thanks,
> Jack

So, I ran some more tests. It seems like this new change affects some
timing in my setup that triggers this failure. I tried to add some
printouts to look into it further, but somehow that reduces the failure
rate significantly. This doesn't seem related to power drawing as it
doesn't update usb2_phy for SSP as you said.

After switching my HW setup, the problem seems to go away. I guess we
can ignore this since the code path looks to be the same as previous.

BR,
Thinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2021-01-08  9:13       ` gregkh
@ 2021-01-08 22:13         ` Thinh Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Thinh Nguyen @ 2021-01-08 22:13 UTC (permalink / raw)
  To: gregkh, Thinh Nguyen
  Cc: Felipe Balbi, Wesley Cheng, peter.chen, linux-kernel, linux-usb

gregkh@linuxfoundation.org wrote:
> On Fri, Jan 08, 2021 at 02:19:30AM +0000, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Wesley Cheng <wcheng@codeaurora.org> writes:
>>>> +void composite_reset(struct usb_gadget *gadget)
>>>> +{
>>>> +	/*
>>>> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
>>>> +	 * specification v1.2 states that a device connected on a SDP shall only
>>>> +	 * draw at max 100mA while in a connected, but unconfigured state.
>>> The requirements are different, though. I think OTG spec has some extra
>>> requirements where only 8mA can be drawn max. You need to check for the
>>> otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
>>> can't draw 100mA (IIRC).
>>>
>> We see issue with this patch series. For our device running at SSP, the
>> device couldn't recover from a port reset and remained in eSS.Inactive
>> state.
>>
>> This patch series is already in Greg's usb-testing. Please review and
>> help fix it.
> Should I just revert this?  I'll be glad to drop it.
>
> thanks,
>
> greg k-h

Unless there's some other issue, let's not do so as it seems to be
related to my HW stability than any SW issue. Sorry for the noise.

BR,
Thinh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2020-11-14  8:12 ` [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect Wesley Cheng
@ 2020-11-16 13:44   ` Peter Chen
  0 siblings, 0 replies; 14+ messages in thread
From: Peter Chen @ 2020-11-16 13:44 UTC (permalink / raw)
  To: Wesley Cheng; +Cc: balbi, gregkh, linux-kernel, linux-usb, jackp

On 20-11-14 00:12:46, Wesley Cheng wrote:
> Add a specific composite reset API to differentiate between disconnect and
> reset events.  This is needed for adjusting the current draw accordingly
> based on the USB battery charging specification.  The device is only allowed
> to draw the 500/900 mA (HS/SS) while in the CONFIGURED state, and only 100 mA
> in the connected and UNCONFIGURED state.
> 
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>

Reviewed-by: Peter Chen <peter.chen@nxp.com>

Peter
> ---
>  drivers/usb/gadget/composite.c | 21 +++++++++++++++++++--
>  include/linux/usb/composite.h  |  2 ++
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 05b176c82cc5..a41f7fe4b518 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2036,7 +2036,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  	return value;
>  }
>  
> -void composite_disconnect(struct usb_gadget *gadget)
> +static void __composite_disconnect(struct usb_gadget *gadget)
>  {
>  	struct usb_composite_dev	*cdev = get_gadget_data(gadget);
>  	unsigned long			flags;
> @@ -2053,6 +2053,23 @@ void composite_disconnect(struct usb_gadget *gadget)
>  	spin_unlock_irqrestore(&cdev->lock, flags);
>  }
>  
> +void composite_disconnect(struct usb_gadget *gadget)
> +{
> +	usb_gadget_vbus_draw(gadget, 0);
> +	__composite_disconnect(gadget);
> +}
> +
> +void composite_reset(struct usb_gadget *gadget)
> +{
> +	/*
> +	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
> +	 * specification v1.2 states that a device connected on a SDP shall only
> +	 * draw at max 100mA while in a connected, but unconfigured state.
> +	 */
> +	usb_gadget_vbus_draw(gadget, 100);
> +	__composite_disconnect(gadget);
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  
>  static ssize_t suspended_show(struct device *dev, struct device_attribute *attr,
> @@ -2373,7 +2390,7 @@ static const struct usb_gadget_driver composite_driver_template = {
>  	.unbind		= composite_unbind,
>  
>  	.setup		= composite_setup,
> -	.reset		= composite_disconnect,
> +	.reset		= composite_reset,
>  	.disconnect	= composite_disconnect,
>  
>  	.suspend	= composite_suspend,
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index 2040696d75b6..0d8a71471512 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -525,6 +525,8 @@ extern struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev,
>  extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
>  
>  extern void composite_disconnect(struct usb_gadget *gadget);
> +extern void composite_reset(struct usb_gadget *gadget);
> +
>  extern int composite_setup(struct usb_gadget *gadget,
>  		const struct usb_ctrlrequest *ctrl);
>  extern void composite_suspend(struct usb_gadget *gadget);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
  2020-11-14  8:12 [PATCH 0/3] Add vbus draw support to DWC3 Wesley Cheng
@ 2020-11-14  8:12 ` Wesley Cheng
  2020-11-16 13:44   ` Peter Chen
  0 siblings, 1 reply; 14+ messages in thread
From: Wesley Cheng @ 2020-11-14  8:12 UTC (permalink / raw)
  To: balbi, gregkh; +Cc: linux-kernel, linux-usb, jackp, Wesley Cheng

Add a specific composite reset API to differentiate between disconnect and
reset events.  This is needed for adjusting the current draw accordingly
based on the USB battery charging specification.  The device is only allowed
to draw the 500/900 mA (HS/SS) while in the CONFIGURED state, and only 100 mA
in the connected and UNCONFIGURED state.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/gadget/composite.c | 21 +++++++++++++++++++--
 include/linux/usb/composite.h  |  2 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 05b176c82cc5..a41f7fe4b518 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2036,7 +2036,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 	return value;
 }
 
-void composite_disconnect(struct usb_gadget *gadget)
+static void __composite_disconnect(struct usb_gadget *gadget)
 {
 	struct usb_composite_dev	*cdev = get_gadget_data(gadget);
 	unsigned long			flags;
@@ -2053,6 +2053,23 @@ void composite_disconnect(struct usb_gadget *gadget)
 	spin_unlock_irqrestore(&cdev->lock, flags);
 }
 
+void composite_disconnect(struct usb_gadget *gadget)
+{
+	usb_gadget_vbus_draw(gadget, 0);
+	__composite_disconnect(gadget);
+}
+
+void composite_reset(struct usb_gadget *gadget)
+{
+	/*
+	 * Section 1.4.13 Standard Downstream Port of the USB battery charging
+	 * specification v1.2 states that a device connected on a SDP shall only
+	 * draw at max 100mA while in a connected, but unconfigured state.
+	 */
+	usb_gadget_vbus_draw(gadget, 100);
+	__composite_disconnect(gadget);
+}
+
 /*-------------------------------------------------------------------------*/
 
 static ssize_t suspended_show(struct device *dev, struct device_attribute *attr,
@@ -2373,7 +2390,7 @@ static const struct usb_gadget_driver composite_driver_template = {
 	.unbind		= composite_unbind,
 
 	.setup		= composite_setup,
-	.reset		= composite_disconnect,
+	.reset		= composite_reset,
 	.disconnect	= composite_disconnect,
 
 	.suspend	= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2040696d75b6..0d8a71471512 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -525,6 +525,8 @@ extern struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev,
 extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
 
 extern void composite_disconnect(struct usb_gadget *gadget);
+extern void composite_reset(struct usb_gadget *gadget);
+
 extern int composite_setup(struct usb_gadget *gadget,
 		const struct usb_ctrlrequest *ctrl);
 extern void composite_suspend(struct usb_gadget *gadget);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-01-08 22:15 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-29 23:03 [PATCH 0/3] Add vbus draw support to DWC3 Wesley Cheng
2020-12-29 23:03 ` [PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback Wesley Cheng
2020-12-29 23:03 ` [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect Wesley Cheng
2021-01-05 13:14   ` Felipe Balbi
2021-01-08  2:19     ` Thinh Nguyen
2021-01-08  8:35       ` Jack Pham
2021-01-08 22:04         ` Thinh Nguyen
2021-01-08  9:13       ` gregkh
2021-01-08 22:13         ` Thinh Nguyen
2020-12-29 23:03 ` [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback Wesley Cheng
2021-01-04 15:45   ` Greg KH
2021-01-04 19:03     ` Wesley Cheng
  -- strict thread matches above, loose matches on Subject: below --
2020-11-14  8:12 [PATCH 0/3] Add vbus draw support to DWC3 Wesley Cheng
2020-11-14  8:12 ` [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect Wesley Cheng
2020-11-16 13:44   ` Peter Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.