Hi, On Mon, Jul 1, 2013 at 1:08 PM, Chander Kashyap <[email protected]>wrote:
> On 28 June 2013 21:20, Simon Glass <[email protected]> wrote: > > Hi Inderpal, > > > > On Thu, Jun 20, 2013 at 12:10 AM, Inderpal Singh > > <[email protected]>wrote: > > > >> Hi Simon, > >> > >> Thanks for review. > >> > >> > >> On 11 June 2013 19:57, Simon Glass <[email protected]> wrote: > >> > >>> Hi, > >>> > >>> On Fri, Jun 7, 2013 at 4:56 AM, Inderpal Singh < > [email protected] > >>> > 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 <[email protected]> > >>>> --- > >>>> 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 > > [email protected] > > http://lists.denx.de/mailman/listinfo/u-boot > > > > > > -- > with warm regards, > Chander Kashyap >
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
