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

[Tom Rini <trini@ti.com>]

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