diff mbox series

Revert "usb: dwc3: Support EBC feature of DWC_usb31"

Message ID 3042f847ff904b4dd4e4cf66a1b9df470e63439e.1707441690.git.Thinh.Nguyen@synopsys.com
State New
Headers show
Series Revert "usb: dwc3: Support EBC feature of DWC_usb31" | expand

Commit Message

Thinh Nguyen Feb. 9, 2024, 1:24 a.m. UTC 
This reverts commit 398aa9a7e77cf23c2a6f882ddd3dcd96f21771dc.

The update to the gadget API to support EBC feature is incomplete. It's
missing at least the following:
 * New usage documentation
 * Gadget capability check
 * Condition for the user to check how many and which endpoints can be
   used as "fifo_mode"
 * Description of how it can affect completed request (e.g. dwc3 won't
   update TRB on completion -- ie. how it can affect request's actual
   length report)

Let's revert this until it's ready.

Fixes: 398aa9a7e77c ("usb: dwc3: Support EBC feature of DWC_usb31")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/core.h    | 1 -
 drivers/usb/dwc3/gadget.c  | 6 ------
 drivers/usb/dwc3/gadget.h  | 2 --
 include/linux/usb/gadget.h | 1 -
 4 files changed, 10 deletions(-)


base-commit: 88bae831f3810e02c9c951233c7ee662aa13dc2c

Comments

Manan Aurora April 17, 2024, 11:49 a.m. UTC | #1 
On Thu, Apr 11, 2024 at 5:52 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
>
> On Wed, Apr 10, 2024, Manan Aurora wrote:
> > Hi Thinh,
> >
> > I'm working on a patch to bring EBC support back, I had a doubt
> > regarding some of the required corrections though (inlined)
> >
> > Please take a look and advise, I'll proceed accordingly.
> >
>
> >
> > On Fri, Feb 9, 2024 at 6:55 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > >
> > > This reverts commit 398aa9a7e77cf23c2a6f882ddd3dcd96f21771dc.
> > >
> > > The update to the gadget API to support EBC feature is incomplete. It's
> > > missing at least the following:
> > >  * New usage documentation
> > I will address this
> > >  * Gadget capability check
> > >  * Condition for the user to check how many and which endpoints can be
> > >    used as "fifo_mode"
>
> > The easiest option seems to be to add a new function that lets users
> > specifically request
> > fifo_mode endpoints eg: usb_fifo_mode_ep_autoconfig_ss
> > This function will cover ensuring that the device supports
> > fifo_endpoints and returning a suitable
> > endpoint (if available) and NULL otherwise. This can be indicated by
> > adding a new bit to
> > the existing ep_caps  structure.
> > Does this seem like an acceptable solution?
>
> That sounds fine to me. For the naming, perhaps just name it as EBC for
> External Buffer Control and provide proper description in documentation.
> "fifo_mode" may not be clear.
>
> Maybe usb_ep_autoconfig_ss_with_ebc()?
>
> How are you planning to implement the above function? Are you going to
> apply the DWC_usb3x specific requirements directly or implement
> gadget_ops->match_ep() to properly return the right endpoint base on the
> endpoint desc? As you're aware, EBC is only applicable to non-streams
> bulk only. Also it doesn't apply to full-speed.
The issue with implementing match_ep is that the API doesn't let us
specify that
an EBC endpoint is desired. What about adding a new function to usb_gadget_ops?
Then we could apply IP-specific restrictions in one place.

Speed can be enforced when we attempt to configure the EP
(config_ep_by_speed etc)

