Hi 2009/9/22 Fred Fan <fanyef...@gmail.com>: > Hi Magnus Liljia: > Thanks for your comments. > Best Regards > Fred > > 2009/9/22, Magnus Lilja <lilja.mag...@gmail.com>: >> >> Hi >> >> >> I've scanned the patch briefly and have some comments below. >> >> gareat...@gmail.com wrote: >> > diff --git a/MAKEALL b/MAKEALL >> > index edebaea..ed8c437 100755 >> > --- a/MAKEALL >> > +++ b/MAKEALL >> <snip> > > > Does means use new board name?
Heh, no. <snip> just means that I've removed parts of your patch (those parts which I don't have any comments on). Sorry for the confusion. >> > +++ b/board/freescale/imx51/Makefile >> <snip> >> Does means use new board name? The board name should be used instead of the imx51 name. >> > + mxc_request_iomux(MX51_PIN_UART1_RXD, IOMUX_CONFIG_ALT0); >> > + mxc_iomux_set_pad(MX51_PIN_UART1_RXD, pad | PAD_CTL_SRE_FAST); >> > + mxc_request_iomux(MX51_PIN_UART1_TXD, IOMUX_CONFIG_ALT0); >> > + mxc_iomux_set_pad(MX51_PIN_UART1_TXD, pad | PAD_CTL_SRE_FAST); >> > + mxc_request_iomux(MX51_PIN_UART1_RTS, IOMUX_CONFIG_ALT0); >> > + mxc_iomux_set_pad(MX51_PIN_UART1_RTS, pad); >> > + mxc_request_iomux(MX51_PIN_UART1_CTS, IOMUX_CONFIG_ALT0); >> > + mxc_iomux_set_pad(MX51_PIN_UART1_CTS, pad); >> > +} >> > + >> > +void setup_nfc(void) >> > +{ >> > + /* Enable NFC IOMUX */ >> > + mxc_request_iomux(MX51_PIN_NANDF_CS0, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS1, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS2, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS3, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS4, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS5, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS6, IOMUX_CONFIG_ALT0); >> > + mxc_request_iomux(MX51_PIN_NANDF_CS7, IOMUX_CONFIG_ALT0); >> > +} >> >> Since it's very likely that setup_nfc() and setup_uart() will be used in >> other i.MX51 boards as well it's a good idea to >> place these functions in cpu/arm_cortexa8/mx51/devices.c (or something >> similar). >> I think it should be board level. Some board based on i.MX51 maybe has >> differenent choice. That can be handed with #ifdef's just like in the i.MX31 case. <...> >> > +#define MXC_SRPGC_EMI_SRPGCR (MXC_SRPGC_EMI_BASE + 0x0) >> > +#define MXC_SRPGC_EMI_PUPSCR (MXC_SRPGC_EMI_BASE + 0x4) >> > +#define MXC_SRPGC_EMI_PDNSCR (MXC_SRPGC_EMI_BASE + 0x8) >> > + >> >> Are all of the above #defines needed/expected to be needed by U-boot? > > No. It just copy from linux kernel code. Does need remove them? Don't no what the policy is. >> > diff --git a/cpu/arm_cortexa8/mx51/interrupts.c >> > b/cpu/arm_cortexa8/mx51/interrupts.c >> > new file mode 100644 >> > index 0000000..c0d70ac >> > --- /dev/null >> > +++ b/cpu/arm_cortexa8/mx51/interrupts.c >> <snip> >> What's means? Ignore the <snip> comments. >> > diff --git a/cpu/arm_cortexa8/mx51/serial.c >> > b/cpu/arm_cortexa8/mx51/serial.c >> > new file mode 100644 >> > index 0000000..580ac13 >> > --- /dev/null >> > +++ b/cpu/arm_cortexa8/mx51/serial.c >> >> I haven't looked in the details of the serial driver, but would it be >> possible to use drivers/serial/serial_mxc.c >> instead? It looks very similar. >> I don't like the implement of this driver. It contains the chip details in >> driver code. >> >> But I will do what as your said. And restructure this driver in another >> patch. That sounds like a good idea if you want to improve the serial driver. Create a separate standalone patch so people can review and test that. Regards, Magnus _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot