[PATCH 00/10] board/ti: k3 boards: Stop using findfdt

[PATCH 00/10] board/ti: k3 boards: Stop using findfdt

[Nishanth Menon]

This is a wide cleanup to switch to setting fdtfile using env_set
instead of scripted magic. 'fdtfile' is expected to be set by default.
This allows the stdboot triggered efi loaders to find the correct OS
device tree file.

This is a refresh of
https://lore.kernel.org/all/86le9dwz4d.fsf@udb0321960.dhcp.ti.com/ which
was the wrong approach.

Bootlogs: https://gist.github.com/nmenon/2f4a142c1bcaa09d544b1f2e206ea134

NOTE: There are a couple of checkpatch WARN (around LATE_INIT) and
CHECK (fdt_ops #ifdeffery) that on closer inspection looks fine and
consistent with other similar usage.

Based on next branch at: c2c598e87cfe Merge branch 'staging' of https://source.denx.de/u-boot/custodians/u-boot-tegra into next

Nishanth Menon (10):
  board: ti: common: Introduce a common fdt ops library
  board: ti: am62ax: Set fdtfile from C code instead of findfdt script
  board: ti: am62x: Set fdtfile from C code instead of findfdt script
  board: ti: am64x: Set fdtfile from C code instead of findfdt script
  board: ti: am65x: Set fdtfile from C code instead of findfdt script
  board: ti: j721e: Set fdtfile from C code instead of findfdt script
  board: ti: j721s2: Set fdtfile from C code instead of findfdt script
  board: beagle: beagleboneai64: Set fdtfile from C code instead of
    findfdt script
  board: beagle: beagleplay: Set fdtfile from C code instead of findfdt
    script
  include: env: ti: Drop default_findfdt

 board/beagle/beagleboneai64/beagleboneai64.c  | 14 ++++
 .../beagle/beagleboneai64/beagleboneai64.env  |  1 -
 board/beagle/beagleplay/beagleplay.c          | 14 ++++
 board/beagle/beagleplay/beagleplay.env        |  1 -
 board/ti/am62ax/am62ax.env                    |  1 -
 board/ti/am62ax/evm.c                         | 10 +++
 board/ti/am62x/am62x.env                      |  1 -
 board/ti/am62x/evm.c                          |  8 +++
 board/ti/am64x/am64x.env                      |  9 ---
 board/ti/am64x/evm.c                          |  8 +++
 board/ti/am65x/am65x.env                      |  3 -
 board/ti/am65x/evm.c                          |  2 +
 board/ti/common/Kconfig                       | 12 ++++
 board/ti/common/Makefile                      |  1 +
 board/ti/common/fdt_ops.c                     | 65 +++++++++++++++++++
 board/ti/common/fdt_ops.h                     | 41 ++++++++++++
 board/ti/j721e/evm.c                          |  8 +++
 board/ti/j721e/j721e.env                      | 10 ---
 board/ti/j721s2/evm.c                         |  8 +++
 board/ti/j721s2/j721s2.env                    |  8 ---
 configs/am62ax_evm_a53_defconfig              |  1 +
 configs/am62x_beagleplay_a53_defconfig        |  3 +-
 configs/am62x_evm_a53_defconfig               |  1 +
 configs/j721e_beagleboneai64_a72_defconfig    |  3 +-
 include/env/ti/default_findfdt.env            | 12 ----
 25 files changed, 197 insertions(+), 48 deletions(-)
 create mode 100644 board/ti/common/fdt_ops.c
 create mode 100644 board/ti/common/fdt_ops.h
 delete mode 100644 include/env/ti/default_findfdt.env

-- 
2.43.0

[PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Nishanth Menon]

Introduce a common fdt operations library for basic device tree
operations that are common between various boards.

The first library to introduce here is the capability to set up
fdtfile as a standard variable as part of board identification rather
than depend on scripted ifdeffery.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/common/Kconfig   | 12 ++++++++
 board/ti/common/Makefile  |  1 +
 board/ti/common/fdt_ops.c | 65 +++++++++++++++++++++++++++++++++++++++
 board/ti/common/fdt_ops.h | 41 ++++++++++++++++++++++++
 4 files changed, 119 insertions(+)
 create mode 100644 board/ti/common/fdt_ops.c
 create mode 100644 board/ti/common/fdt_ops.h

diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
index 49edd98014ab..06a8a36aa1cd 100644
--- a/board/ti/common/Kconfig
+++ b/board/ti/common/Kconfig
@@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
 	imply CMD_SPI
 	imply CMD_TIME
 	imply CMD_USB if USB
+
+config TI_EVM_FDT_FOLDER_PATH
+	string "Location of Folder path where dtb is present"
+	default "ti/davinci" if ARCH_DAVINCI
+	default "ti/keystone" if ARCH_KEYSTONE
+	default "ti/omap" if ARCH_OMAP2PLUS
+	default "ti" if ARCH_K3
+	depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
+	help
+	   Folder path for kernel device tree default.
+	   This is used along with fdtfile path to locate the kernel
+	   device tree blob.
diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
index 26bf12e2e6d5..5ac361ba7fcf 100644
--- a/board/ti/common/Makefile
+++ b/board/ti/common/Makefile
@@ -3,3 +3,4 @@
 
 obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
 obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
+obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
new file mode 100644
index 000000000000..f8770cae4a54
--- /dev/null
+++ b/board/ti/common/fdt_ops.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Library to support FDT file operations which are common
+ *
+ * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#include <env.h>
+#include <vsprintf.h>
+#include "fdt_ops.h"
+
+void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)
+{
+	char *fdt_file_name = NULL;
+	char fdtfile[TI_FDT_FILE_MAX];
+
+	if (name_fdt) {
+		while (fdt_map) {
+			/* Check for NULL terminator in the list */
+			if (!fdt_map->name_fdt)
+				break;
+			if (!strncmp(fdt_map->name_fdt, name_fdt, TI_NAME_FDT_MAX)) {
+				fdt_file_name = fdt_map->fdt_file_name;
+				break;
+			}
+			fdt_map++;
+		}
+	}
+
+	/* match not found OR null name_fdt */
+	if (!fdt_file_name) {
+		/*
+		 * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
+		 * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
+		 */
+#ifdef CONFIG_DEFAULT_FDT_FILE
+		if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
+			snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
+				 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_FDT_FILE);
+		} else
+#endif
+		{
+			snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
+				 CONFIG_TI_EVM_FDT_FOLDER_PATH,
+				 CONFIG_DEFAULT_DEVICE_TREE);
+		}
+	} else {
+		snprintf(fdtfile, sizeof(fdtfile), "%s/%s", CONFIG_TI_EVM_FDT_FOLDER_PATH,
+			 fdt_file_name);
+	}
+
+	env_set("fdtfile", fdtfile);
+
+	/*
+	 * XXX: DEPRECATION WARNING: 2 u-boot versions.
+	 *
+	 * Maintain compatibility with downstream scripts that may be using
+	 * name_fdt
+	 */
+	if (name_fdt)
+		env_set("name_fdt", name_fdt);
+	/* Also set the findfdt legacy script to warn users to stop using this */
+	env_set("findfdt",
+		"echo WARN: fdtfile already set. Stop using findfdt in script");
+}
diff --git a/board/ti/common/fdt_ops.h b/board/ti/common/fdt_ops.h
new file mode 100644
index 000000000000..c01697bed28f
--- /dev/null
+++ b/board/ti/common/fdt_ops.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Library to support common device tree manipulation for TI EVMs
+ *
+ * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com
+ */
+
+#ifndef __FDT_OPS_H
+#define __FDT_OPS_H
+
+#define TI_NAME_FDT_MAX 20
+#define TI_FDT_FILE_MAX 200
+
+/**
+ *  struct ti_fdt_map - mapping of device tree blob name to board name
+ *  @name_fdt: board_name up to TI_NAME_FDT_MAX long
+ *  @fdt_file_name: device tree blob name as described by kernel
+ */
+struct ti_fdt_map {
+	const char *name_fdt;
+	char *fdt_file_name;
+};
+
+/**
+ * ti_set_fdt_env  - Find the correct device tree file name and set 'fdtfile'
+ * env variable with correct folder structure appropriate to the architecture
+ * and kernel conventions. This function is invoked typically as part of
+ * board_late_init
+ *
+ * fdt name is picked by:
+ * a) If a match is found, use the match
+ * b) If not, CONFIG_DEFAULT_FDT_FILE (Boot OS device tree) if that is defined
+ *    and not null
+ * c) If not, Use CONFIG_DEFAULT_DEVICE_TREE (DT control for bootloader)
+ *
+ * @name_fdt: match to search with (max of TI_NAME_FDT_MAX chars)
+ * @fdt_map: NULL terminated array of device tree file name matches.
+ */
+void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map);
+
+#endif /* __FDT_OPS_H */
-- 
2.43.0

[PATCH 02/10] board: ti: am62ax: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

Stop using the findfdt script and switch to setting the fdtfile from
C code.

While at this, replace findfdt in environment with a warning as it is
no longer needed

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/am62ax/am62ax.env       |  1 -
 board/ti/am62ax/evm.c            | 10 ++++++++++
 configs/am62ax_evm_a53_defconfig |  1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/board/ti/am62ax/am62ax.env b/board/ti/am62ax/am62ax.env
index a6d967e982d4..334374abb73e 100644
--- a/board/ti/am62ax/am62ax.env
+++ b/board/ti/am62ax/am62ax.env
@@ -1,5 +1,4 @@
 #include <env/ti/ti_common.env>
-#include <env/ti/default_findfdt.env>
 #include <env/ti/mmc.env>
 
 name_kern=Image
