Patchwork [U-Boot] some USB cleanup on EfikaMX

login
register
mail settings
Submitter Jana Rapava
Date Sept. 8, 2011, 9:06 p.m.
Message ID <CAB+7RbE2MXooOuN3pniuBw7+K71Q0avCfxzuZXuBFfCZpvdctA@mail.gmail.com>
Download mbox | patch
Permalink /patch/113984/
State Changes Requested
Headers show

Comments

Jana Rapava - Sept. 8, 2011, 9:06 p.m.

Wolfgang Denk - Sept. 8, 2011, 10:32 p.m.
Dear Jana Rapava,

In message <CAB+7RbE2MXooOuN3pniuBw7+K71Q0avCfxzuZXuBFfCZpvdctA@mail.gmail.com> you wrote:
> --===============1315613369==
> Content-Type: multipart/alternative; boundary=00151744109ca7dd4e04ac7470e6
> 
> --00151744109ca7dd4e04ac7470e6
> Content-Type: text/plain; charset=ISO-8859-1

Please resubmit as a proper patch.
See http://www.denx.de/wiki/U-Boot/Patches for instructions.

Most important: your signed-off-by: line is missing, as is a
descriptionof which specific problem you are fixing.

> --- a/board/efikamx/efikamx-usb.c
> +++ b/board/efikamx/efikamx-usb.c
> @@ -154,6 +154,7 @@ void efika_usb_phy_reset(void)
>         gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_D27), 1);
>  }
> 
> +

Please do not add random (and excessive) white space.

> +       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);

Your mailer line-wrapped your patch and thus corrupted it.  Please
make sure to configure your mailer not to do that. Even better, use
"git format-patch" to create hte patch and "git send-email" to submit
it.

> -       tmp = readl(OTG_BASE_ADDR + 0x808);
> -       tmp &= ~(1 << 8);
> -       tmp |= 1 << 5;
> -       writel(tmp, OTG_BASE_ADDR + 0x808);
> +       tmp = readl(OTG_BASE_ADDR + MX51_PHY_CTRL0_OFFSET);
> +       tmp &= ~MX51_OTG_OVERCURD_BIT;
> +       tmp |= MX51_EHCI_POWERPINSE_BIT;
> +       writel(tmp, OTG_BASE_ADDR + MX51_PHY_CTRL0_OFFSET);

Please consider using clrsetbits_le32() here instead.

Please fix globally.

Thanks.

Best regards,

Wolfgang Denk
Stefano Babic - Sept. 9, 2011, 10:18 a.m.
On 09/08/2011 11:06 PM, Jana Rapava wrote:
> --- a/board/efikamx/efikamx-usb.c
> +++ b/board/efikamx/efikamx-usb.c
> @@ -154,6 +154,7 @@ void efika_usb_phy_reset(void)
>         gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_D27), 1);
>  }

Hi Jana,

Wolfgang has already addressed most of the issues in your patch. Some
further comments from my site:

> 
> -       tmp = readl(OTG_BASE_ADDR + 0x80c);
> -       tmp &= ~0x3;
> -       tmp |= 0x1;
> -       writel(tmp, OTG_BASE_ADDR + 0x80c);
> +       tmp = readl(OTG_BASE_ADDR + MX51_PHY_CTRL1_OFFSET);
> +       tmp &= ~0x3; /* make sure bits 0 and 1 are set to zero */

The comment adds no information - we already know that bits 0 and 1 are
set to 0, but we do not still know why. Substitute this comment with an
explanation about the meaning of the bits and the *reason* they must be
0. Or set defines as you have already done in the rest of the file.


> +       tmp &= ~(MX51_H2_ULPI_IE_BIT | MX51_H2_WUE_BIT);
> +       //Host 2 VBUS enable controlled by Host 2 controller

Do not use C++ comments.

Best regards,
Stefano Babic

Patch

--- a/board/efikamx/efikamx-usb.c
+++ b/board/efikamx/efikamx-usb.c
@@ -154,6 +154,7 @@  void efika_usb_phy_reset(void)
        gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_D27), 1);
 }

+
 /*
  * Configure control registers of the USB controller
  */
