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. We can in the Makefiles however do more: ifdef CONFIG_SPL_BUILD COBJS-$(CONFIG_SPL_...) += spl_foo.o endif than we do today. -- Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
