Re: [PATCH 2/3] ARM: stm32mp: add Linux Automation TAC board

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Apr 11, 2025 at 09:27:47AM +0200, Ahmad Fatoum wrote:
> Add support for the Linux Automation GmbH Test Automation
> Controller (TAC). The board boots from eMMC, but requires USB loading if
> no previous barebox has been put into the eMMC boot partition.
> 
> Signed-off-by: Leonard Göhrs <l.goehrs@pengutronix.de>
> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/boards/Makefile                  |   1 +
>  arch/arm/boards/lxa-tac/Makefile          |   2 +
>  arch/arm/boards/lxa-tac/board.c           |  33 ++++++
>  arch/arm/boards/lxa-tac/lowlevel.c        |  71 ++++++++++++
>  arch/arm/configs/multi_v7_defconfig       |   1 +
>  arch/arm/configs/stm32mp_defconfig        |   1 +
>  arch/arm/dts/Makefile                     |   2 +
>  arch/arm/dts/stm32mp153c-lxa-tac-gen3.dts |  38 ++++++
>  arch/arm/dts/stm32mp157c-lxa-tac-gen1.dts |   7 ++
>  arch/arm/dts/stm32mp157c-lxa-tac-gen2.dts |  38 ++++++
>  arch/arm/dts/stm32mp15xc-lxa-tac.dtsi     | 134 ++++++++++++++++++++++
>  arch/arm/mach-stm32mp/Kconfig             |   4 +
>  images/Makefile.stm32mp                   |   2 +
>  include/mach/stm32mp/stm32.h              |   3 +
>  14 files changed, 337 insertions(+)
>  create mode 100644 arch/arm/boards/lxa-tac/Makefile
>  create mode 100644 arch/arm/boards/lxa-tac/board.c
>  create mode 100644 arch/arm/boards/lxa-tac/lowlevel.c
>  create mode 100644 arch/arm/dts/stm32mp153c-lxa-tac-gen3.dts
>  create mode 100644 arch/arm/dts/stm32mp157c-lxa-tac-gen1.dts
>  create mode 100644 arch/arm/dts/stm32mp157c-lxa-tac-gen2.dts
>  create mode 100644 arch/arm/dts/stm32mp15xc-lxa-tac.dtsi
> 
> diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
> index 1e67b304f185..187bfed46f91 100644
> --- a/arch/arm/boards/Makefile
> +++ b/arch/arm/boards/Makefile
> @@ -126,6 +126,7 @@ obj-$(CONFIG_MACH_SOLIDRUN_MICROSOM)		+= solidrun-microsom/
>  obj-$(CONFIG_MACH_STM32MP15XX_DKX)		+= stm32mp15xx-dkx/
>  obj-$(CONFIG_MACH_STM32MP13XX_DK)		+= stm32mp13xx-dk/
>  obj-$(CONFIG_MACH_LXA_MC1)			+= lxa-mc1/
> +obj-$(CONFIG_MACH_LXA_TAC)			+= lxa-tac/
>  obj-$(CONFIG_MACH_STM32MP15X_EV1)		+= stm32mp15x-ev1/
>  obj-$(CONFIG_MACH_TECHNEXION_PICO_HOBBIT)	+= technexion-pico-hobbit/
>  obj-$(CONFIG_MACH_TECHNEXION_WANDBOARD)		+= technexion-wandboard/
> diff --git a/arch/arm/boards/lxa-tac/Makefile b/arch/arm/boards/lxa-tac/Makefile
> new file mode 100644
> index 000000000000..092c31d6b28d
> --- /dev/null
> +++ b/arch/arm/boards/lxa-tac/Makefile
> @@ -0,0 +1,2 @@
> +lwl-y += lowlevel.o
> +obj-y += board.o
> diff --git a/arch/arm/boards/lxa-tac/board.c b/arch/arm/boards/lxa-tac/board.c
> new file mode 100644
> index 000000000000..96ffc46500bf
> --- /dev/null
> +++ b/arch/arm/boards/lxa-tac/board.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <common.h>
> +#include <linux/sizes.h>
> +#include <init.h>
> +#include <asm/memory.h>
> +#include <mach/stm32mp/bbu.h>
> +#include <bootsource.h>
> +#include <deep-probe.h>
> +#include <of.h>
> +
> +static int tac_probe(struct device *dev)
> +{
> +	barebox_set_hostname("lxatac");
> +
> +	stm32mp_bbu_mmc_fip_register("mmc", "/dev/mmc1", BBU_HANDLER_FLAG_DEFAULT);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tac_of_match[] = {
> +	{ .compatible = "lxa,stm32mp157c-tac-gen1" },
> +	{ .compatible = "lxa,stm32mp157c-tac-gen2" },
> +	{ .compatible = "lxa,stm32mp153c-tac-gen3" },
> +	{ /* sentinel */ },
> +};
> +BAREBOX_DEEP_PROBE_ENABLE(tac_of_match);
> +
> +static struct driver tac_board_driver = {
> +	.name = "board-lxa-tac",
> +	.probe = tac_probe,
> +	.of_compatible = tac_of_match,
> +};
> +late_platform_driver(tac_board_driver);
> diff --git a/arch/arm/boards/lxa-tac/lowlevel.c b/arch/arm/boards/lxa-tac/lowlevel.c
> new file mode 100644
> index 000000000000..eee7f8c0cead
> --- /dev/null
> +++ b/arch/arm/boards/lxa-tac/lowlevel.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <common.h>
> +#include <debug_ll.h>
> +#include <mach/stm32mp/entry.h>
> +#include <mach/stm32mp/stm32.h>
> +#include <soc/stm32/gpio.h>
> +
> +extern char __dtb_z_stm32mp157c_lxa_tac_gen1_start[];
> +extern char __dtb_z_stm32mp157c_lxa_tac_gen2_start[];
> +extern char __dtb_z_stm32mp153c_lxa_tac_gen3_start[];
> +
> +/*
> + * Major board generation is set via traces in copper
> + * Minor board generation can be changed via resistors.
> + * The revision is available on GPIOs:
> + * [PZ0, PZ1, PZ2, PZ3, PZ6, PZ7]
> + */
> +#define BOARD_GEN(major, minor)	(((major) << 2) | minor)
> +#define BOARD_GEN1		BOARD_GEN(0, 0)
> +#define BOARD_GEN2		BOARD_GEN(1, 0)
> +#define BOARD_GEN3		BOARD_GEN(2, 0)
> +
> +static const int board_rev_pins[] = {0, 1, 2, 3, 6, 7};
> +
> +static u32 get_board_rev(void)
> +{
> +	u32 board_rev = 0;
> +
> +	/* Enable GPIOZ bank */
> +	setbits_le32(STM32MP15_RCC_MP_AHB5ENSETR, BIT(0));
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(board_rev_pins); i++) {
> +		int pin = board_rev_pins[i];
> +
> +		__stm32_pmx_gpio_input(STM32MP15_GPIOZ_BASE, pin);
> +		board_rev |= __stm32_pmx_gpio_get(STM32MP15_GPIOZ_BASE, pin) << i;
> +	}
> +
> +	return board_rev;
> +}

