[PATCH v6 1/2] board: kontron: add sl28 support

[Tom Rini <trini@konsulko.com>]

On Sun, Aug 30, 2020 at 10:03:00PM +0200, Michael Walle wrote:

> Add basic support for the Kontron SMARC-sAL28 board. This includes just
> the bare minimum to be able to bring up the board and boot linux.
> 
> For now, the Single and Dual PHY variant is supported. Other variants
> will fall back to the basic variant.
> 
> In particular, there is no watchdog support for now. This means that you
> have to disable the default watchdog, otherwise you'll end up in the
> recovery bootloader. See the board README for details.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
[snip]
> diff --git a/board/kontron/sl28/MAINTAINERS b/board/kontron/sl28/MAINTAINERS
> new file mode 100644
> index 0000000000..047a057646
> --- /dev/null
> +++ b/board/kontron/sl28/MAINTAINERS
> @@ -0,0 +1,6 @@
> +Kontron SMARC-sAL28 board
> +M:	Michael Walle <michael@walle.cc>
> +S:	Maintained
> +F:	board/kontron/sl28/
> +F:	include/configs/kontron_sl28.h
> +F:	configs/kontron_sl28_defconfig

You should list the DTS files here too.

[snip]
> diff --git a/board/kontron/sl28/README b/board/kontron/sl28/README
> new file mode 100644
> index 0000000000..3ddd9aeeb8
> --- /dev/null
> +++ b/board/kontron/sl28/README

This should be rST and in doc/board/ somewhere please.

[snip]
> diff --git a/include/configs/kontron_sl28.h b/include/configs/kontron_sl28.h
> new file mode 100644
> index 0000000000..a5e3219560
> --- /dev/null
> +++ b/include/configs/kontron_sl28.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef __SL28_H
> +#define __SL28_H
> +
> +#include <asm/arch/stream_id_lsch3.h>
> +#include <asm/arch/config.h>
> +#include <asm/arch/soc.h>

Do we need these in here?

[snip]
> +#ifdef CONFIG_SPL_BUILD
> +#define CONFIG_SYS_MONITOR_BASE		CONFIG_SPL_TEXT_BASE
> +#else
> +#define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_TEXT_BASE
> +#endif

I don't like this, but I see it's copied from other layerscapes.  Is it
really needed?

> +
> +#define CONFIG_SYS_LOAD_ADDR		(CONFIG_SYS_DDR_SDRAM_BASE + 0x10000000)
> +
> +/* environment */
> +#define CONFIG_LOADADDR 0x81000000
> +#define ENV_MEM_LAYOUT_SETTINGS \
> +	"scriptaddr=0x90000000\0" \
> +	"pxefile_addr_r=0x90100000\0" \
> +	"kernel_addr_r=" __stringify(CONFIG_LOADADDR) "\0" \
> +	"fdt_addr_r=0x83000000\0" \
> +	"ramdisk_addr_r=0x83100000\0" \
> +	"fdtfile=freescale/fsl-ls1028a-kontron-sl28.dtb\0"

These are too small of a range of kernel/device tree/ramdisk addresses,
and there will be overlap/overwrites at some point.  I've been pointing
people at the big block comment in include/configs/ti_armv7_common.h as
it's still correct for minimum sizes for aarch64 as well.

> +#ifndef CONFIG_SPL_BUILD
> +#define BOOT_TARGET_DEVICES(func) \
> +	func(MMC, mmc, 1) \
> +	func(MMC, mmc, 0) \
> +	func(NVME, nvme, 0) \
> +	func(USB, usb, 0) \
> +	func(DHCP, dhcp, 0) \
> +	func(PXE, pxe, 0)
> +#include <config_distro_bootcmd.h>
> +#else
> +#define BOOTENV
> +#endif

Is env in SPL enabled?  This is another case that I don't really like to
see done.

> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +	"fdt_high=0xffffffffffffffff\0" \
> +	"initrd_high=0xffffffffffffffff\0" \

You cannot disable fdt relocation by default and you likely shouldn't
for initrd.  If we would be placing the device tree outside of visible
to the kernel memory use bootm_size to limit relocation but it's so easy
to construct real life cases where the device tree is not 8 byte aligned
and Linux blows up in non-obvious ways.  Thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200831/186f53ec/attachment.sig>