[PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[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.

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(-)

-- 
2.43.2

[PATCH 1/4] pinctrl: rockchip: Add gpio_request_enable() ops

[Jonas Karlman]

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;
-- 
2.43.2

[PATCH 2/4] gpio: rockchip: Add request() ops

[Jonas Karlman]

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,
-- 
2.43.2

[PATCH 3/4] rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround

[Jonas Karlman]

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;
-- 
2.43.2

Re: [PATCH 3/4] rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround

[Kever Yang]

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 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;

[PATCH 4/4] rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround

[Jonas Karlman]

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;
-- 
2.43.2

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Alex Bee]

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(-)
> 

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[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?

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(-)
>>
> 

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Mark Kettenis]

> 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(-)
> >>
> > 
> 
> 

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Jonas Karlman]

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(-)
>>>>
>>>
>>
>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Mark Kettenis]

> 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(-)
> >>>>
> >>>
> >>
> >>
> 
> 

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Jonas Karlman]

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(-)
>>>>>>
>>>>>
>>>>
>>>>
>>
>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Mark Kettenis]

> 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(-)
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>
> >>
> 
> 

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Alex Bee]

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(-)
>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Mark Kettenis]

> 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(-)
> >>>
> 

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[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.

> 
> 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(-)
>>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Alex Bee]

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-20220930P.PDF

> 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(-)
>>>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[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.

> 
>>> 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-20220930P.PDF
> 
>> 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(-)
>>>>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Alex Bee]

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  : 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.
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-20220930P.PDF
>>
>>> 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(-)
>>>>>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[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.
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  : 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.
> 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-control-interaction-with-the-gpio-subsystem

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-needing-both-pin-control-and-gpios

> 
> 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-20220930P.PDF
>>>
>>>> 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(-)
>>>>>>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Alex Bee]

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  : 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.
>> 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-control-interaction-with-the-gpio-subsystem
>
> 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-needing-both-pin-control-and-gpios
>
>> 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-20220930P.PDF
>>>>
>>>>> 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(-)
>>>>>>>>>

Re: [PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

[Kever Yang]

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-control-interaction-with-the-gpio-subsystem 
>>
>>
>> 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-needing-both-pin-control-and-gpios 
>>
>>
>>> 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-20220930P.PDF 
>>>>>
>>>>>
>>>>>> 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(-)
>>>>>>>>>>