[U-Boot] [PATCH v2 4/4] arm, at91: add siemens corvus board

[Heiko Schocher]

Hello Andreas,

Am 04.11.2013 09:53, schrieb Andreas Bießmann:
> Hi Heiko,
>
> On 11/04/2013 07:40 AM, Heiko Schocher wrote:
>> enable support for the siemens AT91SAM9G20 based board corvus.
>>
>> 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>
>> Cc: Bo Shen<voice.shen at atmel.com>
>>
>> ---
>> - changes for v2:
>>    - add comments from Bo Shen
>>        - use gpio api
>>        - remove unneccessary comment
>>        - use at91_wait_for_reset()
>>        - remove unneccessary code in board file
>>        - Coding Style cleanup (tabs and unneccessary 1 after config define
>>          removed, use ".xxx = x" notation for initializing structs)
>>        - remove reset_phy()
>>        - remove CONFIG_SYS_MEMTEST_x defines, as mtest command
>>          is not used on this board.
>>        - changes load address
>>        - delete lcd support
>>    - add comments from Andreas Bießmann:
>>      - rearrange some init calls
>>      - remove some unneeded ifdef
>> ---
>>   board/siemens/corvus/Makefile |  39 ++++++++
>>   board/siemens/corvus/board.c  | 216 ++++++++++++++++++++++++++++++++++++++++++
>>   boards.cfg                    |   1 +
>>   include/configs/corvus.h      | 165 ++++++++++++++++++++++++++++++++
>>   4 files changed, 421 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..11f0d49
>> --- /dev/null
>> +++ b/board/siemens/corvus/board.c
>> @@ -0,0 +1,216 @@
[...]
>> +#ifdef CONFIG_MACB
>> +static void corvus_macb_hw_init(void)
>> +{
>> +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>> +	struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
>> +	unsigned long erstl;
>> +
>> +	/* Enable clock */
>> +	writel(1<<  ATMEL_ID_EMAC,&pmc->pcer);
>> +
>> +	/*
>> +	 * Disable pull-up on:
>> +	 *      RXDV (PA15) =>  PHY normal mode (not Test mode)
>> +	 *      ERX0 (PA12) =>  PHY ADDR0
>> +	 *      ERX1 (PA13) =>  PHY ADDR1 =>  PHYADDR = 0x0
>> +	 *
>> +	 * PHY has internal pull-down
>> +	 */
>> +	at91_set_pio_pullup(AT91_PIO_PORTA, 15, 0);
>> +	at91_set_pio_pullup(AT91_PIO_PORTA, 12, 0);
>> +	at91_set_pio_pullup(AT91_PIO_PORTA, 13, 0);
>
> Here applies the same statement as for the taurus board: Could you
> please wait for 'ATMEL_LEGACY' PIO API change or provide at91 portmux
> API (I think the AVR32 portmux API is a good starting point).

Yes.

>> +
>> +	erstl = readl(&rstc->mr)&  AT91_RSTC_MR_ERSTL_MASK;
>> +
>> +	/* Need to reset PHY ->  500ms reset */
>> +	writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
>> +		AT91_RSTC_MR_URSTEN,&rstc->mr);
>> +
>> +	writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST,&rstc->cr);
>> +
>> +	/* Wait for end of reset */
>> +	at91_wait_for_reset(100);
>
> You say above, that this will be 500ms reset pulse ... but wait just
> 100ms. Is that Ok?
> Please also check the taurus board.

We should rework this as Wolfgang suggested, or?

>> +	/* Restore NRST value */
>> +	writel(AT91_RSTC_KEY | erstl | AT91_RSTC_MR_URSTEN,
>> +	&rstc->mr);
>> +
>> +	/* Re-enable pull-up */
>> +	at91_set_pio_pullup(AT91_PIO_PORTA, 15, 1);
>> +	at91_set_pio_pullup(AT91_PIO_PORTA, 12, 1);
>> +	at91_set_pio_pullup(AT91_PIO_PORTA, 13, 1);
>> +
>> +	/* And the pins. */
>> +	at91_macb_hw_init();
>> +}
>> +#endif
>> +
>> +int board_early_init_f(void)
>> +{
>> +	at91_seriald_hw_init();
>> +	return 0;
>> +}
>> +
>> +int board_init(void)
>> +{
>> +	/* address of boot parameters */
>> +	gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>> +
>> +#ifdef CONFIG_CMD_NAND
>> +	corvus_nand_hw_init();
>> +#endif
>> +#ifdef CONFIG_ATMEL_SPI
>> +	at91_spi0_hw_init(1<<  4);
>> +#endif
>> +#ifdef CONFIG_HAS_DATAFLASH
>> +	at91_spi0_hw_init(1<<  0);
>> +#endif
>> +#ifdef CONFIG_MACB
>> +	corvus_macb_hw_init();
>> +#endif
>> +#ifdef CONFIG_CMD_USB
>> +	at91sam9m10g45ek_usb_hw_init();
>
> NAK, this is located in another board file (at91sam9m10g45ek), please adopt.

reworked.

>> +#endif
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_RESET_PHY_R
>
> Why provide empty reset_phy? Just remove the CONFIG_RESET_PHY_R in board
> config.

removed.

[...]
>> diff --git a/include/configs/corvus.h b/include/configs/corvus.h
>> new file mode 100644
>> index 0000000..b864562
>> --- /dev/null
>> +++ b/include/configs/corvus.h
>> @@ -0,0 +1,165 @@
[...]
>> +/* 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 */
>
> Could you please remove the<tab>  between 'define' and defined name.

removed.

[...]

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