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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
