[v3,4/6] board: tbs2910: enable CONFIG_DISTRO_DEFAULTS
diff mbox series

Message ID 20200208053839.7101-4-GNUtoo@cyberdimension.org
State Deferred
Delegated to: Stefano Babic
Headers show
Series
  • [v3,1/6] board: tbs2910: disable loadb and loads commands
Related show

Commit Message

Denis 'GNUtoo' Carikli Feb. 8, 2020, 5:38 a.m. UTC
The side effect is that it increase the size of the
resultimg image, which is already very close to the
size limit.

With arm-linux-gnueabi-gcc 9.2.0-1 from the Parabola
GNU/Linux distribution, we have the following size
increase:
- text: 8744 bytes
- data: 132 bytes
- bss: 60 bytes
- total: 8936 bytes

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 configs/tbs2910_defconfig | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

Comments

Soeren Moch Feb. 8, 2020, 3:47 p.m. UTC | #1
On 08.02.20 06:38, Denis 'GNUtoo' Carikli wrote:
> The side effect is that it increase the size of the
> resultimg image, which is already very close to the
> size limit.
>
> With arm-linux-gnueabi-gcc 9.2.0-1 from the Parabola
> GNU/Linux distribution, we have the following size
> increase:
> - text: 8744 bytes
> - data: 132 bytes
> - bss: 60 bytes
> - total: 8936 bytes
>
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Thank you very much for splitting up the patches and for your thorough
size analysis. This makes it very easy to understand the size
contributions of the several parts.

It was really surprising for me that CONFIG_DISTRO_DEFAULTS brings so
much additional text. I thought this would only enable
CONFIG_CMD_SYSBOOT, but we also get PXE support. I wasn't aware of this
before.

As you already noticed, we are quite close to the size limit of the
u-boot binary. So I would prefer to omit PXE.

Do you really want to use PXE boot on this board? Or would it be OK to
only enable CMD_SYSBOOT instead of DISTRO_DEFAULTS (and remove DHCP and
PXE boot targets in patch 5/6)?

Sorry for not bringing up this earlier,
Soeren
Tom Rini Feb. 10, 2020, 2:40 p.m. UTC | #2
On Sat, Feb 08, 2020 at 04:47:08PM +0100, Soeren Moch wrote:
> On 08.02.20 06:38, Denis 'GNUtoo' Carikli wrote:
> > The side effect is that it increase the size of the
> > resultimg image, which is already very close to the
> > size limit.
> >
> > With arm-linux-gnueabi-gcc 9.2.0-1 from the Parabola
> > GNU/Linux distribution, we have the following size
> > increase:
> > - text: 8744 bytes
> > - data: 132 bytes
> > - bss: 60 bytes
> > - total: 8936 bytes
> >
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> Thank you very much for splitting up the patches and for your thorough
> size analysis. This makes it very easy to understand the size
> contributions of the several parts.
> 
> It was really surprising for me that CONFIG_DISTRO_DEFAULTS brings so
> much additional text. I thought this would only enable
> CONFIG_CMD_SYSBOOT, but we also get PXE support. I wasn't aware of this
> before.

Distro boot brings in a ton of "support booting via any supported IO
device" code.

That said, the "bring in PXE" part of DISTRO_DEFAULTS predates sysboot
being pulled out of the PXE code, where it was historically introduced.
I would like to see a patch to change this part, stand-alone and CC'ing
the distribution folks that might have something to say about this.  I
know there are use-cases for it, but I don't know how critical they are
to be everywhere by default vs opt-in.  Thanks all!
Denis 'GNUtoo' Carikli Feb. 27, 2020, 12:48 a.m. UTC | #3
On Mon, 10 Feb 2020 09:40:50 -0500
Tom Rini <trini@konsulko.com> wrote:
> That said, the "bring in PXE" part of DISTRO_DEFAULTS predates sysboot
> being pulled out of the PXE code, where it was historically
> introduced. I would like to see a patch to change this part,
> stand-alone and CC'ing the distribution folks that might have
> something to say about this.  I know there are use-cases for it, but
> I don't know how critical they are to be everywhere by default vs
> opt-in.  Thanks all!
I've done the patch, but I've not sent it yet.

With it, if board are using things like func(PXE, pxe, na) in
BOOT_TARGET_DEVICES, it will breaks the compilation, and we would have
an error that looks like that:
> In file included from include/configs/tbs2910.h:19,
>                  from include/config.h:5,
>                  from include/common.h:16,
>                  from env/common.c:10:
> include/config_distro_bootcmd.h:398:2: error: expected '}' before
> 'BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE'
> 398 |
> BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE

