[PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Sumit Garg]

From: Sumit Garg <sumit.garg@oss.qualcomm.com>

When debug serial port isn't connected, it is at least reported on RB1
that autoboot gets interrupted. It is probably due to random characters
on the UART RX line when disconnected. Lets try to fix this inconsistent
behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
interrupted only when the <SPACE> key gets pressed.

Reported-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
---
 configs/qcom_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig
index 36e2f45d22a..e3dac23f1d0 100644
--- a/configs/qcom_defconfig
+++ b/configs/qcom_defconfig
@@ -15,6 +15,9 @@ CONFIG_FIT_VERBOSE=y
 CONFIG_BOOTSTD_FULL=y
 # CONFIG_BOOTMETH_VBE is not set
 CONFIG_BOOTDELAY=1
+CONFIG_AUTOBOOT_KEYED=y
+CONFIG_AUTOBOOT_PROMPT="Hit <SPACE> key to stop autoboot in %2ds\n"
+CONFIG_AUTOBOOT_STOP_STR=" "
 CONFIG_OF_BOARD_SETUP=y
 CONFIG_USE_PREBOOT=y
 CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR=y
-- 
2.43.0

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Casey Connolly]

On 5/8/25 12:32, Sumit Garg wrote:
> From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> 
> When debug serial port isn't connected, it is at least reported on RB1
> that autoboot gets interrupted. It is probably due to random characters
> on the UART RX line when disconnected. Lets try to fix this inconsistent
> behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
> interrupted only when the <SPACE> key gets pressed.

Honestly I'd much rather fix the bug in the UART driver, maybe it's 
broken pinctrl?

The older UART driver (for msm8916) had a similar but that Stephen was 
able to fix with better configuration.

Anyhow, this is a sensible enough workaround if nobody wants to look 
into GENI...

Reviewed-by: Casey Connolly <casey.connolly@linaro.org>>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> ---
>   configs/qcom_defconfig | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig
> index 36e2f45d22a..e3dac23f1d0 100644
> --- a/configs/qcom_defconfig
> +++ b/configs/qcom_defconfig
> @@ -15,6 +15,9 @@ CONFIG_FIT_VERBOSE=y
>   CONFIG_BOOTSTD_FULL=y
>   # CONFIG_BOOTMETH_VBE is not set
>   CONFIG_BOOTDELAY=1
> +CONFIG_AUTOBOOT_KEYED=y
> +CONFIG_AUTOBOOT_PROMPT="Hit <SPACE> key to stop autoboot in %2ds\n"
> +CONFIG_AUTOBOOT_STOP_STR=" "
>   CONFIG_OF_BOARD_SETUP=y
>   CONFIG_USE_PREBOOT=y
>   CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR=y
-- 
Casey (she/they)

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Stephan Gerhold]

On Fri, May 09, 2025 at 12:45:20PM +0200, Casey Connolly wrote:
> On 5/8/25 12:32, Sumit Garg wrote:
> > From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > 
> > When debug serial port isn't connected, it is at least reported on RB1
> > that autoboot gets interrupted. It is probably due to random characters
> > on the UART RX line when disconnected. Lets try to fix this inconsistent
> > behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
> > interrupted only when the <SPACE> key gets pressed.
> 
> Honestly I'd much rather fix the bug in the UART driver, maybe it's broken
> pinctrl?
> 
> The older UART driver (for msm8916) had a similar but that Stephen was able
> to fix with better configuration.
> 
> Anyhow, this is a sensible enough workaround if nobody wants to look into
> GENI...
> 

Well, the main fix I did for that isn't related to the UART driver at
all, it's simply adding bias-pull-up for the RX pin so that it isn't
left floating when UART is not connected.

For RB1, qcm2290.dtsi &qup_uart4_default currently has bias-disable for
both RX and TX pins. So please try applying bias-pull-up + bootph-all
for the GPIO that belongs to RX.

For details, please see the following patch series for MSM8916:
https://lore.kernel.org/linux-arm-msm/20250422-msm8916-console-pinctrl-v2-0-f345b7a53c91@linaro.org/T/

Thanks,
Stephan

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Sumit Garg]

