| Message ID | 1556792423-4833-4-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: Add support for disabling U1 and U2 entries | expand |
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
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
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
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
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
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
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
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))
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(+)