| Message ID | 1532168920-3269-1-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 21, 2018 at 03:58:40PM +0530, Anurag Kumar Vulisha wrote: > By default when core sees any transaction error(CRC or overflow) > it replies with terminating retry ACK (Retry=1 and Nump == 0). > Enabling this Auto Retry feature in controller, on seeing any > transaction errors makes the core to send an non-terminating ACK > transaction packet (that is, ACK TP with Retry=1 and Nump != 0). > Doing so will give controller a chance to recover from the error > condition. > > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> > --- > Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++ > drivers/usb/dwc3/core.c | 16 ++++++++++++++++ > drivers/usb/dwc3/core.h | 6 ++++++ > 3 files changed, 27 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt > index 7f13ebe..2ba2bc2 100644 > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > @@ -94,6 +94,11 @@ Optional properties: > this and tx-thr-num-pkt-prd to a valid, non-zero value > 1-16 (DWC_usb31 programming guide section 1.2.3) to > enable periodic ESS TX threshold. > + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the s/_/-/ > + controller operating in Host mode on seeing transaction > + errors(CRC errors or internal overrun scenerios) on IN > + transfers to reply to the device with a non-terminating > + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0) Seems like the property is unnecessary. When would you not want this retry behavior? Why not just enable unconditionally in the driver? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, Thanks for your comments, please see my comments inline >-----Original Message----- >From: Rob Herring [mailto:robh@kernel.org] >Sent: Thursday, July 26, 2018 1:33 AM >To: Anurag Kumar Vulisha <anuragku@xilinx.com> >Cc: gregkh@linuxfoundation.org; mark.rutland@arm.com; balbi@kernel.org; >v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; >linux-kernel@vger.kernel.org >Subject: Re: [PATCH] usb: dwc3: core: Add quirk for enabling AutoRetry feature in the >controller > >On Sat, Jul 21, 2018 at 03:58:40PM +0530, Anurag Kumar Vulisha wrote: >> By default when core sees any transaction error(CRC or overflow) >> it replies with terminating retry ACK (Retry=1 and Nump == 0). >> Enabling this Auto Retry feature in controller, on seeing any >> transaction errors makes the core to send an non-terminating ACK >> transaction packet (that is, ACK TP with Retry=1 and Nump != 0). >> Doing so will give controller a chance to recover from the error >> condition. >> >> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> >> --- >> Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++ >> drivers/usb/dwc3/core.c | 16 ++++++++++++++++ >> drivers/usb/dwc3/core.h | 6 ++++++ >> 3 files changed, 27 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 7f13ebe..2ba2bc2 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -94,6 +94,11 @@ Optional properties: >> this and tx-thr-num-pkt-prd to a valid, non-zero value >> 1-16 (DWC_usb31 programming guide section 1.2.3) to >> enable periodic ESS TX threshold. >> + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the > >s/_/-/ Sorry, I am not able to understand what needs to fixed here. Please help me in understanding, so that I can fix it in v2. > >> + controller operating in Host mode on seeing transaction >> + errors(CRC errors or internal overrun scenerios) on IN >> + transfers to reply to the device with a non-terminating >> + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0) > >Seems like the property is unnecessary. When would you not want this >retry behavior? Why not just enable unconditionally in the driver? > There is no harm in adding this fix always but I think this Retry feature should be added depending on the user and type of the application. For example, applying this feature in a streaming application where isochronous transfers are used might end up in retrying in failed packet until timeout happens, this might reduce the performance. And there may be cases where the user want to know when the transfer error occurred. Enabling this feature always will fail in both the above mentioned cases. Because of this reason added this quirk, so that user can decide when to apply and not. Please correct me if I am wrong Thanks, Anurag Kumar Vulisha -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi, Anurag Kumar Vulisha <anuragku@xilinx.com> writes: > Hi Rob, > > Thanks for your comments, please see my comments inline > >>-----Original Message----- >>From: Rob Herring [mailto:robh@kernel.org] >>Sent: Thursday, July 26, 2018 1:33 AM >>To: Anurag Kumar Vulisha <anuragku@xilinx.com> >>Cc: gregkh@linuxfoundation.org; mark.rutland@arm.com; balbi@kernel.org; >>v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; >>linux-kernel@vger.kernel.org >>Subject: Re: [PATCH] usb: dwc3: core: Add quirk for enabling AutoRetry feature in the >>controller >> >>On Sat, Jul 21, 2018 at 03:58:40PM +0530, Anurag Kumar Vulisha wrote: >>> By default when core sees any transaction error(CRC or overflow) >>> it replies with terminating retry ACK (Retry=1 and Nump == 0). >>> Enabling this Auto Retry feature in controller, on seeing any >>> transaction errors makes the core to send an non-terminating ACK >>> transaction packet (that is, ACK TP with Retry=1 and Nump != 0). >>> Doing so will give controller a chance to recover from the error >>> condition. >>> >>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> >>> --- >>> Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++ >>> drivers/usb/dwc3/core.c | 16 ++++++++++++++++ >>> drivers/usb/dwc3/core.h | 6 ++++++ >>> 3 files changed, 27 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>b/Documentation/devicetree/bindings/usb/dwc3.txt >>> index 7f13ebe..2ba2bc2 100644 >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>> @@ -94,6 +94,11 @@ Optional properties: >>> this and tx-thr-num-pkt-prd to a valid, non-zero value >>> 1-16 (DWC_usb31 programming guide section 1.2.3) to >>> enable periodic ESS TX threshold. >>> + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the >> >>s/_/-/ > Sorry, I am not able to understand what needs to fixed here. Please help me in > understanding, so that I can fix it in v2. replace _ with - >>> + controller operating in Host mode on seeing transaction >>> + errors(CRC errors or internal overrun scenerios) on IN >>> + transfers to reply to the device with a non-terminating >>> + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0) >> >>Seems like the property is unnecessary. When would you not want this >>retry behavior? Why not just enable unconditionally in the driver? >> > There is no harm in adding this fix always but I think this Retry > feature should be added depending on the user and type of the > application. For example, applying this feature in a streaming > application where isochronous transfers are used might end up in read the docs. Auto Retry in only for non-isochronous IN endpoints. You don't need any quirk for this. Just enable it unconditionally. Actually, you wanna make sure the core can run in Host mode before setting this, but that's it. No need for a quirk.
Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:balbi@kernel.org] >Sent: Thursday, July 26, 2018 4:44 PM >To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Rob Herring <robh@kernel.org> >Cc: gregkh@linuxfoundation.org; mark.rutland@arm.com; >v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; >linux-kernel@vger.kernel.org >Subject: RE: [PATCH] usb: dwc3: core: Add quirk for enabling AutoRetry feature in the >controller > > >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>>b/Documentation/devicetree/bindings/usb/dwc3.txt >>>> index 7f13ebe..2ba2bc2 100644 >>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>>> @@ -94,6 +94,11 @@ Optional properties: >>>> this and tx-thr-num-pkt-prd to a valid, non-zero value >>>> 1-16 (DWC_usb31 programming guide section 1.2.3) to >>>> enable periodic ESS TX threshold. >>>> + - snps,enable_auto_retry: Set to enable Auto retry Feature to make >>>> + the >>> >>>s/_/-/ >> Sorry, I am not able to understand what needs to fixed here. Please >> help me in understanding, so that I can fix it in v2. > >replace _ with - > >>>> + controller operating in Host mode on seeing transaction >>>> + errors(CRC errors or internal overrun scenerios) on IN >>>> + transfers to reply to the device with a non-terminating >>>> + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0) >>> >>>Seems like the property is unnecessary. When would you not want this >>>retry behavior? Why not just enable unconditionally in the driver? >>> >> There is no harm in adding this fix always but I think this Retry >> feature should be added depending on the user and type of the >> application. For example, applying this feature in a streaming >> application where isochronous transfers are used might end up in > >read the docs. Auto Retry in only for non-isochronous IN endpoints. You don't need >any quirk for this. Just enable it unconditionally. Actually, you wanna make sure the >core can run in Host mode before setting this, but that's it. No need for a quirk. > Thanks for making me understand. Will remove this quirk and send v2 Best Regards, Anurag Kumar Vulisha -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 7f13ebe..2ba2bc2 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -94,6 +94,11 @@ Optional properties: this and tx-thr-num-pkt-prd to a valid, non-zero value 1-16 (DWC_usb31 programming guide section 1.2.3) to enable periodic ESS TX threshold. + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the + controller operating in Host mode on seeing transaction + errors(CRC errors or internal overrun scenerios) on IN + transfers to reply to the device with a non-terminating + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0) - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated. diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 1038075..1c1ab93 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -883,6 +883,20 @@ static int dwc3_core_init(struct dwc3 *dwc) dwc3_writel(dwc->regs, DWC3_GUCTL1, reg); } + if (dwc->autoretry_enable) { + reg = dwc3_readl(dwc->regs, DWC3_GUCTL); + + /* Enable Auto retry Feature to make the controller operating in + * Host mode on seeing transaction errors(CRC errors or internal + * overrun scenerios) on IN transfers to reply to the device + * with a non-terminating retry ACK (i.e, an ACK transcation + * packet with Retry=1 & Nump != 0) + */ + reg |= DWC3_GUCTL_HSTINAUTORETRY; + + dwc3_writel(dwc->regs, DWC3_GUCTL, reg); + } + /* * Must config both number of packets and max burst settings to enable * RX and/or TX threshold. @@ -1179,6 +1193,8 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->dis_metastability_quirk = device_property_read_bool(dev, "snps,dis_metastability_quirk"); + dwc->autoretry_enable = device_property_read_bool(dev, + "snps,enable_auto_retry"); dwc->lpm_nyet_threshold = lpm_nyet_threshold; dwc->tx_de_emphasis = tx_de_emphasis; diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 285ce0e..558dfda 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -227,6 +227,9 @@ #define DWC3_GCTL_GBLHIBERNATIONEN BIT(1) #define DWC3_GCTL_DSBLCLKGTNG BIT(0) +/* Global User Control Register */ +#define DWC3_GUCTL_HSTINAUTORETRY BIT(14) + /* Global User Control 1 Register */ #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS BIT(28) #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW BIT(24) @@ -984,6 +987,8 @@ struct dwc3_scratchpad_array { * 2 - No de-emphasis * 3 - Reserved * @dis_metastability_quirk: set to disable metastability quirk. + * @autoretry_enable: set to enable AutoRetry feature in controller to + * give an non terminating retry ACK (Retry=1 & Nump != 0) * @imod_interval: set the interrupt moderation interval in 250ns * increments or 0 to disable. */ @@ -1153,6 +1158,7 @@ struct dwc3 { unsigned tx_de_emphasis:2; unsigned dis_metastability_quirk:1; + unsigned autoretry_enable:1; u16 imod_interval; };
By default when core sees any transaction error(CRC or overflow) it replies with terminating retry ACK (Retry=1 and Nump == 0). Enabling this Auto Retry feature in controller, on seeing any transaction errors makes the core to send an non-terminating ACK transaction packet (that is, ACK TP with Retry=1 and Nump != 0). Doing so will give controller a chance to recover from the error condition. Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> --- Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++ drivers/usb/dwc3/core.c | 16 ++++++++++++++++ drivers/usb/dwc3/core.h | 6 ++++++ 3 files changed, 27 insertions(+)