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

[Junhui Liu]

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 <seashell11234...@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 <seashell11234...@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.

> 
>> 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.

-- 
Best regards,
Junhui Liu