[U-Boot] [PATCH v4 10/14] OMAP3 SPL: Add identify_nand_chip function

[Tom Rini]

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 <grinberg at compulab.co.il> wrote:
>>>>> On 11/20/11 16:26, Tom Rini wrote:
>>>>>> On Sun, Nov 20, 2011 at 12:36 AM, Igor Grinberg <grinberg at 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 <trini at 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