diff --git a/board/ti/am62ax/evm.c b/board/ti/am62ax/evm.c
index cd3360a43029..62d3664936e7 100644
--- a/board/ti/am62ax/evm.c
+++ b/board/ti/am62ax/evm.c
@@ -13,6 +13,8 @@
 #include <fdt_support.h>
 #include <spl.h>
 
+#include "../common/fdt_ops.h"
+
 int board_init(void)
 {
 	return 0;
@@ -27,3 +29,11 @@ int dram_init_banksize(void)
 {
 	return fdtdec_setup_memory_banksize();
 }
+
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+	ti_set_fdt_env(NULL, NULL);
+	return 0;
+}
+#endif
diff --git a/configs/am62ax_evm_a53_defconfig b/configs/am62ax_evm_a53_defconfig
index 38083586a3ec..e5fcd8cc5b6f 100644
--- a/configs/am62ax_evm_a53_defconfig
+++ b/configs/am62ax_evm_a53_defconfig
@@ -24,6 +24,7 @@ CONFIG_SPL_LOAD_FIT_ADDRESS=0x81000000
 CONFIG_BOOTSTD_FULL=y
 CONFIG_BOOTSTD_DEFAULTS=y
 CONFIG_BOOTCOMMAND="run envboot; bootflow scan -lb"
+CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_MAX_SIZE=0x58000
 CONFIG_SPL_PAD_TO=0x0
 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
-- 
2.43.0

[PATCH 03/10] board: ti: am62x: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

Stop using the findfdt script and switch to setting the fdtfile from
C code.

While at this, replace findfdt in environment with a warning as it is
no longer needed

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/am62x/am62x.env        | 1 -
 board/ti/am62x/evm.c            | 8 ++++++++
 configs/am62x_evm_a53_defconfig | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/board/ti/am62x/am62x.env b/board/ti/am62x/am62x.env
index e53a55c38fbb..9cb186c2a03c 100644
--- a/board/ti/am62x/am62x.env
+++ b/board/ti/am62x/am62x.env
@@ -1,5 +1,4 @@
 #include <env/ti/ti_common.env>
-#include <env/ti/default_findfdt.env>
 #include <env/ti/mmc.env>
 
 name_kern=Image
diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
index ad939088402e..b76ef1b94a61 100644
--- a/board/ti/am62x/evm.c
+++ b/board/ti/am62x/evm.c
@@ -54,6 +54,14 @@ int dram_init(void)
 	return fdtdec_setup_mem_size_base();
 }
 
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+	ti_set_fdt_env(NULL, NULL);
+	return 0;
+}
+#endif
+
 int dram_init_banksize(void)
 {
 	return fdtdec_setup_memory_banksize();
diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig
index aa96c1b3125c..3cf3b93a93fc 100644
--- a/configs/am62x_evm_a53_defconfig
+++ b/configs/am62x_evm_a53_defconfig
@@ -32,6 +32,7 @@ CONFIG_BOOTSTD_FULL=y
 CONFIG_BOOTSTD_DEFAULTS=y
 CONFIG_SYS_BOOTM_LEN=0x800000
 CONFIG_BOOTCOMMAND="run envboot; bootflow scan -lb"
+CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_MAX_SIZE=0x58000
 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
 CONFIG_SPL_BSS_START_ADDR=0x80c80000
-- 
2.43.0

[PATCH 04/10] board: ti: am64x: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

We now can provide a map and have the standard fdtfile variable set from
code itself. This allows for bootstd to "just work".

While at this, replace findfdt in environment with a warning as it is no
longer needed.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/am64x/am64x.env | 9 ---------
 board/ti/am64x/evm.c     | 8 ++++++++
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/board/ti/am64x/am64x.env b/board/ti/am64x/am64x.env
index efd736b99be4..9a8812d4ee54 100644
--- a/board/ti/am64x/am64x.env
+++ b/board/ti/am64x/am64x.env
@@ -2,14 +2,6 @@
 #include <env/ti/mmc.env>
 #include <env/ti/k3_dfu.env>
 
-findfdt=
-	if test $board_name = am64x_gpevm; then
-		setenv name_fdt ti/k3-am642-evm.dtb; fi;
-	if test $board_name = am64x_skevm; then
-		setenv name_fdt ti/k3-am642-sk.dtb; fi;
-	if test $name_fdt = undefined; then
-		echo WARNING: Could not determine device tree to use; fi;
-	setenv fdtfile ${name_fdt}
 name_kern=Image
 console=ttyS2,115200n8
 args_all=setenv optargs earlycon=ns16550a,mmio32,0x02800000 ${mtdparts}
@@ -43,7 +35,6 @@ get_fit_usb=load usb ${bootpart} ${addr_fit}
 usbboot=setenv boot usb;
 	setenv bootpart 0:2;
 	usb start;
-	run findfdt;
 	run init_usb;
 	run get_kern_usb;
 	run get_fdt_usb;
diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
index a6dcff2eb434..e2f506d2c6ea 100644
--- a/board/ti/am64x/evm.c
+++ b/board/ti/am64x/evm.c
@@ -16,6 +16,7 @@
 #include <env.h>
 
 #include "../common/board_detect.h"
+#include "../common/fdt_ops.h"
 
 #define board_is_am64x_gpevm() (board_ti_k3_is("AM64-GPEVM") || \
 				board_ti_k3_is("AM64-HSEVM"))
@@ -180,6 +181,12 @@ int checkboard(void)
 }
 
 #ifdef CONFIG_BOARD_LATE_INIT
+static struct ti_fdt_map ti_am64_evm_fdt_map[] = {
+	{"am64x_gpevm", "k3-am642-evm.dtb"},
+	{"am64x_skevm", "k3-am642-sk.dtb"},
+	{ /* Sentinel. */ }
+};
+
 static void setup_board_eeprom_env(void)
 {
 	char *name = "am64x_gpevm";
@@ -197,6 +204,7 @@ static void setup_board_eeprom_env(void)
 
 invalid_eeprom:
 	set_board_info_env_am6(name);
+	ti_set_fdt_env(name, ti_am64_evm_fdt_map);
 }
 
 static void setup_serial(void)
-- 
2.43.0

[PATCH 05/10] board: ti: am65x: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

We now can provide a map and have the standard fdtfile variable set from
code itself. This allows for bootstd to "just work".

While at this, replace findfdt in environment with a warning as it is no
longer needed.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/am65x/am65x.env | 3 ---
 board/ti/am65x/evm.c     | 2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/board/ti/am65x/am65x.env b/board/ti/am65x/am65x.env
index 286b9c300c05..814374d68cf0 100644
--- a/board/ti/am65x/am65x.env
+++ b/board/ti/am65x/am65x.env
@@ -5,9 +5,6 @@
 #include <env/ti/k3_rproc.env>
 #endif
 
-findfdt=
-	setenv name_fdt ti/k3-am654-base-board.dtb;
-	setenv fdtfile ${name_fdt}
 name_kern=Image
 console=ttyS2,115200n8
 args_all=setenv optargs ${optargs} earlycon=ns16550a,mmio32,0x02800000
diff --git a/board/ti/am65x/evm.c b/board/ti/am65x/evm.c
index df209021c1b7..3109c9a2acac 100644
--- a/board/ti/am65x/evm.c
+++ b/board/ti/am65x/evm.c
@@ -22,6 +22,7 @@
 #include <linux/printk.h>
 
 #include "../common/board_detect.h"
+#include "../common/fdt_ops.h"
 
 #define board_is_am65x_base_board()	board_ti_is("AM6-COMPROCEVM")
 
@@ -141,6 +142,7 @@ static void setup_board_eeprom_env(void)
 
 invalid_eeprom:
 	set_board_info_env_am6(name);
+	ti_set_fdt_env(NULL, NULL);
 }
 
 static int init_daughtercard_det_gpio(char *gpio_name, struct gpio_desc *desc)
-- 
2.43.0

[PATCH 06/10] board: ti: j721e: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

We now can provide a map and have the standard fdtfile variable set from
code itself. This allows for bootstd to "just work".

While at this, replace findfdt in environment with a warning as it is no
longer needed.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/j721e/evm.c     |  8 ++++++++
 board/ti/j721e/j721e.env | 10 ----------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
index c541880107ec..ad6ef4553e04 100644
--- a/board/ti/j721e/evm.c
+++ b/board/ti/j721e/evm.c
@@ -16,6 +16,7 @@
 #include <dm.h>
 
 #include "../common/board_detect.h"
+#include "../common/fdt_ops.h"
 
 #define board_is_j721e_som()	(board_ti_k3_is("J721EX-PM1-SOM") || \
 				 board_ti_k3_is("J721EX-PM2-SOM"))
@@ -424,6 +425,12 @@ void configure_serdes_sierra(void)
 }
 
 #ifdef CONFIG_BOARD_LATE_INIT
+static struct ti_fdt_map ti_j721e_evm_fdt_map[] = {
+	{"j721e", "k3-j721e-common-proc-board.dtb"},
+	{"j721e-sk", "k3-j721e-sk.dtb"},
+	{"j7200", "k3-j7200-common-proc-board.dtb"},
+	{ /* Sentinel. */ }
+};
 static void setup_board_eeprom_env(void)
 {
 	char *name = "j721e";
@@ -443,6 +450,7 @@ static void setup_board_eeprom_env(void)
 
 invalid_eeprom:
 	set_board_info_env_am6(name);
+	ti_set_fdt_env(name, ti_j721e_evm_fdt_map);
 }
 
 static void setup_serial(void)
