Re: [PATCH] board: ti: Add support for the AM335x GP EVM mini board

... trimming the CC list.

On Tue, Sep 28, 2021 at 11:41:36AM +0200, Amjad Ouled-Ameur wrote:

From: Andreas Dannenberg dannenberg@ti.com

This is not really a new board but rather a minimal bootloader solution for the AM335x GP EVM. In terms of interfaces, it only supports booting from MMC0 or UART0 and only activates a minimal set of drivers that are that are necessary to run the device such as DDR, I2C, and PMIC.

The goal is to provide a bare minimum starting point to boot Linux for basing custom board-ports on. The limited complexity of this solution should make it easier to achieve a successful boot to U-Boot prompt vs. trying to pair down the full-featured multi-platform AM335x U-Boot available through am335x_evm_defconfig.

Signed-off-by: Andreas Dannenberg dannenberg@ti.com [Amjad: fix checkpatch and compile warnings] Signed-off-by: Amjad Ouled-Ameur aouledameur@baylibre.com

There's a few problems here, and I say this as someone who is doing the "mini" version for J721e/etc. First:

diff --git a/arch/arm/dts/am335x-evm-mini.dts b/arch/arm/dts/am335x-evm-mini.dts new file mode 100644 index 000000000000..f45da0fd3f6f --- /dev/null +++ b/arch/arm/dts/am335x-evm-mini.dts

No new dts files. You use the same dts for kernel and u-boot, and have to write one once. And it all Just Works when you want to enable more features on your platform.

diff --git a/arch/arm/mach-omap2/am33xx/Kconfig b/arch/arm/mach-omap2/am33xx/Kconfig index 4268419b166b..296559a00c15 100644 --- a/arch/arm/mach-omap2/am33xx/Kconfig +++ b/arch/arm/mach-omap2/am33xx/Kconfig @@ -63,6 +63,36 @@ config TARGET_AM335X_EVM to write software and develop hardware around an AM335x processor subsystem.

+config TARGET_AM335X_EVM_MINI

• bool "Support am335x_evm_mini"
• select BOARD_LATE_INIT
• select DM
• select DM_GPIO
• select DM_SERIAL
• imply CMD_DM
• imply SPL_DM
• imply SPL_DM_SEQ_ALIAS
• imply SPL_ENV_SUPPORT
• imply SPL_FS_EXT4
• imply SPL_FS_FAT
• imply SPL_GPIO_SUPPORT
• imply SPL_I2C_SUPPORT
• imply SPL_LIBCOMMON_SUPPORT
• imply SPL_LIBDISK_SUPPORT
• imply SPL_LIBGENERIC_SUPPORT
• imply SPL_MMC_SUPPORT
• imply SPL_OF_LIBFDT
• imply SPL_POWER_SUPPORT
• imply SPL_SEPARATE_BSS
• imply SPL_SERIAL_SUPPORT
• imply SPL_SYS_MALLOC_SIMPLE
• imply SPL_YMODEM_SUPPORT
• help

•  This option specifies support for the AM335x
  

•  GP and HS EVM development platforms using a minimal
  

•  system configuration, only supporting a small subset
  

•  of boot media and other features.
  

There should be, maybe, a few selects, for things you simply cannot avoid, and no imply's. The point is something that's clear about what is enabled and why it's enabled. And you've got a bunch of extra stuff enabled there.

diff --git a/board/ti/am335x/board_hs_mini.h b/board/ti/am335x/board_hs_mini.h new file mode 100644 index 000000000000..e03ba141f286 --- /dev/null +++ b/board/ti/am335x/board_hs_mini.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • TI AM437x boards information header
• • Derived from AM335x board.
• • Copyright (C) 2020, Texas Instruments, Incorporated - http://www.ti.com/
• */

+#ifndef _BOARD_HS_H_ +#define _BOARD_HS_H_

+#ifdef CONFIG_TI_SECURE_DEVICE +void board_fit_image_post_process(void **p_image, size_t *p_size) +{

• secure_boot_verify_image(p_image, p_size);

+} +#endif

+#endif

I'd like some of the TI folks to chime in on if we really need to have a "mini" HS variant too.

diff --git a/board/ti/am335x/board_mini.c b/board/ti/am335x/board_mini.c new file mode 100644 index 000000000000..94f0a2910be3 --- /dev/null +++ b/board/ti/am335x/board_mini.c @@ -0,0 +1,249 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Mini board functions for TI AM335X based boards
• • Copyright (C) 2020, Texas Instruments, Incorporated - http://www.ti.com/
• */

