[U-Boot] [PATCH 1/3] usb: dwc2: Add driver for Synopsis DWC2 USB IP block

[Marek Vasut]

On Tuesday, September 23, 2014 at 11:59:28 PM, Dinh Nguyen wrote:

[...]

> > +static int wait_for_bit(void *reg, const uint32_t mask, bool set)
> > +{
> > +	unsigned int timeout = 1000000;
> > +	uint32_t val;
> > +
> > +	while (--timeout) {
> > +		val = readl(reg);
> > +		if (!set)
> > +			val = ~val;
> > +
> > +		if ((val & mask) == mask)
> > +			return 0;
> > +
> > +		udelay(1);
> > +	}
> > +
> > +	printf("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n",
> > +	       __func__, reg, mask, set);
> 
> This debug print doesn't really tell us much. We've timed out, but we have
> no idea from where?
> 
> > +
> > +	return -ETIMEDOUT;
> 
> You're returning -ETIMEDOUT, but none of the users of wait_for_bit make any
> use of this. Perhaps, a printf from the caller function would tell us more?

OK, makes sense. I changed this one to debug() and added a printf() after each 
call.

btw. please try to trim down the content of the patch when replying only to the 
relevant part, so others don't have to look up the relevant bits among billions
of lines of irrelevant stuff.

[...]

> > +/**
> > + * Do core a soft reset of the core.  Be careful with this because it
> > + * resets all the internal state machines of the core.
> > + */
> > +static void dwc_otg_core_reset(struct dwc2_core_regs *regs)
> > +{
> > +	/* Wait for AHB master IDLE state. */
> > +	wait_for_bit(&regs->grstctl, DWC2_GRSTCTL_AHBIDLE, 1);
> > +
> > +	/* Core Soft Reset */
> > +	writel(DWC2_GRSTCTL_CSFTRST, &regs->grstctl);
> > +	wait_for_bit(&regs->grstctl, DWC2_GRSTCTL_CSFTRST, 0);
> > +
> > +	/* Wait for 3 PHY Clocks */
> 
> This comment is probably not true since "3 PHY clocks" was 1 uS in
> dwc_otg_flush_tx_fifo() and dwc_otg_flush_rx_fifo(), and here it's
> 100ms. The Linux version for this driver has a 150ms - 200ms range. So
> 150ms here should be good. The comment from the linux driver is:
> 
> "NOTE: This long sleep is _very_ important, otherwise the core will
>  not stay in host mode after a connector ID change!"

Added this note and tweaked the timeout to 150ms, though 100ms works for me too.

[...]

Fixed the bits as suggested.

> > +#define DWC2_GUSBCFG_TX_END_DELAY			(1 << 28)
> > +#define DWC2_GUSBCFG_TX_END_DELAY_OFFSET		28
> 
> bits 29 and 30 of GUSBCFG are to Force Host and Device mode, respectively.
> These bits may get used in the future.

That's good, what's their exact name please ?

[...]

> > +#define DWC2_DEVICE_GRXSTS_EPNUM_MASK			(0xF << 0)
> > +#define DWC2_DEVICE_GRXSTS_EPNUM_OFFSET			0
> > +#define DWC2_DEVICE_GRXSTS_BCNT_MASK			(0x7FF << 4)
> > +#define DWC2_DEVICE_GRXSTS_BCNT_OFFSET			4
> > +#define DWC2_DEVICE_GRXSTS_DPID_MASK			(0x3 << 15)
> > +#define DWC2_DEVICE_GRXSTS_DPID_OFFSET			15
> > +#define DWC2_DEVICE_GRXSTS_PKTSTS_MASK			(0xF << 17)
> > +#define DWC2_DEVICE_GRXSTS_PKTSTS_OFFSET		17
> > +#define DWC2_DEVICE_GRXSTS_FN_MASK			(0xF << 21)
> > +#define DWC2_DEVICE_GRXSTS_FN_OFFSET			21
> > +#define DWC2_HOST_GRXSTS_CHNUM_MASK			(0xF << 0)
> > +#define DWC2_HOST_GRXSTS_CHNUM_OFFSET			0
> > +#define DWC2_HOST_GRXSTS_BCNT_MASK			(0x7FF << 4)
> > +#define DWC2_HOST_GRXSTS_BCNT_OFFSET			4
> > +#define DWC2_HOST_GRXSTS_DPID_MASK			(0x3 << 15)
> > +#define DWC2_HOST_GRXSTS_DPID_OFFSET			15
> > +#define DWC2_HOST_GRXSTS_PKTSTS_MASK			(0xF << 17)
> > +#define DWC2_HOST_GRXSTS_PKTSTS_OFFSET			17
> 
> Not sure why there needs to be an entry for DEVICE_ and HOST_ of GRXSTS
> defines here?

Good point, fixed.