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

[Marek Vasut]

On 12/30/24 4:30 AM, Junhui Liu via B4 Relay 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

Signed-off-by: Kongyang Liu <seashell11234...@gmail.com>
Signed-off-by: Junhui Liu <liujh2...@outlook.com>
---
  drivers/usb/common/Makefile                |   2 +
  drivers/usb/common/dwc2_core.c             | 103 +++++++++++++++++++++++++++++
  drivers/usb/common/dwc2_core.h             |   4 ++
  drivers/usb/gadget/dwc2_udc_otg.c          |  12 +---
  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c |   6 +-
  drivers/usb/host/dwc2.c                    |  80 ++--------------------
  6 files changed, 117 insertions(+), 90 deletions(-)

diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index 
11cc4657a0f403b84b1b8336781e1893d9c7a8f1..73e5bc6d7fdca692276e119911b47db4bf03586a
 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -4,6 +4,8 @@
  #
  
  obj-$(CONFIG_$(XPL_)DM_USB) += common.o
+obj-$(CONFIG_USB_DWC2) += dwc2_core.o
+obj-$(CONFIG_USB_GADGET_DWC2_OTG) += dwc2_core.o
  obj-$(CONFIG_USB_ISP1760) += usb_urb.o
  obj-$(CONFIG_USB_MUSB_HOST) += usb_urb.o
  obj-$(CONFIG_USB_MUSB_GADGET) += usb_urb.o
diff --git a/drivers/usb/common/dwc2_core.c b/drivers/usb/common/dwc2_core.c
new file mode 100644
index 
0000000000000000000000000000000000000000..323326e05d7b0318d883d4a3b8186b812c42f9ca
--- /dev/null
+++ b/drivers/usb/common/dwc2_core.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2024, Kongyang Liu <seashell11234...@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <wait_bit.h>
+
+#include "dwc2_core.h"
+
+int dwc2_core_reset(struct dwc2_core_regs *regs)
+{
+       u32 snpsid;
+       int ret;
+       bool host_mode = false;
+
+       if (!(readl(&regs->global_regs.gotgctl) & GOTGCTL_CONID_B) ||
+           (readl(&regs->global_regs.gusbcfg) & GUSBCFG_FORCEDEVMODE))
+               host_mode = true;
+
+       /* Core Soft Reset */
+       snpsid = readl(&regs->global_regs.gsnpsid);
+       writel(GRSTCTL_CSFTRST, &regs->global_regs.grstctl);
+       if (FIELD_GET(GSNPSID_VER_MASK, snpsid) < 0x420a) {
+               ret = wait_for_bit_le32(&regs->global_regs.grstctl, 
GRSTCTL_CSFTRST,
+                                       false, 1000, false);
+               if (ret) {
+                       log_warning("%s: Waiting for GRSTCTL_CSFTRST 
timeout\n", __func__);
+                       return -EBUSY;

Return -ETIMEDOUT or even better, return ret; to propagate the return 
value of wait_for_bit_le32() as-is , which is really -ETIMEDOUT if the 
wait times out.

+               }
+       } else {
+               ret = wait_for_bit_le32(&regs->global_regs.grstctl, 
GRSTCTL_CSFTRST_DONE,
+                                       true, 1000, false);
+               if (ret) {
+                       log_warning("%s: Waiting for GRSTCTL_CSFTRST_DONE 
timeout\n", __func__);
+                       return -EBUSY;

DTTO, return ret;

+               }
+               clrsetbits_le32(&regs->global_regs.grstctl, GRSTCTL_CSFTRST, 
GRSTCTL_CSFTRST_DONE);
+       }
+
+       /* 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 -EBUSY;

DTTO, return ret;

+       }
+
+       if (host_mode) {
+               ret = wait_for_bit_le32(&regs->global_regs.gintsts, 
GINTSTS_CURMODE_HOST,
+                                       host_mode, 1000, false);
+               if (ret) {
+                       log_warning("%s: Waiting for GINTSTS_CURMODE_HOST 
timeout\n", __func__);
+                       return -EBUSY;

DTTO, return ret;

+               }
+       }
+
+       return 0;
+}
+
+void dwc2_flush_tx_fifo(struct dwc2_core_regs *regs, const int num)
+{
+       int ret;
+
+       log_debug("Flush Tx FIFO %d\n", num);
+
+       /* 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__);


Should this function return an integer and an error code on failure ?

+       writel(GRSTCTL_TXFFLSH | FIELD_PREP(GRSTCTL_TXFNUM_MASK, num), 
&regs->global_regs.grstctl);
+
+       ret = wait_for_bit_le32(&regs->global_regs.grstctl, GRSTCTL_TXFFLSH, 
false, 1000, false);
+       if (ret)
+               log_warning("%s: Waiting for GRSTCTL_TXFFLSH timeout\n", 
__func__);
+
+       /* Wait for 3 PHY Clocks */
+       udelay(1);
+}
+
+void 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__);
+
+       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__);
+
+       /* Wait for 3 PHY Clocks */
+       udelay(1);

Shouldn't this delay be derived from the PHY clock frequency somehow ?
Are we sure 1us is always sufficient ?

+}
[...]