diff --git a/board/ti/j721e/j721e.env b/board/ti/j721e/j721e.env
index cb27bf5e2b24..38bfd7d49634 100644
--- a/board/ti/j721e/j721e.env
+++ b/board/ti/j721e/j721e.env
@@ -7,16 +7,6 @@
 #include <env/ti/k3_rproc.env>
 #endif
 
-default_device_tree=ti/k3-j721e-common-proc-board.dtb
-findfdt=
-	setenv name_fdt ${default_device_tree};
-	if test $board_name = j721e; then
-		setenv name_fdt ti/k3-j721e-common-proc-board.dtb; fi;
-	if test $board_name = j7200; then
-		setenv name_fdt ti/k3-j7200-common-proc-board.dtb; fi;
-	if test $board_name = j721e-eaik || test $board_name = j721e-sk; then
-		setenv name_fdt ti/k3-j721e-sk.dtb; fi;
-	setenv fdtfile ${name_fdt}
 name_kern=Image
 console=ttyS2,115200n8
 args_all=setenv optargs earlycon=ns16550a,mmio32,0x02800000
-- 
2.43.0

[PATCH 07/10] board: ti: j721s2: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

We now can provide a map and have the standard fdtfile variable set from
code itself. This allows for bootstd to "just work".

While at this, replace findfdt in environment with a warning as it is no
longer needed.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/ti/j721s2/evm.c      | 8 ++++++++
 board/ti/j721s2/j721s2.env | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/board/ti/j721s2/evm.c b/board/ti/j721s2/evm.c
index 1220cd84519b..5a0281d6b483 100644
--- a/board/ti/j721s2/evm.c
+++ b/board/ti/j721s2/evm.c
@@ -23,6 +23,7 @@
 #include <dm/root.h>
 
 #include "../common/board_detect.h"
+#include "../common/fdt_ops.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -114,6 +115,12 @@ int checkboard(void)
 	return 0;
 }
 
+static struct ti_fdt_map ti_j721s2_evm_fdt_map[] = {
+	{"j721s2", "k3-j721s2-common-proc-board.dtb"},
+	{"am68-sk", "k3-am68-sk-base-board.dtb"},
+	{ /* Sentinel. */ }
+};
+
 static void setup_board_eeprom_env(void)
 {
 	char *name = "j721s2";
@@ -131,6 +138,7 @@ static void setup_board_eeprom_env(void)
 
 invalid_eeprom:
 	set_board_info_env_am6(name);
+	ti_set_fdt_env(name, ti_j721s2_evm_fdt_map);
 }
 
 static void setup_serial(void)
diff --git a/board/ti/j721s2/j721s2.env b/board/ti/j721s2/j721s2.env
index 64e3d9da85f0..9a03b9f30aee 100644
--- a/board/ti/j721s2/j721s2.env
+++ b/board/ti/j721s2/j721s2.env
@@ -7,14 +7,6 @@
 #include <env/ti/k3_rproc.env>
 #endif
 
-default_device_tree=ti/k3-j721s2-common-proc-board.dtb
-findfdt=
-	setenv name_fdt ${default_device_tree};
-	if test $board_name = j721s2; then			\
-		setenv name_fdt ti/k3-j721s2-common-proc-board.dtb; fi;
-	if test $board_name = am68-sk; then
-		setenv name_fdt ti/k3-am68-sk-base-board.dtb; fi;
-	setenv fdtfile ${name_fdt}
 name_kern=Image
 console=ttyS2,115200n8
 args_all=setenv optargs earlycon=ns16550a,mmio32,0x02880000
-- 
2.43.0

[PATCH 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

Stop using the findfdt script and switch to setting the fdtfile from C
code.

While at this, replace findfdt in environment with a warning as it is
no longer needed

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/beagle/beagleboneai64/beagleboneai64.c   | 14 ++++++++++++++
 board/beagle/beagleboneai64/beagleboneai64.env |  1 -
 configs/j721e_beagleboneai64_a72_defconfig     |  3 ++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/board/beagle/beagleboneai64/beagleboneai64.c b/board/beagle/beagleboneai64/beagleboneai64.c
index c8c1c78ae5a2..1982f738b04e 100644
--- a/board/beagle/beagleboneai64/beagleboneai64.c
+++ b/board/beagle/beagleboneai64/beagleboneai64.c
@@ -28,3 +28,17 @@ int dram_init_banksize(void)
 {
 	return fdtdec_setup_memory_banksize();
 }
+
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+	char fdtfile[50];
+
+	snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
+		 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
+
+	env_set("fdtfile", fdtfile);
+
+	return 0;
+}
+#endif
diff --git a/board/beagle/beagleboneai64/beagleboneai64.env b/board/beagle/beagleboneai64/beagleboneai64.env
index 4f0a94a8113e..647b25d14c8e 100644
--- a/board/beagle/beagleboneai64/beagleboneai64.env
+++ b/board/beagle/beagleboneai64/beagleboneai64.env
@@ -1,5 +1,4 @@
 #include <env/ti/ti_common.env>
-#include <env/ti/default_findfdt.env>
 #include <env/ti/mmc.env>
 
 name_kern=Image
diff --git a/configs/j721e_beagleboneai64_a72_defconfig b/configs/j721e_beagleboneai64_a72_defconfig
index 959f86844d32..9e53658eacb9 100644
--- a/configs/j721e_beagleboneai64_a72_defconfig
+++ b/configs/j721e_beagleboneai64_a72_defconfig
@@ -34,7 +34,8 @@ CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n"
 CONFIG_AUTOBOOT_DELAY_STR="d"
 CONFIG_AUTOBOOT_STOP_STR=" "
 CONFIG_OF_SYSTEM_SETUP=y
-CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot; bootflow scan -lb;run set_led_state_fail_load"
+CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan -lb;run set_led_state_fail_load"
+CONFIG_BOARD_LATE_INIT=y
 CONFIG_LOGLEVEL=7
 CONFIG_SPL_MAX_SIZE=0xc0000
 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
-- 
2.43.0

[PATCH 09/10] board: beagle: beagleplay: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

Stop using the findfdt script and switch to setting the fdtfile from C
code.

While at this, replace findfdt in environment with a warning as it is
no longer needed.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 board/beagle/beagleplay/beagleplay.c   | 14 ++++++++++++++
 board/beagle/beagleplay/beagleplay.env |  1 -
 configs/am62x_beagleplay_a53_defconfig |  3 ++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/board/beagle/beagleplay/beagleplay.c b/board/beagle/beagleplay/beagleplay.c
index 1c376dea372f..00ff86165790 100644
--- a/board/beagle/beagleplay/beagleplay.c
+++ b/board/beagle/beagleplay/beagleplay.c
@@ -27,3 +27,17 @@ int dram_init_banksize(void)
 {
 	return fdtdec_setup_memory_banksize();
 }
+
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+	char fdtfile[50];
+
+	snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
+		 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
+
+	env_set("fdtfile", fdtfile);
+
+	return 0;
+}
+#endif
diff --git a/board/beagle/beagleplay/beagleplay.env b/board/beagle/beagleplay/beagleplay.env
index 4f0a94a8113e..647b25d14c8e 100644
--- a/board/beagle/beagleplay/beagleplay.env
+++ b/board/beagle/beagleplay/beagleplay.env
@@ -1,5 +1,4 @@
 #include <env/ti/ti_common.env>
-#include <env/ti/default_findfdt.env>
 #include <env/ti/mmc.env>
 
 name_kern=Image
diff --git a/configs/am62x_beagleplay_a53_defconfig b/configs/am62x_beagleplay_a53_defconfig
index 0be20045a974..1f43891d10bb 100644
--- a/configs/am62x_beagleplay_a53_defconfig
+++ b/configs/am62x_beagleplay_a53_defconfig
@@ -33,7 +33,8 @@ CONFIG_AUTOBOOT_KEYED=y
 CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n"
 CONFIG_AUTOBOOT_DELAY_STR="d"
 CONFIG_AUTOBOOT_STOP_STR=" "
-CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot; bootflow scan -lb;run set_led_state_fail_load"
+CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan -lb;run set_led_state_fail_load"
+CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_MAX_SIZE=0x58000
 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
 CONFIG_SPL_BSS_START_ADDR=0x80c80000
-- 
2.43.0

[PATCH 10/10] include: env: ti: Drop default_findfdt

[Nishanth Menon]

We shouldn't need finfdt anymore. Drop the env script.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 include/env/ti/default_findfdt.env | 12 ------------
 1 file changed, 12 deletions(-)
 delete mode 100644 include/env/ti/default_findfdt.env

diff --git a/include/env/ti/default_findfdt.env b/include/env/ti/default_findfdt.env
deleted file mode 100644
index a2b51dd923bb..000000000000
--- a/include/env/ti/default_findfdt.env
+++ /dev/null
@@ -1,12 +0,0 @@
-default_device_tree=CONFIG_DEFAULT_DEVICE_TREE
-default_device_tree_arch=ti
-#ifdef CONFIG_ARM64
-findfdt=
-	setenv name_fdt ${default_device_tree_arch}/${default_device_tree}.dtb;
-	setenv fdtfile ${name_fdt}
-#else
-default_device_tree_subarch=omap
-findfdt=
-	setenv name_fdt ${default_device_tree_arch}/${default_device_tree_subarch}/${default_device_tree}.dtb;
-	setenv fdtfile ${name_fdt}
-#endif
-- 
2.43.0

Re: [PATCH 00/10] board/ti: k3 boards: Stop using findfdt

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Mon, Jan 08, 2024 at 11:32:51AM -0600, Nishanth Menon wrote:

