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