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

[Igor Grinberg <grinberg@compulab.co.il>]

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

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.


-- 
Regards,
Igor.