> This is a wide cleanup to switch to setting fdtfile using env_set
> instead of scripted magic. 'fdtfile' is expected to be set by default.
> This allows the stdboot triggered efi loaders to find the correct OS
> device tree file.
> 
> This is a refresh of
> https://lore.kernel.org/all/86le9dwz4d.fsf@udb0321960.dhcp.ti.com/ which
> was the wrong approach.
> 
> Bootlogs: https://gist.github.com/nmenon/2f4a142c1bcaa09d544b1f2e206ea134
> 
> NOTE: There are a couple of checkpatch WARN (around LATE_INIT) and
> CHECK (fdt_ops #ifdeffery) that on closer inspection looks fine and
> consistent with other similar usage.
> 
> Based on next branch at: c2c598e87cfe Merge branch 'staging' of https://source.denx.de/u-boot/custodians/u-boot-tegra into next

This overall seems fine, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Andrew Davis]

On 1/8/24 11:32 AM, Nishanth Menon wrote:
> Introduce a common fdt operations library for basic device tree
> operations that are common between various boards.
> 
> The first library to introduce here is the capability to set up
> fdtfile as a standard variable as part of board identification rather
> than depend on scripted ifdeffery.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>   board/ti/common/Kconfig   | 12 ++++++++
>   board/ti/common/Makefile  |  1 +
>   board/ti/common/fdt_ops.c | 65 +++++++++++++++++++++++++++++++++++++++
>   board/ti/common/fdt_ops.h | 41 ++++++++++++++++++++++++
>   4 files changed, 119 insertions(+)
>   create mode 100644 board/ti/common/fdt_ops.c
>   create mode 100644 board/ti/common/fdt_ops.h
> 
> diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
> index 49edd98014ab..06a8a36aa1cd 100644
> --- a/board/ti/common/Kconfig
> +++ b/board/ti/common/Kconfig
> @@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
>   	imply CMD_SPI
>   	imply CMD_TIME
>   	imply CMD_USB if USB
> +
> +config TI_EVM_FDT_FOLDER_PATH
> +	string "Location of Folder path where dtb is present"
> +	default "ti/davinci" if ARCH_DAVINCI
> +	default "ti/keystone" if ARCH_KEYSTONE
> +	default "ti/omap" if ARCH_OMAP2PLUS
> +	default "ti" if ARCH_K3
> +	depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> +	help
> +	   Folder path for kernel device tree default.
> +	   This is used along with fdtfile path to locate the kernel
> +	   device tree blob.
> diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
> index 26bf12e2e6d5..5ac361ba7fcf 100644
> --- a/board/ti/common/Makefile
> +++ b/board/ti/common/Makefile
> @@ -3,3 +3,4 @@
>   
>   obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
>   obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
> +obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
> diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
> new file mode 100644
> index 000000000000..f8770cae4a54
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Library to support FDT file operations which are common
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include <env.h>
> +#include <vsprintf.h>
> +#include "fdt_ops.h"
> +
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)
> +{
> +	char *fdt_file_name = NULL;
> +	char fdtfile[TI_FDT_FILE_MAX];
> +
> +	if (name_fdt) {
> +		while (fdt_map) {
> +			/* Check for NULL terminator in the list */
> +			if (!fdt_map->name_fdt)
> +				break;
> +			if (!strncmp(fdt_map->name_fdt, name_fdt, TI_NAME_FDT_MAX)) {
> +				fdt_file_name = fdt_map->fdt_file_name;
> +				break;
> +			}
> +			fdt_map++;
> +		}
> +	}
> +
> +	/* match not found OR null name_fdt */
> +	if (!fdt_file_name) {
> +		/*
> +		 * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> +		 * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> +		 */
> +#ifdef CONFIG_DEFAULT_FDT_FILE
> +		if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> +				 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_FDT_FILE);
> +		} else
> +#endif
> +		{
> +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> +				 CONFIG_TI_EVM_FDT_FOLDER_PATH,
> +				 CONFIG_DEFAULT_DEVICE_TREE);
> +		}
> +	} else {
> +		snprintf(fdtfile, sizeof(fdtfile), "%s/%s", CONFIG_TI_EVM_FDT_FOLDER_PATH,
> +			 fdt_file_name);
> +	}
> +
> +	env_set("fdtfile", fdtfile);
> +
> +	/*
> +	 * XXX: DEPRECATION WARNING: 2 u-boot versions.
> +	 *
> +	 * Maintain compatibility with downstream scripts that may be using
> +	 * name_fdt
> +	 */
> +	if (name_fdt)
> +		env_set("name_fdt", name_fdt);

"name_fdt" should match "fdtfile", you should have just:

env_set("name_fdt", fdtfile);

are you mixing this up with "board_name"?

Andrew

> +	/* Also set the findfdt legacy script to warn users to stop using this */
> +	env_set("findfdt",
> +		"echo WARN: fdtfile already set. Stop using findfdt in script");
> +}
> diff --git a/board/ti/common/fdt_ops.h b/board/ti/common/fdt_ops.h
> new file mode 100644
> index 000000000000..c01697bed28f
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Library to support common device tree manipulation for TI EVMs
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com
> + */
> +
> +#ifndef __FDT_OPS_H
> +#define __FDT_OPS_H
> +
> +#define TI_NAME_FDT_MAX 20
> +#define TI_FDT_FILE_MAX 200
> +
> +/**
> + *  struct ti_fdt_map - mapping of device tree blob name to board name
> + *  @name_fdt: board_name up to TI_NAME_FDT_MAX long
> + *  @fdt_file_name: device tree blob name as described by kernel
> + */
> +struct ti_fdt_map {
> +	const char *name_fdt;
> +	char *fdt_file_name;
> +};
> +
> +/**
> + * ti_set_fdt_env  - Find the correct device tree file name and set 'fdtfile'
> + * env variable with correct folder structure appropriate to the architecture
> + * and kernel conventions. This function is invoked typically as part of
> + * board_late_init
> + *
> + * fdt name is picked by:
> + * a) If a match is found, use the match
> + * b) If not, CONFIG_DEFAULT_FDT_FILE (Boot OS device tree) if that is defined
> + *    and not null
> + * c) If not, Use CONFIG_DEFAULT_DEVICE_TREE (DT control for bootloader)
> + *
> + * @name_fdt: match to search with (max of TI_NAME_FDT_MAX chars)
> + * @fdt_map: NULL terminated array of device tree file name matches.
> + */
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map);
> +
> +#endif /* __FDT_OPS_H */

Re: [PATCH 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script

[Andrew Davis]

On 1/8/24 11:32 AM, Nishanth Menon wrote:
> Stop using the findfdt script and switch to setting the fdtfile from C
> code.
> 
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>   board/beagle/beagleboneai64/beagleboneai64.c   | 14 ++++++++++++++
>   board/beagle/beagleboneai64/beagleboneai64.env |  1 -
>   configs/j721e_beagleboneai64_a72_defconfig     |  3 ++-
>   3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.c b/board/beagle/beagleboneai64/beagleboneai64.c
> index c8c1c78ae5a2..1982f738b04e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.c
> +++ b/board/beagle/beagleboneai64/beagleboneai64.c
> @@ -28,3 +28,17 @@ int dram_init_banksize(void)
>   {
>   	return fdtdec_setup_memory_banksize();
>   }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> +	char fdtfile[50];
> +
> +	snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> +		 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
> +

Why not use your new ti_set_fdt_env() helper? It makes this board
consistent with TI boards (sets "name_fdt" and adds warning for
"findfdt" like your commit message says you do..).

Andrew

> +	env_set("fdtfile", fdtfile);
> +
> +	return 0;
> +}
> +#endif
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.env b/board/beagle/beagleboneai64/beagleboneai64.env
> index 4f0a94a8113e..647b25d14c8e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.env
> +++ b/board/beagle/beagleboneai64/beagleboneai64.env
> @@ -1,5 +1,4 @@
>   #include <env/ti/ti_common.env>
> -#include <env/ti/default_findfdt.env>
>   #include <env/ti/mmc.env>
>   
>   name_kern=Image
> diff --git a/configs/j721e_beagleboneai64_a72_defconfig b/configs/j721e_beagleboneai64_a72_defconfig
> index 959f86844d32..9e53658eacb9 100644
> --- a/configs/j721e_beagleboneai64_a72_defconfig
> +++ b/configs/j721e_beagleboneai64_a72_defconfig
> @@ -34,7 +34,8 @@ CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n"
>   CONFIG_AUTOBOOT_DELAY_STR="d"
>   CONFIG_AUTOBOOT_STOP_STR=" "
>   CONFIG_OF_SYSTEM_SETUP=y
> -CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot; bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOARD_LATE_INIT=y
>   CONFIG_LOGLEVEL=7
>   CONFIG_SPL_MAX_SIZE=0xc0000
>   CONFIG_SPL_HAS_BSS_LINKER_SECTION=y

Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Nishanth Menon]

