[U-Boot] ad-hoc config error

[york sun]

On 09/21/2016 08:45 AM, Tom Rini wrote:
> On Wed, Sep 21, 2016 at 03:22:59PM +0000, york sun wrote:
>> On 09/20/2016 03:30 PM, Tom Rini wrote:
>>> On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
>>>> On 09/20/2016 02:36 PM, Tom Rini wrote:
>>>>> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
>>>>>
>>>>>> Tom and Simon,
>>>>>>
>>>>>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
>>>>>> new macros defined will have the compiling error. How shall we fix it?
>>>>>> Some macros can be added to Kconfig. But some are for local use, better
>>>>>> than magic numbers. Adding them to the white-list doesn't sound right.
>>>>>> What's your suggestion?
>>>>>
>>>>> Things that don't belong in Kconfig don't belong in the CONFIG namespace
>>>>> either, probably.  For example, the cache line stuff is in Kconfig and
>>>>> select'ed but for another example, various magic numbers used in the TI
>>>>> secure boot stuff (which can vary from SoC to SoC) are just not in the
>>>>> CONFIG namespace now.
>>>>>
>>>>
>>>> For those can be put in Kconfig, I can convert.
>>>> Can you point me examples to use macros for magic numbers?
>>>> What about the white listed macros? Are we supposed to leave them there,
>>>> or slowly convert them to other name space?
>>>
>>> Things on the whitelist should be converted, either to Kconfig or moved
>>> out of the namespace.  Can you give me an example of something you
>>> aren't sure how to convert?
>>>
>> For example,
>>
>> CONFIG_SYS_DDR_MODE_1_1000
>> CONFIG_SYS_DDR_MODE_1_1200
>> CONFIG_SYS_DDR_MODE_1_1333
>> CONFIG_SYS_DDR_MODE_1_667
>> CONFIG_SYS_DDR_MODE_1_800
>> CONFIG_SYS_DDR_MODE_1_900
>>
>> Those are DDR parameters defined per board if used. What will be proper
>> names to convert to?
>
> Poking at this a bit more, looking at say
> board/freescale/corenet_ds/p4080ds_ddr.c (which I found grepping for
> CONFIG_SYS_DDR_MODE_1_1200) reminds me of
> arch/arm/cpu/armv7/am33xx/ddr.c and board/ti/am335x/board.c and no, I'm
> not convinced that:
>         .timing_cfg_0 = CONFIG_SYS_DDR_TIMING_0_800,
> is more clear than:
>         .timing_cfg_0 = 0xcc330104,
>
> Especially since there's not a call back to a TRM/whatever that
> describes the order of the fields for each register.  To me a link like
> that is more descriptive.  I further assume that, after a bit more
> grepping, these values, like the counterparts for am33xx are DDR chip
> specific so I might go so far as to point at
> arch/arm/include/asm/arch-omap3/mem.h where we have a series of
> #define MEMORYVENDOR_MEMCTRL_FIELD 0xMAGIC
> as if you re-use the part found on the ref board on your custom board
> you can use (or at least start with) those values.
>
> And yes, naming of memory controller related magic values is a mess and
> is inconsistent even over the TI parts.  My first reaction is that the
> am33xx stuff, which came later, worked out "better" than the omap3 way
> did.
>

Tom,

If the macros are only used locally, we can replace them with the actual 
magic numbers. But even that is different from what we have been 
encouraging developers to avoid using magic numbers. I believe using 
macros makes the code more readable.

On the other hand, if we want to share a driver for multiple boards, the 
macros have advantage. See this patch 
http://patchwork.ozlabs.org/patch/663051/. This is the one I am trying 
to merge. The author abstracted the DDR init sequence and use macros so 
multiple boards can use the same driver. Each board only needs to define 
a set of macros. I think this use should be accepted. Do we simply 
remove the CONFIG_ from the macro names, or put them in a well-defined 
namespace?

York