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