Comments

A number of boards are populated with a PoP chip for both DDR and NAND
memory. Other boards may simply use this as an easy way to identify
board revs. So we provide a function that can be called early to reset
the NAND chip and return the result of NAND_CMD_READID. All of this
code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.
Signed-off-by: Tom Rini <trini@ti.com>
---
arch/arm/cpu/armv7/omap3/Makefile | 3 +
arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++
arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +
3 files changed, 91 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c

On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Tom,>> On 11/19/11 00:48, Tom Rini wrote:>> A number of boards are populated with a PoP chip for both DDR and NAND>> memory. Other boards may simply use this as an easy way to identify>> board revs. So we provide a function that can be called early to reset>> the NAND chip and return the result of NAND_CMD_READID. All of this>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>> Signed-off-by: Tom Rini <trini@ti.com>>> --->> arch/arm/cpu/armv7/omap3/Makefile | 3 +>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>> 3 files changed, 91 insertions(+), 0 deletions(-)>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>> index 8e85891..4b38e45 100644>> --- a/arch/arm/cpu/armv7/omap3/Makefile>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>> @@ -31,6 +31,9 @@ COBJS += board.o>> COBJS += clock.o>> COBJS += mem.o>> COBJS += sys_info.o>> +ifdef CONFIG_SPL_BUILD>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>> +endif>> You haven't responded to my question on the above stuff.> Otherwise all the series look good to me.
Missed that, sorry!
>> Original version available at:> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>> Here is the relevant part:>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>> >>> index 8e85891..772f3d4 100644>>>> >>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>> >>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>> >>> @@ -31,6 +31,9 @@ COBJS += board.o>>>> >>> COBJS += clock.o>>>> >>> COBJS += mem.o>>>> >>> COBJS += sys_info.o>>>> >>> +ifdef CONFIG_SPL_BUILD>>>> >>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>> >>> +endif>>> >>>>> >> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>> >> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>> >> it in #ifdef?>> >>> > But then it would build for both SPL and non-SPL cases.>> No, it should not.> What do you think of the following:> In the Makefile have only:> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>> Then in the spl_pop_probe.c have this type of check:> #ifndef CONFIG_SPL_BUILD> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD> #endif>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol> be a part of the CONFIG_SPL_BUILD symbols group.
Well, if we always link this, but then #error, U-Boot won't build :)
I guess the reason to not #ifndef CONFIG_SPL_BUILD the whole file is
that the normal style for SPL is to only include the file when
building for SPL.

On 11/20/11 16:26, Tom Rini wrote:
> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>> Hi Tom,>>>> On 11/19/11 00:48, Tom Rini wrote:>>> A number of boards are populated with a PoP chip for both DDR and NAND>>> memory. Other boards may simply use this as an easy way to identify>>> board revs. So we provide a function that can be called early to reset>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>> --->>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>> index 8e85891..4b38e45 100644>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>> @@ -31,6 +31,9 @@ COBJS += board.o>>> COBJS += clock.o>>> COBJS += mem.o>>> COBJS += sys_info.o>>> +ifdef CONFIG_SPL_BUILD>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>> +endif>>>> You haven't responded to my question on the above stuff.>> Otherwise all the series look good to me.> > Missed that, sorry!> >>>> Original version available at:>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>> Here is the relevant part:>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>> COBJS += clock.o>>>>>>>> COBJS += mem.o>>>>>>>> COBJS += sys_info.o>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>> +endif>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>> it in #ifdef?>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>> No, it should not.>> What do you think of the following:>> In the Makefile have only:>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>> Then in the spl_pop_probe.c have this type of check:>> #ifndef CONFIG_SPL_BUILD>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>> #endif>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>> be a part of the CONFIG_SPL_BUILD symbols group.> > Well, if we always link this, but then #error, U-Boot won't build :)
No you do not always link this... please, read more carefully...
Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will
be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without
CONFIG_SPL_BUILD being defined, then it will emit an error.
> I guess the reason to not #ifndef CONFIG_SPL_BUILD the whole file is> that the normal style for SPL is to only include the file when> building for SPL.
I don't understand the sentence above and
the way it is related to my question.

