[PATCH] usb: dwc2: update reset method for host and device mode

[Kongyang Liu]

Marek Vasut <marex at denx.de> 于2024年4月22日周一 04:45写道：
>
> On 3/28/24 2:14 PM, Kongyang Liu wrote:
>
> [...]
>
> > @@ -464,12 +464,26 @@ static void reconfig_usbd(struct dwc2_udc *dev)
> >   {
> >       /* 2. Soft-reset OTG Core and then unreset again. */
> >       int i;
> > -     unsigned int uTemp = writel(CORE_SOFT_RESET, &reg->grstctl);
> > +     unsigned int uTemp;
> >       uint32_t dflt_gusbcfg;
> >       uint32_t rx_fifo_sz, tx_fifo_sz, np_tx_fifo_sz;
> >       u32 max_hw_ep;
> >       int pdata_hw_ep;
> >
>
> Drop this newline
>

I will fix it

> > +     u32 snpsid, greset;
> > +
> > +     snpsid = readl(&reg->gsnpsid);
> > +     writel(CORE_SOFT_RESET, &reg->grstctl);
> > +     if ((snpsid & SNPSID_VER_MASK) < (SNPSID_VER_420a & SNPSID_VER_MASK)) {
>
> Can you use FIELD_GET()/FIELD_PREP() for this ?
>

I will fix it

> > +             wait_for_bit_le32(&reg->grstctl, CORE_SOFT_RESET, false, 1000, false);
> > +     } else {
> > +             wait_for_bit_le32(&reg->grstctl, CORE_SOFT_RESET_DONE, true, 1000, false);
> > +             greset = readl(&reg->grstctl);
> > +             greset &= ~CORE_SOFT_RESET;
> > +             greset |= CORE_SOFT_RESET_DONE;
> > +             writel(greset, &reg->grstctl);
>
> clrsetbits_le32()
>

I will fix it

> [...]
>
> > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> > index 637eb2dd06..1baeff96ee 100644
> > --- a/drivers/usb/host/dwc2.c
> > +++ b/drivers/usb/host/dwc2.c
> > @@ -159,6 +159,7 @@ static void dwc_otg_core_reset(struct udevice *dev,
> >                              struct dwc2_core_regs *regs)
> >   {
> >       int ret;
> > +     u32 snpsid, greset;
> >
> >       /* Wait for AHB master IDLE state. */
> >       ret = wait_for_bit_le32(&regs->grstctl, DWC2_GRSTCTL_AHBIDLE,
> > @@ -167,9 +168,20 @@ static void dwc_otg_core_reset(struct udevice *dev,
> >               dev_info(dev, "%s: Timeout!\n", __func__);
> >
> >       /* Core Soft Reset */
> > +     snpsid = readl(&regs->gsnpsid);
> >       writel(DWC2_GRSTCTL_CSFTRST, &regs->grstctl);
> > -     ret = wait_for_bit_le32(&regs->grstctl, DWC2_GRSTCTL_CSFTRST,
> > -                             false, 1000, false);
> > +     if ((snpsid & DWC2_SNPSID_VER_MASK) < (DWC2_SNPSID_DEVID_VER_420a & DWC2_SNPSID_VER_MASK)) {
> > +             ret = wait_for_bit_le32(&regs->grstctl, DWC2_GRSTCTL_CSFTRST,
> > +                                     false, 1000, false);
> > +     } else {
> > +             ret = wait_for_bit_le32(&regs->grstctl, DWC2_GRSTCTL_GSFTRST_DONE,
> > +                                     true, 1000, false);
> > +             greset = readl(&regs->grstctl);
> > +             greset &= ~DWC2_GRSTCTL_CSFTRST;
> > +             greset |= DWC2_GRSTCTL_GSFTRST_DONE;
> > +             writel(greset, &regs->grstctl);
>
> Same comments as above.
>
> Maybe this should be pulled into dedicated function to avoid duplication?
>

For U-Boot, the dwc2 USB driver is split into two modules: host and gadget.
Each has its own register definitions and definitions for register bits,
which makes it difficult to extract a single function. Moreover, deciding
where to place this function is also an issue.

> > +     }
> > +
> >       if (ret)
> >               dev_info(dev, "%s: Timeout!\n", __func__);
> >
> > @@ -1180,7 +1192,8 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv)
> >                snpsid >> 12 & 0xf, snpsid & 0xfff);
> >
> >       if ((snpsid & DWC2_SNPSID_DEVID_MASK) != DWC2_SNPSID_DEVID_VER_2xx &&
> > -         (snpsid & DWC2_SNPSID_DEVID_MASK) != DWC2_SNPSID_DEVID_VER_3xx) {
> > +             (snpsid & DWC2_SNPSID_DEVID_MASK) != DWC2_SNPSID_DEVID_VER_3xx &&
> > +         (snpsid & DWC2_SNPSID_DEVID_MASK) != DWC2_SNPSID_DEVID_VER_4xx) {
>
> Try FIELD_GET/FIELD_PREP

I will fix it

Best regards
Kongyang Liu