Hi, On 19 February 2015 at 07:22, Simon Glass <s...@chromium.org> wrote: > Hi Masahiro, > > On 19 February 2015 at 00:47, Masahiro Yamada <yamad...@jp.panasonic.com> > wrote: >> Since commit b724bd7d6349 (dm: Kconfig: Move CONFIG_SYS_MALLOC_F_LEN >> to Kconfig), the ".config" created by the configuration has been >> wrong. >> >> For example, the following is a snippet of the ".config" generated >> by "make beaver_defconfig": >> >> --------------->8----------------- >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y >> # CONFIG_SYS_MALLOC_F is not set >> CONFIG_SYS_MALLOC_F_LEN=0x1800 >> # CONFIG_EXPERT is not set >> ---------------8<----------------- >> >> CONFIG_SYS_MALLOC_F_LEN is supposed to depend on CONFIG_SYS_MALLOC_F >> (see the top level Kconfig), but the ".config" above is not actually >> following that dependency. >> >> This is caused by two mistakes of commit b724bd7d6349. >> >> [1] Wrong default value of CONFIG_SYS_MALLOC_F >> CONFIG_SYS_MALLOC_F is a boolean option, but its default value is >> set to 0x400. > > I sent a patch for this. > >> >> [2] Missing "if SYS_MALLOC_F" in the default setting in each Kconfig >> For example, arch/arm/cpu/armv7/tegra-common/Kconfig has the line >> "default 0x1800" for SYS_MALLOC_F_LEN. It must be described as >> "default 0x1800 if SYS_MALLOC_F" to follow the dependency. > > Does this actually matter? What does it break? > >> >> Those two bugs together create such a broken ".config". >> >> Unfortunately, even if we correct both [1] and [2], the value of >> CONFIG_SYS_MALLOC_F_LEN is not set as we expect. >> The "default 0x1800 if SYS_MALLOC_F" would be simply ignored because >> the "default 0x400" in the top level Kconfig is parsed first. >> >> Notice that if multiple default lines appear for the same CONFIG, >> the first one takes precedence. > > I sent a patch for this also as I don't really think the current > ordering is useful. > >> >> So, this commit correct [1] and [2], also leaves some comments >> in arch/arm/cpu/armv7/tegra-common/Kconfig and arch/x86/Kconfig >> to notify not-working default values. >> >> If you want to change the default value of CONFIG_SYS_MALLOC_F_LEN, >> the easiest way would be to specify it in each *_defconfig. >> >> It is true that describing SoC-common default values in each Kconfig >> seems handy, but it often introduces nasty problems. >> If you do not understand well how Kconfig works, as you see above, >> you could easily create a broken .config file. > > It is actually quite painful to not support this. If you add a new > board you need to work out what the settings should be for that board. > As we add more and more settings this is going to get even harder. It > seems much better for Tegra (for example) to be able to specify > sensible defaults that will work on most boards. > > Otherwise we have no place to record that (for example) Tegra124 needs > this feature on, or this value set. Before Kconfig we have > tegra-common.h and tegra124-common.h. We discussed this problem before > and I thought it was agreed that defaults in Kconfig were the best way > forward. > > If we can't use Kconfig then I think we will need to figure out > something else - perhaps includes in the defconfig files as previously > discussed. But that would be a new kconfig feature so I don't think > anyone is thrilled with the idea. >
So to be clear, I'd really like to NAK this patch. Please see my suggested alternatives instead: http://patchwork.ozlabs.org/patch/441669/ http://patchwork.ozlabs.org/patch/441670/ Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
