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