On 12:50-20240108, Andrew Davis wrote:
> On 1/8/24 11:32 AM, Nishanth Menon wrote:
> > Introduce a common fdt operations library for basic device tree
> > operations that are common between various boards.
> > 
> > The first library to introduce here is the capability to set up
> > fdtfile as a standard variable as part of board identification rather
> > than depend on scripted ifdeffery.
> > 
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > ---
> >   board/ti/common/Kconfig   | 12 ++++++++
> >   board/ti/common/Makefile  |  1 +
> >   board/ti/common/fdt_ops.c | 65 +++++++++++++++++++++++++++++++++++++++
> >   board/ti/common/fdt_ops.h | 41 ++++++++++++++++++++++++
> >   4 files changed, 119 insertions(+)
> >   create mode 100644 board/ti/common/fdt_ops.c
> >   create mode 100644 board/ti/common/fdt_ops.h
> > 
> > diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
> > index 49edd98014ab..06a8a36aa1cd 100644
> > --- a/board/ti/common/Kconfig
> > +++ b/board/ti/common/Kconfig
> > @@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
> >   	imply CMD_SPI
> >   	imply CMD_TIME
> >   	imply CMD_USB if USB
> > +
> > +config TI_EVM_FDT_FOLDER_PATH
> > +	string "Location of Folder path where dtb is present"
> > +	default "ti/davinci" if ARCH_DAVINCI
> > +	default "ti/keystone" if ARCH_KEYSTONE
> > +	default "ti/omap" if ARCH_OMAP2PLUS
> > +	default "ti" if ARCH_K3
> > +	depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> > +	help
> > +	   Folder path for kernel device tree default.
> > +	   This is used along with fdtfile path to locate the kernel
> > +	   device tree blob.
> > diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
> > index 26bf12e2e6d5..5ac361ba7fcf 100644
> > --- a/board/ti/common/Makefile
> > +++ b/board/ti/common/Makefile
> > @@ -3,3 +3,4 @@
> >   obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
> >   obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
> > +obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
> > diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
> > new file mode 100644
> > index 000000000000..f8770cae4a54
> > --- /dev/null
> > +++ b/board/ti/common/fdt_ops.c
> > @@ -0,0 +1,65 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Library to support FDT file operations which are common
> > + *
> > + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> > + */
> > +
> > +#include <env.h>
> > +#include <vsprintf.h>
> > +#include "fdt_ops.h"
> > +
> > +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)
> > +{
> > +	char *fdt_file_name = NULL;
> > +	char fdtfile[TI_FDT_FILE_MAX];
> > +
> > +	if (name_fdt) {
> > +		while (fdt_map) {
> > +			/* Check for NULL terminator in the list */
> > +			if (!fdt_map->name_fdt)
> > +				break;
> > +			if (!strncmp(fdt_map->name_fdt, name_fdt, TI_NAME_FDT_MAX)) {
> > +				fdt_file_name = fdt_map->fdt_file_name;
> > +				break;
> > +			}
> > +			fdt_map++;
> > +		}
> > +	}
> > +
> > +	/* match not found OR null name_fdt */
> > +	if (!fdt_file_name) {
> > +		/*
> > +		 * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> > +		 * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> > +		 */
> > +#ifdef CONFIG_DEFAULT_FDT_FILE
> > +		if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> > +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> > +				 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_FDT_FILE);
> > +		} else
> > +#endif
> > +		{
> > +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> > +				 CONFIG_TI_EVM_FDT_FOLDER_PATH,
> > +				 CONFIG_DEFAULT_DEVICE_TREE);
> > +		}
> > +	} else {
> > +		snprintf(fdtfile, sizeof(fdtfile), "%s/%s", CONFIG_TI_EVM_FDT_FOLDER_PATH,
> > +			 fdt_file_name);
> > +	}
> > +
> > +	env_set("fdtfile", fdtfile);
> > +
> > +	/*
> > +	 * XXX: DEPRECATION WARNING: 2 u-boot versions.
> > +	 *
> > +	 * Maintain compatibility with downstream scripts that may be using
> > +	 * name_fdt
> > +	 */
> > +	if (name_fdt)
> > +		env_set("name_fdt", name_fdt);
> 
> "name_fdt" should match "fdtfile", you should have just:
> 
> env_set("name_fdt", fdtfile);
> 
> are you mixing this up with "board_name"?

It should have been fdtfile. Thanks for catching it.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

On 13:00-20240108, Andrew Davis wrote:
> > +#ifdef CONFIG_BOARD_LATE_INIT
> > +int board_late_init(void)
> > +{
> > +	char fdtfile[50];
> > +
> > +	snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> > +		 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
> > +
> 
> Why not use your new ti_set_fdt_env() helper? It makes this board

I had it originally, but did not want the code to depend on
board/ti/common functions. So dropped it in later version.

> consistent with TI boards (sets "name_fdt" and adds warning for
> "findfdt" like your commit message says you do..).

Will fix up the commit message in the next revision


-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Jon Humphreys]

Nishanth Menon <nm@ti.com> writes:

> Introduce a common fdt operations library for basic device tree
> operations that are common between various boards.
>
> The first library to introduce here is the capability to set up
> fdtfile as a standard variable as part of board identification rather
> than depend on scripted ifdeffery.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  board/ti/common/Kconfig   | 12 ++++++++
>  board/ti/common/Makefile  |  1 +
>  board/ti/common/fdt_ops.c | 65 +++++++++++++++++++++++++++++++++++++++
>  board/ti/common/fdt_ops.h | 41 ++++++++++++++++++++++++
>  4 files changed, 119 insertions(+)
>  create mode 100644 board/ti/common/fdt_ops.c
>  create mode 100644 board/ti/common/fdt_ops.h
>
> diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
> index 49edd98014ab..06a8a36aa1cd 100644
> --- a/board/ti/common/Kconfig
> +++ b/board/ti/common/Kconfig
> @@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
>  	imply CMD_SPI
>  	imply CMD_TIME
>  	imply CMD_USB if USB
> +
> +config TI_EVM_FDT_FOLDER_PATH
> +	string "Location of Folder path where dtb is present"
> +	default "ti/davinci" if ARCH_DAVINCI
> +	default "ti/keystone" if ARCH_KEYSTONE
> +	default "ti/omap" if ARCH_OMAP2PLUS
> +	default "ti" if ARCH_K3
> +	depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> +	help
> +	   Folder path for kernel device tree default.
> +	   This is used along with fdtfile path to locate the kernel
> +	   device tree blob.

It's not clear to me why we need the flexibility of specifying a FDT
filename per board independently of the FDT folder path.  Why can't the path
be part of the fdt_map?

> diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
> index 26bf12e2e6d5..5ac361ba7fcf 100644
> --- a/board/ti/common/Makefile
> +++ b/board/ti/common/Makefile
> @@ -3,3 +3,4 @@
>  
>  obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
>  obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
> +obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
> diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
> new file mode 100644
> index 000000000000..f8770cae4a54
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Library to support FDT file operations which are common
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include <env.h>
> +#include <vsprintf.h>
> +#include "fdt_ops.h"
> +
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)

This function takes a board name and sets the FDT name, so why isn't the
first parameter called 'board_name' or similar?

> +{
> +	char *fdt_file_name = NULL;
> +	char fdtfile[TI_FDT_FILE_MAX];
> +
> +	if (name_fdt) {
> +		while (fdt_map) {
> +			/* Check for NULL terminator in the list */
> +			if (!fdt_map->name_fdt)
> +				break;
> +			if (!strncmp(fdt_map->name_fdt, name_fdt, TI_NAME_FDT_MAX)) {

Why do we need a max length?  Shouldn't strcmp() be fine given the
name_fdt member of the fdt_map is set in code (ie, not read in)?

> +				fdt_file_name = fdt_map->fdt_file_name;
> +				break;
> +			}
> +			fdt_map++;
> +		}
> +	}
> +
> +	/* match not found OR null name_fdt */
> +	if (!fdt_file_name) {
> +		/*
> +		 * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> +		 * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> +		 */
> +#ifdef CONFIG_DEFAULT_FDT_FILE
> +		if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> +				 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_FDT_FILE);

I do not see where any TI platforms set CONFIG_DEFAULT_FDT_FILE, so why
have logic that checks for it?  We don't use it.  With this patch
(fdt_map) I don't see why we would start needing it in the future.

> +		} else
> +#endif
> +		{
> +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> +				 CONFIG_TI_EVM_FDT_FOLDER_PATH,
> +				 CONFIG_DEFAULT_DEVICE_TREE);

If fdtfile isn't set, EFI bootmeth falls back to the control DT anyway,
so this is unnecessary duplication of logic.

> +		}
> +	} else {
> +		snprintf(fdtfile, sizeof(fdtfile), "%s/%s", CONFIG_TI_EVM_FDT_FOLDER_PATH,
> +			 fdt_file_name);
> +	}
> +
> +	env_set("fdtfile", fdtfile);
> +
> +	/*
> +	 * XXX: DEPRECATION WARNING: 2 u-boot versions.
> +	 *
> +	 * Maintain compatibility with downstream scripts that may be using
> +	 * name_fdt
> +	 */
> +	if (name_fdt)
> +		env_set("name_fdt", name_fdt);
> +	/* Also set the findfdt legacy script to warn users to stop using this */
> +	env_set("findfdt",
> +		"echo WARN: fdtfile already set. Stop using findfdt in script");
> +}
> diff --git a/board/ti/common/fdt_ops.h b/board/ti/common/fdt_ops.h
> new file mode 100644
> index 000000000000..c01697bed28f
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Library to support common device tree manipulation for TI EVMs
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com
> + */
> +
> +#ifndef __FDT_OPS_H
> +#define __FDT_OPS_H
> +
> +#define TI_NAME_FDT_MAX 20

TI_BOARD_NAME_MAX??

