Re: [PATCH 3/3] ARM: protonic-stm32mp1: Add Add Priva E-Measuringbox board support

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

On Wed, Dec 18, 2024 at 07:09:00AM +0100, Oleksij Rempel wrote:
> diff --git a/arch/arm/boards/protonic-stm32mp13/board.c b/arch/arm/boards/protonic-stm32mp13/board.c
> new file mode 100644
> index 000000000000..199da2496486
> --- /dev/null
> +++ b/arch/arm/boards/protonic-stm32mp13/board.c
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// SPDX-FileCopyrightText: 2021 David Jander, Protonic Holland
> +// SPDX-FileCopyrightText: 2021 Oleksij Rempel, Pengutronix
> +
> +#include <bootsource.h>
> +#include <common.h>
> +#include <init.h>
> +#include <mach/stm32mp/bbu.h>
> +#include <mach/stm32mp/bsec.h>
> +#include <of_device.h>
> +#include <gpio.h>
> +#include <soc/stm32/gpio.h>
> +#include <deep-probe.h>
> +
> +#include "../../drivers/nvmem/stm32-bsec-optee-ta.h"

If we need this here, should we move this file to somewhere under
/include/?

> +
> +/* board specific flags */
> +#define PRT_STM32_BOOTSRC_SD		BIT(2)
> +#define PRT_STM32_BOOTSRC_EMMC		BIT(1)
> +#define PRT_STM32_BOOTSRC_SPI_NOR	BIT(0)
> +
> +
> +#define PRT_STM32_GPIO_HWID_PL_N	13 /* PA13 */
> +#define PRT_STM32_GPIO_HWID_CP		14 /* PA14 */
> +#define PRT_STM32_GPIO_HWID_Q7		45 /* PC13 */
> +
> +struct prt_stm32_machine_data {
> +	u32 flags;
> +};
> +
> +struct bsec_priv {
> +	struct tee_context *ctx;
> +};
> +
> +struct prt_stm32_boot_dev {
> +	char *name;
> +	char *env;
> +	char *dev;
> +	int flags;
> +	int boot_idx;
> +	enum bootsource boot_src;
> +};

...

> +
> +static int prt_stm32_read_serial(struct device *dev)
> +{
> +	u32 ser_high, ser_mid, ser_low;
> +	struct bsec_priv *priv;
> +	char serial[11];
> +	int ret;
> +
> +	priv = xzalloc(sizeof(*priv));
> +	if (!priv)
> +		return -ENOMEM;

priv is only used in this function. Better allocate statically.

> +
> +	ret = stm32_bsec_optee_ta_open(&priv->ctx);
> +	if (ret) {
> +		dev_err(dev, "Failed to open BSEC TA: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = stm32_bsec_pta_read(&priv->ctx, 60, &ser_high);
> +	if (ret)
> +		goto exit_pta_read;
> +
> +	ret = stm32_bsec_pta_read(&priv->ctx, 61, &ser_mid);
> +	if (ret)
> +		goto exit_pta_read;
> +
> +	ret = stm32_bsec_pta_read(&priv->ctx, 62, &ser_low);
> +	if (ret)
> +		goto exit_pta_read;
> +
> +	stm32_bsec_optee_ta_close(&priv->ctx);
> +
> +	ser_low = ser_low & 0xff;
> +	memcpy(serial, &ser_high, 4);
> +	memcpy(serial+4, &ser_mid, 4);
> +	memcpy(serial+8, &ser_low, 4);

Size argument should be 1, otherwise you are writing past the array.

> +	serial[10] = 0; /* Failsafe */

The serial number only seems to be 9 bytes, so should be serial[9] = 0;

> +
> +	return prt_stm32_set_serial(dev, serial);
> +
> +exit_pta_read:
> +	stm32_bsec_optee_ta_close(&priv->ctx);

Given that priv is only used in this function you should call
stm32_bsec_optee_ta_close() also in the success path.

> +	dev_err(dev, "Failed to read serial: %pe\n", ERR_PTR(ret));
> +	return ret;
> +}
> +
> +static int prt_stm32_init_shift_reg(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = gpio_direction_output(PRT_STM32_GPIO_HWID_PL_N, 1);
> +	if (ret)
> +		goto error_out;
> +
> +	ret = gpio_direction_output(PRT_STM32_GPIO_HWID_CP, 1);
> +	if (ret)
> +		goto error_out;
> +
> +	ret = gpio_direction_input(PRT_STM32_GPIO_HWID_Q7);
> +	if (ret)
> +		goto error_out;
> +
> +	__stm32_pmx_set_output_type((void __iomem *)0x50002000, 13,
> +					       STM32_PIN_OUT_PUSHPULL);
> +	__stm32_pmx_set_output_type((void __iomem *)0x50002000, 14,
> +					       STM32_PIN_OUT_PUSHPULL);
> +
> +	return 0;
> +
> +error_out:
> +	dev_err(dev, "Failed to init shift register: %pe\n", ERR_PTR(ret));
> +	return ret;
> +}
> +

...

> +ENTRY_FUNCTION(start_stm32mp133c_prihmb, r0, r1, r2)
> +{
> +	void *fdt;
> +
> +	stm32mp_cpu_lowlevel_init();
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_LL))
> +		setup_uart();

putc_ll() expands to a no-op with CONFIG_DEBUG_LL disabled, so no need
for this guard.

> +
> +	fdt = __dtb_z_stm32mp133c_prihmb_start + get_runtime_offset();
> +
> +	stm32mp1_barebox_entry(fdt);
> +}
> diff --git a/arch/arm/dts/stm32mp131.dtsi b/arch/arm/dts/stm32mp131.dtsi
> index 89a7ffcb814f..b5d5d20d252b 100644
> --- a/arch/arm/dts/stm32mp131.dtsi
> +++ b/arch/arm/dts/stm32mp131.dtsi
> @@ -13,6 +13,10 @@ memory-controller@5a003000 {
>  	};
>  };
>  
> +&bsec {
> +	barebox,provide-mac-address = <&ethernet1 0x39>;
> +};

We should likely prefer the nvmem binding over the barebox binding.

> +
>  &iwdg2 {
>  	barebox,restart-warm-bootrom;
>  };
...

> diff --git a/arch/arm/dts/stm32mp133c-prihmb.dtsi b/arch/arm/dts/stm32mp133c-prihmb.dtsi
> new file mode 100644
> index 000000000000..eefd34c25fe4
> --- /dev/null
> +++ b/arch/arm/dts/stm32mp133c-prihmb.dtsi
> @@ -0,0 +1,494 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/dts-v1/;
> +
> +// this file will be dropped after kernel upstreaming is done
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/regulator/st,stm32mp13-regulator.h>
> +#include <arm/st/stm32mp133.dtsi>
> +#include <arm/st/stm32mp13xc.dtsi>
> +#include <arm/st/stm32mp13-pinctrl.dtsi>
> +
> +/ {
> +	model = "Priva E-Measuringbox board";
> +	compatible = "pri,prihmb", "st,stm32mp133";
> +
> +	aliases {
> +		ethernet0 = &ethernet1;
> +		mdio-gpio0 = &mdio0;
> +		mmc0 = &sdmmc1;
> +		mmc1 = &sdmmc2;
> +		serial0 = &uart4;
> +		serial1 = &usart6;
> +		serial2 = &uart7;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};

The same is in the board dts also.

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 |