| Message ID | 1557220655-123090-1-git-send-email-cst@phaseone.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: configfs: Add lpm_Ux_disable | expand |
On Tue, 2019-05-07 at 11:09 +0200, Claus H. Stovgaard wrote: > 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 create an interface via > configfs for disabling U1/U2, enabling a workaround for devices based > on > dwc3. > Sorry messed up a bit with a missing cover letter. This feature relates to Anurag patch [1] and a thread [2] from earlier Where Anurags patch focus on setting U1/U2 latency in the BOS descriptor from the devicetree, this patch focuses on having a configfs interface for forcing the UDC (here the dwc3) to not enable U1/U2 and reject the SET_SEL(U1/U2). Looking forward to input. [1] https://www.spinics.net/lists/linux-usb/msg179732.html [2] https://www.spinics.net/lists/linux-usb/msg179393.html
Hi Claus, Claus H. Stovgaard wrote: > On Tue, 2019-05-07 at 11:09 +0200, Claus H. Stovgaard wrote: >> 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 create an interface via >> configfs for disabling U1/U2, enabling a workaround for devices based >> on >> dwc3. >> > Sorry messed up a bit with a missing cover letter. > This feature relates to Anurag patch [1] and a thread [2] from earlier > > Where Anurags patch focus on setting U1/U2 latency in the BOS > descriptor from the devicetree, this patch focuses on having a configfs > interface for forcing the UDC (here the dwc3) to not enable U1/U2 and > reject the SET_SEL(U1/U2). > > Looking forward to input. > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dusb_msg179732.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w&m=wKdyWmYpbW791LAm7rYwvFYx5E0bjENyXZzHvK4vyFo&s=es7kki6iuLJUp2rJnzP9alXKyfJPNSfyxTVCKKDd_rQ&e= > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dusb_msg179393.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w&m=wKdyWmYpbW791LAm7rYwvFYx5E0bjENyXZzHvK4vyFo&s=cFTmO9wPf7b6TZxFUAAIJM0Z_wM1ttNIc1rct0uR6co&e= > > I'm not sure who will submit the patch to make change to DWC3 for disabling U1/U2 (Anurag or you), but can you split your patch between dwc3 and configfs. Thanks, Thinh
Hi Thinh On tir, 2019-05-07 at 18:53 +0000, Thinh Nguyen wrote: > Claus H. Stovgaard wrote: > > > > Where Anurags patch focus on setting U1/U2 latency in the BOS > > descriptor from the devicetree, this patch focuses on having a > > configfs > > interface for forcing the UDC (here the dwc3) to not enable U1/U2 > > and > > reject the SET_SEL(U1/U2). > > > > Looking forward to input. > > > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinic > > s.net_lists_linux- > > 2Dusb_msg179732.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyh > > jrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY- > > w&m=wKdyWmYpbW791LAm7rYwvFYx5E0bjENyXZzHvK4vyFo&s=es7kki6iuLJUp2rJn > > zP9alXKyfJPNSfyxTVCKKDd_rQ&e= > > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinic > > s.net_lists_linux- > > 2Dusb_msg179393.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyh > > jrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY- > > w&m=wKdyWmYpbW791LAm7rYwvFYx5E0bjENyXZzHvK4vyFo&s=cFTmO9wPf7b6TZxFU > > AAIJM0Z_wM1ttNIc1rct0uR6co&e= > > > > > > I'm not sure who will submit the patch to make change to DWC3 for > disabling U1/U2 (Anurag or you), but can you split your patch between > dwc3 and configfs. Have just written with Anurag, and he will submit a new patch set, where he has taken the control in ep0.c from my patch, and combined with his devicetree bindings. So the plan is to drop the configfs interface completely, keep the devicetree binding names (dis-u1-entry- quirk) and let it do 3 things. *Sets the latency to 0 in the BOS *Disable U1/U2 acceptence *Disable U1/U2 initiating This also include rejecting SET_SEL. We think this is the best option, and then dropping this patch as it is. I will just send and email later on this thread, when the patch is available on marc.info as a link for reference to the future. Thanks Claus
Hi Claus & Thinh, >-----Original Message----- >From: Claus H. Stovgaard [mailto:cst@phaseone.com] >Sent: Wednesday, May 08, 2019 2:59 AM >To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; linux-usb@vger.kernel.org >Cc: Felipe Balbi <balbi@kernel.org>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Anurag Kumar Vulisha ><anuragku@xilinx.com> >Subject: Re: [PATCH] usb: gadget: configfs: Add lpm_Ux_disable > >Hi Thinh > >On tir, 2019-05-07 at 18:53 +0000, Thinh Nguyen wrote: >> Claus H. Stovgaard wrote: >> > >> > Where Anurags patch focus on setting U1/U2 latency in the BOS >> > descriptor from the devicetree, this patch focuses on having a >> > configfs >> > interface for forcing the UDC (here the dwc3) to not enable U1/U2 >> > and >> > reject the SET_SEL(U1/U2). >> > >> > Looking forward to input. >> > >> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinic >> > s.net_lists_linux- >> > >2Dusb_msg179732.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyh >> > jrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY- >> > >w&m=wKdyWmYpbW791LAm7rYwvFYx5E0bjENyXZzHvK4vyFo&s=es7kki6iuLJUp2rJn >> > zP9alXKyfJPNSfyxTVCKKDd_rQ&e= >> > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinic >> > s.net_lists_linux- >> > >2Dusb_msg179393.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyh >> > jrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY- >> > >w&m=wKdyWmYpbW791LAm7rYwvFYx5E0bjENyXZzHvK4vyFo&s=cFTmO9wPf7b6TZx >FU >> > AAIJM0Z_wM1ttNIc1rct0uR6co&e= >> > >> > >> >> I'm not sure who will submit the patch to make change to DWC3 for >> disabling U1/U2 (Anurag or you), but can you split your patch between >> dwc3 and configfs. > >Have just written with Anurag, and he will submit a new patch set, >where he has taken the control in ep0.c from my patch, and combined >with his devicetree bindings. So the plan is to drop the configfs >interface completely, keep the devicetree binding names (dis-u1-entry- >quirk) and let it do 3 things. >*Sets the latency to 0 in the BOS >*Disable U1/U2 acceptence >*Disable U1/U2 initiating >This also include rejecting SET_SEL. > >We think this is the best option, and then dropping this patch as it >is. > >I will just send and email later on this thread, when the patch is >available on marc.info as a link for reference to the future. > I have sent the v2 patch series with the changes that Claus mentioned above. The patch can be found here https://marc.info/?l=linux-kernel&m=155730212618388&w=2 Thanks, Anurag Kumar Vulisha
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; };
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 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(-)