> +#define TI_FDT_FILE_MAX 200
> +
> +/**
> + *  struct ti_fdt_map - mapping of device tree blob name to board name
> + *  @name_fdt: board_name up to TI_NAME_FDT_MAX long

If this is the board_name, why call it name_fdt?  Why not board_name?

> + *  @fdt_file_name: device tree blob name as described by kernel
> + */
> +struct ti_fdt_map {
> +	const char *name_fdt;
> +	char *fdt_file_name;
> +};
> +
> +/**
> + * ti_set_fdt_env  - Find the correct device tree file name and set
>     'fdtfile'

"Find the correct device tree file name based on the board name and "...

> + * env variable with correct folder structure appropriate to the architecture
> + * and kernel conventions. This function is invoked typically as part of
> + * board_late_init
> + *
> + * fdt name is picked by:
> + * a) If a match is found, use the match

"a) If a board name match is found, use the match"

> + * b) If not, CONFIG_DEFAULT_FDT_FILE (Boot OS device tree) if that is defined
> + *    and not null
> + * c) If not, Use CONFIG_DEFAULT_DEVICE_TREE (DT control for bootloader)
> + *
> + * @name_fdt: match to search with (max of TI_NAME_FDT_MAX chars)
> + * @fdt_map: NULL terminated array of device tree file name matches.
> + */
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map);
> +
> +#endif /* __FDT_OPS_H */
> -- 
> 2.43.0

Re: [PATCH 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script

[Jon Humphreys]

Nishanth Menon <nm@ti.com> writes:

> Stop using the findfdt script and switch to setting the fdtfile from C
> code.
>
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  board/beagle/beagleboneai64/beagleboneai64.c   | 14 ++++++++++++++
>  board/beagle/beagleboneai64/beagleboneai64.env |  1 -
>  configs/j721e_beagleboneai64_a72_defconfig     |  3 ++-
>  3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.c b/board/beagle/beagleboneai64/beagleboneai64.c
> index c8c1c78ae5a2..1982f738b04e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.c
> +++ b/board/beagle/beagleboneai64/beagleboneai64.c
> @@ -28,3 +28,17 @@ int dram_init_banksize(void)
>  {
>  	return fdtdec_setup_memory_banksize();
>  }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> +	char fdtfile[50];
> +
> +	snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> +		 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);

This would set the board to using the control DT, not boot DT.  Is that
what you meant?

But anyway, why not just hard code the FDT path/name here since there is
only one for this board?  I don't see the value in the extra logic of
using the config values (or having a fdt_map).  (Same for beagleplay)

> +
> +	env_set("fdtfile", fdtfile);
> +
> +	return 0;
> +}
> +#endif
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.env b/board/beagle/beagleboneai64/beagleboneai64.env
> index 4f0a94a8113e..647b25d14c8e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.env
> +++ b/board/beagle/beagleboneai64/beagleboneai64.env
> @@ -1,5 +1,4 @@
>  #include <env/ti/ti_common.env>
> -#include <env/ti/default_findfdt.env>
>  #include <env/ti/mmc.env>
>  
>  name_kern=Image
> diff --git a/configs/j721e_beagleboneai64_a72_defconfig b/configs/j721e_beagleboneai64_a72_defconfig
> index 959f86844d32..9e53658eacb9 100644
> --- a/configs/j721e_beagleboneai64_a72_defconfig
> +++ b/configs/j721e_beagleboneai64_a72_defconfig
> @@ -34,7 +34,8 @@ CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n"
>  CONFIG_AUTOBOOT_DELAY_STR="d"
>  CONFIG_AUTOBOOT_STOP_STR=" "
>  CONFIG_OF_SYSTEM_SETUP=y
> -CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot; bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOARD_LATE_INIT=y
>  CONFIG_LOGLEVEL=7
>  CONFIG_SPL_MAX_SIZE=0xc0000
>  CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> -- 
> 2.43.0

Re: [PATCH 02/10] board: ti: am62ax: Set fdtfile from C code instead of findfdt script

[Jon Humphreys]

Nishanth Menon <nm@ti.com> writes:

> Stop using the findfdt script and switch to setting the fdtfile from
> C code.
>
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  board/ti/am62ax/am62ax.env       |  1 -
>  board/ti/am62ax/evm.c            | 10 ++++++++++
>  configs/am62ax_evm_a53_defconfig |  1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/board/ti/am62ax/am62ax.env b/board/ti/am62ax/am62ax.env
> index a6d967e982d4..334374abb73e 100644
> --- a/board/ti/am62ax/am62ax.env
> +++ b/board/ti/am62ax/am62ax.env
> @@ -1,5 +1,4 @@
>  #include <env/ti/ti_common.env>
> -#include <env/ti/default_findfdt.env>
>  #include <env/ti/mmc.env>
>  
>  name_kern=Image
> diff --git a/board/ti/am62ax/evm.c b/board/ti/am62ax/evm.c
> index cd3360a43029..62d3664936e7 100644
> --- a/board/ti/am62ax/evm.c
> +++ b/board/ti/am62ax/evm.c
> @@ -13,6 +13,8 @@
>  #include <fdt_support.h>
>  #include <spl.h>
>  
> +#include "../common/fdt_ops.h"
> +
>  int board_init(void)
>  {
>  	return 0;
> @@ -27,3 +29,11 @@ int dram_init_banksize(void)
>  {
>  	return fdtdec_setup_memory_banksize();
>  }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> +	ti_set_fdt_env(NULL, NULL);

If a board only has one FDT possible, why not just hard code the path
here, rather than use the set_fdt_env() logic with NULLs and create/set
TI_EVM_FDT_FOLDER_PATH config that is only used here anyway?

If you think we might add additional board types / FDTs later, then
instead let's formalize the fdt_map concept and create a CONFIG_USES_FDT_MAP
that will then rely on the fdt_map and call the ti_set_fdt_env(), and if
not set, then it just returns a hard coded value, which could be based on
the CONFIG_DEFAULT_DEVICE_TREE config.

> +	return 0;
> +}
> +#endif
> diff --git a/configs/am62ax_evm_a53_defconfig b/configs/am62ax_evm_a53_defconfig
> index 38083586a3ec..e5fcd8cc5b6f 100644
> --- a/configs/am62ax_evm_a53_defconfig
> +++ b/configs/am62ax_evm_a53_defconfig
> @@ -24,6 +24,7 @@ CONFIG_SPL_LOAD_FIT_ADDRESS=0x81000000
>  CONFIG_BOOTSTD_FULL=y
>  CONFIG_BOOTSTD_DEFAULTS=y
>  CONFIG_BOOTCOMMAND="run envboot; bootflow scan -lb"
> +CONFIG_BOARD_LATE_INIT=y
>  CONFIG_SPL_MAX_SIZE=0x58000
>  CONFIG_SPL_PAD_TO=0x0
>  CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> -- 
> 2.43.0

Re: [PATCH 04/10] board: ti: am64x: Set fdtfile from C code instead of findfdt script

[Roger Quadros]

On 08/01/2024 19:32, Nishanth Menon wrote:
> We now can provide a map and have the standard fdtfile variable set from
> code itself. This allows for bootstd to "just work".
> 
> While at this, replace findfdt in environment with a warning as it is no
> longer needed.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  board/ti/am64x/am64x.env | 9 ---------
>  board/ti/am64x/evm.c     | 8 ++++++++
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/board/ti/am64x/am64x.env b/board/ti/am64x/am64x.env
> index efd736b99be4..9a8812d4ee54 100644
> --- a/board/ti/am64x/am64x.env
> +++ b/board/ti/am64x/am64x.env
> @@ -2,14 +2,6 @@
>  #include <env/ti/mmc.env>
>  #include <env/ti/k3_dfu.env>
>  
> -findfdt=
> -	if test $board_name = am64x_gpevm; then
> -		setenv name_fdt ti/k3-am642-evm.dtb; fi;
> -	if test $board_name = am64x_skevm; then
> -		setenv name_fdt ti/k3-am642-sk.dtb; fi;
> -	if test $name_fdt = undefined; then
> -		echo WARNING: Could not determine device tree to use; fi;
> -	setenv fdtfile ${name_fdt}
>  name_kern=Image
>  console=ttyS2,115200n8
>  args_all=setenv optargs earlycon=ns16550a,mmio32,0x02800000 ${mtdparts}
> @@ -43,7 +35,6 @@ get_fit_usb=load usb ${bootpart} ${addr_fit}
>  usbboot=setenv boot usb;
>  	setenv bootpart 0:2;
>  	usb start;
> -	run findfdt;
>  	run init_usb;
>  	run get_kern_usb;
>  	run get_fdt_usb;
> diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
> index a6dcff2eb434..e2f506d2c6ea 100644
> --- a/board/ti/am64x/evm.c
> +++ b/board/ti/am64x/evm.c
> @@ -16,6 +16,7 @@
>  #include <env.h>
>  
>  #include "../common/board_detect.h"
> +#include "../common/fdt_ops.h"
>  
>  #define board_is_am64x_gpevm() (board_ti_k3_is("AM64-GPEVM") || \
>  				board_ti_k3_is("AM64-HSEVM"))
> @@ -180,6 +181,12 @@ int checkboard(void)
>  }
>  
>  #ifdef CONFIG_BOARD_LATE_INIT
> +static struct ti_fdt_map ti_am64_evm_fdt_map[] = {
> +	{"am64x_gpevm", "k3-am642-evm.dtb"},
> +	{"am64x_skevm", "k3-am642-sk.dtb"},

"am64x_gpevm" and "am64x_skevm" strings are used multiple times in this file.
see setup_board_eeprom_env()

Please use a MACRO for them.

What is the logic of choosing this name and can it be updated at this point?
e.g."gp" is misleading in the board name as the boards are now shipped with
HS-FS chip and are no longer GP.


> +	{ /* Sentinel. */ }
> +};
> +
>  static void setup_board_eeprom_env(void)
>  {
>  	char *name = "am64x_gpevm";
> @@ -197,6 +204,7 @@ static void setup_board_eeprom_env(void)
>  
>  invalid_eeprom:
>  	set_board_info_env_am6(name);
> +	ti_set_fdt_env(name, ti_am64_evm_fdt_map);
>  }
>  
>  static void setup_serial(void)

-- 
cheers,
-roger

Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Roger Quadros]