On Fri, May 09, 2025 at 04:54:43PM +0200, Stephan Gerhold wrote:
> On Fri, May 09, 2025 at 12:45:20PM +0200, Casey Connolly wrote:
> > On 5/8/25 12:32, Sumit Garg wrote:
> > > From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > > 
> > > When debug serial port isn't connected, it is at least reported on RB1
> > > that autoboot gets interrupted. It is probably due to random characters
> > > on the UART RX line when disconnected. Lets try to fix this inconsistent
> > > behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
> > > interrupted only when the <SPACE> key gets pressed.
> > 
> > Honestly I'd much rather fix the bug in the UART driver, maybe it's broken
> > pinctrl?
> > 
> > The older UART driver (for msm8916) had a similar but that Stephen was able
> > to fix with better configuration.
> > 
> > Anyhow, this is a sensible enough workaround if nobody wants to look into
> > GENI...
> > 
> 
> Well, the main fix I did for that isn't related to the UART driver at
> all, it's simply adding bias-pull-up for the RX pin so that it isn't
> left floating when UART is not connected.
> 
> For RB1, qcm2290.dtsi &qup_uart4_default currently has bias-disable for
> both RX and TX pins. So please try applying bias-pull-up + bootph-all
> for the GPIO that belongs to RX.
> 
> For details, please see the following patch series for MSM8916:
> https://lore.kernel.org/linux-arm-msm/20250422-msm8916-console-pinctrl-v2-0-f345b7a53c91@linaro.org/T/

Thanks for this reference but unfortunately this doesn't fix the issue
on RB1. I have tried the diff below [1]. It seems most likely due the
chainloading via ABL on RB1 as compared to U-Boot running as primary
bootloader on db410c. The RX line seems to be floating even before the
U-Boot starts which is causing the autoboot to stop. So for the
chainloading configuration, we need to enable AUTOBOOT_KEYED. I think
it should be the default sane configuration on Qcom platforms rather
than allowing autoboot stopped via any random character on RX line.

[1]

diff --git a/dts/upstream/src/arm64/qcom/qcm2290.dtsi b/dts/upstream/src/arm64/qcom/qcm2290.dtsi
index f0746123e59..9392bd6f3e0 100644
--- a/dts/upstream/src/arm64/qcom/qcm2290.dtsi
+++ b/dts/upstream/src/arm64/qcom/qcm2290.dtsi
@@ -551,10 +551,28 @@
                        };

                        qup_uart4_default: qup-uart4-default-state {
+                               tx-pins {
+                                       pins = "gpio12";
+                                       function = "qup4";
+                                       drive-strength = <16>;
+                                       bias-disable;
+                                       bootph-all;
+                               };
+
+                               rx-pins {
+                                       pins = "gpio13";
+                                       function = "qup4";
+                                       drive-strength = <16>;
+                                       bias-pull-up;
+                                       bootph-all;
+                               };
+                       };
+
+                       qup_uart4_sleep: qup-uart4-sleep-state {
                                pins = "gpio12", "gpio13";
-                               function = "qup4";
+                               function = "gpio";
                                drive-strength = <2>;
-                               bias-disable;
+                               bias-pull-down;
                        };

                        sdc1_state_on: sdc1-on-state {
@@ -1293,7 +1311,8 @@
                                clocks = <&gcc GCC_QUPV3_WRAP0_S4_CLK>;
                                clock-names = "se";
                                pinctrl-0 = <&qup_uart4_default>;
-                               pinctrl-names = "default";
+                               pinctrl-1 = <&qup_uart4_sleep>;
+                               pinctrl-names = "default", "sleep";
                                interconnects = <&qup_virt MASTER_QUP_CORE_0 RPM_ALWAYS_TAG
                                                 &qup_virt SLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
                                                <&bimc MASTER_APPSS_PROC RPM_ALWAYS_TAG


-Sumit

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Casey Connolly]

Hi Sumit,

