[PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

GPIO12 and GPIO13 are used for the debug UART and must not be available
to drivers or user-space. Add them to the gpio-reserved-ranges.

Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts
index a37860175d273..384427e98dfbd 100644
--- a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts
+++ b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts
@@ -606,9 +606,8 @@ &sleep_clk {
 };
 
 &tlmm {
-	gpio-reserved-ranges = <43 2>, <49 1>, <54 1>,
-			       <56 3>, <61 2>, <64 1>,
-			       <68 1>, <72 8>, <96 1>;
+	gpio-reserved-ranges = <12 2>, <43 2>, <49 1>, <54 1>, <56 3>,
+			       <61 2>, <64 1>, <68 1>, <72 8>, <96 1>;
 
 	uart3_default: uart3-default-state {
 		cts-pins {
-- 
2.48.1

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Konrad Dybcio]

On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> GPIO12 and GPIO13 are used for the debug UART and must not be available
> to drivers or user-space. Add them to the gpio-reserved-ranges.
> 
> Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

That also makes them unavailable to the kernel though, no?

Konrad

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Bartosz Golaszewski]

On Mon, Jun 16, 2025 at 6:20 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > GPIO12 and GPIO13 are used for the debug UART and must not be available
> > to drivers or user-space. Add them to the gpio-reserved-ranges.
> >
> > Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
>
> That also makes them unavailable to the kernel though, no?
>

Yes. They could only be used by QUP - I2C or SPI #4 - on sm6115 but
none of these are used on RB2. I just noticed that my console froze
when I accidentally requested GPIO12 and figured that it makes sense
to make them unavailable. Let me know if this should be dropped.

Bart

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Konrad Dybcio]

On 6/16/25 6:43 PM, Bartosz Golaszewski wrote:
> On Mon, Jun 16, 2025 at 6:20 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> GPIO12 and GPIO13 are used for the debug UART and must not be available
>>> to drivers or user-space. Add them to the gpio-reserved-ranges.
>>>
>>> Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>
>> That also makes them unavailable to the kernel though, no?
>>
> 
> Yes. They could only be used by QUP - I2C or SPI #4 - on sm6115 but
> none of these are used on RB2. I just noticed that my console froze
> when I accidentally requested GPIO12 and figured that it makes sense
> to make them unavailable. Let me know if this should be dropped.

We usually carry an active/sleep pair of pinctrl configs - would they
be affected by these changes?

Konrad

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Bjorn Andersson]

On Mon, Jun 16, 2025 at 06:43:16PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 16, 2025 at 6:20 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
> >
> > On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > GPIO12 and GPIO13 are used for the debug UART and must not be available
> > > to drivers or user-space. Add them to the gpio-reserved-ranges.
> > >
> > > Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> >
> > That also makes them unavailable to the kernel though, no?
> >
> 
> Yes. They could only be used by QUP - I2C or SPI #4 - on sm6115 but
> none of these are used on RB2. I just noticed that my console froze
> when I accidentally requested GPIO12 and figured that it makes sense
> to make them unavailable. Let me know if this should be dropped.
> 

I'm guessing that this would be a problem for any pin that is used for
some other function. Should we instead prevent userspace from being able
to request pins that are not in "gpio" pinmux state?

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Bartosz Golaszewski]

On Tue, Jun 17, 2025 at 5:18 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Mon, Jun 16, 2025 at 06:43:16PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jun 16, 2025 at 6:20 PM Konrad Dybcio
> > <konrad.dybcio@oss.qualcomm.com> wrote:
> > >
> > > On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > GPIO12 and GPIO13 are used for the debug UART and must not be available
> > > > to drivers or user-space. Add them to the gpio-reserved-ranges.
> > > >
> > > > Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > ---
> > >
> > > That also makes them unavailable to the kernel though, no?
> > >
> >
> > Yes. They could only be used by QUP - I2C or SPI #4 - on sm6115 but
> > none of these are used on RB2. I just noticed that my console froze
> > when I accidentally requested GPIO12 and figured that it makes sense
> > to make them unavailable. Let me know if this should be dropped.
> >
>
> I'm guessing that this would be a problem for any pin that is used for
> some other function. Should we instead prevent userspace from being able
> to request pins that are not in "gpio" pinmux state?
>

That's supported by the "strict" flag in struct pinmux_ops. However
the two pins in question are muxed to GPIOs as far as the msm pinctrl
driver is concerned so it wouldn't help. Turning on the strict flag at
the global level of the pinctrl-msm driver would be risky though as it
would affect so many platforms, I'm sure it would break things. So IMO
it's either this change or let's drop it and leave it as is.

Bartosz

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[Bjorn Andersson]

On Tue, Jun 17, 2025 at 01:28:41PM +0200, Bartosz Golaszewski wrote:
> On Tue, Jun 17, 2025 at 5:18 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Mon, Jun 16, 2025 at 06:43:16PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Jun 16, 2025 at 6:20 PM Konrad Dybcio
> > > <konrad.dybcio@oss.qualcomm.com> wrote:
> > > >
> > > > On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > GPIO12 and GPIO13 are used for the debug UART and must not be available
> > > > > to drivers or user-space. Add them to the gpio-reserved-ranges.
> > > > >
> > > > > Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
> > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > ---
> > > >
> > > > That also makes them unavailable to the kernel though, no?
> > > >
> > >
> > > Yes. They could only be used by QUP - I2C or SPI #4 - on sm6115 but
> > > none of these are used on RB2. I just noticed that my console froze
> > > when I accidentally requested GPIO12 and figured that it makes sense
> > > to make them unavailable. Let me know if this should be dropped.
> > >
> >
> > I'm guessing that this would be a problem for any pin that is used for
> > some other function. Should we instead prevent userspace from being able
> > to request pins that are not in "gpio" pinmux state?
> >
> 
> That's supported by the "strict" flag in struct pinmux_ops. However
> the two pins in question are muxed to GPIOs as far as the msm pinctrl
> driver is concerned so it wouldn't help.