On 08/01/2024 19:32, Nishanth Menon wrote:
> Introduce a common fdt operations library for basic device tree
> operations that are common between various boards.
> 
> The first library to introduce here is the capability to set up
> fdtfile as a standard variable as part of board identification rather
> than depend on scripted ifdeffery.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  board/ti/common/Kconfig   | 12 ++++++++
>  board/ti/common/Makefile  |  1 +
>  board/ti/common/fdt_ops.c | 65 +++++++++++++++++++++++++++++++++++++++
>  board/ti/common/fdt_ops.h | 41 ++++++++++++++++++++++++
>  4 files changed, 119 insertions(+)
>  create mode 100644 board/ti/common/fdt_ops.c
>  create mode 100644 board/ti/common/fdt_ops.h
> 
> diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
> index 49edd98014ab..06a8a36aa1cd 100644
> --- a/board/ti/common/Kconfig
> +++ b/board/ti/common/Kconfig
> @@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
>  	imply CMD_SPI
>  	imply CMD_TIME
>  	imply CMD_USB if USB
> +
> +config TI_EVM_FDT_FOLDER_PATH
> +	string "Location of Folder path where dtb is present"
> +	default "ti/davinci" if ARCH_DAVINCI
> +	default "ti/keystone" if ARCH_KEYSTONE
> +	default "ti/omap" if ARCH_OMAP2PLUS
> +	default "ti" if ARCH_K3
> +	depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> +	help
> +	   Folder path for kernel device tree default.
> +	   This is used along with fdtfile path to locate the kernel
> +	   device tree blob.
> diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
> index 26bf12e2e6d5..5ac361ba7fcf 100644
> --- a/board/ti/common/Makefile
> +++ b/board/ti/common/Makefile
> @@ -3,3 +3,4 @@
>  
>  obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
>  obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
> +obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
> diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
> new file mode 100644
> index 000000000000..f8770cae4a54
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Library to support FDT file operations which are common
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include <env.h>
> +#include <vsprintf.h>
> +#include "fdt_ops.h"
> +
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)
> +{
> +	char *fdt_file_name = NULL;
> +	char fdtfile[TI_FDT_FILE_MAX];
> +
> +	if (name_fdt) {
> +		while (fdt_map) {
> +			/* Check for NULL terminator in the list */
> +			if (!fdt_map->name_fdt)
> +				break;
> +			if (!strncmp(fdt_map->name_fdt, name_fdt, TI_NAME_FDT_MAX)) {
> +				fdt_file_name = fdt_map->fdt_file_name;
> +				break;
> +			}
> +			fdt_map++;
> +		}
> +	}
> +
> +	/* match not found OR null name_fdt */
> +	if (!fdt_file_name) {
> +		/*
> +		 * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> +		 * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> +		 */
> +#ifdef CONFIG_DEFAULT_FDT_FILE
> +		if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> +				 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_FDT_FILE);
> +		} else
> +#endif
> +		{
> +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> +				 CONFIG_TI_EVM_FDT_FOLDER_PATH,
> +				 CONFIG_DEFAULT_DEVICE_TREE);
> +		}
> +	} else {
> +		snprintf(fdtfile, sizeof(fdtfile), "%s/%s", CONFIG_TI_EVM_FDT_FOLDER_PATH,
> +			 fdt_file_name);
> +	}
> +
> +	env_set("fdtfile", fdtfile);
> +
> +	/*
> +	 * XXX: DEPRECATION WARNING: 2 u-boot versions.
> +	 *
> +	 * Maintain compatibility with downstream scripts that may be using
> +	 * name_fdt
> +	 */
> +	if (name_fdt)
> +		env_set("name_fdt", name_fdt);
> +	/* Also set the findfdt legacy script to warn users to stop using this */
> +	env_set("findfdt",
> +		"echo WARN: fdtfile already set. Stop using findfdt in script");
> +}
> diff --git a/board/ti/common/fdt_ops.h b/board/ti/common/fdt_ops.h
> new file mode 100644
> index 000000000000..c01697bed28f
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Library to support common device tree manipulation for TI EVMs
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com
> + */
> +
> +#ifndef __FDT_OPS_H
> +#define __FDT_OPS_H
> +
> +#define TI_NAME_FDT_MAX 20
> +#define TI_FDT_FILE_MAX 200
> +
> +/**
> + *  struct ti_fdt_map - mapping of device tree blob name to board name
> + *  @name_fdt: board_name up to TI_NAME_FDT_MAX long
> + *  @fdt_file_name: device tree blob name as described by kernel
> + */
> +struct ti_fdt_map {
> +	const char *name_fdt;

Can we call this board_name? as name_fdt corresponds to device tree name.

> +	char *fdt_file_name;
> +};
> +
> +/**
> + * ti_set_fdt_env  - Find the correct device tree file name and set 'fdtfile'
> + * env variable with correct folder structure appropriate to the architecture
> + * and kernel conventions. This function is invoked typically as part of
> + * board_late_init
> + *
> + * fdt name is picked by:
> + * a) If a match is found, use the match
> + * b) If not, CONFIG_DEFAULT_FDT_FILE (Boot OS device tree) if that is defined
> + *    and not null
> + * c) If not, Use CONFIG_DEFAULT_DEVICE_TREE (DT control for bootloader)
> + *
> + * @name_fdt: match to search with (max of TI_NAME_FDT_MAX chars)
> + * @fdt_map: NULL terminated array of device tree file name matches.
> + */
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map);
> +
> +#endif /* __FDT_OPS_H */

-- 
cheers,
-roger

Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Nishanth Menon]

On 20:20-20240108, Jon Humphreys wrote:
[...]

> > +config TI_EVM_FDT_FOLDER_PATH
> > +	string "Location of Folder path where dtb is present"
> > +	default "ti/davinci" if ARCH_DAVINCI
> > +	default "ti/keystone" if ARCH_KEYSTONE
> > +	default "ti/omap" if ARCH_OMAP2PLUS
> > +	default "ti" if ARCH_K3
> > +	depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> > +	help
> > +	   Folder path for kernel device tree default.
> > +	   This is used along with fdtfile path to locate the kernel
> > +	   device tree blob.
> 
> It's not clear to me why we need the flexibility of specifying a FDT
> filename per board independently of the FDT folder path.  Why can't the path
> be part of the fdt_map?

Because of https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=724ba6751532055db75992fc6ae21c3e322e94a7

This does not happen too often, but the folder paths are irritating and
impact multiple platforms at the same time.

[...]
> > + *
> > + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> > + */
> > +
> > +#include <env.h>
> > +#include <vsprintf.h>
> > +#include "fdt_ops.h"
> > +
> > +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)
> 
> This function takes a board name and sets the FDT name, so why isn't the
> first parameter called 'board_name' or similar?

Fair enough - board_name is more appropriate.
> 
> > +{
> > +	char *fdt_file_name = NULL;
> > +	char fdtfile[TI_FDT_FILE_MAX];
> > +
> > +	if (name_fdt) {
> > +		while (fdt_map) {
> > +			/* Check for NULL terminator in the list */
> > +			if (!fdt_map->name_fdt)
> > +				break;
> > +			if (!strncmp(fdt_map->name_fdt, name_fdt, TI_NAME_FDT_MAX)) {
> 
> Why do we need a max length?  Shouldn't strcmp() be fine given the
> name_fdt member of the fdt_map is set in code (ie, not read in)?

prefer strncmp to strcmp where possible. it isn't a matter of set in
code or not, it is a possibility of making a mistake - hence the
constant sized string.

> 
> > +				fdt_file_name = fdt_map->fdt_file_name;
> > +				break;
> > +			}
> > +			fdt_map++;
> > +		}
> > +	}
> > +
> > +	/* match not found OR null name_fdt */
> > +	if (!fdt_file_name) {
> > +		/*
> > +		 * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> > +		 * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> > +		 */
> > +#ifdef CONFIG_DEFAULT_FDT_FILE
> > +		if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> > +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> > +				 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_FDT_FILE);
> 
> I do not see where any TI platforms set CONFIG_DEFAULT_FDT_FILE, so why
> have logic that checks for it?  We don't use it.  With this patch
> (fdt_map) I don't see why we would start needing it in the future.

You don't need to explicitly set since it is already set by default - check the
.config.

> 
> > +		} else
> > +#endif
> > +		{
> > +			snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> > +				 CONFIG_TI_EVM_FDT_FOLDER_PATH,
> > +				 CONFIG_DEFAULT_DEVICE_TREE);
> 
> If fdtfile isn't set, EFI bootmeth falls back to the control DT anyway,
> so this is unnecessary duplication of logic.

Wrong translation of what is going on here - it is assuming the
fdtfile _name_ is the same as the board file used for u-boot not the
control DT content(which is the fall back for bootmeth). It is possible
that the content is the same as what U-Boot is using, but the file
name is what is being selected here.

[...]
> > +
> > +#define TI_NAME_FDT_MAX 20
> 
> TI_BOARD_NAME_MAX??

