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

[PATCH 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. For example, when performing performance
benchmarking on mass storage gadget the U1 and U2 entries can be disabled.
Another example is 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 (since ping is issued from host for every uframe interval). In this
case the U1 and U2 entry can be disabled. This 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
commands to the gadget. Thus entry of U1 and U2 states can be avioded.
This patch updates the same.

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/gadget.c                      | 19 +++++++++++++++++++
 drivers/usb/dwc3/gadget.h                      |  6 ++++++
 drivers/usb/gadget/composite.c                 |  2 +-
 include/linux/usb/gadget.h                     |  3 ++-
 7 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.1.1

[PATCH 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>
---
 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..b087248 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 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>
---
 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 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. For example, when performing performance
benchmarking on mass storage gadget the U1 and U2 entries can be disabled.
Another example is 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 (since ping is issued from host for every uframe interval). In this
case the U1 and U2 entry can be disabled. This 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
commands to the gadget. Thus entry of U1 and U2 states can be avioded.
This patch updates the same

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
---
 drivers/usb/dwc3/core.c   |  4 ++++
 drivers/usb/dwc3/core.h   |  4 ++++
 drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
 drivers/usb/dwc3/gadget.h |  6 ++++++
 4 files changed, 33 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a1b126f..4f0912c 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/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 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. For example, when performing performance
> benchmarking on mass storage gadget the U1 and U2 entries can be disabled.
> Another example is 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 (since ping is issued from host for every uframe interval). In this
> case the U1 and U2 entry can be disabled. This 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
> commands to the gadget. Thus entry of U1 and U2 states can be avioded.
> This patch updates the same

I don't think we should assume that's the case for every host driver.

>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> ---
>  drivers/usb/dwc3/core.c   |  4 ++++
>  drivers/usb/dwc3/core.h   |  4 ++++
>  drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
>  drivers/usb/dwc3/gadget.h |  6 ++++++
>  4 files changed, 33 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a1b126f..4f0912c 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");

Please use "-" rather than "_" in the property names.

>  	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/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))

Setting the exit latency for U1/U2 to 0 to prevent U1/U2 entry looks
more like a workaround than actually disabling U1/U2. There are
DCTL.INITU1/2ENA and DCTL.ACCEPTU1/2ENA for that.

Also, if these properties are set, then the device should rejects
SET_FEATURE(U1/U2) requests.

You can review Felipe's little code snippet from here to disable U1/U2:
https://marc.info/?l=linux-usb&m=155419284426942&w=2

BR,
Thinh

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

[Claus H. Stovgaard]

Hi Thinh and Anurag

On man, 2019-05-06 at 19:21 +0000, Thinh Nguyen wrote:

> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index a1b126f..4f0912c 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");
> 
> Please use "-" rather than "_" in the property names.

I have thought about this feature over the weekend, and think the
naming should be changed to something like "snps,bos-u1-exit-lat-in-us" 
and named the same in the code. And then be the value used by the
get_config_params. E.g. the device-tree is used to set the values
directly used for bUxdevExitLat instead of named something not related
to exit latency.

With this the name and function is a 1 to 1 match, and you can among
others set it to 0 for optaining what Anurag wants.

Regarding the disabling of U1 / U2. I send this to Anurag
https://marc.info/?l=linux-usb&m=155683299311954&w=2
Here i created a configfs interface with the names "lpm_U1_disable" and
"lpm_U2_disable" for controlling the DTCL of dwc3, and reject
SET_FEATURE(U1/U2)

Will send this in seperate patch tomorrow, in the hope that Anurags
feature can become a way for controlling exit latency, and my patch
become a way for disabling U1/U2

BR
Claus

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

[Anurag Kumar Vulisha]

Hi Claus,

>-----Original Message-----
>From: Claus H. Stovgaard [mailto:cst@phaseone.com]
>Sent: Tuesday, May 07, 2019 2:28 AM
>To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; 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>
>Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; v.anuragkumar@gmail.com
>Subject: Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>entries
>
>Hi Thinh and Anurag
>
>On man, 2019-05-06 at 19:21 +0000, Thinh Nguyen wrote:
>
>> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
>> > a1b126f..4f0912c 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");
>>
>> Please use "-" rather than "_" in the property names.
>
>I have thought about this feature over the weekend, and think the naming should be
>changed to something like "snps,bos-u1-exit-lat-in-us"
>and named the same in the code. And then be the value used by the
>get_config_params. E.g. the device-tree is used to set the values directly used for
>bUxdevExitLat instead of named something not related to exit latency.
>
>With this the name and function is a 1 to 1 match, and you can among others set it to
>0 for optaining what Anurag wants.
>

