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