On 5/12/25 08:01, Sumit Garg wrote:
> On Fri, May 09, 2025 at 04:54:43PM +0200, Stephan Gerhold wrote:
>> On Fri, May 09, 2025 at 12:45:20PM +0200, Casey Connolly wrote:
>>> On 5/8/25 12:32, Sumit Garg wrote:
>>>> From: Sumit Garg <sumit.garg@oss.qualcomm.com>
>>>>
>>>> When debug serial port isn't connected, it is at least reported on RB1
>>>> that autoboot gets interrupted. It is probably due to random characters
>>>> on the UART RX line when disconnected. Lets try to fix this inconsistent
>>>> behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
>>>> interrupted only when the <SPACE> key gets pressed.
>>>
>>> Honestly I'd much rather fix the bug in the UART driver, maybe it's broken
>>> pinctrl?
>>>
>>> The older UART driver (for msm8916) had a similar but that Stephen was able
>>> to fix with better configuration.
>>>
>>> Anyhow, this is a sensible enough workaround if nobody wants to look into
>>> GENI...
>>>
>>
>> Well, the main fix I did for that isn't related to the UART driver at
>> all, it's simply adding bias-pull-up for the RX pin so that it isn't
>> left floating when UART is not connected.
>>
>> For RB1, qcm2290.dtsi &qup_uart4_default currently has bias-disable for
>> both RX and TX pins. So please try applying bias-pull-up + bootph-all
>> for the GPIO that belongs to RX.
>>
>> For details, please see the following patch series for MSM8916:
>> https://lore.kernel.org/linux-arm-msm/20250422-msm8916-console-pinctrl-v2-0-f345b7a53c91@linaro.org/T/
> 
> Thanks for this reference but unfortunately this doesn't fix the issue
> on RB1. I have tried the diff below [1]. It seems most likely due the
> chainloading via ABL on RB1 as compared to U-Boot running as primary
> bootloader on db410c. The RX line seems to be floating even before the
> U-Boot starts which is causing the autoboot to stop. So for the
> chainloading configuration, we need to enable AUTOBOOT_KEYED. I think
> it should be the default sane configuration on Qcom platforms rather
> than allowing autoboot stopped via any random character on RX line.

I dug into it and the rogue inputs actually come from the button-kbd 
driver, maybe the buttons are slightly misconfigured. This explains why 
it only happens on some boards.

So the right solution here is to fix the buttons, or disable button-kbd 
if you don't need it.

Kind regards,

> 
> [1]
> 
> diff --git a/dts/upstream/src/arm64/qcom/qcm2290.dtsi b/dts/upstream/src/arm64/qcom/qcm2290.dtsi
> index f0746123e59..9392bd6f3e0 100644
> --- a/dts/upstream/src/arm64/qcom/qcm2290.dtsi
> +++ b/dts/upstream/src/arm64/qcom/qcm2290.dtsi
> @@ -551,10 +551,28 @@
>                          };
> 
>                          qup_uart4_default: qup-uart4-default-state {
> +                               tx-pins {
> +                                       pins = "gpio12";
> +                                       function = "qup4";
> +                                       drive-strength = <16>;
> +                                       bias-disable;
> +                                       bootph-all;
> +                               };
> +
> +                               rx-pins {
> +                                       pins = "gpio13";
> +                                       function = "qup4";
> +                                       drive-strength = <16>;
> +                                       bias-pull-up;
> +                                       bootph-all;
> +                               };
> +                       };
> +
> +                       qup_uart4_sleep: qup-uart4-sleep-state {
>                                  pins = "gpio12", "gpio13";
> -                               function = "qup4";
> +                               function = "gpio";
>                                  drive-strength = <2>;
> -                               bias-disable;
> +                               bias-pull-down;
>                          };
> 
>                          sdc1_state_on: sdc1-on-state {
> @@ -1293,7 +1311,8 @@
>                                  clocks = <&gcc GCC_QUPV3_WRAP0_S4_CLK>;
>                                  clock-names = "se";
>                                  pinctrl-0 = <&qup_uart4_default>;
> -                               pinctrl-names = "default";
> +                               pinctrl-1 = <&qup_uart4_sleep>;
> +                               pinctrl-names = "default", "sleep";
>                                  interconnects = <&qup_virt MASTER_QUP_CORE_0 RPM_ALWAYS_TAG
>                                                   &qup_virt SLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
>                                                  <&bimc MASTER_APPSS_PROC RPM_ALWAYS_TAG
> 
> 
> -Sumit

-- 
Casey (she/they)

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Sumit Garg]

