Hi Eugen,

On 08.04.19 15:12, [email protected] wrote:

Hi Stefan,

Thanks for reviewing,


On 08.04.2019 15:59, Stefan Roese wrote:


On 08.04.19 14:50, [email protected] wrote:
From: Eugen Hristev <[email protected]>

The SAMA5D2 ICP Board features the SAMA5D27 SoC,
together with QSPI Flash, Wilc3000 wireless device and
EtherCat support.

Signed-off-by: Eugen Hristev <[email protected]>
---
   arch/arm/dts/Makefile                 |   3 +
   arch/arm/dts/at91-sama5d2_icp.dts     | 100 +++++++++++++++++
   arch/arm/mach-at91/Kconfig            |  12 +++
   board/atmel/sama5d2_icp/Kconfig       |  15 +++
   board/atmel/sama5d2_icp/MAINTAINERS   |   7 ++
   board/atmel/sama5d2_icp/Makefile      |   7 ++
   board/atmel/sama5d2_icp/sama5d2_icp.c | 197
++++++++++++++++++++++++++++++++++
   configs/sama5d2_icp_mmc_defconfig     |  72 +++++++++++++
   include/configs/sama5d2_icp.h         |  70 ++++++++++++
   9 files changed, 483 insertions(+)
   create mode 100644 arch/arm/dts/at91-sama5d2_icp.dts
   create mode 100644 board/atmel/sama5d2_icp/Kconfig
   create mode 100644 board/atmel/sama5d2_icp/MAINTAINERS
   create mode 100644 board/atmel/sama5d2_icp/Makefile
   create mode 100644 board/atmel/sama5d2_icp/sama5d2_icp.c
   create mode 100644 configs/sama5d2_icp_mmc_defconfig
   create mode 100644 include/configs/sama5d2_icp.h

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 930b7e0..a3bfb11 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -643,6 +643,9 @@ dtb-$(CONFIG_TARGET_SAMA5D2_XPLAINED) += \
   dtb-$(CONFIG_TARGET_SAMA5D27_SOM1_EK) += \
       at91-sama5d27_som1_ek.dtb
+dtb-$(CONFIG_TARGET_SAMA5D2_ICP) += \
+    at91-sama5d2_icp.dtb
+
   dtb-$(CONFIG_TARGET_SAMA5D3XEK) += \
       sama5d31ek.dtb \
       sama5d33ek.dtb \
diff --git a/arch/arm/dts/at91-sama5d2_icp.dts
b/arch/arm/dts/at91-sama5d2_icp.dts
new file mode 100644
index 0000000..8830b05
--- /dev/null
+++ b/arch/arm/dts/at91-sama5d2_icp.dts
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0+ OR X11
+/*
+ * at91-sama5d2_icp.dts - Device Tree file for SAMA5D2 ICP board
+ *            SAMA5D2 Industrial Connectivity Board
+ *
+ *  Copyright (c) 2018, Microchip Technology Inc.
+ *                2018, Eugen Hristev <[email protected]>
+ */
+/dts-v1/;
+#include "sama5d2.dtsi"
+#include "sama5d2-pinfunc.h"
+
+/ {
+    model = "Microchip SAMA5D2 ICP";
+    compatible = "atmel,sama5d2-icp", "atmel,sama5d27",
"atmel,sama5d2", "atmel,sama5";
+
+    aliases {
+        serial0 = &uart0;
+        i2c1    = &i2c1;
+    };
+
+    chosen {
+        u-boot,dm-pre-reloc;

Please move all U-Boot specific DT properties into a separate
foo-u-boot.dtsi file. That makes it easier to sync the dts files
with the Linux versions.

This means that I would have to make a prerequisite patch that modifies
the current "sama5d2.dtsi" file ?

If this file also has some U-Boot properties, then those should be
moved to a separate file as well (please see my answer below).

Since this is the base include which I
am using for this board.

Or you mean some kind of small file just for this board that will only
add pre-reloc properties ?

Yes. Just add the file "at91-sama5d2_icp-u-boot.dtsi" with the U-Boot
specific properties and it will get included automatically by the
build system (no need to manually include it). Take a look at
this file for example:

arch/arm/dts/socfpga_cyclone5_socrates-u-boot.dtsi



+        stdout-path = "serial0:115200n8";
+    };
+
+    ahb {
+
+        sdmmc0: sdio-host@a0000000 {
+            bus-width = <4>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_sdmmc0_default>;
+            status = "okay";
+            u-boot,dm-pre-reloc;

Here as well.

+        };
+
+        apb {
+            uart0: serial@f801c000 { /* mikrobus1 uart */
+                pinctrl-names = "default";
+                pinctrl-0 = <&pinctrl_mikrobus1_uart>;
+                u-boot,dm-pre-reloc;

etc.

+                status = "okay";
+            };
+
+            i2c1: i2c@fc028000 {
+                dmas = <0>, <0>;
+                pinctrl-names = "default";
+                pinctrl-0 = <&pinctrl_i2c1_default>;
+                status = "okay";
+
+                eeprom@50 {
+                    compatible = "atmel,24c32";
+                    reg = <0x50>;
+                    pagesize = <16>;
+                };
+
+                eeprom@52 {
+                    compatible = "atmel,24c32";
+                    reg = <0x52>;
+                    pagesize = <16>;
+                };
+
+                eeprom@53 {
+                    compatible = "atmel,24c32";
+                    reg = <0x53>;
+                    pagesize = <16>;
+                };
+            };
+            pioA: gpio@fc038000 {
+                status = "okay";
+                pinctrl {
+                    pinctrl_i2c1_default: i2c1_default {
+                        pinmux = <PIN_PD19__TWD1>,
+                             <PIN_PD20__TWCK1>;
+                        bias-disable;
+                    };
+
+                    pinctrl_sdmmc0_default: sdmmc0_default {
+                        pinmux = <PIN_PA1__SDMMC0_CMD>,
+                             <PIN_PA2__SDMMC0_DAT0>,
+                             <PIN_PA3__SDMMC0_DAT1>,
+                             <PIN_PA4__SDMMC0_DAT2>,
+                             <PIN_PA5__SDMMC0_DAT3>,
+                             <PIN_PA0__SDMMC0_CK>,
+                             <PIN_PA13__SDMMC0_CD>;
+                        bias-disable;
+                        u-boot,dm-pre-reloc;
+                    };
+
+                    pinctrl_mikrobus1_uart: mikrobus1_uart {
+                        pinmux = <PIN_PB26__URXD0>,
+                             <PIN_PB27__UTXD0>;
+                        bias-disable;
+                        u-boot,dm-pre-reloc;
+                    };
+                };
+            };
+        };
+    };
+};
diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
index a089e94..c3b21b7 100644
--- a/arch/arm/mach-at91/Kconfig
+++ b/arch/arm/mach-at91/Kconfig
@@ -180,6 +180,17 @@ config TARGET_SAMA5D27_SOM1_EK
         processor-based SAMA5D2 MPU with up to 1 Gbit DDR2-SDRAM
         in a single package.
+config TARGET_SAMA5D2_ICP
+    bool "SAMA5D2 Industrial Connectivity Platform (ICP)"
+    select CPU_V7A
+    select SUPPORT_SPL
+    select BOARD_EARLY_INIT_F
+    select BOARD_LATE_INIT
+    help
+      The SAMA5D2 ICP embeds SAMA5D27 rev. C SoC, together with
+      a 64Mbit QSPI flash, 3xMikrobus connectors, 4xUSB ,
+      EtherCat and WILC3000 devices on board.
+
   config TARGET_SAMA5D3_XPLAINED
       bool "SAMA5D3 Xplained board"
       select BOARD_EARLY_INIT_F
@@ -281,6 +292,7 @@ source "board/atmel/at91sam9x5ek/Kconfig"
   source "board/atmel/sama5d2_ptc_ek/Kconfig"
   source "board/atmel/sama5d2_xplained/Kconfig"
   source "board/atmel/sama5d27_som1_ek/Kconfig"
+source "board/atmel/sama5d2_icp/Kconfig"
   source "board/atmel/sama5d3_xplained/Kconfig"
   source "board/atmel/sama5d3xek/Kconfig"
   source "board/atmel/sama5d4_xplained/Kconfig"
