Patchwork [U-Boot,v2] exynos5250: move board specific configs to board specific config file

login
register
mail settings
Submitter Inderpal Singh
Date June 7, 2013, 11:56 a.m.
Message ID <1370606193-2566-1-git-send-email-inderpal.singh@linaro.org>
Download mbox | patch
Permalink /patch/249666/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Inderpal Singh - June 7, 2013, 11:56 a.m.
Not all boards based on exynos5250 have SPI flash, same serial port and might
not require display and the same lds script. Hence move them to board specific
config file.

Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
---
v1 was posted as the second patch of [1]

Changes in v2:
	- split from the patchset at [1]
	- moved CONFIG_LCD and CONFIG_SPI_FLASH
	- rebased to latest u-boot-samsung master branch

[1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101

 include/configs/exynos5250-dt.h |   11 +----------
 include/configs/smdk5250.h      |   16 ++++++++++++++--
 include/configs/snow.h          |   16 ++++++++++++++--
 3 files changed, 29 insertions(+), 14 deletions(-)
Simon Glass - June 11, 2013, 2:27 p.m.
Hi,

On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <inderpal.singh@linaro.org>wrote:

> Not all boards based on exynos5250 have SPI flash, same serial port and
> might
> not require display and the same lds script. Hence move them to board
> specific
> config file.
>

At least for the serial port this should be controlled by the device tree.
There are quite a lot of pending patches for exynos, one of which enables
this and removes the need for the CONFIG_SERIAL3 define.

If you want to have a look, I pushed them to:

http://git.denx.de/u-boot-x86.git in branch 'snow'


>
> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
> ---
> v1 was posted as the second patch of [1]
>
> Changes in v2:
>         - split from the patchset at [1]
>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>         - rebased to latest u-boot-samsung master branch
>
> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>
>  include/configs/exynos5250-dt.h |   11 +----------
>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>  include/configs/snow.h          |   16 ++++++++++++++--
>  3 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/include/configs/exynos5250-dt.h
> b/include/configs/exynos5250-dt.h
> index 03b07b2..22e47eb 100644
> --- a/include/configs/exynos5250-dt.h
> +++ b/include/configs/exynos5250-dt.h
> @@ -29,7 +29,6 @@
>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>  #define CONFIG_S5P                     /* S5P Family */
>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5 Family */
> -#define CONFIG_SMDK5250                        /* which is in a SMDK5250
> */
>

This is a misnomer - it actually means Exynos 5250 I think. The only thing
it controls is the generation of the SPL packaging tool. So for now it
should be defined for all Exynos5250 boards.


>
>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>
> @@ -78,7 +77,6 @@
>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>
>  /* select serial console configuration */
> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>  #define CONFIG_BAUDRATE                        115200
>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>
> @@ -148,8 +146,6 @@
>  #define CONFIG_SPL
>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>
> -/* specific .lds file */
> -#define CONFIG_SPL_LDSCRIPT
>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>

Again I suspect this is a misnomer.


>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>
> @@ -158,7 +154,7 @@
>  /* Miscellaneous configurable options */
>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser    */
> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer Size
> */
>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>  #define CONFIG_SYS_MAXARGS             16      /* max number of command
> args */
> @@ -198,7 +194,6 @@
>  /* FLASH and environment organization */
>  #define CONFIG_SYS_NO_FLASH
>  #undef CONFIG_CMD_IMLS
> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>
>  #define CONFIG_SYS_MMC_ENV_DEV         0
>
> @@ -247,9 +242,6 @@
>  #define CONFIG_I2C_EDID
>
>  /* SPI */
> -#define CONFIG_ENV_IS_IN_SPI_FLASH
> -#define CONFIG_SPI_FLASH
> -
>  #ifdef CONFIG_SPI_FLASH
>  #define CONFIG_EXYNOS_SPI
>  #define CONFIG_CMD_SF
> @@ -306,7 +298,6 @@
>  #define CONFIG_SHA256
>
>  /* Display */
> -#define CONFIG_LCD
>  #ifdef CONFIG_LCD
>  #define CONFIG_EXYNOS_FB
>  #define CONFIG_EXYNOS_DP
> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
> index 81f83a8..4af1909 100644
> --- a/include/configs/smdk5250.h
> +++ b/include/configs/smdk5250.h
> @@ -25,9 +25,21 @@
>  #ifndef __CONFIG_SMDK_H
>  #define __CONFIG_SMDK_H
>
> -#include <configs/exynos5250-dt.h>
> -
>  #undef CONFIG_DEFAULT_DEVICE_TREE
>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>
> +#define CONFIG_SMDK5250                        /* which is in a SMDK5250
> */
> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
> +
> +/* specific .lds file */
> +#define CONFIG_SPL_LDSCRIPT
>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
> +#define CONFIG_IDENT_STRING            " for SMDK5250"
> +#define CONFIG_SPI_FLASH
> +#define CONFIG_ENV_IS_IN_SPI_FLASH
> +
> +/* Display */
> +#define CONFIG_LCD
> +
> +#include <configs/exynos5250-dt.h>
> +
>  #endif /* __CONFIG_SMDK_H */
> diff --git a/include/configs/snow.h b/include/configs/snow.h
> index b8460fd..e940c69 100644
> --- a/include/configs/snow.h
> +++ b/include/configs/snow.h
> @@ -25,9 +25,21 @@
>  #ifndef __CONFIG_SNOW_H
>  #define __CONFIG_SNOW_H
>
> -#include <configs/exynos5250-dt.h>
> -
>  #undef CONFIG_DEFAULT_DEVICE_TREE
>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>
> +#define CONFIG_SMDK5250                        /* which is in a SMDK5250
> */
> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
> +
> +/* specific .lds file */
> +#define CONFIG_SPL_LDSCRIPT
>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
> +#define CONFIG_IDENT_STRING            " for SMDK5250"
> +#define CONFIG_SPI_FLASH
> +#define CONFIG_ENV_IS_IN_SPI_FLASH
> +
> +/* Display */
> +#define CONFIG_LCD
> +
> +#include <configs/exynos5250-dt.h>
> +
>  #endif /* __CONFIG_SNOW_H */
>

The intent with the exynos5250-dt file is that it supports any board with
that chip, so it should enable any feature used by Exynos5250 boards.
Granted that might not suit all boards, which only need a subset of the
features. Perhaps we should create an exynos5250-dt-base.h, with just the
core options defined. Then other boards can include the base file, and
exynos5250-dt can stay as the 'enable everything/ config?

Regards,
Simon
Inderpal Singh - June 20, 2013, 7:10 a.m.
Hi Simon,

Thanks for review.


On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:

> Hi,
>
> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <inderpal.singh@linaro.org>wrote:
>
>> Not all boards based on exynos5250 have SPI flash, same serial port and
>> might
>> not require display and the same lds script. Hence move them to board
>> specific
>> config file.
>>
>
> At least for the serial port this should be controlled by the device tree.
> There are quite a lot of pending patches for exynos, one of which enables
> this and removes the need for the CONFIG_SERIAL3 define.
>
> If you want to have a look, I pushed them to:
>
> http://git.denx.de/u-boot-x86.git in branch 'snow'
>

I was not aware of this patchset. Thanks for pointing out. I will update my
patchset to use DT for serial.


>
>>
>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>> ---
>> v1 was posted as the second patch of [1]
>>
>> Changes in v2:
>>         - split from the patchset at [1]
>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>>         - rebased to latest u-boot-samsung master branch
>>
>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>>
>>  include/configs/exynos5250-dt.h |   11 +----------
>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>>  include/configs/snow.h          |   16 ++++++++++++++--
>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/configs/exynos5250-dt.h
>> b/include/configs/exynos5250-dt.h
>> index 03b07b2..22e47eb 100644
>> --- a/include/configs/exynos5250-dt.h
>> +++ b/include/configs/exynos5250-dt.h
>> @@ -29,7 +29,6 @@
>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>>  #define CONFIG_S5P                     /* S5P Family */
>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5 Family */
>> -#define CONFIG_SMDK5250                        /* which is in a SMDK5250
>> */
>>
>
> This is a misnomer - it actually means Exynos 5250 I think. The only thing
> it controls is the generation of the SPL packaging tool. So for now it
> should be defined for all Exynos5250 boards.
>

Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.


>
>
>>
>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>>
>> @@ -78,7 +77,6 @@
>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>>
>>  /* select serial console configuration */
>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>  #define CONFIG_BAUDRATE                        115200
>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>>
>> @@ -148,8 +146,6 @@
>>  #define CONFIG_SPL
>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>>
>> -/* specific .lds file */
>> -#define CONFIG_SPL_LDSCRIPT
>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>
>
> Again I suspect this is a misnomer.
>

Since all boards might not need this lds file, so how about moving lds file
to board/samsung/common and renaming it to exynos5250-uboot-spl.lds. The
boards needing this will have to include in their respective config files.
Let me know your opinion.


>
>
>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>>
>> @@ -158,7 +154,7 @@
>>  /* Miscellaneous configurable options */
>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser    */
>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
>> Size */
>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>>  #define CONFIG_SYS_MAXARGS             16      /* max number of command
>> args */
>> @@ -198,7 +194,6 @@
>>  /* FLASH and environment organization */
>>  #define CONFIG_SYS_NO_FLASH
>>  #undef CONFIG_CMD_IMLS
>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>>
>>  #define CONFIG_SYS_MMC_ENV_DEV         0
>>
>> @@ -247,9 +242,6 @@
>>  #define CONFIG_I2C_EDID
>>
>>  /* SPI */
>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
>> -#define CONFIG_SPI_FLASH
>> -
>>  #ifdef CONFIG_SPI_FLASH
>>  #define CONFIG_EXYNOS_SPI
>>  #define CONFIG_CMD_SF
>> @@ -306,7 +298,6 @@
>>  #define CONFIG_SHA256
>>
>>  /* Display */
>> -#define CONFIG_LCD
>>  #ifdef CONFIG_LCD
>>  #define CONFIG_EXYNOS_FB
>>  #define CONFIG_EXYNOS_DP
>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
>> index 81f83a8..4af1909 100644
>> --- a/include/configs/smdk5250.h
>> +++ b/include/configs/smdk5250.h
>> @@ -25,9 +25,21 @@
>>  #ifndef __CONFIG_SMDK_H
>>  #define __CONFIG_SMDK_H
>>
>> -#include <configs/exynos5250-dt.h>
>> -
>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>>
>> +#define CONFIG_SMDK5250                        /* which is in a SMDK5250
>> */
>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>> +
>> +/* specific .lds file */
>> +#define CONFIG_SPL_LDSCRIPT
>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>> +#define CONFIG_SPI_FLASH
>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>> +
>> +/* Display */
>> +#define CONFIG_LCD
>> +
>> +#include <configs/exynos5250-dt.h>
>> +
>>  #endif /* __CONFIG_SMDK_H */
>> diff --git a/include/configs/snow.h b/include/configs/snow.h
>> index b8460fd..e940c69 100644
>> --- a/include/configs/snow.h
>> +++ b/include/configs/snow.h
>> @@ -25,9 +25,21 @@
>>  #ifndef __CONFIG_SNOW_H
>>  #define __CONFIG_SNOW_H
>>
>> -#include <configs/exynos5250-dt.h>
>> -
>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>>
>> +#define CONFIG_SMDK5250                        /* which is in a SMDK5250
>> */
>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>> +
>> +/* specific .lds file */
>> +#define CONFIG_SPL_LDSCRIPT
>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>> +#define CONFIG_SPI_FLASH
>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>> +
>> +/* Display */
>> +#define CONFIG_LCD
>> +
>> +#include <configs/exynos5250-dt.h>
>> +
>>  #endif /* __CONFIG_SNOW_H */
>>
>
> The intent with the exynos5250-dt file is that it supports any board with
> that chip, so it should enable any feature used by Exynos5250 boards.
> Granted that might not suit all boards, which only need a subset of the
> features. Perhaps we should create an exynos5250-dt-base.h, with just the
> core options defined. Then other boards can include the base file, and
> exynos5250-dt can stay as the 'enable everything/ config?
>
>
So as per you suggestion, there would be 3 files. One
exynos5250-dt-base.h, second exynos5250-dt.h and third the board specific
config file.

How about having core options unconditionally enabled in exynos5250-dt.h
and other options with #ifdef. The board specific config files can define
the other options. This way only 2 files will do.

For example, let exynos5250-dt.h has SPI related configs under #ifdef
CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
based on the spi flash presence in respective boards.

Let me know your opinion.

Thanks,
Inder


Regards,
> Simon
>
>
Inderpal Singh - June 28, 2013, 10:27 a.m.
Hi Simon,


On 20 June 2013 12:40, Inderpal Singh <inderpal.singh@linaro.org> wrote:

> Hi Simon,
>
> Thanks for review.
>
>
> On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:
>
>> Hi,
>>
>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <inderpal.singh@linaro.org
>> > wrote:
>>
>>> Not all boards based on exynos5250 have SPI flash, same serial port and
>>> might
>>> not require display and the same lds script. Hence move them to board
>>> specific
>>> config file.
>>>
>>
>> At least for the serial port this should be controlled by the device
>> tree. There are quite a lot of pending patches for exynos, one of which
>> enables this and removes the need for the CONFIG_SERIAL3 define.
>>
>> If you want to have a look, I pushed them to:
>>
>> http://git.denx.de/u-boot-x86.git in branch 'snow'
>>
>
> I was not aware of this patchset. Thanks for pointing out. I will update
> my patchset to use DT for serial.
>
>
>>
>>>
>>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>>> ---
>>> v1 was posted as the second patch of [1]
>>>
>>> Changes in v2:
>>>         - split from the patchset at [1]
>>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>>>         - rebased to latest u-boot-samsung master branch
>>>
>>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>>>
>>>  include/configs/exynos5250-dt.h |   11 +----------
>>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>>>  include/configs/snow.h          |   16 ++++++++++++++--
>>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/configs/exynos5250-dt.h
>>> b/include/configs/exynos5250-dt.h
>>> index 03b07b2..22e47eb 100644
>>> --- a/include/configs/exynos5250-dt.h
>>> +++ b/include/configs/exynos5250-dt.h
>>> @@ -29,7 +29,6 @@
>>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>>>  #define CONFIG_S5P                     /* S5P Family */
>>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5 Family
>>> */
>>> -#define CONFIG_SMDK5250                        /* which is in a
>>> SMDK5250 */
>>>
>>
>> This is a misnomer - it actually means Exynos 5250 I think. The only
>> thing it controls is the generation of the SPL packaging tool. So for now
>> it should be defined for all Exynos5250 boards.
>>
>
> Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.
>
>
>>
>>
>>>
>>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>>>
>>> @@ -78,7 +77,6 @@
>>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>>>
>>>  /* select serial console configuration */
>>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>  #define CONFIG_BAUDRATE                        115200
>>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>>>
>>> @@ -148,8 +146,6 @@
>>>  #define CONFIG_SPL
>>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>>>
>>> -/* specific .lds file */
>>> -#define CONFIG_SPL_LDSCRIPT
>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>
>>
>> Again I suspect this is a misnomer.
>>
>
> Since all boards might not need this lds file, so how about moving lds
> file to board/samsung/common and renaming it to exynos5250-uboot-spl.lds.
> The boards needing this will have to include in their respective config
> files.
> Let me know your opinion.
>

Any thoughts about it?


>
>
>>
>>
>>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>>>
>>> @@ -158,7 +154,7 @@
>>>  /* Miscellaneous configurable options */
>>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser
>>>  */
>>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
>>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
>>> Size */
>>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>>>  #define CONFIG_SYS_MAXARGS             16      /* max number of command
>>> args */
>>> @@ -198,7 +194,6 @@
>>>  /* FLASH and environment organization */
>>>  #define CONFIG_SYS_NO_FLASH
>>>  #undef CONFIG_CMD_IMLS
>>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>
>>>  #define CONFIG_SYS_MMC_ENV_DEV         0
>>>
>>> @@ -247,9 +242,6 @@
>>>  #define CONFIG_I2C_EDID
>>>
>>>  /* SPI */
>>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
>>> -#define CONFIG_SPI_FLASH
>>> -
>>>  #ifdef CONFIG_SPI_FLASH
>>>  #define CONFIG_EXYNOS_SPI
>>>  #define CONFIG_CMD_SF
>>> @@ -306,7 +298,6 @@
>>>  #define CONFIG_SHA256
>>>
>>>  /* Display */
>>> -#define CONFIG_LCD
>>>  #ifdef CONFIG_LCD
>>>  #define CONFIG_EXYNOS_FB
>>>  #define CONFIG_EXYNOS_DP
>>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
>>> index 81f83a8..4af1909 100644
>>> --- a/include/configs/smdk5250.h
>>> +++ b/include/configs/smdk5250.h
>>> @@ -25,9 +25,21 @@
>>>  #ifndef __CONFIG_SMDK_H
>>>  #define __CONFIG_SMDK_H
>>>
>>> -#include <configs/exynos5250-dt.h>
>>> -
>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>>>
>>> +#define CONFIG_SMDK5250                        /* which is in a
>>> SMDK5250 */
>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>> +
>>> +/* specific .lds file */
>>> +#define CONFIG_SPL_LDSCRIPT
>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>> +#define CONFIG_SPI_FLASH
>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>> +
>>> +/* Display */
>>> +#define CONFIG_LCD
>>> +
>>> +#include <configs/exynos5250-dt.h>
>>> +
>>>  #endif /* __CONFIG_SMDK_H */
>>> diff --git a/include/configs/snow.h b/include/configs/snow.h
>>> index b8460fd..e940c69 100644
>>> --- a/include/configs/snow.h
>>> +++ b/include/configs/snow.h
>>> @@ -25,9 +25,21 @@
>>>  #ifndef __CONFIG_SNOW_H
>>>  #define __CONFIG_SNOW_H
>>>
>>> -#include <configs/exynos5250-dt.h>
>>> -
>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>>>
>>> +#define CONFIG_SMDK5250                        /* which is in a
>>> SMDK5250 */
>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>> +
>>> +/* specific .lds file */
>>> +#define CONFIG_SPL_LDSCRIPT
>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>> +#define CONFIG_SPI_FLASH
>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>> +
>>> +/* Display */
>>> +#define CONFIG_LCD
>>> +
>>> +#include <configs/exynos5250-dt.h>
>>> +
>>>  #endif /* __CONFIG_SNOW_H */
>>>
>>
>> The intent with the exynos5250-dt file is that it supports any board with
>> that chip, so it should enable any feature used by Exynos5250 boards.
>> Granted that might not suit all boards, which only need a subset of the
>> features. Perhaps we should create an exynos5250-dt-base.h, with just the
>> core options defined. Then other boards can include the base file, and
>> exynos5250-dt can stay as the 'enable everything/ config?
>>
>>
> So as per you suggestion, there would be 3 files. One
> exynos5250-dt-base.h, second exynos5250-dt.h and third the board specific
> config file.
>
> How about having core options unconditionally enabled in exynos5250-dt.h
> and other options with #ifdef. The board specific config files can define
> the other options. This way only 2 files will do.
>
> For example, let exynos5250-dt.h has SPI related configs under #ifdef
> CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
> based on the spi flash presence in respective boards.
>
> Let me know your opinion.
>

Any feedback about the above proposal?

Thanks,
Inder



>
> Thanks,
> Inder
>
>
> Regards,
>> Simon
>>
>>
>
Simon Glass - June 28, 2013, 3:50 p.m.
Hi Inderpal,

On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh
<inderpal.singh@linaro.org>wrote:

> Hi Simon,
>
> Thanks for review.
>
>
> On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:
>
>> Hi,
>>
>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <inderpal.singh@linaro.org
>> > wrote:
>>
>>> Not all boards based on exynos5250 have SPI flash, same serial port and
>>> might
>>> not require display and the same lds script. Hence move them to board
>>> specific
>>> config file.
>>>
>>
>> At least for the serial port this should be controlled by the device
>> tree. There are quite a lot of pending patches for exynos, one of which
>> enables this and removes the need for the CONFIG_SERIAL3 define.
>>
>> If you want to have a look, I pushed them to:
>>
>> http://git.denx.de/u-boot-x86.git in branch 'snow'
>>
>
> I was not aware of this patchset. Thanks for pointing out. I will update
> my patchset to use DT for serial.
>
>
>>
>>>
>>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>>> ---
>>> v1 was posted as the second patch of [1]
>>>
>>> Changes in v2:
>>>         - split from the patchset at [1]
>>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>>>         - rebased to latest u-boot-samsung master branch
>>>
>>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>>>
>>>  include/configs/exynos5250-dt.h |   11 +----------
>>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>>>  include/configs/snow.h          |   16 ++++++++++++++--
>>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/configs/exynos5250-dt.h
>>> b/include/configs/exynos5250-dt.h
>>> index 03b07b2..22e47eb 100644
>>> --- a/include/configs/exynos5250-dt.h
>>> +++ b/include/configs/exynos5250-dt.h
>>> @@ -29,7 +29,6 @@
>>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>>>  #define CONFIG_S5P                     /* S5P Family */
>>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5 Family
>>> */
>>> -#define CONFIG_SMDK5250                        /* which is in a
>>> SMDK5250 */
>>>
>>
>> This is a misnomer - it actually means Exynos 5250 I think. The only
>> thing it controls is the generation of the SPL packaging tool. So for now
>> it should be defined for all Exynos5250 boards.
>>
>
> Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.
>

Sounds good.


>
>
>>
>>
>>>
>>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>>>
>>> @@ -78,7 +77,6 @@
>>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>>>
>>>  /* select serial console configuration */
>>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>  #define CONFIG_BAUDRATE                        115200
>>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>>>
>>> @@ -148,8 +146,6 @@
>>>  #define CONFIG_SPL
>>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>>>
>>> -/* specific .lds file */
>>> -#define CONFIG_SPL_LDSCRIPT
>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>
>>
>> Again I suspect this is a misnomer.
>>
>
> Since all boards might not need this lds file, so how about moving lds
> file to board/samsung/common and renaming it to exynos5250-uboot-spl.lds.
> The boards needing this will have to include in their respective config
> files.
> Let me know your opinion.
>

OK, so long has you know of boards that don't need it?


>
>
>>
>>
>>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>>>
>>> @@ -158,7 +154,7 @@
>>>  /* Miscellaneous configurable options */
>>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser
>>>  */
>>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
>>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
>>> Size */
>>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>>>  #define CONFIG_SYS_MAXARGS             16      /* max number of command
>>> args */
>>> @@ -198,7 +194,6 @@
>>>  /* FLASH and environment organization */
>>>  #define CONFIG_SYS_NO_FLASH
>>>  #undef CONFIG_CMD_IMLS
>>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>
>>>  #define CONFIG_SYS_MMC_ENV_DEV         0
>>>
>>> @@ -247,9 +242,6 @@
>>>  #define CONFIG_I2C_EDID
>>>
>>>  /* SPI */
>>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
>>> -#define CONFIG_SPI_FLASH
>>> -
>>>  #ifdef CONFIG_SPI_FLASH
>>>  #define CONFIG_EXYNOS_SPI
>>>  #define CONFIG_CMD_SF
>>> @@ -306,7 +298,6 @@
>>>  #define CONFIG_SHA256
>>>
>>>  /* Display */
>>> -#define CONFIG_LCD
>>>  #ifdef CONFIG_LCD
>>>  #define CONFIG_EXYNOS_FB
>>>  #define CONFIG_EXYNOS_DP
>>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
>>> index 81f83a8..4af1909 100644
>>> --- a/include/configs/smdk5250.h
>>> +++ b/include/configs/smdk5250.h
>>> @@ -25,9 +25,21 @@
>>>  #ifndef __CONFIG_SMDK_H
>>>  #define __CONFIG_SMDK_H
>>>
>>> -#include <configs/exynos5250-dt.h>
>>> -
>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>>>
>>> +#define CONFIG_SMDK5250                        /* which is in a
>>> SMDK5250 */
>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>> +
>>> +/* specific .lds file */
>>> +#define CONFIG_SPL_LDSCRIPT
>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>> +#define CONFIG_SPI_FLASH
>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>> +
>>> +/* Display */
>>> +#define CONFIG_LCD
>>> +
>>> +#include <configs/exynos5250-dt.h>
>>> +
>>>  #endif /* __CONFIG_SMDK_H */
>>> diff --git a/include/configs/snow.h b/include/configs/snow.h
>>> index b8460fd..e940c69 100644
>>> --- a/include/configs/snow.h
>>> +++ b/include/configs/snow.h
>>> @@ -25,9 +25,21 @@
>>>  #ifndef __CONFIG_SNOW_H
>>>  #define __CONFIG_SNOW_H
>>>
>>> -#include <configs/exynos5250-dt.h>
>>> -
>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>>>
>>> +#define CONFIG_SMDK5250                        /* which is in a
>>> SMDK5250 */
>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>> +
>>> +/* specific .lds file */
>>> +#define CONFIG_SPL_LDSCRIPT
>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>> +#define CONFIG_SPI_FLASH
>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>> +
>>> +/* Display */
>>> +#define CONFIG_LCD
>>> +
>>> +#include <configs/exynos5250-dt.h>
>>> +
>>>  #endif /* __CONFIG_SNOW_H */
>>>
>>
>> The intent with the exynos5250-dt file is that it supports any board with
>> that chip, so it should enable any feature used by Exynos5250 boards.
>> Granted that might not suit all boards, which only need a subset of the
>> features. Perhaps we should create an exynos5250-dt-base.h, with just the
>> core options defined. Then other boards can include the base file, and
>> exynos5250-dt can stay as the 'enable everything/ config?
>>
>>
> So as per you suggestion, there would be 3 files. One
> exynos5250-dt-base.h, second exynos5250-dt.h and third the board specific
> config file.
>
> How about having core options unconditionally enabled in exynos5250-dt.h
> and other options with #ifdef. The board specific config files can define
> the other options. This way only 2 files will do.
>
> For example, let exynos5250-dt.h has SPI related configs under #ifdef
> CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
> based on the spi flash presence in respective boards.
>

> Let me know your opinion.
>

Well the problem is who sets CONFIG_SPI_FLASH

For us at least, exynos5250-dt is a good upstream target, since we can add
all features into it and it will should boot on all the different boards.
It helps to make sure that other boards don't get non-device-tree config
that breaks this approach.

So I think you do need a base config file. But I think a better name might
be exynos5250-dt-common.h instead of exynos5250-dt-base.h.

Regards,
Simon
Chander Kashyap - July 1, 2013, 4:08 a.m.
On 28 June 2013 21:20, Simon Glass <sjg@chromium.org> wrote:
> Hi Inderpal,
>
> On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh
> <inderpal.singh@linaro.org>wrote:
>
>> Hi Simon,
>>
>> Thanks for review.
>>
>>
>> On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi,
>>>
>>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <inderpal.singh@linaro.org
>>> > wrote:
>>>
>>>> Not all boards based on exynos5250 have SPI flash, same serial port and
>>>> might
>>>> not require display and the same lds script. Hence move them to board
>>>> specific
>>>> config file.
>>>>
>>>
>>> At least for the serial port this should be controlled by the device
>>> tree. There are quite a lot of pending patches for exynos, one of which
>>> enables this and removes the need for the CONFIG_SERIAL3 define.
>>>
>>> If you want to have a look, I pushed them to:
>>>
>>> http://git.denx.de/u-boot-x86.git in branch 'snow'
>>>
>>
>> I was not aware of this patchset. Thanks for pointing out. I will update
>> my patchset to use DT for serial.
>>
>>
>>>
>>>>
>>>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>>>> ---
>>>> v1 was posted as the second patch of [1]
>>>>
>>>> Changes in v2:
>>>>         - split from the patchset at [1]
>>>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>>>>         - rebased to latest u-boot-samsung master branch
>>>>
>>>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>>>>
>>>>  include/configs/exynos5250-dt.h |   11 +----------
>>>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>>>>  include/configs/snow.h          |   16 ++++++++++++++--
>>>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/include/configs/exynos5250-dt.h
>>>> b/include/configs/exynos5250-dt.h
>>>> index 03b07b2..22e47eb 100644
>>>> --- a/include/configs/exynos5250-dt.h
>>>> +++ b/include/configs/exynos5250-dt.h
>>>> @@ -29,7 +29,6 @@
>>>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>>>>  #define CONFIG_S5P                     /* S5P Family */
>>>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5 Family
>>>> */
>>>> -#define CONFIG_SMDK5250                        /* which is in a
>>>> SMDK5250 */
>>>>
>>>
>>> This is a misnomer - it actually means Exynos 5250 I think. The only
>>> thing it controls is the generation of the SPL packaging tool. So for now
>>> it should be defined for all Exynos5250 boards.
>>>
>>
>> Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.
>>
>
> Sounds good.
>
>
>>
>>
>>>
>>>
>>>>
>>>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>>>>
>>>> @@ -78,7 +77,6 @@
>>>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>>>>
>>>>  /* select serial console configuration */
>>>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>>  #define CONFIG_BAUDRATE                        115200
>>>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>>>>
>>>> @@ -148,8 +146,6 @@
>>>>  #define CONFIG_SPL
>>>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>>>>
>>>> -/* specific .lds file */
>>>> -#define CONFIG_SPL_LDSCRIPT
>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>>
>>>
>>> Again I suspect this is a misnomer.
>>>
>>
>> Since all boards might not need this lds file, so how about moving lds
>> file to board/samsung/common and renaming it to exynos5250-uboot-spl.lds.
>> The boards needing this will have to include in their respective config
>> files.
>> Let me know your opinion.
>>
>
> OK, so long has you know of boards that don't need it?
>
>
>>
>>
>>>
>>>
>>>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>>>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>>>>
>>>> @@ -158,7 +154,7 @@
>>>>  /* Miscellaneous configurable options */
>>>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>>>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser
>>>>  */
>>>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
>>>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>>>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
>>>> Size */
>>>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>>>>  #define CONFIG_SYS_MAXARGS             16      /* max number of command
>>>> args */
>>>> @@ -198,7 +194,6 @@
>>>>  /* FLASH and environment organization */
>>>>  #define CONFIG_SYS_NO_FLASH
>>>>  #undef CONFIG_CMD_IMLS
>>>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>>
>>>>  #define CONFIG_SYS_MMC_ENV_DEV         0
>>>>
>>>> @@ -247,9 +242,6 @@
>>>>  #define CONFIG_I2C_EDID
>>>>
>>>>  /* SPI */
>>>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>> -#define CONFIG_SPI_FLASH
>>>> -
>>>>  #ifdef CONFIG_SPI_FLASH
>>>>  #define CONFIG_EXYNOS_SPI
>>>>  #define CONFIG_CMD_SF
>>>> @@ -306,7 +298,6 @@
>>>>  #define CONFIG_SHA256
>>>>
>>>>  /* Display */
>>>> -#define CONFIG_LCD
>>>>  #ifdef CONFIG_LCD
>>>>  #define CONFIG_EXYNOS_FB
>>>>  #define CONFIG_EXYNOS_DP
>>>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
>>>> index 81f83a8..4af1909 100644
>>>> --- a/include/configs/smdk5250.h
>>>> +++ b/include/configs/smdk5250.h
>>>> @@ -25,9 +25,21 @@
>>>>  #ifndef __CONFIG_SMDK_H
>>>>  #define __CONFIG_SMDK_H
>>>>
>>>> -#include <configs/exynos5250-dt.h>
>>>> -
>>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>>>>
>>>> +#define CONFIG_SMDK5250                        /* which is in a
>>>> SMDK5250 */
>>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>> +
>>>> +/* specific .lds file */
>>>> +#define CONFIG_SPL_LDSCRIPT
>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>> +#define CONFIG_SPI_FLASH
>>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>> +
>>>> +/* Display */
>>>> +#define CONFIG_LCD
>>>> +
>>>> +#include <configs/exynos5250-dt.h>
>>>> +
>>>>  #endif /* __CONFIG_SMDK_H */
>>>> diff --git a/include/configs/snow.h b/include/configs/snow.h
>>>> index b8460fd..e940c69 100644
>>>> --- a/include/configs/snow.h
>>>> +++ b/include/configs/snow.h
>>>> @@ -25,9 +25,21 @@
>>>>  #ifndef __CONFIG_SNOW_H
>>>>  #define __CONFIG_SNOW_H
>>>>
>>>> -#include <configs/exynos5250-dt.h>
>>>> -
>>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>>>>
>>>> +#define CONFIG_SMDK5250                        /* which is in a
>>>> SMDK5250 */
>>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>> +
>>>> +/* specific .lds file */
>>>> +#define CONFIG_SPL_LDSCRIPT
>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>> +#define CONFIG_SPI_FLASH
>>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>> +
>>>> +/* Display */
>>>> +#define CONFIG_LCD
>>>> +
>>>> +#include <configs/exynos5250-dt.h>
>>>> +
>>>>  #endif /* __CONFIG_SNOW_H */
>>>>
>>>
>>> The intent with the exynos5250-dt file is that it supports any board with
>>> that chip, so it should enable any feature used by Exynos5250 boards.
>>> Granted that might not suit all boards, which only need a subset of the
>>> features. Perhaps we should create an exynos5250-dt-base.h, with just the
>>> core options defined. Then other boards can include the base file, and
>>> exynos5250-dt can stay as the 'enable everything/ config?
>>>
>>>
>> So as per you suggestion, there would be 3 files. One
>> exynos5250-dt-base.h, second exynos5250-dt.h and third the board specific
>> config file.
>>
>> How about having core options unconditionally enabled in exynos5250-dt.h
>> and other options with #ifdef. The board specific config files can define
>> the other options. This way only 2 files will do.
>>
>> For example, let exynos5250-dt.h has SPI related configs under #ifdef
>> CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
>> based on the spi flash presence in respective boards.
>>
>
>> Let me know your opinion.SPI
>>
>
> Well the problem is who sets CONFIG_SPI_FLASH
IIUC the CONFIG_SPI_FLASH will be set in respective board config file.
Hence only boards want to have SPI boot support will define it.
>
> For us at least, exynos5250-dt is a good upstream target, since we can add
> all features into it and it will should boot on all the different boards.
> It helps to make sure that other boards don't get non-device-tree config
> that breaks this approach.
>
> So I think you do need a base config file. But I think a better name might
> be exynos5250-dt-common.h instead of exynos5250-dt-base.h.
>
> Regards,
> Simon
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



--
with warm regards,
Chander Kashyap
Simon Glass - July 1, 2013, 4:20 a.m.
Hi,

On Mon, Jul 1, 2013 at 1:08 PM, Chander Kashyap
<chander.kashyap@linaro.org>wrote:

> On 28 June 2013 21:20, Simon Glass <sjg@chromium.org> wrote:
> > Hi Inderpal,
> >
> > On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh
> > <inderpal.singh@linaro.org>wrote:
> >
> >> Hi Simon,
> >>
> >> Thanks for review.
> >>
> >>
> >> On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:
> >>
> >>> Hi,
> >>>
> >>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <
> inderpal.singh@linaro.org
> >>> > wrote:
> >>>
> >>>> Not all boards based on exynos5250 have SPI flash, same serial port
> and
> >>>> might
> >>>> not require display and the same lds script. Hence move them to board
> >>>> specific
> >>>> config file.
> >>>>
> >>>
> >>> At least for the serial port this should be controlled by the device
> >>> tree. There are quite a lot of pending patches for exynos, one of which
> >>> enables this and removes the need for the CONFIG_SERIAL3 define.
> >>>
> >>> If you want to have a look, I pushed them to:
> >>>
> >>> http://git.denx.de/u-boot-x86.git in branch 'snow'
> >>>
> >>
> >> I was not aware of this patchset. Thanks for pointing out. I will update
> >> my patchset to use DT for serial.
> >>
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
> >>>> ---
> >>>> v1 was posted as the second patch of [1]
> >>>>
> >>>> Changes in v2:
> >>>>         - split from the patchset at [1]
> >>>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
> >>>>         - rebased to latest u-boot-samsung master branch
> >>>>
> >>>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
> >>>>
> >>>>  include/configs/exynos5250-dt.h |   11 +----------
> >>>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
> >>>>  include/configs/snow.h          |   16 ++++++++++++++--
> >>>>  3 files changed, 29 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/include/configs/exynos5250-dt.h
> >>>> b/include/configs/exynos5250-dt.h
> >>>> index 03b07b2..22e47eb 100644
> >>>> --- a/include/configs/exynos5250-dt.h
> >>>> +++ b/include/configs/exynos5250-dt.h
> >>>> @@ -29,7 +29,6 @@
> >>>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
> >>>>  #define CONFIG_S5P                     /* S5P Family */
> >>>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5
> Family
> >>>> */
> >>>> -#define CONFIG_SMDK5250                        /* which is in a
> >>>> SMDK5250 */
> >>>>
> >>>
> >>> This is a misnomer - it actually means Exynos 5250 I think. The only
> >>> thing it controls is the generation of the SPL packaging tool. So for
> now
> >>> it should be defined for all Exynos5250 boards.
> >>>
> >>
> >> Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.
> >>
> >
> > Sounds good.
> >
> >
> >>
> >>
> >>>
> >>>
> >>>>
> >>>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
> >>>>
> >>>> @@ -78,7 +77,6 @@
> >>>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
> >>>>
> >>>>  /* select serial console configuration */
> >>>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
> >>>>  #define CONFIG_BAUDRATE                        115200
> >>>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
> >>>>
> >>>> @@ -148,8 +146,6 @@
> >>>>  #define CONFIG_SPL
> >>>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
> >>>>
> >>>> -/* specific .lds file */
> >>>> -#define CONFIG_SPL_LDSCRIPT
> >>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
> >>>>
> >>>
> >>> Again I suspect this is a misnomer.
> >>>
> >>
> >> Since all boards might not need this lds file, so how about moving lds
> >> file to board/samsung/common and renaming it to
> exynos5250-uboot-spl.lds.
> >> The boards needing this will have to include in their respective config
> >> files.
> >> Let me know your opinion.
> >>
> >
> > OK, so long has you know of boards that don't need it?
> >
> >
> >>
> >>
> >>>
> >>>
> >>>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
> >>>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
> >>>>
> >>>> @@ -158,7 +154,7 @@
> >>>>  /* Miscellaneous configurable options */
> >>>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
> >>>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser
> >>>>  */
> >>>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
> >>>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
> >>>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
> >>>> Size */
> >>>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size
> */
> >>>>  #define CONFIG_SYS_MAXARGS             16      /* max number of
> command
> >>>> args */
> >>>> @@ -198,7 +194,6 @@
> >>>>  /* FLASH and environment organization */
> >>>>  #define CONFIG_SYS_NO_FLASH
> >>>>  #undef CONFIG_CMD_IMLS
> >>>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
> >>>>
> >>>>  #define CONFIG_SYS_MMC_ENV_DEV         0
> >>>>
> >>>> @@ -247,9 +242,6 @@
> >>>>  #define CONFIG_I2C_EDID
> >>>>
> >>>>  /* SPI */
> >>>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
> >>>> -#define CONFIG_SPI_FLASH
> >>>> -
> >>>>  #ifdef CONFIG_SPI_FLASH
> >>>>  #define CONFIG_EXYNOS_SPI
> >>>>  #define CONFIG_CMD_SF
> >>>> @@ -306,7 +298,6 @@
> >>>>  #define CONFIG_SHA256
> >>>>
> >>>>  /* Display */
> >>>> -#define CONFIG_LCD
> >>>>  #ifdef CONFIG_LCD
> >>>>  #define CONFIG_EXYNOS_FB
> >>>>  #define CONFIG_EXYNOS_DP
> >>>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
> >>>> index 81f83a8..4af1909 100644
> >>>> --- a/include/configs/smdk5250.h
> >>>> +++ b/include/configs/smdk5250.h
> >>>> @@ -25,9 +25,21 @@
> >>>>  #ifndef __CONFIG_SMDK_H
> >>>>  #define __CONFIG_SMDK_H
> >>>>
> >>>> -#include <configs/exynos5250-dt.h>
> >>>> -
> >>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
> >>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
> >>>>
> >>>> +#define CONFIG_SMDK5250                        /* which is in a
> >>>> SMDK5250 */
> >>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
> >>>> +
> >>>> +/* specific .lds file */
> >>>> +#define CONFIG_SPL_LDSCRIPT
> >>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
> >>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
> >>>> +#define CONFIG_SPI_FLASH
> >>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
> >>>> +
> >>>> +/* Display */
> >>>> +#define CONFIG_LCD
> >>>> +
> >>>> +#include <configs/exynos5250-dt.h>
> >>>> +
> >>>>  #endif /* __CONFIG_SMDK_H */
> >>>> diff --git a/include/configs/snow.h b/include/configs/snow.h
> >>>> index b8460fd..e940c69 100644
> >>>> --- a/include/configs/snow.h
> >>>> +++ b/include/configs/snow.h
> >>>> @@ -25,9 +25,21 @@
> >>>>  #ifndef __CONFIG_SNOW_H
> >>>>  #define __CONFIG_SNOW_H
> >>>>
> >>>> -#include <configs/exynos5250-dt.h>
> >>>> -
> >>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
> >>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
> >>>>
> >>>> +#define CONFIG_SMDK5250                        /* which is in a
> >>>> SMDK5250 */
> >>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
> >>>> +
> >>>> +/* specific .lds file */
> >>>> +#define CONFIG_SPL_LDSCRIPT
> >>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
> >>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
> >>>> +#define CONFIG_SPI_FLASH
> >>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
> >>>> +
> >>>> +/* Display */
> >>>> +#define CONFIG_LCD
> >>>> +
> >>>> +#include <configs/exynos5250-dt.h>
> >>>> +
> >>>>  #endif /* __CONFIG_SNOW_H */
> >>>>
> >>>
> >>> The intent with the exynos5250-dt file is that it supports any board
> with
> >>> that chip, so it should enable any feature used by Exynos5250 boards.
> >>> Granted that might not suit all boards, which only need a subset of the
> >>> features. Perhaps we should create an exynos5250-dt-base.h, with just
> the
> >>> core options defined. Then other boards can include the base file, and
> >>> exynos5250-dt can stay as the 'enable everything/ config?
> >>>
> >>>
> >> So as per you suggestion, there would be 3 files. One
> >> exynos5250-dt-base.h, second exynos5250-dt.h and third the board
> specific
> >> config file.
> >>
> >> How about having core options unconditionally enabled in exynos5250-dt.h
> >> and other options with #ifdef. The board specific config files can
> define
> >> the other options. This way only 2 files will do.
> >>
> >> For example, let exynos5250-dt.h has SPI related configs under #ifdef
> >> CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
> >> based on the spi flash presence in respective boards.
> >>
> >
> >> Let me know your opinion.SPI
> >>
> >
> > Well the problem is who sets CONFIG_SPI_FLASH
> IIUC the CONFIG_SPI_FLASH will be set in respective board config file.
> Hence only boards want to have SPI boot support will define it.
>

Yes, so if those boards include exynos5250-dt-common.h they can then define
CONFIG_SPI_FLASH if they want to.

But for exynos5250-dt.h, it can define more things (including
CONFIG_SPI_FLASH) so that it can boot on any board, and provide a good
build test target.

Regards,
Simon


> >
> > For us at least, exynos5250-dt is a good upstream target, since we can
> add
> > all features into it and it will should boot on all the different boards.
> > It helps to make sure that other boards don't get non-device-tree config
> > that breaks this approach.
> >
> > So I think you do need a base config file. But I think a better name
> might
> > be exynos5250-dt-common.h instead of exynos5250-dt-base.h.
> >
> > Regards,
> > Simon
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
>
>
> --
> with warm regards,
> Chander Kashyap
>
Inderpal Singh - July 1, 2013, 10:28 a.m.
Hi Simon,


On 28 June 2013 21:20, Simon Glass <sjg@chromium.org> wrote:

> Hi Inderpal,
>
> On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh <
> inderpal.singh@linaro.org> wrote:
>
>> Hi Simon,
>>
>> Thanks for review.
>>
>>
>> On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi,
>>>
>>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <
>>> inderpal.singh@linaro.org> wrote:
>>>
>>>> Not all boards based on exynos5250 have SPI flash, same serial port and
>>>> might
>>>> not require display and the same lds script. Hence move them to board
>>>> specific
>>>> config file.
>>>>
>>>
>>> At least for the serial port this should be controlled by the device
>>> tree. There are quite a lot of pending patches for exynos, one of which
>>> enables this and removes the need for the CONFIG_SERIAL3 define.
>>>
>>> If you want to have a look, I pushed them to:
>>>
>>> http://git.denx.de/u-boot-x86.git in branch 'snow'
>>>
>>
>> I was not aware of this patchset. Thanks for pointing out. I will update
>> my patchset to use DT for serial.
>>
>>
>>>
>>>>
>>>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>>>> ---
>>>> v1 was posted as the second patch of [1]
>>>>
>>>> Changes in v2:
>>>>         - split from the patchset at [1]
>>>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>>>>         - rebased to latest u-boot-samsung master branch
>>>>
>>>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>>>>
>>>>  include/configs/exynos5250-dt.h |   11 +----------
>>>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>>>>  include/configs/snow.h          |   16 ++++++++++++++--
>>>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/include/configs/exynos5250-dt.h
>>>> b/include/configs/exynos5250-dt.h
>>>> index 03b07b2..22e47eb 100644
>>>> --- a/include/configs/exynos5250-dt.h
>>>> +++ b/include/configs/exynos5250-dt.h
>>>> @@ -29,7 +29,6 @@
>>>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>>>>  #define CONFIG_S5P                     /* S5P Family */
>>>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5 Family
>>>> */
>>>> -#define CONFIG_SMDK5250                        /* which is in a
>>>> SMDK5250 */
>>>>
>>>
>>> This is a misnomer - it actually means Exynos 5250 I think. The only
>>> thing it controls is the generation of the SPL packaging tool. So for now
>>> it should be defined for all Exynos5250 boards.
>>>
>>
>> Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.
>>
>
> Sounds good.
>
>
>>
>>
>>>
>>>
>>>>
>>>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>>>>
>>>> @@ -78,7 +77,6 @@
>>>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>>>>
>>>>  /* select serial console configuration */
>>>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>>  #define CONFIG_BAUDRATE                        115200
>>>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>>>>
>>>> @@ -148,8 +146,6 @@
>>>>  #define CONFIG_SPL
>>>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>>>>
>>>> -/* specific .lds file */
>>>> -#define CONFIG_SPL_LDSCRIPT
>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>>
>>>
>>> Again I suspect this is a misnomer.
>>>
>>
>>  Since all boards might not need this lds file, so how about moving lds
>> file to board/samsung/common and renaming it to exynos5250-uboot-spl.lds.
>> The boards needing this will have to include in their respective config
>> files.
>> Let me know your opinion.
>>
>
> OK, so long has you know of boards that don't need it?
>
>
>>
>>
>>>
>>>
>>>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>>>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>>>>
>>>> @@ -158,7 +154,7 @@
>>>>  /* Miscellaneous configurable options */
>>>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>>>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser
>>>>  */
>>>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
>>>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>>>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
>>>> Size */
>>>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>>>>  #define CONFIG_SYS_MAXARGS             16      /* max number of
>>>> command args */
>>>> @@ -198,7 +194,6 @@
>>>>  /* FLASH and environment organization */
>>>>  #define CONFIG_SYS_NO_FLASH
>>>>  #undef CONFIG_CMD_IMLS
>>>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>>
>>>>  #define CONFIG_SYS_MMC_ENV_DEV         0
>>>>
>>>> @@ -247,9 +242,6 @@
>>>>  #define CONFIG_I2C_EDID
>>>>
>>>>  /* SPI */
>>>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>> -#define CONFIG_SPI_FLASH
>>>> -
>>>>  #ifdef CONFIG_SPI_FLASH
>>>>  #define CONFIG_EXYNOS_SPI
>>>>  #define CONFIG_CMD_SF
>>>> @@ -306,7 +298,6 @@
>>>>  #define CONFIG_SHA256
>>>>
>>>>  /* Display */
>>>> -#define CONFIG_LCD
>>>>  #ifdef CONFIG_LCD
>>>>  #define CONFIG_EXYNOS_FB
>>>>  #define CONFIG_EXYNOS_DP
>>>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
>>>> index 81f83a8..4af1909 100644
>>>> --- a/include/configs/smdk5250.h
>>>> +++ b/include/configs/smdk5250.h
>>>> @@ -25,9 +25,21 @@
>>>>  #ifndef __CONFIG_SMDK_H
>>>>  #define __CONFIG_SMDK_H
>>>>
>>>> -#include <configs/exynos5250-dt.h>
>>>> -
>>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>>>>
>>>> +#define CONFIG_SMDK5250                        /* which is in a
>>>> SMDK5250 */
>>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>> +
>>>> +/* specific .lds file */
>>>> +#define CONFIG_SPL_LDSCRIPT
>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>> +#define CONFIG_SPI_FLASH
>>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>> +
>>>> +/* Display */
>>>> +#define CONFIG_LCD
>>>> +
>>>> +#include <configs/exynos5250-dt.h>
>>>> +
>>>>  #endif /* __CONFIG_SMDK_H */
>>>> diff --git a/include/configs/snow.h b/include/configs/snow.h
>>>> index b8460fd..e940c69 100644
>>>> --- a/include/configs/snow.h
>>>> +++ b/include/configs/snow.h
>>>> @@ -25,9 +25,21 @@
>>>>  #ifndef __CONFIG_SNOW_H
>>>>  #define __CONFIG_SNOW_H
>>>>
>>>> -#include <configs/exynos5250-dt.h>
>>>> -
>>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>>>>
>>>> +#define CONFIG_SMDK5250                        /* which is in a
>>>> SMDK5250 */
>>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>> +
>>>> +/* specific .lds file */
>>>> +#define CONFIG_SPL_LDSCRIPT
>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>> +#define CONFIG_SPI_FLASH
>>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>> +
>>>> +/* Display */
>>>> +#define CONFIG_LCD
>>>> +
>>>> +#include <configs/exynos5250-dt.h>
>>>> +
>>>>  #endif /* __CONFIG_SNOW_H */
>>>>
>>>
>>> The intent with the exynos5250-dt file is that it supports any board
>>> with that chip, so it should enable any feature used by Exynos5250 boards.
>>> Granted that might not suit all boards, which only need a subset of the
>>> features. Perhaps we should create an exynos5250-dt-base.h, with just the
>>> core options defined. Then other boards can include the base file, and
>>> exynos5250-dt can stay as the 'enable everything/ config?
>>>
>>>
>> So as per you suggestion, there would be 3 files. One
>> exynos5250-dt-base.h, second exynos5250-dt.h and third the board specific
>> config file.
>>
>> How about having core options unconditionally enabled in exynos5250-dt.h
>> and other options with #ifdef. The board specific config files can define
>> the other options. This way only 2 files will do.
>>
>> For example, let exynos5250-dt.h has SPI related configs under #ifdef
>> CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
>> based on the spi flash presence in respective boards.
>>
>
>> Let me know your opinion.
>>
>
> Well the problem is who sets CONFIG_SPI_FLASH
>
> For us at least, exynos5250-dt is a good upstream target, since we can add
> all features into it and it will should boot on all the different boards.
> It helps to make sure that other boards don't get non-device-tree config
> that breaks this approach.
>
> So I think you do need a base config file. But I think a better name might
> be exynos5250-dt-common.h instead of exynos5250-dt-base.h.
>

As per my understanding the exynos5250-dt.h was meant to be common for all
exynos5250 based boards. Creating one more base common file looks a bit
overdone to me.

I will drop this patch. And for Arndale board support, I will just #undef
the config options which are not needed as of now.

Thanks,
Inder



> Regards,
> Simon
>
>
Simon Glass - July 2, 2013, 2:53 a.m.
Hi,

On Mon, Jul 1, 2013 at 7:28 PM, Inderpal Singh <inderpal.singh@linaro.org>wrote:

> Hi Simon,
>
>
> On 28 June 2013 21:20, Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Inderpal,
>>
>> On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh <
>> inderpal.singh@linaro.org> wrote:
>>
>>> Hi Simon,
>>>
>>> Thanks for review.
>>>
>>>
>>> On 11 June 2013 19:57, Simon Glass <sjg@chromium.org> wrote:
>>>
>>>> Hi,
>>>>
>>>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <
>>>> inderpal.singh@linaro.org> wrote:
>>>>
>>>>> Not all boards based on exynos5250 have SPI flash, same serial port
>>>>> and might
>>>>> not require display and the same lds script. Hence move them to board
>>>>> specific
>>>>> config file.
>>>>>
>>>>
>>>> At least for the serial port this should be controlled by the device
>>>> tree. There are quite a lot of pending patches for exynos, one of which
>>>> enables this and removes the need for the CONFIG_SERIAL3 define.
>>>>
>>>> If you want to have a look, I pushed them to:
>>>>
>>>> http://git.denx.de/u-boot-x86.git in branch 'snow'
>>>>
>>>
>>> I was not aware of this patchset. Thanks for pointing out. I will update
>>> my patchset to use DT for serial.
>>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>>>>> ---
>>>>> v1 was posted as the second patch of [1]
>>>>>
>>>>> Changes in v2:
>>>>>         - split from the patchset at [1]
>>>>>         - moved CONFIG_LCD and CONFIG_SPI_FLASH
>>>>>         - rebased to latest u-boot-samsung master branch
>>>>>
>>>>> [1] http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/157101
>>>>>
>>>>>  include/configs/exynos5250-dt.h |   11 +----------
>>>>>  include/configs/smdk5250.h      |   16 ++++++++++++++--
>>>>>  include/configs/snow.h          |   16 ++++++++++++++--
>>>>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/include/configs/exynos5250-dt.h
>>>>> b/include/configs/exynos5250-dt.h
>>>>> index 03b07b2..22e47eb 100644
>>>>> --- a/include/configs/exynos5250-dt.h
>>>>> +++ b/include/configs/exynos5250-dt.h
>>>>> @@ -29,7 +29,6 @@
>>>>>  #define CONFIG_SAMSUNG                 /* in a SAMSUNG core */
>>>>>  #define CONFIG_S5P                     /* S5P Family */
>>>>>  #define CONFIG_EXYNOS5                 /* which is in a Exynos5
>>>>> Family */
>>>>> -#define CONFIG_SMDK5250                        /* which is in a
>>>>> SMDK5250 */
>>>>>
>>>>
>>>> This is a misnomer - it actually means Exynos 5250 I think. The only
>>>> thing it controls is the generation of the SPL packaging tool. So for now
>>>> it should be defined for all Exynos5250 boards.
>>>>
>>>
>>> Yes its a misnomer. I will change the name to CONFIG_EXYNOS5250.
>>>
>>
>> Sounds good.
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>  #include <asm/arch/cpu.h>              /* get chip and board defs */
>>>>>
>>>>> @@ -78,7 +77,6 @@
>>>>>  #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + (4 << 20))
>>>>>
>>>>>  /* select serial console configuration */
>>>>> -#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>>>  #define CONFIG_BAUDRATE                        115200
>>>>>  #define EXYNOS5_DEFAULT_UART_OFFSET    0x010000
>>>>>
>>>>> @@ -148,8 +146,6 @@
>>>>>  #define CONFIG_SPL
>>>>>  #define COPY_BL2_FNPTR_ADDR    0x02020030
>>>>>
>>>>> -/* specific .lds file */
>>>>> -#define CONFIG_SPL_LDSCRIPT
>>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>>>
>>>>
>>>> Again I suspect this is a misnomer.
>>>>
>>>
>>>  Since all boards might not need this lds file, so how about moving lds
>>> file to board/samsung/common and renaming it to exynos5250-uboot-spl.lds.
>>> The boards needing this will have to include in their respective config
>>> files.
>>> Let me know your opinion.
>>>
>>
>> OK, so long has you know of boards that don't need it?
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>  #define CONFIG_SPL_TEXT_BASE   0x02023400
>>>>>  #define CONFIG_SPL_MAX_FOOTPRINT       (14 * 1024)
>>>>>
>>>>> @@ -158,7 +154,7 @@
>>>>>  /* Miscellaneous configurable options */
>>>>>  #define CONFIG_SYS_LONGHELP            /* undef to save memory */
>>>>>  #define CONFIG_SYS_HUSH_PARSER         /* use "hush" command parser
>>>>>  */
>>>>> -#define CONFIG_SYS_PROMPT              "SMDK5250 # "
>>>>> +#define CONFIG_SYS_PROMPT              "EXYNOS5250 # "
>>>>>  #define CONFIG_SYS_CBSIZE              256     /* Console I/O Buffer
>>>>> Size */
>>>>>  #define CONFIG_SYS_PBSIZE              384     /* Print Buffer Size */
>>>>>  #define CONFIG_SYS_MAXARGS             16      /* max number of
>>>>> command args */
>>>>> @@ -198,7 +194,6 @@
>>>>>  /* FLASH and environment organization */
>>>>>  #define CONFIG_SYS_NO_FLASH
>>>>>  #undef CONFIG_CMD_IMLS
>>>>> -#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>>>
>>>>>  #define CONFIG_SYS_MMC_ENV_DEV         0
>>>>>
>>>>> @@ -247,9 +242,6 @@
>>>>>  #define CONFIG_I2C_EDID
>>>>>
>>>>>  /* SPI */
>>>>> -#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>>> -#define CONFIG_SPI_FLASH
>>>>> -
>>>>>  #ifdef CONFIG_SPI_FLASH
>>>>>  #define CONFIG_EXYNOS_SPI
>>>>>  #define CONFIG_CMD_SF
>>>>> @@ -306,7 +298,6 @@
>>>>>  #define CONFIG_SHA256
>>>>>
>>>>>  /* Display */
>>>>> -#define CONFIG_LCD
>>>>>  #ifdef CONFIG_LCD
>>>>>  #define CONFIG_EXYNOS_FB
>>>>>  #define CONFIG_EXYNOS_DP
>>>>> diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
>>>>> index 81f83a8..4af1909 100644
>>>>> --- a/include/configs/smdk5250.h
>>>>> +++ b/include/configs/smdk5250.h
>>>>> @@ -25,9 +25,21 @@
>>>>>  #ifndef __CONFIG_SMDK_H
>>>>>  #define __CONFIG_SMDK_H
>>>>>
>>>>> -#include <configs/exynos5250-dt.h>
>>>>> -
>>>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-smdk5250
>>>>>
>>>>> +#define CONFIG_SMDK5250                        /* which is in a
>>>>> SMDK5250 */
>>>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>>> +
>>>>> +/* specific .lds file */
>>>>> +#define CONFIG_SPL_LDSCRIPT
>>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>>> +#define CONFIG_SPI_FLASH
>>>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>>> +
>>>>> +/* Display */
>>>>> +#define CONFIG_LCD
>>>>> +
>>>>> +#include <configs/exynos5250-dt.h>
>>>>> +
>>>>>  #endif /* __CONFIG_SMDK_H */
>>>>> diff --git a/include/configs/snow.h b/include/configs/snow.h
>>>>> index b8460fd..e940c69 100644
>>>>> --- a/include/configs/snow.h
>>>>> +++ b/include/configs/snow.h
>>>>> @@ -25,9 +25,21 @@
>>>>>  #ifndef __CONFIG_SNOW_H
>>>>>  #define __CONFIG_SNOW_H
>>>>>
>>>>> -#include <configs/exynos5250-dt.h>
>>>>> -
>>>>>  #undef CONFIG_DEFAULT_DEVICE_TREE
>>>>>  #define CONFIG_DEFAULT_DEVICE_TREE     exynos5250-snow
>>>>>
>>>>> +#define CONFIG_SMDK5250                        /* which is in a
>>>>> SMDK5250 */
>>>>> +#define CONFIG_SERIAL3                 /* use SERIAL 3 */
>>>>> +
>>>>> +/* specific .lds file */
>>>>> +#define CONFIG_SPL_LDSCRIPT
>>>>>  "board/samsung/smdk5250/smdk5250-uboot-spl.lds"
>>>>> +#define CONFIG_IDENT_STRING            " for SMDK5250"
>>>>> +#define CONFIG_SPI_FLASH
>>>>> +#define CONFIG_ENV_IS_IN_SPI_FLASH
>>>>> +
>>>>> +/* Display */
>>>>> +#define CONFIG_LCD
>>>>> +
>>>>> +#include <configs/exynos5250-dt.h>
>>>>> +
>>>>>  #endif /* __CONFIG_SNOW_H */
>>>>>
>>>>
>>>> The intent with the exynos5250-dt file is that it supports any board
>>>> with that chip, so it should enable any feature used by Exynos5250 boards.
>>>> Granted that might not suit all boards, which only need a subset of the
>>>> features. Perhaps we should create an exynos5250-dt-base.h, with just the
>>>> core options defined. Then other boards can include the base file, and
>>>> exynos5250-dt can stay as the 'enable everything/ config?
>>>>
>>>>
>>> So as per you suggestion, there would be 3 files. One
>>> exynos5250-dt-base.h, second exynos5250-dt.h and third the board specific
>>> config file.
>>>
>>> How about having core options unconditionally enabled in exynos5250-dt.h
>>> and other options with #ifdef. The board specific config files can define
>>> the other options. This way only 2 files will do.
>>>
>>> For example, let exynos5250-dt.h has SPI related configs under #ifdef
>>> CONFIG_SPI_FLASH and let smdk5250.h or arndale.h define CONFIG_SPI_FLASH
>>> based on the spi flash presence in respective boards.
>>>
>>
>>> Let me know your opinion.
>>>
>>
>> Well the problem is who sets CONFIG_SPI_FLASH
>>
>> For us at least, exynos5250-dt is a good upstream target, since we can
>> add all features into it and it will should boot on all the different
>> boards. It helps to make sure that other boards don't get non-device-tree
>> config that breaks this approach.
>>
>> So I think you do need a base config file. But I think a better name
>> might be exynos5250-dt-common.h instead of exynos5250-dt-base.h.
>>
>
> As per my understanding the exynos5250-dt.h was meant to be common for all
> exynos5250 based boards. Creating one more base common file looks a bit
> overdone to me.
>
>
Yes it is supposed to be able to boot on all boards. But if you are wanting
to #undef options, then you could create a new board.


> I will drop this patch. And for Arndale board support, I will just #undef
> the config options which are not needed as of now.
>

OK.

Regards,
Simon

Patch

diff --git a/include/configs/exynos5250-dt.h b/include/configs/exynos5250-dt.h
index 03b07b2..22e47eb 100644
--- a/include/configs/exynos5250-dt.h
+++ b/include/configs/exynos5250-dt.h
@@ -29,7 +29,6 @@ 
 #define CONFIG_SAMSUNG			/* in a SAMSUNG core */
 #define CONFIG_S5P			/* S5P Family */
 #define CONFIG_EXYNOS5			/* which is in a Exynos5 Family */
-#define CONFIG_SMDK5250			/* which is in a SMDK5250 */
 
 #include <asm/arch/cpu.h>		/* get chip and board defs */
 
@@ -78,7 +77,6 @@ 
 #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + (4 << 20))
 
 /* select serial console configuration */
-#define CONFIG_SERIAL3			/* use SERIAL 3 */
 #define CONFIG_BAUDRATE			115200
 #define EXYNOS5_DEFAULT_UART_OFFSET	0x010000
 
@@ -148,8 +146,6 @@ 
 #define CONFIG_SPL
 #define COPY_BL2_FNPTR_ADDR	0x02020030
 
-/* specific .lds file */
-#define CONFIG_SPL_LDSCRIPT	"board/samsung/smdk5250/smdk5250-uboot-spl.lds"
 #define CONFIG_SPL_TEXT_BASE	0x02023400
 #define CONFIG_SPL_MAX_FOOTPRINT	(14 * 1024)
 
@@ -158,7 +154,7 @@ 
 /* Miscellaneous configurable options */
 #define CONFIG_SYS_LONGHELP		/* undef to save memory */
 #define CONFIG_SYS_HUSH_PARSER		/* use "hush" command parser	*/
-#define CONFIG_SYS_PROMPT		"SMDK5250 # "
+#define CONFIG_SYS_PROMPT		"EXYNOS5250 # "
 #define CONFIG_SYS_CBSIZE		256	/* Console I/O Buffer Size */
 #define CONFIG_SYS_PBSIZE		384	/* Print Buffer Size */
 #define CONFIG_SYS_MAXARGS		16	/* max number of command args */
@@ -198,7 +194,6 @@ 
 /* FLASH and environment organization */
 #define CONFIG_SYS_NO_FLASH
 #undef CONFIG_CMD_IMLS
-#define CONFIG_IDENT_STRING		" for SMDK5250"
 
 #define CONFIG_SYS_MMC_ENV_DEV		0
 
@@ -247,9 +242,6 @@ 
 #define CONFIG_I2C_EDID
 
 /* SPI */
-#define CONFIG_ENV_IS_IN_SPI_FLASH
-#define CONFIG_SPI_FLASH
-
 #ifdef CONFIG_SPI_FLASH
 #define CONFIG_EXYNOS_SPI
 #define CONFIG_CMD_SF
@@ -306,7 +298,6 @@ 
 #define CONFIG_SHA256
 
 /* Display */
-#define CONFIG_LCD
 #ifdef CONFIG_LCD
 #define CONFIG_EXYNOS_FB
 #define CONFIG_EXYNOS_DP
diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h
index 81f83a8..4af1909 100644
--- a/include/configs/smdk5250.h
+++ b/include/configs/smdk5250.h
@@ -25,9 +25,21 @@ 
 #ifndef __CONFIG_SMDK_H
 #define __CONFIG_SMDK_H
 
-#include <configs/exynos5250-dt.h>
-
 #undef CONFIG_DEFAULT_DEVICE_TREE
 #define CONFIG_DEFAULT_DEVICE_TREE	exynos5250-smdk5250
 
+#define CONFIG_SMDK5250			/* which is in a SMDK5250 */
+#define CONFIG_SERIAL3			/* use SERIAL 3 */
+
+/* specific .lds file */
+#define CONFIG_SPL_LDSCRIPT	"board/samsung/smdk5250/smdk5250-uboot-spl.lds"
+#define CONFIG_IDENT_STRING		" for SMDK5250"
+#define CONFIG_SPI_FLASH
+#define CONFIG_ENV_IS_IN_SPI_FLASH
+
+/* Display */
+#define CONFIG_LCD
+
+#include <configs/exynos5250-dt.h>
+
 #endif	/* __CONFIG_SMDK_H */
diff --git a/include/configs/snow.h b/include/configs/snow.h
index b8460fd..e940c69 100644
--- a/include/configs/snow.h
+++ b/include/configs/snow.h
@@ -25,9 +25,21 @@ 
 #ifndef __CONFIG_SNOW_H
 #define __CONFIG_SNOW_H
 
-#include <configs/exynos5250-dt.h>
-
 #undef CONFIG_DEFAULT_DEVICE_TREE
 #define CONFIG_DEFAULT_DEVICE_TREE	exynos5250-snow
 
+#define CONFIG_SMDK5250			/* which is in a SMDK5250 */
+#define CONFIG_SERIAL3			/* use SERIAL 3 */
+
+/* specific .lds file */
+#define CONFIG_SPL_LDSCRIPT	"board/samsung/smdk5250/smdk5250-uboot-spl.lds"
+#define CONFIG_IDENT_STRING		" for SMDK5250"
+#define CONFIG_SPI_FLASH
+#define CONFIG_ENV_IS_IN_SPI_FLASH
+
+/* Display */
+#define CONFIG_LCD
+
+#include <configs/exynos5250-dt.h>
+
 #endif	/* __CONFIG_SNOW_H */