[U-Boot] ad-hoc config error

[york sun]

On 09/21/2016 12:01 PM, Tom Rini wrote:
> On Wed, Sep 21, 2016 at 04:07:49PM +0000, york sun wrote:
>> 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.
>
> So that would be the arch/arm/include/asm/arch-omap/mem.h case then.
> That actually assembles the magic numbers by saying that for a given
> memory chip from $VENDOR you need to set each of the fields thusly, and
> then shifts them in to the places the various memory controller
> registers want to them.
>
>> 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
>
> This would be the arch/arm/cpu/armv7/am33xx/ddr.c case.  A large number
> of different boards and DDR chips work, I just pointed out one of them.
> Or in NXP terms, arch/arm/cpu/armv7/mx6/ddr.c.
>
>> Do we simply
>> remove the CONFIG_ from the macro names, or put them in a well-defined
>> namespace?
>
> Very roughly, you should model the abstraction a bit differently.
> board/freescale/ls1012afrdm/ls1012afrdm.c should call mmdc_init and pass
> it a struct that contains the various DDR timing values and instead of:
> /* DDR board-specific timing parameters */
> It should say:
> /*
>  * We have a <VENDOR> <CHIP> for DDR.  The timing values are also however
>  * board-specific, please see <TRM or tech note or whatever that NXP
>  * advises customers to read>
>  */
> and then define them and put them into the struct.  Or just put them
> into the struct.  I think that having drivers/ddr/fsl/fsl_mmdc.c be
> compiled with the values is a step in the wrong direction as it makes
> supporting multiple platforms with a single binary harder.
>

I like the struct idea better.

York