Hi, On Mon, Jul 1, 2013 at 7:28 PM, Inderpal Singh <inderpal.si...@linaro.org>wrote:
> Hi Simon, > > > On 28 June 2013 21:20, Simon Glass <s...@chromium.org> wrote: > >> Hi Inderpal, >> >> On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh < >> inderpal.si...@linaro.org> wrote: >> >>> Hi Simon, >>> >>> Thanks for review. >>> >>> >>> On 11 June 2013 19:57, Simon Glass <s...@chromium.org> wrote: >>> >>>> Hi, >>>> >>>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh < >>>> inderpal.si...@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.si...@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
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
