diff mbox series

rockchip: ringneck-px30: put STM32_RST line in input mode instead of output

Message ID 20240209-ringneck-stm32-rst-input-v1-1-fa9d23ac8ebe@theobroma-systems.com
State Accepted
Delegated to: Kever Yang
Headers show
Series rockchip: ringneck-px30: put STM32_RST line in input mode instead of output | expand

Commit Message

Quentin Schulz Feb. 9, 2024, 1:18 p.m. UTC 
From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

The STM32_RST line is routed to the ATtiny microcontroller
PA0/RESET/UPDI pin. By driving the PX30 SoC pin as GPIO output high, we
prevent external UPDI to be used for flashing without first putting this
pin as GPIO input, an extra step we could avoid in userspace.

There's an external hardware pull-up strong enough to keep the STM32_RST
state high on ATtiny side but weak enough it can be overridden by
external UPDI. This also means it is safe to use for the STM32 variant,
where STM32_RST line will be in the same state as if output high was
used.

The Q7 standard specifies that MFG_NC1 and MFG_NC2 (used for UPDI for
Ringneck) pins should neither be driven by the carrierboard, nor have
pull-up or pull-down resistors. This means this commit is safe to use
regardless of the carrierboard this module would be connected to
(provided it follows the Q7 standard).

Fixes: 6acdd63e8771 ("rockchip: ringneck-px30: always reset STM32 companion controller on boot")
Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 board/theobroma-systems/ringneck_px30/ringneck-px30.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


---
base-commit: a4650bf65e4b7d3ef04c90ba8031374428e4a682
change-id: 20240209-ringneck-stm32-rst-input-ca1cdfa67ff5

Best regards,

Comments

Kever Yang Feb. 18, 2024, 1:14 a.m. UTC | #1 
Hi Quentin,

On 2024/2/9 21:18, Quentin Schulz wrote:
> From: Quentin Schulz<quentin.schulz@theobroma-systems.com>
>
> The STM32_RST line is routed to the ATtiny microcontroller
> PA0/RESET/UPDI pin. By driving the PX30 SoC pin as GPIO output high, we
> prevent external UPDI to be used for flashing without first putting this
> pin as GPIO input, an extra step we could avoid in userspace.

A little confuse here, this GPIO is an output for PX30, right?So the 
config is:

1. the PX30 SPL init STM32_RST as input, with hardware pull-up the keep 
STM32 work;

2. when need UPDI for flashing, need to set STM32_RST to output and 
trigger the reset in userspace?


>
> There's an external hardware pull-up strong enough to keep the STM32_RST
> state high on ATtiny side but weak enough it can be overridden by
> external UPDI. This also means it is safe to use for the STM32 variant,
> where STM32_RST line will be in the same state as if output high was
> used.
>
> The Q7 standard specifies that MFG_NC1 and MFG_NC2 (used for UPDI for
> Ringneck) pins should neither be driven by the carrierboard, nor have
> pull-up or pull-down resistors.

This sounds like it should use output and pull-none for these GPIOs?


Thanks,

- Kever

> This means this commit is safe to use
> regardless of the carrierboard this module would be connected to
> (provided it follows the Q7 standard).
>
> Fixes: 6acdd63e8771 ("rockchip: ringneck-px30: always reset STM32 companion controller on boot")
> Cc: Quentin Schulz<foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz<quentin.schulz@theobroma-systems.com>
> ---
>   board/theobroma-systems/ringneck_px30/ringneck-px30.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> index ff7e414303d..2ab1e7a12fa 100644
> --- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> +++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> @@ -76,9 +76,9 @@ void spl_board_init(void)
>   
>   	mdelay(1);
>   
> -	ret = gpio_direction_output(STM32_RST, 1);
> +	ret = gpio_direction_input(STM32_RST);
>   	if (ret) {
> -		debug("Failed to configure STM32_RST as output high\n");
> +		debug("Failed to configure STM32_RST as input\n");
>   		return;
>   	}
>   }
>
> ---
> base-commit: a4650bf65e4b7d3ef04c90ba8031374428e4a682
> change-id: 20240209-ringneck-stm32-rst-input-ca1cdfa67ff5
>
> Best regards,
Quentin Schulz Feb. 19, 2024, 9:50 a.m. UTC | #2 
Hi Kever,

On 2/18/24 02:14, Kever Yang wrote:
> Hi Quentin,
> 
> On 2024/2/9 21:18, Quentin Schulz wrote:
>> From: Quentin Schulz<quentin.schulz@theobroma-systems.com>
>>
>> The STM32_RST line is routed to the ATtiny microcontroller
>> PA0/RESET/UPDI pin. By driving the PX30 SoC pin as GPIO output high, we
>> prevent external UPDI to be used for flashing without first putting this
>> pin as GPIO input, an extra step we could avoid in userspace.
> 
> A little confuse here, this GPIO is an output for PX30, right?So the 
> config is:
> 
> 1. the PX30 SPL init STM32_RST as input, with hardware pull-up the keep 
> STM32 work;
> 

The pin needs to be high for STM32, and high **but not driven** for 
ATtiny in order to allow flashing scripts to work.

> 2. when need UPDI for flashing, need to set STM32_RST to output and 
> trigger the reset in userspace?
> 

For STM32, STM32_RST needs to be driven low, then STM32_BOOT needs to be 
driven high, then STM32_RST needs to be high to deassert reset.

For ATtiny, STM32_RST needs to be NOT driven so that UPDI lines can be 
used to interact with the MCU. Note that we also have the ability to do 
bitbang UPDI on that STM32_RST pin but that's another topic (just 
explaining why it is routed while seemingly useless for ATtiny).

All the above is for entering the flashing mode.

However, in U-Boot we do NOT want to enter flashing mode, we want to 
exit it, c.f. 
https://lore.kernel.org/u-boot/20231103-ringneck-stm32-reset-v2-1-a0e5559f89bf@theobroma-systems.com/

An external HW pull-up is required because of glitches on the line when 
powering up/down. However, this is only done on newer versions of the 
PCB, so we need to tackle old versions.

The old versions do not have this external HW pull-up and the glitch may 
cause the MCU to enter its flashing mode. Therefore, we force it to exit 
the flashing mode by always hard resetting it into the normal runtime 
mode. This is what spl_board_init() does. STM32_RST and STM32_BOOT are 
controlled by our flashing script for the STM32 variant for the MCU, so 
the default state when entering the Linux kernel doesn't matter. For the 
ATtiny MCU variant, we do not handle those GPIOs as part of the flashing 
script, therefore the default state when entering the Linux kernel 
should be the expected value for which we can use UPDI to flash the 
ATtiny. For ATtiny, the reset line is shared between STM32_RST that goes 
to the SoC and the UPDI lines exposed over the Q7 header. If STM32_RST 
is driven by the SoC, the UPDI lines won't be able to interact with the 
MCU. Therefore it needs to be put into input mode, whether in U-Boot or 
in Linux userspace.



> 
>>
>> There's an external hardware pull-up strong enough to keep the STM32_RST
>> state high on ATtiny side but weak enough it can be overridden by
>> external UPDI. This also means it is safe to use for the STM32 variant,
>> where STM32_RST line will be in the same state as if output high was
>> used.
>>
>> The Q7 standard specifies that MFG_NC1 and MFG_NC2 (used for UPDI for
>> Ringneck) pins should neither be driven by the carrierboard, nor have
>> pull-up or pull-down resistors.
> 
> This sounds like it should use output and pull-none for these GPIOs?
> 

This would be a configuration for the Linux kernel DT, but we would 
still need some custom handling in U-Boot. What are you suggesting 
exactly with this?

Cheers,
Quentin
Quentin Schulz March 22, 2024, 9:38 a.m. UTC | #3 
Hi Kever,

On 2/19/24 10:50, Quentin Schulz wrote:
> Hi Kever,
> 
> On 2/18/24 02:14, Kever Yang wrote:
>> Hi Quentin,
>>
>> On 2024/2/9 21:18, Quentin Schulz wrote:
>>> From: Quentin Schulz<quentin.schulz@theobroma-systems.com>
>>>
>>> The STM32_RST line is routed to the ATtiny microcontroller
>>> PA0/RESET/UPDI pin. By driving the PX30 SoC pin as GPIO output high, we
>>> prevent external UPDI to be used for flashing without first putting this
>>> pin as GPIO input, an extra step we could avoid in userspace.
>>
>> A little confuse here, this GPIO is an output for PX30, right?So the 
>> config is:
>>
>> 1. the PX30 SPL init STM32_RST as input, with hardware pull-up the 
>> keep STM32 work;
>>
> 
> The pin needs to be high for STM32, and high **but not driven** for 
> ATtiny in order to allow flashing scripts to work.
> 
>> 2. when need UPDI for flashing, need to set STM32_RST to output and 
>> trigger the reset in userspace?
>>
> 
> For STM32, STM32_RST needs to be driven low, then STM32_BOOT needs to be 
> driven high, then STM32_RST needs to be high to deassert reset.
> 
> For ATtiny, STM32_RST needs to be NOT driven so that UPDI lines can be 
> used to interact with the MCU. Note that we also have the ability to do 
> bitbang UPDI on that STM32_RST pin but that's another topic (just 
> explaining why it is routed while seemingly useless for ATtiny).
> 
> All the above is for entering the flashing mode.
> 
> However, in U-Boot we do NOT want to enter flashing mode, we want to 
> exit it, c.f. 
> https://lore.kernel.org/u-boot/20231103-ringneck-stm32-reset-v2-1-a0e5559f89bf@theobroma-systems.com/
> 
> An external HW pull-up is required because of glitches on the line when 
> powering up/down. However, this is only done on newer versions of the 
> PCB, so we need to tackle old versions.
> 
> The old versions do not have this external HW pull-up and the glitch may 
> cause the MCU to enter its flashing mode. Therefore, we force it to exit 
> the flashing mode by always hard resetting it into the normal runtime 
> mode. This is what spl_board_init() does. STM32_RST and STM32_BOOT are 
> controlled by our flashing script for the STM32 variant for the MCU, so 
> the default state when entering the Linux kernel doesn't matter. For the 
> ATtiny MCU variant, we do not handle those GPIOs as part of the flashing 
> script, therefore the default state when entering the Linux kernel 
> should be the expected value for which we can use UPDI to flash the 
> ATtiny. For ATtiny, the reset line is shared between STM32_RST that goes 
> to the SoC and the UPDI lines exposed over the Q7 header. If STM32_RST 
> is driven by the SoC, the UPDI lines won't be able to interact with the 
> MCU. Therefore it needs to be put into input mode, whether in U-Boot or 
> in Linux userspace.
> 

