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

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

[Prashanth K]

Changes in v3
 - Fixed minor syntax error and warnings

Changes in v2
 - Added min() calculation against CONFIG_USB_GADGET_VBUS_DRAW in case
  of unconfigured state.

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
doesn't 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 |  4 ++++
 2 files changed, 6 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH v3 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 89dcfac..a83f34e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4241,15 +4241,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

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

[Thinh Nguyen]

On Thu, Feb 23, 2023, Prashanth K wrote:
> 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 89dcfac..a83f34e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4241,15 +4241,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
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

[PATCH v3 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,
device can draw up to 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 403563c..386140f 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2449,6 +2449,10 @@ void composite_resume(struct usb_gadget *gadget)
 			usb_gadget_clear_selfpowered(gadget);
 
 		usb_gadget_vbus_draw(gadget, maxpower);
+	} else {
+		maxpower = CONFIG_USB_GADGET_VBUS_DRAW;
+		maxpower = min(maxpower, 100U);
+		usb_gadget_vbus_draw(gadget, maxpower);
 	}
 
 	cdev->suspended = 0;
-- 
2.7.4

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

[Thinh Nguyen]

On Thu, Feb 23, 2023, Prashanth K wrote:
> Currently we don't change the current value if device isn't in
> configured state. But the battery charging specification says,

Can you provide the spec section also?

> device can draw up to 100mA of current if its in unconfigured

Is this related to being self-powered?

> 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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 403563c..386140f 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2449,6 +2449,10 @@ void composite_resume(struct usb_gadget *gadget)
>  			usb_gadget_clear_selfpowered(gadget);
>  
>  		usb_gadget_vbus_draw(gadget, maxpower);
> +	} else {
> +		maxpower = CONFIG_USB_GADGET_VBUS_DRAW;
> +		maxpower = min(maxpower, 100U);
> +		usb_gadget_vbus_draw(gadget, maxpower);
>  	}
>  
>  	cdev->suspended = 0;
> -- 
> 2.7.4
> 

Thanks,
Thinh

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

[Prashanth K]

On 24-02-23 01:40 am, Thinh Nguyen wrote:
> On Thu, Feb 23, 2023, Prashanth K wrote:
>> Currently we don't change the current value if device isn't in
>> configured state. But the battery charging specification says,
> 
> Can you provide the spec section also?
> 
1.2 Background
1.4.13 Standard Downstream Port

Did you mean to add these in the commit message?
>> device can draw up to 100mA of current if its in unconfigured
> 
> Is this related to being self-powered?
I think its applicable for bus-powered devices.

Thanks
Prashanth K

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

[Thinh Nguyen]

On Fri, Feb 24, 2023, Prashanth K wrote:
> 
> 
> On 24-02-23 01:40 am, Thinh Nguyen wrote:
> > On Thu, Feb 23, 2023, Prashanth K wrote:
> > > Currently we don't change the current value if device isn't in
> > > configured state. But the battery charging specification says,
> > 
> > Can you provide the spec section also?
> > 
> 1.2 Background
> 1.4.13 Standard Downstream Port
> 
> Did you mean to add these in the commit message?

Yes, it's better to have the reference in case we need to revisit this.

> > > device can draw up to 100mA of current if its in unconfigured
> > 
> > Is this related to being self-powered?

> I think its applicable for bus-powered devices.

No, I mean before configured state, is the device considered
self-powered? Since being self-powered means drawing 100mA or less, we
can use USB_SELF_POWER_VBUS_MAX_DRAW to provide more context. If it's
totally unrelated, then you can ignore this.

Thanks,
Thinh

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

[Prashanth K]

On 25-02-23 12:08 am, Thinh Nguyen wrote:
> On Fri, Feb 24, 2023, Prashanth K wrote:
>>
>>
>> On 24-02-23 01:40 am, Thinh Nguyen wrote:
>>> On Thu, Feb 23, 2023, Prashanth K wrote:
>>>> Currently we don't change the current value if device isn't in
>>>> configured state. But the battery charging specification says,
>>>
>>> Can you provide the spec section also?
>>>
>> 1.2 Background
>> 1.4.13 Standard Downstream Port
>>
>> Did you mean to add these in the commit message?
> 
> Yes, it's better to have the reference in case we need to revisit this.
I have added it in v4 patch, thanks for pointing it out.
> 
>>>> device can draw up to 100mA of current if its in unconfigured
>>>
>>> Is this related to being self-powered?
> 
>> I think its applicable for bus-powered devices.
> 
> No, I mean before configured state, is the device considered
> self-powered? Since being self-powered means drawing 100mA or less, we
> can use USB_SELF_POWER_VBUS_MAX_DRAW to provide more context. If it's
> totally unrelated, then you can ignore this.
> 
> Thanks,
> Thinh
As per my understanding, those are 2 different things. A self-powered 
device isn't allowed to draw more than 100mA. And an unconfigured device
isn't allowed to draw more than 100mA (in HS). One thing that I recently 
found out is that, as per usb3.0 spec, SS device can only draw up to 
150mA if its unconfigured state. So i have to check the speed and set 
the current values accordingly.

Thanks,
Prashanth K

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

[Thinh Nguyen]

On Sat, Feb 25, 2023, Prashanth K wrote:
> 
> 
> On 25-02-23 12:08 am, Thinh Nguyen wrote:
> > On Fri, Feb 24, 2023, Prashanth K wrote:
> > > 
> > > 
> > > On 24-02-23 01:40 am, Thinh Nguyen wrote:
> > > > On Thu, Feb 23, 2023, Prashanth K wrote:
> > > > > Currently we don't change the current value if device isn't in
> > > > > configured state. But the battery charging specification says,
> > > > 
> > > > Can you provide the spec section also?
> > > > 
> > > 1.2 Background
> > > 1.4.13 Standard Downstream Port
> > > 
> > > Did you mean to add these in the commit message?
> > 
> > Yes, it's better to have the reference in case we need to revisit this.
> I have added it in v4 patch, thanks for pointing it out.
> > 
> > > > > device can draw up to 100mA of current if its in unconfigured
> > > > 
> > > > Is this related to being self-powered?
> > 
> > > I think its applicable for bus-powered devices.
> > 
> > No, I mean before configured state, is the device considered
> > self-powered? Since being self-powered means drawing 100mA or less, we
> > can use USB_SELF_POWER_VBUS_MAX_DRAW to provide more context. If it's
> > totally unrelated, then you can ignore this.
> > 
> > Thanks,
> > Thinh
> As per my understanding, those are 2 different things. A self-powered device
> isn't allowed to draw more than 100mA. And an unconfigured device
> isn't allowed to draw more than 100mA (in HS). One thing that I recently
> found out is that, as per usb3.0 spec, SS device can only draw up to 150mA
> if its unconfigured state. So i have to check the speed and set the current
> values accordingly.

I see. Thanks for the info.

Thanks,
Thinh