Re: [PATCH v4 1/2] imx: imx6ul: Add support for Kontron Electronics SL/BL i.MX6UL/ULL boards (N63xx/N64xx)

[Tom Rini <trini@konsulko.com>]

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

On Wed, Jul 21, 2021 at 10:03:26AM +0200, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This adds support for i.MX6UL/ULL-based evaluation kits with SoMs by
> Kontron Electronics GmbH.
> 
> Currently there are the following SoM flavors (SoM-Line):
>   * N6310: SOM with i.MX6UL-2, 256MB RAM, 256MB SPI NAND
>   * N6311: SOM with i.MX6UL-2, 512MB RAM, 512MB SPI NAND
>   * N6411: SOM with i.MX6ULL, 512MB RAM, 512MB SPI NAND
> 
> And the according evaluation boards (Board-Line):
>   * N6310-S: Baseboard with SOM N6310, eMMC, display (optional), ...
>   * N6311-S: Baseboard with SOM N6311, eMMC, display (optional), ...
>   * N6411-S: Baseboard with SOM N6411, eMMC, display (optional), ...
> 
> Currently U-Boot describes i.MX6UL and i.MX6ULL through separate config
> options at compile-time. Though the differences are so minor, that for
> the scope of these SoMs we just use a single defconfig that is compatible
> with both SoCs.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Reviewed-by: Stefano Babic <sbabic@denx.de>
[snip]
> +/*
> + * #######################################
> + * ### ENVIRONMENT                     ###
> + * #######################################
> + */
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +	"bootargs_base=" KONTRON_ENV_KERNEL_MTDPARTS "\0" \
> +	"script=boot.scr\0" \
> +	"kernel_addr_r=" KONTRON_ENV_KERNEL_ADDR "\0" \
> +	"fdt_addr_r=" KONTRON_ENV_FDT_ADDR "\0" \
> +	"ramdisk_addr_r=" KONTRON_ENV_RAMDISK_ADDR "\0" \
> +	"pxefile_addr_r=" KONTRON_ENV_PXE_ADDR "\0" \
> +	"scriptaddr=" KONTRON_ENV_PXE_ADDR "\0" \

I assume there's more platforms coming in, and thus the common and mx6
split.  But I still see some problems.

> +	"bootdir=\0" \
> +	"bootdelay=3\0" \
> +	"ipaddr=192.168.1.11\0" \
> +	"serverip=192.168.1.10\0" \
> +	"gatewayip=192.168.1.10\0" \
> +	"netmask=255.255.255.0\0" \

It's really really frowned upon to put the ipaddr/etc in to the
hard-coded default environment.  Is this absolutely necessary?

[snip]
> +#define KONTRON_ENV_KERNEL_MTDPARTS	"mtdparts=spi1.0:128k(spl),832k(u-boot),64k(env);spi4.0:192m(rootfs),-(user)"

This should just be in the device tree (for Linux!) and we should be
parsing that.

> +#define KONTRON_ENV_KERNEL_ADDR		"0x82000000"
> +#define KONTRON_ENV_FDT_ADDR		"0x83000000"

16MB for kernel + BSS is very very small.  You're likely to hit
overlaps.

> +#define KONTRON_ENV_PXE_ADDR		"0x83100000"
> +#define KONTRON_ENV_RAMDISK_ADDR	"0x83200000"

What is the smallest amount of memory you need to work with on these
platforms?  The header I referenced in the other thread spells out a
bunch of safe sizes / offsets to use, but that can be tricky on smaller
memory platforms.

-- 
Tom

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