Support configuration and use of bulk endpoints in the so-called EBC
mode described in the DBC_usb31 databook (appendix E)
Added a bit fifo_mode to usb_ep to indicate to the UDC driver that a
specific endpoint is to operate in the EBC (or equivalent) mode when
enabled
Added macros for bits 15 and 14 of DEPCFG parameter 1 to indicate EBC
mode and write back behaviour. These bits will be set to 1 when
configuring an EBC endpoint as described in the programming guide
Signed-off-by: Manan Aurora <[email protected]>
---
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 6 ++++++
drivers/usb/dwc3/gadget.h | 2 ++
include/linux/usb/gadget.h | 4 ++++
4 files changed, 13 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index efe6caf4d0e8..c5b578188cd3 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -374,6 +374,7 @@
/* 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 858fe4c299b7..47d2737d528b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -673,6 +673,12 @@ 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, ¶ms);
}
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 55a56cf67d73..fd7a4e94397e 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -26,6 +26,8 @@ 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 6532beb587b1..c526ae09bcee 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -209,6 +209,9 @@ struct usb_ep_caps {
* by this EP (0 - 16, actual number is 2^n)
* @mult: multiplier, 'mult' value for SS Isoc EPs
* @maxburst: the maximum number of bursts supported by this EP (for usb3)
+ * @fifo_mode: indicates that the control of this EP is handed off to an
+ * hardware fifo device. Depends on hardware support eg. EBC feature
+ * of DWC usb3.1 device or equivalent. Set before enabling the EP
* @driver_data:for use by the gadget driver.
* @address: used to identify the endpoint when finding descriptor that
* matches connection speed
@@ -236,6 +239,7 @@ 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;
--
2.42.0.820.g83a721a137-goog
On Sat, Nov 11, 2023 at 4:39 AM Thinh Nguyen <[email protected]> wrote:
>
> On Thu, Nov 02, 2023, Manan Aurora wrote:
> > Support configuration and use of bulk endpoints in the so-called EBC
> > mode described in the DBC_usb31 databook (appendix E)
> >
> > Added a bit fifo_mode to usb_ep to indicate to the UDC driver that a
> > specific endpoint is to operate in the EBC (or equivalent) mode when
> > enabled
>
> This should be unique to dwc3, and it's only for bulk. I don't think
> usb_ep or the user of usb_ep should know this.
In our use case we have a function driver that configures an allocated bulk
endpoint to operate as an EBC EP. So the function driver already depends on the
feature.
dwc3_ep seems like the correct place to put this field but a function
driver that allocates
EPs and configures them for this use case would need to include dwc3 headers.
If other vendors offer an equivalent feature this dependency would
become an issue.
Exporting a symbol from dwc3 is an easy option but dwc3 doesn't
currently export symbols
hence I tried to avoid that
> Also since DWC3_DEPCFG_EBC_HWO_NOWB must be set, the controller does not
> write back to the TRB. Did you handle how the driver would update the
> usb request on completion? (e.g. how much was transferred).
In our use case, we intend to have a link TRB and issue a startXfer
command. Completion
handling and continuing the transfer will be offloaded to dedicated
FIFO hardware.
But we can definitely rework this to disable no-writeback mode by
default and allow this to
be separately enabled
>
> BR,
> Thinh
>
> >
> > Added macros for bits 15 and 14 of DEPCFG parameter 1 to indicate EBC
> > mode and write back behaviour. These bits will be set to 1 when
> > configuring an EBC endpoint as described in the programming guide
> >
> > Signed-off-by: Manan Aurora <[email protected]>
> > ---
> > drivers/usb/dwc3/core.h | 1 +
> > drivers/usb/dwc3/gadget.c | 6 ++++++
> > drivers/usb/dwc3/gadget.h | 2 ++
> > include/linux/usb/gadget.h | 4 ++++
> > 4 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index efe6caf4d0e8..c5b578188cd3 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -374,6 +374,7 @@
> > /* 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 858fe4c299b7..47d2737d528b 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -673,6 +673,12 @@ 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, ¶ms);
> > }
> >
> > diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> > index 55a56cf67d73..fd7a4e94397e 100644
> > --- a/drivers/usb/dwc3/gadget.h
> > +++ b/drivers/usb/dwc3/gadget.h
> > @@ -26,6 +26,8 @@ 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 6532beb587b1..c526ae09bcee 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -209,6 +209,9 @@ struct usb_ep_caps {
> > * by this EP (0 - 16, actual number is 2^n)
> > * @mult: multiplier, 'mult' value for SS Isoc EPs
> > * @maxburst: the maximum number of bursts supported by this EP (for usb3)
> > + * @fifo_mode: indicates that the control of this EP is handed off to an
> > + * hardware fifo device. Depends on hardware support eg. EBC feature
> > + * of DWC usb3.1 device or equivalent. Set before enabling the EP
> > * @driver_data:for use by the gadget driver.
> > * @address: used to identify the endpoint when finding descriptor that
> > * matches connection speed
> > @@ -236,6 +239,7 @@ 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;
> > --
> > 2.42.0.820.g83a721a137-goog
> >
On Thu, Nov 16, 2023, Manan Aurora wrote:
> On Sat, Nov 11, 2023 at 4:39 AM Thinh Nguyen <[email protected]> wrote:
> >
> > On Thu, Nov 02, 2023, Manan Aurora wrote:
> > > Support configuration and use of bulk endpoints in the so-called EBC
> > > mode described in the DBC_usb31 databook (appendix E)
> > >
> > > Added a bit fifo_mode to usb_ep to indicate to the UDC driver that a
> > > specific endpoint is to operate in the EBC (or equivalent) mode when
> > > enabled
> >
> > This should be unique to dwc3, and it's only for bulk. I don't think
> > usb_ep or the user of usb_ep should know this.
>
> In our use case we have a function driver that configures an allocated bulk
> endpoint to operate as an EBC EP. So the function driver already depends on the
> feature.
This should be abstracted from the function driver. The function driver
should not need to know about this feature.
>
> dwc3_ep seems like the correct place to put this field but a function
> driver that allocates
> EPs and configures them for this use case would need to include dwc3 headers.
> If other vendors offer an equivalent feature this dependency would
> become an issue.
>
> Exporting a symbol from dwc3 is an easy option but dwc3 doesn't
> currently export symbols
> hence I tried to avoid that
>
> > Also since DWC3_DEPCFG_EBC_HWO_NOWB must be set, the controller does not
> > write back to the TRB. Did you handle how the driver would update the
> > usb request on completion? (e.g. how much was transferred).
>
> In our use case, we intend to have a link TRB and issue a startXfer
> command. Completion
> handling and continuing the transfer will be offloaded to dedicated
> FIFO hardware.
> But we can definitely rework this to disable no-writeback mode by
> default and allow this to
> be separately enabled
>
Ok.
Thanks,
Thinh
Hi Greg/Manan,
On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> On Thu, Nov 16, 2023, Manan Aurora wrote:
> > On Sat, Nov 11, 2023 at 4:39 AM Thinh Nguyen <[email protected]> wrote:
> > >
> > > On Thu, Nov 02, 2023, Manan Aurora wrote:
> > > > Support configuration and use of bulk endpoints in the so-called EBC
> > > > mode described in the DBC_usb31 databook (appendix E)
> > > >
> > > > Added a bit fifo_mode to usb_ep to indicate to the UDC driver that a
> > > > specific endpoint is to operate in the EBC (or equivalent) mode when
> > > > enabled
> > >
> > > This should be unique to dwc3, and it's only for bulk. I don't think
> > > usb_ep or the user of usb_ep should know this.
> >
> > In our use case we have a function driver that configures an allocated bulk
> > endpoint to operate as an EBC EP. So the function driver already depends on the
> > feature.
>
> This should be abstracted from the function driver. The function driver
> should not need to know about this feature.
>
> >
> > dwc3_ep seems like the correct place to put this field but a function
> > driver that allocates
> > EPs and configures them for this use case would need to include dwc3 headers.
> > If other vendors offer an equivalent feature this dependency would
> > become an issue.
> >
> > Exporting a symbol from dwc3 is an easy option but dwc3 doesn't
> > currently export symbols
> > hence I tried to avoid that
> >
> > > Also since DWC3_DEPCFG_EBC_HWO_NOWB must be set, the controller does not
> > > write back to the TRB. Did you handle how the driver would update the
> > > usb request on completion? (e.g. how much was transferred).
> >
> > In our use case, we intend to have a link TRB and issue a startXfer
> > command. Completion
> > handling and continuing the transfer will be offloaded to dedicated
> > FIFO hardware.
> > But we can definitely rework this to disable no-writeback mode by
> > default and allow this to
> > be separately enabled
> >
>
> Ok.
>
Looks like this change was applied to Greg's branch. Unless I'm missing
something, I don't think my concerns are addressed yet. Here are the
reiteration of the concerns:
1) The gadget driver should not need to know the dwc3's FIFO mode. It's
specific to dwc3 capability and should be handled in dwc3 driver only.
Usually these properties are selected in device tree bindings and not
specified through the gadget driver API.
2) This specific mode "EBC" doesn't write back to TRBs. It's not clear
how this is handled when updating the request's status. It's also only
applicable to bulk endpoint. If it's to be applied to the usb gadget
API, it's not documented fully.
Thanks,
Thinh
Hi Manan,
On Wed, Jan 17, 2024, Manan Aurora wrote:
> Hi Thinh,
>
> I'm fine with reverting the change until it matches what the intended
> use case is. I've added some notes:
>
> On Wed, Jan 17, 2024 at 6:41 AM Thinh Nguyen <[email protected]> wrote:
> >
> > Hi Greg/Manan,
> >
> > On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> > > On Thu, Nov 16, 2023, Manan Aurora wrote:
> > > > On Sat, Nov 11, 2023 at 4:39 AM Thinh Nguyen <[email protected]> wrote:
> > > > >
> > > > > On Thu, Nov 02, 2023, Manan Aurora wrote:
> > > > > > Support configuration and use of bulk endpoints in the so-called EBC
> > > > > > mode described in the DBC_usb31 databook (appendix E)
> > > > > >
> > > > > > Added a bit fifo_mode to usb_ep to indicate to the UDC driver that a
> > > > > > specific endpoint is to operate in the EBC (or equivalent) mode when
> > > > > > enabled
> > > > >
> > > > > This should be unique to dwc3, and it's only for bulk. I don't think
> > > > > usb_ep or the user of usb_ep should know this.
> > > >
> > > > In our use case we have a function driver that configures an allocated bulk
> > > > endpoint to operate as an EBC EP. So the function driver already depends on the
> > > > feature.
> > >
> > > This should be abstracted from the function driver. The function driver
> > > should not need to know about this feature.
> > >
> > > >
> > > > dwc3_ep seems like the correct place to put this field but a function
> > > > driver that allocates
> > > > EPs and configures them for this use case would need to include dwc3 headers.
> > > > If other vendors offer an equivalent feature this dependency would
> > > > become an issue.
> > > >
> > > > Exporting a symbol from dwc3 is an easy option but dwc3 doesn't
> > > > currently export symbols
> > > > hence I tried to avoid that
> > > >
> > > > > Also since DWC3_DEPCFG_EBC_HWO_NOWB must be set, the controller does not
> > > > > write back to the TRB. Did you handle how the driver would update the
> > > > > usb request on completion? (e.g. how much was transferred).
> > > >
> > > > In our use case, we intend to have a link TRB and issue a startXfer
> > > > command. Completion
> > > > handling and continuing the transfer will be offloaded to dedicated
> > > > FIFO hardware.
> > > > But we can definitely rework this to disable no-writeback mode by
> > > > default and allow this to
> > > > be separately enabled
> > > >
> > >
> > > Ok.
> > >
> >
> > Looks like this change was applied to Greg's branch. Unless I'm missing
> > something, I don't think my concerns are addressed yet. Here are the
> > reiteration of the concerns:
> >
> > 1) The gadget driver should not need to know the dwc3's FIFO mode. It's
> > specific to dwc3 capability and should be handled in dwc3 driver only.
> > Usually these properties are selected in device tree bindings and not
> > specified through the gadget driver API.
>
> I'm not sure how this will work when we have multiple functions and only
> some of them use EBC.The other EPs are working as usual.
> In terms of DT binding I can think of forcing certain EPs into EBC mode
> and using them for any gadget that needs EBC but that will remove those
> EPs from circulation for other functions. It would be great if you could
> suggest a good alternative we can use.
Ok. If there are only specific endpoints should use this feature, then
we will need to update the gadget API to support that as you have here.
Please document how you intend to let the gadget driver know that the HW
is capable of external FIFO (ie. update usb_gadget structure), and for
how many endpoint. Also update any expected behavior when using this
feature.
>
> >
> > 2) This specific mode "EBC" doesn't write back to TRBs. It's not clear
> > how this is handled when updating the request's status. It's also only
> > applicable to bulk endpoint. If it's to be applied to the usb gadget
> > API, it's not documented fully.
>
> I will push a patch to remove the no-writeback bit based on the decision
> above.
>
Sure. We can keep what's already in Greg's. Please update the change as
discussed.
Thanks,
Thinh
On Tue, Jan 16, 2024, Thinh Nguyen wrote:
> Hi Manan,
>
> On Wed, Jan 17, 2024, Manan Aurora wrote:
> > Hi Thinh,
> >
> > I'm fine with reverting the change until it matches what the intended
> > use case is. I've added some notes:
> >
> > On Wed, Jan 17, 2024 at 6:41 AM Thinh Nguyen <[email protected]> wrote:
> > >
> > > Hi Greg/Manan,
> > >
> > > On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> > > > On Thu, Nov 16, 2023, Manan Aurora wrote:
> > > > > On Sat, Nov 11, 2023 at 4:39 AM Thinh Nguyen <[email protected]> wrote:
> > > > > >
> > > > > > On Thu, Nov 02, 2023, Manan Aurora wrote:
> > > > > > > Support configuration and use of bulk endpoints in the so-called EBC
> > > > > > > mode described in the DBC_usb31 databook (appendix E)
> > > > > > >
> > > > > > > Added a bit fifo_mode to usb_ep to indicate to the UDC driver that a
> > > > > > > specific endpoint is to operate in the EBC (or equivalent) mode when
> > > > > > > enabled
> > > > > >
> > > > > > This should be unique to dwc3, and it's only for bulk. I don't think
> > > > > > usb_ep or the user of usb_ep should know this.
> > > > >
> > > > > In our use case we have a function driver that configures an allocated bulk
> > > > > endpoint to operate as an EBC EP. So the function driver already depends on the
> > > > > feature.
> > > >
> > > > This should be abstracted from the function driver. The function driver
> > > > should not need to know about this feature.
> > > >
> > > > >
> > > > > dwc3_ep seems like the correct place to put this field but a function
> > > > > driver that allocates
> > > > > EPs and configures them for this use case would need to include dwc3 headers.
> > > > > If other vendors offer an equivalent feature this dependency would
> > > > > become an issue.
> > > > >
> > > > > Exporting a symbol from dwc3 is an easy option but dwc3 doesn't
> > > > > currently export symbols
> > > > > hence I tried to avoid that
> > > > >
> > > > > > Also since DWC3_DEPCFG_EBC_HWO_NOWB must be set, the controller does not
> > > > > > write back to the TRB. Did you handle how the driver would update the
> > > > > > usb request on completion? (e.g. how much was transferred).
> > > > >
> > > > > In our use case, we intend to have a link TRB and issue a startXfer
> > > > > command. Completion
> > > > > handling and continuing the transfer will be offloaded to dedicated
> > > > > FIFO hardware.
> > > > > But we can definitely rework this to disable no-writeback mode by
> > > > > default and allow this to
> > > > > be separately enabled
> > > > >
> > > >
> > > > Ok.
> > > >
> > >
> > > Looks like this change was applied to Greg's branch. Unless I'm missing
> > > something, I don't think my concerns are addressed yet. Here are the
> > > reiteration of the concerns:
> > >
> > > 1) The gadget driver should not need to know the dwc3's FIFO mode. It's
> > > specific to dwc3 capability and should be handled in dwc3 driver only.
> > > Usually these properties are selected in device tree bindings and not
> > > specified through the gadget driver API.
> >
> > I'm not sure how this will work when we have multiple functions and only
> > some of them use EBC.The other EPs are working as usual.
> > In terms of DT binding I can think of forcing certain EPs into EBC mode
> > and using them for any gadget that needs EBC but that will remove those
> > EPs from circulation for other functions. It would be great if you could
> > suggest a good alternative we can use.
>
> Ok. If there are only specific endpoints should use this feature, then
> we will need to update the gadget API to support that as you have here.
> Please document how you intend to let the gadget driver know that the HW
> is capable of external FIFO (ie. update usb_gadget structure), and for
> how many endpoint. Also update any expected behavior when using this
> feature.
>
> >
> > >
> > > 2) This specific mode "EBC" doesn't write back to TRBs. It's not clear
> > > how this is handled when updating the request's status. It's also only
> > > applicable to bulk endpoint. If it's to be applied to the usb gadget
> > > API, it's not documented fully.
> >
> > I will push a patch to remove the no-writeback bit based on the decision
> > above.
> >
>
> Sure. We can keep what's already in Greg's. Please update the change as
> discussed.
>
Actually, IMHO we should revert this until the interface is well defined
and documented. The rc releases > 1 should be for fixes and not new
changes to the API.
Thanks,
Thinh