Re: [PATCH] usb: dwc3: gadget: Add TxFIFO resizing supports for single port RAM

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

++ Alan Stern

On Fri, Nov 08, 2024, Selvarasu Ganesan wrote:

> >> +	ram_depth = spram_type ? DWC3_RAM0_DEPTH(dwc->hwparams.hwparams6) :
> >> +			DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
> > Don't use spram_type as a boolean. Perhaps define a macro for type value
> > 1 and 0 (for single vs 2-port)
> Are you expecting something like below?
> 
> #define DWC3_SINGLE_PORT_RAM     1
> #define DWC3_TW0_PORT_RAM        0

Yes. I think it's more readable if we name the variable to "ram_type"
and use the macros above as I suggested.

If you still plan to use it as boolean, please rename the variable
spram_type to is_single_port_ram (no one knows what "spram_type" mean
without the programming guide or some documention).

>

< snip >

> >>
> > We may need to think a little more on how to budgeting the resource
> > properly to accomodate for different requirements. If there's no single
> > formula to satisfy for all platform, perhaps we may need to introduce
> > parameters that users can set base on the needs of their application.

> Agree. Need to introduce some parameters to control the required fifos 
> by user that based their usecase.
> Here's a rephrased version of your proposal:
> 
> To address the issue of determining the required number of FIFOs for 
> different types of transfers, we propose introducing dynamic FIFO 
> calculation for all type of EP transfers based on the maximum packet 
> size, and remove hard code value for required fifos in driver,  

The current fifo calculation already takes on the max packet size into
account.

For SuperSpeed and above, we can guess how much fifo is needed base on
the maxburst and mult settings. However, for bulk endpoint in highspeed,
it needs a bit more checking.

> Additionally, we suggest introducing DT properties(tx-fifo-max-num-iso, 
> tx-fifo-max-bulk and tx-fifo-max-intr) for all types of transfers 

This constraint should be decided from the function driver. We should
try to keep this more generic since your gadget may be used as mass
storage device instead of UVC where bulk performance is needed more.

> (except control EP) to allow users to control the required FIFOs instead 
> of relying solely on the tx-fifo-max-num. This approach will provide 
> more flexibility and customization options for users based on their 
> specific use cases.
> 
> Please let me know if you have any comments on the above approach.
> 

How about this: Implement gadget->ops->match_ep() for dwc3 and update
the note in usb_ep_autoconfig() API.

If the function driver looks for an endpoint by passing in the
descriptor with wMaxPacketSize set to 0, mark the endpoint to used for
performance. This is closely related to the usb_ep_autoconfig() behavior
where it returns the endpoint's maxpacket_limit if wMaxPacketSize is not
provided. We just need to expand this behavior to look for performance
endpoint.

If the function driver provides the wMaxPacketSize during
usb_ep_autoconfig(), then use the minimum required fifo.

What do you think? Will this work for you?

BR,
Thinh