| Message ID | 1556792423-4833-1-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: gadget: Add support for disabling U1 and U2 entries | expand |
Hi On tor, 2019-05-02 at 15:50 +0530, 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. > Will just vote for this feature, as I will also be able to use it for solving Rob Webers and my issue [1] Just today I was making another solution for this feature, using the configfs instead of the devicetree. Though thinks your solution is better, as it uses the U1DevExitLat and U2DevExitLat instead. I just added my solution to the bottem of the mail for reference. [1] https://www.spinics.net/lists/linux-usb/msg179393.html --- From 798ea2f5f365ecdf2dbcf436a2a0302e208c6c73 Mon Sep 17 00:00:00 2001 From: "Claus H. Stovgaard" <cst@phaseone.com> Date: Thu, 2 May 2019 17:54:45 +0200 Subject: [PATCH] usb: gadget: configfs: Add lpm_Ux_disable 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. This patch create an interface via configfs for disabling U1/U2, enabling a workaround for devices based on dwc3. Signed-off-by: Claus H. Stovgaard <cst@phaseone.com> --- drivers/usb/dwc3/ep0.c | 9 ++++++- drivers/usb/gadget/configfs.c | 56 +++++++++++++++++++++++++++++++++++++++++++ include/linux/usb/gadget.h | 6 ++++- 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 8efde17..5b2d26b 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->gadget_driver->lpm_U1_disable) + 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->gadget_driver->lpm_U2_disable) + 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->gadget_driver->lpm_U1_disable) + reg |= DWC3_DCTL_ACCEPTU1ENA; + if (!dwc->gadget_driver->lpm_U2_disable) + reg |= DWC3_DCTL_ACCEPTU2ENA; dwc3_writel(dwc->regs, DWC3_DCTL, reg); } break; diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c index 0251299..2ee9d10 100644 --- a/drivers/usb/gadget/configfs.c +++ b/drivers/usb/gadget/configfs.c @@ -229,6 +229,56 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct config_item *item, return len; } +static ssize_t gadget_dev_desc_lpm_U1_disable_show(struct config_item *item, + char *page) +{ + struct gadget_info *gi = to_gadget_info(item); + + return sprintf(page, "%d\n", + gi->composite.gadget_driver.lpm_U1_disable); +} + +static ssize_t gadget_dev_desc_lpm_U1_disable_store(struct config_item *item, + const char *page, size_t len) +{ + struct gadget_info *gi = to_gadget_info(item); + bool disable; + int ret; + + ret = strtobool(page, &disable); + if (!ret) { + gi->composite.gadget_driver.lpm_U1_disable = disable; + ret = len; + } + + return ret; +} + +static ssize_t gadget_dev_desc_lpm_U2_disable_show(struct config_item *item, + char *page) +{ + struct gadget_info *gi = to_gadget_info(item); + + return sprintf(page, "%d\n", + gi->composite.gadget_driver.lpm_U2_disable); +} + +static ssize_t gadget_dev_desc_lpm_U2_disable_store(struct config_item *item, + const char *page, size_t len) +{ + struct gadget_info *gi = to_gadget_info(item); + bool disable; + int ret; + + ret = strtobool(page, &disable); + if (!ret) { + gi->composite.gadget_driver.lpm_U2_disable = disable; + ret = len; + } + + return ret; +} + static ssize_t gadget_dev_desc_UDC_show(struct config_item *item, char *page) { char *udc_name = to_gadget_info(item)- >composite.gadget_driver.udc_name; @@ -299,6 +349,8 @@ CONFIGFS_ATTR(gadget_dev_desc_, idVendor); CONFIGFS_ATTR(gadget_dev_desc_, idProduct); CONFIGFS_ATTR(gadget_dev_desc_, bcdDevice); CONFIGFS_ATTR(gadget_dev_desc_, bcdUSB); +CONFIGFS_ATTR(gadget_dev_desc_, lpm_U1_disable); +CONFIGFS_ATTR(gadget_dev_desc_, lpm_U2_disable); CONFIGFS_ATTR(gadget_dev_desc_, UDC); static struct configfs_attribute *gadget_root_attrs[] = { @@ -310,6 +362,8 @@ static struct configfs_attribute *gadget_root_attrs[] = { &gadget_dev_desc_attr_idProduct, &gadget_dev_desc_attr_bcdDevice, &gadget_dev_desc_attr_bcdUSB, + &gadget_dev_desc_attr_lpm_U1_disable, + &gadget_dev_desc_attr_lpm_U2_disable, &gadget_dev_desc_attr_UDC, NULL, }; @@ -1408,6 +1462,8 @@ static const struct usb_gadget_driver configfs_driver_template = { .name = "configfs-gadget", }, .match_existing_only = 1, + .lpm_U1_disable = 0, + .lpm_U2_disable = 0, }; static struct config_group *gadgets_make( diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 7595056..25fe72b 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -619,7 +619,9 @@ static inline int usb_gadget_activate(struct usb_gadget *gadget) * this driver will be bound to any available UDC. * @pending: UDC core private data used for deferred probe of this driver. * @match_existing_only: If udc is not found, return an error and don't add this - * gadget driver to list of pending driver + * gadget driver to list of pending driver. + * @lpm_U1_disable: Instruct the UDC to disable U1 if possible. + * @lpm_U2_disable: Instruct the UDC to disable U2 if possible. * * Devices are disabled till a gadget driver successfully bind()s, which * means the driver will handle setup() requests needed to enumerate (and @@ -684,6 +686,8 @@ struct usb_gadget_driver { char *udc_name; struct list_head pending; unsigned match_existing_only:1; + unsigned lpm_U1_disable:1; + unsigned lpm_U2_disable:1; };
Hi Claus, >-----Original Message----- >From: claus.stovgaard@gmail.com [mailto:claus.stovgaard@gmail.com] >Sent: Friday, May 03, 2019 3:06 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; v.anuragkumar@gmail.com; Rob Weber ><rob@gnarbox.com> >Subject: Re: [PATCH 0/3] usb: gadget: Add support for disabling U1 and U2 entries > >Hi > >On tor, 2019-05-02 at 15:50 +0530, 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. >> > >Will just vote for this feature, as I will also be able to use it for >solving Rob Webers and my issue [1] > Thanks for testing and voting for this patch. >Just today I was making another solution for this feature, using the >configfs instead of the devicetree. Though thinks your solution is >better, as it uses the U1DevExitLat and U2DevExitLat instead. I just >added my solution to the bottem of the mail for reference. > >[1] https://www.spinics.net/lists/linux-usb/msg179393.html > Your approach below is also good, but you are just avoiding the gadget dwc3 controller from entering into U1 and U2 states by disabling the ACCEPTU1ENA and ACCEPTU2ENA bits in DCTL but not preventing the host from sending the LG0_U1 and LGO_U2 link command signaling to the gadget. The host will keep on trying to get the link into U1 or U2 by sending LGO_U1 or LGO_U2 and the gadget rejects these signals by sending LXU link command. To avoid this extra overhead I thought that sending zero value in the BOS descriptor's U1DevExitLat and U2DevExitLat fields would be the best option. Host on seeing U 1 & U2 Exit Latencies doesn't initiate LPM U1 and U2 commands. Thanks, Anurag Kumar Vulisha >--- > >From 798ea2f5f365ecdf2dbcf436a2a0302e208c6c73 Mon Sep 17 00:00:00 2001 >From: "Claus H. Stovgaard" <cst@phaseone.com> >Date: Thu, 2 May 2019 17:54:45 +0200 >Subject: [PATCH] usb: gadget: configfs: Add lpm_Ux_disable > >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. This patch create an interface via >configfs for disabling U1/U2, enabling a workaround for devices based >on >dwc3. > >Signed-off-by: Claus H. Stovgaard <cst@phaseone.com> >--- > drivers/usb/dwc3/ep0.c | 9 ++++++- > drivers/usb/gadget/configfs.c | 56 >+++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb/gadget.h | 6 ++++- > 3 files changed, 69 insertions(+), 2 deletions(-) > >diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >index 8efde17..5b2d26b 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->gadget_driver->lpm_U1_disable) >+ 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->gadget_driver->lpm_U2_disable) >+ 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->gadget_driver->lpm_U1_disable) >+ reg |= DWC3_DCTL_ACCEPTU1ENA; >+ if (!dwc->gadget_driver->lpm_U2_disable) >+ reg |= DWC3_DCTL_ACCEPTU2ENA; > dwc3_writel(dwc->regs, DWC3_DCTL, reg); > } > break; >diff --git a/drivers/usb/gadget/configfs.c >b/drivers/usb/gadget/configfs.c >index 0251299..2ee9d10 100644 >--- a/drivers/usb/gadget/configfs.c >+++ b/drivers/usb/gadget/configfs.c >@@ -229,6 +229,56 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct >config_item *item, > return len; > } > >+static ssize_t gadget_dev_desc_lpm_U1_disable_show(struct config_item >*item, >+ char *page) >+{ >+ struct gadget_info *gi = to_gadget_info(item); >+ >+ return sprintf(page, "%d\n", >+ gi->composite.gadget_driver.lpm_U1_disable); >+} >+ >+static ssize_t gadget_dev_desc_lpm_U1_disable_store(struct config_item >*item, >+ const char *page, size_t len) >+{ >+ struct gadget_info *gi = to_gadget_info(item); >+ bool disable; >+ int ret; >+ >+ ret = strtobool(page, &disable); >+ if (!ret) { >+ gi->composite.gadget_driver.lpm_U1_disable = disable; >+ ret = len; >+ } >+ >+ return ret; >+} >+ >+static ssize_t gadget_dev_desc_lpm_U2_disable_show(struct config_item >*item, >+ char *page) >+{ >+ struct gadget_info *gi = to_gadget_info(item); >+ >+ return sprintf(page, "%d\n", >+ gi->composite.gadget_driver.lpm_U2_disable); >+} >+ >+static ssize_t gadget_dev_desc_lpm_U2_disable_store(struct config_item >*item, >+ const char *page, size_t len) >+{ >+ struct gadget_info *gi = to_gadget_info(item); >+ bool disable; >+ int ret; >+ >+ ret = strtobool(page, &disable); >+ if (!ret) { >+ gi->composite.gadget_driver.lpm_U2_disable = disable; >+ ret = len; >+ } >+ >+ return ret; >+} >+ > static ssize_t gadget_dev_desc_UDC_show(struct config_item *item, char >*page) > { > char *udc_name = to_gadget_info(item)- >>composite.gadget_driver.udc_name; >@@ -299,6 +349,8 @@ CONFIGFS_ATTR(gadget_dev_desc_, idVendor); > CONFIGFS_ATTR(gadget_dev_desc_, idProduct); > CONFIGFS_ATTR(gadget_dev_desc_, bcdDevice); > CONFIGFS_ATTR(gadget_dev_desc_, bcdUSB); >+CONFIGFS_ATTR(gadget_dev_desc_, lpm_U1_disable); >+CONFIGFS_ATTR(gadget_dev_desc_, lpm_U2_disable); > CONFIGFS_ATTR(gadget_dev_desc_, UDC); > > static struct configfs_attribute *gadget_root_attrs[] = { >@@ -310,6 +362,8 @@ static struct configfs_attribute >*gadget_root_attrs[] = { > &gadget_dev_desc_attr_idProduct, > &gadget_dev_desc_attr_bcdDevice, > &gadget_dev_desc_attr_bcdUSB, >+ &gadget_dev_desc_attr_lpm_U1_disable, >+ &gadget_dev_desc_attr_lpm_U2_disable, > &gadget_dev_desc_attr_UDC, > NULL, > }; >@@ -1408,6 +1462,8 @@ static const struct usb_gadget_driver >configfs_driver_template = { > .name = "configfs-gadget", > }, > .match_existing_only = 1, >+ .lpm_U1_disable = 0, >+ .lpm_U2_disable = 0, > }; > > static struct config_group *gadgets_make( >diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >index 7595056..25fe72b 100644 >--- a/include/linux/usb/gadget.h >+++ b/include/linux/usb/gadget.h >@@ -619,7 +619,9 @@ static inline int usb_gadget_activate(struct >usb_gadget *gadget) > * this driver will be bound to any available UDC. > * @pending: UDC core private data used for deferred probe of this >driver. > * @match_existing_only: If udc is not found, return an error and >don't add this >- * gadget driver to list of pending driver >+ * gadget driver to list of pending driver. >+ * @lpm_U1_disable: Instruct the UDC to disable U1 if possible. >+ * @lpm_U2_disable: Instruct the UDC to disable U2 if possible. > * > * Devices are disabled till a gadget driver successfully bind()s, >which > * means the driver will handle setup() requests needed to enumerate >(and >@@ -684,6 +686,8 @@ struct usb_gadget_driver { > char *udc_name; > struct list_head pending; > unsigned match_existing_only:1; >+ unsigned lpm_U1_disable:1; >+ unsigned lpm_U2_disable:1; > }; > > >-- >2.7.4
Hi Anurag On Fri, 2019-05-03 at 07:34 +0000, Anurag Kumar Vulisha wrote: > Hi Claus, > Thanks for testing and voting for this patch. I have first tested your patches today. My test setup is a ZynqMP device, running kernel 4.14 (Xilinx version) with your patches. I then create an overlay for the devicetree with the new parameters, and unbind/bind the dwc3 driver. Next I have a host running Windows 10 and a MacBook pro with Type-C ports. For logging the communication I use a Total Phase Beagle USB3 5000 V2 analyzer. The test showed that OS-X does as expected. When BOS descriptor (bU1DevExtLat and bU2DevExtLat) returns 0, it does not enable LPM. Windows 10 on the other hand does not, and even though it received 0 as bU1DevExtLat and bU2DevExtLat it send Set Sel with U1SEL 85 us, U1PEL 0 us, U2SEL 85 us and U2PEL 0 us. Next the Windows 10 host sends the U1 Enable and the U2 enable as Set Device Feature, resulting in the system entering U2. > > > > Just today I was making another solution for this feature, using > > the > > configfs instead of the devicetree. Though thinks your solution is > > better, as it uses the U1DevExitLat and U2DevExitLat instead. I > > just > > added my solution to the bottem of the mail for reference. > > > > [1] https://www.spinics.net/lists/linux-usb/msg179393.html > > > Your approach below is also good, but you are just avoiding the > gadget dwc3 > controller from entering into U1 and U2 states by disabling the > ACCEPTU1ENA > and ACCEPTU2ENA bits in DCTL but not preventing the host from sending > the > LG0_U1 and LGO_U2 link command signaling to the gadget. The host will > keep > on trying to get the link into U1 or U2 by sending LGO_U1 or LGO_U2 > and the > gadget rejects these signals by sending LXU link command. To avoid > this extra > overhead I thought that sending zero value in the BOS descriptor's > U1DevExitLat and U2DevExitLat fields would be the best option. Host > on seeing > U 1 & U2 Exit Latencies doesn't initiate LPM U1 and U2 commands. > > Thanks, > Anurag Kumar Vulisha Correct that it does not prevent the host from sending LG0_U1 and LG0_U2, and there is your solution better on hosts using the BOS descriptor for disabling LPM. So based on my test with Windows 10, I think we should combine the solutions. To prevent LG0_U1/LG0_U2 when possible and still being able to completely disable U1/U2. Regarding interface for controlling it. I am very novice regarding Linux kernel development, but would think the BOS descriptor control would be better from a configfs interface then devicetree as I don't see BOS descriptor as hardware specific. I am more in doubt about the forcing of U1/U2 as I did with setting hardware registers, as it control hardware registers. So will like to hear from other more experienced developers. Regards Claus