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

[Marek Vasut <marex@denx.de>]

On 1/7/25 1:39 PM, Junhui Liu wrote:
> 
> 
> On 06/01/2025 16:37, Marek Vasut wrote:
>> 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 ^ .
> 
> OK, I will add comments in the next version.
Thank you