[U-Boot] [PATCH] imx51:Add support basic boot code of freescale imx51 bbg board

[Magnus Lilja]

Hi

2009/9/22 Fred Fan <fanyefeng at gmail.com>:
> Hi Magnus Liljia:
>     Thanks for your comments.
>      Best Regards
> Fred
>
> 2009/9/22, Magnus Lilja <lilja.magnus at gmail.com>:
>>
>> Hi
>>
>>
>> I've scanned the patch briefly and have some comments below.
>>
>> gareatech at 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