[U-Boot] [PATCH v3] Add USB support for Efika

[Marek Vasut <marek.vasut@gmail.com>]

On Sunday, September 18, 2011 04:19:28 AM Jana Rapava wrote:
> From: Marek Vasut <marek.vasut@gmail.com>
> 
> This commit adds USB support for EfikaMX and EfikaSB.
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Signed-off-by: Jana Rapava <fermata7@gmail.com>
> ---
> 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

Dear Jana Rapava,

the patch doesn't pass checkpatch:
WARNING: line over 80 characters
#253: FILE: board/efikamx/efikamx-usb.c:163:
+                       (MXX1_OTG_WUE | MXX1_OTG_PM | MX51_H1_ULPI_IE | 
MX51_H1_WUE),
and a lot of other such stuff.

Also, CC Stefano Babic and Remy Bohmer for next iteration, they're imx custodian 
and usb custodian respectively.
> 
>  board/efikamx/Makefile      |    3 +
>  board/efikamx/efikamx-usb.c |  349
> +++++++++++++++++++++++++++++++++++++++++++ board/efikamx/efikamx.c     | 
>  10 ++
>  drivers/usb/host/ehci-hcd.c |   19 +++
>  drivers/usb/host/ehci-mxc.c |    9 +-
>  include/configs/efikamx.h   |   35 ++++-
>  include/usb/ehci-fsl.h      |  112 ++++++++++++++-
>  7 files changed, 524 insertions(+), 13 deletions(-)
>  create mode 100644 board/efikamx/efikamx-usb.c
> 
> diff --git a/board/efikamx/Makefile b/board/efikamx/Makefile
> index ee4a16e..860e4d2 100644
> --- a/board/efikamx/Makefile
> +++ b/board/efikamx/Makefile
> @@ -28,6 +28,9 @@ include $(TOPDIR)/config.mk
>  LIB	= $(obj)lib$(BOARD).o
> 
>  COBJS	:= efikamx.o
> +ifdef	CONFIG_CMD_USB
> +COBJS	+= efikamx-usb.o
> +endif
> 
>  SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
>  OBJS	:= $(addprefix $(obj),$(COBJS))
> diff --git a/board/efikamx/efikamx-usb.c b/board/efikamx/efikamx-usb.c
> new file mode 100644
> index 0000000..19227d4
> --- /dev/null
> +++ b/board/efikamx/efikamx-usb.c

[...]

> +
> +/*
> + * Configure control registers of the USB controller
> + */
> +void control_regs_setup(struct usb_control_regs *control)
> +{
> +	clrsetbits_le32(&control->usbctrl,
> +			(MXX1_OTG_WUE | MXX1_OTG_PM | MX51_H1_ULPI_IE | 
MX51_H1_WUE),

What's this MXX1_ stuff ?

> +			MX51_H1_PM);
> +
> +	clrsetbits_le32(&control->phyctrl0,
> +			MX51_OTG_OVERCURD,
> +			MX51_EHCI_POWERPINSE);
> +
> +	clrsetbits_le32(&control->phyctrl1,
> +			MX51_SYSCLOCK_MASK,
> +			MX51_SYSCLOCK_24_MHZ);
> +
> +	setbits_le32(&control->usbctrl1, MX51_H1_EXTCLKE);
> +
> +	clrsetbits_le32(&control->uh2ctrl,
> +			(MX51_H2_ULPI_IE | MX51_H2_WUE),
> +			MX51_H2_PM);
> +
> +	udelay(10000);
> +}
> +
> +#define ULPI_ADDR_SHIFT		16
> +#define ulpi_write_mask(value)	((value) & 0xff)
> +#define ulpi_read_mask(value)	(((value) >> 8) & 0xff)
> +
> +
> +void ulpi_write(u32 reg, u32 value, struct usb_ehci *ehci)
> +{
> +	if (!(readl(&ehci->ulpi_viewpoint) & ULPI_SS)) {
> +		writel(ULPI_WU, &ehci->ulpi_viewpoint);
> +		while (readl(&ehci->ulpi_viewpoint) & ULPI_WU)
> +		;

Checkpatch issue here. Please fix globally.

> +	}
> +	writel(ULPI_RWRUN | ULPI_RWCTRL | reg << ULPI_ADDR_SHIFT |
> ulpi_write_mask(value), +		&ehci->ulpi_viewpoint);
> +
> +	while (readl(&ehci->ulpi_viewpoint) & ULPI_RWRUN)
> +	;

DTTO

> +}
> +
> +u32 ulpi_read(u32 reg, struct usb_ehci *ehci)
> +{
> +	u32 tmp;
> +	if (!(readl(&ehci->ulpi_viewpoint) & ULPI_SS)) {
> +		writel(ULPI_WU, &ehci->ulpi_viewpoint);
> +		while (readl(&ehci->ulpi_viewpoint) & ULPI_WU)
> +		;

Please avoid endless loops. Please fix globally.

> +	}
> +	writel(ULPI_RWRUN | reg << ULPI_ADDR_SHIFT, &ehci->ulpi_viewpoint);
> +	do {
> +		tmp = readl(&ehci->ulpi_viewpoint);
> +	} while (tmp & ULPI_RWRUN);
> +	return ulpi_read_mask(tmp);
> +}
> +
> +void ehciX_init(u32 base, struct ulpi_regs *ulpi, struct usb_ehci *ehci)

