Re: [PATCH v3 7/8] usb: dwc2: Unify flush and reset logic with v4.20a support

[Marek Vasut <marex@denx.de>]

On 1/6/25 10:14 AM, Junhui Liu wrote:
> Hi Marek,
> 
> On 05/01/2025 20:19, Marek Vasut wrote:
>> On 1/4/25 4:37 AM, Junhui Liu wrote:
>>> From: Kongyang Liu <seashell11234455@gmail.com>
>>>
>>> This patch merges flush and reset logic for both host and gadget code
>>> into a common set of functions, reducing duplication. It also adds support
>>> for the updated reset logic to compatible with core version since v4.20a.
>>>
>>> This patch mainly refers to the patch in the kernel.
>>> link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=65dc2e725286106f99c6f6b78e3d9c52c15f3a9c
>>
>> [...]
>>
>>> +++ b/drivers/usb/common/dwc2_core.c
>>> @@ -0,0 +1,115 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (c) 2024, Kongyang Liu <seashell11234455@gmail.com>
>>
>> 2025 now .
> 
> You're right. It should be 2024-2025.
> 
>>
>> [...]
>>
>>> +int dwc2_flush_rx_fifo(struct dwc2_core_regs *regs)
>>> +{
>>> +	int ret;
>>> +
>>> +	log_debug("Flush Rx FIFO\n");
>>> +
>>> +	/* Wait for AHB master IDLE state */
>>> +	ret = wait_for_bit_le32(&regs->global_regs.grstctl, GRSTCTL_AHBIDLE, true, 1000, false);
>>> +	if (ret) {
>>> +		log_warning("%s: Waiting for GRSTCTL_AHBIDLE timeout\n", __func__);
>>> +		return ret;
>>> +	}
>>> +
>>> +	writel(GRSTCTL_RXFFLSH, &regs->global_regs.grstctl);
>>> +
>>> +	ret = wait_for_bit_le32(&regs->global_regs.grstctl, GRSTCTL_RXFFLSH, false, 1000, false);
>>> +	if (ret) {
>>> +		log_warning("%s: Waiting for GRSTCTL_RXFFLSH timeout\n", __func__);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* Wait for at least 3 PHY Clocks */
>>> +	udelay(1);
>> Shouldn't this delay be derived from the PHY clock frequency somehow ?
>> Are we sure 1us is always sufficient ?
> 
> According to the datasheet, the PHY clock can be selected to 6/30/48/60
> MHz depending on the speed mode. And 1us is sufficient even for 6MHz
> (twice the 3 PHY clock at 6MHz), so I think 1us is acceptable here.

Please add a code comment like that ^ .

> If we want to derive the delay dynamically from the PHY clock frequency,
> we would need to read the PHY clock and speed mode cfg from the register
> and calculate the appropriate delay. This would add complexity to the
> code. In the current situation, a fixed delay of 1 us is sufficient for
> all supported clock frequencies and keeps the implementation simpler.
Thanks for the clarification, add a code comment and that is all fine.