Re: [RFC 1/1] arm: mach-k3: Rework spl/get_boot_device()

[Anshul Dalal <anshuld@ti.com>]

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@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@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@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