[PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Some boards use pinctrl to set a non-gpio pinmux on a pin that is later requested to be used for gpio output.
Add a pin_to_bank() helper and implement gpio_request_enable() ops so that the gpio request() ops can be implemented and a gpio requested pin automatically is pinmuxed for gpio use, similar to Linux kernel.
Reset ctrl->nr_pins to 0 so that pin_to_bank() can locate a bank after the second probe in U-Boot proper.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- The pin_to_bank() helper will also be used in a follow up series adding support for the pinmux status cmd. --- .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c index 8ef089994f46..a423abcafb23 100644 --- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c +++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c @@ -150,6 +150,23 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin) return ((val >> bit) & mask); }
+static struct rockchip_pin_bank *rockchip_pin_to_bank(struct udevice *dev, + unsigned int pin) +{ + struct rockchip_pinctrl_priv *priv = dev_get_priv(dev); + struct rockchip_pin_ctrl *ctrl = priv->ctrl; + struct rockchip_pin_bank *bank = ctrl->pin_banks; + int i; + + for (i = 0; i < ctrl->nr_banks; ++i, ++bank) { + if (pin >= bank->pin_base && + pin < bank->pin_base + bank->nr_pins) + return bank; + } + + return NULL; +} + static int rockchip_pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index) { struct rockchip_pinctrl_priv *priv = dev_get_priv(dev); @@ -250,6 +267,18 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) return 0; }
+static int rockchip_pinctrl_gpio_request_enable(struct udevice *dev, + unsigned int selector) +{ + struct rockchip_pin_bank *bank; + + bank = rockchip_pin_to_bank(dev, selector); + if (!bank) + return -EINVAL; + + return rockchip_set_mux(bank, selector - bank->pin_base, RK_FUNC_GPIO); +} + static int rockchip_perpin_drv_list[DRV_TYPE_MAX][8] = { { 2, 4, 8, 12, -1, -1, -1, -1 }, { 3, 6, 9, 12, -1, -1, -1, -1 }, @@ -497,6 +526,7 @@ static int rockchip_pinctrl_set_state(struct udevice *dev, const struct pinctrl_ops rockchip_pinctrl_ops = { .set_state = rockchip_pinctrl_set_state, .get_gpio_mux = rockchip_pinctrl_get_gpio_mux, + .gpio_request_enable = rockchip_pinctrl_gpio_request_enable, };
/* retrieve the soc specific data */ @@ -513,6 +543,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(struct udevice *d drv_pmu_offs = ctrl->pmu_drv_offset; drv_grf_offs = ctrl->grf_drv_offset; bank = ctrl->pin_banks; + ctrl->nr_pins = 0;
for (i = 0; i < ctrl->nr_banks; ++i, ++bank) { int bank_pins = 0;
Some boards use pinctrl to set a non-gpio pinmux on a pin that is later requested to be used for gpio output.
Add a request() ops that call pinctrl_gpio_request() when the required gpio-ranges prop has been defined to signal pinctrl driver to use gpio pinmux.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs affected, gpio-ranges props for remaining RK SoCs will be added in a follow up series adding support for the pinmux status cmd. --- drivers/gpio/rk_gpio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c index 2e901ac5c734..956894501633 100644 --- a/drivers/gpio/rk_gpio.c +++ b/drivers/gpio/rk_gpio.c @@ -126,6 +126,15 @@ static int rockchip_gpio_get_function(struct udevice *dev, unsigned offset) return (data & mask) ? GPIOF_OUTPUT : GPIOF_INPUT; }
+static int rockchip_gpio_request(struct udevice *dev, unsigned offset, + const char *label) +{ + if (CONFIG_IS_ENABLED(PINCTRL) && dev_read_bool(dev, "gpio-ranges")) + return pinctrl_gpio_request(dev, offset, label); + + return 0; +} + /* Simple SPL interface to GPIOs */ #ifdef CONFIG_SPL_BUILD
@@ -216,6 +225,7 @@ static int rockchip_gpio_probe(struct udevice *dev) }
static const struct dm_gpio_ops gpio_rockchip_ops = { + .request = rockchip_gpio_request, .direction_input = rockchip_gpio_direction_input, .direction_output = rockchip_gpio_direction_output, .get_value = rockchip_gpio_get_value,
The GPIO2_D6 pin is changed to use func-4 using pcie30x2m1_pins during probe of pcie3x2. This cause the device to lock-up when pci driver use the reset-gpios unless the pin is first changed to use gpio pinmux.
Drop the board u-boot.dtsi workaround now that the gpio and pinctrl drivers automatically use gpio pinmux when a gpio is requested.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi index 9d18f5d0b364..f4e2dc91ddfb 100644 --- a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi +++ b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi @@ -6,18 +6,6 @@
#include "rk356x-u-boot.dtsi"
-&pcie3x2 { - pinctrl-0 = <&pcie3x2_reset_h>; -}; - -&pinctrl { - pcie { - pcie3x2_reset_h: pcie3x2-reset-h { - rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>; - }; - }; -}; - &sdhci { cap-mmc-highspeed; mmc-hs200-1_8v;
Hi Jonas,
On 2024/5/11 19:28, Jonas Karlman wrote:
The GPIO2_D6 pin is changed to use func-4 using pcie30x2m1_pins during probe of pcie3x2. This cause the device to lock-up when pci driver use the reset-gpios unless the pin is first changed to use gpio pinmux.
The reset-gpio for PCIe is for sure to use this IO as GPIO instead of function IO. And also CLKREQ and WAKEN should work in GPIO mode. So keep the all PCIe IO to pinmux to function IO should not be correct. Could you try with something like this: &pcie3x2 { /delete-property/ pinctrl-names /delete-property/ pinctrl-0; };
Thanks, - Kever
Drop the board u-boot.dtsi workaround now that the gpio and pinctrl drivers automatically use gpio pinmux when a gpio is requested.
Signed-off-by: Jonas Karlmanjonas@kwiboo.se
arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi index 9d18f5d0b364..f4e2dc91ddfb 100644 --- a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi +++ b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi @@ -6,18 +6,6 @@
#include "rk356x-u-boot.dtsi"
-&pcie3x2 {
- pinctrl-0 = <&pcie3x2_reset_h>;
-};
-&pinctrl {
- pcie {
pcie3x2_reset_h: pcie3x2-reset-h {rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>;};- };
-};
- &sdhci { cap-mmc-highspeed; mmc-hs200-1_8v;
The GPIO0_C3 pin is changed to use func-3 using pcie30x1m0_pins during probe of pcie3x1. This cause the device to lock-up when pci driver use the reset-gpios unless the pin is first changed to use gpio pinmux.
Drop the board u-boot.dtsi workaround now that the gpio and pinctrl drivers automatically use gpio pinmux when a gpio is requested.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi b/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi index 74755a44eaee..00fa0dfee19c 100644 --- a/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi +++ b/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi @@ -2,18 +2,6 @@
#include "rk356x-u-boot.dtsi"
-&pcie3x1 { - pinctrl-0 = <&pcie30x1_reset_h>; -}; - -&pinctrl { - pcie { - pcie30x1_reset_h: pcie30x1-reset-h { - rockchip,pins = <0 RK_PC3 RK_FUNC_GPIO &pcfg_pull_none>; - }; - }; -}; - &sdhci { cap-mmc-highspeed; mmc-hs200-1_8v;
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
Regards, Jonas
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Date: Sat, 11 May 2024 20:47:40 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Jonas & Alex,
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I missed Alex's mail, but OpenBSD certainly is one of the OSes that could break if the pinctrl settings get removed. We currently have no code to automatically mux the gpio pins on these Rockchip SoCs. I suppose as long as U-Boot probes the PCIe bus, the pins will already be muxed correctly and things will continue to work. But I think there are certain boot scenarios where this won't happen.
I've wondered in the past what the purpose of the gpio-ranges properties was. I never considered that they could be used to automatically mux the pins for GPIO since the gpio-ranges mapping provides no indication of what the correct pin settings arefor the GPIO pins.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
Well, it removes the incentive to fix the upstream DTs and would make it harder to notice missing pinctrls in the DTs.
Cheers,
Mark
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Hi Mark,
On 2024-05-11 21:57, Mark Kettenis wrote:
Date: Sat, 11 May 2024 20:47:40 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Jonas & Alex,
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I missed Alex's mail, but OpenBSD certainly is one of the OSes that could break if the pinctrl settings get removed. We currently have no code to automatically mux the gpio pins on these Rockchip SoCs. I suppose as long as U-Boot probes the PCIe bus, the pins will already be muxed correctly and things will continue to work. But I think there are certain boot scenarios where this won't happen.
The control FDT on the known affected boards was only patched to prevent the boards from a full device freeze when PCIe was enumerated. An OS should probably not depend on a workaround made for U-Boot use.
Is it common for *BDS to use the control FDT or is it more common that a separate .dtb-file to be loaded during boot?
To recap, the issue on e.g. ROCK 3A is that the upstream DT use pinctrl to signal PCIE30X2 CLKREQn_M1/WAKEn_M1/PERSTn_M1 function, DT also specify a reset-gpios prop pointing on PERSTn_M1. To me this seem like a correct description of HW.
When U-Boot probe the PCIE30X2 device the pinctrl driver would automatically configure CLKREQn_M1/WAKEn_M1/PERSTn_M1 function.
When U-Boot RK PCIe driver try to use the reset-gpios pin the device would freeze unless the PERSTn pin is first re-configured for gpio use.
Current workaround in U-Boot throws away the existing upstream pinctrl and replace it to only configure gpio func on the PERSTn pin. This also leaves the CLKREQn and WAKEn to incorrectly use gpio func.
With this series I try to cleanup the old workaround that I added without much insight beyond: device no longer freeze and nvme "works" :-)
I've wondered in the past what the purpose of the gpio-ranges properties was. I never considered that they could be used to automatically mux the pins for GPIO since the gpio-ranges mapping provides no indication of what the correct pin settings arefor the GPIO pins.
The gpio-ranges prop is only used to provide details on how the pins are mapped between gpio and pin controller. It is not used to change any muxing. But the property is required to make the U-Boot helpers pinctrl_gpio_request()/pinctrl_gpio_free() work and figure out what pinctrl udevice to use if another driver use e.g. gpio_request_by_name() to request use of a pin for gpio use.
Keep in mind that this series should not change any behavior except for the special case when DT pinctrl may configure a pin for non-gpio function and later U-Boot request the pin to be used for gpio.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
Well, it removes the incentive to fix the upstream DTs and would make it harder to notice missing pinctrls in the DTs.
I can understand that, but gpio pinmux is default so any missing pinctrl would still cause issues, this changes nothing in that regard.
This only affect if pinctrl exists and pin gets configured for non-gpio function use. And later a driver/cmd tries to use such pin for gpio use. Before this patch the use of gpio_request() would not change the pin mux for gpio use. After this series when code explicily request a pin for gpio use the pinctrl driver gets notified that it should change pinmux of the pin for gpio use.
Regards, Jonas
Cheers,
Mark
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Date: Sat, 11 May 2024 22:55:18 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Mark,
On 2024-05-11 21:57, Mark Kettenis wrote:
Date: Sat, 11 May 2024 20:47:40 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Jonas & Alex,
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I missed Alex's mail, but OpenBSD certainly is one of the OSes that could break if the pinctrl settings get removed. We currently have no code to automatically mux the gpio pins on these Rockchip SoCs. I suppose as long as U-Boot probes the PCIe bus, the pins will already be muxed correctly and things will continue to work. But I think there are certain boot scenarios where this won't happen.
The control FDT on the known affected boards was only patched to prevent the boards from a full device freeze when PCIe was enumerated. An OS should probably not depend on a workaround made for U-Boot use.
Sure, but I think that means we should try to minimise the number of workarounds in U-Boot ;).
Is it common for *BDS to use the control FDT or is it more common that a separate .dtb-file to be loaded during boot?
Not sure what the other BSDs do; at this point they're largely separate codebases that just share a common ancestry. And I don't really like the term "control FDT"; in ideal world there should be just be a single FDT that gets used by the various firmware layers, U-Boot and the OS. But yes, my goal is to make OpenBSD work with FDT provided by U-Boot without loading a separte .dtb file.
To recap, the issue on e.g. ROCK 3A is that the upstream DT use pinctrl to signal PCIE30X2 CLKREQn_M1/WAKEn_M1/PERSTn_M1 function, DT also specify a reset-gpios prop pointing on PERSTn_M1. To me this seem like a correct description of HW.
Actually I'm starting to think that it isn't. If you look at table 11-11 in Part 2 of the RK3588 TRM (V1.0) you'll see that the pcie_perst_n signal is designated as input. The corrsponding chapter is missing from the copy of the RK3568 TRM I have, but it is likely that the same applies to the PCIe conroller on the RK3568 SoC. Now these PCIe controllers can function in either Root Complex (RC) or Endpoint (EP) mode. And given the the pcie_perst_n signal is designated as input I think that means that the PERSTn_M1 function only makes sense when the controller is in EP mode. So for the majority of the boards that use the PCIe controller in RC mode, the DT should only configure the CLKREQn_M1 and WAKEn_M1 functions and configure the pin that is used to output PERST# as a GPIO.
When U-Boot probe the PCIE30X2 device the pinctrl driver would automatically configure CLKREQn_M1/WAKEn_M1/PERSTn_M1 function.
When U-Boot RK PCIe driver try to use the reset-gpios pin the device would freeze unless the PERSTn pin is first re-configured for gpio use.
Current workaround in U-Boot throws away the existing upstream pinctrl and replace it to only configure gpio func on the PERSTn pin. This also leaves the CLKREQn and WAKEn to incorrectly use gpio func.
With this series I try to cleanup the old workaround that I added without much insight beyond: device no longer freeze and nvme "works" :-)
I've wondered in the past what the purpose of the gpio-ranges properties was. I never considered that they could be used to automatically mux the pins for GPIO since the gpio-ranges mapping provides no indication of what the correct pin settings arefor the GPIO pins.
The gpio-ranges prop is only used to provide details on how the pins are mapped between gpio and pin controller. It is not used to change any muxing. But the property is required to make the U-Boot helpers pinctrl_gpio_request()/pinctrl_gpio_free() work and figure out what pinctrl udevice to use if another driver use e.g. gpio_request_by_name() to request use of a pin for gpio use.
But that means you rely on the Linux (and now U-Boot) driver interfaces. What I was saying is that this is all rather implicit and not explicitly documented in the device tree bindings.
Keep in mind that this series should not change any behavior except for the special case when DT pinctrl may configure a pin for non-gpio function and later U-Boot request the pin to be used for gpio.
To me such a case would be suspicious. What would be the purpose of such a configuration? And wouldn't this mean that you now implictly rely on the gpio being requested after the pinctl config has been applied?
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
Well, it removes the incentive to fix the upstream DTs and would make it harder to notice missing pinctrls in the DTs.
I can understand that, but gpio pinmux is default so any missing pinctrl would still cause issues, this changes nothing in that regard.
This only affect if pinctrl exists and pin gets configured for non-gpio function use. And later a driver/cmd tries to use such pin for gpio use. Before this patch the use of gpio_request() would not change the pin mux for gpio use. After this series when code explicily request a pin for gpio use the pinctrl driver gets notified that it should change pinmux of the pin for gpio use.
Regards, Jonas
Cheers,
Mark
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Hi Mark,
On 2024-05-12 22:41, Mark Kettenis wrote:
Date: Sat, 11 May 2024 22:55:18 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Mark,
On 2024-05-11 21:57, Mark Kettenis wrote:
Date: Sat, 11 May 2024 20:47:40 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Jonas & Alex,
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I missed Alex's mail, but OpenBSD certainly is one of the OSes that could break if the pinctrl settings get removed. We currently have no code to automatically mux the gpio pins on these Rockchip SoCs. I suppose as long as U-Boot probes the PCIe bus, the pins will already be muxed correctly and things will continue to work. But I think there are certain boot scenarios where this won't happen.
The control FDT on the known affected boards was only patched to prevent the boards from a full device freeze when PCIe was enumerated. An OS should probably not depend on a workaround made for U-Boot use.
Sure, but I think that means we should try to minimise the number of workarounds in U-Boot ;).
Agree, and something this series tries to accomplish, removing one "bad" workaround so that once upstream DT is fixed and it trickle down to dts/upstream/ nothing else needs to be changed in U-Boot.
Is it common for *BDS to use the control FDT or is it more common that a separate .dtb-file to be loaded during boot?
Not sure what the other BSDs do; at this point they're largely separate codebases that just share a common ancestry. And I don't really like the term "control FDT"; in ideal world there should be just be a single FDT that gets used by the various firmware layers, U-Boot and the OS. But yes, my goal is to make OpenBSD work with FDT provided by U-Boot without loading a separte .dtb file.
Thanks for the insights. Typically I test linux-next kernel using the FDT provided by U-Boot. Any suggestions on how I can get a minimal OpenBSD snapshot to boot on e.g. a RK356x board?
To recap, the issue on e.g. ROCK 3A is that the upstream DT use pinctrl to signal PCIE30X2 CLKREQn_M1/WAKEn_M1/PERSTn_M1 function, DT also specify a reset-gpios prop pointing on PERSTn_M1. To me this seem like a correct description of HW.
Actually I'm starting to think that it isn't. If you look at table 11-11 in Part 2 of the RK3588 TRM (V1.0) you'll see that the pcie_perst_n signal is designated as input. The corrsponding chapter is missing from the copy of the RK3568 TRM I have, but it is likely that the same applies to the PCIe conroller on the RK3568 SoC. Now these PCIe controllers can function in either Root Complex (RC) or Endpoint (EP) mode. And given the the pcie_perst_n signal is designated as input I think that means that the PERSTn_M1 function only makes sense when the controller is in EP mode. So for the majority of the boards that use the PCIe controller in RC mode, the DT should only configure the CLKREQn_M1 and WAKEn_M1 functions and configure the pin that is used to output PERST# as a GPIO.
Thanks for this insights!
I can also clarify that the freeze only happens when a PCIe device is not attached, removing the reset-gpio prop and leaving the pin in PERSTn_M1 function my nvme drive is discovered correctly. Without anything attached the device freeze during "pci enum".
When U-Boot probe the PCIE30X2 device the pinctrl driver would automatically configure CLKREQn_M1/WAKEn_M1/PERSTn_M1 function.
When U-Boot RK PCIe driver try to use the reset-gpios pin the device would freeze unless the PERSTn pin is first re-configured for gpio use.
Current workaround in U-Boot throws away the existing upstream pinctrl and replace it to only configure gpio func on the PERSTn pin. This also leaves the CLKREQn and WAKEn to incorrectly use gpio func.
With this series I try to cleanup the old workaround that I added without much insight beyond: device no longer freeze and nvme "works" :-)
I've wondered in the past what the purpose of the gpio-ranges properties was. I never considered that they could be used to automatically mux the pins for GPIO since the gpio-ranges mapping provides no indication of what the correct pin settings arefor the GPIO pins.
The gpio-ranges prop is only used to provide details on how the pins are mapped between gpio and pin controller. It is not used to change any muxing. But the property is required to make the U-Boot helpers pinctrl_gpio_request()/pinctrl_gpio_free() work and figure out what pinctrl udevice to use if another driver use e.g. gpio_request_by_name() to request use of a pin for gpio use.
But that means you rely on the Linux (and now U-Boot) driver interfaces. What I was saying is that this is all rather implicit and not explicitly documented in the device tree bindings.
I fully understand this, however, from a developer perspective and seeing how there is no API to configure pinmux from code but there is an API to request and work with gpio, I would expect that the abstraction layer handle anything needed to be able to use the returned gpio.
Keep in mind that this series should not change any behavior except for the special case when DT pinctrl may configure a pin for non-gpio function and later U-Boot request the pin to be used for gpio.
To me such a case would be suspicious. What would be the purpose of such a configuration? And wouldn't this mean that you now implictly rely on the gpio being requested after the pinctl config has been applied?
Main purpose is to ensure the U-Boot gpio API works as expected for debugging and development purposes. For final DTs I am expecting that pinctrl is configured correctly.
Regards, Jonas
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
Well, it removes the incentive to fix the upstream DTs and would make it harder to notice missing pinctrls in the DTs.
I can understand that, but gpio pinmux is default so any missing pinctrl would still cause issues, this changes nothing in that regard.
This only affect if pinctrl exists and pin gets configured for non-gpio function use. And later a driver/cmd tries to use such pin for gpio use. Before this patch the use of gpio_request() would not change the pin mux for gpio use. After this series when code explicily request a pin for gpio use the pinctrl driver gets notified that it should change pinmux of the pin for gpio use.
Regards, Jonas
Cheers,
Mark
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Date: Mon, 13 May 2024 00:23:33 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Jonas,
Sorry for the delayed reply. Last week was a bit busy here...
Hi Mark,
On 2024-05-12 22:41, Mark Kettenis wrote:
Date: Sat, 11 May 2024 22:55:18 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Mark,
On 2024-05-11 21:57, Mark Kettenis wrote:
Date: Sat, 11 May 2024 20:47:40 +0200 From: Jonas Karlman jonas@kwiboo.se
Hi Jonas & Alex,
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman: > This series add gpio request() and pinctrl gpio_request_enable() ops so > that a gpio requested pin automatically use gpio pinmux and U-Boot > behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I missed Alex's mail, but OpenBSD certainly is one of the OSes that could break if the pinctrl settings get removed. We currently have no code to automatically mux the gpio pins on these Rockchip SoCs. I suppose as long as U-Boot probes the PCIe bus, the pins will already be muxed correctly and things will continue to work. But I think there are certain boot scenarios where this won't happen.
The control FDT on the known affected boards was only patched to prevent the boards from a full device freeze when PCIe was enumerated. An OS should probably not depend on a workaround made for U-Boot use.
Sure, but I think that means we should try to minimise the number of workarounds in U-Boot ;).
Agree, and something this series tries to accomplish, removing one "bad" workaround so that once upstream DT is fixed and it trickle down to dts/upstream/ nothing else needs to be changed in U-Boot.
To me it looks as if you're trying to replace an "incomplete" fix with a "questionable" workaround.
Is it common for *BDS to use the control FDT or is it more common that a separate .dtb-file to be loaded during boot?
Not sure what the other BSDs do; at this point they're largely separate codebases that just share a common ancestry. And I don't really like the term "control FDT"; in ideal world there should be just be a single FDT that gets used by the various firmware layers, U-Boot and the OS. But yes, my goal is to make OpenBSD work with FDT provided by U-Boot without loading a separte .dtb file.
Thanks for the insights. Typically I test linux-next kernel using the FDT provided by U-Boot. Any suggestions on how I can get a minimal OpenBSD snapshot to boot on e.g. a RK356x board?
Very simple. Just take it a "miniroot" image, e.g. the 7.5 release image:
https://ftp.openbsd.org/pub/OpenBSD/7.5/arm64/miniroot75.img
and use dd to write it to a micro-SD card or a USB flash drive. I usually put OpenBSD on a USB flash drive and U-Boot on a micro-SD card for testing. If you prefer to use a micro-SD card, just use dd to write u-boot-rockchip.bin on top of the "miniroot" image.
This will boot into the installer, but that is probably good enough for testing U-Boot.
To recap, the issue on e.g. ROCK 3A is that the upstream DT use pinctrl to signal PCIE30X2 CLKREQn_M1/WAKEn_M1/PERSTn_M1 function, DT also specify a reset-gpios prop pointing on PERSTn_M1. To me this seem like a correct description of HW.
Actually I'm starting to think that it isn't. If you look at table 11-11 in Part 2 of the RK3588 TRM (V1.0) you'll see that the pcie_perst_n signal is designated as input. The corrsponding chapter is missing from the copy of the RK3568 TRM I have, but it is likely that the same applies to the PCIe conroller on the RK3568 SoC. Now these PCIe controllers can function in either Root Complex (RC) or Endpoint (EP) mode. And given the the pcie_perst_n signal is designated as input I think that means that the PERSTn_M1 function only makes sense when the controller is in EP mode. So for the majority of the boards that use the PCIe controller in RC mode, the DT should only configure the CLKREQn_M1 and WAKEn_M1 functions and configure the pin that is used to output PERST# as a GPIO.
Thanks for this insights!
I can also clarify that the freeze only happens when a PCIe device is not attached, removing the reset-gpio prop and leaving the pin in PERSTn_M1 function my nvme drive is discovered correctly. Without anything attached the device freeze during "pci enum".
When U-Boot probe the PCIE30X2 device the pinctrl driver would automatically configure CLKREQn_M1/WAKEn_M1/PERSTn_M1 function.
When U-Boot RK PCIe driver try to use the reset-gpios pin the device would freeze unless the PERSTn pin is first re-configured for gpio use.
Current workaround in U-Boot throws away the existing upstream pinctrl and replace it to only configure gpio func on the PERSTn pin. This also leaves the CLKREQn and WAKEn to incorrectly use gpio func.
With this series I try to cleanup the old workaround that I added without much insight beyond: device no longer freeze and nvme "works" :-)
I've wondered in the past what the purpose of the gpio-ranges properties was. I never considered that they could be used to automatically mux the pins for GPIO since the gpio-ranges mapping provides no indication of what the correct pin settings arefor the GPIO pins.
The gpio-ranges prop is only used to provide details on how the pins are mapped between gpio and pin controller. It is not used to change any muxing. But the property is required to make the U-Boot helpers pinctrl_gpio_request()/pinctrl_gpio_free() work and figure out what pinctrl udevice to use if another driver use e.g. gpio_request_by_name() to request use of a pin for gpio use.
But that means you rely on the Linux (and now U-Boot) driver interfaces. What I was saying is that this is all rather implicit and not explicitly documented in the device tree bindings.
I fully understand this, however, from a developer perspective and seeing how there is no API to configure pinmux from code but there is an API to request and work with gpio, I would expect that the abstraction layer handle anything needed to be able to use the returned gpio.
Keep in mind that this series should not change any behavior except for the special case when DT pinctrl may configure a pin for non-gpio function and later U-Boot request the pin to be used for gpio.
To me such a case would be suspicious. What would be the purpose of such a configuration? And wouldn't this mean that you now implictly rely on the gpio being requested after the pinctl config has been applied?
Main purpose is to ensure the U-Boot gpio API works as expected for debugging and development purposes. For final DTs I am expecting that pinctrl is configured correctly.
Regards, Jonas
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
Well, it removes the incentive to fix the upstream DTs and would make it harder to notice missing pinctrls in the DTs.
I can understand that, but gpio pinmux is default so any missing pinctrl would still cause issues, this changes nothing in that regard.
This only affect if pinctrl exists and pin gets configured for non-gpio function use. And later a driver/cmd tries to use such pin for gpio use. Before this patch the use of gpio_request() would not change the pin mux for gpio use. After this series when code explicily request a pin for gpio use the pinctrl driver gets notified that it should change pinmux of the pin for gpio use.
Regards, Jonas
Cheers,
Mark
Alex > > With the gpio and pinctrl ops implemented this series also remove a PCIe > reset-gpios related device lock-up workaround from board u-boot.dtsi. > > PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently > define gpio-ranges props and is affected by this series. > > A follow up series adding support for the pinmux status cmd will also > add gpio-ranges props for remaining RK SoCs. > > Jonas Karlman (4): > pinctrl: rockchip: Add gpio_request_enable() ops > gpio: rockchip: Add request() ops > rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround > rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround > > arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- > arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- > drivers/gpio/rk_gpio.c | 10 ++++++ > .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ > 4 files changed, 41 insertions(+), 24 deletions(-) >
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Alex
Regards, Jonas
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Date: Sun, 12 May 2024 21:49:21 +0200 From: Alex Bee knaerzche@gmail.com
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
Right. See the reply I just sent to Jonas. I think that is the right approach. Probably means that the upstream rk3568-pinctrl.dtsi should grow separate nodes for when the PCIe controller is in EP or RC mode. Or maybe the perstn configuration should be split out from the clkreqn/waken configuration like how buttonrstn is configured.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Alex
Regards, Jonas
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote:
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpio
And with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpio
Not sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Regards, Jonas
Alex
Regards, Jonas
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Am 12.05.24 um 23:37 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote:
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
Yeah, sure: but that certainly should be done/happen first, before it's removed here. Everybody knows how long that takes, patches being forgotten ....
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpio
And with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpio
Not sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253 GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of blobs involved. for RK35xx SoCs - so maybe something switches the func for this pin for some reason.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Generally speaking: device trees _must_ represent the actual hardware completely independent from any driver or any software. They are NOT helpers or configuration for drivers. Drivers can use them and have to adapt to them - not vice versa. So: Being as explict as possible is a must. At some point, it is planned, to split whole DT "subsystem" from the linux kernel. I also have a vague remembering (some mailing list discussion I can't find right now), that this whole implicit function switching of pins is sort of "not welcome" in linux anymore. So adding it here also is sort of a "step back" from that POV.
Alex
[0] https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3...
Regards, Jonas
Alex
Regards, Jonas
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
On 2024-05-13 00:34, Alex Bee wrote:
Am 12.05.24 um 23:37 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote:
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman:
This series add gpio request() and pinctrl gpio_request_enable() ops so that a gpio requested pin automatically use gpio pinmux and U-Boot behaves more similar to Linux kernel.
I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
Yeah, sure: but that certainly should be done/happen first, before it's removed here. Everybody knows how long that takes, patches being forgotten ....
I am fully aware of patches being forgotten :-)
Still, here I try to cleanup a bad workaround that I probably never should have added in the first place.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpio
And with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpio
Not sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253 GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of blobs involved. for RK35xx SoCs - so maybe something switches the func for this pin for some reason.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Generally speaking: device trees _must_ represent the actual hardware completely independent from any driver or any software. They are NOT helpers or configuration for drivers. Drivers can use them and have to adapt to them - not vice versa. So: Being as explict as possible is a must.
I am fully aware of this, and I would argue that the DT already is very explicit about the HW in regard to PCIe3x2.
The pinctrl function/group explains what function needs to be activated to route PCIe3x2 related signals to the PCIe controller. And the reset-gpios explains what pin is used for PERST# signal.
However, if we instead are too explicit in the pinctrl function/group and say that that the PERST# pin should use gpio iomux function, aren't we then actually just describing something that is relevant for software/driver?
And actually ends up loosing information on how the pin can be configured to route the PERST# pin to the controller?
And as Mark pointed out, the PERST# pin should probably be configured for gpio output when controller works in RC mode, and should use PERSTn func when controller works in EP mode.
Guess that could/should possible be described as different pinctrl states or if I am not mistaken EP is described as a separate device using a different compatible.
At some point, it is planned, to split whole DT "subsystem" from the linux kernel. I also have a vague remembering (some mailing list discussion I can't find right now), that this whole implicit function switching of pins is sort of "not welcome" in linux anymore. So adding it here also is sort of a "step back" from that POV.
I was not expecting this much backlash on this series, so thanks for some concrete feedback on why we maybe should not add the gpio request() ops :-)
Maybe I should just drop this and only keep the follow up series.
Regards, Jonas
Alex
[0] https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3...
Regards, Jonas
Alex
Regards, Jonas
Alex
With the gpio and pinctrl ops implemented this series also remove a PCIe reset-gpios related device lock-up workaround from board u-boot.dtsi.
PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently define gpio-ranges props and is affected by this series.
A follow up series adding support for the pinmux status cmd will also add gpio-ranges props for remaining RK SoCs.
Jonas Karlman (4): pinctrl: rockchip: Add gpio_request_enable() ops gpio: rockchip: Add request() ops rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- drivers/gpio/rk_gpio.c | 10 ++++++ .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+), 24 deletions(-)
Am 13.05.24 um 01:22 schrieb Jonas Karlman:
On 2024-05-13 00:34, Alex Bee wrote:
Am 12.05.24 um 23:37 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote:
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote:
Hi Jonas,
Am 11.05.24 um 13:28 schrieb Jonas Karlman: > This series add gpio request() and pinctrl gpio_request_enable() ops so > that a gpio requested pin automatically use gpio pinmux and U-Boot > behaves more similar to Linux kernel. I'm not sure that's a good idea. While linux does it the same way, we really shouldn't expect every software/os/ … which uses DT (now or in future) to implicitly switch the pin function when using a pin as gpio. So the real fix would probably be to add the the correct pinctrl settings to the upstream DT of those boards and sync it later on (not sure those if those SoCs already using OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
Yeah, sure: but that certainly should be done/happen first, before it's removed here. Everybody knows how long that takes, patches being forgotten ....
I am fully aware of patches being forgotten :-)
Still, here I try to cleanup a bad workaround that I probably never should have added in the first place.
Why is it bad? It's incomplete and the correct pin configuration should have been added to the upstream board DT in the first place. That's what *-u-boot.dtsi are doing at times :) It's not as worse as adding implicit pin function switching to a driver to compensate the incomplete/incorrect upstream board device tree, imho.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpioAnd with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpioNot sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253 GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of blobs involved. for RK35xx SoCs - so maybe something switches the func for this pin for some reason.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Generally speaking: device trees _must_ represent the actual hardware completely independent from any driver or any software. They are NOT helpers or configuration for drivers. Drivers can use them and have to adapt to them - not vice versa. So: Being as explict as possible is a must.
I am fully aware of this, and I would argue that the DT already is very explicit about the HW in regard to PCIe3x2.
The pinctrl function/group explains what function needs to be activated to route PCIe3x2 related signals to the PCIe controller. And the reset-gpios explains what pin is used for PERST# signal.
However, if we instead are too explicit in the pinctrl function/group and say that that the PERST# pin should use gpio iomux function, aren't we then actually just describing something that is relevant for software/driver?
And actually ends up loosing information on how the pin can be configured to route the PERST# pin to the controller?
And as Mark pointed out, the PERST# pin should probably be configured for gpio output when controller works in RC mode, and should use PERSTn func when controller works in EP mode.
Guess that could/should possible be described as different pinctrl states or if I am not mistaken EP is described as a separate device using a different compatible.
Actually I wouldn't be too surprised if the "reset-gpios" property does actually only exist to not have to define a extra pinctrl and the actual reset-functionality (i.e. pulling the pin up / down by the driver) isn't required at all. Though, untested. That would be similar to define a "cd-gpios" property for a (sd) mmc-controller which is a similar workaround for not switching the card detection pin to gpio func.
At some point, it is planned, to split whole DT "subsystem" from the linux kernel. I also have a vague remembering (some mailing list discussion I can't find right now), that this whole implicit function switching of pins is sort of "not welcome" in linux anymore. So adding it here also is sort of a "step back" from that POV.
I was not expecting this much backlash on this series, so thanks for some concrete feedback on why we maybe should not add the gpio request() ops :-)
I did my best but couldn't find the discussion I had in mind. The context was: pinctrl and gpio are different subsystems and one really shouldn't configure the other. That makes sense, I guess. That it exists for Rockchip in linux kernel might be related to the fact that this originally was a single driver which was splited up later.
Anyway: I hope we can agree on the point that the correct pinctrl settings for the controller of this board should make it to the upstream device tree in some way :D
Regards, Alex
Maybe I should just drop this and only keep the follow up series.
Regards, Jonas
Alex
[0] https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3...
Regards, Jonas
Alex
Regards, Jonas
Alex > With the gpio and pinctrl ops implemented this series also remove a PCIe > reset-gpios related device lock-up workaround from board u-boot.dtsi. > > PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently > define gpio-ranges props and is affected by this series. > > A follow up series adding support for the pinmux status cmd will also > add gpio-ranges props for remaining RK SoCs. > > Jonas Karlman (4): > pinctrl: rockchip: Add gpio_request_enable() ops > gpio: rockchip: Add request() ops > rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround > rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround > > arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- > arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- > drivers/gpio/rk_gpio.c | 10 ++++++ > .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ > 4 files changed, 41 insertions(+), 24 deletions(-) >
On 2024-05-22 16:18, Alex Bee wrote:
Am 13.05.24 um 01:22 schrieb Jonas Karlman:
On 2024-05-13 00:34, Alex Bee wrote:
Am 12.05.24 um 23:37 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote:
Am 11.05.24 um 20:47 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-11 19:44, Alex Bee wrote: > Hi Jonas, > > Am 11.05.24 um 13:28 schrieb Jonas Karlman: >> This series add gpio request() and pinctrl gpio_request_enable() ops so >> that a gpio requested pin automatically use gpio pinmux and U-Boot >> behaves more similar to Linux kernel. > I'm not sure that's a good idea. > While linux does it the same way, we really shouldn't expect every > software/os/ … which uses DT (now or in future) to implicitly switch the > pin function when using a pin as gpio. So the real fix would probably be > to add the the correct pinctrl settings to the upstream DT of those > boards and sync it later on (not sure those if those SoCs already using > OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now. I fully agree that the pinctrl for the problematic boards should be corrected in upstream DT, but that is a separate issue and should not block adding support for the request()/gpio_request_enable() ops.
While the pcie reset-gpios full board freeze that was my driving factor to fully implement the gpio request() ops it is not the only use case, using the gpio cmd on a pin that use a non-gpio pinmux is another.
Or do you see any technical issue with having the gpio request() ops implemented and having it ensure gpio pinmux is used on a gpio requested pin? Similar to how gpio/pinctrl is behaving in Linux and on some other platforms in U-Boot?
No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
Yeah, sure: but that certainly should be done/happen first, before it's removed here. Everybody knows how long that takes, patches being forgotten ....
I am fully aware of patches being forgotten :-)
Still, here I try to cleanup a bad workaround that I probably never should have added in the first place.
Why is it bad? It's incomplete and the correct pin configuration should have been added to the upstream board DT in the first place. That's what *-u-boot.dtsi are doing at times :) It's not as worse as adding implicit pin function switching to a driver to compensate the incomplete/incorrect upstream board device tree, imho.
From my point-of-view the upstream pinctrl is more correct and I wrongly
added a patch that completely disregarded existing pinctrl just to fix pci enumeration in U-Boot when no pci device is connected without thinking of how it could affect linux/bsd, hence why I now think that old workaround/hack is something bad and something I want to cleanup/remove. In my mind the current pinctrl use for pcie3 in U-Boot is describing the hw much worse than what is done in upstream Linux.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpioAnd with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpioNot sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253 GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of blobs involved. for RK35xx SoCs - so maybe something switches the func for this pin for some reason.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Generally speaking: device trees _must_ represent the actual hardware completely independent from any driver or any software. They are NOT helpers or configuration for drivers. Drivers can use them and have to adapt to them - not vice versa. So: Being as explict as possible is a must.
I am fully aware of this, and I would argue that the DT already is very explicit about the HW in regard to PCIe3x2.
The pinctrl function/group explains what function needs to be activated to route PCIe3x2 related signals to the PCIe controller. And the reset-gpios explains what pin is used for PERST# signal.
However, if we instead are too explicit in the pinctrl function/group and say that that the PERST# pin should use gpio iomux function, aren't we then actually just describing something that is relevant for software/driver?
And actually ends up loosing information on how the pin can be configured to route the PERST# pin to the controller?
And as Mark pointed out, the PERST# pin should probably be configured for gpio output when controller works in RC mode, and should use PERSTn func when controller works in EP mode.
Guess that could/should possible be described as different pinctrl states or if I am not mistaken EP is described as a separate device using a different compatible.
Actually I wouldn't be too surprised if the "reset-gpios" property does actually only exist to not have to define a extra pinctrl and the actual reset-functionality (i.e. pulling the pin up / down by the driver) isn't required at all. Though, untested. That would be similar to define a "cd-gpios" property for a (sd) mmc-controller which is a similar workaround for not switching the card detection pin to gpio func.
From what I could test the driver need to mux the pin for gpio output
use when we use the controller in RC mode and the pin should be muxed to pcie func when the controller is used in EP mode.
Leaving the pin muxed for pcie func when we use the controller in RC mode seem to make the board freeze.
At some point, it is planned, to split whole DT "subsystem" from the linux kernel. I also have a vague remembering (some mailing list discussion I can't find right now), that this whole implicit function switching of pins is sort of "not welcome" in linux anymore. So adding it here also is sort of a "step back" from that POV.
I was not expecting this much backlash on this series, so thanks for some concrete feedback on why we maybe should not add the gpio request() ops :-)
I did my best but couldn't find the discussion I had in mind. The context was: pinctrl and gpio are different subsystems and one really shouldn't configure the other. That makes sense, I guess. That it exists for Rockchip in linux kernel might be related to the fact that this originally was a single driver which was splited up later.
From what I can find in the Linux pinctrl documentation it seem to suggest that
the gpiolib driver should actually hide pinmuxing from a gpio consumer driver.
See "Pin control interaction with the GPIO subsystem":
""" NOTE that platforms and individual drivers shall NOT request GPIO pins to be controlled e.g. muxed in. Instead, implement a proper gpiolib driver and have that driver request proper muxing and other control for its pins. """
https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#pin-contr...
And from "Drivers needing both pin control and GPIOs":
""" But there are also situations where it makes sense for the GPIO subsystem to communicate directly with the pinctrl subsystem, using the latter as a back-end. This is when the GPIO driver may call out to the functions described in the section Pin control interaction with the GPIO subsystem above. This only involves per-pin multiplexing, and will be completely hidden behind the gpiod_*() function namespace. In this case, the driver need not interact with the pin control subsystem at all.
If a pin control driver and a GPIO driver is dealing with the same pins and the use cases involve multiplexing, you MUST implement the pin controller as a back-end for the GPIO driver like this, unless your hardware design is such that the GPIO controller can override the pin controller’s multiplexing state through hardware without the need to interact with the pin control system. """
https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#drivers-n...
Anyway: I hope we can agree on the point that the correct pinctrl settings for the controller of this board should make it to the upstream device tree in some way :D
Fully agree that it should be correct in upstream, my point is that on one hand I actually think it already is correct in upstream and that changing the reset pin to use gpio func as default instead would then be done to assist software, and not correctly describe the hw.
On the other hand, if I am not mistaken to use the controller in EP mode another compatible/device node needs to be described and if that is the case I can fully agree that the device node for RC mode could include pinctrl to mux the reset pin for gpio func.
Regardless on what is to be done with pinctrl in DT, I still think it will be good to implement these gpio/pinctrl ops in U-Boot to closer match "a proper gpiolib driver" as suggested in the Linux pinctrl documentation where such driver request proper muxing and other control for its pins.
Anyway, I will merge the two gpio/pinctrl series and rearrange patches, and also drop any DT changes in a v2.
Regards, Jonas
Regards, Alex
Maybe I should just drop this and only keep the follow up series.
Regards, Jonas
Alex
[0] https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3...
Regards, Jonas
Alex
Regards, Jonas
> Alex >> With the gpio and pinctrl ops implemented this series also remove a PCIe >> reset-gpios related device lock-up workaround from board u-boot.dtsi. >> >> PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently >> define gpio-ranges props and is affected by this series. >> >> A follow up series adding support for the pinmux status cmd will also >> add gpio-ranges props for remaining RK SoCs. >> >> Jonas Karlman (4): >> pinctrl: rockchip: Add gpio_request_enable() ops >> gpio: rockchip: Add request() ops >> rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround >> rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround >> >> arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- >> arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- >> drivers/gpio/rk_gpio.c | 10 ++++++ >> .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ >> 4 files changed, 41 insertions(+), 24 deletions(-) >>
Am 22.05.24 um 18:20 schrieb Jonas Karlman:
On 2024-05-22 16:18, Alex Bee wrote:
Am 13.05.24 um 01:22 schrieb Jonas Karlman:
On 2024-05-13 00:34, Alex Bee wrote:
Am 12.05.24 um 23:37 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote:
Am 11.05.24 um 20:47 schrieb Jonas Karlman: > Hi Alex, > > On 2024-05-11 19:44, Alex Bee wrote: >> Hi Jonas, >> >> Am 11.05.24 um 13:28 schrieb Jonas Karlman: >>> This series add gpio request() and pinctrl gpio_request_enable() ops so >>> that a gpio requested pin automatically use gpio pinmux and U-Boot >>> behaves more similar to Linux kernel. >> I'm not sure that's a good idea. >> While linux does it the same way, we really shouldn't expect every >> software/os/ … which uses DT (now or in future) to implicitly switch the >> pin function when using a pin as gpio. So the real fix would probably be >> to add the the correct pinctrl settings to the upstream DT of those >> boards and sync it later on (not sure those if those SoCs already using >> OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now. > I fully agree that the pinctrl for the problematic boards should be > corrected in upstream DT, but that is a separate issue and should not > block adding support for the request()/gpio_request_enable() ops. > > While the pcie reset-gpios full board freeze that was my driving factor > to fully implement the gpio request() ops it is not the only use case, > using the gpio cmd on a pin that use a non-gpio pinmux is another. > > Or do you see any technical issue with having the gpio request() ops > implemented and having it ensure gpio pinmux is used on a gpio requested > pin? Similar to how gpio/pinctrl is behaving in Linux and on some other > platforms in U-Boot? No, no general ("technical") issue with adding a .request hook to the gpio driver. But now you are now moving the original workaround to an even more invisible place which does things implicitly. Maybe just don't remove the pinctrl from the boards u-boot-dtsi's - just replace it with
&pcie30x2m1_pins { rockchip,pins = <2 RK_PD4 4 &pcfg_pull_none>, <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, <2 RK_PD5 4 &pcfg_pull_none>; };
Even if it would (now) work without. It, at least, documents that there are things left to do for the upstream DT.
This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
Yeah, sure: but that certainly should be done/happen first, before it's removed here. Everybody knows how long that takes, patches being forgotten ....
I am fully aware of patches being forgotten :-)
Still, here I try to cleanup a bad workaround that I probably never should have added in the first place.
Why is it bad? It's incomplete and the correct pin configuration should have been added to the upstream board DT in the first place. That's what *-u-boot.dtsi are doing at times :) It's not as worse as adding implicit pin function switching to a driver to compensate the incomplete/incorrect upstream board device tree, imho.
From my point-of-view the upstream pinctrl is more correct and I wrongly added a patch that completely disregarded existing pinctrl just to fix pci enumeration in U-Boot when no pci device is connected without thinking of how it could affect linux/bsd, hence why I now think that old workaround/hack is something bad and something I want to cleanup/remove.
Yeah, sure. That's why I suggested[0] to replace the current override like
...
-&pcie3x2 { - pinctrl-0 = <&pcie3x2_reset_h>; -}; - -&pinctrl { - pcie { - pcie3x2_reset_h: pcie3x2-reset-h { - rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>; - }; - }; +&pcie30x2m1_pins { + rockchip,pins = + <2 RK_PD4 4 &pcfg_pull_none>, + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, + <2 RK_PD5 4 &pcfg_pull_none>; };
....
The proper upstream solution should probably add another pinctrl-set to rk3568-pinctrl.dtsi - someting like ...
+ /omit-if-no-ref/ + pcie30x2m1_ep_pins: pcie30x2m1-ep-pins { + rockchip,pins = + /* pcie30x2_clkreqnm1 */ + <2 RK_PD4 4 &pcfg_pull_none>, + /* pcie30x2_perstnm1 */ + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, + /* pcie30x2_wakenm1 */ + <2 RK_PD5 4 &pcfg_pull_none>; + }; +
... and boards needing this just switch from pcie30x2m1_pins to pcie30x2m1_ep_pins
In my mind the current pinctrl use for pcie3 in U-Boot is describing the hw much worse than what is done in upstream Linux.
No, not really. It describes pin settings wrong. The DT suggests GPIO2_D6 is used with func4, but in reality its used with func0 (gpio). The only reason that works correctly is probe order: if the pinctrl driver would probe after the gpio driver (which is sort of impossible for Rockchip) the pin would be switched to func4 and the controller wouldn't work.
What you were saying in reply to Mark's email is not completely true: Not all pins are initialized with gpio func as default. It actually depends on the pin which function the bootrom sets initially. In case of RK356x's GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So, in fact, you are changing it's function when implicitly setting it's func to gpio (func0).
Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpioAnd with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpioNot sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253 GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of blobs involved. for RK35xx SoCs - so maybe something switches the func for this pin for some reason.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Generally speaking: device trees _must_ represent the actual hardware completely independent from any driver or any software. They are NOT helpers or configuration for drivers. Drivers can use them and have to adapt to them - not vice versa. So: Being as explict as possible is a must.
I am fully aware of this, and I would argue that the DT already is very explicit about the HW in regard to PCIe3x2.
The pinctrl function/group explains what function needs to be activated to route PCIe3x2 related signals to the PCIe controller. And the reset-gpios explains what pin is used for PERST# signal.
However, if we instead are too explicit in the pinctrl function/group and say that that the PERST# pin should use gpio iomux function, aren't we then actually just describing something that is relevant for software/driver?
And actually ends up loosing information on how the pin can be configured to route the PERST# pin to the controller?
And as Mark pointed out, the PERST# pin should probably be configured for gpio output when controller works in RC mode, and should use PERSTn func when controller works in EP mode.
Guess that could/should possible be described as different pinctrl states or if I am not mistaken EP is described as a separate device using a different compatible.
Actually I wouldn't be too surprised if the "reset-gpios" property does actually only exist to not have to define a extra pinctrl and the actual reset-functionality (i.e. pulling the pin up / down by the driver) isn't required at all. Though, untested. That would be similar to define a "cd-gpios" property for a (sd) mmc-controller which is a similar workaround for not switching the card detection pin to gpio func.
From what I could test the driver need to mux the pin for gpio output use when we use the controller in RC mode and the pin should be muxed to pcie func when the controller is used in EP mode.
Leaving the pin muxed for pcie func when we use the controller in RC mode seem to make the board freeze.
From what you're saying (maybe I should test myself at some point :)
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_up>;
instead of
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>;
could do the trick and no "reset-gpios" required at all.
At some point, it is planned, to split whole DT "subsystem" from the linux kernel. I also have a vague remembering (some mailing list discussion I can't find right now), that this whole implicit function switching of pins is sort of "not welcome" in linux anymore. So adding it here also is sort of a "step back" from that POV.
I was not expecting this much backlash on this series, so thanks for some concrete feedback on why we maybe should not add the gpio request() ops :-)
I did my best but couldn't find the discussion I had in mind. The context was: pinctrl and gpio are different subsystems and one really shouldn't configure the other. That makes sense, I guess. That it exists for Rockchip in linux kernel might be related to the fact that this originally was a single driver which was splited up later.
From what I can find in the Linux pinctrl documentation it seem to suggest that the gpiolib driver should actually hide pinmuxing from a gpio consumer driver.
See "Pin control interaction with the GPIO subsystem":
""" NOTE that platforms and individual drivers shall NOT request GPIO pins to be controlled e.g. muxed in. Instead, implement a proper gpiolib driver and have that driver request proper muxing and other control for its pins. """
https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#pin-contr...
And from "Drivers needing both pin control and GPIOs":
""" But there are also situations where it makes sense for the GPIO subsystem to communicate directly with the pinctrl subsystem, using the latter as a back-end. This is when the GPIO driver may call out to the functions described in the section Pin control interaction with the GPIO subsystem above. This only involves per-pin multiplexing, and will be completely hidden behind the gpiod_*() function namespace. In this case, the driver need not interact with the pin control subsystem at all.
If a pin control driver and a GPIO driver is dealing with the same pins and the use cases involve multiplexing, you MUST implement the pin controller as a back-end for the GPIO driver like this, unless your hardware design is such that the GPIO controller can override the pin controller’s multiplexing state through hardware without the need to interact with the pin control system. """
https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#drivers-n...
Anyway: I hope we can agree on the point that the correct pinctrl settings for the controller of this board should make it to the upstream device tree in some way :D
Fully agree that it should be correct in upstream, my point is that on one hand I actually think it already is correct in upstream and that changing the reset pin to use gpio func as default instead would then be done to assist software, and not correctly describe the hw.
On the other hand, if I am not mistaken to use the controller in EP mode another compatible/device node needs to be described and if that is the case I can fully agree that the device node for RC mode could include pinctrl to mux the reset pin for gpio func.
Regardless on what is to be done with pinctrl in DT, I still think it will be good to implement these gpio/pinctrl ops in U-Boot to closer match "a proper gpiolib driver" as suggested in the Linux pinctrl documentation where such driver request proper muxing and other control for its pins.
I can't exactly follow where in the documenation you are reading this, but adding the ops should not be an issue, but DT shouldn't depend on it.
Regards
Alex
[0] https://lore.kernel.org/all/1b3b8c24-a3d9-4c36-a72f-45baee63f385@gmail.com/
Anyway, I will merge the two gpio/pinctrl series and rearrange patches, and also drop any DT changes in a v2.
Regards, Jonas
Regards, Alex
Maybe I should just drop this and only keep the follow up series.
Regards, Jonas
Alex
[0] https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3...
Regards, Jonas
Alex
> Regards, > Jonas > >> Alex >>> With the gpio and pinctrl ops implemented this series also remove a PCIe >>> reset-gpios related device lock-up workaround from board u-boot.dtsi. >>> >>> PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently >>> define gpio-ranges props and is affected by this series. >>> >>> A follow up series adding support for the pinmux status cmd will also >>> add gpio-ranges props for remaining RK SoCs. >>> >>> Jonas Karlman (4): >>> pinctrl: rockchip: Add gpio_request_enable() ops >>> gpio: rockchip: Add request() ops >>> rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround >>> rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround >>> >>> arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- >>> arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- >>> drivers/gpio/rk_gpio.c | 10 ++++++ >>> .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 +++++++++++++++++++ >>> 4 files changed, 41 insertions(+), 24 deletions(-) >>>
Hi Alex, Jonas,
On 2024/5/23 03:46, Alex Bee wrote:
Am 22.05.24 um 18:20 schrieb Jonas Karlman:
On 2024-05-22 16:18, Alex Bee wrote:
Am 13.05.24 um 01:22 schrieb Jonas Karlman:
On 2024-05-13 00:34, Alex Bee wrote:
Am 12.05.24 um 23:37 schrieb Jonas Karlman:
Hi Alex,
On 2024-05-12 21:49, Alex Bee wrote: > Am 11.05.24 um 20:47 schrieb Jonas Karlman: >> Hi Alex, >> >> On 2024-05-11 19:44, Alex Bee wrote: >>> Hi Jonas, >>> >>> Am 11.05.24 um 13:28 schrieb Jonas Karlman: >>>> This series add gpio request() and pinctrl >>>> gpio_request_enable() ops so >>>> that a gpio requested pin automatically use gpio pinmux and >>>> U-Boot >>>> behaves more similar to Linux kernel. >>> I'm not sure that's a good idea. >>> While linux does it the same way, we really shouldn't expect >>> every >>> software/os/ … which uses DT (now or in future) to implicitly >>> switch the >>> pin function when using a pin as gpio. So the real fix would >>> probably be >>> to add the the correct pinctrl settings to the upstream DT of >>> those >>> boards and sync it later on (not sure those if those SoCs >>> already using >>> OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now. >> I fully agree that the pinctrl for the problematic boards >> should be >> corrected in upstream DT, but that is a separate issue and >> should not >> block adding support for the request()/gpio_request_enable() ops. >> >> While the pcie reset-gpios full board freeze that was my >> driving factor >> to fully implement the gpio request() ops it is not the only >> use case, >> using the gpio cmd on a pin that use a non-gpio pinmux is another. >> >> Or do you see any technical issue with having the gpio >> request() ops >> implemented and having it ensure gpio pinmux is used on a gpio >> requested >> pin? Similar to how gpio/pinctrl is behaving in Linux and on >> some other >> platforms in U-Boot? > No, no general ("technical") issue with adding a .request hook > to the gpio > driver. But now you are now moving the original workaround to an > even more > invisible place which does things implicitly. Maybe just don't > remove the > pinctrl from the boards u-boot-dtsi's - just replace it with > > &pcie30x2m1_pins { > rockchip,pins = > <2 RK_PD4 4 &pcfg_pull_none>, > <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, > <2 RK_PD5 4 &pcfg_pull_none>; > }; > > Even if it would (now) work without. It, at least, documents > that there are > things left to do for the upstream DT. This is what I was doing when testing PCIe on ROCK 3B, I would still like to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
I do not see the point of keeping a workaround that no longer is needed, especially when there is plans to also adjust upstream DT.
Yeah, sure: but that certainly should be done/happen first, before it's removed here. Everybody knows how long that takes, patches being forgotten ....
I am fully aware of patches being forgotten :-)
Still, here I try to cleanup a bad workaround that I probably never should have added in the first place.
Why is it bad? It's incomplete and the correct pin configuration should have been added to the upstream board DT in the first place. That's what *-u-boot.dtsi are doing at times :) It's not as worse as adding implicit pin function switching to a driver to compensate the incomplete/incorrect upstream board device tree, imho.
From my point-of-view the upstream pinctrl is more correct and I wrongly added a patch that completely disregarded existing pinctrl just to fix pci enumeration in U-Boot when no pci device is connected without thinking of how it could affect linux/bsd, hence why I now think that old workaround/hack is something bad and something I want to cleanup/remove.
Yeah, sure. That's why I suggested[0] to replace the current override like
...
-&pcie3x2 { - pinctrl-0 = <&pcie3x2_reset_h>; -};
-&pinctrl { - pcie { - pcie3x2_reset_h: pcie3x2-reset-h { - rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>; - }; - }; +&pcie30x2m1_pins { + rockchip,pins = + <2 RK_PD4 4 &pcfg_pull_none>, + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, + <2 RK_PD5 4 &pcfg_pull_none>; };
The PERST is for sure should work as GPIO, and the same as WAKE;
for CLKREQ, only those board want to support L1SS need to work as function IO,
in most case, it's fine to work as GPIO, so below update would be a fix for -u-boot.dtsi.
&pcie3x2 { /delete-property/ pinctrl-names /delete-property/ pinctrl-0; };
And the update in upstream linux dts should be the correct fix.
....
The proper upstream solution should probably add another pinctrl-set to rk3568-pinctrl.dtsi - someting like ...
+ /omit-if-no-ref/ + pcie30x2m1_ep_pins: pcie30x2m1-ep-pins { + rockchip,pins = + /* pcie30x2_clkreqnm1 */ + <2 RK_PD4 4 &pcfg_pull_none>, + /* pcie30x2_perstnm1 */ + <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>, + /* pcie30x2_wakenm1 */ + <2 RK_PD5 4 &pcfg_pull_none>; + };
... and boards needing this just switch from pcie30x2m1_pins to pcie30x2m1_ep_pins
In my mind the current pinctrl use for pcie3 in U-Boot is describing the hw much worse than what is done in upstream Linux.
No, not really. It describes pin settings wrong. The DT suggests GPIO2_D6 is used with func4, but in reality its used with func0 (gpio). The only reason that works correctly is probe order: if the pinctrl driver would probe after the gpio driver (which is sort of impossible for Rockchip) the pin would be switched to func4 and the controller wouldn't work.
> What you were saying in reply to Mark's email is not completely > true: Not > all pins are initialized with gpio func as default. It actually > depends on > the pin which function the bootrom sets initially. In case of > RK356x's > GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for > instance. So, > in fact, you are changing it's function when implicitly setting > it's func > to gpio (func0). Sure, bootrom will change pin func on some pins so that it e.g. can read from storage etc, but in general gpio mux is what most pins will use after POR.
On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
=> pinmux status GPIO2_D4 : gpio GPIO2_D5 : gpio GPIO2_D6 : gpio
And with this after a "pci enum":
=> pinmux status GPIO2_D4 : func-4 GPIO2_D5 : func-4 GPIO2_D6 : gpio
Not sure why your board would use BT656_D6M0 (func2) after POR unless there is some other code writing to the IOMUX regs.
Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253 GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of blobs involved. for RK35xx SoCs - so maybe something switches the func for this pin for some reason.
The only thing this series changes is that when a U-Boot drivers use e.g. gpio_request_by_name("reset-gpios") the referenced pin in DT will implicitly be configured for gpio func.
From my point of view, I would prefer the dts pinctrl as the only source to set the IOMUX, this is another source of setting IOMUX, GPIO and PINCTRL should be totally separate, and the DT should be the one describe the HW function.
Thanks, - Kever
I would still like to understand if there is any other reason, not related to dropping current "bad" PCIe DT workaround, to not to adopt this implicit configuration and match Linux kernel?
Generally speaking: device trees _must_ represent the actual hardware completely independent from any driver or any software. They are NOT helpers or configuration for drivers. Drivers can use them and have to adapt to them - not vice versa. So: Being as explict as possible is a must.
I am fully aware of this, and I would argue that the DT already is very explicit about the HW in regard to PCIe3x2.
The pinctrl function/group explains what function needs to be activated to route PCIe3x2 related signals to the PCIe controller. And the reset-gpios explains what pin is used for PERST# signal.
However, if we instead are too explicit in the pinctrl function/group and say that that the PERST# pin should use gpio iomux function, aren't we then actually just describing something that is relevant for software/driver?
And actually ends up loosing information on how the pin can be configured to route the PERST# pin to the controller?
And as Mark pointed out, the PERST# pin should probably be configured for gpio output when controller works in RC mode, and should use PERSTn func when controller works in EP mode.
Guess that could/should possible be described as different pinctrl states or if I am not mistaken EP is described as a separate device using a different compatible.
Actually I wouldn't be too surprised if the "reset-gpios" property does actually only exist to not have to define a extra pinctrl and the actual reset-functionality (i.e. pulling the pin up / down by the driver) isn't required at all. Though, untested. That would be similar to define a "cd-gpios" property for a (sd) mmc-controller which is a similar workaround for not switching the card detection pin to gpio func.
From what I could test the driver need to mux the pin for gpio output use when we use the controller in RC mode and the pin should be muxed to pcie func when the controller is used in EP mode.
Leaving the pin muxed for pcie func when we use the controller in RC mode seem to make the board freeze.
From what you're saying (maybe I should test myself at some point :)
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_up>;
instead of
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>;
could do the trick and no "reset-gpios" required at all.
At some point, it is planned, to split whole DT "subsystem" from the linux kernel. I also have a vague remembering (some mailing list discussion I can't find right now), that this whole implicit function switching of pins is sort of "not welcome" in linux anymore. So adding it here also is sort of a "step back" from that POV.
I was not expecting this much backlash on this series, so thanks for some concrete feedback on why we maybe should not add the gpio request() ops :-)
I did my best but couldn't find the discussion I had in mind. The context was: pinctrl and gpio are different subsystems and one really shouldn't configure the other. That makes sense, I guess. That it exists for Rockchip in linux kernel might be related to the fact that this originally was a single driver which was splited up later.
From what I can find in the Linux pinctrl documentation it seem to suggest that the gpiolib driver should actually hide pinmuxing from a gpio consumer driver.
See "Pin control interaction with the GPIO subsystem":
""" NOTE that platforms and individual drivers shall NOT request GPIO pins to be controlled e.g. muxed in. Instead, implement a proper gpiolib driver and have that driver request proper muxing and other control for its pins. """
https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#pin-contr...
And from "Drivers needing both pin control and GPIOs":
""" But there are also situations where it makes sense for the GPIO subsystem to communicate directly with the pinctrl subsystem, using the latter as a back-end. This is when the GPIO driver may call out to the functions described in the section Pin control interaction with the GPIO subsystem above. This only involves per-pin multiplexing, and will be completely hidden behind the gpiod_*() function namespace. In this case, the driver need not interact with the pin control subsystem at all.
If a pin control driver and a GPIO driver is dealing with the same pins and the use cases involve multiplexing, you MUST implement the pin controller as a back-end for the GPIO driver like this, unless your hardware design is such that the GPIO controller can override the pin controller’s multiplexing state through hardware without the need to interact with the pin control system. """
https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#drivers-n...
Anyway: I hope we can agree on the point that the correct pinctrl settings for the controller of this board should make it to the upstream device tree in some way :D
Fully agree that it should be correct in upstream, my point is that on one hand I actually think it already is correct in upstream and that changing the reset pin to use gpio func as default instead would then be done to assist software, and not correctly describe the hw.
On the other hand, if I am not mistaken to use the controller in EP mode another compatible/device node needs to be described and if that is the case I can fully agree that the device node for RC mode could include pinctrl to mux the reset pin for gpio func.
Regardless on what is to be done with pinctrl in DT, I still think it will be good to implement these gpio/pinctrl ops in U-Boot to closer match "a proper gpiolib driver" as suggested in the Linux pinctrl documentation where such driver request proper muxing and other control for its pins.
I can't exactly follow where in the documenation you are reading this, but adding the ops should not be an issue, but DT shouldn't depend on it.
Regards
Alex
[0] https://lore.kernel.org/all/1b3b8c24-a3d9-4c36-a72f-45baee63f385@gmail.com/
Anyway, I will merge the two gpio/pinctrl series and rearrange patches, and also drop any DT changes in a v2.
Regards, Jonas
Regards, Alex
Maybe I should just drop this and only keep the follow up series.
Regards, Jonas
Alex
[0] https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3...
Regards, Jonas
> Alex > >> Regards, >> Jonas >> >>> Alex >>>> With the gpio and pinctrl ops implemented this series also >>>> remove a PCIe >>>> reset-gpios related device lock-up workaround from board >>>> u-boot.dtsi. >>>> >>>> PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs >>>> that currently >>>> define gpio-ranges props and is affected by this series. >>>> >>>> A follow up series adding support for the pinmux status cmd >>>> will also >>>> add gpio-ranges props for remaining RK SoCs. >>>> >>>> Jonas Karlman (4): >>>> pinctrl: rockchip: Add gpio_request_enable() ops >>>> gpio: rockchip: Add request() ops >>>> rockchip: rk3568-rock-3a: Drop PCIe reset-gpios >>>> workaround >>>> rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios >>>> workaround >>>> >>>> arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi | 12 ------- >>>> arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 12 ------- >>>> drivers/gpio/rk_gpio.c | 10 ++++++ >>>> .../pinctrl/rockchip/pinctrl-rockchip-core.c | 31 >>>> +++++++++++++++++++ >>>> 4 files changed, 41 insertions(+), 24 deletions(-) >>>>
participants (4)
-
Alex Bee -
Jonas Karlman -
Kever Yang -
Mark Kettenis