On Tue, May 13, 2025 at 04:54:22PM +0100, Casey Connolly wrote:
> Hi Sumit,
> 
> On 5/12/25 08:01, Sumit Garg wrote:
> > On Fri, May 09, 2025 at 04:54:43PM +0200, Stephan Gerhold wrote:
> > > On Fri, May 09, 2025 at 12:45:20PM +0200, Casey Connolly wrote:
> > > > On 5/8/25 12:32, Sumit Garg wrote:
> > > > > From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > > > > 
> > > > > When debug serial port isn't connected, it is at least reported on RB1
> > > > > that autoboot gets interrupted. It is probably due to random characters
> > > > > on the UART RX line when disconnected. Lets try to fix this inconsistent
> > > > > behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
> > > > > interrupted only when the <SPACE> key gets pressed.
> > > > 
> > > > Honestly I'd much rather fix the bug in the UART driver, maybe it's broken
> > > > pinctrl?
> > > > 
> > > > The older UART driver (for msm8916) had a similar but that Stephen was able
> > > > to fix with better configuration.
> > > > 
> > > > Anyhow, this is a sensible enough workaround if nobody wants to look into
> > > > GENI...
> > > > 
> > > 
> > > Well, the main fix I did for that isn't related to the UART driver at
> > > all, it's simply adding bias-pull-up for the RX pin so that it isn't
> > > left floating when UART is not connected.
> > > 
> > > For RB1, qcm2290.dtsi &qup_uart4_default currently has bias-disable for
> > > both RX and TX pins. So please try applying bias-pull-up + bootph-all
> > > for the GPIO that belongs to RX.
> > > 
> > > For details, please see the following patch series for MSM8916:
> > > https://lore.kernel.org/linux-arm-msm/20250422-msm8916-console-pinctrl-v2-0-f345b7a53c91@linaro.org/T/
> > 
> > Thanks for this reference but unfortunately this doesn't fix the issue
> > on RB1. I have tried the diff below [1]. It seems most likely due the
> > chainloading via ABL on RB1 as compared to U-Boot running as primary
> > bootloader on db410c. The RX line seems to be floating even before the
> > U-Boot starts which is causing the autoboot to stop. So for the
> > chainloading configuration, we need to enable AUTOBOOT_KEYED. I think
> > it should be the default sane configuration on Qcom platforms rather
> > than allowing autoboot stopped via any random character on RX line.
> 
> I dug into it and the rogue inputs actually come from the button-kbd driver,
> maybe the buttons are slightly misconfigured. This explains why it only
> happens on some boards.
> 
> So the right solution here is to fix the buttons, or disable button-kbd if
> you don't need it.

I tried disabling button-kbd using following patch but still the
autoboot gets stopped on RB1 with disconnected serial cable. It is most
likely caused by rougue chars on serial sue to floating RX line before the
boot flow reached U-Boot. I am even able to reproduce the same issue on
db845c in chainloaded config.

So, AUTOBOOT_KEYED seems to be the best possible solution here for
chainloaded config.

diff --git a/board/qualcomm/default.env b/board/qualcomm/default.env
index 87703b5a643..eab153d6879 100644
--- a/board/qualcomm/default.env
+++ b/board/qualcomm/default.env
@@ -1,4 +1,4 @@
-stdin=serial,button-kbd
+stdin=serial
 stdout=serial,vidconsole
 stderr=serial,vidconsole
 preboot=scsi scan; usb start
diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig
index e3dac23f1d0..45a203ddf78 100644
--- a/configs/qcom_defconfig
+++ b/configs/qcom_defconfig
@@ -80,8 +77,6 @@ CONFIG_DM_I2C=y
 CONFIG_SYS_I2C_QUP=y
 CONFIG_SYS_I2C_GENI=y
 CONFIG_I2C_MUX=y
-CONFIG_DM_KEYBOARD=y
-CONFIG_BUTTON_KEYBOARD=y
 CONFIG_IOMMU=y
 CONFIG_QCOM_HYP_SMMU=y
 CONFIG_MISC=y

-Sumit

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Stephan Gerhold]

On Mon, May 12, 2025 at 12:31:58PM +0530, Sumit Garg wrote:
> On Fri, May 09, 2025 at 04:54:43PM +0200, Stephan Gerhold wrote:
> > On Fri, May 09, 2025 at 12:45:20PM +0200, Casey Connolly wrote:
> > > On 5/8/25 12:32, Sumit Garg wrote:
> > > > From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > > > 
> > > > When debug serial port isn't connected, it is at least reported on RB1
> > > > that autoboot gets interrupted. It is probably due to random characters
> > > > on the UART RX line when disconnected. Lets try to fix this inconsistent
> > > > behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
> > > > interrupted only when the <SPACE> key gets pressed.
> > > 
> > > Honestly I'd much rather fix the bug in the UART driver, maybe it's broken
> > > pinctrl?
> > > 
> > > The older UART driver (for msm8916) had a similar but that Stephen was able
> > > to fix with better configuration.
> > > 
> > > Anyhow, this is a sensible enough workaround if nobody wants to look into
> > > GENI...
> > > 
> > 
> > Well, the main fix I did for that isn't related to the UART driver at
> > all, it's simply adding bias-pull-up for the RX pin so that it isn't
> > left floating when UART is not connected.
> > 
> > For RB1, qcm2290.dtsi &qup_uart4_default currently has bias-disable for
> > both RX and TX pins. So please try applying bias-pull-up + bootph-all
> > for the GPIO that belongs to RX.
> > 
> > For details, please see the following patch series for MSM8916:
> > https://lore.kernel.org/linux-arm-msm/20250422-msm8916-console-pinctrl-v2-0-f345b7a53c91@linaro.org/T/
> 
> Thanks for this reference but unfortunately this doesn't fix the issue
> on RB1. I have tried the diff below [1]. It seems most likely due the
> chainloading via ABL on RB1 as compared to U-Boot running as primary
> bootloader on db410c. The RX line seems to be floating even before the
> U-Boot starts which is causing the autoboot to stop. So for the
> chainloading configuration, we need to enable AUTOBOOT_KEYED. I think
> it should be the default sane configuration on Qcom platforms rather
> than allowing autoboot stopped via any random character on RX line.
> 

