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

[Frieder Schrempf]

Hi Tom,

On 21.07.21 15:33, Tom Rini wrote:
> On Wed, Jul 21, 2021 at 10:03:26AM +0200, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf at 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 at kontron.de>
>> Reviewed-by: Stefano Babic <sbabic at 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.

Yes, probably. But I admit that the use of the shared header file is rather limited. I'm thinking of getting rid of it in the next iteration of this patch.

> 
>> +	"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?

Indeed! No it's not necessary and it's rather a historical relict. I will remove all those variables.

> [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.

Agree, I will also remove this.

> 
>> +#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.

As this board has a minimum of 256 MB DDR, I think I will just use the layout from the reference you provided.

Thanks for reviewing!
Frieder