[U-Boot] [PATCH 1/5] MSM7630: USB Gadget support

[Wolfgang Denk]

Dear Srikanth Reddy Vintha,

In message <1344846046-11111-2-git-send-email-srikanth.reddy at lntinfotech.com> you wrote:
> Signed-off-by: Srikanth Reddy Vintha <srikanth.reddy at lntinfotech.com>
> ---
>  arch/arm/include/asm/arch-msm7630/irqs.h |  162 +++++++++
>  drivers/serial/usbtty.h                  |    2 +
>  drivers/usb/gadget/Makefile              |    1 +
>  drivers/usb/gadget/msm_udc.c             |  540 ++++++++++++++++++++++++++++++
>  include/configs/msm7630_surf.h           |   11 +-
>  include/usb/msm_udc.h                    |  178 ++++++++++
>  6 files changed, 891 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/include/asm/arch-msm7630/irqs.h
>  create mode 100644 drivers/usb/gadget/msm_udc.c
>  create mode 100644 include/usb/msm_udc.h
...
> diff --git a/drivers/serial/usbtty.h b/drivers/serial/usbtty.h
> index e449cd7..8321447 100644
> --- a/drivers/serial/usbtty.h
> +++ b/drivers/serial/usbtty.h
> @@ -35,6 +35,8 @@
>  #include <usb/pxa27x_udc.h>
>  #elif defined(CONFIG_SPEAR3XX) || defined(CONFIG_SPEAR600)
>  #include <usb/spr_udc.h>
> +#elif defined(CONFIG_MSM_UDC)
> +#include <usb/msm_udc.h>
>  #endif

Please keep list sorted.

>  #include <version.h>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 64b091f..44ffa8a 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -44,6 +44,7 @@ COBJS-$(CONFIG_OMAP1610) += omap1510_udc.o
>  COBJS-$(CONFIG_MPC885_FAMILY) += mpc8xx_udc.o
>  COBJS-$(CONFIG_CPU_PXA27X) += pxa27x_udc.o
>  COBJS-$(CONFIG_SPEARUDC) += spr_udc.o
> +COBJS-$(CONFIG_MSM_UDC) += msm_udc.o

Ditto.

...
> +#ifdef DEBUG
> +/* Debug output must not use stdout as that can cause an infinite loop when
> + * stdout is usbtty */

Incorrect multiline comment style. Please fix globally.

> +#define udcdbg serial_printf
> +#else
> +#define udcdbg(fmt, args...)
> +#endif

Why cannot you use the generic debug() facilities?

...
> +	writel(EPT_RX(0), USB_ENDPTSETUPSTAT);
...
> +			writel(0, USB_ENDPTCTRL(1));
...

This looks very much as if you weren't using I/O accessors correctly.

...
> +	if ((dQH->next != TERMINATE) && (dQH->next != 0))
> +		udcdbg(",dQH busy\n");
> +	else {
> +		int bit = direction ? EPT_TX(num) : EPT_RX(num);
> +		dQH->next = (unsigned) dTD;
> +		dQH->info = 0;
> +		udcdbg(",bit=%x,dQH=%p\n", bit, dQH);
> +		writel(bit, USB_ENDPTPRIME);
> +	}

See CodingStyle: if one branch of a conditional statement needs
braces, then braces shall be used for both branches.  Please fix
globally.


> +void udc_disconnect(void)
> +{
> +	udcdbg("Disconnect USB\n");
> +	writel(0x00080000, USB_USBCMD); /* disable pullup */

Please define nice, readable symbolic constants for such magioc
numbers.  Please fix globally.

> +	mdelay(10);

Add a comment why this delay is needed, and why it has to be 10 ms.

...
> +#define USB_ID               (MSM_USB_BASE + 0x0000)
> +#define USB_HWGENERAL        (MSM_USB_BASE + 0x0004)
> +#define USB_HWHOST           (MSM_USB_BASE + 0x0008)
> +#define USB_HWDEVICE         (MSM_USB_BASE + 0x000C)
> +#define USB_HWTXBUF          (MSM_USB_BASE + 0x0010)
> +#define USB_HWRXBUF          (MSM_USB_BASE + 0x0014)
> +#define USB_SBUSCFG          (MSM_USB_BASE + 0x0090)
> +
> +#define USB_AHB_BURST        (MSM_USB_BASE + 0x0090)
> +#define USB_AHB_MODE         (MSM_USB_BASE + 0x0098)
> +#define USB_CAPLENGTH        (MSM_USB_BASE + 0x0100) /* 8 bit */
> +#define USB_HCIVERSION       (MSM_USB_BASE + 0x0102) /* 16 bit */
> +#define USB_HCSPARAMS        (MSM_USB_BASE + 0x0104)
> +#define USB_HCCPARAMS        (MSM_USB_BASE + 0x0108)
> +#define USB_DCIVERSION       (MSM_USB_BASE + 0x0120)    /* 16 bit */
> +#define USB_USBCMD           (MSM_USB_BASE + 0x0140)
> +#define USB_USBSTS           (MSM_USB_BASE + 0x0144)
> +#define USB_USBINTR          (MSM_USB_BASE + 0x0148)
> +#define USB_FRINDEX          (MSM_USB_BASE + 0x014C)
> +#define USB_DEVICEADDR       (MSM_USB_BASE + 0x0154)
> +#define USB_ENDPOINTLISTADDR (MSM_USB_BASE + 0x0158)
> +#define USB_BURSTSIZE        (MSM_USB_BASE + 0x0160)
> +#define USB_TXFILLTUNING     (MSM_USB_BASE + 0x0164)
> +#define USB_ULPI_VIEWPORT    (MSM_USB_BASE + 0x0170)
> +#define USB_ENDPTNAK         (MSM_USB_BASE + 0x0178)
> +#define USB_ENDPTNAKEN       (MSM_USB_BASE + 0x017C)
> +#define USB_PORTSC           (MSM_USB_BASE + 0x0184)
> +#define USB_OTGSC            (MSM_USB_BASE + 0x01A4)
> +#define USB_USBMODE          (MSM_USB_BASE + 0x01A8)
> +#define USB_ENDPTSETUPSTAT   (MSM_USB_BASE + 0x01AC)
> +#define USB_ENDPTPRIME       (MSM_USB_BASE + 0x01B0)
> +#define USB_ENDPTFLUSH       (MSM_USB_BASE + 0x01B4)
> +#define USB_ENDPTSTAT        (MSM_USB_BASE + 0x01B8)
> +#define USB_ENDPTCOMPLETE    (MSM_USB_BASE + 0x01BC)
> +#define USB_ENDPTCTRL(n)     (MSM_USB_BASE + 0x01C0 + (4 * (n)))

I expected this to be coming.  We don;t allow access to device
registers through base address plus offset notations.  Please define C
structs to describe your peripherals, and uses porper I/O accessors
with these.

Please fix globally.


I stop my review here.  There are too many global changes needed;
please fix these, then resubmit.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Keep your eyes wide open before marriage, half shut afterwards.
                                                 -- Benjamin Franklin