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

[Michael Walle]

Hi Tom,

thanks for the review!

Am 2020-08-31 18:08, schrieb Tom Rini:
> 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 at walle.cc>

..

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

They are included in the ls1028a_common.h, but I cannot include this 
common
file because its is really just a "common across freescale development
boards" file. Therefore, I had to duplicate it here. Removing these 
includes
results in build errors, eg. DCFG_BASE is undefined etc. I really don't 
want
to go down that rabbit hole and fixing all the layerscape/freescale 
stuff.

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

I don't think so. I'll remove it.

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

thanks for pointing that out.

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

No it's not and it seems to build without the else case. IIRC it wasn't
building a year ago when I did the original port.

I'll post a new version with your suggested changes, soon.

-michael