[PATCH v1 0/2] Fix vbus draw of dwc3 gadget

[PATCH v1 0/2] Fix vbus draw of dwc3 gadget

[Prashanth K]

Currently dwc3 gadget processes the suspend interrupt event only
if the device is in configured state. But consider a case where
device is not configured and got suspend interrupt, in that case
our gadget would still use 100mA as composite_suspend didn't happen.
But battery charging specification (BC1.2) expects a downstream
device to draw less than 2.5mA when unconnected OR suspended.

And while resuming, the gadget can draw upto 100mA if its not
configured, but the current implementation of composite_resume
doesnt consider the case of unconfigured device. This series
addresses the above mentioned issues.

Prashanth K (2):
  usb: dwc3: gadget: Change condition for processing suspend event
  usb: gadget: composite: Draw 100mA current if not configured

 drivers/usb/dwc3/gadget.c      | 11 ++---------
 drivers/usb/gadget/composite.c |  2 ++
 2 files changed, 4 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] usb: dwc3: gadget: Change condition for processing suspend event

[Prashanth K]

Currently we process the suspend interrupt event only if the
device is in configured state. Consider a case where device
is not configured and got suspend interrupt, in that case our
gadget will still use 100mA as composite_suspend didn't happen.
But battery charging specification (BC1.2) expects a downstream
device to draw less than 2.5mA when unconnected OR suspended.

Fix this by removing the condition for processing suspend event,
and thus composite_resume would set vbus draw to 2.

Fixes: 72704f876f50 ("dwc3: gadget: Implement the suspend entry event handler")
Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3c63fa9..07989c6 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4239,15 +4239,8 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
 		break;
 	case DWC3_DEVICE_EVENT_SUSPEND:
 		/* It changed to be suspend event for version 2.30a and above */
-		if (!DWC3_VER_IS_PRIOR(DWC3, 230A)) {
-			/*
-			 * Ignore suspend event until the gadget enters into
-			 * USB_STATE_CONFIGURED state.
-			 */
-			if (dwc->gadget->state >= USB_STATE_CONFIGURED)
-				dwc3_gadget_suspend_interrupt(dwc,
-						event->event_info);
-		}
+		if (!DWC3_VER_IS_PRIOR(DWC3, 230A))
+			dwc3_gadget_suspend_interrupt(dwc, event->event_info);
 		break;
 	case DWC3_DEVICE_EVENT_SOF:
 	case DWC3_DEVICE_EVENT_ERRATIC_ERROR:
-- 
2.7.4

[PATCH v1 2/2] usb: gadget: composite: Draw 100mA current if not configured

[Prashanth K]

Currently we don't change the current value if device isn't in
configured state. But the battery charging specification says,
the device can draw upto 100mA of current if its in unconfigured
state. Hence add a Vbus_draw work in composite_resume to draw
100mA if the device isn't configured.

Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
---
 drivers/usb/gadget/composite.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index fa7dd6c..147d278 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2531,6 +2531,8 @@ void composite_resume(struct usb_gadget *gadget)
 			usb_gadget_clear_selfpowered(gadget);
 
 		usb_gadget_vbus_draw(gadget, maxpower);
+	} else {
+		usb_gadget_vbus_draw(gadget, 100);
 	}
 
 	cdev->suspended = 0;
-- 
2.7.4

Re: [PATCH v1 2/2] usb: gadget: composite: Draw 100mA current if not configured

[Jack Pham]

Hi Prashanth,

On Thu, Feb 23, 2023 at 10:48:30AM +0530, Prashanth K wrote:
> Currently we don't change the current value if device isn't in
> configured state. But the battery charging specification says,
> the device can draw upto 100mA of current if its in unconfigured

Here you say spec says "up to" (BTW you have a typo) 100mA...

> state. Hence add a Vbus_draw work in composite_resume to draw
> 100mA if the device isn't configured.

But here and in the patch you are calling the function to draw exactly
100mA.  Consider the possibility that a gadget could be configured to
draw less current than that or not anything at all, we should make sure
to honor that as an absolute maximum.

> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> ---
>  drivers/usb/gadget/composite.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index fa7dd6c..147d278 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2531,6 +2531,8 @@ void composite_resume(struct usb_gadget *gadget)
>  			usb_gadget_clear_selfpowered(gadget);
>  
>  		usb_gadget_vbus_draw(gadget, maxpower);
> +	} else {
> +		usb_gadget_vbus_draw(gadget, 100);

Similar to the configured case, maybe you can perform a min()
calculation against either or both the config->MaxPower or
CONFIG_USB_GADGET_VBUS_DRAW.

Thanks,
Jack

Re: [PATCH v1 2/2] usb: gadget: composite: Draw 100mA current if not configured

[Prashanth K]

On 23-02-23 01:03 pm, Jack Pham wrote:
> Hi Prashanth,
> 
> On Thu, Feb 23, 2023 at 10:48:30AM +0530, Prashanth K wrote:
>> Currently we don't change the current value if device isn't in
>> configured state. But the battery charging specification says,
>> the device can draw upto 100mA of current if its in unconfigured
> 
> Here you say spec says "up to" (BTW you have a typo) 100mA...
> 
Will fix it in v2
>> state. Hence add a Vbus_draw work in composite_resume to draw
>> 100mA if the device isn't configured.
> 
> But here and in the patch you are calling the function to draw exactly
> 100mA.  Consider the possibility that a gadget could be configured to
> draw less current than that or not anything at all, we should make sure
> to honor that as an absolute maximum.
That's right
> 
>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index fa7dd6c..147d278 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2531,6 +2531,8 @@ void composite_resume(struct usb_gadget *gadget)
>>   			usb_gadget_clear_selfpowered(gadget);
>>   
>>   		usb_gadget_vbus_draw(gadget, maxpower);
>> +	} else {
>> +		usb_gadget_vbus_draw(gadget, 100);
> 
> Similar to the configured case, maybe you can perform a min()
> calculation against either or both the config->MaxPower or
> CONFIG_USB_GADGET_VBUS_DRAW.
> 
Thanks for the suggestion, will update it in v2 patch
> Thanks,
> Jack