[U-Boot] [PATCH 4/4] Armada100: Adds support for USB on gplugD

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <1310106168-17166-4-git-send-email-ajay.bhargav at 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: wd at denx.de
Like winter snow on summer lawn, time past is time gone.