+#include <common.h> +#include <dm.h> +#include <env.h> +#include <errno.h> +#include <init.h> +#include <spl.h> +#include <serial.h> +#include <asm/arch/cpu.h> +#include <asm/arch/hardware.h> +#include <asm/arch/omap.h> +#include <asm/arch/ddr_defs.h> +#include <asm/arch/clock.h> +#include <asm/arch/mmc_host_def.h> +#include <asm/arch/sys_proto.h> +#include <asm/arch/mem.h> +#include <asm/io.h> +#include <asm/emif.h> +#include <asm/omap_common.h> +#include <asm/omap_sec_common.h> +#include <asm/omap_mmc.h> +#include <i2c.h> +#include <power/tps65910.h> +#include <env_internal.h> +#include <watchdog.h> +#include "board_mini.h" +#include "board_hs_mini.h"

Check if we really need all of those includes still.

[snip]

+const struct dpll_params *get_dpll_mpu_params(void) +{

• int ind = get_sys_clk_index();
• int freq = am335x_get_efuse_mpu_max_freq(cdev);
• switch (freq) {
• case MPUPLL_M_1000:

• return &dpll_mpu_opp[ind][5];
  

• case MPUPLL_M_800:

• return &dpll_mpu_opp[ind][4];
  

• case MPUPLL_M_720:

• return &dpll_mpu_opp[ind][3];
  

• case MPUPLL_M_600:

• return &dpll_mpu_opp[ind][2];
  

• case MPUPLL_M_500:

• return &dpll_mpu_opp100;
  

• case MPUPLL_M_300:

• return &dpll_mpu_opp[ind][0];
  

• }
• return &dpll_mpu_opp[ind][0];

+}

+void scale_vcores_generic(int freq) +{

• int sil_rev, mpu_vdd;
• /*

• * The GP EVM, IDK and EVM SK use a TPS65910 PMIC.  For all
  

• * MPU frequencies we support we use a CORE voltage of
  

• * 1.10V.  For MPU voltage we need to switch based on
  

• * the frequency we are running at.
  

• */
  

• if (IS_ENABLED(CONFIG_DM_I2C)) {

• if (power_tps65910_init(0))
  

• 	return;
  

• } else {

• if (i2c_probe(TPS65910_CTRL_I2C_ADDR))
  

• 	return;
  

• }
• /*

• * Depending on MPU clock and PG we will need a different
  

• * VDD to drive at that speed.
  

• */
  

• sil_rev = readl(&cdev->deviceid) >> 28;
• mpu_vdd = am335x_get_tps65910_mpu_vdd(sil_rev, freq);
• /* Tell the TPS65910 to use i2c */
• tps65910_set_i2c_control();
• /* First update MPU voltage. */
• if (tps65910_voltage_update(MPU, mpu_vdd))

• return;
  

• /* Second, update the CORE voltage. */
• if (tps65910_voltage_update(CORE, TPS65910_OP_REG_SEL_1_1_0))

• return;
  

+}

Are we supposed to be doing all of the frequency scaling stuff in the "mini" version? And also, just select SPL_DM_I2C so that we can simplify the code.

+void set_uart_mux_conf(void) +{

• if (CONFIG_CONS_INDEX == 1)

• enable_uart0_pin_mux();
  

• else if (CONFIG_CONS_INDEX == 2)

• enable_uart1_pin_mux();
  

• else if (CONFIG_CONS_INDEX == 3)

• enable_uart2_pin_mux();
  

• else if (CONFIG_CONS_INDEX == 4)

• enable_uart3_pin_mux();
  

• else if (CONFIG_CONS_INDEX == 5)

• enable_uart4_pin_mux();
  

• else if (CONFIG_CONS_INDEX == 6)

• enable_uart5_pin_mux();
  

+}

All of this was because of the IDK having a different UART port than the other designs, and we don't need that for "mini".

+int board_late_init(void) +{

• if (IS_ENABLED(CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG)) {

• env_set("board_name", CONFIG_SYS_BOARD);
  

• /*
  

•  * Default FIT boot on HS devices. Non FIT images are not allowed
  

•  * on HS devices.
  

•  */
  

• if (get_device_type() == HS_DEVICE)
  

• 	env_set("boot_fit", "1");
  

• }
• return 0;

+}

Pending the answer about HS devices, this and also select'ing BOARD_LATE_INIT can go.

