[U-Boot] [PATCH] arm: AT91: add at91sam9x5ek board support

["Andreas Bießmann" <andreas.devel@googlemail.com>]

Hi Bo,

On 29.06.2012 11:28, Bo Shen wrote:
> Hi Andreas,
> 
> On 6/27/2012 21:47, Andreas Bie?mann wrote:
>> On 22.05.2012 12:06, Bo Shen wrote:

<snip>

>>> +
>>> +/*
>>> + * User Peripheral physical base addresses.
>>> + */
>>> +#define ATMEL_BASE_SPI0        0xf0000000
>>> +#define ATMEL_BASE_SPI1        0xf0004000
>>> +#define ATMEL_BASE_HSMCI0    0xf0008000
>>> +#define ATMEL_BASE_HSMCI1    0xf000c000
>>
>> Tabstop is 8 char, can you please allign these here for better
>> readability?
> 
> The tabstop is 8 char, when I use vi to edit the file, it is aligned.
> But when use git send patch, it display like this.
> May I need to fix this issue?

I'm sorry, I've read the patch which adds a single char at beginning of
line ('+'). After applying the patch it is aligned correctly!

<snip>

>>> +#endif /* __ASSEMBLY__ */
>>> +
>>> +#define    AT91_MATRIX_ULBT_INFINITE    (0 << 0)
>>> +#define    AT91_MATRIX_ULBT_SINGLE        (1 << 0)
>>> +#define    AT91_MATRIX_ULBT_FOUR        (2 << 0)
>>> +#define    AT91_MATRIX_ULBT_EIGHT        (3 << 0)
>>> +#define    AT91_MATRIX_ULBT_SIXTEEN    (4 << 0)
>>> +#define    AT91_MATRIX_ULBT_THIRTYTWO    (5 << 0)
>>> +#define    AT91_MATRIX_ULBT_SIXTYFOUR    (6 << 0)
>>> +#define    AT91_MATRIX_ULBT_128        (7 << 0)
>>> +
>>> +#define    AT91_MATRIX_DEFMSTR_TYPE_NONE    (0 << 16)
>>> +#define    AT91_MATRIX_DEFMSTR_TYPE_LAST    (1 << 16)
>>> +#define    AT91_MATRIX_DEFMSTR_TYPE_FIXED    (2 << 16)
>>
>> please remove the tab between 'define' and the definitions above.
> 
> Ok, I will fix this at next version patch.

Great, and can you please use the same indention in the whole file (some
have tabs and some have blanks).

<snip>

>>> +#ifdef CONFIG_RESET_PHY_R
>>> +void reset_phy(void)
>>> +{
>>> +#ifdef CONFIG_MACB
>>> +    /*
>>> +     * Initialize ethernet HW addr prior to starting Linux,
>>> +     * needed for nfsroot
>>> +     */
>>> +    eth_init(gd->bd);
>>
>> Isn't there a generic solution?
> 
> I will try to find a generic solution.

Well that does not mean that you should implement something here. I
thought there is some generic mechanism already in u-boot and therefore
it is not required to call eth_init() here to get the MAC-addr written
into PHY. But I'm not that familiar with it. Take it as a question, give
it a try and remove the line, ask some network custodian ;)
AFAIR other boards have written the HW address correctly without this line.

<snip>

>>> +void lcd_enable(void)
>>> +{
>>> +    if (has_lcdc())
>> Isn't has_lcd() cpu specific? I can not understand why one should enable
>> CONFIG_LCD if the cpu can not provide ... but that is ok with me.
> 
> sam9x5 is a series SoC (9G15, 9G25, 9G35, 9X25, 9X35) with different
> peripherals. Try to use one configuration file, so it need to decide
> whether SoC supports?

Well, you could add a config parameter in boards.cfg to switch the
feature on/off. But you can leave it as is, I have no strong meaning
about that particular piece of code, it was just a question.

<snip>

>>> +/* serial console */
>>> +#define CONFIG_ATMEL_USART
>>> +#define CONFIG_USART_BASE    ATMEL_BASE_DBGU
>>> +#define CONFIG_USART_ID        ATMEL_ID_SYS
>> --------------------------------^
>> fix alignment here
> 
> The tabstop is 8 char, when I use vi to edit the file, it is aligned.
> But when use git send patch, it display like this.
> May I need to fix this issue?

You are right. See comment above, maybe I should have applied the patch
before reading.

>>> +#define CONFIG_BOOTARGS        "mem=128M console=ttyS0,115200 " \
>>> +                "mtdparts=atmel_nand:" \
>>> +                "8M(bootstrap/uboot/kernel)ro,-(rootfs) " \
>>> +                "root=/dev/mtdblock1 rw " \
>>> +                "rootfstype=ubifs ubi.mtd=1 root=ubi0:rootfs"
>>
>> You could provide MTDPARTS_DEFAULT and MTDIDS_DEFAULT to be able to use
>> e.g. ubifs in u-boot (FYI, see also CONFIG_CMD_MTDPARTS).
> 
> Ok, I will fix this at next version patch.

Well, as I said before: This is for your information. I feel it is very
useful especially when working with ubifs in u-boot (just to mention
that, no need to change it).

Best regards

Andreas Bie?mann