Can we have this in the next merge request for next please? Or maybe 
there's something I need to change here?

Cheers,
Quentin
Kever Yang March 26, 2024, 9:17 a.m. UTC | #4 
Hi Quentin,

On 2024/3/22 17:38, Quentin Schulz wrote:
> Hi Kever,
>
> On 2/19/24 10:50, Quentin Schulz wrote:
>> Hi Kever,
>>
>> On 2/18/24 02:14, Kever Yang wrote:
>>> Hi Quentin,
>>>
>>> On 2024/2/9 21:18, Quentin Schulz wrote:
>>>> From: Quentin Schulz<quentin.schulz@theobroma-systems.com>
>>>>
>>>> The STM32_RST line is routed to the ATtiny microcontroller
>>>> PA0/RESET/UPDI pin. By driving the PX30 SoC pin as GPIO output 
>>>> high, we
>>>> prevent external UPDI to be used for flashing without first putting 
>>>> this
>>>> pin as GPIO input, an extra step we could avoid in userspace.
>>>
>>> A little confuse here, this GPIO is an output for PX30, right?So the 
>>> config is:
>>>
>>> 1. the PX30 SPL init STM32_RST as input, with hardware pull-up the 
>>> keep STM32 work;
>>>
>>
>> The pin needs to be high for STM32, and high **but not driven** for 
>> ATtiny in order to allow flashing scripts to work.
>>
>>> 2. when need UPDI for flashing, need to set STM32_RST to output and 
>>> trigger the reset in userspace?
>>>
>>
>> For STM32, STM32_RST needs to be driven low, then STM32_BOOT needs to 
>> be driven high, then STM32_RST needs to be high to deassert reset.
>>
>> For ATtiny, STM32_RST needs to be NOT driven so that UPDI lines can 
>> be used to interact with the MCU. Note that we also have the ability 
>> to do bitbang UPDI on that STM32_RST pin but that's another topic 
>> (just explaining why it is routed while seemingly useless for ATtiny).
>>
>> All the above is for entering the flashing mode.
>>
>> However, in U-Boot we do NOT want to enter flashing mode, we want to 
>> exit it, c.f. 
>> https://lore.kernel.org/u-boot/20231103-ringneck-stm32-reset-v2-1-a0e5559f89bf@theobroma-systems.com/
>>
>> An external HW pull-up is required because of glitches on the line 
>> when powering up/down. However, this is only done on newer versions 
>> of the PCB, so we need to tackle old versions.
>>
>> The old versions do not have this external HW pull-up and the glitch 
>> may cause the MCU to enter its flashing mode. Therefore, we force it 
>> to exit the flashing mode by always hard resetting it into the normal 
>> runtime mode. This is what spl_board_init() does. STM32_RST and 
>> STM32_BOOT are controlled by our flashing script for the STM32 
>> variant for the MCU, so the default state when entering the Linux 
>> kernel doesn't matter. For the ATtiny MCU variant, we do not handle 
>> those GPIOs as part of the flashing script, therefore the default 
>> state when entering the Linux kernel should be the expected value for 
>> which we can use UPDI to flash the ATtiny. For ATtiny, the reset line 
>> is shared between STM32_RST that goes to the SoC and the UPDI lines 
>> exposed over the Q7 header. If STM32_RST is driven by the SoC, the 
>> UPDI lines won't be able to interact with the MCU. Therefore it needs 
>> to be put into input mode, whether in U-Boot or in Linux userspace.
>>
>
> Can we have this in the next merge request for next please? Or maybe 
> there's something I need to change here?

I will merge this patch next time when I send the PR.


Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever


>
> Cheers,
> Quentin
diff mbox series

Patch

diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
index ff7e414303d..2ab1e7a12fa 100644
--- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
+++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
@@ -76,9 +76,9 @@  void spl_board_init(void)
 
 	mdelay(1);
 
-	ret = gpio_direction_output(STM32_RST, 1);
+	ret = gpio_direction_input(STM32_RST);
 	if (ret) {
-		debug("Failed to configure STM32_RST as output high\n");
+		debug("Failed to configure STM32_RST as input\n");
 		return;
 	}
 }