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

Michael Walle
2020-09-07T12:25:15Z

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