Should I still send the patch as-is by mentioning that issue somehow,
in order to start a discussion on the topic?

Or would I need to do more in depth changes before sending the patch?

In the later case I would also need to find some time to do that, so it
might not be done that fast.

Denis.
Tom Rini Feb. 28, 2020, 3:23 p.m. UTC | #4
On Thu, Feb 27, 2020 at 01:48:23AM +0100, Denis 'GNUtoo' Carikli wrote:
> On Mon, 10 Feb 2020 09:40:50 -0500
> Tom Rini <trini@konsulko.com> wrote:
> > That said, the "bring in PXE" part of DISTRO_DEFAULTS predates sysboot
> > being pulled out of the PXE code, where it was historically
> > introduced. I would like to see a patch to change this part,
> > stand-alone and CC'ing the distribution folks that might have
> > something to say about this.  I know there are use-cases for it, but
> > I don't know how critical they are to be everywhere by default vs
> > opt-in.  Thanks all!
> I've done the patch, but I've not sent it yet.
> 
> With it, if board are using things like func(PXE, pxe, na) in
> BOOT_TARGET_DEVICES, it will breaks the compilation, and we would have
> an error that looks like that:
> > In file included from include/configs/tbs2910.h:19,
> >                  from include/config.h:5,
> >                  from include/common.h:16,
> >                  from env/common.c:10:
> > include/config_distro_bootcmd.h:398:2: error: expected '}' before
> > 'BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE'
> > 398 |
> > BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
> 
> Should I still send the patch as-is by mentioning that issue somehow,
> in order to start a discussion on the topic?

Sounds like some of the logic needs updating.  If a board is saying
func(PXE,...) then it should be enabling CMD_PXE.  So yes, I'd like to
see things get a bit farther if you can, thanks!

Patch
diff mbox series

diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
index 5603bef64e..1cf09bb741 100644
--- a/configs/tbs2910_defconfig
+++ b/configs/tbs2910_defconfig
@@ -9,18 +9,15 @@  CONFIG_NR_DRAM_BANKS=1
 CONFIG_PRE_CON_BUF_ADDR=0x7c000000
 CONFIG_CMD_HDMIDETECT=y
 CONFIG_AHCI=y
+CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTDELAY=3
-CONFIG_USE_BOOTCOMMAND=y
 CONFIG_BOOTCOMMAND="mmc rescan; if run bootcmd_up1; then run bootcmd_up2; else run bootcmd_mmc; fi"
 CONFIG_USE_PREBOOT=y
 CONFIG_PREBOOT="echo PCI:; pci enum; pci 1; usb start; if hdmidet; then run set_con_hdmi; else run set_con_serial; fi"
 CONFIG_PRE_CONSOLE_BUFFER=y
-CONFIG_SUPPORT_RAW_INITRD=y
 CONFIG_BOUNCE_BUFFER=y
 CONFIG_BOARD_EARLY_INIT_F=y
-CONFIG_HUSH_PARSER=y
 CONFIG_SYS_PROMPT="Matrix U-Boot> "
-CONFIG_CMD_BOOTZ=y
 # CONFIG_BOOTM_PLAN9 is not set
 # CONFIG_BOOTM_RTEMS is not set
 # CONFIG_BOOTM_VXWORKS is not set
@@ -31,22 +28,14 @@  CONFIG_CMD_I2C=y
 # CONFIG_CMD_LOADB is not set
 # CONFIG_CMD_LOADS is not set
 CONFIG_CMD_MMC=y
-CONFIG_CMD_PART=y
 CONFIG_CMD_PCI=y
 CONFIG_CMD_SATA=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_USB_MASS_STORAGE=y
-CONFIG_CMD_DHCP=y
-CONFIG_CMD_MII=y
-CONFIG_CMD_PING=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
-CONFIG_CMD_EXT2=y
-CONFIG_CMD_EXT4=y
 CONFIG_CMD_EXT4_WRITE=y
-CONFIG_CMD_FAT=y
-CONFIG_CMD_FS_GENERIC=y
-CONFIG_EFI_PARTITION=y
+# CONFIG_ISO_PARTITION is not set
 CONFIG_OF_CONTROL=y
 CONFIG_OF_EMBED=y
 CONFIG_DEFAULT_DEVICE_TREE="imx6q-tbs2910"
@@ -76,7 +65,6 @@  CONFIG_RTC_DS1307=y
 CONFIG_DM_THERMAL=y
 CONFIG_USB=y
 CONFIG_DM_USB=y
-CONFIG_USB_STORAGE=y
 CONFIG_USB_KEYBOARD=y
 CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE=y
 CONFIG_USB_GADGET=y