On Mon, Aug 31, 2020 at 04:54:40PM +0200, Oliver Graute wrote:

> Add i.MX8QM DMSSE20 a1 board support
[snip]
> diff --git a/board/advantech/imx8qm_dmsse20_a1/MAINTAINERS 
> b/board/advantech/imx8qm_dmsse20_a1/MAINTAINERS
> new file mode 100644
> index 0000000000..c1e8048bbb
> --- /dev/null
> +++ b/board/advantech/imx8qm_dmsse20_a1/MAINTAINERS
> @@ -0,0 +1,6 @@
> +i.MX8QM ROM DMSSE20 a1 BOARD
> +M:   Oliver Graute <oliver.gra...@kococonnector.com>
> +S:   Maintained
> +F:   board/advantech/imx8qm_dmsse20_a1/
> +F:   include/configs/imx8qm_dmsse20.h
> +F:   configs/imx8qm_dmsse20a1_defconfig

You should also list the dts files.

[snip]
> diff --git a/board/advantech/imx8qm_dmsse20_a1/README 
> b/board/advantech/imx8qm_dmsse20_a1/README
> new file mode 100644
> index 0000000000..2105e9c425
> --- /dev/null
> +++ b/board/advantech/imx8qm_dmsse20_a1/README

This should be rST and in doc/board/

[snip]
> diff --git a/include/configs/imx8qm_dmsse20.h 
> b/include/configs/imx8qm_dmsse20.h
> new file mode 100644
> index 0000000000..e1e0bc6e38
> --- /dev/null
> +++ b/include/configs/imx8qm_dmsse20.h
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier:  GPL-2.0+
> +/*
> + * Copyright 2017-2019 NXP
> + */
> +
> +#ifndef __IMX8QM_DMSSE20_H
> +#define __IMX8QM_DMSSE20_H
> +
> +#include <linux/sizes.h>
> +#include <asm/arch/imx-regs.h>

Do we really need these includes here?

> +#define CONFIG_REMAKE_ELF
> +
> +#ifdef CONFIG_SPL_BUILD
> +#define CONFIG_SPL_MAX_SIZE          (124 * 1024)
> +#define CONFIG_SPL_BSS_START_ADDR    0x00128000
> +#define CONFIG_SPL_BSS_MAX_SIZE      0x1000  /* 4 KB */
> +#endif

These guards are unnecessary.

> +#define CONFIG_NR_DRAM_BANKS         4
> +
> +/* #define CONFIG_ARCH_MISC_INIT */

Don't include commented out defines please.

> +
> +/* #define CONFIG_CMD_READ */
> +
> +/* Flat Device Tree Definitions */
> +#define CONFIG_OF_BOARD_SETUP
> +
> +/* #undef CONFIG_CMD_EXPORTENV */
> +/* #undef CONFIG_CMD_IMPORTENV */
> +/* #undef CONFIG_CMD_IMLS */
> +
> +/* #undef CONFIG_CMD_CRC32 */
> +#undef CONFIG_BOOTM_NETBSD

You cannot enable/disable commands in the header file, that's an error
to checkpatch as they've all been in Kconfig for some time.
CONFIG_BOOTM_xxx has been for a bit as well.  Please audit the whole
file for things which are already in Kconfig, thanks.

> +#define CONFIG_MFG_ENV_SETTINGS \
> +     "mfgtool_args=setenv bootargs console=${console},${baudrate} " \
> +     "initrd_addr=0x83100000\0" \
> +     "initrd_high=0xffffffffffffffff\0" \

It's not a good idea, but not fatal to disable initrd relocation.

> +     "emmc_dev=0\0" \
> +     "sd_dev=1\0" \
> +
> +/* Initial environment variables */
> +#define CONFIG_EXTRA_ENV_SETTINGS            \
> +     CONFIG_MFG_ENV_SETTINGS \
> +     M4_BOOT_ENV \
> +     "script=boot.scr\0" \
> +     "image=Image\0" \
> +     "panel=NULL\0" \
> +     "console=ttyLP0\0" \
> +     "earlycon=lpuart32,0x5a060000\0" \
> +     "fdt_addr=0x83000000\0"                 \
> +     "fdt_high=0xffffffffffffffff\0"         \
> +

It is however fatal to disable fdt relocation.  It's far too easy for
real life examples to generated non-8-byte-aligned addresses for the dtb
and the Linux blows up.  Please also see
include/configs/ti_armv7_common.h and the big block comment about where
to place and how much space to have between the kernel, initrd and
device tree.  While that's for a 32bit platform, it's all applicable to
aarch64 (perhaps with the caveat that dtb can be up to 2MiB? off the top
of my head).

-- 
Tom

Attachment: signature.asc
Description: PGP signature

Reply via email to