On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> On 11/20/11 16:26, Tom Rini wrote:>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>> Hi Tom,>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>> memory. Other boards may simply use this as an easy way to identify>>>> board revs. So we provide a function that can be called early to reset>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>> --->>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>> index 8e85891..4b38e45 100644>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>> COBJS += clock.o>>>> COBJS += mem.o>>>> COBJS += sys_info.o>>>> +ifdef CONFIG_SPL_BUILD>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>> +endif>>>>>> You haven't responded to my question on the above stuff.>>> Otherwise all the series look good to me.>>>> Missed that, sorry!>>>>>>>> Original version available at:>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>> Here is the relevant part:>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>> COBJS += clock.o>>>>>>>>> COBJS += mem.o>>>>>>>>> COBJS += sys_info.o>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>> +endif>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>> it in #ifdef?>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>> No, it should not.>>> What do you think of the following:>>> In the Makefile have only:>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>> Then in the spl_pop_probe.c have this type of check:>>> #ifndef CONFIG_SPL_BUILD>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>> #endif>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>> Well, if we always link this, but then #error, U-Boot won't build :)>> No you do not always link this... please, read more carefully...> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without> CONFIG_SPL_BUILD being defined, then it will emit an error.
So make the config file do:
#ifdef CONFIG_SPL_BUILD
#define CONFIG_SPL_OMAP3_POP_PROBE
#endif
? That's now how the rest of the SPL code works.

On 11/21/11 16:12, Tom Rini wrote:
> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>> On 11/20/11 16:26, Tom Rini wrote:>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>> Hi Tom,>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>> memory. Other boards may simply use this as an easy way to identify>>>>> board revs. So we provide a function that can be called early to reset>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>> --->>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>> index 8e85891..4b38e45 100644>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>> COBJS += clock.o>>>>> COBJS += mem.o>>>>> COBJS += sys_info.o>>>>> +ifdef CONFIG_SPL_BUILD>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>> +endif>>>>>>>> You haven't responded to my question on the above stuff.>>>> Otherwise all the series look good to me.>>>>>> Missed that, sorry!>>>>>>>>>>> Original version available at:>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>> Here is the relevant part:>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>> COBJS += clock.o>>>>>>>>>> COBJS += mem.o>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>> +endif>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>> it in #ifdef?>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>> No, it should not.>>>> What do you think of the following:>>>> In the Makefile have only:>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>> #ifndef CONFIG_SPL_BUILD>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>> #endif>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>> No you do not always link this... please, read more carefully...>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>> CONFIG_SPL_BUILD being defined, then it will emit an error.> > So make the config file do:> #ifdef CONFIG_SPL_BUILD> #define CONFIG_SPL_OMAP3_POP_PROBE> #endif> ? That's now how the rest of the SPL code works.
Well, yes I think it makes sense for all SPL related config options
to do something like:
#ifdef CONFIG_SPL_BUILD
#define CONFIG_SPL_OMAP3_POP_PROBE
#define CONFIG_SPL_...
#define CONFIG_SPL_...
#endif
And the error message, I have proposed above, will prevent
people from doing stupid things, like defining
CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.
At least for now, until we have Kbuild with dependencies and stuff...

