[PATCH v2 0/3] usb: gadget: Add support for disabling U1 and U2 entries

[PATCH v2 0/3] usb: gadget: Add support for disabling U1 and U2 entries

[Anurag Kumar Vulisha]

Gadget applications may have a requirement to disable the U1 and U2
entry based on the usecase. Below are few usecases where the disabling
U1/U2 entries may be possible.

Usecase 1:
When combining dwc3 with an redriver for a USB Type-C device solution, it
sometimes have problems with leaving U1/U2 for certain hosts, resulting in
link training errors and reconnects. For this U1/U2 state entries may be
avoided.

Usecase 2:
When performing performance benchmarking on mass storage gadget the
U1 and U2 entries can be disabled.

Usecase 3:
When periodic transfers like ISOC transfers are used with bInterval
of 1 which doesn't require the link to enter into U1 or U2 state entry
(since ping is issued from host for every uframe interval). In this
case the U1 and U2 entry can be disabled.

Disablement of U1/U2 can be done by setting U1DevExitLat and U2DevExitLat
values to 0 in the BOS descriptor. Host on seeing 0 value for U1DevExitLat
and U2DevExitLat, it doesn't send SET_SEL requests to the gadget. There
may be some hosts which may send SET_SEL requests even after seeing 0 in
the UxDevExitLat of BOS descriptor. To aviod U1/U2 entries for these type
of hosts, dwc3 controller can be programmed to reject those U1/U2 requests
by not enabling ACCEPTUxENA bits in DCTL register.

This patch series updates the same.

Changes in v2:
	1. As suggested by Thinh Nguyen changed the "snps,dis_u1_entry_quirk"
	   to "snps,dis-u1-entry-quirk"
	2. Merged the changes done by Claus H. Stovgaard in ep0.c for rejecting
	   U1/U2 requests into this patch. Changes done by Claus can be found
	   here https://marc.info/?l=linux-kernel&m=155722068820568&w=2

Anurag Kumar Vulisha (3):
  doc: dt: bindings: usb: dwc3: Update entries for disabling U1 and U2
  usb: gadget: send usb_gadget as an argument in get_config_params
  usb: dwc3: gadget: Add support for disabling U1 and U2 entries

 Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
 drivers/usb/dwc3/core.c                        |  4 ++++
 drivers/usb/dwc3/core.h                        |  4 ++++
 drivers/usb/dwc3/ep0.c                         |  9 ++++++++-
 drivers/usb/dwc3/gadget.c                      | 19 +++++++++++++++++++
 drivers/usb/dwc3/gadget.h                      |  6 ++++++
 drivers/usb/gadget/composite.c                 |  2 +-
 include/linux/usb/gadget.h                     |  3 ++-
 8 files changed, 46 insertions(+), 3 deletions(-)

-- 
2.1.1

[PATCH v2 1/3] doc: dt: bindings: usb: dwc3: Update entries for disabling U1 and U2

[Anurag Kumar Vulisha]

This patch updates the documentation with the information related
to the quirks that needs to be added for disabling the link entering
into the U1 and U2 states

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v2
	1. As suggested by Thinh Nguyen changed the "snps,dis_u1_entry_quirk"
	   to "snps,dis-u1-entry-quirk"
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 8e5265e..66780a4 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -64,6 +64,8 @@ Optional properties:
  - snps,dis_u2_susphy_quirk: when set core will disable USB2 suspend phy.
  - snps,dis_enblslpm_quirk: when set clears the enblslpm in GUSB2PHYCFG,
 			disabling the suspend signal to the PHY.
+ - snps,dis-u1-entry-quirk: set if link entering into U1 needs to be disabled.
+ - snps,dis-u2-entry-quirk: set if link entering into U2 needs to be disabled.
  - snps,dis_rxdet_inp3_quirk: when set core will disable receiver detection
 			in PHY P3 power state.
  - snps,dis-u2-freeclk-exists-quirk: when set, clear the u2_freeclk_exists
-- 
2.1.1

[PATCH v2 2/3] usb: gadget: send usb_gadget as an argument in get_config_params

[Anurag Kumar Vulisha]

Passing struct usb_gadget * as an extra argument in get_config_params
makes gadget drivers to easily update the U1DevExitLat & U2DevExitLat
values based on the values passed from the device tree. This patch
does the same

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 Changes in v2:
	- None
