Re: [PATCH v5] usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs

[AKASH KUMAR]

Hi,

On 10/1/2024 6:50 AM, Thinh Nguyen wrote:
> On Sat, Sep 28, 2024, Akash Kumar wrote:
> > The current logic is rigid, setting num_fifos to fixed values.
> > 3 for any maxburst greater than 1.
> > tx_fifo_resize_max_num for maxburst greater than 6.
> > Additionally, it did not differentiate much between bulk and
> > isochronous transfers, applying similar logic to both.
> >
> > The updated logic is more flexible and specifically designed to meet
> > the unique requirements of both bulk and isochronous transfers. We
> > have made every effort to satisfy all needs and requirements,
> > verified on our specific platform and application.
> >
> > The updated logic is more flexible and specifically designed to meet
> > the unique requirements of both bulk and isochronous transfers. We
> > have made every effort to satisfy all needs and requirements, verified
> > on our specific platform and application.
> Was there a copy-and-paste issue with the above paragraph?
yeah my mistake, will fix.
> > Bulk Transfers: Ensures that num_fifos is optimized by considering both
> > the maxburst and DT property tx_fifo_resize_max_num for super speed and
> > above. For high-speed and below bulk endpoints, a 2K TxFIFO allocation
> > is used to meet efficient data transfer needs, considering
> > FIFO-constrained platforms.
> >
> > Isochronous Transfers: Ensures that num_fifos is sufficient by considering
> > the maximum packet multiplier for HS and maxburst for SS, along with a
> > constraint with the DT property tx_fifo_resize_max_num.
> >
> > This change aims to optimize the allocation of Tx FIFOs for both bulk
> > and isochronous endpoints, potentially improving data transfer efficiency
> > and overall performance. It also enhances support for all use cases,
> > which can be tweaked with DT parameters and the endpoint’s maxburst and
> > maxpacket.
> >
> > Signed-off-by: Akash Kumar <quic_akakum@xxxxxxxxxxx>
> > ---
> > Changes for v5:
> > Update Calculation for HS and below bulk and isoc eps based on
> > suggestion and fixed at 2k for bulk eps considering fifo
> > constrained platforms.
> >
> > Changes for v4:
> > Updated commit message as per review comments to clarify that it has
> > been tested on specific platforms only and tried best to match all
> > expectations.
> >
> > Changes for v3:
> > Redefine logic for resizing tx fifos,added check based on  operating
> > speed and used maxp for HS and maxburst for SS  and defined max
> > allocation based on dt property.
> >
> > Changes for v2:
> > Redefine logic for resizing tx fifos, handled fifo based on  minimum
> > of maxp and maxburts.
> >
> > Changes for v1:
> > Added additional condition to allocate tx fifo for hs isoc  eps,
> > keeping the other resize logic same.
> > ---
> >   drivers/usb/dwc3/gadget.c | 20 +++++++++++++-------
> >   1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 291bc549935b..035d49155d57 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -771,15 +771,21 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)
> >   
> >   	ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
> >   
> > -	if ((dep->endpoint.maxburst > 1 &&
> > -	     usb_endpoint_xfer_bulk(dep->endpoint.desc)) ||
> > -	    usb_endpoint_xfer_isoc(dep->endpoint.desc))
> > -		num_fifos = 3;
> > +	if (dwc->gadget->speed <= USB_SPEED_HIGH) {
> > +		if (usb_endpoint_xfer_bulk(dep->endpoint.desc))
> > +			num_fifos = 2;
> > +		if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
> > +			num_fifos = min_t(unsigned int,
> > +					  usb_endpoint_maxp_mult(dep->endpoint.desc) + 1,
> > +					  dwc->tx_fifo_resize_max_num);
> > +	}
> >   
> > -	if (dep->endpoint.maxburst > 6 &&
> > +	if (dwc->gadget->speed > USB_SPEED_HIGH &&
> >   	    (usb_endpoint_xfer_bulk(dep->endpoint.desc) ||
> > -	     usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31))
> > -		num_fifos = dwc->tx_fifo_resize_max_num;
> > +	     usb_endpoint_xfer_isoc(dep->endpoint.desc)))
> > +		num_fifos = min_t(unsigned int,
> > +				  dep->endpoint.maxburst,
> > +				  dwc->tx_fifo_resize_max_num);
> >   
> Do you have a default value for other endpoints?
num_fifos is initialized to 1 in beginning for other eps.
> Perhaps we can do something like this? What do you think?
yeah can used since we are using gadget speed only to decide and it will 
be faster as well.
> 	num_fifos = 1;
>
> 	switch (dwc->gadget->speed) {
> 	case USB_SPEED_SUPER_PLUS:
> 	case USB_SPEED_SUPER:
> 		if (usb_endpoint_xfer_bulk(dep->endpoint.desc) ||
> 		    usb_endpoint_xfer_isoc(dep->endpoint.desc))
> 			num_fifos = min_t(unsigned int,
> 					  dep->endpoint.maxburst,
> 					  dwc->tx_fifo_resize_max_num);
> 		break;
> 	case USB_SPEED_HIGH:
> 		if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
> 			num_fifos = min_t(unsigned int,
> 					  usb_endpoint_maxp_mult(dep->endpoint.desc) + 1,
> 					  dwc->tx_fifo_resize_max_num);
> 			break;
> 		}
> 		fallthrough;
> 	case USB_SPEED_FULL:
> 		if (usb_endpoint_xfer_bulk(dep->endpoint.desc))
> 			num_fifos = 2;
> 		break;
> 	default:
> 		break;
> 	}
>
> Thanks,
> Thinh
>
> >   	/* FIFO size for a single buffer */
> >   	fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1);
> > --
> > 2.17.1
Thanks,
Akash