Re: [PATCH v12 3/6] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements

[Wesley Cheng]

On 7/6/2021 11:13 AM, Greg KH wrote:
> On Fri, Jul 02, 2021 at 02:37:32AM -0700, Wesley Cheng wrote:
>> Some devices have USB compositions which may require multiple endpoints
>> that support EP bursting.  HW defined TX FIFO sizes may not always be
>> sufficient for these compositions.  By utilizing flexible TX FIFO
>> allocation, this allows for endpoints to request the required FIFO depth to
>> achieve higher bandwidth.  With some higher bMaxBurst configurations, using
>> a larger TX FIFO size results in better TX throughput.
>>
>> By introducing the check_config() callback, the resizing logic can fetch
>> the maximum number of endpoints used in the USB composition (can contain
>> multiple configurations), which helps ensure that the resizing logic can
>> fulfill the configuration(s), or return an error to the gadget layer
>> otherwise during bind time.
>>
>> Signed-off-by: Wesley Cheng <wcheng@xxxxxxxxxxxxxx>
>> ---
>>  drivers/usb/dwc3/core.c   |   9 ++
>>  drivers/usb/dwc3/core.h   |  15 ++++
>>  drivers/usb/dwc3/ep0.c    |   2 +
>>  drivers/usb/dwc3/gadget.c | 221 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 247 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index e0a8e79..a7bcdb9d 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1267,6 +1267,7 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  	u8			rx_max_burst_prd;
>>  	u8			tx_thr_num_pkt_prd;
>>  	u8			tx_max_burst_prd;
>> +	u8			tx_fifo_resize_max_num;
>>  	const char		*usb_psy_name;
>>  	int			ret;
>>  
>> @@ -1282,6 +1283,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  	 */
>>  	hird_threshold = 12;
>>  
>> +	tx_fifo_resize_max_num = 6;
>> +

Hi Greg,
> 
> No comment as to why 6 was picked, like the other defaults in this
> function?
> 
> Why was 6 picked?
> 
> 
Talked with Thinh about this sometime back about why 6 was picked.  It
was just an arbitrary setting we decided on throughout our testing, as
that was what provided the best tput numbers for our system.  Hence why
it was suggested to have a separate property, so other vendors can set
this to accommodate their difference in HW latencies.

>>  	dwc->maximum_speed = usb_get_maximum_speed(dev);
>>  	dwc->max_ssp_rate = usb_get_maximum_ssp_rate(dev);
>>  	dwc->dr_mode = usb_get_dr_mode(dev);
>> @@ -1325,6 +1328,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  				&tx_thr_num_pkt_prd);
>>  	device_property_read_u8(dev, "snps,tx-max-burst-prd",
>>  				&tx_max_burst_prd);
>> +	dwc->do_fifo_resize = device_property_read_bool(dev,
>> +							"tx-fifo-resize");
>> +	device_property_read_u8(dev, "tx-fifo-max-num",
>> +				&tx_fifo_resize_max_num);
> 
> So you overwrite the "max" with whatever is given to you?  What if
> tx-fifo-resize is not enabled?
>
If tx-fifo-resize is not enabled, then there shouldn't be anything that
will reference this property.  As mentioned in the previous comment, HW
vendors may not need a FIFO size of 6 max packets for their particular
system, so they should be able to program this to their needs.

If someone programs to this a large number, the logic works where it
will allocate based off the space left after ensuring enough space for 1
FIFO per ep.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project