Re: [PATCH] usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized

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

Jack Pham wrote:
> On Tue, Oct 19, 2021 at 02:38:30AM +0000, Thinh Nguyen wrote:
>> Jack Pham wrote:
>>> On Fri, Oct 15, 2021 at 10:20:48PM +0000, Thinh Nguyen wrote:
>>>> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
>>>> flag to check whether it's been resized is not ok. Also, what happened
>>>> after resizing the txfifo? Do you restore its previous default value?
>>>
>>> The choice to use the resizing algorithm is a fixed setting determined
>>> by device property.  So if it is enabled for a particular platform
>>> instance, it means we don't intend to use any of the default values.
>>> All the registers will get cleared to 0 upon every Set Configuration so
>>> that each EP enable call thereafter will be starting from a clean slate.
>>
>> Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller
>> version, we introduce different fields (as the case for DWC32 and
>> DWC31). So this may not apply for all the time. I just noticed that the
>> entire GTXFIFOSIZ was written with 0. Please only write to the specific
>> fields with proper macros.
> 
> Hmm, I thought Wesley's original change already takes care to do that:
> 

I was referring to the field such as TXFRAMNUM that the check "if
(GTXFIFOSIZ)" may not work because this field may be non-zero by default.

> void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
> {
> ...
> 	/* Clear existing TXFIFO for all IN eps except ep0 */
> 	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
>              num += 2) {
> 		dep = dwc->eps[num];
> 		/* Don't change TXFRAMNUM on usb31 version */
> 		size = DWC3_IP_IS(DWC3) ? 0 :
> 			dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
>                 		DWC31_GTXFIFOSIZ_TXFRAMNUM;

I think it's better to use masks such as below since it's
self-documented, but it's probably fine as is.

size &= ~(DWC3x_GTXFIFOSIZ_TXFSADDR(~0) | DWC3x_GTXFIFOSIZ_TXFDEP(~0));

> 
> 		dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1), size);
> 		dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
> 	}
> 	dwc->num_ep_resized = 0;
> }
> 
> Just before the write, the read is masked with the TXFRAMNUM bit in case
> of !DWC3, i.e. DWC31 or DWC32.  The rest would be 0'ed out.  Sorry if my
> previous reply implied the entire register was written as 0.
> 

For DWC32, the field TXFRAMNUM is replaced with something else, but the
TXFSADDR and TXFDEP are unchanged since DWC31. Check that we don't
unintentionally overwrite bit(15) of GTXFIFOSIZ for DWC32 and DWC31 when
updating txfifo.

Thanks,
Thinh