---
 drivers/usb/gadget/composite.c | 2 +-
 include/linux/usb/gadget.h     | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index b8a1584..9118b42 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -653,7 +653,7 @@ static int bos_desc(struct usb_composite_dev *cdev)
 
 		/* Get Controller configuration */
 		if (cdev->gadget->ops->get_config_params) {
-			cdev->gadget->ops->get_config_params(
+			cdev->gadget->ops->get_config_params(cdev->gadget,
 				&dcd_config_params);
 		} else {
 			dcd_config_params.bU1devExitLat =
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 7595056..fb19141 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -310,7 +310,8 @@ struct usb_gadget_ops {
 	int	(*pullup) (struct usb_gadget *, int is_on);
 	int	(*ioctl)(struct usb_gadget *,
 				unsigned code, unsigned long param);
-	void	(*get_config_params)(struct usb_dcd_config_params *);
+	void	(*get_config_params)(struct usb_gadget *,
+				     struct usb_dcd_config_params *);
 	int	(*udc_start)(struct usb_gadget *,
 			struct usb_gadget_driver *);
 	int	(*udc_stop)(struct usb_gadget *);
-- 
2.1.1

[PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Anurag Kumar Vulisha]

Gadget applications may have a requirement to disable the U1 and U2
entry based on the usecase. Below are few usecases where the disabling
U1/U2 entries may be possible.

Usecase 1:
When combining dwc3 with an redriver for a USB Type-C device solution, it
sometimes have problems with leaving U1/U2 for certain hosts, resulting in
link training errors and reconnects. For this U1/U2 state entries may be
avoided.

Usecase 2:
When performing performance benchmarking on mass storage gadget the
U1 and U2 entries can be disabled.

Usecase 3:
When periodic transfers like ISOC transfers are used with bInterval
of 1 which doesn't require the link to enter into U1 or U2 state entry
(since ping is issued from host for every uframe interval). In this
case the U1 and U2 entry can be disabled.

Disablement of U1/U2 can be done by setting U1DevExitLat and U2DevExitLat
values to 0 in the BOS descriptor. Host on seeing 0 value for U1DevExitLat
and U2DevExitLat, it doesn't send SET_SEL requests to the gadget. There
may be some hosts which may send SET_SEL requests even after seeing 0 in
the UxDevExitLat of BOS descriptor. To aviod U1/U2 entries for these type
of hosts, dwc3 controller can be programmed to reject those U1/U2 requests
by not enabling ACCEPTUxENA bits in DCTL register.

This patch updates the same.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Signed-off-by: Claus H. Stovgaard <cst@phaseone.com>
---
 Changes in v2
	1. As suggested by Thinh Nguyen changed the "snps,dis_u1_entry_quirk"
	   to "snps,dis-u1-entry-quirk"
	2. Merged the changes done by Claus H. Stovgaard in ep0.c for rejecting
	   U1/U2 requests into this patch. Changes done by Claus can be found
	   here https://marc.info/?l=linux-kernel&m=155722068820568&w=2
	3. Changed the commit message.
---
 drivers/usb/dwc3/core.c   |  4 ++++
 drivers/usb/dwc3/core.h   |  4 ++++
 drivers/usb/dwc3/ep0.c    |  9 ++++++++-
 drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
 drivers/usb/dwc3/gadget.h |  6 ++++++
 5 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a1b126f..180239b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1285,6 +1285,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,dis_u2_susphy_quirk");
 	dwc->dis_enblslpm_quirk = device_property_read_bool(dev,
 				"snps,dis_enblslpm_quirk");
+	dwc->dis_u1_entry_quirk = device_property_read_bool(dev,
+				"snps,dis-u1-entry-quirk");
+	dwc->dis_u2_entry_quirk = device_property_read_bool(dev,
+				"snps,dis-u2-entry-quirk");
 	dwc->dis_rxdet_inp3_quirk = device_property_read_bool(dev,
 				"snps,dis_rxdet_inp3_quirk");
 	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1528d39..fa398e2 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1015,6 +1015,8 @@ struct dwc3_scratchpad_array {
  * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
  * @dis_enblslpm_quirk: set if we clear enblslpm in GUSB2PHYCFG,
  *                      disabling the suspend signal to the PHY.
+ * @dis_u1_entry_quirk: set if link entering into U1 state needs to be disabled.
+ * @dis_u2_entry_quirk: set if link entering into U2 state needs to be disabled.
  * @dis_rxdet_inp3_quirk: set if we disable Rx.Detect in P3
  * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
  *			in GUSB2PHYCFG, specify that USB2 PHY doesn't
@@ -1206,6 +1208,8 @@ struct dwc3 {
 	unsigned		dis_u3_susphy_quirk:1;
 	unsigned		dis_u2_susphy_quirk:1;
 	unsigned		dis_enblslpm_quirk:1;
+	unsigned		dis_u1_entry_quirk:1;
+	unsigned		dis_u2_entry_quirk:1;
 	unsigned		dis_rxdet_inp3_quirk:1;
 	unsigned		dis_u2_freeclk_exists_quirk:1;
 	unsigned		dis_del_phy_power_chg_quirk:1;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 8efde17..8e94efc 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -379,6 +379,8 @@ static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state,
 	if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
 			(dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
 		return -EINVAL;
+	if (dwc->dis_u1_entry_quirk)
+		return -EINVAL;
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	if (set)
@@ -401,6 +403,8 @@ static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state,
 	if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
 			(dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
 		return -EINVAL;
+	if (dwc->dis_u2_entry_quirk)
+		return -EINVAL;
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	if (set)
@@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
 			 * nothing is pending from application.
 			 */
 			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-			reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA);
+			if (!dwc->dis_u1_entry_quirk)
+				reg |= DWC3_DCTL_ACCEPTU1ENA;
+			if (!dwc->dis_u2_entry_quirk)
+				reg |= DWC3_DCTL_ACCEPTU2ENA;
 			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
 		}
 		break;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e293400..f2d3112 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 	return 0;
 }
 
+static void dwc3_gadget_config_params(struct usb_gadget *g,
+				      struct usb_dcd_config_params *params)
+{
+	struct dwc3		*dwc = gadget_to_dwc(g);
+
+	/* U1 Device exit Latency */
+	if (dwc->dis_u1_entry_quirk)
+		params->bU1devExitLat = 0;
+	else
+		params->bU1devExitLat = DWC3_DEFAULT_U1_DEV_EXIT_LAT;
+
+	/* U2 Device exit Latency */
+	if (dwc->dis_u2_entry_quirk)
+		params->bU2DevExitLat = 0;
+	else
+		params->bU2DevExitLat = DWC3_DEFAULT_U2_DEV_EXIT_LAT;
+}
+
 static void dwc3_gadget_set_speed(struct usb_gadget *g,
 				  enum usb_device_speed speed)
 {
@@ -2142,6 +2160,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.udc_start		= dwc3_gadget_start,
 	.udc_stop		= dwc3_gadget_stop,
 	.udc_set_speed		= dwc3_gadget_set_speed,
+	.get_config_params	= dwc3_gadget_config_params,
 };
 
 /* -------------------------------------------------------------------------- */
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 3ed738e..5faf4d1 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -48,6 +48,12 @@ struct dwc3;
 /* DEPXFERCFG parameter 0 */
 #define DWC3_DEPXFERCFG_NUM_XFER_RES(n)	((n) & 0xffff)
 
+/* U1 Device exit Latency */
+#define DWC3_DEFAULT_U1_DEV_EXIT_LAT	0x0A	/* Less then 10 microsec */
+
+/* U2 Device exit Latency */
+#define DWC3_DEFAULT_U2_DEV_EXIT_LAT	0x1FF	/* Less then 511 microsec */
+
 /* -------------------------------------------------------------------------- */
 
 #define to_dwc3_request(r)	(container_of(r, struct dwc3_request, request))
-- 
2.1.1

Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Thinh Nguyen]

Hi Anurag,

Anurag Kumar Vulisha wrote:
> Gadget applications may have a requirement to disable the U1 and U2
> entry based on the usecase. Below are few usecases where the disabling
> U1/U2 entries may be possible.
>
> Usecase 1:
> When combining dwc3 with an redriver for a USB Type-C device solution, it
> sometimes have problems with leaving U1/U2 for certain hosts, resulting in
> link training errors and reconnects. For this U1/U2 state entries may be
> avoided.
>
> Usecase 2:
> When performing performance benchmarking on mass storage gadget the
> U1 and U2 entries can be disabled.
>
> Usecase 3:
> When periodic transfers like ISOC transfers are used with bInterval
> of 1 which doesn't require the link to enter into U1 or U2 state entry
> (since ping is issued from host for every uframe interval). In this
> case the U1 and U2 entry can be disabled.
>
> Disablement of U1/U2 can be done by setting U1DevExitLat and U2DevExitLat
> values to 0 in the BOS descriptor. Host on seeing 0 value for U1DevExitLat
> and U2DevExitLat, it doesn't send SET_SEL requests to the gadget. There
> may be some hosts which may send SET_SEL requests even after seeing 0 in
> the UxDevExitLat of BOS descriptor. To aviod U1/U2 entries for these type
> of hosts, dwc3 controller can be programmed to reject those U1/U2 requests
> by not enabling ACCEPTUxENA bits in DCTL register.
>
> This patch updates the same.
>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> Signed-off-by: Claus H. Stovgaard <cst@phaseone.com>
> ---
>  Changes in v2
> 	1. As suggested by Thinh Nguyen changed the "snps,dis_u1_entry_quirk"
> 	   to "snps,dis-u1-entry-quirk"
> 	2. Merged the changes done by Claus H. Stovgaard in ep0.c for rejecting
> 	   U1/U2 requests into this patch. Changes done by Claus can be found
> 	   here https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dkernel-26m-3D155722068820568-26w-3D2&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w&m=dJMdvubLsepuGRDdkLZNJ00bhu52jPV7TZaFkDGD0Vs&s=wT7eyWpRKPAqXmLfdfiArbnZ7vE9Vi8DOfRdULmeIqY&e=
> 	3. Changed the commit message.
> ---
>  drivers/usb/dwc3/core.c   |  4 ++++
>  drivers/usb/dwc3/core.h   |  4 ++++
>  drivers/usb/dwc3/ep0.c    |  9 ++++++++-
>  drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
>  drivers/usb/dwc3/gadget.h |  6 ++++++
>  5 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a1b126f..180239b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1285,6 +1285,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  				"snps,dis_u2_susphy_quirk");
>  	dwc->dis_enblslpm_quirk = device_property_read_bool(dev,
>  				"snps,dis_enblslpm_quirk");
> +	dwc->dis_u1_entry_quirk = device_property_read_bool(dev,
> +				"snps,dis-u1-entry-quirk");
> +	dwc->dis_u2_entry_quirk = device_property_read_bool(dev,
> +				"snps,dis-u2-entry-quirk");
>  	dwc->dis_rxdet_inp3_quirk = device_property_read_bool(dev,
>  				"snps,dis_rxdet_inp3_quirk");
>  	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 1528d39..fa398e2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1015,6 +1015,8 @@ struct dwc3_scratchpad_array {
>   * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
>   * @dis_enblslpm_quirk: set if we clear enblslpm in GUSB2PHYCFG,
>   *                      disabling the suspend signal to the PHY.
> + * @dis_u1_entry_quirk: set if link entering into U1 state needs to be disabled.
> + * @dis_u2_entry_quirk: set if link entering into U2 state needs to be disabled.
>   * @dis_rxdet_inp3_quirk: set if we disable Rx.Detect in P3
>   * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
>   *			in GUSB2PHYCFG, specify that USB2 PHY doesn't
> @@ -1206,6 +1208,8 @@ struct dwc3 {
>  	unsigned		dis_u3_susphy_quirk:1;
>  	unsigned		dis_u2_susphy_quirk:1;
>  	unsigned		dis_enblslpm_quirk:1;
> +	unsigned		dis_u1_entry_quirk:1;
> +	unsigned		dis_u2_entry_quirk:1;
>  	unsigned		dis_rxdet_inp3_quirk:1;
>  	unsigned		dis_u2_freeclk_exists_quirk:1;
>  	unsigned		dis_del_phy_power_chg_quirk:1;
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 8efde17..8e94efc 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -379,6 +379,8 @@ static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state,
>  	if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
>  			(dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
>  		return -EINVAL;
> +	if (dwc->dis_u1_entry_quirk)

We only need to reject on SET_FEATURE(enable U1/U2) and not
SET_FEATURE(disable U1/U2).

Let's change the if condition to if (set && dis_u1_entry_quirk).

> +		return -EINVAL;
>  
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	if (set)
> @@ -401,6 +403,8 @@ static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state,
>  	if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
>  			(dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
>  		return -EINVAL;
> +	if (dwc->dis_u2_entry_quirk)

Same comment as previous.

> +		return -EINVAL;
>  
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	if (set)
> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
>  			 * nothing is pending from application.
>  			 */
>  			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> -			reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA);
> +			if (!dwc->dis_u1_entry_quirk)
> +				reg |= DWC3_DCTL_ACCEPTU1ENA;
> +			if (!dwc->dis_u2_entry_quirk)
> +				reg |= DWC3_DCTL_ACCEPTU2ENA;
>  			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>  		}
>  		break;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e293400..f2d3112 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  	return 0;
>  }
>  
> +static void dwc3_gadget_config_params(struct usb_gadget *g,
> +				      struct usb_dcd_config_params *params)
> +{
> +	struct dwc3		*dwc = gadget_to_dwc(g);
> +
> +	/* U1 Device exit Latency */
> +	if (dwc->dis_u1_entry_quirk)
> +		params->bU1devExitLat = 0;

It doesn't make sense to have exit latency of 0. Rejecting
SET_FEATURE(enable U1/U2) should already let the host know that the
device doesn't support U1/U2.

> +	else
> +		params->bU1devExitLat = DWC3_DEFAULT_U1_DEV_EXIT_LAT;
> +
> +	/* U2 Device exit Latency */
> +	if (dwc->dis_u2_entry_quirk)
> +		params->bU2DevExitLat = 0;
> +	else
> +		params->bU2DevExitLat = DWC3_DEFAULT_U2_DEV_EXIT_LAT;

This is a le16 value. Assign it with cpu_to_le16().

> +}
> +
>  static void dwc3_gadget_set_speed(struct usb_gadget *g,
>  				  enum usb_device_speed speed)
>  {
> @@ -2142,6 +2160,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.udc_start		= dwc3_gadget_start,
>  	.udc_stop		= dwc3_gadget_stop,
>  	.udc_set_speed		= dwc3_gadget_set_speed,
> +	.get_config_params	= dwc3_gadget_config_params,
>  };
>  
>  /* -------------------------------------------------------------------------- */
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 3ed738e..5faf4d1 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -48,6 +48,12 @@ struct dwc3;
>  /* DEPXFERCFG parameter 0 */
>  #define DWC3_DEPXFERCFG_NUM_XFER_RES(n)	((n) & 0xffff)
>  
> +/* U1 Device exit Latency */
> +#define DWC3_DEFAULT_U1_DEV_EXIT_LAT	0x0A	/* Less then 10 microsec */
> +
> +/* U2 Device exit Latency */
> +#define DWC3_DEFAULT_U2_DEV_EXIT_LAT	0x1FF	/* Less then 511 microsec */
> +
>  /* -------------------------------------------------------------------------- */
>  
>  #define to_dwc3_request(r)	(container_of(r, struct dwc3_request, request))

BR,
Thinh

RE: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Anurag Kumar Vulisha]

Hi Thinh,

>-----Original Message-----
>From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>Sent: Thursday, May 09, 2019 1:03 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
><mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Thinh Nguyen
><Thinh.Nguyen@synopsys.com>; Claus H. Stovgaard <cst@phaseone.com>
>Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; v.anuragkumar@gmail.com
>Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>entries
>
>Hi Anurag,
>
>Anurag Kumar Vulisha wrote:
>> Gadget applications may have a requirement to disable the U1 and U2
>> entry based on the usecase. Below are few usecases where the disabling
>> U1/U2 entries may be possible.
>>
>> Usecase 1:
>> When combining dwc3 with an redriver for a USB Type-C device solution, it
>> sometimes have problems with leaving U1/U2 for certain hosts, resulting in
>> link training errors and reconnects. For this U1/U2 state entries may be
>> avoided.
>>
>> Usecase 2:
>> When performing performance benchmarking on mass storage gadget the
>> U1 and U2 entries can be disabled.
>>
>> Usecase 3:
>> When periodic transfers like ISOC transfers are used with bInterval
>> of 1 which doesn't require the link to enter into U1 or U2 state entry
>> (since ping is issued from host for every uframe interval). In this
>> case the U1 and U2 entry can be disabled.
>>
>> Disablement of U1/U2 can be done by setting U1DevExitLat and U2DevExitLat
>> values to 0 in the BOS descriptor. Host on seeing 0 value for U1DevExitLat
>> and U2DevExitLat, it doesn't send SET_SEL requests to the gadget. There
>> may be some hosts which may send SET_SEL requests even after seeing 0 in
>> the UxDevExitLat of BOS descriptor. To aviod U1/U2 entries for these type
>> of hosts, dwc3 controller can be programmed to reject those U1/U2 requests
>> by not enabling ACCEPTUxENA bits in DCTL register.
>>
>> This patch updates the same.
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>> Signed-off-by: Claus H. Stovgaard <cst@phaseone.com>
>> ---
>>  Changes in v2
>> 	1. As suggested by Thinh Nguyen changed the "snps,dis_u1_entry_quirk"
>> 	   to "snps,dis-u1-entry-quirk"
>> 	2. Merged the changes done by Claus H. Stovgaard in ep0.c for rejecting
>> 	   U1/U2 requests into this patch. Changes done by Claus can be found
>> 	   here https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-
>3Fl-3Dlinux-2Dkernel-26m-3D155722068820568-26w-
>3D2&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0Vs
>G2d8FCmzkTY-
>w&m=dJMdvubLsepuGRDdkLZNJ00bhu52jPV7TZaFkDGD0Vs&s=wT7eyWpRKPAqXmLf
>dfiArbnZ7vE9Vi8DOfRdULmeIqY&e=
>> 	3. Changed the commit message.
>> ---
>>  drivers/usb/dwc3/core.c   |  4 ++++
>>  drivers/usb/dwc3/core.h   |  4 ++++
>>  drivers/usb/dwc3/ep0.c    |  9 ++++++++-
>>  drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
>>  drivers/usb/dwc3/gadget.h |  6 ++++++
>>  5 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index a1b126f..180239b 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1285,6 +1285,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  				"snps,dis_u2_susphy_quirk");
>>  	dwc->dis_enblslpm_quirk = device_property_read_bool(dev,
>>  				"snps,dis_enblslpm_quirk");
>> +	dwc->dis_u1_entry_quirk = device_property_read_bool(dev,
>> +				"snps,dis-u1-entry-quirk");
>> +	dwc->dis_u2_entry_quirk = device_property_read_bool(dev,
>> +				"snps,dis-u2-entry-quirk");
>>  	dwc->dis_rxdet_inp3_quirk = device_property_read_bool(dev,
>>  				"snps,dis_rxdet_inp3_quirk");
>>  	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 1528d39..fa398e2 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1015,6 +1015,8 @@ struct dwc3_scratchpad_array {
>>   * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
>>   * @dis_enblslpm_quirk: set if we clear enblslpm in GUSB2PHYCFG,
>>   *                      disabling the suspend signal to the PHY.
>> + * @dis_u1_entry_quirk: set if link entering into U1 state needs to be disabled.
>> + * @dis_u2_entry_quirk: set if link entering into U2 state needs to be disabled.
>>   * @dis_rxdet_inp3_quirk: set if we disable Rx.Detect in P3
>>   * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
>>   *			in GUSB2PHYCFG, specify that USB2 PHY doesn't
>> @@ -1206,6 +1208,8 @@ struct dwc3 {
>>  	unsigned		dis_u3_susphy_quirk:1;
>>  	unsigned		dis_u2_susphy_quirk:1;
>>  	unsigned		dis_enblslpm_quirk:1;
>> +	unsigned		dis_u1_entry_quirk:1;
>> +	unsigned		dis_u2_entry_quirk:1;
>>  	unsigned		dis_rxdet_inp3_quirk:1;
>>  	unsigned		dis_u2_freeclk_exists_quirk:1;
>>  	unsigned		dis_del_phy_power_chg_quirk:1;
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 8efde17..8e94efc 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -379,6 +379,8 @@ static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum
>usb_device_state state,
>>  	if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
>>  			(dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
>>  		return -EINVAL;
>> +	if (dwc->dis_u1_entry_quirk)
>
>We only need to reject on SET_FEATURE(enable U1/U2) and not
>SET_FEATURE(disable U1/U2).
>
>Let's change the if condition to if (set && dis_u1_entry_quirk).
>

Thanks for reviewing the patch.
I agree. Will correct it in the next series.

>> +		return -EINVAL;
>>
>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>  	if (set)
>> @@ -401,6 +403,8 @@ static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum
>usb_device_state state,
>>  	if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
>>  			(dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
>>  		return -EINVAL;
>> +	if (dwc->dis_u2_entry_quirk)
>
>Same comment as previous.
>

Will fix it in the next series

>> +		return -EINVAL;
>>
>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>  	if (set)
>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
>usb_ctrlrequest *ctrl)
>>  			 * nothing is pending from application.
>>  			 */
>>  			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> -			reg |= (DWC3_DCTL_ACCEPTU1ENA |
>DWC3_DCTL_ACCEPTU2ENA);
>> +			if (!dwc->dis_u1_entry_quirk)
>> +				reg |= DWC3_DCTL_ACCEPTU1ENA;
>> +			if (!dwc->dis_u2_entry_quirk)
>> +				reg |= DWC3_DCTL_ACCEPTU2ENA;
>>  			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>>  		}
>>  		break;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index e293400..f2d3112 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>  	return 0;
>>  }
>>
>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>> +				      struct usb_dcd_config_params *params)
>> +{
>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>> +
>> +	/* U1 Device exit Latency */
>> +	if (dwc->dis_u1_entry_quirk)
>> +		params->bU1devExitLat = 0;
>
>It doesn't make sense to have exit latency of 0. Rejecting
>SET_FEATURE(enable U1/U2) should already let the host know that the
>device doesn't support U1/U2.
>
I am okay to remove this, but I feel that it is better to report zero value instead
of a non-zero value in exit latency of BOS when U1 or U2 entries are not supported. 
Advantage of reporting 0 is that some hosts doesn't even send SET_FEATURE(U1/U2)
requests on seeing zero value in BOS descriptor. Also there can be cases where U1 is
disabled and U2 entry is allowed or vice versa, for these kind of cases the driver can
set zero exit latency value for U1 and non-zero exit latency value for U2 . Based on this
I think it would be better to report 0 when U1/U2 states are not enabled. Please provide
your opinion on this.

>> +	else
>> +		params->bU1devExitLat = DWC3_DEFAULT_U1_DEV_EXIT_LAT;
>> +
>> +	/* U2 Device exit Latency */
>> +	if (dwc->dis_u2_entry_quirk)
>> +		params->bU2DevExitLat = 0;
>> +	else
>> +		params->bU2DevExitLat = DWC3_DEFAULT_U2_DEV_EXIT_LAT;
>
>This is a le16 value. Assign it with cpu_to_le16().
>
Sure, will correct this in next series

Thanks,
Anurag Kumar Vulisha

>> +}
>> +
>>  static void dwc3_gadget_set_speed(struct usb_gadget *g,
>>  				  enum usb_device_speed speed)
>>  {
>> @@ -2142,6 +2160,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
>>  	.udc_start		= dwc3_gadget_start,
>>  	.udc_stop		= dwc3_gadget_stop,
>>  	.udc_set_speed		= dwc3_gadget_set_speed,
>> +	.get_config_params	= dwc3_gadget_config_params,
>>  };
>>
>>  /* -------------------------------------------------------------------------- */
>> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
>> index 3ed738e..5faf4d1 100644
>> --- a/drivers/usb/dwc3/gadget.h
>> +++ b/drivers/usb/dwc3/gadget.h
>> @@ -48,6 +48,12 @@ struct dwc3;
>>  /* DEPXFERCFG parameter 0 */
>>  #define DWC3_DEPXFERCFG_NUM_XFER_RES(n)	((n) & 0xffff)
>>
>> +/* U1 Device exit Latency */
>> +#define DWC3_DEFAULT_U1_DEV_EXIT_LAT	0x0A	/* Less then 10 microsec */
>> +
>> +/* U2 Device exit Latency */
>> +#define DWC3_DEFAULT_U2_DEV_EXIT_LAT	0x1FF	/* Less then 511 microsec
>*/
>> +
>>  /* -------------------------------------------------------------------------- */
>>
>>  #define to_dwc3_request(r)	(container_of(r, struct dwc3_request, request))
>
>BR,
>Thinh

Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Thinh Nguyen]