diff --git a/board/ti/am335x/board_mini.h b/board/ti/am335x/board_mini.h new file mode 100644 index 000000000000..0fa1a8680027 --- /dev/null +++ b/board/ti/am335x/board_mini.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Mini AM335x boards information header
• • Copyright (C) 2020, Texas Instruments, Incorporated - http://www.ti.com/
• */

+#ifndef _BOARD_H_ +#define _BOARD_H_

+/**

• • AM335X (EMIF_4D) EMIF REG_COS_COUNT_1, REG_COS_COUNT_2, and
• • REG_PR_OLD_COUNT values to avoid LCDC DMA FIFO underflows and Frame
• • Synchronization Lost errors. The values are the biggest that work
• • reliably with offered video modes and the memory subsystem on the
• • boards. These register have are briefly documented in "7.3.3.5.2
• • Command Starvation" section of AM335x TRM. The REG_COS_COUNT_1 and
• • REG_COS_COUNT_2 do not have any effect on current versions of
• • AM335x.
• */

+#define EMIF_OCP_CONFIG_BEAGLEBONE_BLACK 0x00141414 +#define EMIF_OCP_CONFIG_AM335X_EVM 0x003d3d3d

We only use the EVM value, so drop the BEAGLEBONE_BLACK one.

diff --git a/board/ti/am335x/mux_mini.c b/board/ti/am335x/mux_mini.c new file mode 100644 index 000000000000..b724198b9cdf --- /dev/null +++ b/board/ti/am335x/mux_mini.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
• */

+#include <common.h> +#include <asm/arch/sys_proto.h> +#include <asm/arch/hardware.h> +#include <asm/arch/mux.h> +#include <asm/io.h> +#include <i2c.h> +#include "board_mini.h"

+static struct module_pin_mux uart0_pin_mux[] = {

• {OFFSET(uart0_rxd), (MODE(0) | PULLUP_EN | RXACTIVE)}, /* UART0_RXD */
• {OFFSET(uart0_txd), (MODE(0) | PULLUDEN)}, /* UART0_TXD */
• {-1},

+};

+static struct module_pin_mux uart1_pin_mux[] = {

• {OFFSET(uart1_rxd), (MODE(0) | PULLUP_EN | RXACTIVE)}, /* UART1_RXD */
• {OFFSET(uart1_txd), (MODE(0) | PULLUDEN)}, /* UART1_TXD */
• {-1},

+};

+static struct module_pin_mux uart2_pin_mux[] = {

• {OFFSET(spi0_sclk), (MODE(1) | PULLUP_EN | RXACTIVE)}, /* UART2_RXD */
• {OFFSET(spi0_d0), (MODE(1) | PULLUDEN)}, /* UART2_TXD */
• {-1},

+};

+static struct module_pin_mux uart3_pin_mux[] = {

• {OFFSET(spi0_cs1), (MODE(1) | PULLUP_EN | RXACTIVE)}, /* UART3_RXD */
• {OFFSET(ecap0_in_pwm0_out), (MODE(1) | PULLUDEN)}, /* UART3_TXD */
• {-1},

+};

+static struct module_pin_mux uart4_pin_mux[] = {

• {OFFSET(gpmc_wait0), (MODE(6) | PULLUP_EN | RXACTIVE)}, /* UART4_RXD */
• {OFFSET(gpmc_wpn), (MODE(6) | PULLUDEN)}, /* UART4_TXD */
• {-1},

+};

+static struct module_pin_mux uart5_pin_mux[] = {

• {OFFSET(lcd_data9), (MODE(4) | PULLUP_EN | RXACTIVE)}, /* UART5_RXD */
• {OFFSET(lcd_data8), (MODE(4) | PULLUDEN)}, /* UART5_TXD */
• {-1},

+};

+static struct module_pin_mux mmc0_pin_mux[] = {

• {OFFSET(mmc0_dat3), (MODE(0) | RXACTIVE | PULLUP_EN)}, /* MMC0_DAT3 */
• {OFFSET(mmc0_dat2), (MODE(0) | RXACTIVE | PULLUP_EN)}, /* MMC0_DAT2 */
• {OFFSET(mmc0_dat1), (MODE(0) | RXACTIVE | PULLUP_EN)}, /* MMC0_DAT1 */
• {OFFSET(mmc0_dat0), (MODE(0) | RXACTIVE | PULLUP_EN)}, /* MMC0_DAT0 */
• {OFFSET(mmc0_clk), (MODE(0) | RXACTIVE | PULLUP_EN)}, /* MMC0_CLK */
• {OFFSET(mmc0_cmd), (MODE(0) | RXACTIVE | PULLUP_EN)}, /* MMC0_CMD */
• {OFFSET(mcasp0_aclkr), (MODE(4) | RXACTIVE)}, /* MMC0_WP */
• {OFFSET(spi0_cs1), (MODE(7) | RXACTIVE | PULLUP_EN)}, /* GPIO0_6 */
• {-1},

+};