Your suggestion looks good but the problem is the U1 and U2 exit latencies are
fixed values in dwc3 controller(can be found in HCSPARAMS3). Adding different
exit latencies may modify the U1SEL/U2SEL values sent from the host but the real
dwc3 controller exit latencies are not getting changed. Because of this reason I
had opted "snps,dis_u1_entry_quirk", so that the U1/U2 exit latency values
reported in BOS descriptor can be either be zero (when U1/U2 entries needs to be
disabled) or non-zero value (reported in HCSPARAMS3) when U1/U2 states allowed.
Based on this I think it is better if we can continue with "snps,dis-u1-entry-quirk"
instead of the "snps,bos-u1-exit-lat-in-us". Please  provide your opinion on this.
 
>Regarding the disabling of U1 / U2. I send this to Anurag
>https://marc.info/?l=linux-usb&m=155683299311954&w=2
>Here i created a configfs interface with the names "lpm_U1_disable" and
>"lpm_U2_disable" for controlling the DTCL of dwc3, and reject
>SET_FEATURE(U1/U2)
>
>Will send this in seperate patch tomorrow, in the hope that Anurags feature can
>become a way for controlling exit latency, and my patch become a way for disabling
>U1/U2
>

I agree with your suggestion. When U1 and U2 entries are not allowed  it is always
better to report zero value for U1/U2 exit latencies in BOS descriptor (no point in
reporting non-zero exit latency values when U1/U2 states are not allowed).  Along
with that changes for preventing the dwc3 controller from initiating or accepting
U1/U2 requests are also required (since there are some host platforms where sending
0 exit latency doesn't work). Based on these observations I believe both your patch
changes and my patch changes needs to be added.

@Thinh Nguyen
Please provide your opinion on this

Thanks,
Anurag Kumar Vulisha

>BR
>Claus

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

[Claus H. Stovgaard]

Hi Anurag

> > > Please use "-" rather than "_" in the property names.
> > I have thought about this feature over the weekend, and think the
> > naming should be
> > changed to something like "snps,bos-u1-exit-lat-in-us"
> > and named the same in the code. And then be the value used by the
> > get_config_params. E.g. the device-tree is used to set the values
> > directly used for
> > bUxdevExitLat instead of named something not related to exit
> > latency.
> > 
> > With this the name and function is a 1 to 1 match, and you can
> > among others set it to
> > 0 for optaining what Anurag wants.
> > 
> Your suggestion looks good but the problem is the U1 and U2 exit
> latencies are
> fixed values in dwc3 controller(can be found in HCSPARAMS3). Adding
> different
> exit latencies may modify the U1SEL/U2SEL values sent from the host
> but the real
> dwc3 controller exit latencies are not getting changed. Because of
> this reason I
> had opted "snps,dis_u1_entry_quirk", so that the U1/U2 exit latency
> values
> reported in BOS descriptor can be either be zero (when U1/U2 entries
> needs to be
> disabled) or non-zero value (reported in HCSPARAMS3) when U1/U2
> states allowed.
> Based on this I think it is better if we can continue with "snps,dis-
> u1-entry-quirk"
> instead of the "snps,bos-u1-exit-lat-in-us". Please  provide your
> opinion on this.

With this in mind I can see why having direct control over the exit
latency value might not be optimum in many situations.
Regarding the name, I think the snps,dis_u1_entry_quirk will be a good
name, if it is combined with the DCTL control. E.g. remove the configfs
part of my patch, and merge the DCTL control with your patches.
If the dt-binding still only control the bos descriptor I think a
better name is something with u1_force_exist_lat_0 or similar.

I don't think setting bos to 0 or controlling DCTL will be used
individual, so to keep things simple I will vote for
snps,dis_u1_entry_quirk, and then just control all elements regarding
disabling U1/U2 from this dt-binding.

Please cut what your need from my patch.

BR Claus

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

[Anurag Kumar Vulisha]

Hi Claus,

>-----Original Message-----
>From: Claus H. Stovgaard [mailto:cst@phaseone.com]
>Sent: Tuesday, May 07, 2019 6:47 PM
>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>
>Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; v.anuragkumar@gmail.com
>Subject: Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>entries
>
>Hi Anurag
>
>> > > Please use "-" rather than "_" in the property names.
>> > I have thought about this feature over the weekend, and think the
>> > naming should be
>> > changed to something like "snps,bos-u1-exit-lat-in-us"
>> > and named the same in the code. And then be the value used by the
>> > get_config_params. E.g. the device-tree is used to set the values
>> > directly used for
>> > bUxdevExitLat instead of named something not related to exit
>> > latency.
>> >
>> > With this the name and function is a 1 to 1 match, and you can
>> > among others set it to
>> > 0 for optaining what Anurag wants.
>> >
>> Your suggestion looks good but the problem is the U1 and U2 exit
>> latencies are
>> fixed values in dwc3 controller(can be found in HCSPARAMS3). Adding
>> different
>> exit latencies may modify the U1SEL/U2SEL values sent from the host
>> but the real
>> dwc3 controller exit latencies are not getting changed. Because of
>> this reason I
>> had opted "snps,dis_u1_entry_quirk", so that the U1/U2 exit latency
>> values
>> reported in BOS descriptor can be either be zero (when U1/U2 entries
>> needs to be
>> disabled) or non-zero value (reported in HCSPARAMS3) when U1/U2
>> states allowed.
>> Based on this I think it is better if we can continue with "snps,dis-
>> u1-entry-quirk"
>> instead of the "snps,bos-u1-exit-lat-in-us". Please  provide your
>> opinion on this.
>
>With this in mind I can see why having direct control over the exit
>latency value might not be optimum in many situations.
>Regarding the name, I think the snps,dis_u1_entry_quirk will be a good
>name, if it is combined with the DCTL control. E.g. remove the configfs
>part of my patch, and merge the DCTL control with your patches.
>If the dt-binding still only control the bos descriptor I think a
>better name is something with u1_force_exist_lat_0 or similar.
>
>I don't think setting bos to 0 or controlling DCTL will be used
>individual, so to keep things simple I will vote for
>snps,dis_u1_entry_quirk, and then just control all elements regarding
>disabling U1/U2 from this dt-binding.
>
>Please cut what your need from my patch.
>

Thanks for providing your input. Merging your solution and mine  would be
good. I will modify the patch to include your changes as well and send it to you.
Once you are okay with the changes, we can post the patch to mainline.

Thanks,
Anurag Kumar Vulisha 

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

[Thinh Nguyen]

Hi,

Anurag Kumar Vulisha wrote:
> Hi Claus,
>
>> -----Original Message-----
>> From: Claus H. Stovgaard [mailto:cst@phaseone.com]
>> Sent: Tuesday, May 07, 2019 2:28 AM
>> To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; 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>
>> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; v.anuragkumar@gmail.com
>> Subject: Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>> entries
>>
>> Hi Thinh and Anurag
>>
>> On man, 2019-05-06 at 19:21 +0000, Thinh Nguyen wrote:
>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
>>>> a1b126f..4f0912c 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");
>>> Please use "-" rather than "_" in the property names.
>> I have thought about this feature over the weekend, and think the naming should be
>> changed to something like "snps,bos-u1-exit-lat-in-us"
>> and named the same in the code. And then be the value used by the
>> get_config_params. E.g. the device-tree is used to set the values directly used for
>> bUxdevExitLat instead of named something not related to exit latency.
>>
>> With this the name and function is a 1 to 1 match, and you can among others set it to
>> 0 for optaining what Anurag wants.
>>
> Your suggestion looks good but the problem is the U1 and U2 exit latencies are
> fixed values in dwc3 controller(can be found in HCSPARAMS3). Adding different
> exit latencies may modify the U1SEL/U2SEL values sent from the host but the real
> dwc3 controller exit latencies are not getting changed. Because of this reason I
> had opted "snps,dis_u1_entry_quirk", so that the U1/U2 exit latency values
> reported in BOS descriptor can be either be zero (when U1/U2 entries needs to be
> disabled) or non-zero value (reported in HCSPARAMS3) when U1/U2 states allowed.
> Based on this I think it is better if we can continue with "snps,dis-u1-entry-quirk"
> instead of the "snps,bos-u1-exit-lat-in-us". Please  provide your opinion on this.

>  
>> Regarding the disabling of U1 / U2. I send this to Anurag
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D155683299311954-26w-3D2&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w&m=MBQpZmX-jgrlpT68k5VR-4xv_DYb5UGUiD5objMqwpA&s=Ca-zBV5t26-ZFPbNAkD8K3F3lbc3CwUXNpAgnkVasg4&e=
>> Here i created a configfs interface with the names "lpm_U1_disable" and
>> "lpm_U2_disable" for controlling the DTCL of dwc3, and reject
>> SET_FEATURE(U1/U2)
>>
>> Will send this in seperate patch tomorrow, in the hope that Anurags feature can
>> become a way for controlling exit latency, and my patch become a way for disabling
>> U1/U2
>>
> I agree with your suggestion. When U1 and U2 entries are not allowed  it is always
> better to report zero value for U1/U2 exit latencies in BOS descriptor (no point in
> reporting non-zero exit latency values when U1/U2 states are not allowed).  Along
> with that changes for preventing the dwc3 controller from initiating or accepting
> U1/U2 requests are also required (since there are some host platforms where sending
> 0 exit latency doesn't work). Based on these observations I believe both your patch
> changes and my patch changes needs to be added.
>
> @Thinh Nguyen
> Please provide your opinion on this
>

The 0 exit latency in the BOS descriptor doesn't mean that device
doesn't support U1/U2 (however unrealistic it may seem).

The exit latency values reported in the BOS decriptor are just
recommended latency. The host controls over what they should be (host
has its own U1/U2 exit latency). I don't think we should have a device
property to set the device exit latency.

If the gadget driver needs to know what the recommended latency to set
in the BOS descriptor, we can have those values exported to some new
fields in the usb_gadget structure.

BR,
Thinh

RE: [PATCH 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: Tuesday, May 07, 2019 12:51 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>
>Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; v.anuragkumar@gmail.com
>Subject: Re: [PATCH 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. For example, when performing performance
>> benchmarking on mass storage gadget the U1 and U2 entries can be disabled.
>> Another example is 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 (since ping is issued from host for every uframe
>> interval). In this case the U1 and U2 entry can be disabled. This 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 commands to the gadget. Thus entry of U1
>and U2 states can be avioded.
>> This patch updates the same
>
>I don't think we should assume that's the case for every host driver.

I agree with you, as Claus already mentioned, observed that windows 10
issues SET_SEL commands even after UxDevExitLat are zero. 
  
>
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>> ---
>>  drivers/usb/dwc3/core.c   |  4 ++++
>>  drivers/usb/dwc3/core.h   |  4 ++++
>>  drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
>> drivers/usb/dwc3/gadget.h |  6 ++++++
>>  4 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
>> a1b126f..4f0912c 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");
>
>Please use "-" rather than "_" in the property names.
>

Okay , will change this

>>  	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/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))
>
>Setting the exit latency for U1/U2 to 0 to prevent U1/U2 entry looks more like a
>workaround than actually disabling U1/U2. There are DCTL.INITU1/2ENA and
>DCTL.ACCEPTU1/2ENA for that.
>
>Also, if these properties are set, then the device should rejects
>SET_FEATURE(U1/U2) requests.
>
>You can review Felipe's little code snippet from here to disable U1/U2:
>https://marc.info/?l=linux-usb&m=155419284426942&w=2
>

As there are some host platforms (like windows 10) which initiates U1/U2 states
even after sending zero in UxExitLat of BOS descriptor, I agree with you that the
dwc3 controller should reject the U1/U2 requests from host by configuring DCTL.
Along with DCTL changes, I think the changes required for sending zero value in
UxExitLat field reported in the BOSDescriptor are also required (there is no point
in sending non-zero exit latencies when U1/U2 state entries are disabled). So, this
patch changes should be added along with the changes reported by Claus.
Please provide your suggestion on this

Thanks,
Anurag Kumar Vulisha