[U-Boot] [PATCH] cmd: fdt: Use separate CMD_FDT Kconfig entry instead of OF_LIBFDT
Simon Glass
sjg at chromium.org
Wed Apr 20 16:40:08 CEST 2016
On 11 April 2016 at 08:59, Michal Simek <michal.simek at xilinx.com> wrote:
> On 11.4.2016 16:50, Simon Glass wrote:
>> Hi Michal,
>>
>> On 11 April 2016 at 08:48, Michal Simek <michal.simek at xilinx.com> wrote:
>>> On 11.4.2016 16:43, Simon Glass wrote:
>>>> Hi Michal,
>>>>
>>>> On 11 April 2016 at 08:41, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>> On 9.4.2016 20:36, Simon Glass wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> On 6 April 2016 at 12:28, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>>>> On 6.4.2016 03:28, Masahiro Yamada wrote:
>>>>>>>> Hi.
>>>>>>>>
>>>>>>>>
>>>>>>>> 2016-04-06 4:09 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>>>>>>>> Hi Michal,
>>>>>>>>>
>>>>>>>>> On 5 April 2016 at 04:15, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>>>>>>> Hi Simon,
>>>>>>>>>>
>>>>>>>>>> On 5.4.2016 02:03, Simon Glass wrote:
>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>
>>>>>>>>>>> On 4 April 2016 at 11:50, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>>>>>>>>> Create CMD_FDT Kconfig entry to have an option to disable fdt command
>>>>>>>>>>>> which is not required for small configuration which requires libfdt
>>>>>>>>>>>> only.
>>>>>>>>>>>> Enable it by default for all targets which enables OF_LIBFDT.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> cmd/Kconfig | 7 +++++++
>>>>>>>>>>>> cmd/Makefile | 2 +-
>>>>>>>>>>>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>>>>>>>>>>> index fe8b4f0510da..8703cdb4a9be 100644
>>>>>>>>>>>> --- a/cmd/Kconfig
>>>>>>>>>>>> +++ b/cmd/Kconfig
>>>>>>>>>>>> @@ -173,6 +173,13 @@ config CMD_ELF
>>>>>>>>>>>> help
>>>>>>>>>>>> Boot an ELF/vxWorks image from the memory.
>>>>>>>>>>>>
>>>>>>>>>>>> +config CMD_FDT
>>>>>>>>>>>> + bool "Flattened Device Tree utility commands"
>>>>>>>>>>>> + default y
>>>>>>>>>>>
>>>>>>>>>>> Should that be:
>>>>>>>>>>>
>>>>>>>>>>> default y if OF_LIBFDT
>>>>>>>>>>>
>>>>>>>>>>> ?
>>>>>>>>>>>
>>>>>>>>>>>> + depends on OF_LIBFDT
>>>>>>>>>>>> + help
>>>>>>>>>>>> + Do FDT related setup before booting into the Operating System.
>>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> In recent commits to this file both formats are used.
>>>>>>>>>>
>>>>>>>>>> +config CMD_BLOCK_CACHE
>>>>>>>>>> + bool "blkcache - control and stats for block cache"
>>>>>>>>>> + depends on BLOCK_CACHE
>>>>>>>>>> + default y if BLOCK_CACHE
>>>>>>>>>>
>>>>>>>>>> even looks non standard.
>>>>>>>>>>
>>>>>>>>>> +config CMD_BOOTEFI
>>>>>>>>>> + bool "bootefi"
>>>>>>>>>> + depends on EFI_LOADER
>>>>>>>>>> + default y
>>>>>>>>>>
>>>>>>>>>> I am happy to change whatever style you prefer but I think it should be
>>>>>>>>>> synchronized. The efi one was Reviewed by you. :-)
>>>>>>>>>
>>>>>>>>> I think Masahiro knows most about this. If it works it's fine with me.
>>>>>>>>> The way you have it is more intuitive and I prefer it. But he did
>>>>>>>>> point at a problem at some point.
>>>>>>>>
>>>>>>>>
>>>>>>>> I think "depends on OF_LIBFDT"
>>>>>>>> is correct in this case.
>>>>>>>>
>>>>>>>>
>>>>>>>> do_fdt() calls fdt_fixup_memory(), which is defined in common/fdt_support.c,
>>>>>>>> which is enabled by CONFIG_OF_LIBFDT.
>>>>>>>>
>>>>>>>> So, CMD_FDT should depend on OF_LIBFDT.
>>>>>>>> Otherwise, "make menuconfig" would allow users
>>>>>>>> to enable CMD_FDT without OF_LIBFDT,
>>>>>>>> which would cause link error.
>>>>>>>>
>>>>>>>>
>>>>>>>>> One other question - won't this disable the 'fdt' command for many boards?
>>>>>>>>
>>>>>>>>
>>>>>>>> config CMD_FDT
>>>>>>>> bool "Flattened Device Tree utility commands"
>>>>>>>> default y
>>>>>>>> depends on OF_LIBFDT
>>>>>>>>
>>>>>>>>
>>>>>>>> "default y" cares about it.
>>>>>>>> So, if CONFIG_OF_LIBFDT is enabled in the defconfig,
>>>>>>>> CONFIG_CMD_FDT should be enabled as well.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> But the following 6 boards opt out of Kconfig.
>>>>>>>> They define CONFIG_OF_LIBFDT in their config headers,
>>>>>>>> so this patch would disable "fdt" command for them.
>>>>>>>>
>>>>>>>> include/configs/legoev3.h
>>>>>>>> include/configs/ma5d4evk.h
>>>>>>>> include/configs/pic32mzdask.h
>>>>>>>> include/configs/stm32f746-disco.h
>>>>>>>> include/configs/xilinx-ppc.h
>>>>>>>> include/configs/zipitz2.h
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Could you move them to defconfigs?
>>>>>>>>
>>>>>>>
>>>>>>> I have sent v2 to address these. I have used buildman and there is up to
>>>>>>> 2k difference when symbol is in Kconfig. Not sure why but it shouldn't
>>>>>>> be big deal.
>>>>>>
>>>>>> What does 2k difference mean?
>>>>>
>>>>> hm. I wanted to use buildman to show me that but that looks weird.
>>>>> It looks like that it is not running make legoev3_defconfig after new
>>>>> patch because LIBFDT is not enabled when 3 commits are built.
>>>>> But when 2 commits are built then it is enabled properly.
>>>>> Isn't there any bug in buildman?
>>>>>
>>>>> Thanks,
>>>>> Michal
>>>>>
>>>>> [u-boot]$ ./tools/buildman/buildman -b xnext/kconfig2 legoev3 -o /tmp/
>>>>> -sSedKv
>>>>> boards.cfg is up to date. Nothing to do.
>>>>> Summary of 3 commits for 1 boards (1 thread, 8 jobs per thread)
>>>>> 01: doc: clarify openssl-based key and certificate generation process
>>>>> 02: kconfig: Move CONFIG_OF_LIBFDT to Kconfig
>>>>> arm: (for 1/1 boards) all -23674.0 bss -1032.0 data -1920.0
>>>>> rodata -3622.0 text -17100.0
>>>>> legoev3 : all -23674 bss -1032 data -1920 rodata
>>>>> -3622 text -17100
>>>>> arm:
>>>>> - autoconf.mk: CONFIG_OF_LIBFDT=y
>>>>> - all: CONFIG_OF_LIBFDT=y
>>>>> legoev3 :
>>>>> - autoconf.mk: CONFIG_OF_LIBFDT=y
>>>>> - all: CONFIG_OF_LIBFDT=y
>>>>> 03: cmd: fdt: Use separate CMD_FDT Kconfig entry instead of OF_LIBFDT
>>>>> (no errors to report)
>>>>> [u-boot]$ git log --oneline | head -n 3
>>>>> 0440a5580b8a cmd: fdt: Use separate CMD_FDT Kconfig entry instead of
>>>>> OF_LIBFDT
>>>>> ef3a52ec50b0 kconfig: Move CONFIG_OF_LIBFDT to Kconfig
>>>>> 4c1d5c29b5de doc: clarify openssl-based key and certificate generation
>>>>> process
>>>>> [u-boot]$
>>>>> [u-boot]$ ./tools/buildman/buildman -b xnext/kconfig2 legoev3 -o /tmp/ -c 2
>>>>> boards.cfg is up to date. Nothing to do.
>>>>> Building 2 commits for 1 boards (1 thread, 8 jobs per thread)
>>>>> 2 0 0 /2 legoev3
>>>>> [u-boot]$ ./tools/buildman/buildman -b xnext/kconfig2 legoev3 -o /tmp/
>>>>> -c 2 -sSedKv
>>>>> boards.cfg is up to date. Nothing to do.
>>>>> Summary of 2 commits for 1 boards (1 thread, 8 jobs per thread)
>>>>> 01: kconfig: Move CONFIG_OF_LIBFDT to Kconfig
>>>>> 02: cmd: fdt: Use separate CMD_FDT Kconfig entry instead of OF_LIBFDT
>>>>> arm:
>>>>> + .config: CONFIG_CMD_FDT=y
>>>>> + autoconf.h: CONFIG_CMD_FDT=1
>>>>> + u-boot.cfg: CONFIG_CMD_FDT=1
>>>>> + all: CONFIG_CMD_FDT=1
>>>>> legoev3 :
>>>>> + .config: CONFIG_CMD_FDT=y
>>>>> + autoconf.h: CONFIG_CMD_FDT=1
>>>>> + u-boot.cfg: CONFIG_CMD_FDT=1
>>>>> + all: CONFIG_CMD_FDT=1
>>>>> (no errors to report)
>>>>> [u-boot]$
>>>>>
>>>>>
>>>>
>>>> You may need the -C flag to make it reconfigure on each commit, and
>>>> now you might need -f also since you already have a build.
>>>
>>> Ah. Ok.
>>>
>>> This is causing that 2k diff.
>>> #define IMAGE_ENABLE_OF_LIBFDT CONFIG_IS_ENABLED(OF_LIBFDT)
>>>
>>> I will run it over all boards which are affected.
>>>
>>> Thanks,
>>> Michal
>>>
>>> arm: (for 1/1 boards) all +2192.0 bss +28.0 rodata +64.0 text
>>> +2100.0
>>> legoev3 : all +2192 bss +28 rodata +64 text +2100
>>> u-boot: add: 7/-1, grow: 4/0 bytes: 2240/-356 (1884)
>>> function old new
>>> delta
>>> boot_get_fdt - 780
>>> +780
>>> boot_prep_linux - 400
>>> +400
>>> image_setup_libfdt - 316
>>> +316
>>> lmb_free - 268
>>> +268
>>> fdt_root - 140
>>> +140
>>> image_setup_linux - 136
>>> +136
>>> bootm_find_images 72 168
>>> +96
>>> do_bootm_states 1952 2004
>>> +52
>>> genimg_get_format 52 76
>>> +24
>>> boot_jump_linux 172 192
>>> +20
>>> genimg_has_config - 8
>>> +8
>>> static.boot_prep_linux 356 -
>>> -356
>>
>> That seems bad. I've been in this code quite a bit - it is tricky to
>> keep the options the same - e.g. my recent conversion of CONFIG_FIT to
>> Kconfig was a bit of a mission. Worth digging into...
>
> I know that you have converted others.
> I have checked and others boards are just the same (except xilinx_ppc
> which is not affected) +2k because of that macro. But is this a problem?
> I mean my goal is to separate CMD_FDT command because it is just too
> huge if you have just small amount of memory.
> I don't know these boards but only Xilinx PPC guy has responded that it
> is fine and none else. It means that they are fine with this change and
> they can fix it if +2k is problem for them.
>
> I fully understand that this is the problem from conversion point of
> view if simple move is causing it but at the end of the day I don't
> think it is worth to invest time to look these 5.
>
> What do you think?
Looks OK to me.
Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Thanks,
> Michal
>
>
> [u-boot]$ ./tools/buildman/buildman -b xnext/kconfig2 legoev3 xilinx-ppc
> ma5d4evk zipitz2 stm32f746-disco pic32mzdask -o /tmp/ -CsSedv
> boards.cfg is up to date. Nothing to do.
> Summary of 3 commits for 7 boards (7 threads, 2 jobs per thread)
> 01: Merge branch 'master' of git://git.denx.de/u-boot-fsl-qoriq
> 02: kconfig: Move CONFIG_OF_LIBFDT to Kconfig
> arm: (for 4/4 boards) all +2033.0 bss +32.0 rodata +66.0 text
> +1935.0
> ma5d4evk : all +2328 bss +12 rodata +68 text +2248
> zipitz2 : all +2256 bss +88 rodata +68 text +2100
> legoev3 : all +2192 bss +28 rodata +64 text +2100
> stm32f746-disco: all +1356 rodata +64 text +1292
> 03: cmd: fdt: Use separate CMD_FDT Kconfig entry instead of OF_LIBFDT
>
>
>
More information about the U-Boot
mailing list