Dear Rupjyoti Sarmah,
In message <201006161734.o5ghya6i014...@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 <dmitch...@appliedmicro.com>
> Signed-off-by: Rupjyoti Sarmah <rsar...@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: w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