+static struct module_pin_mux i2c0_pin_mux[] = {

• {OFFSET(i2c0_sda), (MODE(0) | RXACTIVE |

• 	PULLUDEN | SLEWCTRL)}, /* I2C_DATA */
  

• {OFFSET(i2c0_scl), (MODE(0) | RXACTIVE |

• 	PULLUDEN | SLEWCTRL)}, /* I2C_SCLK */
  

• {-1},

+};

+void enable_uart0_pin_mux(void) +{

• configure_module_pin_mux(uart0_pin_mux);

+}

+void enable_uart1_pin_mux(void) +{

• configure_module_pin_mux(uart1_pin_mux);

+}

+void enable_uart2_pin_mux(void) +{

• configure_module_pin_mux(uart2_pin_mux);

+}

+void enable_uart3_pin_mux(void) +{

• configure_module_pin_mux(uart3_pin_mux);

+}

+void enable_uart4_pin_mux(void) +{

• configure_module_pin_mux(uart4_pin_mux);

+}

+void enable_uart5_pin_mux(void) +{

• configure_module_pin_mux(uart5_pin_mux);

+}

+void enable_i2c0_pin_mux(void) +{

• configure_module_pin_mux(i2c0_pin_mux);

+}

+void enable_board_pin_mux(void) +{

• /* Do board-specific muxes */
• configure_module_pin_mux(mmc0_pin_mux);

+}

We don't need all of the UART muxes.

diff --git a/configs/am335x_evm_mini_defconfig b/configs/am335x_evm_mini_defconfig new file mode 100644 index 000000000000..087d64d742d3 --- /dev/null +++ b/configs/am335x_evm_mini_defconfig @@ -0,0 +1,42 @@ +CONFIG_ARM=y +CONFIG_ARCH_CPU_INIT=y +CONFIG_ARCH_OMAP2PLUS=y +CONFIG_TI_COMMON_CMD_OPTIONS=y +CONFIG_DEFAULT_DEVICE_TREE="am335x-evm-mini" +CONFIG_AM33XX=y +CONFIG_TARGET_AM335X_EVM_MINI=y +CONFIG_SPL=y +CONFIG_DISTRO_DEFAULTS=y +CONFIG_ANDROID_BOOT_IMAGE=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_OF_BOARD_SETUP=y +CONFIG_BOOTCOMMAND="if test ${boot_fit} -eq 1; then run update_to_fit; fi; run findfdt; run init_console; run envboot; run distro_bootcmd" +CONFIG_LOGLEVEL=3 +CONFIG_SYS_CONSOLE_INFO_QUIET=y +CONFIG_ARCH_MISC_INIT=y +CONFIG_SPL_FIT_IMAGE_TINY=y +# CONFIG_SPL_FS_EXT4 is not set +CONFIG_SPL_I2C=y +# CONFIG_SPL_NAND_SUPPORT is not set +CONFIG_SPL_POWER=y +# CONFIG_SPL_AM33XX_ENABLE_RTC32K_OSC is not set +CONFIG_CMD_SPL=y +# CONFIG_CMD_EEPROM is not set +# CONFIG_CMD_SETEXPR is not set +# CONFIG_SPL_EFI_PARTITION is not set +CONFIG_OF_CONTROL=y +CONFIG_SYS_RELOC_GD_ENV_ADDR=y +CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y +CONFIG_SPL_ENV_IS_NOWHERE=y +CONFIG_VERSION_VARIABLE=y +# CONFIG_NET is not set +CONFIG_DM_I2C=y +CONFIG_MISC=y +# CONFIG_MMC_HW_PARTITIONING is not set +CONFIG_MMC_OMAP_HS=y +CONFIG_TIMER=y +CONFIG_OMAP_TIMER=y +CONFIG_DYNAMIC_CRC_TABLE=y +CONFIG_RSA=y +CONFIG_LZO=y +# CONFIG_OF_LIBFDT_OVERLAY is not set

Audit this again please, if we're taking a non-default value for a CONFIG option, it should likely be to disable something, not enable it.

