[U-Boot] [PATCH v1 3/3] arm, at91: add siemens corvus board

[Heiko Schocher]

Hello Andreas,

Am 29.10.2013 11:01, schrieb Andreas Bießmann:
> Hi Heiko,
>
> some additional comments on top of Bo's.
>
> On 10/28/2013 06:45 AM, Bo Shen wrote:
>> Hi Heiko Schocher,
>>    Please add commit message.
>>
>> On 10/22/2013 13:51, Heiko Schocher wrote:
>>> Signed-off-by: Boris Schmidt<boris.schmidt at siemens.com>
>>> Reviewed-by: Heiko Schocher<hs at denx.de>
>>> Cc: Andreas Bießmann<andreas.devel at googlemail.com>
>>> ---
>>>    board/siemens/corvus/Makefile |  39 +++++
>>>    board/siemens/corvus/board.c  | 345
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>    boards.cfg                    |   1 +
>>>    include/configs/corvus.h      | 185 ++++++++++++++++++++++
>>>    4 files changed, 570 insertions(+)
>>>    create mode 100644 board/siemens/corvus/Makefile
>>>    create mode 100644 board/siemens/corvus/board.c
>>>    create mode 100644 include/configs/corvus.h
>>>
>>> diff --git a/board/siemens/corvus/board.c b/board/siemens/corvus/board.c
>>> new file mode 100644
>>> index 0000000..1940da7
>>> --- /dev/null
>>> +++ b/board/siemens/corvus/board.c
[...]
>>> +int board_init(void)
>>> +{
>>> +    /* Enable Ctrlc */
>>> +    console_init_f();
>>
>> Remove it.
>>
>>> +    /* arch number of corvus-Board */
>>> +    gd->bd->bi_arch_number = MACH_TYPE_CORVUS;
>>
>> move to board related configuration file.
>>
>>> +    /* adress of boot parameters */
>>
>> s/adress/address
>>
>>> +    gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100; +
>>> +
>>> +    board_early_init_f();
>
> Why call board_early_init_f() here? Isn't it called bby generic board
> code in the init_sequence_f[] when CONFIG_BOARD_EARLY_INIT_F is defined?

removed.

>>> +#ifdef CONFIG_CMD_NAND
>>> +    corvus_nand_hw_init();
>>> +#endif
>>> +#ifdef CONFIG_HAS_DATAFLASH
>
> Isn't SPI required for dataflash? Consider rearrange the ifdiffery here.

rearranged.

[...]
>>> +int board_eth_init(bd_t *bis)
>>> +{
>>> +    int rc = 0;
>>> +#ifdef CONFIG_MACB
>>> +    rc = macb_eth_initialize(0, (void *)ATMEL_BASE_EMAC, 0x00);
>>> +#endif
>>> +    return rc;
>>> +}
>>> +
>>> +/* SPI chip select control */
>>> +#ifdef CONFIG_ATMEL_SPI
>
> I think this is not required, should be catched by garbage collector.

removed this ifdef

[...]
>>> diff --git a/include/configs/corvus.h b/include/configs/corvus.h
>>> new file mode 100644
>>> index 0000000..09513f9
>>> --- /dev/null
>>> +++ b/include/configs/corvus.h
>>> @@ -0,0 +1,185 @@
>>> +/*
>>> + * Common board functions for siemens AT91SAM9G45 based boards
>>> + * (C) Copyright 2013 Siemens AG
>>> + *
>>> + * Based on:
>>> + * U-Boot file: include/configs/at91sam9m10g45ek.h
>>> + * (C) Copyright 2007-2008
>>> + * Stelian Pop<stelian at popies.net>
>>> + * Lead Tech Design<www.leadtechdesign.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +
>>> +
>
> remove some of the new lines here.

Done.

>>> +#ifndef __CONFIG_H
>>> +#define __CONFIG_H
>>> +
>>> +#include<asm/hardware.h>
>>> +
>>> +#define MACH_TYPE_CORVUS               2066
>>> +
>>> +/*
>>> + * Warning: changing CONFIG_SYS_TEXT_BASE requires
>>> + * adapting the initial boot program.
>>> + * Since the linker has to swallow that define, we must use a pure
>>> + * hex number here!
>>> + */
>>> +
>>> +#define CONFIG_SYS_TEXT_BASE  0x73f00000
>
> Is'nt the RAM placed @0x20000000 for this devices? Ah got it, The
> sam9m10 has DDR1 @CS0 and DDR0 @CS6

Yep.

>>> +
>>> +#define CONFIG_AT91_LEGACY
>>> +#define CONFIG_ATMEL_LEGACY        /* required until (g)pio is fixed */
>
> The ATMEL_LEGACY is required for dataflash, please consider using the
> generic mtd sf stuff instead.

Ok, think about that ...

[...]
>>> +/* LED */
>>> +#define CONFIG_AT91_LED
>>> +#define    CONFIG_RED_LED        AT91_PIN_PD31    /* this is the
>>> user1 led */
>>> +#define    CONFIG_GREEN_LED    AT91_PIN_PD0    /* this is the user2
>>> led */
>
> Would you mind to use the generic gpio led stuff instead?

You mean ./drivers/misc/gpio_led.c ?

I check this.

[...]
>>> +/* Ethernet */
>>> +#define CONFIG_MACB
>>> +#define CONFIG_RMII
>>> +#define CONFIG_NET_RETRY_COUNT        20
>>> +#define CONFIG_RESET_PHY_R
>>> +
>>> +/* USB */
>>> +#define CONFIG_USB_EHCI
>>> +#define CONFIG_USB_EHCI_ATMEL
>>> +#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS    2
>>> +#define CONFIG_DOS_PARTITION
>>> +#define CONFIG_USB_STORAGE
>>> +
>>> +#define CONFIG_SYS_LOAD_ADDR        0x22000000    /* load address */
>>
>> Does this work on your board?
>
> ;) good catch ...

Fixed.

>>> +
>>> +#define CONFIG_SYS_MEMTEST_START    CONFIG_SYS_SDRAM_BASE
>>> +#define CONFIG_SYS_MEMTEST_END        0x23e00000
>>
>> Ditto.
>>
>>> +/* bootstrap + u-boot + env in nandflash */
>>> +#define CONFIG_ENV_IS_IN_NAND
>>> +#define CONFIG_ENV_OFFSET        0x100000
>>> +#define CONFIG_ENV_OFFSET_REDUND    0x180000
>>> +#define CONFIG_ENV_SIZE            0x20000
>>> +
>>> +#define CONFIG_BOOTCOMMAND                        \
>>> +    "nand read 0x70000000 0x200000 0x300000;"            \
>
> nand read ${laodaddr} kernel ?
>
> When using mtdparts in u-boot ... just my 2¢

You mean:
nand read ${loadaddr} kernel
;-)

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany