[U-Boot] [PATCH] board: atmel: add SAMA5D2 ICP board
Stefan Roese
sr at denx.de
Mon Apr 8 13:46:53 UTC 2019
Hi Eugen,
On 08.04.19 15:12, Eugen.Hristev at microchip.com wrote:
>
> Hi Stefan,
>
> Thanks for reviewing,
>
>
> On 08.04.2019 15:59, Stefan Roese wrote:
>
>>
>> On 08.04.19 14:50, Eugen.Hristev at microchip.com wrote:
>>> From: Eugen Hristev <eugen.hristev at microchip.com>
>>>
>>> The SAMA5D2 ICP Board features the SAMA5D27 SoC,
>>> together with QSPI Flash, Wilc3000 wireless device and
>>> EtherCat support.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev at microchip.com>
>>> ---
>>> 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 <eugen.hristev at microchip.com>
>>> + */
>>> +/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 at 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 at f801c000 { /* mikrobus1 uart */
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pinctrl_mikrobus1_uart>;
>>> + u-boot,dm-pre-reloc;
>>
>> etc.
>>
>>> + status = "okay";
>>> + };
>>> +
>>> + i2c1: i2c at fc028000 {
>>> + dmas = <0>, <0>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pinctrl_i2c1_default>;
>>> + status = "okay";
>>> +
>>> + eeprom at 50 {
>>> + compatible = "atmel,24c32";
>>> + reg = <0x50>;
>>> + pagesize = <16>;
>>> + };
>>> +
>>> + eeprom at 52 {
>>> + compatible = "atmel,24c32";
>>> + reg = <0x52>;
>>> + pagesize = <16>;
>>> + };
>>> +
>>> + eeprom at 53 {
>>> + compatible = "atmel,24c32";
>>> + reg = <0x53>;
>>> + pagesize = <16>;
>>> + };
>>> + };
>>> + pioA: gpio at 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 <eugen.hristev at microchip.com>
>>> +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 <eugen.hristev at microchip.com>
>>> +#
>>> +
>>> +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 <eugen.hristev at microchip.com>
>>> + */
>>> +
>>> +#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
More information about the U-Boot
mailing list