From mboxrd@z Thu Jan  1 00:00:00 1970
From: Oliver Graute <oliver.graute@gmail.com>
Date: Wed, 4 Nov 2020 16:21:14 +0100
Subject: [PATCH v1] imx: support i.MX8QM DMSSE20 a1 board
In-Reply-To: <20200831161751.GK24856@bill-the-cat>
References: <20200831145441.4836-1-oliver.graute@kococonnector.com>
 <20200831161751.GK24856@bill-the-cat>
Message-ID: <20201104152114.GB23897@optiplex>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 31/08/20, Tom Rini wrote:
> 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.graute@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.
ok will do that

> [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/
ok will do that
> 
> [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?

I got some errors if I remove these two lines. So I keep them.

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

ok will remove that  

> > +#define CONFIG_NR_DRAM_BANKS		4
> > +
> > +/* #define CONFIG_ARCH_MISC_INIT */
> 
> Don't include commented out defines please.

ok will remove that  

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

ok will remove that  

> > +	"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).

not sure If I full understood that. I will remove the fdt_high line.

thx for your feedback.

Best regards,

Oliver