Re: [PATCH] usb: dwc3: Support EBC feature of DWC_usb31

[Thinh Nguyen]

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 <Thinh.Nguyen@xxxxxxxxxxxx> 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 <Thinh.Nguyen@xxxxxxxxxxxx> 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