Is this information available during runtime with some pr_* message or
variable?

> +
> +static void noinline select_fdt_and_start(void *fdt)
> +{
> +	putc_ll('>');
> +
> +	switch (get_board_rev()) {
> +	case BOARD_GEN1:
> +		fdt = runtime_address(__dtb_z_stm32mp157c_lxa_tac_gen1_start);
> +		break;
> +	case BOARD_GEN2:
> +		fdt = runtime_address(__dtb_z_stm32mp157c_lxa_tac_gen2_start);
> +		break;
> +	case BOARD_GEN3:
> +		fdt = runtime_address(__dtb_z_stm32mp153c_lxa_tac_gen3_start);
> +		break;
> +	}

This check only matches for minor revision 0. Shouldn't the minor
version rather be ignored here?

> +
> +	stm32mp1_barebox_entry(fdt);
> +}
> +
> +ENTRY_FUNCTION(start_stm32mp15xc_lxa_tac, r0, r1, r2)
> +{
> +	stm32mp_cpu_lowlevel_init();
> +
> +	/*
> +	 * stm32mp_cpu_lowlevel_init sets up a stack. Do the remaining
> +	 * init in a non-naked function. Register r2 points to the fdt
> +	 * from the FIT image which can be used as a default.
> +	 */

"the fdt from the FIT image" seems to imply that this entry is only used
as a 2nd state entry, but it's used as a 1st stage entry as well, isn't
it?

I would rephrase that to something like "in case of a 2nd stage boot r2
points to the fdt..."

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |