[PATCH] board: tbs2910: Add support for generic distro configuration

[Soeren Moch]

On 26.01.20 00:15, Denis 'GNUtoo' Carikli wrote:
> On Sat, 25 Jan 2020 20:52:36 +0100
> Soeren Moch <smoch at 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