From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 22 Nov 2011 07:51:28 -0700 Subject: [U-Boot] [PATCH v4 10/14] OMAP3 SPL: Add identify_nand_chip function In-Reply-To: <4ECBB2A5.10602@compulab.co.il> References: <1321656491-19874-1-git-send-email-trini@ti.com> <1321656491-19874-11-git-send-email-trini@ti.com> <4EC8ADF4.8050702@compulab.co.il> <4EC9F7ED.50904@compulab.co.il> <4ECA6306.10501@compulab.co.il> <4ECA6F4F.1090003@ti.com> <4ECBB2A5.10602@compulab.co.il> Message-ID: <4ECBB6F0.70102@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 wrote: >>>>> On 11/20/11 16:26, Tom Rini wrote: >>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg 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 >>>>>>>> --- >>>>>>>> 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 at 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