| Message ID | 1462372709-24828-1-git-send-email-casantos@datacom.ind.br |
|---|---|
| State | Not Applicable |
| Headers | show |
Hello, Thanks for this patch. See my comments below. On Wed, 4 May 2016 11:38:29 -0300, Carlos Santos wrote: > The mkimage utility needs dtc when the input is in Flat Image Trees (FIT) > format. If dtc is not available mkimage fails. Example: > > $ mkimage -f firmware.its firmware.im > sh: dtc: command not found > > Signed-off-by: Carlos Santos <casantos@datacom.ind.br> > --- > package/uboot-tools/Config.in.host | 1 + > package/uboot-tools/uboot-tools.mk | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/package/uboot-tools/Config.in.host b/package/uboot-tools/Config.in.host > index b5a42d9..5c44eaf 100644 > --- a/package/uboot-tools/Config.in.host > +++ b/package/uboot-tools/Config.in.host > @@ -1,5 +1,6 @@ > config BR2_PACKAGE_HOST_UBOOT_TOOLS > bool "host u-boot tools" > + select BR2_PACKAGE_HOST_DTC > help > Companion tools for Das U-Boot bootloader. > > diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk > index f47b3db..a07fbfa 100644 > --- a/package/uboot-tools/uboot-tools.mk > +++ b/package/uboot-tools/uboot-tools.mk > @@ -65,6 +65,8 @@ define UBOOT_TOOLS_INSTALL_TARGET_CMDS > $(UBOOT_TOOLS_INSTALL_DUMPIMAGE) > endef > > +HOST_UBOOT_TOOLS_DEPENDENCIES += host-dtc I am not sure I like the idea of having host-uboot-tools always depend on host-dtc, as it adds build time while host-dtc is only needed for some specific use cases of mkimage (generating FIT images). There are really three options I believe: (1) What you did, i.e have host-dtc as an unconditional dependency of host-uboot-tools. Everybody pays the price of building host-dtc even if it's not needed. (2) Add a sub-option to host-uboot-tools so that people can say "I need it with FIT image support", which will add host-dtc as a dependency. (3) Just do nothing, and let our users be smart enough to realize that when mkimage complains that dtc is missing, they should enable host-dtc. Arnout, Yann, Peter? Best regards, Thomas
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: Hi, >> +HOST_UBOOT_TOOLS_DEPENDENCIES += host-dtc > I am not sure I like the idea of having host-uboot-tools always depend > on host-dtc, as it adds build time while host-dtc is only needed for > some specific use cases of mkimage (generating FIT images). > There are really three options I believe: > (1) What you did, i.e have host-dtc as an unconditional dependency of > host-uboot-tools. Everybody pays the price of building host-dtc > even if it's not needed. > (2) Add a sub-option to host-uboot-tools so that people can say "I > need it with FIT image support", which will add host-dtc as a > dependency. > (3) Just do nothing, and let our users be smart enough to realize that > when mkimage complains that dtc is missing, they should enable > host-dtc. Agreed, it's quite similar to the issues with genimage and dosfstools. Either option 2 or 3 sounds good to me.
> From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@buildroot.org, "Arnout Vandecappelle" <arnout@mind.be>, "Yann E. MORIN" <yann.morin.1998@free.fr>, "Peter > Korsgaard" <peter@korsgaard.com> > Sent: Thursday, May 5, 2016 5:09:01 PM > Subject: Re: [Buildroot] [PATCH 1/1] uboot-tools: add missing dependency on host-dtc for the host package > Hello, > > Thanks for this patch. See my comments below. > > On Wed, 4 May 2016 11:38:29 -0300, Carlos Santos wrote: >> The mkimage utility needs dtc when the input is in Flat Image Trees (FIT) >> format. If dtc is not available mkimage fails. Example: >> >> $ mkimage -f firmware.its firmware.im >> sh: dtc: command not found >> >> Signed-off-by: Carlos Santos <casantos@datacom.ind.br> >> --- >> package/uboot-tools/Config.in.host | 1 + >> package/uboot-tools/uboot-tools.mk | 2 ++ >> 2 files changed, 3 insertions(+) >> >> diff --git a/package/uboot-tools/Config.in.host >> b/package/uboot-tools/Config.in.host >> index b5a42d9..5c44eaf 100644 >> --- a/package/uboot-tools/Config.in.host >> +++ b/package/uboot-tools/Config.in.host >> @@ -1,5 +1,6 @@ >> config BR2_PACKAGE_HOST_UBOOT_TOOLS >> bool "host u-boot tools" >> + select BR2_PACKAGE_HOST_DTC >> help >> Companion tools for Das U-Boot bootloader. >> >> diff --git a/package/uboot-tools/uboot-tools.mk >> b/package/uboot-tools/uboot-tools.mk >> index f47b3db..a07fbfa 100644 >> --- a/package/uboot-tools/uboot-tools.mk >> +++ b/package/uboot-tools/uboot-tools.mk >> @@ -65,6 +65,8 @@ define UBOOT_TOOLS_INSTALL_TARGET_CMDS >> $(UBOOT_TOOLS_INSTALL_DUMPIMAGE) >> endef >> >> +HOST_UBOOT_TOOLS_DEPENDENCIES += host-dtc > > I am not sure I like the idea of having host-uboot-tools always depend > on host-dtc, as it adds build time while host-dtc is only needed for > some specific use cases of mkimage (generating FIT images). The extra time needed to build host-dtc is negligible. This is what I get when I build it on my Core i5 notebook, including download time: $ time make host-dtc-dirclean host-dtc [...] real 0m8.387s user 0m7.552s sys 0m2.231s > There are really three options I believe: > > (1) What you did, i.e have host-dtc as an unconditional dependency of > host-uboot-tools. Everybody pays the price of building host-dtc > even if it's not needed. It builds in a few seconds and does not have any impact on the size or performance of the target system, since it is a host tool. > (2) Add a sub-option to host-uboot-tools so that people can say "I > need it with FIT image support", which will add host-dtc as a > dependency. The user would spend more time choosing the extra option than downloading and building host-dtc. > (3) Just do nothing, and let our users be smart enough to realize that > when mkimage complains that dtc is missing, they should enable > host-dtc. The poor user would waste even more time attempting to find what broke the build. Carlos Santos (Casantos) DATACOM, P&D
Hello, On Thu, 5 May 2016 17:32:51 -0300 (BRT), Carlos Santos wrote: > The extra time needed to build host-dtc is negligible. This is what I get when I build it on my Core i5 notebook, including download time: > > $ time make host-dtc-dirclean host-dtc > [...] > real 0m8.387s > user 0m7.552s > sys 0m2.231s This is just the time for host-dtc. But host-dtc depends on host-bison and host-flex, which in turn bring host-m4. In total, on my machine, the time to build host-dtc from a clean build is almost one minute. Not a big amount of time. But FIT images are not that common, and one key aspect of Buildroot is its build time, which we try to keep low by making sure we don't rebuild the whole world needlessly. Thomas
Carlos, All, On 2016-05-05 17:32 -0300, Carlos Santos spake thusly: > > From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > > To: "Carlos Santos" <casantos@datacom.ind.br> > > Cc: buildroot@buildroot.org, "Arnout Vandecappelle" <arnout@mind.be>, "Yann E. MORIN" <yann.morin.1998@free.fr>, "Peter > > Korsgaard" <peter@korsgaard.com> > > Sent: Thursday, May 5, 2016 5:09:01 PM > > Subject: Re: [Buildroot] [PATCH 1/1] uboot-tools: add missing dependency on host-dtc for the host package > > > Hello, > > > > Thanks for this patch. See my comments below. > > > > On Wed, 4 May 2016 11:38:29 -0300, Carlos Santos wrote: > >> The mkimage utility needs dtc when the input is in Flat Image Trees (FIT) > >> format. If dtc is not available mkimage fails. Example: > >> > >> $ mkimage -f firmware.its firmware.im > >> sh: dtc: command not found > >> > >> Signed-off-by: Carlos Santos <casantos@datacom.ind.br> > >> --- > >> package/uboot-tools/Config.in.host | 1 + > >> package/uboot-tools/uboot-tools.mk | 2 ++ > >> 2 files changed, 3 insertions(+) > >> > >> diff --git a/package/uboot-tools/Config.in.host > >> b/package/uboot-tools/Config.in.host > >> index b5a42d9..5c44eaf 100644 > >> --- a/package/uboot-tools/Config.in.host > >> +++ b/package/uboot-tools/Config.in.host > >> @@ -1,5 +1,6 @@ > >> config BR2_PACKAGE_HOST_UBOOT_TOOLS > >> bool "host u-boot tools" > >> + select BR2_PACKAGE_HOST_DTC > >> help > >> Companion tools for Das U-Boot bootloader. > >> > >> diff --git a/package/uboot-tools/uboot-tools.mk > >> b/package/uboot-tools/uboot-tools.mk > >> index f47b3db..a07fbfa 100644 > >> --- a/package/uboot-tools/uboot-tools.mk > >> +++ b/package/uboot-tools/uboot-tools.mk > >> @@ -65,6 +65,8 @@ define UBOOT_TOOLS_INSTALL_TARGET_CMDS > >> $(UBOOT_TOOLS_INSTALL_DUMPIMAGE) > >> endef > >> > >> +HOST_UBOOT_TOOLS_DEPENDENCIES += host-dtc > > > > I am not sure I like the idea of having host-uboot-tools always depend > > on host-dtc, as it adds build time while host-dtc is only needed for > > some specific use cases of mkimage (generating FIT images). > > The extra time needed to build host-dtc is negligible. This is what I > get when I build it on my Core i5 notebook, including download time: > > $ time make host-dtc-dirclean host-dtc > [...] > real 0m8.387s > user 0m7.552s > sys 0m2.231s You forgot that dtc (and thus host-dtc) depends on host-bison and host-flex, and host-bison pulls in host-m4. So you'd have to account for the build time of those, too (but of course, only in case they were not otherwise needed). > > There are really three options I believe: > > > > (1) What you did, i.e have host-dtc as an unconditional dependency of > > host-uboot-tools. Everybody pays the price of building host-dtc > > even if it's not needed. > > It builds in a few seconds and does not have any impact on the size or > performance of the target system, since it is a host tool. > > > (2) Add a sub-option to host-uboot-tools so that people can say "I > > need it with FIT image support", which will add host-dtc as a > > dependency. > > The user would spend more time choosing the extra option than > downloading and building host-dtc. > > > (3) Just do nothing, and let our users be smart enough to realize that > > when mkimage complains that dtc is missing, they should enable > > host-dtc. > > The poor user would waste even more time attempting to find what broke > the build. Right, I'm not too fond of proposal #3 either. I thought we were striving to "make it work out of the box" as much as possible, goal that option #3 completely defeats. Option #2 is not much better however, since it would probably not be easy to match the build failure to that option... Option #1 will make it just work in every cases, at the expense of a bit longer build time (mostly because of dependencies of host-dtc). However, given how pervasive DTC is becoming, I think it would kinda make sense to have a mkimage that supports DT every time (I hope that adding DT support to mkimage does not remove functionality, does it?) So my vote would be with option #1. Regards, Yann E. MORIN.
Hello, On Thu, 5 May 2016 23:27:16 +0200, Yann E. MORIN wrote: > However, given how pervasive DTC is becoming, I think it would kinda > make sense to have a mkimage that supports DT every time (I hope that > adding DT support to mkimage does not remove functionality, does it?) Read again in which cases mkimage needs host-dtc. It is *NOT* related to Device Tree at all. It is related to the FIT image format, which happens to rely on the Device Tree syntax and compiler, but has *nothing* to do with the Device Tree being used by the kernel to get a description of the HW. Best regards, Thomas
Peter, All, On 2016-05-05 22:17 +0200, Peter Korsgaard spake thusly: > >>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > >> +HOST_UBOOT_TOOLS_DEPENDENCIES += host-dtc > > > I am not sure I like the idea of having host-uboot-tools always depend > > on host-dtc, as it adds build time while host-dtc is only needed for > > some specific use cases of mkimage (generating FIT images). > > > There are really three options I believe: > > > (1) What you did, i.e have host-dtc as an unconditional dependency of > > host-uboot-tools. Everybody pays the price of building host-dtc > > even if it's not needed. > > > (2) Add a sub-option to host-uboot-tools so that people can say "I > > need it with FIT image support", which will add host-dtc as a > > dependency. > > > (3) Just do nothing, and let our users be smart enough to realize that > > when mkimage complains that dtc is missing, they should enable > > host-dtc. > > Agreed, it's quite similar to the issues with genimage and > dosfstools. I'm always a bit uneasy speaking about genimage, as I don't use it. However, I would contend that we should try to provide as much a "works out of the box" experience as possible, so I'd lean on the side of always enable optional packages to make genimage always happy. But then, genimage being so versatile, it would be perfectly possible to ask it to build all sorts of filesystems, or aggregate all sorts of blobs (e.g. DTB), and so on. Should we forcibly enable all those tools when genimage is enabled? I guess not. Granted, most of the boards we currently have use a VFAT partition (except those from boundary devices, the wandboard and the a20_olinuxino, i.e. 4 boards). So, I'm completely undecided for genimage. On the one side, there the "out of the box" experience; on the other side, there's the build time... But dosfstools has no dependency, and it does build quite fast, so we could at least enable that one... > Either option 2 or 3 sounds good to me. Hmm... See my other reply for why I think option #1 is better. ;-) Regards, Yann E. MORIN.
Thomas, All, On 2016-05-05 23:38 +0200, Thomas Petazzoni spake thusly: > On Thu, 5 May 2016 23:27:16 +0200, Yann E. MORIN wrote: > > However, given how pervasive DTC is becoming, I think it would kinda > > make sense to have a mkimage that supports DT every time (I hope that > > adding DT support to mkimage does not remove functionality, does it?) > > Read again in which cases mkimage needs host-dtc. It is *NOT* related > to Device Tree at all. It is related to the FIT image format, which > happens to rely on the Device Tree syntax and compiler, but has > *nothing* to do with the Device Tree being used by the kernel to get a > description of the HW. Oh, OK. /me unconsciously associated dtc with generating DTB. But if it can be used for other things as well... Then option 2 is the best solution IMHO. Regards, Yann E. MORIN.
> From: "Yann E. MORIN" <yann.morin.1998@free.fr> > To: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > Cc: "Carlos Santos" <casantos@datacom.ind.br>, buildroot@buildroot.org, "Arnout Vandecappelle" <arnout@mind.be>, "Peter > Korsgaard" <peter@korsgaard.com> > Sent: Thursday, May 5, 2016 6:44:32 PM > Subject: Re: [Buildroot] [PATCH 1/1] uboot-tools: add missing dependency on host-dtc for the host package > Thomas, All, > > On 2016-05-05 23:38 +0200, Thomas Petazzoni spake thusly: >> On Thu, 5 May 2016 23:27:16 +0200, Yann E. MORIN wrote: >> > However, given how pervasive DTC is becoming, I think it would kinda >> > make sense to have a mkimage that supports DT every time (I hope that >> > adding DT support to mkimage does not remove functionality, does it?) >> >> Read again in which cases mkimage needs host-dtc. It is *NOT* related >> to Device Tree at all. It is related to the FIT image format, which >> happens to rely on the Device Tree syntax and compiler, but has >> *nothing* to do with the Device Tree being used by the kernel to get a >> description of the HW. > > Oh, OK. > > /me unconsciously associated dtc with generating DTB. But if it can be > used for other things as well... > > Then option 2 is the best solution IMHO. Done (see patch v2). Carlos Santos (Casantos) DATACOM, P&D
diff --git a/package/uboot-tools/Config.in.host b/package/uboot-tools/Config.in.host index b5a42d9..5c44eaf 100644 --- a/package/uboot-tools/Config.in.host +++ b/package/uboot-tools/Config.in.host @@ -1,5 +1,6 @@ config BR2_PACKAGE_HOST_UBOOT_TOOLS bool "host u-boot tools" + select BR2_PACKAGE_HOST_DTC help Companion tools for Das U-Boot bootloader. diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk index f47b3db..a07fbfa 100644 --- a/package/uboot-tools/uboot-tools.mk +++ b/package/uboot-tools/uboot-tools.mk @@ -65,6 +65,8 @@ define UBOOT_TOOLS_INSTALL_TARGET_CMDS $(UBOOT_TOOLS_INSTALL_DUMPIMAGE) endef +HOST_UBOOT_TOOLS_DEPENDENCIES += host-dtc + ifeq ($(BR2_PACKAGE_HOST_UBOOT_TOOLS_FIT_SIGNATURE_SUPPORT),y) HOST_UBOOT_TOOLS_DEPENDENCIES += host-openssl endif
The mkimage utility needs dtc when the input is in Flat Image Trees (FIT) format. If dtc is not available mkimage fails. Example: $ mkimage -f firmware.its firmware.im sh: dtc: command not found Signed-off-by: Carlos Santos <casantos@datacom.ind.br> --- package/uboot-tools/Config.in.host | 1 + package/uboot-tools/uboot-tools.mk | 2 ++ 2 files changed, 3 insertions(+)