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

[Pavel Machek]

Hi!

> This is the USB host controller used on the Altera SoCFPGA and Raspbery Pi.
> 
> This code has three checkpatch warnings, but to make sure it stays at least
> readable and clear, these are not fixed. These bugs are in the USB request
> handling combinatorial logic, so any abstracting of those is out of question.

Not too bad. Minor comments below.


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

> +/**
> + * 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.

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

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

Note this.

> +/**
> + * 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?

> +	/* Program the HCSPLIT register for SPLITs */
> +	writel(0, &hc_regs->hcsplt);
> +}
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

This is little un-conventional :-).

> +	if (usb_pipeint(pipe)) {
> +		puts("Root-Hub submit IRQ: NOT implemented");

Missing \n?

> +	wValue	      = cpu_to_le16 (cmd->value);
> +	wLength	      = cpu_to_le16 (cmd->length);

Extra space.

> +
> +	switch (bmRType_bReq) {
> +	case (USB_REQ_GET_STATUS << 8) | USB_DIR_IN:
> +		*(__u16 *)buffer = cpu_to_le16(1);


Can we use u16 (not __u16) here?

> +	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?

> +	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?

> +		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?

> +	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?

Move loop into function to keep complexity down?

> +	writel((uint32_t)status_buffer, &hc_regs->hcdma);

Can we get rid of the cast?

> +#define DWC2_HWCFG1_EP_DIR6_MASK			(0x3 << 12)
> +#define DWC2_HWCFG1_EP_DIR6_OFFSET			12

Quick grep shows this is unused. And all of these come in mask+offset
variety. Can we get rid of some?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html