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

[Govindraj]

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.

>>
>> Signed-off-by: Govindraj.R <govindraj.raja at ti.com>
>> ---
>>  arch/arm/include/asm/arch-omap3/ehci.h       |   55 ++++++
>>  arch/arm/include/asm/arch-omap3/ehci_omap3.h |   58 -------
>>  arch/arm/include/asm/arch-omap4/ehci.h       |   49 ++++++
>>  arch/arm/include/asm/ehci-omap.h             |  147 +++++++++++++++++
>>  drivers/usb/host/ehci-omap.c                 |  228

[...]

>> +
>> +#ifdef CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS
>> +#define OMAP_HS_USB_PORTS    CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS
>> +#else
>> +#define OMAP_HS_USB_PORTS    3
>> +#endif
>> +
>> +#define is_ehci_phy_mode(x)  (x == OMAP_EHCI_PORT_MODE_PHY)
>> +#define is_ehci_tll_mode(x)  (x == OMAP_EHCI_PORT_MODE_TLL)
>> +#define is_ehci_hsic_mode(x) (x == OMAP_EHCI_PORT_MODE_HSIC)
>
> This is unsafe ... use ((x) == ...)

Okay, will correct this.

>
>> +
>> +/* Values of UHH_REVISION - Note: these are not given in the TRM */
>> +#define OMAP_USBHS_REV1                                      0x00000010 /*
> OMAP3 */
>> +#define OMAP_USBHS_REV2                                      0x50700100 /*
> OMAP4 */
>> +

[...]

>> +/* ULPI */
>> +#define ULPI_SET(a)                                  (a + 1)
>> +#define ULPI_CLR(a)                                  (a + 2)
>
> ULPI ... use generic stuff

Actually this for omap specific configuration done with omap
reg map.

EHCI register INSNREG05_ULPI needs to be configured if we
are in ulpi-phy mode same is done here.

>
>> +#define ULPI_FUNC_CTRL                                       0x04
>> +#define ULPI_FUNC_CTRL_RESET                         (1 << 5)
>> +
>> +struct omap_usbhs_board_data {
>> +     enum usbhs_omap_port_mode port_mode[OMAP_HS_USB_PORTS];
>> +};
>> +
>> +struct omap_usbtll {
>> +     u32 rev;                /* 0x00 */
>> +     u32 hwinfo;             /* 0x04 */
>> +     u8 pad1[0x8];
>
> reserved1[] instead of pad1[], fix globally

yes fine, will correct this.

[..]

>>
>> +struct omap_uhh *const uhh = (struct omap_uhh *)OMAP_UHH_BASE;
>> +struct omap_usbtll *const usbtll = (struct omap_usbtll *)OMAP_USBTLL_BASE;
>> +struct omap_ehci *const ehci = (struct omap_ehci *)OMAP_EHCI_BASE;
>
> static
>

yes correct, will change this.

>> +
>> +static int omap_uhh_reset(void)
>> +{

[...]

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

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

--
Thanks,
Govindraj.R