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

Marek Vasut
2011-09-12T19:25:30Z

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