On 11/21/2011 07:41 AM, Igor Grinberg wrote:
> On 11/21/11 16:12, Tom Rini wrote:>> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>> On 11/20/11 16:26, Tom Rini wrote:>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>> Hi Tom,>>>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>>> memory. Other boards may simply use this as an easy way to identify>>>>>> board revs. So we provide a function that can be called early to reset>>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>>> --->>>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>> index 8e85891..4b38e45 100644>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>> COBJS += clock.o>>>>>> COBJS += mem.o>>>>>> COBJS += sys_info.o>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>>> +endif>>>>>>>>>> You haven't responded to my question on the above stuff.>>>>> Otherwise all the series look good to me.>>>>>>>> Missed that, sorry!>>>>>>>>>>>>>> Original version available at:>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>>>> Here is the relevant part:>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>>> it in #ifdef?>>>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>>>> No, it should not.>>>>> What do you think of the following:>>>>> In the Makefile have only:>>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>>> #ifndef CONFIG_SPL_BUILD>>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>>> #endif>>>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>>>> No you do not always link this... please, read more carefully...>>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>>> CONFIG_SPL_BUILD being defined, then it will emit an error.>>>> So make the config file do:>> #ifdef CONFIG_SPL_BUILD>> #define CONFIG_SPL_OMAP3_POP_PROBE>> #endif>> ? That's now how the rest of the SPL code works.> > Well, yes I think it makes sense for all SPL related config options> to do something like:> #ifdef CONFIG_SPL_BUILD> #define CONFIG_SPL_OMAP3_POP_PROBE> #define CONFIG_SPL_...> #define CONFIG_SPL_...> #endif> > And the error message, I have proposed above, will prevent> people from doing stupid things, like defining> CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.> At least for now, until we have Kbuild with dependencies and stuff...
Well, I guess the point I'd try and make is that it's not how SPL is
done today. Really following the existing format would be (in the
Makefile):
ifdef CONFIG_SPL_BUILD
ifdef CONFIG_SPL_OMAP3_ID_NAND
COBJS-y += spl_id_nand.o
endif
endif
I can see the point you're making but I'm asking if we need to change
everyone around to your suggested way of building before we can merge
these changes in? Thanks!

On 11/21/11 17:33, Tom Rini wrote:
> On 11/21/2011 07:41 AM, Igor Grinberg wrote:>> On 11/21/11 16:12, Tom Rini wrote:>>> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>> On 11/20/11 16:26, Tom Rini wrote:>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>> Hi Tom,>>>>>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>>>> memory. Other boards may simply use this as an easy way to identify>>>>>>> board revs. So we provide a function that can be called early to reset>>>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>>>> --->>>>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>> index 8e85891..4b38e45 100644>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>> COBJS += clock.o>>>>>>> COBJS += mem.o>>>>>>> COBJS += sys_info.o>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>>>> +endif>>>>>>>>>>>> You haven't responded to my question on the above stuff.>>>>>> Otherwise all the series look good to me.>>>>>>>>>> Missed that, sorry!>>>>>>>>>>>>>>>>> Original version available at:>>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>>>>>> Here is the relevant part:>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>>>> it in #ifdef?>>>>>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>>>>>> No, it should not.>>>>>> What do you think of the following:>>>>>> In the Makefile have only:>>>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>>>> #ifndef CONFIG_SPL_BUILD>>>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>>>> #endif>>>>>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>>>>>> No you do not always link this... please, read more carefully...>>>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>>>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>>>> CONFIG_SPL_BUILD being defined, then it will emit an error.>>>>>> So make the config file do:>>> #ifdef CONFIG_SPL_BUILD>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>> #endif>>> ? That's now how the rest of the SPL code works.>>>> Well, yes I think it makes sense for all SPL related config options>> to do something like:>> #ifdef CONFIG_SPL_BUILD>> #define CONFIG_SPL_OMAP3_POP_PROBE>> #define CONFIG_SPL_...>> #define CONFIG_SPL_...>> #endif>>>> And the error message, I have proposed above, will prevent>> people from doing stupid things, like defining>> CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.>> At least for now, until we have Kbuild with dependencies and stuff...> > Well, I guess the point I'd try and make is that it's not how SPL is> done today. Really following the existing format would be (in the> Makefile):> ifdef CONFIG_SPL_BUILD> ifdef CONFIG_SPL_OMAP3_ID_NAND> COBJS-y += spl_id_nand.o> endif> endif
This is bad!
We don't want the code to look like the above crap, do we?
Because next thing will be even worth:
ifdef CONFIG_SPL_BUILD
ifdef CONFIG_SPL_OMAP3_ID_NAND
ifdef CONFIG_SPL_OMAP3_ID_NAND_SHIT...
COBJS-y += spl_id_nand_shit...o
endif
endif
endif
> > I can see the point you're making but I'm asking if we need to change> everyone around to your suggested way of building before we can merge> these changes in? Thanks!
Ok. I understand your point. No, I don't think we should.
The real question is, do we want it look like the above crap?
If not, then please, do it right in this patch and all the rest
can be changed later.
Also would be nice to make all future patches do the right thing.

