Re: [PATCH v3 2/2] board: mntre: imx8mq: Add MNT Reform 2 board support

[Vagrant Cascadian <vagrant@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

On 2021-12-14, Patrick Wildt wrote:
> The MNT Reform 2 is a modular DIY laptop.  In its initial version it
> is based on the BoundaryDevices i.MX8MQ SoM.  Some parts have been
> lifted from BoundaryDevices official U-Boot downstream project.

Successfully loaded u-boot, loaded a kernel and .dtb, thanks!

Tested-By: Vagrant Cascadian <vagrant@debian.org>

This may not be specific to this patch series, but one problem I had
during build is that it doesn't appear to respect the BL31 environment
variable that many other platforms support to specify the path to the
ATF firmware; is there a way to add that with binman?

Similarly, it would be ideal to have environment variables for the other
various firmware (lpddr*.bin, signed-hdmi*.bin) needed to build. The
Debian u-boot packages build all boards from a single source, and
copying *.bin into the top-level directory for each one at the right
time seems trickier than telling each build target where the firmware is
via environment variables.


A couple relatively small things in the patch itself:

> diff --git a/include/configs/imx8mq_reform2.h b/include/configs/imx8mq_reform2.h
> new file mode 100644
> index 0000000000..8aed1acfcf
> --- /dev/null
> +++ b/include/configs/imx8mq_reform2.h
...
> +#define CONFIG_EXTRA_ENV_SETTINGS		\
> +	BOOTENV \
> +	"scriptaddr=" __stringify(CONFIG_SYS_LOAD_ADDR) "\0" \
> +	"kernel_addr_r=" __stringify(CONFIG_SYS_LOAD_ADDR) "\0" \
> +	"image=Image\0" \
> +	"console=ttymxc0,115200\0" \
> +	"fdt_addr_r=0x43000000\0"			\

This should have ramdisk_addr_r, maybe:

  "ramdisk_addr_r=0x44000000\0"

Maybe 0x4400000 is a bit overkill, but anything that that starts
sufficiently after fdt_addr_r to leave room for the .dtb file.

This is needed to support distro_bootcmd functionality (boot scripts,
extlinux.conf support, etc.) where an initrd/initramfs should be an
available option.


> +	"boot_fdt=try\0" \
> +	"fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \

This should be something like:

  "fdtfile=freescale/" CONFIG_DEFAULT_FDT_FILE "\0" \

Or maybe CONFIG_DEFAULT_FDT_FILE should have the vendor directory
prepended in configs/imx8mq_reform2_defconfig? Not sure which is more
correct, but the boot environment should have the vendor directory
included in fdtfile one way or another.


Thanks again, it is very nice to be able to build my own bootloader. :)


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]