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

[Frieder Schrempf <frieder.schrempf@kontron.de>]

Hi Michael,

On 23.07.21 11:33, Michael Walle wrote:
> Am 2021-07-21 10:03, schrieb Frieder Schrempf:
>> 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>
>> ---
>> Fixes in v4:
>>   * Fix checkpatch errors/warnings
>>   * Stop disabling initrd/fdt relocation
>>
>> Fixes in v3:
>>   * Guard binman nodes to fix build of other UL boards
>>
>> Fixes in v2:
>>   * Add MAINTAINERS file
>>   * Rename board directory
>>   * Use binman to generate FIT
>>   * Support legacy images
>>   * Add Stefano's R-b tag
>> ---
>>  arch/arm/dts/Makefile                         |   4 +-
>>  .../dts/imx6ul-kontron-n631x-s-u-boot.dtsi    |   7 +
>>  arch/arm/dts/imx6ul-kontron-n631x-s.dts       |  17 +
>>  arch/arm/dts/imx6ul-kontron-n631x-som.dtsi    |  14 +
>>  .../dts/imx6ul-kontron-n6x1x-s-u-boot.dtsi    |  98 ++++
>>  arch/arm/dts/imx6ul-kontron-n6x1x-s.dts       | 423 ++++++++++++++++++
>>  arch/arm/dts/imx6ul-kontron-n6x1x-s.dtsi      | 420 +++++++++++++++++
>>  .../dts/imx6ul-kontron-n6x1x-som-common.dtsi  | 124 +++++
>>  .../dts/imx6ull-kontron-n641x-s-u-boot.dtsi   |   7 +
>>  arch/arm/dts/imx6ull-kontron-n641x-s.dts      |  16 +
>>  arch/arm/dts/imx6ull-kontron-n641x-som.dtsi   |  13 +
>>  arch/arm/mach-imx/mx6/Kconfig                 |   9 +
>>  board/kontron/sl-mx6ul/Kconfig                |  15 +
>>  board/kontron/sl-mx6ul/MAINTAINERS            |   9 +
>>  board/kontron/sl-mx6ul/Makefile               |   8 +
>>  board/kontron/sl-mx6ul/kontron_mx6ul.c        |  85 ++++
> 
> Can we name that just mx6ul.c and drop the kontron prefix? Just for
> consitency reasons.

I think it is quite common to name the board C-file according to the name of the board configuration. I don't like mx6ul.c as it is the SoC name, but renaming it to sl-mx6ul.c which refers to the product name of the SoM seems reasonable. And while at it I will also rename the defconfig and header files accordingly.

> 
>>  board/kontron/sl-mx6ul/spl.c                  | 377 ++++++++++++++++
>>  configs/kontron_mx6ul_defconfig               | 109 +++++
>>  include/configs/kontron_common.h              |  84 ++++
>>  include/configs/kontron_mx6ul.h               |  52 +++
> 
> no README in doc/board/kontron/...?

I will need to think about the content of such a file, but in general it is a good idea to add it.

> 
>>  20 files changed, 1890 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n631x-s-u-boot.dtsi
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n631x-s.dts
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n631x-som.dtsi
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n6x1x-s-u-boot.dtsi
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n6x1x-s.dts
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n6x1x-s.dtsi
>>  create mode 100644 arch/arm/dts/imx6ul-kontron-n6x1x-som-common.dtsi
>>  create mode 100644 arch/arm/dts/imx6ull-kontron-n641x-s-u-boot.dtsi
>>  create mode 100644 arch/arm/dts/imx6ull-kontron-n641x-s.dts
>>  create mode 100644 arch/arm/dts/imx6ull-kontron-n641x-som.dtsi
>>  create mode 100644 board/kontron/sl-mx6ul/Kconfig
>>  create mode 100644 board/kontron/sl-mx6ul/MAINTAINERS
>>  create mode 100644 board/kontron/sl-mx6ul/Makefile
>>  create mode 100644 board/kontron/sl-mx6ul/kontron_mx6ul.c
>>  create mode 100644 board/kontron/sl-mx6ul/spl.c
>>  create mode 100644 configs/kontron_mx6ul_defconfig
>>  create mode 100644 include/configs/kontron_common.h
>>  create mode 100644 include/configs/kontron_mx6ul.h
>>
>> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
>> index 91228b7032..b2b0e50dc5 100644
>> --- a/arch/arm/dts/Makefile
>> +++ b/arch/arm/dts/Makefile
>> @@ -811,7 +811,9 @@ dtb-$(CONFIG_MX6UL) += \
>>      imx6ul-liteboard.dtb \
>>      imx6ul-phytec-segin-ff-rdk-nand.dtb \
>>      imx6ul-pico-hobbit.dtb \
>> -    imx6ul-pico-pi.dtb
>> +    imx6ul-pico-pi.dtb \
>> +    imx6ul-kontron-n631x-s.dtb \
>> +    imx6ull-kontron-n641x-s.dtb
>>
>>  dtb-$(CONFIG_MX6ULL) += \
>>      imx6ull-14x14-evk.dtb \
>> diff --git a/arch/arm/dts/imx6ul-kontron-n631x-s-u-boot.dtsi
>> b/arch/arm/dts/imx6ul-kontron-n631x-s-u-boot.dtsi
>> new file mode 100644
>> index 0000000000..d3f013c58c
>> --- /dev/null
>> +++ b/arch/arm/dts/imx6ul-kontron-n631x-s-u-boot.dtsi
>> @@ -0,0 +1,7 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright (C) 2017 exceet electronics GmbH
>> + * Copyright (C) 2018 Kontron Electronics GmbH
> 
> 2021? I don't know how that is handled in u-boot though.

I once learned somewhere that you actually keep the original date in a copyright note as it is intended to state when this was originated and these files have been around downstream as long as 2017/2018. But I'm not a lawyer, so I won't bet on this...


[...]

>> +
>> +int do_board_detect(void)
> static?

Yes

[...]

>> +
>> +/*
>> + * #######################################
>> + * ### 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" \
>> +    "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" \
>> +    "ethact=" CONFIG_ETHPRIME "\0" \
>> +    "hostname=" CONFIG_HOSTNAME "\0" \
>> +    "bootubipart=spi-nand0\0" \
>> +    "bootubivol=boot\0" \
>> +    BOOTENV
>> +
> 
> no fdtfile? or does the fallback apply?

Currently the fdt is only selected through the extlinux.conf. I'll have to look at how fdtfile is supposed to work and how to set it in this case.

> 
> Please not that I just skimmed over the patch, no thorough review.

Thanks, nevertheless!

Frieder