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
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