diff --git a/include/configs/am335x_evm_mini.h b/include/configs/am335x_evm_mini.h new file mode 100644 index 000000000000..f4c8dfe78e1a --- /dev/null +++ b/include/configs/am335x_evm_mini.h @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
• */

+#ifndef __CONFIG_AM335X_EVM_MINI_H +#define __CONFIG_AM335X_EVM_MINI_H

+#include <configs/ti_am335x_common.h> +#include <linux/sizes.h>

+#ifndef CONFIG_SPL_BUILD +# define CONFIG_TIMESTAMP +#endif

You cannot guard options with SPL_BUILD.

+#define CONFIG_SYS_BOOTM_LEN SZ_16M

+#define CONFIG_MACH_TYPE MACH_TYPE_AM335XEVM

This isn't going to boot a non-DT kernel.

+/* Clock Defines */ +#define V_OSCK 24000000 /* Clock output from T2 */ +#define V_SCLK (V_OSCK)

+#define BOOTENV_DEV_LEGACY_MMC(devtypeu, devtypel, instance) \

• "bootcmd_" #devtypel #instance "=" \
• "setenv mmcdev " #instance "; "\
• "setenv bootpart " #instance ":2 ; "\
• "run mmcboot\0"

+#define BOOTENV_DEV_NAME_LEGACY_MMC(devtypeu, devtypel, instance) \

• #devtypel #instance " "

+#define BOOT_TARGET_DEVICES(func) \

• func(MMC, mmc, 0) \
• func(LEGACY_MMC, legacy_mmc, 0)

+#include <config_distro_bootcmd.h>

No legacy boot, distro only. Which means..

+#ifndef CONFIG_SPL_BUILD +#include <environment/ti/mmc.h>

+#define CONFIG_EXTRA_ENV_SETTINGS \

• DEFAULT_LINUX_BOOT_ENV \
• DEFAULT_MMC_TI_ARGS \
• DEFAULT_FIT_TI_ARGS \
• "bootpart=0:2\0" \
• "bootdir=/boot\0" \
• "bootfile=zImage\0" \
• "fdtfile=undefined\0" \
• "console=ttyS0,115200n8\0" \
• "partitions=" \

• "uuid_disk=${uuid_gpt_disk};" \
  

• "name=bootloader,start=384K,size=1792K," \
  

• 	"uuid=${uuid_gpt_bootloader};" \
  

• "name=rootfs,start=2688K,size=-,uuid=${uuid_gpt_rootfs}\0" \
  

• "optargs=\0" \
• "ramroot=/dev/ram0 rw\0" \
• "ramrootfstype=ext2\0" \
• "ramargs=setenv bootargs console=${console} " \

• "${optargs} " \
  

• "root=${ramroot} " \
  

• "rootfstype=${ramrootfstype}\0" \
  

• "loadramdisk=load mmc ${mmcdev} ${rdaddr} ramdisk.gz\0" \
• "ramboot=echo Booting from ramdisk ...; " \

• "run ramargs; " \
  

• "bootz ${loadaddr} ${rdaddr} ${fdtaddr}\0" \
  

• "default_device_tree=am335x-evm.dtb\0" \
• "findfdt=" \

• "setenv name_fdt ${default_device_tree};" \
  

• "setenv fdtfile ${name_fdt}\0" \
  

• "init_console=" \

• "setenv console ttyS0,115200n8\0" \
  

• BOOTENV

Clean this up.

+#endif

+/* NS16550 Configuration */ +#define CONFIG_SYS_NS16550_COM1 0x44e09000 /* Base EVM has UART0 */ +#define CONFIG_SYS_NS16550_COM2 0x48022000 /* UART1 */ +#define CONFIG_SYS_NS16550_COM3 0x48024000 /* UART2 */ +#define CONFIG_SYS_NS16550_COM4 0x481a6000 /* UART3 */ +#define CONFIG_SYS_NS16550_COM5 0x481a8000 /* UART4 */ +#define CONFIG_SYS_NS16550_COM6 0x481aa000 /* UART5 */

+/* PMIC support */ +#define CONFIG_POWER_TPS65910

+/*

• • Disable MMC DM for SPL build and can be re-enabled after adding
• • DM support in SPL
• */

+#ifdef CONFIG_SPL_BUILD +#undef CONFIG_DM_MMC +#undef CONFIG_TIMER +#endif

You can't do build time guards like that. And I'm sure most of these kind of comments apply to the am43xx_evm version too. Thanks.