[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