[RFC 1/1] arm: mach-k3: Rework spl/get_boot_device()
Anshul Dalal
anshuld at ti.com
Wed Sep 10 13:32:23 CEST 2025
Hello Wadim,
The direction of your patch looks good to me, thanks for taking the time
to refactor!
A few comments below:
On Wed Aug 13, 2025 at 3:59 PM IST, Wadim Egorov wrote:
> The current implementation requires every K3 SoC to provide its own
> get_boot_device()/get_*_bootmedia() functions which look very identical.
> Rework it using table lookups for SoC data and reduce code duplication.
>
> Signed-off-by: Wadim Egorov <w.egorov at phytec.de>
> ---
> arch/arm/mach-k3/Makefile | 2 +-
> arch/arm/mach-k3/am62x/boot.c | 129 +++++-------------
> arch/arm/mach-k3/boot-device.c | 109 +++++++++++++++
> arch/arm/mach-k3/boot-device.h | 27 ++++
> arch/arm/mach-k3/include/mach/am62_hardware.h | 1 +
> 5 files changed, 169 insertions(+), 99 deletions(-)
> create mode 100644 arch/arm/mach-k3/boot-device.c
> create mode 100644 arch/arm/mach-k3/boot-device.h
>
> diff --git a/arch/arm/mach-k3/Makefile b/arch/arm/mach-k3/Makefile
> index b2fd5810b67..7dc77328e5c 100644
> --- a/arch/arm/mach-k3/Makefile
> +++ b/arch/arm/mach-k3/Makefile
> @@ -6,7 +6,7 @@
> obj-$(CONFIG_ARM64) += arm64/
> obj-$(CONFIG_CPU_V7R) += r5/
> obj-$(CONFIG_OF_LIBFDT) += common_fdt.o
> -obj-y += common.o security.o k3-ddr.o
> +obj-y += common.o security.o k3-ddr.o boot-device.o
> obj-$(CONFIG_SOC_K3_AM62A7) += am62ax/
> obj-$(CONFIG_SOC_K3_AM62P5) += am62px/
> obj-$(CONFIG_SOC_K3_AM625) += am62x/
> diff --git a/arch/arm/mach-k3/am62x/boot.c b/arch/arm/mach-k3/am62x/boot.c
> index a3a6cda6bdb..6d458f546a6 100644
> --- a/arch/arm/mach-k3/am62x/boot.c
> +++ b/arch/arm/mach-k3/am62x/boot.c
[snip]
> @@ -141,3 +44,33 @@ const char *get_reset_reason(void)
>
> return "UNKNOWN";
> }
> +
> +struct k3_boot_map am62_boot_device_primary_table[] = {
> + { BOOT_DEVICE_OSPI, 0, 0, 0, BOOT_DEVICE_SPI },
> + { BOOT_DEVICE_QSPI, 0, 0, 0, BOOT_DEVICE_SPI },
> + { BOOT_DEVICE_XSPI, 0, 0, 0, BOOT_DEVICE_SPI },
> + { BOOT_DEVICE_SPI, 0, 0, 0, BOOT_DEVICE_SPI },
> + { BOOT_DEVICE_ETHERNET_RGMII, 0, 0, 0, BOOT_DEVICE_ETHERNET },
> + { BOOT_DEVICE_ETHERNET_RMII, 0, 0, 0, BOOT_DEVICE_ETHERNET },
> + { BOOT_DEVICE_EMMC, 0, 0, 0, BOOT_DEVICE_MMC1 },
> + { BOOT_DEVICE_MMC, MAIN_DEVSTAT_PRIMARY_MMC_PORT_MASK, MAIN_DEVSTAT_PRIMARY_MMC_PORT_SHIFT, 1, BOOT_DEVICE_MMC2 },
> + { BOOT_DEVICE_MMC, MAIN_DEVSTAT_PRIMARY_MMC_PORT_MASK, MAIN_DEVSTAT_PRIMARY_MMC_PORT_SHIFT, 0, BOOT_DEVICE_MMC1 },
> + { BOOT_DEVICE_DFU, MAIN_DEVSTAT_PRIMARY_USB_MODE_MASK, MAIN_DEVSTAT_PRIMARY_USB_MODE_SHIFT, 1, BOOT_DEVICE_USB },
> + { BOOT_DEVICE_DFU, MAIN_DEVSTAT_PRIMARY_USB_MODE_MASK, MAIN_DEVSTAT_PRIMARY_USB_MODE_SHIFT, 0, BOOT_DEVICE_DFU },
> + { BOOT_DEVICE_NOBOOT, 0, 0, 0, BOOT_DEVICE_RAM },
> +};
> +
> +struct k3_boot_map am62_boot_device_backup_table[] = {
> + { BACKUP_BOOT_DEVICE_UART, 0, 0, 0, BOOT_DEVICE_UART },
> + { BACKUP_BOOT_DEVICE_USB, 0, 0, 0, BOOT_DEVICE_USB },
> + { BACKUP_BOOT_DEVICE_ETHERNET, 0, 0, 0, BOOT_DEVICE_ETHERNET },
> + { BACKUP_BOOT_DEVICE_MMC, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_MASK, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_SHIFT, 0, BOOT_DEVICE_MMC1 },
> + { BACKUP_BOOT_DEVICE_MMC, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_MASK, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_SHIFT, 1, BOOT_DEVICE_MMC2 },
> + { BACKUP_BOOT_DEVICE_SPI, 0, 0, 0, BOOT_DEVICE_SPI },
> + { BACKUP_BOOT_DEVICE_I2C, 0, 0, 0, BOOT_DEVICE_I2C },
> + { BACKUP_BOOT_DEVICE_DFU, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_MASK, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_SHIFT, 0, BOOT_DEVICE_DFU },
> + { BACKUP_BOOT_DEVICE_DFU, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_MASK, MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_SHIFT, 1, BOOT_DEVICE_USB },
> +};
> +
> +unsigned int am62_boot_device_primary_table_count = ARRAY_SIZE(am62_boot_device_primary_table);
> +unsigned int am62_boot_device_backup_table_count = ARRAY_SIZE(am62_boot_device_backup_table);
It might be better to have boot-device.h export an extern that the
boards can set. Something like this:
boot-device.h:
extern struct k3_boot_device_info *info;
am62x/boot.c:
static const struct k3_boot_device_info am62_map = {
.boot_device_primary_table = am62_boot_device_primary_table,
.boot_device_primary_count = ARRAY_SIZE(am62_boot_device_primary_table),
.boot_device_backup_table = am62_boot_device_backup_table,
.boot_device_backup_count = ARRAY_SIZE(am62_boot_device_backup_table),
.main_devstat_reg = (void __iomem *)CTRLMMR_MAIN_DEVSTAT,
.wkup_devstat_reg = 0,
.bootmode_addr = (void __iomem *)K3_BOOT_PARAM_TABLE_INDEX_OCRAM,
};
struct k3_boot_device_info *info = &am62_map;
This would allow you to remove the SOC specifics ifdefs from
boot-device.c below.
> diff --git a/arch/arm/mach-k3/boot-device.c b/arch/arm/mach-k3/boot-device.c
> new file mode 100644
> index 00000000000..9b4f0ae7aa6
> --- /dev/null
> +++ b/arch/arm/mach-k3/boot-device.c
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2025 PHYTEC Messtechnik GmbH
> + * Author: Wadim Egorov <w.egorov at phytec.de>
> + */
> +
> +#include "boot-device.h"
> +
> +#include <asm/io.h>
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/am62_spl.h>
> +
> +struct k3_boot_device_info {
> + struct k3_boot_map *boot_device_primary_table;
> + unsigned int *boot_device_primary_count;
> + struct k3_boot_map *boot_device_backup_table;
> + unsigned int *boot_device_backup_count;
> + void __iomem *main_devstat_reg;
> + void __iomem *wkup_devstat_reg;
> + void __iomem *bootmode_addr;
> +};
> +
> +static const struct k3_boot_device_info k3_boot_device_info[] = {
> +#if defined(CONFIG_SOC_K3_AM625)
> + {
> + .boot_device_primary_table = am62_boot_device_primary_table,
> + .boot_device_primary_count = &am62_boot_device_primary_table_count,
> + .boot_device_backup_table = am62_boot_device_backup_table,
> + .boot_device_backup_count = &am62_boot_device_backup_table_count,
> + .main_devstat_reg = (void __iomem *)CTRLMMR_MAIN_DEVSTAT,
> + .wkup_devstat_reg = 0,
> + .bootmode_addr = (void __iomem *)K3_BOOT_PARAM_TABLE_INDEX_OCRAM,
> + },
> +#endif
> +};
Why use an array here? We are only making use of the first entry
anyways.
> +
> +static u32 __get_backup_bootmedia(u32 devstat)
> +{
> + u32 bkup_bootmode = (devstat & MAIN_DEVSTAT_BACKUP_BOOTMODE_MASK) >>
> + MAIN_DEVSTAT_BACKUP_BOOTMODE_SHIFT;
> + u32 bkup_bootmode_cfg = (devstat & MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_MASK) >>
> + MAIN_DEVSTAT_BACKUP_BOOTMODE_CFG_SHIFT;
> +
> + const struct k3_boot_device_info *info = &k3_boot_device_info[0];
> + unsigned int i;
> +
> + for (i = 0; i < *info->boot_device_backup_count; i++) {
> + struct k3_boot_map *m = &info->boot_device_backup_table[i];
> +
> + if (bkup_bootmode != m->mode)
> + continue;
> +
> + if (m->cfg_mask == 0)
> + return m->result;
> +
> + if (((bkup_bootmode_cfg & m->cfg_mask) >> m->cfg_shift) == m->cfg_value)
> + return m->result;
> + }
> +
> + return BOOT_DEVICE_RAM;
> +}
> +
> +static u32 __get_primary_bootmedia(u32 devstat)
> +{
> + u32 bootmode = (devstat & MAIN_DEVSTAT_PRIMARY_BOOTMODE_MASK) >>
> + MAIN_DEVSTAT_PRIMARY_BOOTMODE_SHIFT;
> + u32 bootmode_cfg = (devstat & MAIN_DEVSTAT_PRIMARY_BOOTMODE_CFG_MASK) >>
> + MAIN_DEVSTAT_PRIMARY_BOOTMODE_CFG_SHIFT;
> + const struct k3_boot_device_info *info = &k3_boot_device_info[0];
> + unsigned int i;
> +
> + for (i = 0; i < *info->boot_device_primary_count; i++) {
> + struct k3_boot_map *m = &info->boot_device_primary_table[i];
> +
> + if (bootmode != m->mode)
> + continue;
> +
> + if (m->cfg_mask == 0)
> + return m->result;
> +
> + if (((bootmode_cfg & m->cfg_mask) >> m->cfg_shift) == m->cfg_value)
> + return m->result;
> + }
> +
> + return bootmode;
> +}
> +
> +u32 get_boot_device(void)
> +{
This might be better kept as a weak function.
> + const struct k3_boot_device_info *info = &k3_boot_device_info[0];
> + u32 bootmode = readl(info->bootmode_addr);
> + u32 main_devstat = readl(info->main_devstat_reg);
> + u32 bootmedia;
> +
> + if (!ARRAY_SIZE(k3_boot_device_info)) {
> + pr_err("%s: no boot-device info for this SoC\n", __func__);
> + return BOOT_DEVICE_NOBOOT;
> + }
> +
> + if (bootmode == K3_PRIMARY_BOOTMODE)
> + bootmedia = __get_primary_bootmedia(main_devstat);
> + else
> + bootmedia = __get_backup_bootmedia(main_devstat);
> +
> + debug("%s: devstat = 0x%x bootmedia = 0x%x bootmode = %d\n",
> + __func__, main_devstat, bootmedia, bootmode);
> +
> + return bootmedia;
> +}
> diff --git a/arch/arm/mach-k3/boot-device.h b/arch/arm/mach-k3/boot-device.h
> new file mode 100644
> index 00000000000..351f710151b
> --- /dev/null
> +++ b/arch/arm/mach-k3/boot-device.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2025 PHYTEC Messtechnik GmbH
> + * Author: Wadim Egorov <w.egorov at phytec.de>
> + */
> +
> +#ifndef _K3_BOOT_DEVICE_H
> +#define _K3_BOOT_DEVICE_H
> +
> +#include <linux/types.h>
> +
> +/* Descriptor for mode + optional cfg test -> normalized device */
> +struct k3_boot_map {
> + u32 mode; /* Raw PRIMARY_BOOTMODE value */
> + u32 cfg_mask; /* Bits in devstat-cfg to test (0 if none) */
> + u32 cfg_shift; /* Shift right before comparing */
> + u32 cfg_value; /* Desired value after mask+shift */
> + u32 result; /* Normalized device to return */
> +};
> +
> +extern struct k3_boot_map am62_boot_device_primary_table[];
> +extern unsigned int am62_boot_device_primary_table_count;
> +
> +extern struct k3_boot_map am62_boot_device_backup_table[];
> +extern unsigned int am62_boot_device_backup_table_count;
> +
You wouldn't need these 4 externs per SoC too with the above suggestion.
Regards,
Anshul
> +#endif /* _K3_BOOT_DEVICE_H */
> diff --git a/arch/arm/mach-k3/include/mach/am62_hardware.h b/arch/arm/mach-k3/include/mach/am62_hardware.h
> index 2f5655bf24a..084f16bf2d2 100644
> --- a/arch/arm/mach-k3/include/mach/am62_hardware.h
> +++ b/arch/arm/mach-k3/include/mach/am62_hardware.h
> @@ -10,6 +10,7 @@
> #define __ASM_ARCH_AM62_HARDWARE_H
>
> #include <config.h>
> +#include <asm/io.h>
> #ifndef __ASSEMBLY__
> #include <linux/bitops.h>
> #endif
More information about the U-Boot
mailing list