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 >>>>>> <grinb...@compulab.co.il> wrote: >>>>>>> On 11/20/11 16:26, Tom Rini wrote: >>>>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg >>>>>>>> <grinb...@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 <tr...@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? -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot