| Message ID | 1424332048-9509-3-git-send-email-yamada.m@jp.panasonic.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Masahiro, On 19 February 2015 at 00:47, Masahiro Yamada <yamada.m@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. > > The default value 0x400 is redundant for OMAP, Exynos, UniPhier, etc. > They can be simply removed. > > There are still redundant "CONFIG_SYS_MALLOC_F_LEN=0x400" in many > defconfig files, but this commit is not touching them. > > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> > --- > [snip] Regards, Simon
Hi, On 19 February 2015 at 07:22, Simon Glass <sjg@chromium.org> wrote: > Hi Masahiro, > > On 19 February 2015 at 00:47, Masahiro Yamada <yamada.m@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
2015-02-19 23:22 GMT+09:00 Simon Glass <sjg@chromium.org>: > Hi Masahiro, > > On 19 February 2015 at 00:47, Masahiro Yamada <yamada.m@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. OK. But this change alone could still generate a strange .config file. >> >> [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? I left a comment in your patch: http://patchwork.ozlabs.org/patch/441669/ >> >> 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. I think this is a good idea. >> >> 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. I would not go as far as say it must be banned. Please be careful when you add default values in Kconfig.
Simon, 2015-02-20 8:19 GMT+09:00 Simon Glass <sjg@chromium.org>: > > 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/ > 2/2 looks good. As I commented in your patch, 1/2 still leaves us a problem. How shall we organize this?
Hi Masahiro, On 19 February 2015 at 16:55, Masahiro YAMADA <yamada.m@jp.panasonic.com> wrote: > Simon, > > > 2015-02-20 8:19 GMT+09:00 Simon Glass <sjg@chromium.org>: > >> >> 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/ >> > > > 2/2 looks good. > > As I commented in your patch, 1/2 still leaves us a problem. > > > How shall we organize this? See my comment on the other patch - maybe we can live with that problem. Regards, Simon
diff --git a/Kconfig b/Kconfig index 75bab7f..d40f9ec 100644 --- a/Kconfig +++ b/Kconfig @@ -58,7 +58,6 @@ config CC_OPTIMIZE_FOR_SIZE config SYS_MALLOC_F bool "Enable malloc() pool before relocation" - default 0x400 help Before relocation memory is very limited on many platforms. Still, we can provide a small malloc() pool if needed. Driver model in diff --git a/arch/arm/cpu/armv7/exynos/Kconfig b/arch/arm/cpu/armv7/exynos/Kconfig index 2064efa..23869ce 100644 --- a/arch/arm/cpu/armv7/exynos/Kconfig +++ b/arch/arm/cpu/armv7/exynos/Kconfig @@ -83,9 +83,6 @@ config DM_GPIO config SYS_MALLOC_F default y if !SPL_BUILD -config SYS_MALLOC_F_LEN - default 0x400 if !SPL_BUILD - source "board/samsung/smdkv310/Kconfig" source "board/samsung/trats/Kconfig" source "board/samsung/universal_c210/Kconfig" diff --git a/arch/arm/cpu/armv7/omap3/Kconfig b/arch/arm/cpu/armv7/omap3/Kconfig index 4644098..2e193ab 100644 --- a/arch/arm/cpu/armv7/omap3/Kconfig +++ b/arch/arm/cpu/armv7/omap3/Kconfig @@ -105,9 +105,6 @@ config DM_SERIAL config SYS_MALLOC_F default y if DM && !SPL_BUILD -config SYS_MALLOC_F_LEN - default 0x400 if DM && !SPL_BUILD - config SYS_SOC default "omap3" diff --git a/arch/arm/cpu/armv7/tegra-common/Kconfig b/arch/arm/cpu/armv7/tegra-common/Kconfig index ee32469..0de13ae 100644 --- a/arch/arm/cpu/armv7/tegra-common/Kconfig +++ b/arch/arm/cpu/armv7/tegra-common/Kconfig @@ -20,8 +20,10 @@ endchoice config SYS_MALLOC_F default y +# This is meaningless. +# "default 0x400" in the top level Kconfig is used in stead. config SYS_MALLOC_F_LEN - default 0x1800 + default 0x1800 if SYS_MALLOC_F config USE_PRIVATE_LIBGCC default y if SPL_BUILD diff --git a/arch/arm/cpu/armv7/uniphier/Kconfig b/arch/arm/cpu/armv7/uniphier/Kconfig index 8fdef28..371b274 100644 --- a/arch/arm/cpu/armv7/uniphier/Kconfig +++ b/arch/arm/cpu/armv7/uniphier/Kconfig @@ -51,9 +51,6 @@ endchoice config SYS_MALLOC_F default y -config SYS_MALLOC_F_LEN - default 0x400 - config CMD_PINMON bool "Enable boot mode pins monitor command" default y diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 35d24e4..2275374 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -79,8 +79,10 @@ config DM_SERIAL config SYS_MALLOC_F default y +# This is meaningless. +# "default 0x400" in the top level Kconfig is used in stead. config SYS_MALLOC_F_LEN - default 0x800 + default 0x800 if SYS_MALLOC_F config RAMBASE hex diff --git a/board/amcc/canyonlands/Kconfig b/board/amcc/canyonlands/Kconfig index 848e08f..c0dbd18 100644 --- a/board/amcc/canyonlands/Kconfig +++ b/board/amcc/canyonlands/Kconfig @@ -43,8 +43,4 @@ config SYS_MALLOC_F bool default y -config SYS_MALLOC_F_LEN - hex - default 0x400 - endif diff --git a/board/ti/am335x/Kconfig b/board/ti/am335x/Kconfig index a20e0c1..cad10c2 100644 --- a/board/ti/am335x/Kconfig +++ b/board/ti/am335x/Kconfig @@ -50,7 +50,4 @@ config DM_SERIAL config SYS_MALLOC_F default y if DM && !SPL_BUILD -config SYS_MALLOC_F_LEN - default 0x400 if DM && !SPL_BUILD - endif
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. [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. 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. 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. The default value 0x400 is redundant for OMAP, Exynos, UniPhier, etc. They can be simply removed. There are still redundant "CONFIG_SYS_MALLOC_F_LEN=0x400" in many defconfig files, but this commit is not touching them. Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> --- Kconfig | 1 - arch/arm/cpu/armv7/exynos/Kconfig | 3 --- arch/arm/cpu/armv7/omap3/Kconfig | 3 --- arch/arm/cpu/armv7/tegra-common/Kconfig | 4 +++- arch/arm/cpu/armv7/uniphier/Kconfig | 3 --- arch/x86/Kconfig | 4 +++- board/amcc/canyonlands/Kconfig | 4 ---- board/ti/am335x/Kconfig | 3 --- 8 files changed, 6 insertions(+), 19 deletions(-)