[RESEND PATCH v5 9/11] ARM: socfpga: Add initial support for ic-automation Moritz III

[Marek Vasut]

On 9/11/20 9:02 AM, Nico Becker wrote:
> Add initial  support for ic-automation Moritz III, which is
> an Cyclone V SOM with ethernet, usb, uart. Booting via
> sd-card or flash is supported.
> 
> Changes for v5:

The changelog shouldn't be part of the commmit message, so it goes below
the "---" line. Git then ignores it.

>         - fixed random ethaddr at failure
>         - fixed comments
> 
> Changes for v4:
>         - re-sort list alphabetically
>         - c style comments
> 
> Changes for v3:
>          - Resend via git send-email
> 
> Changes for v2:
>         - Coding Style cleanup
> 
> Signed-off-by: Nico Becker <n.becker at ic-automation.de>
> ---

Here ...

[...]

> +++ b/arch/arm/dts/socfpga_cyclone5_ica_moritz_iii-u-boot.dtsi
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * U-Boot additions
> + *
> + * Copyright (C) 2012 Altera Corporation <www.altera.com>
> + * Copyright (c) 2018 Simon Goldschmidt

I think the copyright assignment needs updating.

> + */
> +
> +#include "socfpga-common-u-boot.dtsi"
> +
> +/{
> +	aliases {
> +		spi0 = "/soc/spi at ff705000";

spi0 = &qspi; should work too ?

[...]

> diff --git a/arch/arm/dts/socfpga_cyclone5_ica_moritz_iii.dts b/arch/arm/dts/socfpga_cyclone5_ica_moritz_iii.dts
> new file mode 100644
> index 0000000000..d81f8ea5bf
> --- /dev/null
> +++ b/arch/arm/dts/socfpga_cyclone5_ica_moritz_iii.dts
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2012 Altera Corporation <www.altera.com>
> + * Copyright (C) 2020 Nico Becker ic-automation GmbH <n.becker at ic-automation.de>
> + */
> +
> +#include "socfpga_cyclone5.dtsi"
> +
> +/ {
> +	model = "ic-automation Moritz III";
> +	compatible = "ic-automation,moritz_iii", "altr,socfpga-cyclone5", "altr,socfpga";
> +
> +	chosen {
> +		bootargs = "earlyprintk";
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	memory at 0 {
> +		name = "memory";
> +		device_type = "memory";
> +		reg = <0x0 0x40000000>; /* 1GB */
> +	};
> +
> +	aliases {
> +		/* this allow the ethaddr uboot environmnet variable contents

environment (typo)

> +		 * to be added to the gmac1 device tree blob.
> +		 */
> +		ethernet0 = &gmac1;
> +	};

[...]

[...]

> +++ b/board/ic-automation/moritz_iii/moritz_iii_board.c

[...]

> +int board_late_init(void)
> +{
> +	u8 mac[MAC_LENGTH];
> +	char serial[SERIALNUMBER_LENGTH + 1];
> +	u8 eeprom_data[OVERALL_LENGTH];
> +	u32 calc_crc32;
> +	u32 *read_crc32;
> +	u8 w_data;
> +	int error;
> +	u8 registers[] = {0x02, 0x03, 0x06, 0x07};
> +
> +	for (int i = 0; i < OVERALL_LENGTH; i++)
> +		eeprom_data[i] = 0x00;

Is that a memset() ?

[...]

> +	/* delete environment var first. Otherwise we are unable to set it's value... */
> +	env_set("ethaddr", "");

ethaddr is not a string, so this will fail. Use NULL instead of "" .
But you might just want to check whether ethaddr is set and skip
overwriting it, that way the user can set their own ethaddr and make it
persistent.

> +	struct udevice *bus;
> +	struct udevice *dev;

Move this to the beginning of the function, just like all the other vars.

[...]

> +	/* check crc32 */
> +	calc_crc32 = crc32(0, eeprom_data, OVERALL_LENGTH - CRC32_LENGTH);
> +	read_crc32 = (u32 *)&eeprom_data[SERIALNUMBER_LENGTH + MAC_LENGTH];

Use get_unaligned on the eeprom data, otherwise this will fail on ARM
hardware with CPSR A-bit clear.

> +	if (*read_crc32 != calc_crc32) {
> +		/* print read data is crc32 not valid */
> +		printf("read data:");
> +		for (int i = 0; i < OVERALL_LENGTH; i++)
> +			printf("%02X", eeprom_data[i]);
> +		printf("\n");
> +		/* print crc32 */
> +		printf("read crc32 %08X calc crc32 %08X\n", *read_crc32, calc_crc32);
> +
> +		strncpy(serial, "00000000", 8);
> +		memset((void *)mac, 0x00, 8);
> +	} else {
> +		/* copy serial */
> +		memcpy((void *)serial, (void *)eeprom_data, SERIALNUMBER_LENGTH);

The typecasts are not needed for memset()/memcpy() .

> +		/* copy MAC address */
> +		memcpy((void *)mac, (void *)&eeprom_data[SERIALNUMBER_LENGTH], MAC_LENGTH);
> +	}
> +
> +	serial[SERIALNUMBER_LENGTH] = 0x00;
> +	printf("Serialnumber = %s\n", serial);


[...]

> +++ b/include/configs/socfpga_ica_moritz_iii.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2014 Marek Vasut <marex at denx.de>
> + * Copyright (C) 2020 Nico Becker ic-automation GmbH <n.becker at ic-automation.de>
> + */
> +
> +#ifndef __CONFIG_SOCFPGA_MORITZ_III_H__
> +#define __CONFIG_SOCFPGA_MORITZ_III_H__
> +
> +#include <asm/arch/base_addr_ac5.h>
> +
> +/* Memory configurations */
> +#define PHYS_SDRAM_1_SIZE					0x40000000	/* 1GiB on SoCDK */
> +
> +/* Booting Linux */
> +#define CONFIG_LOADADDR						0x01000000
> +#define CONFIG_SYS_LOAD_ADDR			CONFIG_LOADADDR
> +
> +/* Boot emv */
> +#define CONFIG_EXTRA_ENV_SETTINGS	\
> +	"fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0"	\
> +	"fpgafile=/lib/firmware/socfpga_sram_bridge.rbf\0"	\
> +	"kernel_addr_r=" __stringify(CONFIG_SYS_LOAD_ADDR)"\0"	\
> +	"bootm_size=0xa000000\0"	\
> +	"fdt_addr_r=0x02000000\0"	\
> +	"ramdisk_addr_r=0x02300000\0"	\
> +	"socfpga_legacy_reset_compat=1\0"	\
> +	"fpgaloadandenable="			\
> +	"fpga load 0 ${loadaddr} ${filesize};"			\
> +	"echo firmware $fpgafile written to fpga;"			\
> +	"bridge enable; echo bridges enabled;\0"	\
> +	"fpgaload=ext4load mmc 0:2 ${loadaddr} ${fpgafile}; run checkfpgafw;\0"	\

use plain load (with CONFIG_FS_GENERIC) and use && as the load command
might fail. You don't want to run the script if it fails. Also use && in
the rest of the scripts where applicable.

> +	"checkfpgafw="			\
> +	"if test ${filesize} -le 0; then "			\
> +		"echo cant load fpga firmare $fpgafile;"	\
> +	"else "			\
> +		"run fpgaloadandenable;"					\
> +	"fi;\0"			\
> +	"bootargs=console=ttyS0,115200 rootfstype=ext4 root=/dev/mmcblk0p2 rw rootwait\0"	\
> +	"bootcmd=run fpgaload; run mmcload;\0"			\
> +	"mmcload=ext4load mmc 0:2 ${kernel_addr_r} boot/zImage;ext4load mmc 0:2 ${fdt_addr_r} boot/${fdtfile};bootz ${kernel_addr_r} - ${fdt_addr_r}\0"
> +
> +/* The rest of the configuration is shared */
> +#include <configs/socfpga_common.h>
> +
> +#endif	/* __CONFIG_SOCFPGA_MORITZ_III_H__ */
>