[U-Boot] [PATCH]ppc4xx/Canyonlands added USB board callbacks

[Rupjyoti Sarmah]

Thanks for your inputs. I will do the necessary changes are resubmit.

Regards,
Rup

-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de]
Sent: Thursday, June 17, 2010 1:24 AM
To: Rupjyoti Sarmah
Cc: u-boot at lists.denx.de; rupjyoti.sarmah at gmail.com; sr at denx.de;
rsarmah at apm.com
Subject: Re: [U-Boot] [PATCH]ppc4xx/Canyonlands added USB board callbacks

Dear Rupjyoti Sarmah,

In message <201006161734.o5GHYa6I014963 at amcc.com> you wrote:
>
> Functions added to support board callbacks for USB init. This
> isolates USB manipulations such that it is only touched if USB is
> used by U-Boot.
>
> Signed-off-by: Dave Mitchell <dmitchell at appliedmicro.com>
> Signed-off-by: Rupjyoti Sarmah <rsarmah at appliedmicro.com>
> ---
>  board/amcc/canyonlands/canyonlands.c |   79
+++++++++++++++++++++++++++-------
>  include/configs/canyonlands.h        |    1 +
>  2 files changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/board/amcc/canyonlands/canyonlands.c
b/board/amcc/canyonlands/canyonlands.c
> index 23874d2..2e30a32 100644
> --- a/board/amcc/canyonlands/canyonlands.c
> +++ b/board/amcc/canyonlands/canyonlands.c
> @@ -34,6 +34,18 @@ extern flash_info_t
flash_info[CONFIG_SYS_MAX_FLASH_BANKS]; /* info for FLASH ch
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +
> +struct ep460c_bcsr {
> +	u8 cpld_rev,
> +		led_user,
> +		board_status,
> +		reset_ctrl,
> +		flash_ctrl,
> +		eth_ctrl,
> +		usb_ctrl,
> +		irq_ctrl;
> +};

Please use standard style to describe structs, and mind indetation, i. e.
write:

	struct ep460c_bcsr {
		u8	cpld_rev,
		u8	led_user,
		u8	board_status,
		...
		u8	irq_ctrl,
	};

>  #define CONFIG_SYS_BCSR3_PCIE		0x10

CONFIG_SYS_* variable sshould not be defined in board code; they are
intended to be used in board config files only.

> +#define BCSR_CPLDREV		0x00
> +#define BCSR_BRDSTS		0x03
> +#define BCSR_FLASHCTRL		0x05
> +#define BCSR_ETHCTRL		0x06
> +#define BCSR_USBCTRL		0x07
> +#define BCSR_USBCTRL_OTG_RST	0x32
> +#define BCSR_USBCTRL_HOST_RST	0x01

Please add comments what these magic numbers do.

>  #if !defined(CONFIG_ARCHES)
>  	/* Enable ethernet and take out of reset */
> +
>  	out_8((void *)CONFIG_SYS_BCSR_BASE + 6, 0);

Remove this blank line, please.

>  	/* Remove NOR-FLASH, NAND-FLASH & EEPROM hardware write protection
*/
> -	out_8((void *)CONFIG_SYS_BCSR_BASE + 5, 0);
> -
> -	/* Enable USB host & USB-OTG */
> -	out_8((void *)CONFIG_SYS_BCSR_BASE + 7, 0);
> +	bcsr_data->flash_ctrl = 0;

NAK. Please always use proper I/O accessors to access device
registers.

> -	if (pvr_460ex()) {
> -		/*
> -		 * Configure USB-STP pins as alternate and not GPIO
> -		 * It seems to be neccessary to configure the STP pins as
GPIO
> -		 * input at powerup (perhaps while USB reset is asserted).
So
> -		 * we configure those pins to their "real" function now.
> -		 */
> -		gpio_config(16, GPIO_OUT, GPIO_ALT1, GPIO_OUT_1);
> -		gpio_config(19, GPIO_OUT, GPIO_ALT1, GPIO_OUT_1);
> -	}

I am not sure if this is really a good idea to remove this code here.

> +	/* Enable USB host & USB-OTG */
> +	bcsr_data->usb_ctrl &= ~(BCSR_USBCTRL_OTG_RST |
BCSR_USBCTRL_HOST_RST);

Please always use proper I/O accessors! Fix globally!

> +	/*
> +	 * Configure USB-STP pins as alternate and not GPIO.
> +	 */
> +	gpio_config(16, GPIO_OUT, GPIO_ALT1, GPIO_OUT_1);
> +	gpio_config(19, GPIO_OUT, GPIO_ALT1, GPIO_OUT_1);

Isn't this too late? And maybe you want to keep the comment that
explains what is being done, and why?

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
"Well I don't see why I have to make one man  miserable  when  I  can
make so many men happy."              - Ellyn Mustard, about marriage