Hi Anurag,

Anurag Kumar Vulisha wrote:
>>> +		return -EINVAL;
>>>
>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>>  	if (set)
>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
>> usb_ctrlrequest *ctrl)
>>>  			 * nothing is pending from application.
>>>  			 */
>>>  			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>> -			reg |= (DWC3_DCTL_ACCEPTU1ENA |
>> DWC3_DCTL_ACCEPTU2ENA);
>>> +			if (!dwc->dis_u1_entry_quirk)
>>> +				reg |= DWC3_DCTL_ACCEPTU1ENA;
>>> +			if (!dwc->dis_u2_entry_quirk)
>>> +				reg |= DWC3_DCTL_ACCEPTU2ENA;
>>>  			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>>>  		}
>>>  		break;
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index e293400..f2d3112 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>  	return 0;
>>>  }
>>>
>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>> +				      struct usb_dcd_config_params *params)
>>> +{
>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>> +
>>> +	/* U1 Device exit Latency */
>>> +	if (dwc->dis_u1_entry_quirk)
>>> +		params->bU1devExitLat = 0;
>> It doesn't make sense to have exit latency of 0. Rejecting
>> SET_FEATURE(enable U1/U2) should already let the host know that the
>> device doesn't support U1/U2.
>>
> I am okay to remove this, but I feel that it is better to report zero value instead
> of a non-zero value in exit latency of BOS when U1 or U2 entries are not supported. 
> Advantage of reporting 0 is that some hosts doesn't even send SET_FEATURE(U1/U2)
> requests on seeing zero value in BOS descriptor. Also there can be cases where U1 is
> disabled and U2 entry is allowed or vice versa, for these kind of cases the driver can
> set zero exit latency value for U1 and non-zero exit latency value for U2 . Based on this
> I think it would be better to report 0 when U1/U2 states are not enabled. Please provide
> your opinion on this.

Hm... I assume you're testing against linux usb stack and xhci host. If
that's the case, it looks like host will still request the device to
enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
needs to be fixed. I think what you have is fine to workaround this issue.

Thanks,
Thinh

RE: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Anurag Kumar Vulisha]

