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

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 <[e-mail]>
---
 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 10, 2024, 7:26 a.m. UTC | #1

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.

Regards,
Manan

On Fri, Feb 9, 2024 at 6:55 AM Thinh Nguyen <[e-mail]> 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?

>  * 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
>
> Let's revert this until it's ready.
>
> Fixes: 398aa9a7e77c ("usb: dwc3: Support EBC feature of DWC_usb31")
> Signed-off-by: Thinh Nguyen <[e-mail]>
> ---
>  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(-)
>
> 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;
>
> base-commit: 88bae831f3810e02c9c951233c7ee662aa13dc2c
> --
> 2.28.0

Thinh Nguyen April 11, 2024, 12:22 a.m. UTC | #2

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 <[e-mail]> 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.

> 
> >  * 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

Manan Aurora April 17, 2024, 11:49 a.m. UTC | #3

On Thu, Apr 11, 2024 at 5:52 AM Thinh Nguyen <[e-mail]> 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 <[e-mail]> 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 | #4

On Wed, Apr 17, 2024, Manan Aurora wrote:
> On Thu, Apr 11, 2024 at 5:52 AM Thinh Nguyen <[e-mail]> 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 <[e-mail]> 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

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;