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

[Jon Humphreys]

Nishanth Menon <nm at 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 at 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