Hi Thinh,

>-----Original Message-----
>From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>Sent: Friday, May 10, 2019 5:30 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
><Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
><mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
><cst@phaseone.com>
>Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; v.anuragkumar@gmail.com
>Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>entries
>
>Hi Anurag,
>
>Anurag Kumar Vulisha wrote:
>>>> +		return -EINVAL;
>>>>
>>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>>>  	if (set)
>>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc,
>struct
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index e293400..f2d3112 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>>  	return 0;
>>>>  }
>>>>
>>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>>> +				      struct usb_dcd_config_params *params)
>>>> +{
>>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>>> +
>>>> +	/* U1 Device exit Latency */
>>>> +	if (dwc->dis_u1_entry_quirk)
>>>> +		params->bU1devExitLat = 0;
>>> It doesn't make sense to have exit latency of 0. Rejecting
>>> SET_FEATURE(enable U1/U2) should already let the host know that the
>>> device doesn't support U1/U2.
>>>
>> I am okay to remove this, but I feel that it is better to report zero value instead
>> of a non-zero value in exit latency of BOS when U1 or U2 entries are not supported.
>> Advantage of reporting 0 is that some hosts doesn't even send
>SET_FEATURE(U1/U2)
>> requests on seeing zero value in BOS descriptor. Also there can be cases where U1 is
>> disabled and U2 entry is allowed or vice versa, for these kind of cases the driver can
>> set zero exit latency value for U1 and non-zero exit latency value for U2 . Based on
>this
>> I think it would be better to report 0 when U1/U2 states are not enabled. Please
>provide
>> your opinion on this.
>
>Hm... I assume you're testing against linux usb stack and xhci host. If
>that's the case, it looks like host will still request the device to
>enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
>needs to be fixed. I think what you have is fine to workaround this issue.

Thanks . Will send the next series with the other fixes that you have suggested

Best Regards,
Anurag Kumar Vulisha

Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Thinh Nguyen]