On 11/22/2011 07:33 AM, Igor Grinberg wrote:
> On 11/21/11 17:33, Tom Rini wrote:>> On 11/21/2011 07:41 AM, Igor Grinberg wrote:>>> On 11/21/11 16:12, Tom Rini wrote:>>>> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>> On 11/20/11 16:26, Tom Rini wrote:>>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>>> Hi Tom,>>>>>>>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>>>>> memory. Other boards may simply use this as an easy way to identify>>>>>>>> board revs. So we provide a function that can be called early to reset>>>>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>>>>> --->>>>>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>> index 8e85891..4b38e45 100644>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>> COBJS += clock.o>>>>>>>> COBJS += mem.o>>>>>>>> COBJS += sys_info.o>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>>>>> +endif>>>>>>>>>>>>>> You haven't responded to my question on the above stuff.>>>>>>> Otherwise all the series look good to me.>>>>>>>>>>>> Missed that, sorry!>>>>>>>>>>>>>>>>>>>> Original version available at:>>>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>>>>>>>> Here is the relevant part:>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>>>>> it in #ifdef?>>>>>>>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>>>>>>>> No, it should not.>>>>>>> What do you think of the following:>>>>>>> In the Makefile have only:>>>>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>>>>> #ifndef CONFIG_SPL_BUILD>>>>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>>>>> #endif>>>>>>>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>>>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>>>>>>>> No you do not always link this... please, read more carefully...>>>>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>>>>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>>>>> CONFIG_SPL_BUILD being defined, then it will emit an error.>>>>>>>> So make the config file do:>>>> #ifdef CONFIG_SPL_BUILD>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>> #endif>>>> ? That's now how the rest of the SPL code works.>>>>>> Well, yes I think it makes sense for all SPL related config options>>> to do something like:>>> #ifdef CONFIG_SPL_BUILD>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>> #define CONFIG_SPL_...>>> #define CONFIG_SPL_...>>> #endif>>>>>> And the error message, I have proposed above, will prevent>>> people from doing stupid things, like defining>>> CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.>>> At least for now, until we have Kbuild with dependencies and stuff...>>>> Well, I guess the point I'd try and make is that it's not how SPL is>> done today. Really following the existing format would be (in the>> Makefile):>> ifdef CONFIG_SPL_BUILD>> ifdef CONFIG_SPL_OMAP3_ID_NAND>> COBJS-y += spl_id_nand.o>> endif>> endif> > This is bad!> We don't want the code to look like the above crap, do we?> Because next thing will be even worth:> ifdef CONFIG_SPL_BUILD> ifdef CONFIG_SPL_OMAP3_ID_NAND> ifdef CONFIG_SPL_OMAP3_ID_NAND_SHIT...> COBJS-y += spl_id_nand_shit...o> endif> endif> endif> >>>> I can see the point you're making but I'm asking if we need to change>> everyone around to your suggested way of building before we can merge>> these changes in? Thanks!> > Ok. I understand your point. No, I don't think we should.> The real question is, do we want it look like the above crap?> If not, then please, do it right in this patch and all the rest> can be changed later.> Also would be nice to make all future patches do the right thing.
OK, will do. Thanks!

