On 11/9/2024 6:35 AM, Thinh Nguyen wrote: > ++ 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). We are fine to use variable name as a is_single_port_ram with boolean. Please find the below updated new patch for your review. https://lore.kernel.org/linux-usb/20241111142049.604-1-selvarasu.g@xxxxxxxxxxx/. > < 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. Agree. > >> 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. Agree. > >> (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? Hi Thinh, Thank you for the suggestions. This method makes more sense to us, and we are fine proceeding with it. As we previously discussed, we plan to create a separate patch for allocating resources for bulk EP. However, it may take some time on our end as we need to perform additional testing with all possible scenarios. In the meantime, please review and help to merge the patch for the single port RAM FIFO resizing logic. Thanks, Selva > > BR, > Thinh