This doesn't sound correct, the pins needs to be muxed to the qup in
order for UART to work, so how can they show as "gpio" function?

> Turning on the strict flag at
> the global level of the pinctrl-msm driver would be risky though as it
> would affect so many platforms, I'm sure it would break things. So IMO
> it's either this change or let's drop it and leave it as is.
> 

I share your concern, but the benefit sounds desirable. I think
protecting the UART pins would set a precedence for filling that list
with all kinds of pins, for all platforms, so lets give this some more
thought, 

Thank you,
Bjorn

Re: [PATCH] arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2

[brgl]

On Wed, 18 Jun 2025 04:33:31 +0200, Bjorn Andersson <andersson@kernel.org> said:
> On Tue, Jun 17, 2025 at 01:28:41PM +0200, Bartosz Golaszewski wrote:
>> On Tue, Jun 17, 2025 at 5:18 AM Bjorn Andersson <andersson@kernel.org> wrote:
>> >
>> > On Mon, Jun 16, 2025 at 06:43:16PM +0200, Bartosz Golaszewski wrote:
>> > > On Mon, Jun 16, 2025 at 6:20 PM Konrad Dybcio
>> > > <konrad.dybcio@oss.qualcomm.com> wrote:
>> > > >
>> > > > On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
>> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> > > > >
>> > > > > GPIO12 and GPIO13 are used for the debug UART and must not be available
>> > > > > to drivers or user-space. Add them to the gpio-reserved-ranges.
>> > > > >
>> > > > > Fixes: 8d58a8c0d930c ("arm64: dts: qcom: Add base qrb4210-rb2 board dts")
>> > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> > > > > ---
>> > > >
>> > > > That also makes them unavailable to the kernel though, no?
>> > > >
>> > >
>> > > Yes. They could only be used by QUP - I2C or SPI #4 - on sm6115 but
>> > > none of these are used on RB2. I just noticed that my console froze
>> > > when I accidentally requested GPIO12 and figured that it makes sense
>> > > to make them unavailable. Let me know if this should be dropped.
>> > >
>> >
>> > I'm guessing that this would be a problem for any pin that is used for
>> > some other function. Should we instead prevent userspace from being able
>> > to request pins that are not in "gpio" pinmux state?
>> >
>>
>> That's supported by the "strict" flag in struct pinmux_ops. However
>> the two pins in question are muxed to GPIOs as far as the msm pinctrl
>> driver is concerned so it wouldn't help.
>
> This doesn't sound correct, the pins needs to be muxed to the qup in
> order for UART to work, so how can they show as "gpio" function?
>

There's no pinctrl-0 property in the uart4 node. But if we do the following:

diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi
b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index c8865779173ec..8c29440e9f43c 100644
--- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
@@ -672,6 +672,18 @@ qup_i2c4_default: qup-i2c4-default-state {
                                bias-pull-up;
                        };

+                       qup_uart4_default: qup-uart4-default-state {
+                               qup_uart4_tx: tx-pins {
+                                       pins = "gpio12";
+                                       function = "qup4";
+                               };
+
+                               qup_uart4_rx: rx-pins {
+                                       pins = "gpio13";
+                                       function = "qup4";
+                               };
+                       };
+
                        qup_i2c5_default: qup-i2c5-default-state {
                                pins = "gpio14", "gpio15";
                                function = "qup5";
@@ -1565,6 +1577,8 @@ uart4: serial@4a90000 {
                                reg = <0x0 0x04a90000 0x0 0x4000>;
                                clock-names = "se";
                                clocks = <&gcc GCC_QUPV3_WRAP0_S4_CLK>;
+                               pinctrl-names = "default";
+                               pinctrl-0 = <&qup_uart4_default>;
                                interrupts = <GIC_SPI 331 IRQ_TYPE_LEVEL_HIGH>;
                                interconnects = <&clk_virt
MASTER_QUP_CORE_0 RPM_ALWAYS_TAG
                                                 &clk_virt
SLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
b/drivers/pinctrl/qcom/pinctrl-msm.c
index 5c4687de1464a..e5c85d21e13c7 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -293,6 +293,7 @@ static const struct pinmux_ops msm_pinmux_ops = {
        .get_function_groups    = msm_get_function_groups,
        .gpio_request_enable    = msm_pinmux_request_gpio,
        .set_mux                = msm_pinmux_set_mux,
+       .strict                 = true,
 };

 static int msm_config_reg(struct msm_pinctrl *pctrl,

Then the problem on RB2 goes away.

>> Turning on the strict flag at
>> the global level of the pinctrl-msm driver would be risky though as it
>> would affect so many platforms, I'm sure it would break things. So IMO
>> it's either this change or let's drop it and leave it as is.
>>
>
> I share your concern, but the benefit sounds desirable. I think
> protecting the UART pins would set a precedence for filling that list
> with all kinds of pins, for all platforms, so lets give this some more
> thought,
>

I can only test this change on so many boards. We could give it a try, it's
early into the cycle so if we get this change into next now, we'd have some
time to see if anything breaks. I can send patches with the above changes
if you're alright with it.

Bart