OK - will fix.
> 
> > +#define TI_FDT_FILE_MAX 200
> > +
> > +/**
> > + *  struct ti_fdt_map - mapping of device tree blob name to board name
> > + *  @name_fdt: board_name up to TI_NAME_FDT_MAX long
> 
> If this is the board_name, why call it name_fdt?  Why not board_name?

will replace with board_name
> 
> > + *  @fdt_file_name: device tree blob name as described by kernel
> > + */
> > +struct ti_fdt_map {
> > +	const char *name_fdt;
> > +	char *fdt_file_name;
> > +};
> > +
> > +/**
> > + * ti_set_fdt_env  - Find the correct device tree file name and set
> >     'fdtfile'
> 
> "Find the correct device tree file name based on the board name and "...

OK.

> 
> > + * env variable with correct folder structure appropriate to the architecture
> > + * and kernel conventions. This function is invoked typically as part of
> > + * board_late_init
> > + *
> > + * fdt name is picked by:
> > + * a) If a match is found, use the match
> 
> "a) If a board name match is found, use the match"

OK.

[...]

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

On 20:24-20240108, Jon Humphreys wrote:
> Nishanth Menon <nm@ti.com> writes:
> 
> > Stop using the findfdt script and switch to setting the fdtfile from C
> > code.
> >
> > While at this, replace findfdt in environment with a warning as it is
> > no longer needed
> >
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > ---
> >  board/beagle/beagleboneai64/beagleboneai64.c   | 14 ++++++++++++++
> >  board/beagle/beagleboneai64/beagleboneai64.env |  1 -
> >  configs/j721e_beagleboneai64_a72_defconfig     |  3 ++-
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/board/beagle/beagleboneai64/beagleboneai64.c b/board/beagle/beagleboneai64/beagleboneai64.c
> > index c8c1c78ae5a2..1982f738b04e 100644
> > --- a/board/beagle/beagleboneai64/beagleboneai64.c
> > +++ b/board/beagle/beagleboneai64/beagleboneai64.c
> > @@ -28,3 +28,17 @@ int dram_init_banksize(void)
> >  {
> >  	return fdtdec_setup_memory_banksize();
> >  }
> > +
> > +#ifdef CONFIG_BOARD_LATE_INIT
> > +int board_late_init(void)
> > +{
> > +	char fdtfile[50];
> > +
> > +	snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> > +		 CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
> 
> This would set the board to using the control DT, not boot DT.  Is that
> what you meant?

control DT file name. not the content.

> But anyway, why not just hard code the FDT path/name here since there is
> only one for this board?  I don't see the value in the extra logic of
> using the config values (or having a fdt_map).  (Same for beagleplay)

because there are potentially variants being privately discussed for both the
platforms - though the timelines are unknown at this point. This will
allow fragments to replace the DEFAULT_DEVICE_TREE to the variant and
rest of the logic will *just work*.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 02/10] board: ti: am62ax: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

On 20:48-20240108, Jon Humphreys wrote:
[..]

> > +
> > +#ifdef CONFIG_BOARD_LATE_INIT
> > +int board_late_init(void)
> > +{
> > +	ti_set_fdt_env(NULL, NULL);
> 
> If a board only has one FDT possible, why not just hard code the path
> here, rather than use the set_fdt_env() logic with NULLs and create/set
> TI_EVM_FDT_FOLDER_PATH config that is only used here anyway?

Centralizing the logic has the additional benefit of standardizing
fall back when maps are not matched. It is better than having each of
the board file introduce random way of doing things.

> 
> If you think we might add additional board types / FDTs later, then
> instead let's formalize the fdt_map concept and create a CONFIG_USES_FDT_MAP
> that will then rely on the fdt_map and call the ti_set_fdt_env(), and if
> not set, then it just returns a hard coded value, which could be based on
> the CONFIG_DEFAULT_DEVICE_TREE config.

Ends up being superflous - there is already a push to reduce the config
options.

[..]

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 04/10] board: ti: am64x: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

On 15:08-20240109, Roger Quadros wrote:
[..]
> >  #ifdef CONFIG_BOARD_LATE_INIT
> > +static struct ti_fdt_map ti_am64_evm_fdt_map[] = {
> > +	{"am64x_gpevm", "k3-am642-evm.dtb"},
> > +	{"am64x_skevm", "k3-am642-sk.dtb"},
> 
> "am64x_gpevm" and "am64x_skevm" strings are used multiple times in this file.
> see setup_board_eeprom_env()
> 
> Please use a MACRO for them.
> 
> What is the logic of choosing this name and can it be updated at this point?
> e.g."gp" is misleading in the board name as the boards are now shipped with
> HS-FS chip and are no longer GP.

Intent of the series was to get rid of findfdt - it was not meant to
cleanup existing name usage in the files. If there is a desire to do so,
please - patches are welcome.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library

[Nishanth Menon]

On 15:08-20240109, Roger Quadros wrote:
[..]
> > +/**
> > + *  struct ti_fdt_map - mapping of device tree blob name to board name
> > + *  @name_fdt: board_name up to TI_NAME_FDT_MAX long
> > + *  @fdt_file_name: device tree blob name as described by kernel
> > + */
> > +struct ti_fdt_map {
> > +	const char *name_fdt;
> 
> Can we call this board_name? as name_fdt corresponds to device tree name.
> 

Done.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 06/10] board: ti: j721e: Set fdtfile from C code instead of findfdt script

[Roger Quadros]

On 08/01/2024 19:32, Nishanth Menon wrote:
> We now can provide a map and have the standard fdtfile variable set from
> code itself. This allows for bootstd to "just work".
> 
> While at this, replace findfdt in environment with a warning as it is no
> longer needed.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  board/ti/j721e/evm.c     |  8 ++++++++
>  board/ti/j721e/j721e.env | 10 ----------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
> index c541880107ec..ad6ef4553e04 100644
> --- a/board/ti/j721e/evm.c
> +++ b/board/ti/j721e/evm.c
> @@ -16,6 +16,7 @@
>  #include <dm.h>
>  
>  #include "../common/board_detect.h"
> +#include "../common/fdt_ops.h"
>  
>  #define board_is_j721e_som()	(board_ti_k3_is("J721EX-PM1-SOM") || \
>  				 board_ti_k3_is("J721EX-PM2-SOM"))
> @@ -424,6 +425,12 @@ void configure_serdes_sierra(void)
>  }
>  
>  #ifdef CONFIG_BOARD_LATE_INIT
> +static struct ti_fdt_map ti_j721e_evm_fdt_map[] = {
> +	{"j721e", "k3-j721e-common-proc-board.dtb"},
> +	{"j721e-sk", "k3-j721e-sk.dtb"},

You missed "j721e-eaik"

> +	{"j7200", "k3-j7200-common-proc-board.dtb"},
> +	{ /* Sentinel. */ }
> +};
>  static void setup_board_eeprom_env(void)
>  {
>  	char *name = "j721e";
> @@ -443,6 +450,7 @@ static void setup_board_eeprom_env(void)
>  
>  invalid_eeprom:
>  	set_board_info_env_am6(name);
> +	ti_set_fdt_env(name, ti_j721e_evm_fdt_map);
>  }
>  
>  static void setup_serial(void)
> diff --git a/board/ti/j721e/j721e.env b/board/ti/j721e/j721e.env
> index cb27bf5e2b24..38bfd7d49634 100644
> --- a/board/ti/j721e/j721e.env
> +++ b/board/ti/j721e/j721e.env
> @@ -7,16 +7,6 @@
>  #include <env/ti/k3_rproc.env>
>  #endif
>  
> -default_device_tree=ti/k3-j721e-common-proc-board.dtb
> -findfdt=
> -	setenv name_fdt ${default_device_tree};
> -	if test $board_name = j721e; then
> -		setenv name_fdt ti/k3-j721e-common-proc-board.dtb; fi;
> -	if test $board_name = j7200; then
> -		setenv name_fdt ti/k3-j7200-common-proc-board.dtb; fi;
> -	if test $board_name = j721e-eaik || test $board_name = j721e-sk; then
> -		setenv name_fdt ti/k3-j721e-sk.dtb; fi;
> -	setenv fdtfile ${name_fdt}
>  name_kern=Image
>  console=ttyS2,115200n8
>  args_all=setenv optargs earlycon=ns16550a,mmio32,0x02800000

-- 
cheers,
-roger

Re: [PATCH 06/10] board: ti: j721e: Set fdtfile from C code instead of findfdt script

[Nishanth Menon]

On 18:20-20240109, Roger Quadros wrote:
> 
> 
> On 08/01/2024 19:32, Nishanth Menon wrote:
> > We now can provide a map and have the standard fdtfile variable set from
> > code itself. This allows for bootstd to "just work".
> > 
> > While at this, replace findfdt in environment with a warning as it is no
> > longer needed.
> > 
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > ---
> >  board/ti/j721e/evm.c     |  8 ++++++++
> >  board/ti/j721e/j721e.env | 10 ----------
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
> > index c541880107ec..ad6ef4553e04 100644
> > --- a/board/ti/j721e/evm.c
> > +++ b/board/ti/j721e/evm.c
> > @@ -16,6 +16,7 @@
> >  #include <dm.h>
> >  
> >  #include "../common/board_detect.h"
> > +#include "../common/fdt_ops.h"
> >  
> >  #define board_is_j721e_som()	(board_ti_k3_is("J721EX-PM1-SOM") || \
> >  				 board_ti_k3_is("J721EX-PM2-SOM"))
> > @@ -424,6 +425,12 @@ void configure_serdes_sierra(void)
> >  }
> >  
> >  #ifdef CONFIG_BOARD_LATE_INIT
> > +static struct ti_fdt_map ti_j721e_evm_fdt_map[] = {
> > +	{"j721e", "k3-j721e-common-proc-board.dtb"},
> > +	{"j721e-sk", "k3-j721e-sk.dtb"},
> 
> You missed "j721e-eaik"

Nope, it is intentional - eaik (the older name) is handled as sk in
setup_board_eeprom_env board_name[1] ==> board_is_j721e_sk  provides
that abstraction[2]


[1] https://github.com/u-boot/u-boot/blob/master/board/ti/j721e/evm.c#L427
[2] https://github.com/u-boot/u-boot/blob/master/board/ti/j721e/evm.c#L23
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D