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

[Marek Vasut]

On Monday, September 22, 2014 at 11:40:36 AM, Pavel Machek wrote:

[...]

> > index c4f5157..c9d2ed5 100644
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -45,3 +45,6 @@ obj-$(CONFIG_USB_EHCI_ZYNQ) += ehci-zynq.o
> > 
> >  obj-$(CONFIG_USB_XHCI) += xhci.o xhci-mem.o xhci-ring.o
> >  obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos5.o
> >  obj-$(CONFIG_USB_XHCI_OMAP) += xhci-omap.o
> > 
> > +
> > +# designware
> > +obj-$(CONFIG_USB_DWC2) += dwc2.o
> 
> Should this be sorted somehow?

It kind-of is . I don't want exotic goop like this among the standard interfaces 
(*HCI) , so this goes at the bottom.

> > +/**
> > + * Initializes the FSLSPClkSel field of the HCFG register
> > + * depending on the PHY type.
> > + */
> > +static void init_fslspclksel(struct dwc2_core_regs *regs)
> > +{
> > +	uint32_t phyclk;
> > +#ifdef CONFIG_DWC2_ULPI_FS_LS
> 
> Having more readable names for config options would be nice.

It means your ULPI PHY is FL/LS only, how would you make it more readable? Also, 
it should match the option name of original DWC2 driver so it's easier to look 
up the details. On the other hand, noone should ever need to dive into toggling 
these config options here, all of the implementations of the DWC2 core known to 
me (I think I own all of the available devices with DWC2 core) work with the 
default settings.

> > +	uint32_t hwcfg2 = readl(&regs->ghwcfg2);
> > +	uint32_t hval = (ghwcfg2 & DWC2_HWCFG2_HS_PHY_TYPE_MASK) >>
> > +			DWC2_HWCFG2_HS_PHY_TYPE_OFFSET;
> > +	uint32_t fval = (ghwcfg2 & DWC2_HWCFG2_FS_PHY_TYPE_MASK) >>
> > +			DWC2_HWCFG2_FS_PHY_TYPE_OFFSET;
> > +
> > +	if ((hval == 2) && (fval == 1))
> > +		phyclk = DWC2_HCFG_FSLSPCLKSEL_48_MHZ;	/* Full speed PHY */
> > +	else
> > +#endif
> 
> Ifdef ending in "else" is evil.

OK, I guess I can "swap" those two ifdefs and drop the else. The logic of the 
code won't change and the else would disappear.

> > +	/* Wait for 3 PHY Clocks */
> > +	udelay(1);
> 
> Note this.

What about it ?

> > +/**
> > + * Do core a soft reset of the core.  Be careful with this because it
> > + * resets all the internal state machines of the core.
> > + */
> 
> This is not valid kerneldoc -> should not use /** style.
> 
> > +	/* Wait for 3 PHY Clocks */
> > +	mdelay(100);
> 
> Recall it. There's big difference between 1usec and 100msec, which is
> it?

Comment re-aligned with docs.

[...]

> > +	case (USB_REQ_SET_FEATURE << 8) | USB_DIR_OUT | USB_RECIP_OTHER |
> > USB_TYPE_CLASS: +		switch (wValue) {
> > +		case USB_PORT_FEAT_SUSPEND:
> > +			break;
> > +
> > +		case USB_PORT_FEAT_RESET:
> > +			clrsetbits_le32(&regs->hprt0, DWC2_HPRT0_PRTENA |
> > +					DWC2_HPRT0_PRTCONNDET |
> > +					DWC2_HPRT0_PRTENCHNG |
> > +					DWC2_HPRT0_PRTOVRCURRCHNG,
> > +					DWC2_HPRT0_PRTRST);
> > +			mdelay(50);
> > +			clrbits_le32(&regs->hprt0, DWC2_HPRT0_PRTRST);
> > +			break;
> > +
> > +		case USB_PORT_FEAT_POWER:
> > +			clrsetbits_le32(&regs->hprt0, DWC2_HPRT0_PRTENA |
> > +					DWC2_HPRT0_PRTCONNDET |
> > +					DWC2_HPRT0_PRTENCHNG |
> > +					DWC2_HPRT0_PRTOVRCURRCHNG,
> > +					DWC2_HPRT0_PRTRST);
> > +			break;
> > +
> > +		case USB_PORT_FEAT_ENABLE:
> > +			break;
> > +		}
> > +		break;
> 
> This is getting bit. Would it be feasible to split it into functions?

Yeah.

> > +	case (USB_REQ_GET_DESCRIPTOR << 8) | USB_DIR_IN | USB_TYPE_CLASS:
> > +	{
> > +		__u32 temp = 0x00000001;
> 
> temp is never writtenn to. Can you just write 1 instead of it?

I added a comment. This is the root port config, so it should be tweakable
in case there is some mutated DWC2 core.

> > +		data[0] = 9;		/* min length; */
> > +		data[1] = 0x29;
> > +		data[2] = temp & RH_A_NDP;
> > +		data[3] = 0;
> > +		if (temp & RH_A_PSM)
> > +			data[3] |= 0x1;
> > +		if (temp & RH_A_NOCP)
> > +			data[3] |= 0x10;
> > +		else if (temp & RH_A_OCPM)
> > +			data[3] |= 0x8;
> > +
> > +		/* corresponds to data[4-7] */
> > +		data[5] = (temp & RH_A_POTPGT) >> 24;
> > +		data[7] = temp & RH_B_DR;
> > +		if (data[2] < 7) {
> > +			data[8] = 0xff;
> > +		} else {
> 
> Thus data[2] is always <2. What is going on here?

It is assembling the data packet ? data[2] is < 2 always only on currently
known hardware. And I will _not_ be hunting down every single such bit once
the new hardware with more root ports comes out.

> > +	default:
> > +		puts("unsupported root hub command");
> 
> \n?
> 
> > +		/* TODO: no endless loop */
> > +		while (1) {
> > +			hcint_new = readl(&hc_regs->hcint);
> > +			if (hcint != hcint_new)
> > +				hcint = hcint_new;
> 
> What is going on here?

What do you mean ? The loop ?

> Move loop into function to keep complexity down?
> 
> > +	writel((uint32_t)status_buffer, &hc_regs->hcdma);
> 
> Can we get rid of the cast?

No, the buffer us aligned u8 *, while the value is really u32 . We are passing 
the u8 * to the DWC hardware DMA.