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

[Thinh Nguyen]

Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>> 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   |  15 +++
>>>  drivers/usb/dwc3/core.h   |  16 ++++
>>>  drivers/usb/dwc3/ep0.c    |   2 +
>>>  drivers/usb/dwc3/gadget.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 265 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index ba74ad7..b194aecd 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,13 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>  	 */
>>>  	hird_threshold = 12;
>>>  
>>> +	/*
>>> +	 * default to a TXFIFO size large enough to fit 6 max packets.  This
>>> +	 * allows for systems with larger bus latencies to have some headroom
>>> +	 * for endpoints that have a large bMaxBurst value.
>>> +	 */
>>> +	tx_fifo_resize_max_num = 6;
>>> +
>>>  	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 +1333,11 @@ 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");
>>> +	if (dwc->do_fifo_resize)
>>> +		device_property_read_u8(dev, "tx-fifo-max-num",
>>> +					&tx_fifo_resize_max_num);
>>
>> Why is this check here? The dwc->tx_fifo_resize_max_num should store
>> whatever property the user sets. Whether the driver wants to use this
> 
> Ack!
> 
>> property should depend on "dwc->do_fifo_resize". Also why don't we have
>> "snps," prefix to be consistent with the other properties?
> 
> Ack!
> 
>> Can we enforce to a single property? If the designer wants to enable
>> this feature, he/she can to provide the tx-fifo-max-num. This would
>> simplify the driver a bit. Since this is to optimize for performance,
>> the user should know/want/test the specific value if they want to set
>> for their setup and not hoping that the default setting not break their
>> setup. So we can remove the "do_fifo_resize" property and just check
>> whether tx_fifo_resize_max_num is set.
> 
> Ack!
> 
> All very valid points :-)
> 

Looks like this series already landed in Greg's testing branch. Not sure
how we usually handle this to address some of our concerns. Add fix
patches on top of Greg's testing branch?

BR,
Thinh