Thanks for your inputs. I will do the necessary changes are resubmit. Regards, Rup
-----Original Message----- From: Wolfgang Denk [mailto:w...@denx.de] Sent: Thursday, June 17, 2010 1:24 AM To: Rupjyoti Sarmah Cc: u-boot@lists.denx.de; rupjyoti.sar...@gmail.com; s...@denx.de; rsar...@apm.com Subject: Re: [U-Boot] [PATCH]ppc4xx/Canyonlands added USB board callbacks 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
