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! -- Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot