Re: [U-Boot] [PATCH] imx51:Add support basic boot code of freescale imx51 bbg board
Hi
2009/9/22 Fred Fan fanyefeng@gmail.com:
Hi Magnus Liljia: Thanks for your comments. Best Regards Fred
2009/9/22, Magnus Lilja lilja.magnus@gmail.com:
Hi
I've scanned the patch briefly and have some comments below.
gareatech@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
HI Magnus Lilja: Best Regards Fred
2009/9/23, Magnus Lilja lilja.magnus@gmail.com:
Hi
2009/9/22 Fred Fan fanyefeng@gmail.com:
Hi Magnus Liljia: Thanks for your comments. Best Regards Fred
2009/9/22, Magnus Lilja lilja.magnus@gmail.com:
Hi
I've scanned the patch briefly and have some comments below.
gareatech@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. OK
+++ b/board/freescale/imx51/Makefile
<snip> Does means use new board name?
The board name should be used instead of the imx51 name.
OK
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.
If we do like i.MX31, the code in soc level has the details of board level.
we should reduce the block of #ifdef.
<...>
+#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.
we prefer to keep the sync with the file in kernel source code.
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
participants (2)
-
Fred Fan -
Magnus Lilja