Rename to something more sensible? ulpi_init ? Is "base" unused?

> +{
> +	u32 tmp = 0;
> +	int reg, i;
> +
> +	/* get ID from ULPI immediate registers*/

space before closing comment.

> +	for (reg = ULPI_ID_REGS_COUNT - 1; reg >= 0; reg--)
> +		tmp |= ulpi_read(reg, ehci) << (reg * 8);
> +	/* split into vendor and product ID */
> +	debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
> +
> +	/* ULPI check integrity */
> +	for (i = 0; i < 2; i++) {
> +		ulpi_write(&ulpi->scratch_write, 0x55 << i, ehci);

Magic constant should be documented.

> +		tmp = ulpi_read(&ulpi->scratch_write, ehci);
> +
> +		if (tmp != (0x55 << i)) {
> +			printf("ULPI integrity check failed\n");
> +			return;
> +		}
> +	}

[...]

> +
> +int ehci_hcd_init(void)
> +{
> +	struct usb_ehci *ehci;
> +	struct usb_control_regs *control;
> +	struct ulpi_regs *ulpi;
> +
> +	/* Init iMX51 EHCI */
> +	efika_usb_phy_reset();
> +	efika_usb_hub_reset();
> +	efika_usb_enable_devices();
> +
> +	control = (struct usb_control_regs *)(OTG_BASE_ADDR +
> +		 MX51_CTRL_REGS);

mx5_usb_control_regs might be a more fitting name for the structure.

> +	control_regs_setup(control);
> +
> +	ulpi = (struct ulpi_regs *)(0);

No need for parenthesis around 0.

> +	/* Init EHCI core */
> +	ehci = (struct usb_ehci *)(OTG_BASE_ADDR +
> +		(MX51_REGISTER_LAYOUT_LENGTH * CONFIG_MXC_USB_PORT));
> +	hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
> +	hcor = (struct ehci_hcor *)((uint32_t) hccr +
> +			HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> +	setbits_le32(&ehci->usbmode, CM_HOST);
> +	setbits_le32(&ehci->control, USB_EN);
> +
> +	switch (CONFIG_MXC_USB_PORT) {
> +	case 0:
> +		ehci0_init(ehci);
> +	case 1:
> +		ehci1_init(ehci, ulpi);
> +#ifdef	MACH_EFIKASB
> +	case 2:
> +		ehci2_init(ehci, ulpi);
> +#endif
> +	};

Fallthrough here isn't intended I presume.

> +
> +	/* EfikaMX USB has issues ... */
> +	udelay(10000);
> +

Too many newlines.

> +
> +	return 0;
> +}
> +
> +int ehci_hcd_stop(void)
> +{
> +	return 0;
> +}
> diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c
> index 5be1f6c..0f84ae0 100644
> --- a/board/efikamx/efikamx.c
> +++ b/board/efikamx/efikamx.c
> @@ -489,6 +489,15 @@ static inline void setup_iomux_ata(void) { }
>  #endif
> 
>  /*
> + * EHCI USB
> + */
> +#ifdef	CONFIG_CMD_USB
> +extern void setup_iomux_usb(void);
> +#else
> +static inline void setup_iomux_usb(void) { }
> +#endif

Avoid using extern.

> +
> +/*
>   * LED configuration
>   */
>  void setup_iomux_led(void)
> @@ -621,6 +630,7 @@ int board_late_init(void)
> 
>  	setup_iomux_led();
>  	setup_iomux_ata();
> +	setup_iomux_usb();
> 
>  	efikamx_toggle_led(EFIKAMX_LED_BLUE);
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 2197119..52b98c2 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -26,6 +26,7 @@
>  #include <asm/io.h>
>  #include <malloc.h>
>  #include <watchdog.h>
> +#include <usb/ehci-fsl.h>
> 
>  #include "ehci.h"
> 
> @@ -709,8 +710,26 @@ ehci_submit_root(struct usb_device *dev, unsigned long
> pipe, void *buffer, * usb 2.0 specification say 50 ms resets on
>  				 * root
>  				 */
> +				/* wait_ms(50); */
> +
> +#if 1
> +	struct usb_ehci *ehci = (struct usb_ehci *)(OTG_BASE_ADDR +
> +		(MX51_REGISTER_LAYOUT_LENGTH));
> +	extern u32 ulpi_read(u32 reg, struct usb_ehci *ehci);
> +	extern void ulpi_write(u32 reg, u32 value, struct usb_ehci *ehci);
> +	#define XXBASE 0x73f80200
> +	/* OTG |= 1 << 4 */
> +	u32 tmp = ulpi_read(0x0a, ehci);
> +	tmp |= (1 << 4);
> +	ulpi_write(0x0a, tmp, ehci);
> +
>  				wait_ms(50);
> +
> +
>  				/* terminate the reset */
> +	reg = ehci_readl(status_reg);
> +	reg |= EHCI_PS_PE;
> +#endif

UGH, no. This needs to be fixed !!

Remy, how are we supposed to handle such cases? For this hardware, when doing 
usb port reset without this code fails the port reset (the VBUS is disabled on 
the ulpi xmitter and it's all over). That's why you need to restart the vbus 
here.

>  				ehci_writel(status_reg, reg & ~EHCI_PS_PR);
>  				/*
>  				 * A host controller must terminate the reset
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
> index a0cfbb7..de7ed00 100644
> --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -37,9 +37,6 @@
>  #endif
> 
>  #ifdef CONFIG_MX31
> -#define MX31_OTG_SIC_SHIFT	29
> -#define MX31_OTG_SIC_MASK	(0x3 << MX31_OTG_SIC_SHIFT)
> -#define MX31_OTG_PM_BIT		(1 << 24)
> 
>  #define MX31_H2_SIC_SHIFT	21
>  #define MX31_H2_SIC_MASK	(0x3 << MX31_H2_SIC_SHIFT)
> @@ -66,11 +63,11 @@ static int mxc_set_usbcontrol(int port, unsigned int
> flags)
> 
>  		switch (port) {
>  		case 0:	/* OTG port */
> -			v &= ~(MX31_OTG_SIC_MASK | MX31_OTG_PM_BIT);
> +			v &= ~(MXX1_OTG_SIC_MASK | MXX1_OTG_PM_BIT);

Replace MXX1 with MXC?

>  			v |= (flags & MXC_EHCI_INTERFACE_MASK)
> -					<< MX31_OTG_SIC_SHIFT;
> +					<< MXX1_OTG_SIC_SHIFT;
>  			if (!(flags & MXC_EHCI_POWER_PINS_ENABLED))
> -				v |= MX31_OTG_PM_BIT;
> +				v |= MXX1_OTG_PM_BIT;
> 
>  			break;
>  		case 1: /* H1 port */

Cheers