[U-Boot] [PATCH] ARM: Add Support for the VInCo platform

[Gregory CLEMENT <gregory.clement@free-electrons.com>]

Hi Thomas,
 
 On mer., d?c. 16 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Dear Gregory CLEMENT,
>
> On Wed, 16 Dec 2015 11:34:00 +0100, Gregory CLEMENT wrote:
>
>> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
>> index c333647..d7e36cb 100644
>> --- a/arch/arm/mach-at91/Kconfig
>> +++ b/arch/arm/mach-at91/Kconfig
>> @@ -114,6 +114,12 @@ config TARGET_SMARTWEB
>>  	select CPU_ARM926EJS
>>  	select SUPPORT_SPL
>>  
>> +config TARGET_VINCO
>> +	bool "Support VINCO"
>> +	select CPU_V7
>> +	select SUPPORT_SPL
>> +
>> +
>
> One too many empty line.
>
>>  endchoice
>>  
>>  config SYS_SOC
>> @@ -143,5 +149,6 @@ source "board/ronetix/pm9g45/Kconfig"
>>  source "board/siemens/corvus/Kconfig"
>>  source "board/siemens/taurus/Kconfig"
>>  source "board/siemens/smartweb/Kconfig"
>> +source "board/l+g/vinco/Kconfig"
>
> Alphabetic ordering should be respected here.
>
>> diff --git a/board/l+g/vinco/Makefile b/board/l+g/vinco/Makefile
>> new file mode 100644
>> index 0000000..d68b3e4
>> --- /dev/null
>> +++ b/board/l+g/vinco/Makefile
>> @@ -0,0 +1,2 @@
>> +
>
> Useless newline.
>
>> +obj-y += vinco.o
>> diff --git a/board/l+g/vinco/vinco.c b/board/l+g/vinco/vinco.c
>> new file mode 100644
>> index 0000000..d9ba987
>> --- /dev/null
>> +++ b/board/l+g/vinco/vinco.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + * Board file for the VinCo platform
>> + * Based on the the SAMA5-EK board file
>> + * Configuration settings for the VinCo platform.
>> + * Copyright (C) 2014 Atmel
>> + *		      Bo Shen <voice.shen@atmel.com>
>> + * Copyright (C) 2015 Free Electrons
>> + *		      Gregory CLEMENT gregory.clement at free-electrons.com
>
> E-mail address should be enclosed in <...>.
>
>
>> +#ifdef CONFIG_ATMEL_SPI
>> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
>> +{
>> +	return bus == 0 && cs == 0;
>> +}
>> +
>> +void spi_cs_activate(struct spi_slave *slave)
>> +{
>> +	at91_set_pio_output(AT91_PIO_PORTC, 3, 0);
>> +}
>> +
>> +void spi_cs_deactivate(struct spi_slave *slave)
>> +{
>> +	at91_set_pio_output(AT91_PIO_PORTC, 3, 1);
>> +}
>> +
>> +static void sama5d4ek_spi0_hw_init(void)
>
> A function named sama5d4ek in the support for a board named L+G VInCo ?
>
>> +{
>> +	at91_set_a_periph(AT91_PIO_PORTC, 0, 0);	/* SPI0_MISO */
>> +	at91_set_a_periph(AT91_PIO_PORTC, 1, 0);	/* SPI0_MOSI */
>> +	at91_set_a_periph(AT91_PIO_PORTC, 2, 0);	/* SPI0_SPCK */
>> +
>> +	at91_set_pio_output(AT91_PIO_PORTC, 3, 1);	/* SPI0_CS0 */
>> +
>> +	/* Enable clock */
>> +	at91_periph_clk_enable(ATMEL_ID_SPI0);
>> +}
>> +#endif /* CONFIG_ATMEL_SPI */
>> +
>> +
>> +#ifdef CONFIG_CMD_USB
>> +static void sama5d4ek_usb_hw_init(void)
>
> Ditto.
>
>> +{
>> +	at91_set_pio_output(AT91_PIO_PORTE, 11, 0);
>> +	at91_set_pio_output(AT91_PIO_PORTE, 12, 0);
>> +	at91_set_pio_output(AT91_PIO_PORTE, 10, 0);
>> +}
>> +#endif
>> +
>> +
>> +#ifdef CONFIG_GENERIC_ATMEL_MCI
>> +void sama5d4ek_mci0_hw_init(void)
>
> Ditto (and multiple times later).
>
>
>> diff --git a/include/configs/vinco.h b/include/configs/vinco.h
>> new file mode 100644
>> index 0000000..678b04b
>> --- /dev/null
>> +++ b/include/configs/vinco.h
>> @@ -0,0 +1,172 @@
>> +/*
>> + * Configuration settings for the VInCo platform.
>> + *
>> + * Based on the settings for the SAMA5-EK board
>> + * Copyright (C) 2014 Atmel
>> + *		      Bo Shen <voice.shen@atmel.com>
>> + * Copyright (C) 2015 Free Electrons
>> + *		      Gregory CLEMENT gregory.clement at free-electrons.com
>
> <...> for e-mail address
>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#ifndef __CONFIG_H
>> +#define __CONFIG_H
>> +
>> +/* No NOR flash, this definition should put before common header */
>
> "should put" ?

"should _be_ put" I guess, I dumbly copy and paste this one.

>
>> +#define CONFIG_SYS_NO_FLASH
>> +
>> +#ifdef CONFIG_SYS_TEXT_BASE
>> +#undef CONFIG_SYS_TEXT_BASE
>> +#endif
>
> Why here? Nothing has been included so far, I guess this is going to be
> defined by the next line, no?
>
>> +#include "at91-sama5_common.h"
>> +
>> +/* The value in the common file is too far away for the VinCo platform */
>> +#ifdef CONFIG_SYS_TEXT_BASE
>> +#undef CONFIG_SYS_TEXT_BASE
>> +#endif
>> +#define CONFIG_SYS_TEXT_BASE		0x20f00000

Thanks for your review, I agree with all your comment and will send a v2
soon.


Gregory
>
> Best regards,
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com