@@ -161,56 +162,56 @@  void control_regs_setup(void)
 {
        uint32_t tmp;

-       tmp = readl(OTG_BASE_ADDR + 0x800);
-       tmp &= ~((1 << 27) | (1 << 24) | (1 << 12) | (1 << 11));
-       tmp |= 1 << 8;
-       writel(tmp, OTG_BASE_ADDR + 0x800);
+       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;
+       writel(tmp, OTG_BASE_ADDR + MX51_USB_CTRL_OFFSET);

-       tmp = readl(OTG_BASE_ADDR + 0x808);
-       tmp &= ~(1 << 8);
-       tmp |= 1 << 5;
-       writel(tmp, OTG_BASE_ADDR + 0x808);
+       tmp = readl(OTG_BASE_ADDR + MX51_PHY_CTRL0_OFFSET);
+       tmp &= ~MX51_OTG_OVERCURD_BIT;
+       tmp |= MX51_EHCI_POWERPINSE_BIT;
+       writel(tmp, OTG_BASE_ADDR + MX51_PHY_CTRL0_OFFSET);

-       tmp = readl(OTG_BASE_ADDR + 0x80c);
-       tmp &= ~0x3;
-       tmp |= 0x1;
-       writel(tmp, OTG_BASE_ADDR + 0x80c);
+       tmp = readl(OTG_BASE_ADDR + MX51_PHY_CTRL1_OFFSET);
+       tmp &= ~0x3; /* make sure bits 0 and 1 are set to zero */
+       tmp |= MX51_SYSCLOCK_24_MHZ_BIT;
+       writel(tmp, OTG_BASE_ADDR + MX51_PHY_CTRL1_OFFSET);

-       tmp = readl(OTG_BASE_ADDR + 0x810);
-       tmp |= 1 << 25;
-       writel(tmp, OTG_BASE_ADDR + 0x810);
+       tmp = readl(OTG_BASE_ADDR + MX51_USB_CTRL1_OFFSET);
+       tmp |= MX51_H1_EXTCLKE_BIT;
+       writel(tmp, OTG_BASE_ADDR + MX51_USB_CTRL1_OFFSET);

-       tmp = readl(OTG_BASE_ADDR + 0x814);
-       tmp &= ~((1 << 8) | (1 << 7));
-       tmp |= 1 << 4;
-       writel(tmp, OTG_BASE_ADDR + 0x814);
+       tmp = readl(OTG_BASE_ADDR + MX51_UH2_CTRL_OFFSET);
+       tmp &= ~(MX51_H2_ULPI_IE_BIT | MX51_H2_WUE_BIT);
+       //Host 2 VBUS enable controlled by Host 2 controller
+       tmp |= MX51_H2_PM_BIT;
+       writel(tmp, OTG_BASE_ADDR + MX51_UH2_CTRL_OFFSET);

        udelay(10000);
 }

-#define        ULPI_VIEWPORT(base)     (base + 0x170)

 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)) {
+               writel(MX51_ULPI_WU_BIT, ULPI_VIEWPORT(base));
+               while (readl(ULPI_VIEWPORT(base)) & MX51_ULPI_WU_BIT);
        }
-       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 | (reg << 16) | (value &
0xff), 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 | (reg << 16), ULPI_VIEWPORT(base));
        do {
                tmp = readl(ULPI_VIEWPORT(base));
-       } while (tmp & (1 << 30));
+       } while (tmp & MX51_ULPI_RWRUN);
        return (tmp >> 8) & 0xff;
 }

diff --git a/include/usb/ehci-fsl.h b/include/usb/ehci-fsl.h
index 67600ed..3af7928 100644
--- a/include/usb/ehci-fsl.h
+++ b/include/usb/ehci-fsl.h
@@ -169,9 +169,46 @@ 
 #define CONFIG_SYS_FSL_USB_ADDR CONFIG_SYS_MPC512x_USB_ADDR
 #endif

+/* Register offsets for MX51 */
+#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
+
+/* Some USB_CTRL register bits*/
+#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)
+
+/* Some PHY_CTRL_0 register bits */
+#define MX51_OTG_OVERCURD_BIT  (1 << 8)
+#define MX51_EHCI_POWERPINSE_BIT       (1 << 5)
+
+/* Some PHY_CTRL_1 register bits */
+#define MX51_SYSCLOCK_24_MHZ_BIT       (1 << 0)
+
+/* Some USB_CTRL_1 register bits*/
+#define MX51_H1_EXTCLKE_BIT    (1 << 25)
+
+/* Some USB Host 2 CTRL register bits*/
+#define MX51_H2_ULPI_IE_BIT    (1 << 8)
+#define MX51_H2_WUE_BIT                (1 << 7)
+#define MX51_H2_PM_BIT         (1 << 4)
+
+#define        ULPI_VIEWPORT(base)     (base + 0x170)
+/* 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)
+
 /*
  * USB Registers
  */
+
 struct usb_ehci {
        u32     id;             /* 0x000 - Identification register */
        u32     hwgeneral;      /* 0x004 - General hardware parameters */
@@ -241,5 +278,4 @@  struct usb_ehci {
        u32     control;        /* 0x500 - Control */
        u8      res13[0xafc];
 };
-
 #endif /* _EHCI_FSL_H */