On 11/22/2011 07:51 AM, Tom Rini wrote:
> On 11/22/2011 07:33 AM, Igor Grinberg wrote:>> On 11/21/11 17:33, Tom Rini wrote:>>> On 11/21/2011 07:41 AM, Igor Grinberg wrote:>>>> On 11/21/11 16:12, Tom Rini wrote:>>>>> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>> On 11/20/11 16:26, Tom Rini wrote:>>>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>>>> Hi Tom,>>>>>>>>>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>>>>>> memory. Other boards may simply use this as an easy way to identify>>>>>>>>> board revs. So we provide a function that can be called early to reset>>>>>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>>>>>> --->>>>>>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>> index 8e85891..4b38e45 100644>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>> COBJS += clock.o>>>>>>>>> COBJS += mem.o>>>>>>>>> COBJS += sys_info.o>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>>>>>> +endif>>>>>>>>>>>>>>>> You haven't responded to my question on the above stuff.>>>>>>>> Otherwise all the series look good to me.>>>>>>>>>>>>>> Missed that, sorry!>>>>>>>>>>>>>>>>>>>>>>> Original version available at:>>>>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>>>>>>>>>> Here is the relevant part:>>>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>>>>>> it in #ifdef?>>>>>>>>>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>>>>>>>>>> No, it should not.>>>>>>>> What do you think of the following:>>>>>>>> In the Makefile have only:>>>>>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>>>>>> #ifndef CONFIG_SPL_BUILD>>>>>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>>>>>> #endif>>>>>>>>>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>>>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>>>>>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>>>>>>>>>> No you do not always link this... please, read more carefully...>>>>>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>>>>>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>>>>>> CONFIG_SPL_BUILD being defined, then it will emit an error.>>>>>>>>>> So make the config file do:>>>>> #ifdef CONFIG_SPL_BUILD>>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>>> #endif>>>>> ? That's now how the rest of the SPL code works.>>>>>>>> Well, yes I think it makes sense for all SPL related config options>>>> to do something like:>>>> #ifdef CONFIG_SPL_BUILD>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>> #define CONFIG_SPL_...>>>> #define CONFIG_SPL_...>>>> #endif>>>>>>>> And the error message, I have proposed above, will prevent>>>> people from doing stupid things, like defining>>>> CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.>>>> At least for now, until we have Kbuild with dependencies and stuff...>>>>>> Well, I guess the point I'd try and make is that it's not how SPL is>>> done today. Really following the existing format would be (in the>>> Makefile):>>> ifdef CONFIG_SPL_BUILD>>> ifdef CONFIG_SPL_OMAP3_ID_NAND>>> COBJS-y += spl_id_nand.o>>> endif>>> endif>>>> This is bad!>> We don't want the code to look like the above crap, do we?>> Because next thing will be even worth:>> ifdef CONFIG_SPL_BUILD>> ifdef CONFIG_SPL_OMAP3_ID_NAND >> ifdef CONFIG_SPL_OMAP3_ID_NAND_SHIT...>> COBJS-y += spl_id_nand_shit...o>> endif>> endif>> endif>>>>>>>> I can see the point you're making but I'm asking if we need to change>>> everyone around to your suggested way of building before we can merge>>> these changes in? Thanks!>>>> Ok. I understand your point. No, I don't think we should.>> The real question is, do we want it look like the above crap?>> If not, then please, do it right in this patch and all the rest>> can be changed later.>> Also would be nice to make all future patches do the right thing.> > OK, will do. Thanks!
Well, there's a problem. spl/Makefile both sets CONFIG_SPL_BUILD and
then says "here's a bunch of core stuff" we need. So... we can't hide
most CONFIG choices under a CONFIG_SPL_BUILD check. We can in the
Makefiles however do more:
ifdef CONFIG_SPL_BUILD
COBJS-$(CONFIG_SPL_...) += spl_foo.o
endif
than we do today.

