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

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@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@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@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@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@denx.de
• • Copyright (C) 2020 Nico Becker ic-automation GmbH n.becker@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__ */