[U-Boot] ad-hoc config error

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160921/d5d61df0/attachment.sig>