* [PATCH 0/3] usb: dwc3: gadget: simple refactoring patches
@ 2022-02-28 15:08 Michael Grzeschik
2022-02-28 15:08 ` [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition Michael Grzeschik
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Michael Grzeschik @ 2022-02-28 15:08 UTC (permalink / raw)
To: linux-usb; +Cc: balbi, gregkh, kernel
This series includes some smaller refactoring patches to improve the
code quality of the dwc3 gadget. It includes no functional changes.
Michael Grzeschik (3):
usb: dwc3: gadget: ep_queue simplify isoc start condition
usb: dwc3: gadget: move cmd_endtransfer to extra function
usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps
drivers/usb/dwc3/gadget.c | 88 +++++++++++++++++----------------------
1 file changed, 39 insertions(+), 49 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition 2022-02-28 15:08 [PATCH 0/3] usb: dwc3: gadget: simple refactoring patches Michael Grzeschik @ 2022-02-28 15:08 ` Michael Grzeschik 2022-03-02 1:24 ` Thinh Nguyen 2022-02-28 15:08 ` [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function Michael Grzeschik 2022-02-28 15:08 ` [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps Michael Grzeschik 2 siblings, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2022-02-28 15:08 UTC (permalink / raw) To: linux-usb; +Cc: balbi, gregkh, kernel To improve reading the code this patch moves the cases to start_isoc or return the function under one common condition check. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/dwc3/gadget.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 183b90923f51ba..0ed837323f6bd3 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1913,13 +1913,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) * errors which will force us issue EndTransfer command. */ if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { - if (!(dep->flags & DWC3_EP_PENDING_REQUEST) && - !(dep->flags & DWC3_EP_TRANSFER_STARTED)) - return 0; - - if ((dep->flags & DWC3_EP_PENDING_REQUEST)) { - if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) + if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) { + if ((dep->flags & DWC3_EP_PENDING_REQUEST)) return __dwc3_gadget_start_isoc(dep); + + return 0; } } -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition 2022-02-28 15:08 ` [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition Michael Grzeschik @ 2022-03-02 1:24 ` Thinh Nguyen 0 siblings, 0 replies; 12+ messages in thread From: Thinh Nguyen @ 2022-03-02 1:24 UTC (permalink / raw) To: Michael Grzeschik, linux-usb; +Cc: balbi, gregkh, kernel Michael Grzeschik wrote: > To improve reading the code this patch moves the cases to start_isoc or > return the function under one common condition check. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/dwc3/gadget.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 183b90923f51ba..0ed837323f6bd3 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1913,13 +1913,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > * errors which will force us issue EndTransfer command. > */ > if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { > - if (!(dep->flags & DWC3_EP_PENDING_REQUEST) && > - !(dep->flags & DWC3_EP_TRANSFER_STARTED)) > - return 0; > - > - if ((dep->flags & DWC3_EP_PENDING_REQUEST)) { > - if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) > + if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) { > + if ((dep->flags & DWC3_EP_PENDING_REQUEST)) > return __dwc3_gadget_start_isoc(dep); > + > + return 0; > } > } > This looks much better. Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks! Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function 2022-02-28 15:08 [PATCH 0/3] usb: dwc3: gadget: simple refactoring patches Michael Grzeschik 2022-02-28 15:08 ` [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition Michael Grzeschik @ 2022-02-28 15:08 ` Michael Grzeschik 2022-03-02 1:22 ` Thinh Nguyen 2022-02-28 15:08 ` [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps Michael Grzeschik 2 siblings, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2022-02-28 15:08 UTC (permalink / raw) To: linux-usb; +Cc: balbi, gregkh, kernel This patch adds the extra function dwc3_end_transfer to consolidate the same codepath. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/dwc3/gadget.c | 56 +++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 0ed837323f6bd3..b89dadaef4db9d 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1788,6 +1788,27 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) return __dwc3_gadget_kick_transfer(dep); } +static void dwc3_end_transfer(struct dwc3_ep *dep, bool force, bool interrupt) +{ + struct dwc3_gadget_ep_cmd_params params; + u32 cmd; + int ret; + + cmd = DWC3_DEPCMD_ENDTRANSFER; + cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; + cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; + cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); + memset(¶ms, 0, sizeof(params)); + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); + WARN_ON_ONCE(ret); + dep->resource_index = 0; + + if (!interrupt) + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; + else if (!ret) + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; +} + static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) { const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; @@ -1842,21 +1863,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) * status, issue END_TRANSFER command and retry on the next XferNotReady * event. */ - if (ret == -EAGAIN) { - struct dwc3_gadget_ep_cmd_params params; - u32 cmd; - - cmd = DWC3_DEPCMD_ENDTRANSFER | - DWC3_DEPCMD_CMDIOC | - DWC3_DEPCMD_PARAM(dep->resource_index); - - dep->resource_index = 0; - memset(¶ms, 0, sizeof(params)); - - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - if (!ret) - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; - } + if (ret == -EAGAIN) + dwc3_end_transfer(dep, false, true); return ret; } @@ -3597,10 +3605,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) { - struct dwc3_gadget_ep_cmd_params params; - u32 cmd; - int ret; - if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) || (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; @@ -3632,19 +3636,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, * This mode is NOT available on the DWC_usb31 IP. */ - cmd = DWC3_DEPCMD_ENDTRANSFER; - cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; - cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; - cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); - memset(¶ms, 0, sizeof(params)); - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - WARN_ON_ONCE(ret); - dep->resource_index = 0; - - if (!interrupt) - dep->flags &= ~DWC3_EP_TRANSFER_STARTED; - else - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + dwc3_end_transfer(dep, force, interrupt); } static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function 2022-02-28 15:08 ` [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function Michael Grzeschik @ 2022-03-02 1:22 ` Thinh Nguyen 2022-03-03 13:11 ` [PATCH v2] " Michael Grzeschik 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2022-03-02 1:22 UTC (permalink / raw) To: Michael Grzeschik, linux-usb; +Cc: balbi, gregkh, kernel Hi, Michael Grzeschik wrote: > This patch adds the extra function dwc3_end_transfer to consolidate the > same codepath. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/dwc3/gadget.c | 56 +++++++++++++++++---------------------- > 1 file changed, 24 insertions(+), 32 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 0ed837323f6bd3..b89dadaef4db9d 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1788,6 +1788,27 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) > return __dwc3_gadget_kick_transfer(dep); > } > > +static void dwc3_end_transfer(struct dwc3_ep *dep, bool force, bool interrupt) Maybe we can name this as __dwc3_stop_active_transfer() to be a bit more consistent in other places of this driver? Place this function above dwc3_gadget_start_isoc_quirk(). Also, can we write a brief doc describing this function here as well? I got a feeling that not many know what setting "force" mean to the controller. If ForceRM is set, then the controller won't update the TRB progress on command completion or clearing HWO bit. It doesn't mean that the command will complete immediately. Thanks, Thinh > +{ > + struct dwc3_gadget_ep_cmd_params params; > + u32 cmd; > + int ret; > + > + cmd = DWC3_DEPCMD_ENDTRANSFER; > + cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; > + cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; > + cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); > + memset(¶ms, 0, sizeof(params)); > + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > + WARN_ON_ONCE(ret); > + dep->resource_index = 0; > + > + if (!interrupt) > + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; > + else if (!ret) > + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > +} > + > static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > { > const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; > @@ -1842,21 +1863,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > * status, issue END_TRANSFER command and retry on the next XferNotReady > * event. > */ > - if (ret == -EAGAIN) { > - struct dwc3_gadget_ep_cmd_params params; > - u32 cmd; > - > - cmd = DWC3_DEPCMD_ENDTRANSFER | > - DWC3_DEPCMD_CMDIOC | > - DWC3_DEPCMD_PARAM(dep->resource_index); > - > - dep->resource_index = 0; > - memset(¶ms, 0, sizeof(params)); > - > - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > - if (!ret) > - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > - } > + if (ret == -EAGAIN) > + dwc3_end_transfer(dep, false, true); > > return ret; > } > @@ -3597,10 +3605,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) > static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, > bool interrupt) > { > - struct dwc3_gadget_ep_cmd_params params; > - u32 cmd; > - int ret; > - > if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) || > (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) > return; > @@ -3632,19 +3636,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, > * This mode is NOT available on the DWC_usb31 IP. > */ > > - cmd = DWC3_DEPCMD_ENDTRANSFER; > - cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; > - cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; > - cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); > - memset(¶ms, 0, sizeof(params)); > - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > - WARN_ON_ONCE(ret); > - dep->resource_index = 0; > - > - if (!interrupt) > - dep->flags &= ~DWC3_EP_TRANSFER_STARTED; > - else > - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > + dwc3_end_transfer(dep, force, interrupt); > } > > static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] usb: dwc3: gadget: move cmd_endtransfer to extra function 2022-03-02 1:22 ` Thinh Nguyen @ 2022-03-03 13:11 ` Michael Grzeschik 2022-03-04 2:26 ` Thinh Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2022-03-03 13:11 UTC (permalink / raw) To: linux-usb; +Cc: balbi, gregkh, kernel This patch adds the extra function __dwc3_stop_active_transfer to consolidate the same codepath. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/dwc3/gadget.c | 68 +++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 37dbf6132731b7..baddf6196ceee2 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1673,6 +1673,39 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) return DWC3_DSTS_SOFFN(reg); } +/** + * __dwc3_stop_active_transfer - stop the current active transfer + * @dep: isoc endpoint + * @force: set forcerm bit in the command + * @interrupt: command complete interrupt after End Transfer command + * + * When setting force, the ForceRM bit will be set. In that case + * the controller won't update the TRB progress on command + * completion. It also won't clear the HWO bit in the TRB. + * The command will also not complete immediatly in that case. + */ + +static void __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) +{ + struct dwc3_gadget_ep_cmd_params params; + u32 cmd; + int ret; + + cmd = DWC3_DEPCMD_ENDTRANSFER; + cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; + cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; + cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); + memset(¶ms, 0, sizeof(params)); + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); + WARN_ON_ONCE(ret); + dep->resource_index = 0; + + if (!interrupt) + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; + else if (!ret) + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; +} + /** * dwc3_gadget_start_isoc_quirk - workaround invalid frame number * @dep: isoc endpoint @@ -1842,21 +1875,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) * status, issue END_TRANSFER command and retry on the next XferNotReady * event. */ - if (ret == -EAGAIN) { - struct dwc3_gadget_ep_cmd_params params; - u32 cmd; - - cmd = DWC3_DEPCMD_ENDTRANSFER | - DWC3_DEPCMD_CMDIOC | - DWC3_DEPCMD_PARAM(dep->resource_index); - - dep->resource_index = 0; - memset(¶ms, 0, sizeof(params)); - - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - if (!ret) - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; - } + if (ret == -EAGAIN) + __dwc3_stop_active_transfer(dep, false, true); return ret; } @@ -3597,10 +3617,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) { - struct dwc3_gadget_ep_cmd_params params; - u32 cmd; - int ret; - if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) || (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; @@ -3632,19 +3648,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, * This mode is NOT available on the DWC_usb31 IP. */ - cmd = DWC3_DEPCMD_ENDTRANSFER; - cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; - cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; - cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); - memset(¶ms, 0, sizeof(params)); - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - WARN_ON_ONCE(ret); - dep->resource_index = 0; - - if (!interrupt) - dep->flags &= ~DWC3_EP_TRANSFER_STARTED; - else - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + __dwc3_stop_active_transfer(dep, force, interrupt); } static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] usb: dwc3: gadget: move cmd_endtransfer to extra function 2022-03-03 13:11 ` [PATCH v2] " Michael Grzeschik @ 2022-03-04 2:26 ` Thinh Nguyen 2022-03-05 0:53 ` [PATCH v3] " Michael Grzeschik 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2022-03-04 2:26 UTC (permalink / raw) To: Michael Grzeschik, linux-usb; +Cc: balbi, gregkh, kernel Hi, Michael Grzeschik wrote: > This patch adds the extra function __dwc3_stop_active_transfer to > consolidate the same codepath. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- Add a comment here on what's changed since previous version. > drivers/usb/dwc3/gadget.c | 68 +++++++++++++++++++++------------------ > 1 file changed, 36 insertions(+), 32 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 37dbf6132731b7..baddf6196ceee2 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1673,6 +1673,39 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) > return DWC3_DSTS_SOFFN(reg); > } > > +/** > + * __dwc3_stop_active_transfer - stop the current active transfer > + * @dep: isoc endpoint > + * @force: set forcerm bit in the command > + * @interrupt: command complete interrupt after End Transfer command > + * > + * When setting force, the ForceRM bit will be set. In that case > + * the controller won't update the TRB progress on command > + * completion. It also won't clear the HWO bit in the TRB. > + * The command will also not complete immediatly in that case. "immediately" > + */ Remove blank line > + > +static void __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) Can we make this return "int"? > +{ > + struct dwc3_gadget_ep_cmd_params params; > + u32 cmd; > + int ret; > + > + cmd = DWC3_DEPCMD_ENDTRANSFER; > + cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; > + cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; > + cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); > + memset(¶ms, 0, sizeof(params)); > + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > + WARN_ON_ONCE(ret); > + dep->resource_index = 0; > + > + if (!interrupt) > + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; > + else if (!ret) > + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; So we can return ret here. > +} > + > /** > * dwc3_gadget_start_isoc_quirk - workaround invalid frame number > * @dep: isoc endpoint > @@ -1842,21 +1875,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > * status, issue END_TRANSFER command and retry on the next XferNotReady > * event. > */ > - if (ret == -EAGAIN) { > - struct dwc3_gadget_ep_cmd_params params; > - u32 cmd; > - > - cmd = DWC3_DEPCMD_ENDTRANSFER | > - DWC3_DEPCMD_CMDIOC | > - DWC3_DEPCMD_PARAM(dep->resource_index); > - > - dep->resource_index = 0; > - memset(¶ms, 0, sizeof(params)); > - > - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > - if (!ret) > - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > - } > + if (ret == -EAGAIN) > + __dwc3_stop_active_transfer(dep, false, true); if (ret == -EAGAIN) ret = __dwc3_stop_active_transfer(dep, false, true); > > return ret; > } > @@ -3597,10 +3617,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) > static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, > bool interrupt) > { > - struct dwc3_gadget_ep_cmd_params params; > - u32 cmd; > - int ret; > - > if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) || > (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) > return; > @@ -3632,19 +3648,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, > * This mode is NOT available on the DWC_usb31 IP. > */ > > - cmd = DWC3_DEPCMD_ENDTRANSFER; > - cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; > - cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; > - cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); > - memset(¶ms, 0, sizeof(params)); > - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > - WARN_ON_ONCE(ret); > - dep->resource_index = 0; > - > - if (!interrupt) > - dep->flags &= ~DWC3_EP_TRANSFER_STARTED; > - else > - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > + __dwc3_stop_active_transfer(dep, force, interrupt); > } > > static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) Thanks, Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] usb: dwc3: gadget: move cmd_endtransfer to extra function 2022-03-04 2:26 ` Thinh Nguyen @ 2022-03-05 0:53 ` Michael Grzeschik 2022-03-06 13:57 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2022-03-05 0:53 UTC (permalink / raw) To: linux-usb; +Cc: balbi, gregkh, kernel This patch adds the extra function __dwc3_stop_active_transfer to consolidate the same codepath. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v1 -> v2: - renamed the function to __dwc3_stop_active_transfer - added function description v2 -> v3: - fixed spelling and removed extra line - make __dwc3_stop_active_transfer return ret - use ret in __dwc3_gadget_start_isoc --- drivers/usb/dwc3/gadget.c | 69 +++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 32 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 37dbf6132731b7..bd9d8494c90b73 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1673,6 +1673,40 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) return DWC3_DSTS_SOFFN(reg); } +/** + * __dwc3_stop_active_transfer - stop the current active transfer + * @dep: isoc endpoint + * @force: set forcerm bit in the command + * @interrupt: command complete interrupt after End Transfer command + * + * When setting force, the ForceRM bit will be set. In that case + * the controller won't update the TRB progress on command + * completion. It also won't clear the HWO bit in the TRB. + * The command will also not complete immediately in that case. + */ +static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) +{ + struct dwc3_gadget_ep_cmd_params params; + u32 cmd; + int ret; + + cmd = DWC3_DEPCMD_ENDTRANSFER; + cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; + cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; + cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); + memset(¶ms, 0, sizeof(params)); + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); + WARN_ON_ONCE(ret); + dep->resource_index = 0; + + if (!interrupt) + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; + else if (!ret) + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + + return ret; +} + /** * dwc3_gadget_start_isoc_quirk - workaround invalid frame number * @dep: isoc endpoint @@ -1842,21 +1876,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) * status, issue END_TRANSFER command and retry on the next XferNotReady * event. */ - if (ret == -EAGAIN) { - struct dwc3_gadget_ep_cmd_params params; - u32 cmd; - - cmd = DWC3_DEPCMD_ENDTRANSFER | - DWC3_DEPCMD_CMDIOC | - DWC3_DEPCMD_PARAM(dep->resource_index); - - dep->resource_index = 0; - memset(¶ms, 0, sizeof(params)); - - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - if (!ret) - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; - } + if (ret == -EAGAIN) + ret = __dwc3_stop_active_transfer(dep, false, true); return ret; } @@ -3597,10 +3618,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) { - struct dwc3_gadget_ep_cmd_params params; - u32 cmd; - int ret; - if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) || (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; @@ -3632,19 +3649,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, * This mode is NOT available on the DWC_usb31 IP. */ - cmd = DWC3_DEPCMD_ENDTRANSFER; - cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0; - cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0; - cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); - memset(¶ms, 0, sizeof(params)); - ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - WARN_ON_ONCE(ret); - dep->resource_index = 0; - - if (!interrupt) - dep->flags &= ~DWC3_EP_TRANSFER_STARTED; - else - dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + __dwc3_stop_active_transfer(dep, force, interrupt); } static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] usb: dwc3: gadget: move cmd_endtransfer to extra function 2022-03-05 0:53 ` [PATCH v3] " Michael Grzeschik @ 2022-03-06 13:57 ` Greg KH 0 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2022-03-06 13:57 UTC (permalink / raw) To: Michael Grzeschik; +Cc: linux-usb, balbi, kernel On Sat, Mar 05, 2022 at 01:53:56AM +0100, Michael Grzeschik wrote: > This patch adds the extra function __dwc3_stop_active_transfer to > consolidate the same codepath. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > v1 -> v2: - renamed the function to __dwc3_stop_active_transfer > - added function description > v2 -> v3: - fixed spelling and removed extra line > - make __dwc3_stop_active_transfer return ret > - use ret in __dwc3_gadget_start_isoc So is this v3 of a single patch in this series, or the whole series? I'm totally confused, please resend the whole, updated series, as v4 please. thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps 2022-02-28 15:08 [PATCH 0/3] usb: dwc3: gadget: simple refactoring patches Michael Grzeschik 2022-02-28 15:08 ` [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition Michael Grzeschik 2022-02-28 15:08 ` [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function Michael Grzeschik @ 2022-02-28 15:08 ` Michael Grzeschik 2022-03-01 1:12 ` Thinh Nguyen 2 siblings, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2022-02-28 15:08 UTC (permalink / raw) To: linux-usb; +Cc: balbi, gregkh, kernel Refactor the codepath for handling DWC3_EP_DELAY_START condition only being checked on non isoc endpoints. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/dwc3/gadget.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index b89dadaef4db9d..d09bd66f498a69 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1901,17 +1901,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE) return 0; - /* - * Start the transfer only after the END_TRANSFER is completed - * and endpoint STALL is cleared. - */ - if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || - (dep->flags & DWC3_EP_WEDGE) || - (dep->flags & DWC3_EP_STALL)) { - dep->flags |= DWC3_EP_DELAY_START; - return 0; - } - /* * NOTICE: Isochronous endpoints should NEVER be prestarted. We must * wait for a XferNotReady event so we will know what's the current @@ -1927,6 +1916,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) return 0; } + } else { + /* + * Start the transfer only after the END_TRANSFER is completed + * and endpoint STALL is cleared. + */ + if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || + (dep->flags & DWC3_EP_WEDGE) || + (dep->flags & DWC3_EP_STALL)) { + dep->flags |= DWC3_EP_DELAY_START; + return 0; + } } __dwc3_gadget_kick_transfer(dep); -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps 2022-02-28 15:08 ` [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps Michael Grzeschik @ 2022-03-01 1:12 ` Thinh Nguyen 2022-03-01 21:06 ` Michael Grzeschik 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2022-03-01 1:12 UTC (permalink / raw) To: Michael Grzeschik, linux-usb; +Cc: balbi, gregkh, kernel Hi, Michael Grzeschik wrote: > Refactor the codepath for handling DWC3_EP_DELAY_START condition > only being checked on non isoc endpoints. The DWC3_EP_DELAY_START should still be applicable to isoc and End Transfer pending. While the End Transfer command is active, don't issue Start Transfer command. Previously I think we have a check for the isoc endpoint and DWC3_EP_DELAY_START because it was intended to check against the halt condition, but it was done incorrectly. (Note that isoc endpoint doesn't halt and there's no STALL handshake). This change should not be applied. If we're to apply the fix for isoc and delay start check, it should be done separately. Thanks, Thinh > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/dwc3/gadget.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index b89dadaef4db9d..d09bd66f498a69 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1901,17 +1901,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE) > return 0; > > - /* > - * Start the transfer only after the END_TRANSFER is completed > - * and endpoint STALL is cleared. > - */ > - if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || > - (dep->flags & DWC3_EP_WEDGE) || > - (dep->flags & DWC3_EP_STALL)) { > - dep->flags |= DWC3_EP_DELAY_START; > - return 0; > - } > - > /* > * NOTICE: Isochronous endpoints should NEVER be prestarted. We must > * wait for a XferNotReady event so we will know what's the current > @@ -1927,6 +1916,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > > return 0; > } > + } else { > + /* > + * Start the transfer only after the END_TRANSFER is completed > + * and endpoint STALL is cleared. > + */ > + if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || > + (dep->flags & DWC3_EP_WEDGE) || > + (dep->flags & DWC3_EP_STALL)) { > + dep->flags |= DWC3_EP_DELAY_START; > + return 0; > + } > } > > __dwc3_gadget_kick_transfer(dep); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps 2022-03-01 1:12 ` Thinh Nguyen @ 2022-03-01 21:06 ` Michael Grzeschik 0 siblings, 0 replies; 12+ messages in thread From: Michael Grzeschik @ 2022-03-01 21:06 UTC (permalink / raw) To: Thinh Nguyen; +Cc: linux-usb, balbi, gregkh, kernel [-- Attachment #1: Type: text/plain, Size: 3071 bytes --] On Tue, Mar 01, 2022 at 01:12:37AM +0000, Thinh Nguyen wrote: >Hi, > >Michael Grzeschik wrote: >> Refactor the codepath for handling DWC3_EP_DELAY_START condition >> only being checked on non isoc endpoints. > >The DWC3_EP_DELAY_START should still be applicable to isoc and End >Transfer pending. While the End Transfer command is active, don't issue >Start Transfer command. > >Previously I think we have a check for the isoc endpoint and >DWC3_EP_DELAY_START because it was intended to check against the halt >condition, but it was done incorrectly. (Note that isoc endpoint doesn't >halt and there's no STALL handshake). > >This change should not be applied. If we're to apply the fix for isoc >and delay start check, it should be done separately. Right! I just realized that we indeed at least also have to check for DWC3_EP_END_TRANSFER_PENDING flag on isoc transfers. Without that, we would open up a race where we kick(update) transfers, that are potentially to late for that current transfer. So yes, please drop that patch. The other patches on the other hand are fine I think. Regards, Michael >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> --- >> drivers/usb/dwc3/gadget.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index b89dadaef4db9d..d09bd66f498a69 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1901,17 +1901,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE) >> return 0; >> >> - /* >> - * Start the transfer only after the END_TRANSFER is completed >> - * and endpoint STALL is cleared. >> - */ >> - if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || >> - (dep->flags & DWC3_EP_WEDGE) || >> - (dep->flags & DWC3_EP_STALL)) { >> - dep->flags |= DWC3_EP_DELAY_START; >> - return 0; >> - } >> - >> /* >> * NOTICE: Isochronous endpoints should NEVER be prestarted. We must >> * wait for a XferNotReady event so we will know what's the current >> @@ -1927,6 +1916,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> >> return 0; >> } >> + } else { >> + /* >> + * Start the transfer only after the END_TRANSFER is completed >> + * and endpoint STALL is cleared. >> + */ >> + if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || >> + (dep->flags & DWC3_EP_WEDGE) || >> + (dep->flags & DWC3_EP_STALL)) { >> + dep->flags |= DWC3_EP_DELAY_START; >> + return 0; >> + } >> } >> >> __dwc3_gadget_kick_transfer(dep); > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-03-06 13:57 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-28 15:08 [PATCH 0/3] usb: dwc3: gadget: simple refactoring patches Michael Grzeschik 2022-02-28 15:08 ` [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition Michael Grzeschik 2022-03-02 1:24 ` Thinh Nguyen 2022-02-28 15:08 ` [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function Michael Grzeschik 2022-03-02 1:22 ` Thinh Nguyen 2022-03-03 13:11 ` [PATCH v2] " Michael Grzeschik 2022-03-04 2:26 ` Thinh Nguyen 2022-03-05 0:53 ` [PATCH v3] " Michael Grzeschik 2022-03-06 13:57 ` Greg KH 2022-02-28 15:08 ` [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps Michael Grzeschik 2022-03-01 1:12 ` Thinh Nguyen 2022-03-01 21:06 ` Michael Grzeschik
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.