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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> wrote:
>> > > >
>> > > > On 6/16/25 4:33 PM, Bartosz Golaszewski wrote:
>> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>> > > > >
>> > > > > 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@xxxxxxxxxx>
>> > > > > ---
>> > > >
>> > > > 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