Dear Srikanth Reddy Vintha,
In message <1344846046-11111-2-git-send-email-srikanth.re...@lntinfotech.com>
you wrote:
> Signed-off-by: Srikanth Reddy Vintha <srikanth.re...@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: w...@denx.de
Keep your eyes wide open before marriage, half shut afterwards.
-- Benjamin Franklin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
