[U-Boot] [PATCH v2 1/4] ehci-omap: Clean up added ehci-omap.c

[Marek Vasut]

> Hi Marek,
> 
> Thanks for you review.
> 
> On Tue, Jan 10, 2012 at 9:37 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> >> From: "Govindraj.R" <govindraj.raja at ti.com>
> >> 
> >> Clean up added ehci-omap.c and make it generic for re-use across
> >> soc having same ehci ip block. Also pass the modes to be configured
> >> and configure the ports accordingly. All usb layers are not cache
> >> aligned till then keep cache off for usb ops as ehci will use
> >> internally dma for all usb ops.
> >> 
> >> * Add a generic common header ehci-omap.h having common ip block
> >>   data and reg shifts.
> >> * Rename and modify ehci-omap3 to ehci.h retain only conflicting
> >>   sysc reg shifts remove others and move to common header file.
> > 
> > Don't reimplement the ulpi stuff ... there's already some ulpi stuff in
> > uboot that needs fixing, so fix it and use it.
> 
> I am not implementing any ulpi stuff I am just configuring OMAP on
> soc usb host controller (ehci). All the configuration stuff
> is OMAP specific things which are done in ehci-omap.c file
> 
> stuffs done are like soft-reset, port mode to be used and putting
> port in no -idle mode(omap specific pm implementation) etc.
> 

This stuff:

+/* ULPI */
+#define ULPI_SET(a)                                    (a + 1)
+#define ULPI_CLR(a)                                    (a + 2)
+#define ULPI_FUNC_CTRL                                 0x04
+#define ULPI_FUNC_CTRL_RESET                           (1 << 5)

is just accidentally conforming to ULPI spec?

M

btw. somewhere in the patch is one more asterisk at the end of line:
+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com*

[...]

> >> +     /* perform TLL soft reset, and wait until reset is complete */
> >> +     writel(OMAP_USBTLL_SYSCONFIG_SOFTRESET, &usbtll->sysc);
> >> +
> >> +     /* Wait for TLL reset to complete */
> >> +     while (!(readl(&usbtll->syss) & OMAP_USBTLL_SYSSTATUS_RESETDONE))
> > 
> > Add timeout, fix globally
> 
> Sorry I didn't get you here.
> 
> The function uses a timeout value init and then same init
> value to used to poll for CONFIG_SYS_HZ time for reset to be
> done else prints timeout failure.

Ah sorry, I didn't notice. OK.
> 
> >> +             if (get_timer(init) > CONFIG_SYS_HZ) {
> >> +                     debug("OMAP EHCI error: timeout resetting TLL\n");
> >> +                     return -EL3RST;
> >> +     }
> >> +
> 
> [...]
> 
> >> +     /* setup ULPI bypass and burst configurations */
> >> +     reg |= (OMAP_UHH_HOSTCONFIG_INCR4_BURST_EN |
> >> +             OMAP_UHH_HOSTCONFIG_INCR8_BURST_EN |
> >> +             OMAP_UHH_HOSTCONFIG_INCR16_BURST_EN);
> >> +     reg &= ~OMAP_UHH_HOSTCONFIG_INCRX_ALIGN_EN;
> > 
> > clrsetbits_le32 ?
> 
> yes can be used.

Use where applicable
> 
> --
> Thanks,
> Govindraj.R