From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Machek Date: Mon, 22 Sep 2014 11:40:36 +0200 Subject: [U-Boot] [PATCH 1/3] usb: dwc2: Add driver for Synopsis DWC2 USB IP block In-Reply-To: <1411305204-11731-2-git-send-email-marex@denx.de> References: <1411305204-11731-1-git-send-email-marex@denx.de> <1411305204-11731-2-git-send-email-marex@denx.de> Message-ID: <20140922094036.GC15350@amd> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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(®s->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(®s->hprt0, DWC2_HPRT0_PRTENA | > + DWC2_HPRT0_PRTCONNDET | > + DWC2_HPRT0_PRTENCHNG | > + DWC2_HPRT0_PRTOVRCURRCHNG, > + DWC2_HPRT0_PRTRST); > + mdelay(50); > + clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTRST); > + break; > + > + case USB_PORT_FEAT_POWER: > + clrsetbits_le32(®s->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