Re: [PATCH 1/2] rockchip: ringneck-px30: always reset STM32 companion controller on boot

["Heiko Stübner" <heiko@sntech.de>]

Hi Quentin,

Am Mittwoch, 25. Oktober 2023, 11:51:14 CEST schrieb Quentin Schulz:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> It's happened that glitches on the STM32_RST and STM32_BOOT lines have
> put the STM32 companion microcontroller into DFU mode making it not boot
> its FW, rendering it useless for the user.
> 
> Considering that the STM32 companion microcontroller is always reset on
> a reboot or power cycle, resetting it once again in U-Boot SPL isn't
> going to hurt it any more.
> 
> For ATtiny companion microcontroller, the situation is a bit different
> because a reboot or power cycle doesn't reset it. Additionally, since it
> can only be reset with a UPDI reset on the STM32_RST line, and that is
> virtually impossible to mistakenly trigger, the ATtiny is unlikely to be
> in unwanted reset or enter reset because U-Boot toggles STM32_RST line.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  .../ringneck_px30/ringneck-px30.c                  | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> index bb1bb4acf5c..804a991e281 100644
> --- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> +++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> @@ -16,12 +16,14 @@
>  #include <usb.h>
>  #include <dm/pinctrl.h>
>  #include <dm/uclass-internal.h>
> +#include <asm/gpio.h>
>  #include <asm/io.h>
>  #include <asm/setup.h>
>  #include <asm/arch-rockchip/clock.h>
>  #include <asm/arch-rockchip/hardware.h>
>  #include <asm/arch-rockchip/periph.h>
>  #include <asm/arch-rockchip/misc.h>
> +#include <linux/delay.h>
>  #include <power/regulator.h>
>  #include <u-boot/sha256.h>
>  
> @@ -169,3 +171,53 @@ int misc_init_r(void)
>  
>  	return 0;
>  }
> +
> +#define STM32_RST	100 // GPIO3_A4
> +#define STM32_BOOT	101 // GPIO3_A5

is "//" the preferred comment style in u-boot?
I'd somehow expect  "/* GPIO3_A4 */"


> +void spl_board_init(void)
> +{
> +	/*
> +	 * Glitches on STM32_BOOT and STM32_RST lines during poweroff or power
> +	 * on may put the STM32 companion microcontroller into DFU mode, let's
> +	 * always reset it into normal mode instead.
> +	 * Toggling the STM32_RST line is safe to do with the ATtiny companion
> +	 * microcontroller variant because it will not trigger an MCU reset
> +	 * since only a UPDI reset command will. Since a UPDI reset is difficult
> +	 * to mistakenly trigger, glitches to the lines are theoretically also
> +	 * incapable of triggering an actual ATtiny reset.
> +	 */
> +	int ret = gpio_request(STM32_RST, "STM32_RST");
> +

nit: int ret;
ret = gpio_request()

so that is connects to the if(ret) below it, like the others?

> +	if (ret) {
> +		debug("Failed to request STM32_RST\n");
> +		return;
> +	}
> +
> +	ret = gpio_request(STM32_BOOT, "STM32_BOOT");
> +	if (ret) {
> +		debug("Failed to request STM32_BOOT\n");
> +		return;
> +	}
> +
> +	// Rely on HW pull-down for inactive level

nit: /* foo */ ?

> +	ret = gpio_direction_input(STM32_BOOT);
> +	if (ret) {
> +		debug("Failed to configure STM32_BOOT\n");
> +		return;
> +	}
> +
> +	ret = gpio_direction_output(STM32_RST, 0);
> +	if (ret) {
> +		debug("Failed to configure STM32_RST\n");
> +		return;
> +	}
> +
> +	mdelay(1);
> +
> +	ret = gpio_direction_output(STM32_RST, 1);
> +	if (ret) {
> +		debug("Failed to configure STM32_RST\n");
> +		return;
> +	}
> +}


Heiko