[U-Boot] [PATCH v2] exynos5250: move board specific configs to board specific config file
Inderpal Singh
inderpal.singh at linaro.org
Mon Jul 1 12:28:19 CEST 2013
Hi Simon,
On 28 June 2013 21:20, Simon Glass <sjg at chromium.org> wrote:
> Hi Inderpal,
>
> On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh <
> inderpal.singh at linaro.org> wrote:
>
>> Hi Simon,
>>
>> Thanks for review.
>>
>>
>> On 11 June 2013 19:57, Simon Glass <sjg at chromium.org> wrote:
>>
>>> Hi,
>>>
>>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh <
>>> inderpal.singh at 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 at 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
>
>
More information about the U-Boot
mailing list