Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
gre...@linuxfoundation.org wrote:
> On Fri, Jan 08, 2021 at 02:19:30AM +, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Wesley Cheng 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
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Jack Pham wrote:
> Hi Thinh,
>
> On Fri, Jan 08, 2021 at 02:19:30AM +, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Wesley Cheng 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
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
On Fri, Jan 08, 2021 at 02:19:30AM +, Thinh Nguyen wrote:
> Hi Wesley,
>
> Felipe Balbi wrote:
> > Hi,
> >
> > Wesley Cheng 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
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Hi Thinh,
On Fri, Jan 08, 2021 at 02:19:30AM +, Thinh Nguyen wrote:
> Hi Wesley,
>
> Felipe Balbi wrote:
> > Hi,
> >
> > Wesley Cheng 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
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Hi Wesley,
Felipe Balbi wrote:
> Hi,
>
> Wesley Cheng 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
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Hi,
Wesley Cheng 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
[PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
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
Signed-off-by: Wesley Cheng
---
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(>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
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
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
Reviewed-by: Peter Chen
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(>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
[PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
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
---
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(>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
