| Message ID | 20200125014240.18126-1-GNUtoo@cyberdimension.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | board: tbs2910: Add support for generic distro configuration | expand |
On Sat, Jan 25, 2020 at 02:42:40AM +0100, Denis 'GNUtoo' Carikli wrote: > This keeps the compatibility with the old bootcmd. > > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> First, I have concerns about this increasing the size of the board. > --- > include/configs/tbs2910.h | 39 ++++++++++++++++++++++++++++++--------- > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/include/configs/tbs2910.h b/include/configs/tbs2910.h > index b598fca1ec..8867918f37 100644 > --- a/include/configs/tbs2910.h > +++ b/include/configs/tbs2910.h > @@ -8,6 +8,26 @@ > #ifndef __TBS2910_CONFIG_H > #define __TBS2910_CONFIG_H > > +#define CONFIG_BOOTCOMMAND \ > + "mmc rescan; " \ > + "if run bootcmd_up1; then " \ > + "run bootcmd_up2; " \ > + "else " \ > + "run bootcmd_mmc || run distro_bootcmd; " \ > + "fi" > + > +#ifndef CONFIG_SPL_BUILD > +#define BOOT_TARGET_DEVICES(func) \ > + func(MMC, mmc, 0) \ > + func(MMC, mmc, 1) \ > + func(MMC, mmc, 2) \ > + func(SATA, sata, 0) \ > + func(USB, usb, 0) \ > + func(PXE, pxe, na) \ > + func(DHCP, dhcp, na) > +#include <config_distro_bootcmd.h> > +#endif > + > #include "mx6_common.h" > > /* General configuration */ > @@ -80,6 +100,13 @@ > #define CONFIG_BOARD_SIZE_LIMIT 392192 /* (CONFIG_ENV_OFFSET - 1024) */ > > #define CONFIG_EXTRA_ENV_SETTINGS \ > + "fdt_addr=0x13000000\0" \ > + "fdt_addr_r=0x13000000\0" \ > + "initrd_high=0xffffffff\0" \ > + "kernel_addr_r=0x10008000\0" \ > + "pxefile_addr_r=0x10008000\0" \ > + "ramdisk_addr_r=0x18000000\0" \ > + "scriptaddr=0x14000000\0" \ Second, why are you disabling initrd relocation? And we should set bootm_size so that all relocations are done within the appropriate memory window. > "bootargs_mmc1=console=ttymxc0,115200 di0_primary console=tty1\0" \ > "bootargs_mmc2=video=mxcfb0:dev=hdmi,1920x1080M@60 " \ > "video=mxcfb1:off video=mxcfb2:off fbmem=28M\0" \ > @@ -102,14 +129,8 @@ > "setenv stderr serial,vga\0" \ > "stderr=serial,vga\0" \ > "stdin=serial,usbkbd\0" \ > - "stdout=serial,vga\0" > - > -#define CONFIG_BOOTCOMMAND \ > - "mmc rescan; " \ > - "if run bootcmd_up1; then " \ > - "run bootcmd_up2; " \ > - "else " \ > - "run bootcmd_mmc; " \ > - "fi" > + "stdout=serial,vga\0" \ > + "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ > + BOOTENV > > #endif /* __TBS2910_CONFIG_H * */ > -- > 2.24.1 >
Hi Denis, thanks for your patch. In general I think it could be a good idea to support distroboot on this board, especially if we can maintain compatibility with the existing boot procedure. However, since this board repeatedly has size problems with the u-boot binary, we carefully need to check binary size. More questions inline. On 25.01.20 02:42, Denis 'GNUtoo' Carikli wrote: > This keeps the compatibility with the old bootcmd. > > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> > --- > include/configs/tbs2910.h | 39 ++++++++++++++++++++++++++++++--------- > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/include/configs/tbs2910.h b/include/configs/tbs2910.h > index b598fca1ec..8867918f37 100644 > --- a/include/configs/tbs2910.h > +++ b/include/configs/tbs2910.h > @@ -8,6 +8,26 @@ > #ifndef __TBS2910_CONFIG_H > #define __TBS2910_CONFIG_H > > +#define CONFIG_BOOTCOMMAND \ > + "mmc rescan; " \ > + "if run bootcmd_up1; then " \ > + "run bootcmd_up2; " \ > + "else " \ > + "run bootcmd_mmc || run distro_bootcmd; " \ > + "fi" > + Why do you define CONFIG_BOOTCOMMAND here instead of modifying the existing one? > +#ifndef CONFIG_SPL_BUILD There in no SPL for tbs2910. So this is not required. > +#define BOOT_TARGET_DEVICES(func) \ > + func(MMC, mmc, 0) \ > + func(MMC, mmc, 1) \ > + func(MMC, mmc, 2) \ > + func(SATA, sata, 0) \ > + func(USB, usb, 0) \ > + func(PXE, pxe, na) \ > + func(DHCP, dhcp, na) > +#include <config_distro_bootcmd.h> > +#endif > + > #include "mx6_common.h" > > /* General configuration */ > @@ -80,6 +100,13 @@ > #define CONFIG_BOARD_SIZE_LIMIT 392192 /* (CONFIG_ENV_OFFSET - 1024) */ > > #define CONFIG_EXTRA_ENV_SETTINGS \ > + "fdt_addr=0x13000000\0" \ > + "fdt_addr_r=0x13000000\0" \ > + "initrd_high=0xffffffff\0" \ This should not be defined. Usually we want to relocate the initrd. Regards, Soeren > + "kernel_addr_r=0x10008000\0" \ > + "pxefile_addr_r=0x10008000\0" \ > + "ramdisk_addr_r=0x18000000\0" \ > + "scriptaddr=0x14000000\0" \ > "bootargs_mmc1=console=ttymxc0,115200 di0_primary console=tty1\0" \ > "bootargs_mmc2=video=mxcfb0:dev=hdmi,1920x1080M@60 " \ > "video=mxcfb1:off video=mxcfb2:off fbmem=28M\0" \ > @@ -102,14 +129,8 @@ > "setenv stderr serial,vga\0" \ > "stderr=serial,vga\0" \ > "stdin=serial,usbkbd\0" \ > - "stdout=serial,vga\0" > - > -#define CONFIG_BOOTCOMMAND \ > - "mmc rescan; " \ > - "if run bootcmd_up1; then " \ > - "run bootcmd_up2; " \ > - "else " \ > - "run bootcmd_mmc; " \ > - "fi" > + "stdout=serial,vga\0" \ > + "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ > + BOOTENV > > #endif /* __TBS2910_CONFIG_H * */
On 25.01.20 17:24, Tom Rini wrote: > On Sat, Jan 25, 2020 at 02:42:40AM +0100, Denis 'GNUtoo' Carikli wrote: > >> This keeps the compatibility with the old bootcmd. >> >> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> > First, I have concerns about this increasing the size of the board. > >> --- >> include/configs/tbs2910.h | 39 ++++++++++++++++++++++++++++++--------- >> 1 file changed, 30 insertions(+), 9 deletions(-) >> >> diff --git a/include/configs/tbs2910.h b/include/configs/tbs2910.h >> index b598fca1ec..8867918f37 100644 >> --- a/include/configs/tbs2910.h >> +++ b/include/configs/tbs2910.h >> @@ -8,6 +8,26 @@ >> #ifndef __TBS2910_CONFIG_H >> #define __TBS2910_CONFIG_H >> >> +#define CONFIG_BOOTCOMMAND \ >> + "mmc rescan; " \ >> + "if run bootcmd_up1; then " \ >> + "run bootcmd_up2; " \ >> + "else " \ >> + "run bootcmd_mmc || run distro_bootcmd; " \ >> + "fi" >> + >> +#ifndef CONFIG_SPL_BUILD >> +#define BOOT_TARGET_DEVICES(func) \ >> + func(MMC, mmc, 0) \ >> + func(MMC, mmc, 1) \ >> + func(MMC, mmc, 2) \ >> + func(SATA, sata, 0) \ >> + func(USB, usb, 0) \ >> + func(PXE, pxe, na) \ >> + func(DHCP, dhcp, na) >> +#include <config_distro_bootcmd.h> >> +#endif >> + >> #include "mx6_common.h" >> >> /* General configuration */ >> @@ -80,6 +100,13 @@ >> #define CONFIG_BOARD_SIZE_LIMIT 392192 /* (CONFIG_ENV_OFFSET - 1024) */ >> >> #define CONFIG_EXTRA_ENV_SETTINGS \ >> + "fdt_addr=0x13000000\0" \ >> + "fdt_addr_r=0x13000000\0" \ >> + "initrd_high=0xffffffff\0" \ >> + "kernel_addr_r=0x10008000\0" \ >> + "pxefile_addr_r=0x10008000\0" \ >> + "ramdisk_addr_r=0x18000000\0" \ >> + "scriptaddr=0x14000000\0" \ > Second, why are you disabling initrd relocation? And we should set > bootm_size so that all relocations are done within the appropriate > memory window. We use CONFIG_SYS_BOOTMAPSZ so far to define the available save low memory. Is bootm_size required for distroboot, or is this a better alternative? Regards, Soeren > >> "bootargs_mmc1=console=ttymxc0,115200 di0_primary console=tty1\0" \ >> "bootargs_mmc2=video=mxcfb0:dev=hdmi,1920x1080M@60 " \ >> "video=mxcfb1:off video=mxcfb2:off fbmem=28M\0" \ >> @@ -102,14 +129,8 @@ >> "setenv stderr serial,vga\0" \ >> "stderr=serial,vga\0" \ >> "stdin=serial,usbkbd\0" \ >> - "stdout=serial,vga\0" >> - >> -#define CONFIG_BOOTCOMMAND \ >> - "mmc rescan; " \ >> - "if run bootcmd_up1; then " \ >> - "run bootcmd_up2; " \ >> - "else " \ >> - "run bootcmd_mmc; " \ >> - "fi" >> + "stdout=serial,vga\0" \ >> + "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ >> + BOOTENV >> >> #endif /* __TBS2910_CONFIG_H * */ >> -- >> 2.24.1 >>
On Sat, Jan 25, 2020 at 08:56:12PM +0100, Soeren Moch wrote: > On 25.01.20 17:24, Tom Rini wrote: > > On Sat, Jan 25, 2020 at 02:42:40AM +0100, Denis 'GNUtoo' Carikli wrote: > > > >> This keeps the compatibility with the old bootcmd. > >> > >> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> > > First, I have concerns about this increasing the size of the board. > > > >> --- > >> include/configs/tbs2910.h | 39 ++++++++++++++++++++++++++++++--------- > >> 1 file changed, 30 insertions(+), 9 deletions(-) > >> > >> diff --git a/include/configs/tbs2910.h b/include/configs/tbs2910.h > >> index b598fca1ec..8867918f37 100644 > >> --- a/include/configs/tbs2910.h > >> +++ b/include/configs/tbs2910.h > >> @@ -8,6 +8,26 @@ > >> #ifndef __TBS2910_CONFIG_H > >> #define __TBS2910_CONFIG_H > >> > >> +#define CONFIG_BOOTCOMMAND \ > >> + "mmc rescan; " \ > >> + "if run bootcmd_up1; then " \ > >> + "run bootcmd_up2; " \ > >> + "else " \ > >> + "run bootcmd_mmc || run distro_bootcmd; " \ > >> + "fi" > >> + > >> +#ifndef CONFIG_SPL_BUILD > >> +#define BOOT_TARGET_DEVICES(func) \ > >> + func(MMC, mmc, 0) \ > >> + func(MMC, mmc, 1) \ > >> + func(MMC, mmc, 2) \ > >> + func(SATA, sata, 0) \ > >> + func(USB, usb, 0) \ > >> + func(PXE, pxe, na) \ > >> + func(DHCP, dhcp, na) > >> +#include <config_distro_bootcmd.h> > >> +#endif > >> + > >> #include "mx6_common.h" > >> > >> /* General configuration */ > >> @@ -80,6 +100,13 @@ > >> #define CONFIG_BOARD_SIZE_LIMIT 392192 /* (CONFIG_ENV_OFFSET - 1024) */ > >> > >> #define CONFIG_EXTRA_ENV_SETTINGS \ > >> + "fdt_addr=0x13000000\0" \ > >> + "fdt_addr_r=0x13000000\0" \ > >> + "initrd_high=0xffffffff\0" \ > >> + "kernel_addr_r=0x10008000\0" \ > >> + "pxefile_addr_r=0x10008000\0" \ > >> + "ramdisk_addr_r=0x18000000\0" \ > >> + "scriptaddr=0x14000000\0" \ > > Second, why are you disabling initrd relocation? And we should set > > bootm_size so that all relocations are done within the appropriate > > memory window. > > We use CONFIG_SYS_BOOTMAPSZ so far to define the available save low > memory. Is bootm_size required for distroboot, or is this a better > alternative? We have many ways of achieving the same ends. Per the README (I had to double check), if SYS_BOOTMAPSZ is not set then if bootm_size is set in the environment we use that. So in the end, same thing. And we just drop initrd_high here.
On Sat, 25 Jan 2020 20:52:36 +0100 Soeren Moch <smoch@web.de> wrote: > Hi Denis, Hi, > thanks for your patch. In general I think it could be a good idea to > support distroboot on this board, especially if we can maintain > compatibility with the existing boot procedure. However, since this > board repeatedly has size problems with the u-boot binary, we > carefully need to check binary size. I saw that. I also experienced size issues with the stock tbs2910_defconfig, and for now I just locally removed support for things that eats up too much space like PCIe support. > There in no SPL for tbs2910. So this is not required. Ahh ok, now I understand. That probably explains the repeated size issues. For my patch, I could guard the code addition with some ifdefs for CONFIG_DISTRO_DEFAULTS if necessary. Note that CONFIG_DISTRO_DEFAULTS is not set in the tbs2910_defconfig, so if done correctly it should not make things worse. In the long run it's probably worth looking into adding SPL support. For instance the Wandboard has it. I'll try to find the time to look into it but I can't guarantee anything. As for the rest of the questions: > Why do you define CONFIG_BOOTCOMMAND here instead of modifying the > existing one? > > +#define CONFIG_BOOTCOMMAND \ > > + "mmc rescan; " \ > > + "if run bootcmd_up1; then " \ > > + "run bootcmd_up2; " \ > > + "else " \ > > + "run bootcmd_mmc || run distro_bootcmd; " \ > > + "fi" > > + > Why do you define CONFIG_BOOTCOMMAND here instead of modifying the > existing one? I'm a bit confused with it: There seem to be many way to do the same thing and I'm not sure which one is the best. Because of that, I just kept it in the code as it was (instead of moving it to tbs2910_defconfig). I'm also not sure if it's best to run distro_bootcmd before or after the bootcmd_up1/bootcmd_up2/bootcmd_mmc commands, which are probably used to boot some distribution like LibreElec. Denis.
On 26.01.20 00:15, Denis 'GNUtoo' Carikli wrote: > On Sat, 25 Jan 2020 20:52:36 +0100 > Soeren Moch <smoch@web.de> wrote: > >> Hi Denis, > Hi, > >> thanks for your patch. In general I think it could be a good idea to >> support distroboot on this board, especially if we can maintain >> compatibility with the existing boot procedure. However, since this >> board repeatedly has size problems with the u-boot binary, we >> carefully need to check binary size. > I saw that. > > I also experienced size issues with the stock tbs2910_defconfig, and > for now I just locally removed support for things that eats up too much > space like PCIe support. > >> There in no SPL for tbs2910. So this is not required. > Ahh ok, now I understand. That probably explains the repeated size > issues. Why? With SPL+u-boot proper it would be even worse, since then there is a gap between SPL and u-boot, u-boot starts at higher address in eMMC, and it would not be smaller. And this 2-stage boot would break the documented u-boot update procedure, since we have to flash 2 files then. > > For my patch, I could guard the code addition with some ifdefs for > CONFIG_DISTRO_DEFAULTS if necessary. Note that CONFIG_DISTRO_DEFAULTS is > not set in the tbs2910_defconfig, so if done correctly it should not > make things worse. That means, without this change in defconfig the whole distroboot does not work? I will only take this patch if distroboot really works with defconfig then. > > In the long run it's probably worth looking into adding SPL support. > For instance the Wandboard has it. I'll try to find the time to look > into it but I can't guarantee anything. SPL only is required to detect i.MX6 flavour and SDRAM size and location. This is not necessary for tbs2910, since this board is only available with i.MX6Q. > > As for the rest of the questions: >> Why do you define CONFIG_BOOTCOMMAND here instead of modifying the >> existing one? >>> +#define CONFIG_BOOTCOMMAND \ >>> + "mmc rescan; " \ >>> + "if run bootcmd_up1; then " \ >>> + "run bootcmd_up2; " \ >>> + "else " \ >>> + "run bootcmd_mmc || run distro_bootcmd; " \ >>> + "fi" >>> + >> Why do you define CONFIG_BOOTCOMMAND here instead of modifying the >> existing one? > I'm a bit confused with it: There seem to be many way to do the same > thing and I'm not sure which one is the best. > > Because of that, I just kept it in the code as it was (instead of > moving it to tbs2910_defconfig). This is ok. What I mean: You moved it in the file, and therefore you unnecessarily changed a lot of lines without actually changing most of it's contents. > > I'm also not sure if it's best to run distro_bootcmd before or > after the bootcmd_up1/bootcmd_up2/bootcmd_mmc commands, which are > probably used to boot some distribution like LibreElec. > You did it right, as last boot option. Otherwise you would break compatibility to the existing boot flow. One additional comment: currently we want to strip down the embedded dtb to reduce size. This means we cannot pass this dtb to linux, and always need to load a different one (together with kernel and initrd). This is no problem for extlinux.conf, but maybe we can omit to define fdtfile just to make sure we have to load a different one. I'm not sure, however, if distroboot scripts work without this default dtb. Soeren
On Sun, 26 Jan 2020 01:40:13 +0100 Soeren Moch <smoch@web.de> wrote: > > Ahh ok, now I understand. That probably explains the repeated size > > issues. > Why? With SPL+u-boot proper it would be even worse, since then there > is a gap between SPL and u-boot, u-boot starts at higher address in > eMMC, and it would not be smaller. And this 2-stage boot would break > the documented u-boot update procedure, since we have to flash 2 > files then. I assumed that in some conditions, the bootrom could load only the SPL in sram. Once loaded, the SPL would initialize the RAM, detect the boot media, and fetch u-boot.img from it, and execute it. I also hoped the the SPL would have been significantly smaller than the current u-boot.imx image. In the meantime, I'll send a v2 with some additional patches to reduce the size of the resulting u-boot.imx. Denis.
On 28.01.20 18:02, Denis 'GNUtoo' Carikli wrote: > On Sun, 26 Jan 2020 01:40:13 +0100 > Soeren Moch <smoch@web.de> wrote: >>> Ahh ok, now I understand. That probably explains the repeated size >>> issues. >> Why? With SPL+u-boot proper it would be even worse, since then there >> is a gap between SPL and u-boot, u-boot starts at higher address in >> eMMC, and it would not be smaller. And this 2-stage boot would break >> the documented u-boot update procedure, since we have to flash 2 >> files then. > I assumed that in some conditions, the bootrom could load only the SPL > in sram. Once loaded, the SPL would initialize the RAM, detect the boot > media, and fetch u-boot.img from it, and execute it. It is different here. i.MX6Q boot rom loads a imx file. This initializes the DDR controller first, then loads the executable payload, u-boot.bin in this case. So there is no restriction from SRAM size, as long as the DDR settings are fixed, what is the case for tbs2910. > > I also hoped the the SPL would have been significantly smaller than the > current u-boot.imx image. It would be. But then you need the u-boot.bin in addition to the SPL.imx , with all the problems explained earlier. > > In the meantime, I'll send a v2 with some additional patches to reduce > the size of the resulting u-boot.imx. As already explained, this is not necessary now. Soeren > > Denis.
On Sun, 26 Jan 2020 01:40:13 +0100 Soeren Moch <smoch@web.de> wrote: > >> Why do you define CONFIG_BOOTCOMMAND here instead of modifying the > >> existing one? > > I'm a bit confused with it: There seem to be many way to do the same > > thing and I'm not sure which one is the best. > > > > Because of that, I just kept it in the code as it was (instead of > > moving it to tbs2910_defconfig). > This is ok. What I mean: You moved it in the file, and therefore you > unnecessarily changed a lot of lines without actually changing most of > it's contents. If CONFIG_BOOTCOMMAND is not defined before, it will be defined in config_distro_bootcmd.h This means that the order in which defines and #include <config_distro_bootcmd.h> is done matters. Which lead me to do the inclusion of config_distro_bootcmd.h as early as possible in the tbs2910.h. This way if there are some unintended redefinition due to things having changed in config_distro_bootcmd.h in the future, the compiler will at warn or error about it. Denis.
diff --git a/include/configs/tbs2910.h b/include/configs/tbs2910.h index b598fca1ec..8867918f37 100644 --- a/include/configs/tbs2910.h +++ b/include/configs/tbs2910.h @@ -8,6 +8,26 @@ #ifndef __TBS2910_CONFIG_H #define __TBS2910_CONFIG_H +#define CONFIG_BOOTCOMMAND \ + "mmc rescan; " \ + "if run bootcmd_up1; then " \ + "run bootcmd_up2; " \ + "else " \ + "run bootcmd_mmc || run distro_bootcmd; " \ + "fi" + +#ifndef CONFIG_SPL_BUILD +#define BOOT_TARGET_DEVICES(func) \ + func(MMC, mmc, 0) \ + func(MMC, mmc, 1) \ + func(MMC, mmc, 2) \ + func(SATA, sata, 0) \ + func(USB, usb, 0) \ + func(PXE, pxe, na) \ + func(DHCP, dhcp, na) +#include <config_distro_bootcmd.h> +#endif + #include "mx6_common.h" /* General configuration */ @@ -80,6 +100,13 @@ #define CONFIG_BOARD_SIZE_LIMIT 392192 /* (CONFIG_ENV_OFFSET - 1024) */ #define CONFIG_EXTRA_ENV_SETTINGS \ + "fdt_addr=0x13000000\0" \ + "fdt_addr_r=0x13000000\0" \ + "initrd_high=0xffffffff\0" \ + "kernel_addr_r=0x10008000\0" \ + "pxefile_addr_r=0x10008000\0" \ + "ramdisk_addr_r=0x18000000\0" \ + "scriptaddr=0x14000000\0" \ "bootargs_mmc1=console=ttymxc0,115200 di0_primary console=tty1\0" \ "bootargs_mmc2=video=mxcfb0:dev=hdmi,1920x1080M@60 " \ "video=mxcfb1:off video=mxcfb2:off fbmem=28M\0" \ @@ -102,14 +129,8 @@ "setenv stderr serial,vga\0" \ "stderr=serial,vga\0" \ "stdin=serial,usbkbd\0" \ - "stdout=serial,vga\0" - -#define CONFIG_BOOTCOMMAND \ - "mmc rescan; " \ - "if run bootcmd_up1; then " \ - "run bootcmd_up2; " \ - "else " \ - "run bootcmd_mmc; " \ - "fi" + "stdout=serial,vga\0" \ + "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ + BOOTENV #endif /* __TBS2910_CONFIG_H * */
This keeps the compatibility with the old bootcmd. Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> --- include/configs/tbs2910.h | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)