On 11/22/11 17:39, Tom Rini wrote:
> On 11/22/2011 07:51 AM, Tom Rini wrote:>> On 11/22/2011 07:33 AM, Igor Grinberg wrote:>>> On 11/21/11 17:33, Tom Rini wrote:>>>> On 11/21/2011 07:41 AM, Igor Grinberg wrote:>>>>> On 11/21/11 16:12, Tom Rini wrote:>>>>>> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>>> On 11/20/11 16:26, Tom Rini wrote:>>>>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>>>>> Hi Tom,>>>>>>>>>>>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>>>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>>>>>>> memory. Other boards may simply use this as an easy way to identify>>>>>>>>>> board revs. So we provide a function that can be called early to reset>>>>>>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>>>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>>>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>>>>>>> --->>>>>>>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>>>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>>>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>>>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>>>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>> index 8e85891..4b38e45 100644>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>> COBJS += clock.o>>>>>>>>>> COBJS += mem.o>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>> You haven't responded to my question on the above stuff.>>>>>>>>> Otherwise all the series look good to me.>>>>>>>>>>>>>>>> Missed that, sorry!>>>>>>>>>>>>>>>>>>>>>>>>>> Original version available at:>>>>>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>>>>>>>>>>>> Here is the relevant part:>>>>>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>>>>>>> it in #ifdef?>>>>>>>>>>>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>>>>>>>>>>>> No, it should not.>>>>>>>>> What do you think of the following:>>>>>>>>> In the Makefile have only:>>>>>>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>>>>>>> #ifndef CONFIG_SPL_BUILD>>>>>>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>>>>>>> #endif>>>>>>>>>>>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>>>>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>>>>>>>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>>>>>>>>>>>> No you do not always link this... please, read more carefully...>>>>>>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>>>>>>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>>>>>>> CONFIG_SPL_BUILD being defined, then it will emit an error.>>>>>>>>>>>> So make the config file do:>>>>>> #ifdef CONFIG_SPL_BUILD>>>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>>>> #endif>>>>>> ? That's now how the rest of the SPL code works.>>>>>>>>>> Well, yes I think it makes sense for all SPL related config options>>>>> to do something like:>>>>> #ifdef CONFIG_SPL_BUILD>>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>>> #define CONFIG_SPL_...>>>>> #define CONFIG_SPL_...>>>>> #endif>>>>>>>>>> And the error message, I have proposed above, will prevent>>>>> people from doing stupid things, like defining>>>>> CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.>>>>> At least for now, until we have Kbuild with dependencies and stuff...>>>>>>>> Well, I guess the point I'd try and make is that it's not how SPL is>>>> done today. Really following the existing format would be (in the>>>> Makefile):>>>> ifdef CONFIG_SPL_BUILD>>>> ifdef CONFIG_SPL_OMAP3_ID_NAND>>>> COBJS-y += spl_id_nand.o>>>> endif>>>> endif>>>>>> This is bad!>>> We don't want the code to look like the above crap, do we?>>> Because next thing will be even worth:>>> ifdef CONFIG_SPL_BUILD>>> ifdef CONFIG_SPL_OMAP3_ID_NAND >>> ifdef CONFIG_SPL_OMAP3_ID_NAND_SHIT...>>> COBJS-y += spl_id_nand_shit...o>>> endif>>> endif>>> endif>>>>>>>>>>> I can see the point you're making but I'm asking if we need to change>>>> everyone around to your suggested way of building before we can merge>>>> these changes in? Thanks!>>>>>> Ok. I understand your point. No, I don't think we should.>>> The real question is, do we want it look like the above crap?>>> If not, then please, do it right in this patch and all the rest>>> can be changed later.>>> Also would be nice to make all future patches do the right thing.>>>> OK, will do. Thanks!> > Well, there's a problem. spl/Makefile both sets CONFIG_SPL_BUILD and> then says "here's a bunch of core stuff" we need. So... we can't hide> most CONFIG choices under a CONFIG_SPL_BUILD check.
Why? What's the problem?
Is a board config file gets included before the CONFIG_SPL_BUILD
gets exported? And then the "sub" symbol does not get defined?
Is that what's going on? or am I missing something?
> We can in the> Makefiles however do more:> ifdef CONFIG_SPL_BUILD> COBJS-$(CONFIG_SPL_...) += spl_foo.o> endif> than we do today.
And it will turn into a crap... and spread over all the U-Boot code...
This is a problem!
What I propose here is to use the same model as
Linux uses - one independent config option per feature,
which can be selected by a board config file.
Is it impossible right now?