>
> >
> > >  * Description of how it can affect completed request (e.g. dwc3 won't
> > >    update TRB on completion -- ie. how it can affect request's actual
> > >    length report)
> > I will remove the NO_WB bit for the EBC endpoint and leave it up to
> > the user to enable/disable this
>
> Thanks,
> Thinh
Thinh Nguyen April 17, 2024, 9:10 p.m. UTC | #2 
On Wed, Apr 17, 2024, Manan Aurora wrote:
> On Thu, Apr 11, 2024 at 5:52 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> >
> > On Wed, Apr 10, 2024, Manan Aurora wrote:
> > > Hi Thinh,
> > >
> > > I'm working on a patch to bring EBC support back, I had a doubt
> > > regarding some of the required corrections though (inlined)
> > >
> > > Please take a look and advise, I'll proceed accordingly.
> > >
> >
> > >
> > > On Fri, Feb 9, 2024 at 6:55 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > > >
> > > > This reverts commit 398aa9a7e77cf23c2a6f882ddd3dcd96f21771dc.
> > > >
> > > > The update to the gadget API to support EBC feature is incomplete. It's
> > > > missing at least the following:
> > > >  * New usage documentation
> > > I will address this
> > > >  * Gadget capability check
> > > >  * Condition for the user to check how many and which endpoints can be
> > > >    used as "fifo_mode"
> >
> > > The easiest option seems to be to add a new function that lets users
> > > specifically request
> > > fifo_mode endpoints eg: usb_fifo_mode_ep_autoconfig_ss
> > > This function will cover ensuring that the device supports
> > > fifo_endpoints and returning a suitable
> > > endpoint (if available) and NULL otherwise. This can be indicated by
> > > adding a new bit to
> > > the existing ep_caps  structure.
> > > Does this seem like an acceptable solution?
> >
> > That sounds fine to me. For the naming, perhaps just name it as EBC for
> > External Buffer Control and provide proper description in documentation.
> > "fifo_mode" may not be clear.
> >
> > Maybe usb_ep_autoconfig_ss_with_ebc()?
> >
> > How are you planning to implement the above function? Are you going to
> > apply the DWC_usb3x specific requirements directly or implement
> > gadget_ops->match_ep() to properly return the right endpoint base on the
> > endpoint desc? As you're aware, EBC is only applicable to non-streams
> > bulk only. Also it doesn't apply to full-speed.

> The issue with implementing match_ep is that the API doesn't let us
> specify that

But I thought you'd add a new bit to ep_caps or the equivalent? The
gadget driver can check that. Just make sure that the dwc3 driver gets
that info somewhere to prepare the ep_caps (perhaps through device tree
binding property?).

> an EBC endpoint is desired. What about adding a new function to usb_gadget_ops?
> Then we could apply IP-specific restrictions in one place.
> 
> Speed can be enforced when we attempt to configure the EP
> (config_ep_by_speed etc)
> 

Sounds good.

Thanks,
Thinh
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index df544ec730d2..2255fc94c8ef 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -376,7 +376,6 @@ 
 /* Global HWPARAMS4 Register */
 #define DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(n)	(((n) & (0x0f << 13)) >> 13)
 #define DWC3_MAX_HIBER_SCRATCHBUFS		15
-#define DWC3_EXT_BUFF_CONTROL		BIT(21)
 
 /* Global HWPARAMS6 Register */
 #define DWC3_GHWPARAMS6_BCSUPPORT		BIT(14)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 564976b3e2b9..4c8dd6724678 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -673,12 +673,6 @@  static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 		params.param1 |= DWC3_DEPCFG_BINTERVAL_M1(bInterval_m1);
 	}
 
-	if (dep->endpoint.fifo_mode) {
-		if (!(dwc->hwparams.hwparams4 & DWC3_EXT_BUFF_CONTROL))
-			return -EINVAL;
-		params.param1 |= DWC3_DEPCFG_EBC_HWO_NOWB | DWC3_DEPCFG_USE_EBC;
-	}
-
 	return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, &params);
 }
 
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index fd7a4e94397e..55a56cf67d73 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -26,8 +26,6 @@  struct dwc3;
 #define DWC3_DEPCFG_XFER_NOT_READY_EN	BIT(10)
 #define DWC3_DEPCFG_FIFO_ERROR_EN	BIT(11)
 #define DWC3_DEPCFG_STREAM_EVENT_EN	BIT(13)
-#define DWC3_DEPCFG_EBC_HWO_NOWB	BIT(14)
-#define DWC3_DEPCFG_USE_EBC		BIT(15)
 #define DWC3_DEPCFG_BINTERVAL_M1(n)	(((n) & 0xff) << 16)
 #define DWC3_DEPCFG_STREAM_CAPABLE	BIT(24)
 #define DWC3_DEPCFG_EP_NUMBER(n)	(((n) & 0x1f) << 25)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index a771ccc038ac..6532beb587b1 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -236,7 +236,6 @@  struct usb_ep {
 	unsigned		max_streams:16;
 	unsigned		mult:2;
 	unsigned		maxburst:5;
-	unsigned		fifo_mode:1;
 	u8			address;
 	const struct usb_endpoint_descriptor	*desc;
 	const struct usb_ss_ep_comp_descriptor	*comp_desc;