arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2
Subsystems (6)
PATCH 1/1 arm64: dts: qcom: add debug UART pins to reserved GPIO ranges on RB2
From: Bartosz Golaszewski <[email protected]>
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.
--- arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
arch/arm64/boot/dts/qcom/qrb4210-rb2.dts +2 -3
@@ -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 {
Signature · 1 line
2.48.1
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>---
That also makes them unavailable to the kernel though, no?
Konrad
Quoted 4 lines · Konrad Dybcio
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 9 lines
Quoted 6 lines
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 <[email protected]>---
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
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Konrad Dybcio
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>---
Quoted 7 lines
Quoted 2 lines
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
Bjorn Andersson
maintainerQuoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Konrad Dybcio
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>---
Quoted 8 lines
Quoted 2 lines
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
Quoted 24 lines · Bjorn Andersson
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Konrad Dybcio
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>---
Quoted 8 lines
Quoted 2 lines
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.
Quoted 4 lines
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
Bjorn Andersson
maintainerQuoted 25 lines · Bartosz Golaszewski
Quoted 24 lines · Bjorn Andersson
Quoted 21 lines · Bartosz Golaszewski
Quoted 4 lines · Konrad Dybcio
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 9 lines
Quoted 6 lines
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 <[email protected]>---
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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 5 lines
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
Quoted 31 lines · Bjorn Andersson
Quoted 30 lines · Bartosz Golaszewski
Quoted 28 lines · Bjorn Andersson
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Konrad Dybcio
Quoted 8 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>---
Quoted 8 lines
Quoted 2 lines
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?
Quoted 7 lines
Quoted 3 lines
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:
(quoted diff) +0 -0
b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index c8865779173ec..8c29440e9f43c 100644
arch/arm64/boot/dts/qcom/sm6115.dtsi +14 -0
@@ -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_virtSLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
(quoted diff) +0 -0
b/drivers/pinctrl/qcom/pinctrl-msm.c
index 5c4687de1464a..e5c85d21e13c7 100644
drivers/pinctrl/qcom/pinctrl-msm.c +1 -0
@@ -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.
Quoted 11 lines
Quoted 5 lines
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