From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut.n900@gmail.com>
Date: Wed, 28 Sep 2011 17:20:13 +0200
Subject: [U-Boot] [PATCH v8 4/4] Add USB support for Efika
In-Reply-To: <1317222457-18412-1-git-send-email-fermata7@gmail.com>
References: <1317222327-18367-1-git-send-email-fermata7@gmail.com>
	<1317222457-18412-1-git-send-email-fermata7@gmail.com>
Message-ID: <1317223213.3682.2.camel@konomi>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

> This commit adds USB support for EfikaMX and EfikaSB.
> 
> Signed-off-by: Jana Rapava <fermata7@gmail.com>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Remy Bohmer <linux@bohmer.net>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
> Changes for v2:
>? ?  - changed to proper patch
> Changes for v3:
>? ?  - merged other USB patches from u-boot-pxa/efikasb
>? ?  - offset-based access changed to struct-based access
>? ?  - use {clrset,clr,set}bits_le32() calls
>? ?  - CodingStyle and naming cleanup
> Changes for v4:
>? ? ?  - split into patchset
>? ? ?  - CodingStyle and naming cleanup
>? ? ?  - remove endless loops
>? ? ?  - silence compiler warnings
> Changes for v5:
>? ? ? ? ?  - change order of arguments in ulpi* functions
>? ? ? ? ?  - change type of reg argument
>? ? ? ? ?  - rename offset macro
> Changes for v6:
>? ? ? ? ? ?  - rebase on top of u-boot-imx/next
>? ? ? ? ? ?  - cleanup of CodingStyle and comments
>? ? ? ? ? ?  - use macro machine_is_efikasb()
>? ? ? ? ? ?  - introduce header file efika.h
> Changes for v7:
>? ? ? ? ? ? ?  - add proper header to efika.h
>? ? ? ? ? ? ?  - include efika.h into efikamx.c
>? ? ? ? ? ? ?  - check return values from ulpi_wait()
> Changes for v8:
> ??? - change the return value of ulpi_wait()
> ??? - CodingStyle cleanup
> ??? - add proper header to efikamx-usb.c

Dear Jana Rapava,

[...]

> +int ulpi_wait(struct usb_ehci *ehci, u32 ulpi_bit)
> +{
> +??? int timeout = ULPI_TIMEOUT;
> +??? u32 tmp;
> +
> +??? /* Wait for the ulpi_bit to become zero. */
> +??? while (--timeout) {
> +??? ??? tmp = readl(&ehci->ulpi_viewpoint);
> +??? ??? if (!(tmp & ulpi_bit))
> +??? ??? ??? break;
> +??? ??? WATCHDOG_RESET();
> +??? }
> +
> +??? if (!timeout)
> +??? ??? return -1;
> +??? else
> +??? ??? return tmp;
> +}

Won't 'return !timeout;' be enough?

Also, can you change 'ulpi_bit' to 'ulpi_mask'? That seems more appropriate.

Maybe this can be even changed to ulpi_wait(ehci, value_to_be_written, bit_to_be_polled), then you won't need those writel()s before every write to ulpi anymore.

> +
> +void ulpi_write(struct usb_ehci *ehci, u32 reg, u32 value)
> +{
> +??? if (!(readl(&ehci->ulpi_viewpoint) & ULPI_SS)) {
> +??? ??? writel(ULPI_WU, &ehci->ulpi_viewpoint);
> +??? ??? if (ulpi_wait(ehci, ULPI_WU) == -1)
> +??? ??? ??? printf("ULPI wakeup timed out\n");
> +??? }
> +
> +??? writel(ULPI_RWRUN | ULPI_RWCTRL |
> +??? ??? reg << ULPI_ADDR_SHIFT | ulpi_write_mask(value),
> +??? ??? &ehci->ulpi_viewpoint);
> +??? if (ulpi_wait(ehci, ULPI_RWRUN) == -1)
> +??? ??? printf("ULPI write timed out\n");
> +}


aha here - if(ulpi_wait(ehci, ULPI_RWRUN | ULPI_RWCTRL ..., uLPI_RWRUN))...

> +
> +u32 ulpi_read(struct usb_ehci *ehci, u32 reg)
> +{
> +??? u32 tmp;
> +??? if (!(readl(&ehci->ulpi_viewpoint) & ULPI_SS)) {
> +??? ??? writel(ULPI_WU, &ehci->ulpi_viewpoint);
> +??? ??? if (ulpi_wait(ehci, ULPI_WU) == -1)
> +??? ??? ??? printf("ULPI wakeup timed out\n");
> +??? }
> +
> +??? writel(ULPI_RWRUN | reg << ULPI_ADDR_SHIFT, &ehci->ulpi_viewpoint);
> +??? tmp = ulpi_wait(ehci, ULPI_RWRUN);
> +??? if (tmp == -1) {
> +??? ??? printf("ULPI read timed out\n");
> +??? ??? return 0;
> +??? }
> +??? return ulpi_read_mask(tmp);
> +}
> +
> +void ulpi_init(struct usb_ehci *ehci, struct mxc_ulpi_regs *ulpi)
> +{
> +??? u32 tmp = 0;
> +??? int reg, i;
> +
> +??? /* get ID from ULPI immediate registers */
> +??? for (reg = ULPI_ID_REGS_COUNT - 1; reg >= 0; reg--)
> +??? ??? tmp |= ulpi_read(ehci, reg) << (reg * 8);

Still (three revs already) no newline here!!

> +??? /* split into vendor and product ID */
> +??? debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);

cheers