[U-Boot] [PATCH 1/2 v2] USB cleanup for EfikaMX

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

On Monday, September 12, 2011 07:20:40 PM Jana Rapava wrote:
> /board/efikamx/efikamx-usb.c: cleanup
> 
> Signed-off-by: Jana Rapava <fermata7@gmail.com>
> ---
>  board/efikamx/efikamx-usb.c |  111
> +++++++++++++++++++++++-------------------- include/usb/ehci-fsl.h      | 
>  49 +++++++++++++++++++
>  2 files changed, 108 insertions(+), 52 deletions(-)

Dear Jana Rapava,

1) PATCH 2/2 is missing.
2) This doesn't apply on mainline, continue as discussed on IRC -- ie. by 
merging the patches.

> 
> diff --git a/board/efikamx/efikamx-usb.c b/board/efikamx/efikamx-usb.c
> index 70d0daa..a94a64f 100644
> --- a/board/efikamx/efikamx-usb.c
> +++ b/board/efikamx/efikamx-usb.c
> @@ -161,57 +161,70 @@ void control_regs_setup(void)
>  {

[...]

> +	tmp = readl(OTG_BASE_ADDR + MX51_USB_CTRL_OFFSET);
> +	tmp &= ~(MX51_OTG_WUE_BIT | MX51_OTG_PM_BIT | MX51_H1_ULPI_IE_BIT |
> MX51_H1_WUE_BIT); +	tmp |= MX51_H1_PM_BIT;

I assume this is ULPI specific, not MX51 specific.

> +	writel(tmp, OTG_BASE_ADDR + MX51_USB_CTRL_OFFSET);

Don't use offset-based access, but struct based access, please fix globally.

struct otg_regs {
uint32_t reg1;
uint32_t reg2;
...
};

then

struct otg_regs *regs = (struct otg_regs *)REG_ADDR;

writel(val, &regs->reg1);

[...]
> 
> -#define	ULPI_VIEWPORT(base)	(base + 0x170)
> +
> +#define ULPI_VIEWPORT(base)	(base + 0x170)

struct ehci probably already contains the correct offset of the ulpi viewport. 
You just need to cast struct ehci to correct offset of the port.

> +
> +/* ULPI viewport control bits */
> +#define MX51_ULPI_WU_BIT	(1 << 31)
> +#define MX51_ULPI_SS_BIT	(1 << 27)
> +#define MX51_ULPI_RWRUN		(1 << 30)
> +#define MX51_ULPI_RWCTRL	(1 << 29)

See above.

> +
> +#define shift_to_ulpiaddr(reg)	(reg << 16)

#define ULPI_something_ADDR_OFFSET	16 ?

> +#define ulpi_write_mask(value)	(value & 0xff)
> +#define ulpi_read_mask(value)	((value >> 8) & 0xff)

Missing parenthesis around value.

> +
> 
>  void ulpi_write(u32 base, u32 reg, u32 value)
>  {
> -	if (!(readl(ULPI_VIEWPORT(base)) & (1 << 27))) {
> -		writel(1 << 31, ULPI_VIEWPORT(base));
> -		while (readl(ULPI_VIEWPORT(base)) & (1 << 31));
> +	if (!(readl(ULPI_VIEWPORT(base)) & MX51_ULPI_SS_BIT)) {

See above.

> +		writel(MX51_ULPI_WU_BIT, ULPI_VIEWPORT(base));
> +		while (readl(ULPI_VIEWPORT(base)) & MX51_ULPI_WU_BIT);

Semicolon should be on the new line. Please fix globally.

>  	}
> -	writel((1 << 30) | (1 << 29) | (reg << 16) | (value & 0xff),
> ULPI_VIEWPORT(base)); -	while (readl(ULPI_VIEWPORT(base)) & (1 << 30));
> +	writel(MX51_ULPI_RWRUN | MX51_ULPI_RWCTRL | shift_to_ulpiaddr(reg) |
> ulpi_write_mask(value), ULPI_VIEWPORT(base)); +
> +	while (readl(ULPI_VIEWPORT(base)) & MX51_ULPI_RWRUN);
>  }
> 
>  u32 ulpi_read(u32 base, u32 reg)
>  {
>  	u32 tmp;
> -	if (!(readl(ULPI_VIEWPORT(base)) & (1 << 27))) {
> -		writel(1 << 31, ULPI_VIEWPORT(base));
> -		while (readl(ULPI_VIEWPORT(base)) & (1 << 31));
> +	if (!(readl(ULPI_VIEWPORT(base)) & MX51_ULPI_SS_BIT)) {
> +		writel(MX51_ULPI_WU_BIT, ULPI_VIEWPORT(base));
> +		while (readl(ULPI_VIEWPORT(base)) & MX51_ULPI_WU_BIT);
>  	}
> -	writel((1 << 30) | (reg << 16), ULPI_VIEWPORT(base));
> +	writel(MX51_ULPI_RWRUN | shift_to_ulpiaddr(reg), ULPI_VIEWPORT(base));
>  	do {
>  		tmp = readl(ULPI_VIEWPORT(base));
> -	} while (tmp & (1 << 30));
> -	return (tmp >> 8) & 0xff;
> +	} while (tmp & MX51_ULPI_RWRUN);
> +	return ulpi_read_mask(tmp);
>  }
> 
>  #define	ULPI_CTL_WRITE_OFF	0x00
> @@ -229,8 +242,9 @@ void ehciX_init(u32 base)
>  	int reg, i;
> 
>  	/* ULPI INIT */
> -	for (reg = 3; reg >=0; reg--)
> +	for (reg = MX51_USB_HOST_COUNT-1; reg >= 0; reg--)

COUNT-1 missing spaces.

>  		tmp |= ulpi_read(base, reg) << (reg * 8);
> +	/* split ID into first and second half */

debug() ?

What's "first" and "second" half, do you mean vendor and product ID ?

>  	printf("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
> 
>  	/* ULPI check integrity */
> @@ -238,7 +252,6 @@ void ehciX_init(u32 base)
>  		ulpi_write(base, ULPI_SCRATCH, 0x55 << i);
>  		tmp = ulpi_read(base, ULPI_SCRATCH);
> 
> -		// check
>  		if (tmp != (0x55 << i)) {
>  			printf("ULPI integrity check failed\n");
>  			return;
> @@ -264,8 +277,7 @@ void ehciX_init(u32 base)
> 
>  void ehci0_init(void)
>  {
> -	/* Set PORTSC to 16b UTMI mode */
> -	writel((1 << 28), OTG_BASE_ADDR + 0x184);
> +	writel(MX51_16BIT_UTMI_BIT, OTG_BASE_ADDR + MX51_OTG_PORTSC1_OFFSET);

See above.

>  }
> 
>  void ehci1_init(void)
> @@ -284,24 +296,21 @@ void ehci1_init(void)
>  	mxc_iomux_set_pad(MX51_PIN_USBH1_STP, USB_PAD_CONFIG);
>  	udelay(10000);
> 
> -	tmp = readl(OTG_BASE_ADDR + 0x340);
> -	tmp &= ~(0xff << 16);
> -	writel(tmp, OTG_BASE_ADDR + 0x340);
> +	tmp = readl(OTG_BASE_ADDR + MX51_UH1_USBCMD_OFFSET);
> +	tmp &= MX51_ITC_IMMEDIATE_MASK;
> +	writel(tmp, OTG_BASE_ADDR + MX51_UH1_USBCMD_OFFSET);

Try clrsetbits_le32() and other calls from asm/io.h

> 
>  	udelay(10000);
> 
> -	/* Set PORTSC to ULPI mode */
> -	writel((2 << 30), OTG_BASE_ADDR + 0x384);
> +	writel(MX51_ULPI_MODE_MASK, OTG_BASE_ADDR + MX51_UH1_PORTSC1_OFFSET);
>  	udelay(10000);
> 
> -	ehciX_init(OTG_BASE_ADDR + 0x200);
> +	ehciX_init(OTG_BASE_ADDR + MX51_UH1_ID_OFFSET);
>  }
> 
> 
>  void ehci2_init(void)
>  {
> -	u32 tmp;
> -
>  	mxc_request_iomux(MX51_PIN_EIM_A26, IOMUX_CONFIG_ALT1);
>  	mxc_iomux_set_pad(MX51_PIN_EIM_A26, PAD_CTL_DRV_HIGH |
>  				PAD_CTL_PKE_ENABLE | PAD_CTL_SRE_FAST);
> @@ -313,11 +322,10 @@ void ehci2_init(void)
>  	mxc_request_iomux(MX51_PIN_EIM_A26, IOMUX_CONFIG_ALT2);
>  	mxc_iomux_set_pad(MX51_PIN_EIM_A26, USB_PAD_CONFIG);
> 
> -	/* Set PORTSC to ULPI mode */
> -	writel((2 << 30), OTG_BASE_ADDR + 0x584);
> +	writel(MX51_ULPI_MODE_MASK, OTG_BASE_ADDR + MX51_UH2_PORTSC1_OFFSET);

I think struct ehci defines this offset.

>  	udelay(10000);
> 
> -	ehciX_init(OTG_BASE_ADDR + 0x400);
> +	ehciX_init(OTG_BASE_ADDR + MX51_UH2_ID_OFFSET);
>  }
> 
>  int ehci_hcd_init(void)
> @@ -338,10 +346,9 @@ int ehci_hcd_init(void)
>  	/* EfikaMX USB has issues ... */
>  	udelay(10000);
> 
> -
>  	/* Init EHCI core */
>  	ehci = (struct usb_ehci *)(OTG_BASE_ADDR +
> -		(0x200 * CONFIG_MXC_USB_PORT));
> +		(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)));
> diff --git a/include/usb/ehci-fsl.h b/include/usb/ehci-fsl.h
> index 67600ed..5617275 100644
> --- a/include/usb/ehci-fsl.h
> +++ b/include/usb/ehci-fsl.h
> @@ -169,6 +169,55 @@
>  #define CONFIG_SYS_FSL_USB_ADDR CONFIG_SYS_MPC512x_USB_ADDR
>  #endif
> 
> +#define MX51_USB_HOST_COUNT	4
> +#define MX51_REGISTER_LAYOUT_LENGTH	0x200
> +
> +/* Register offsets for MX51 */
> +#define MX51_UH1_ID_OFFSET	0x200
> +#define MX51_UH2_ID_OFFSET	0x400
> +
> +#define MX51_USB_CTRL_OFFSET	0x800
> +#define MX51_PHY_CTRL0_OFFSET	0x808
> +#define MX51_PHY_CTRL1_OFFSET	0x80c
> +#define MX51_USB_CTRL1_OFFSET	0x810
> +#define MX51_UH2_CTRL_OFFSET	0x814

MX5_, not MX51, I think this is valid for MX53 too.

> +
> +#define MX51_OTG_PORTSC1_OFFSET	0x184
> +#define MX51_UH1_PORTSC1_OFFSET	0x384
> +#define MX51_UH2_PORTSC1_OFFSET	0x584
> +
> +#define MX51_UH1_USBCMD_OFFSET	0x340

Use struct ehci where this is used with proper cast.
> +
> +/* USB_CTRL register bits of interest*/
> +#define MX51_OTG_WUE_BIT	(1 << 27)
> +#define MX51_OTG_PM_BIT		(1 << 24)
> +#define MX51_H1_ULPI_IE_BIT	(1 << 12)
> +#define MX51_H1_WUE_BIT		(1 << 11)
> +#define MX51_H1_PM_BIT		(1 << 8)
> +
> +/* PHY_CTRL_0 register bits of interest */
> +#define MX51_OTG_OVERCURD_BIT		(1 << 8)
> +#define MX51_EHCI_POWERPINSE_BIT	(1 << 5)
> +
> +/* PHY_CTRL_1 register bits of interest */
> +#define MX51_SYSCLOCK_24_MHZ_BIT	(1 << 0)
> +#define MX51_SYSCLOCK_MASK		(0xffffffff << 0)

don't use << 0 here. Is this even needed?

> +
> +/* USB_CTRL_1 register bits of interest */
> +#define MX51_H1_EXTCLKE_BIT	(1 << 25)
> +
> +/* USB Host 2 CTRL register bits of interest */
> +#define MX51_H2_ULPI_IE_BIT	(1 << 8)
> +#define MX51_H2_WUE_BIT		(1 << 7)
> +#define MX51_H2_PM_BIT		(1 << 4)
> +
> +/* PORTSCx bits of interest */
> +#define MX51_ULPI_MODE_MASK	(2 << 30)
> +#define MX51_16BIT_UTMI_BIT	(1 << 28)
> +
> +/* USBCMD bits of interest */
> +#define MX51_ITC_IMMEDIATE_MASK	(~(0xff << 16))
> +
>  /*
>   * USB Registers
>   */

Cheers