On 11/23/2011 12:39 AM, Igor Grinberg wrote:
> On 11/22/11 17:39, Tom Rini wrote:>> On 11/22/2011 07:51 AM, Tom Rini wrote:>>> On 11/22/2011 07:33 AM, Igor Grinberg wrote:>>>> On 11/21/11 17:33, Tom Rini wrote:>>>>> On 11/21/2011 07:41 AM, Igor Grinberg wrote:>>>>>> On 11/21/11 16:12, Tom Rini wrote:>>>>>>> On Mon, Nov 21, 2011 at 12:04 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>>>> On 11/20/11 16:26, Tom Rini wrote:>>>>>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:>>>>>>>>>> Hi Tom,>>>>>>>>>>>>>>>>>>>> On 11/19/11 00:48, Tom Rini wrote:>>>>>>>>>>> A number of boards are populated with a PoP chip for both DDR and NAND>>>>>>>>>>> memory. Other boards may simply use this as an easy way to identify>>>>>>>>>>> board revs. So we provide a function that can be called early to reset>>>>>>>>>>> the NAND chip and return the result of NAND_CMD_READID. All of this>>>>>>>>>>> code is put into spl_id_nand.c and controlled via CONFIG_SPL_OMAP3_ID_NAND.>>>>>>>>>>>>>>>>>>>>>> Signed-off-by: Tom Rini <trini@ti.com>>>>>>>>>>>> --->>>>>>>>>>> arch/arm/cpu/armv7/omap3/Makefile | 3 +>>>>>>>>>>> arch/arm/cpu/armv7/omap3/spl_id_nand.c | 87 +++++++++++++++++++++++++++>>>>>>>>>>> arch/arm/include/asm/arch-omap3/sys_proto.h | 1 +>>>>>>>>>>> 3 files changed, 91 insertions(+), 0 deletions(-)>>>>>>>>>>> create mode 100644 arch/arm/cpu/armv7/omap3/spl_id_nand.c>>>>>>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>> index 8e85891..4b38e45 100644>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_ID_NAND) += spl_id_nand.o>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>>>> You haven't responded to my question on the above stuff.>>>>>>>>>> Otherwise all the series look good to me.>>>>>>>>>>>>>>>>>> Missed that, sorry!>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Original version available at:>>>>>>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg68828.html>>>>>>>>>>>>>>>>>>>> Here is the relevant part:>>>>>>>>>>>>>>>>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>>>> index 8e85891..772f3d4 100644>>>>>>>>>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile>>>>>>>>>>>>>>>> @@ -31,6 +31,9 @@ COBJS += board.o>>>>>>>>>>>>>>>> COBJS += clock.o>>>>>>>>>>>>>>>> COBJS += mem.o>>>>>>>>>>>>>>>> COBJS += sys_info.o>>>>>>>>>>>>>>>> +ifdef CONFIG_SPL_BUILD>>>>>>>>>>>>>>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>>>> +endif>>>>>>>>>>>>>>>>>>>>>>>>>>>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no">>>>>>>>>>>>>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose>>>>>>>>>>>>>> it in #ifdef?>>>>>>>>>>>>>>>>>>>>>>>> But then it would build for both SPL and non-SPL cases.>>>>>>>>>>>>>>>>>>>> No, it should not.>>>>>>>>>> What do you think of the following:>>>>>>>>>> In the Makefile have only:>>>>>>>>>> COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE) += spl_pop_probe.o>>>>>>>>>>>>>>>>>>>> Then in the spl_pop_probe.c have this type of check:>>>>>>>>>> #ifndef CONFIG_SPL_BUILD>>>>>>>>>> # error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD>>>>>>>>>> #endif>>>>>>>>>>>>>>>>>>>> This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol>>>>>>>>>> be a part of the CONFIG_SPL_BUILD symbols group.>>>>>>>>>>>>>>>>>> Well, if we always link this, but then #error, U-Boot won't build :)>>>>>>>>>>>>>>>> No you do not always link this... please, read more carefully...>>>>>>>> Only when CONFIG_SPL_OMAP3_POP_PROBE symbol is defined, the file will>>>>>>>> be compiled, but if CONFIG_SPL_OMAP3_POP_PROBE defined without>>>>>>>> CONFIG_SPL_BUILD being defined, then it will emit an error.>>>>>>>>>>>>>> So make the config file do:>>>>>>> #ifdef CONFIG_SPL_BUILD>>>>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>>>>> #endif>>>>>>> ? That's now how the rest of the SPL code works.>>>>>>>>>>>> Well, yes I think it makes sense for all SPL related config options>>>>>> to do something like:>>>>>> #ifdef CONFIG_SPL_BUILD>>>>>> #define CONFIG_SPL_OMAP3_POP_PROBE>>>>>> #define CONFIG_SPL_...>>>>>> #define CONFIG_SPL_...>>>>>> #endif>>>>>>>>>>>> And the error message, I have proposed above, will prevent>>>>>> people from doing stupid things, like defining>>>>>> CONFIG_SPL_OMAP3_POP_PROBE without the CONFIG_SPL_BUILD.>>>>>> At least for now, until we have Kbuild with dependencies and stuff...>>>>>>>>>> Well, I guess the point I'd try and make is that it's not how SPL is>>>>> done today. Really following the existing format would be (in the>>>>> Makefile):>>>>> ifdef CONFIG_SPL_BUILD>>>>> ifdef CONFIG_SPL_OMAP3_ID_NAND>>>>> COBJS-y += spl_id_nand.o>>>>> endif>>>>> endif>>>>>>>> This is bad!>>>> We don't want the code to look like the above crap, do we?>>>> Because next thing will be even worth:>>>> ifdef CONFIG_SPL_BUILD>>>> ifdef CONFIG_SPL_OMAP3_ID_NAND >>>> ifdef CONFIG_SPL_OMAP3_ID_NAND_SHIT...>>>> COBJS-y += spl_id_nand_shit...o>>>> endif>>>> endif>>>> endif>>>>>>>>>>>>>> I can see the point you're making but I'm asking if we need to change>>>>> everyone around to your suggested way of building before we can merge>>>>> these changes in? Thanks!>>>>>>>> Ok. I understand your point. No, I don't think we should.>>>> The real question is, do we want it look like the above crap?>>>> If not, then please, do it right in this patch and all the rest>>>> can be changed later.>>>> Also would be nice to make all future patches do the right thing.>>>>>> OK, will do. Thanks!>>>> Well, there's a problem. spl/Makefile both sets CONFIG_SPL_BUILD and>> then says "here's a bunch of core stuff" we need. So... we can't hide>> most CONFIG choices under a CONFIG_SPL_BUILD check.> > Why? What's the problem?> Is a board config file gets included before the CONFIG_SPL_BUILD> gets exported? And then the "sub" symbol does not get defined?
Correct. Give the change you're proposing a try on devkit8000, you'll
see what I mean.
> Is that what's going on? or am I missing something?> >> We can in the>> Makefiles however do more:>> ifdef CONFIG_SPL_BUILD>> COBJS-$(CONFIG_SPL_...) += spl_foo.o>> endif>> than we do today.> > And it will turn into a crap... and spread over all the U-Boot code...> This is a problem!> > What I propose here is to use the same model as> Linux uses - one independent config option per feature,> which can be selected by a board config file.> Is it impossible right now?
Yes, needs a good bit of thinking.