diff --git a/board/atmel/sama5d2_icp/Kconfig
b/board/atmel/sama5d2_icp/Kconfig
new file mode 100644
index 0000000..3859845
--- /dev/null
+++ b/board/atmel/sama5d2_icp/Kconfig
@@ -0,0 +1,15 @@
+if TARGET_SAMA5D2_ICP
+
+config SYS_BOARD
+    default "sama5d2_icp"
+
+config SYS_VENDOR
+    default "atmel"
+
+config SYS_SOC
+    default "at91"
+
+config SYS_CONFIG_NAME
+    default "sama5d2_icp"
+
+endif
diff --git a/board/atmel/sama5d2_icp/MAINTAINERS
b/board/atmel/sama5d2_icp/MAINTAINERS
new file mode 100644
index 0000000..db984b6
--- /dev/null
+++ b/board/atmel/sama5d2_icp/MAINTAINERS
@@ -0,0 +1,7 @@
+SAMA5D2 ICP BOARD
+M:     Eugen Hristev <[email protected]>
+S:     Maintained
+F:     board/atmel/sama5d2_icp/
+F:     include/configs/sama5d2_icp.h
+F:     configs/sama5d2_icp_mmc_defconfig
+
diff --git a/board/atmel/sama5d2_icp/Makefile
b/board/atmel/sama5d2_icp/Makefile
new file mode 100644
index 0000000..fd7e870
--- /dev/null
+++ b/board/atmel/sama5d2_icp/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier:     GPL-2.0+
+#
+# Copyright (C) 2018 Microchip Technology Inc.
+#                   Eugen Hristev <[email protected]>
+#
+
+obj-y += sama5d2_icp.o
diff --git a/board/atmel/sama5d2_icp/sama5d2_icp.c
b/board/atmel/sama5d2_icp/sama5d2_icp.c
new file mode 100644
index 0000000..667d14c
--- /dev/null
+++ b/board/atmel/sama5d2_icp/sama5d2_icp.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Microchip Technology, Inc.
+ *              Eugen Hristev <[email protected]>
+ */
+
+#include <common.h>
+#include <debug_uart.h>
+#include <asm/io.h>
+#include <asm/arch/at91_common.h>
+#include <asm/arch/atmel_pio4.h>
+#include <asm/arch/atmel_mpddrc.h>
+#include <asm/arch/atmel_sdhci.h>
+#include <asm/arch/clk.h>
+#include <asm/arch/gpio.h>
+#include <asm/arch/sama5d2.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+    return 0;
+}
+#endif

You have selected BOARD_LATE_INIT for this board. So no need for
these ifdef's here.

True, but if LATE_INIT is removed, this can stop compiling correctly.

Then don't remove it. ;)

I would guess that we have tons of board ports that won't compile
if the wrong defines are set or unset. My preference is to make the
code clean and add only the absolute minimum of #ifdef's.

An exception would be, if the code needs to support multiple board
variants, one with the option enabled and one with the option
disabled. Then such an #ifdef might make sense. But in the above
case, I think it should be removed.
+
+#ifdef CONFIG_DEBUG_UART_BOARD_INIT
+static void board_uart0_hw_init(void)
+{
+    atmel_pio4_set_c_periph(AT91_PIO_PORTB, 26, ATMEL_PIO_PUEN_MASK);
/* URXD0 */
+    atmel_pio4_set_c_periph(AT91_PIO_PORTB, 27, 0);    /* UTXD0 */
+
+    at91_periph_clk_enable(ATMEL_ID_UART0);
+}
+
+void board_debug_uart_init(void)
+{
+    board_uart0_hw_init();
+}
+#endif
+
+#ifdef CONFIG_BOARD_EARLY_INIT_F
+int board_early_init_f(void)
+{
+#ifdef CONFIG_DEBUG_UART
+    debug_uart_init();
+#endif
+    return 0;
+}
+#endif

Same for BOARD_EARLY_INIT_F.

BTW: Do you really need to enable DEBUG_UART for all the at91
platforms in the defconfig?

These are evaluation kits which people use for various testing, etc.
Just showing <debug uart> before the U-boot banner (relocation) is very
useful for people having issues with the DM Uart (this early one will
work with directly setting the pinout and clk ..., no DTB extract yet)
-> it means u-boot starts

I see. With this explanation this makes perfect sense. Thanks.

+
+int board_init(void)
+{
+    /* address of boot parameters */
+    gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
+
+    return 0;
+}
+
+int dram_init(void)
+{
+    gd->ram_size = get_ram_size((void *)CONFIG_SYS_SDRAM_BASE,
+                    CONFIG_SYS_SDRAM_SIZE);
+    return 0;
+}
+
+#define MAC24AA_MAC_OFFSET    0xfa
+
+#ifdef CONFIG_MISC_INIT_R
+int misc_init_r(void)
+{
+#ifdef CONFIG_I2C_EEPROM
+    at91_set_ethaddr(MAC24AA_MAC_OFFSET);
+#endif
+    return 0;
+}
+#endif

I would also remove these #ifdef's above to make the code less
ugly.

It would break the build if the board removes the I2C support... you
still think it's better to not use the ifdefs ?

Again, please read my thinking about these #ifdef's above. I would
vote to remove them if there is no board configuration that needs
to run without those defines.

Thanks,
Stefan
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to