Can try again with the following diff added in addition to the
bias-pull-up changes from below? Pretty sure this is needed to apply the
pinctrl before the UART driver is initialized in the pre-reloc stage.

We're resetting the UART controller during initialization, so I don't
think whatever happens before U-Boot should make any difference.

Thanks,
Stephan

diff --git a/drivers/pinctrl/qcom/pinctrl-qcm2290.c b/drivers/pinctrl/qcom/pinctrl-qcm2290.c
index 84f76b63b93..dcd499d99ae 100644
--- a/drivers/pinctrl/qcom/pinctrl-qcm2290.c
+++ b/drivers/pinctrl/qcom/pinctrl-qcm2290.c
@@ -67,4 +67,5 @@ U_BOOT_DRIVER(pinctrl_qcm2290) = {
 	.of_match = msm_pinctrl_ids,
 	.ops = &msm_pinctrl_ops,
 	.bind = msm_pinctrl_bind,
+	.flags = DM_FLAG_PRE_RELOC,
 };

> [1]
> 
> diff --git a/dts/upstream/src/arm64/qcom/qcm2290.dtsi b/dts/upstream/src/arm64/qcom/qcm2290.dtsi
> index f0746123e59..9392bd6f3e0 100644
> --- a/dts/upstream/src/arm64/qcom/qcm2290.dtsi
> +++ b/dts/upstream/src/arm64/qcom/qcm2290.dtsi
> @@ -551,10 +551,28 @@
>                         };
> 
>                         qup_uart4_default: qup-uart4-default-state {
> +                               tx-pins {
> +                                       pins = "gpio12";
> +                                       function = "qup4";
> +                                       drive-strength = <16>;
> +                                       bias-disable;
> +                                       bootph-all;
> +                               };
> +
> +                               rx-pins {
> +                                       pins = "gpio13";
> +                                       function = "qup4";
> +                                       drive-strength = <16>;
> +                                       bias-pull-up;
> +                                       bootph-all;
> +                               };
> +                       };
> +
> +                       qup_uart4_sleep: qup-uart4-sleep-state {
>                                 pins = "gpio12", "gpio13";
> -                               function = "qup4";
> +                               function = "gpio";
>                                 drive-strength = <2>;
> -                               bias-disable;
> +                               bias-pull-down;
>                         };
> 
>                         sdc1_state_on: sdc1-on-state {
> @@ -1293,7 +1311,8 @@
>                                 clocks = <&gcc GCC_QUPV3_WRAP0_S4_CLK>;
>                                 clock-names = "se";
>                                 pinctrl-0 = <&qup_uart4_default>;
> -                               pinctrl-names = "default";
> +                               pinctrl-1 = <&qup_uart4_sleep>;
> +                               pinctrl-names = "default", "sleep";
>                                 interconnects = <&qup_virt MASTER_QUP_CORE_0 RPM_ALWAYS_TAG
>                                                  &qup_virt SLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
>                                                 <&bimc MASTER_APPSS_PROC RPM_ALWAYS_TAG
> 

Re: [PATCH] qcom_defconfig: Enable AUTOBOOT_KEYED support

[Casey Connolly]

On Thu, 08 May 2025 16:02:38 +0530, Sumit Garg wrote:
> When debug serial port isn't connected, it is at least reported on RB1
> that autoboot gets interrupted. It is probably due to random characters
> on the UART RX line when disconnected. Lets try to fix this inconsistent
> behaviour via enabling AUTOBOOT_KEYED such that the autoboot gets
> interrupted only when the <SPACE> key gets pressed.
> 
> Reviewed-by:
> 
> [...]

Applied, thanks!

[1/1] qcom_defconfig: Enable AUTOBOOT_KEYED support
      https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commit/78f177fcc2ed

Best regards,
-- 
Casey Connolly <casey.connolly@linaro.org>