Dear Ajay Bhargav,
In message <1310106168-17166-4-git-send-email-ajay.bhar...@einfochips.com> you
wrote:
...
> diff --git a/arch/arm/cpu/arm926ejs/armada100/cpu.c
> b/arch/arm/cpu/arm926ejs/armada100/cpu.c
> index c21938e..2db42a0 100644
> --- a/arch/arm/cpu/arm926ejs/armada100/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/armada100/cpu.c
...
> +
> +/* Both USB host as well as USB ETH requires this function.
> + * So moving it from usb eth file (usbeth/mv_u2o_ctl.c) to this common file
> */
> +/* CHIP ID:
> + * Z0: 0x00a0c910
> + * Y0: 0x00f0c910
> + */
Incorrect multiline comment style. Please fix globally.
...
> --- /dev/null
> +++ b/arch/arm/cpu/arm926ejs/armada100/pxa168_u2h.c
> @@ -0,0 +1,154 @@
...
> +#undef DEBUG
Please don't undefine what is not defined anyway.
> +#if DEBUG
> +static unsigned int usb_debug = DEBUG;
NAK. "DEBUG' in U-Boot is either not defined, or defined without a
specific value. This code assumes it is defined as a numeric value,
which is an incorrect assumption.
> +#else
> +#define usb_debug 0 /* gcc will remove all the debug code for us */
> +#endif
> +
> +/*****************************************************************************
> + * The registers read/write routines
> +
> ******************************************************************************/
> +static unsigned usb_sph_get(unsigned *base, unsigned offset)
> +{
> + return readl(base + (offset>>2));
> +}
NAK. Please use C structs, and use the standard I/O accessors
directly.
> +static void usb_sph_set(unsigned *base, unsigned offset, unsigned value)
> +{
> + unsigned int reg;
> +
> + if (usb_debug)
> + printf("base %p off %x base+off %p read %x\n", base, offset,
> + (base + (offset>>2)), *(unsigned *)(base + (offset>>2)));
Braces needed for multiline statements. Please fix globally.
> +static void usb_sph_clear(unsigned *base, unsigned offset, unsigned value)
...
> +static void usb_sph_write(unsigned *base, unsigned offset, unsigned value)
NAK, see above.
> + /* Turn on Main PMU clocks ACGR */
> + writel(0x1EFFFF, 0xD4051024);
Please provide #defines for these magic numbers.
> + /* USB clk reset */
> + writel(0x18, PMUA_USB_CLK_RES_CTRL);
> + writel(0x1b, PMUA_USB_CLK_RES_CTRL);
> +
> + /* enable the pull up */
> + if (!cpu_is_pxa910_168()) {
> + if (pxa910_is_z0()) {
> + writel((1<<20), (0xc0000004));
> + } else {
> + tmp = readl(0xd4207004);
> + tmp |= (1<<20);
> + writel(tmp, (0xd4207004));
Ditto.
...
> --- a/arch/arm/include/asm/arch-armada100/cpu.h
> +++ b/arch/arm/include/asm/arch-armada100/cpu.h
> @@ -28,6 +28,36 @@
> #include <asm/io.h>
> #include <asm/system.h>
>
> +#define CPUID_ID 0
> +
> +#define __stringify_1(x) #x
> +#define __stringify(x) __stringify_1(x)
> +
> +#define read_cpuid(reg)
> \
> + ({ \
> + unsigned int __val; \
> + asm("mrc p15, 0, %0, c0, c0, " __stringify(reg) \
> + : "=r" (__val) \
> + : \
> + : "cc"); \
> + __val; \
> + })
> +
> +#define __cpu_is_pxa910_168(id)
> \
> + ({ \
> + unsigned int _id = (id) & 0xffff; \
> + _id == 0x9263 || _id == 0x8400; \
> + })
> +
> +
> +#define cpu_is_pxa910_168() \
> + ({ \
> + unsigned int id = read_cpuid(CPUID_ID); \
> + __cpu_is_pxa910_168(id); \
> + })
CodingStyle says:
"Generally, inline functions are preferable to macros resembling functions."
...
> +/* ASPEN */
> +#define UTMI_REVISION 0x0
> +#define UTMI_CTRL 0x4
> +#define UTMI_PLL 0x8
> +#define UTMI_TX 0xc
> +#define UTMI_RX 0x10
> +#define UTMI_IVREF 0x14
> +#define UTMI_T0 0x18
> +#define UTMI_T1 0x1c
> +#define UTMI_T2 0x20
> +#define UTMI_T3 0x24
> +#define UTMI_T4 0x28
> +#define UTMI_T5 0x2c
> +#define UTMI_RESERVE 0x30
> +#define UTMI_USB_INT 0x34
> +#define UTMI_DBG_CTL 0x38
> +#define UTMI_OTG_ADDON 0x3c
etc.
NAK. Please use C structs instead.
...
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -46,6 +46,7 @@ COBJS-$(CONFIG_USB_EHCI_IXP4XX) += ehci-ixp.o
> COBJS-$(CONFIG_USB_EHCI_KIRKWOOD) += ehci-kirkwood.o
> COBJS-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o
> COBJS-$(CONFIG_USB_EHCI_VCT) += ehci-vct.o
> +COBJS-$(CONFIG_USB_EHCI_PXA168) += ehci-pxa168.o
Please keep lists sorted. Fix globally.
...
> +/*
> + * Destroy the appropriate control structures corresponding
> + * the the EHCI host controller.
> + */
> +int ehci_hcd_stop(void)
> +{
> + return 0;
> +}
This is probably not sufficient to really stop the EHCI controller,
which is a mandatory action here.
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
Like winter snow on summer lawn, time past is time gone.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
