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

[Tom Rini <trini@ti.com>]

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@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@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@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 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

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!

-- 
Tom