Hi,

Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
>> -----Original Message-----
>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>> Sent: Friday, May 10, 2019 5:30 AM
>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
>> <mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
>> <cst@phaseone.com>
>> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; v.anuragkumar@gmail.com
>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>> entries
>>
>> Hi Anurag,
>>
>> Anurag Kumar Vulisha wrote:
>>>>> +		return -EINVAL;
>>>>>
>>>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>>>>  	if (set)
>>>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc,
>> struct
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index e293400..f2d3112 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>>>  	return 0;
>>>>>  }
>>>>>
>>>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>>>> +				      struct usb_dcd_config_params *params)
>>>>> +{
>>>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>>>> +
>>>>> +	/* U1 Device exit Latency */
>>>>> +	if (dwc->dis_u1_entry_quirk)
>>>>> +		params->bU1devExitLat = 0;
>>>> It doesn't make sense to have exit latency of 0. Rejecting
>>>> SET_FEATURE(enable U1/U2) should already let the host know that the
>>>> device doesn't support U1/U2.
>>>>
>>> I am okay to remove this, but I feel that it is better to report zero value instead
>>> of a non-zero value in exit latency of BOS when U1 or U2 entries are not supported.
>>> Advantage of reporting 0 is that some hosts doesn't even send
>> SET_FEATURE(U1/U2)
>>> requests on seeing zero value in BOS descriptor. Also there can be cases where U1 is
>>> disabled and U2 entry is allowed or vice versa, for these kind of cases the driver can
>>> set zero exit latency value for U1 and non-zero exit latency value for U2 . Based on
>> this
>>> I think it would be better to report 0 when U1/U2 states are not enabled. Please
>> provide
>>> your opinion on this.
>> Hm... I assume you're testing against linux usb stack and xhci host. If
>> that's the case, it looks like host will still request the device to
>> enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
>> needs to be fixed. I think what you have is fine to workaround this issue.
> Thanks . Will send the next series with the other fixes that you have suggested
>
> Best Regards,
> Anurag Kumar Vulisha
>

I want to try something. Can you see if this helps with your performance
test without setting the U1/U2 exit latency to 0?
(No need to change what you have in your patch. This is just for testing).

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 2f94568ba385..619351c581cf 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4057,8 +4057,18 @@ static void usb_enable_link_state(struct usb_hcd
*hcd, struct usb_device *udev,
                /* Only a configured device will accept the Set Feature
                 * U1/U2_ENABLE
                 */
-               if (udev->actconfig)
-                       usb_set_device_initiated_lpm(udev, state, true);
+               if (udev->actconfig) {
+                       if (usb_set_device_initiated_lpm(udev, state,
true)) {
+                               /*
+                                * Don't request U1/U2 entry if the device
+                                * cannot enable U1/U2.
+                                */
+                               usb_set_lpm_timeout(udev, state, 0);
+                              
hcd->driver->disable_usb3_lpm_timeout(hcd, udev,
+                                                                    
state);
+                               return;
+                       }
+               }
 
                /* As soon as usb_set_lpm_timeout(timeout) returns 0, the
                 * hub-initiated LPM is enabled. Thus, LPM is enabled no


Thanks,
Thinh

RE: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Anurag Kumar Vulisha]

Hi Thinh,

>-----Original Message-----
>From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>Sent: Saturday, May 11, 2019 7:18 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
><Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
><mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
><cst@phaseone.com>
>Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; v.anuragkumar@gmail.com
>Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>entries
>
>Hi,
>
>Anurag Kumar Vulisha wrote:
>> Hi Thinh,
>>
>>> -----Original Message-----
>>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>>> Sent: Friday, May 10, 2019 5:30 AM
>>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
>>> <Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
>>> <mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
>>> <cst@phaseone.com>
>>> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; v.anuragkumar@gmail.com
>>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and
>U2
>>> entries
>>>
>>> Hi Anurag,
>>>
>>> Anurag Kumar Vulisha wrote:
>>>>>> +		return -EINVAL;
>>>>>>
>>>>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>>>>>  	if (set)
>>>>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc,
>>> struct
>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>> index e293400..f2d3112 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>>>>  	return 0;
>>>>>>  }
>>>>>>
>>>>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>>>>> +				      struct usb_dcd_config_params *params)
>>>>>> +{
>>>>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>>>>> +
>>>>>> +	/* U1 Device exit Latency */
>>>>>> +	if (dwc->dis_u1_entry_quirk)
>>>>>> +		params->bU1devExitLat = 0;
>>>>> It doesn't make sense to have exit latency of 0. Rejecting
>>>>> SET_FEATURE(enable U1/U2) should already let the host know that the
>>>>> device doesn't support U1/U2.
>>>>>
>>>> I am okay to remove this, but I feel that it is better to report zero value instead
>>>> of a non-zero value in exit latency of BOS when U1 or U2 entries are not
>supported.
>>>> Advantage of reporting 0 is that some hosts doesn't even send
>>> SET_FEATURE(U1/U2)
>>>> requests on seeing zero value in BOS descriptor. Also there can be cases where
>U1 is
>>>> disabled and U2 entry is allowed or vice versa, for these kind of cases the driver
>can
>>>> set zero exit latency value for U1 and non-zero exit latency value for U2 . Based
>on
>>> this
>>>> I think it would be better to report 0 when U1/U2 states are not enabled. Please
>>> provide
>>>> your opinion on this.
>>> Hm... I assume you're testing against linux usb stack and xhci host. If
>>> that's the case, it looks like host will still request the device to
>>> enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
>>> needs to be fixed. I think what you have is fine to workaround this issue.
>> Thanks . Will send the next series with the other fixes that you have suggested
>>
>> Best Regards,
>> Anurag Kumar Vulisha
>>
>
>I want to try something. Can you see if this helps with your performance
>test without setting the U1/U2 exit latency to 0?
>(No need to change what you have in your patch. This is just for testing).
>

With your patch , the link doesn't enter into U1/U2 and I am also getting
better performance

Thanks,
Anurag Kumar Vulisha

>diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>index 2f94568ba385..619351c581cf 100644
>--- a/drivers/usb/core/hub.c
>+++ b/drivers/usb/core/hub.c
>@@ -4057,8 +4057,18 @@ static void usb_enable_link_state(struct usb_hcd
>*hcd, struct usb_device *udev,
>                /* Only a configured device will accept the Set Feature
>                 * U1/U2_ENABLE
>                 */
>-               if (udev->actconfig)
>-                       usb_set_device_initiated_lpm(udev, state, true);
>+               if (udev->actconfig) {
>+                       if (usb_set_device_initiated_lpm(udev, state,
>true)) {
>+                               /*
>+                                * Don't request U1/U2 entry if the device
>+                                * cannot enable U1/U2.
>+                                */
>+                               usb_set_lpm_timeout(udev, state, 0);
>+
>hcd->driver->disable_usb3_lpm_timeout(hcd, udev,
>+
>state);
>+                               return;
>+                       }
>+               }
>
>                /* As soon as usb_set_lpm_timeout(timeout) returns 0, the
>                 * hub-initiated LPM is enabled. Thus, LPM is enabled no
>
>

Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Thinh Nguyen]

Hi Anurag,

Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
>> -----Original Message-----
>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>> Sent: Saturday, May 11, 2019 7:18 AM
>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
>> <mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
>> <cst@phaseone.com>
>> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; v.anuragkumar@gmail.com
>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>> entries
>>
>> Hi,
>>
>> Anurag Kumar Vulisha wrote:
>>> Hi Thinh,
>>>
>>>> -----Original Message-----
>>>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>>>> Sent: Friday, May 10, 2019 5:30 AM
>>>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
>>>> <Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
>>>> <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
>>>> <mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
>>>> <cst@phaseone.com>
>>>> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; v.anuragkumar@gmail.com
>>>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and
>> U2
>>>> entries
>>>>
>>>> Hi Anurag,
>>>>
>>>> Anurag Kumar Vulisha wrote:
>>>>>>> +		return -EINVAL;
>>>>>>>
>>>>>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>>>>>>  	if (set)
>>>>>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc,
>>>> struct
>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>>> index e293400..f2d3112 100644
>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>>>>>> +				      struct usb_dcd_config_params *params)
>>>>>>> +{
>>>>>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>>>>>> +
>>>>>>> +	/* U1 Device exit Latency */
>>>>>>> +	if (dwc->dis_u1_entry_quirk)
>>>>>>> +		params->bU1devExitLat = 0;
>>>>>> It doesn't make sense to have exit latency of 0. Rejecting
>>>>>> SET_FEATURE(enable U1/U2) should already let the host know that the
>>>>>> device doesn't support U1/U2.
>>>>>>
>>>>> I am okay to remove this, but I feel that it is better to report zero value instead
>>>>> of a non-zero value in exit latency of BOS when U1 or U2 entries are not
>> supported.
>>>>> Advantage of reporting 0 is that some hosts doesn't even send
>>>> SET_FEATURE(U1/U2)
>>>>> requests on seeing zero value in BOS descriptor. Also there can be cases where
>> U1 is
>>>>> disabled and U2 entry is allowed or vice versa, for these kind of cases the driver
>> can
>>>>> set zero exit latency value for U1 and non-zero exit latency value for U2 . Based
>> on
>>>> this
>>>>> I think it would be better to report 0 when U1/U2 states are not enabled. Please
>>>> provide
>>>>> your opinion on this.
>>>> Hm... I assume you're testing against linux usb stack and xhci host. If
>>>> that's the case, it looks like host will still request the device to
>>>> enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
>>>> needs to be fixed. I think what you have is fine to workaround this issue.
>>> Thanks . Will send the next series with the other fixes that you have suggested
>>>
>>> Best Regards,
>>> Anurag Kumar Vulisha
>>>
>> I want to try something. Can you see if this helps with your performance
>> test without setting the U1/U2 exit latency to 0?
>> (No need to change what you have in your patch. This is just for testing).
>>
> With your patch , the link doesn't enter into U1/U2 and I am also getting
> better performance
>
> Thanks,
> Anurag Kumar Vulisha

Thanks for testing.

BR,
Thinh