[PATCH] PCI: dw-rockchip: Fix GPIO initialization flag

[PATCH] PCI: dw-rockchip: Fix GPIO initialization flag

[Niklas Cassel]

PERST is active low according to the PCIe specification.

However, the existing pcie-dw-rockchip.c driver does:
gpiod_set_value(..., 0); msleep(100); gpiod_set_value(..., 1);
When asserting + deasserting PERST.

This is of course wrong, but because all the device trees for this
compatible string have also incorrectly marked this GPIO as ACTIVE_HIGH:
$ git grep -B 10 reset-gpios arch/arm64/boot/dts/rockchip/rk3568*
$ git grep -B 10 reset-gpios arch/arm64/boot/dts/rockchip/rk3588*

The actual toggling of PERST is correct.
(And we cannot change it anyway, since that would break device tree
compatibility.)

However, this driver does request the GPIO to be initialized as
GPIOD_OUT_HIGH, which does cause a silly sequence where PERST gets
toggled back and forth for no good reason.

Fix this by requesting the GPIO to be initialized as GPIOD_OUT_LOW
(which for this driver means PERST asserted).

This will avoid an unnecessary signal change where PERST gets deasserted
(by devm_gpiod_get_optional()) and then gets asserted
(by rockchip_pcie_start_link()) just a few instructions later.

Before patch, debug prints on EP side, when booting RC:
[  845.606810] pci: PERST asserted by host!
[  852.483985] pci: PERST de-asserted by host!
[  852.503041] pci: PERST asserted by host!
[  852.610318] pci: PERST de-asserted by host!

After patch, debug prints on EP side, when booting RC:
[  125.107921] pci: PERST asserted by host!
[  132.111429] pci: PERST de-asserted by host!

Without this change, there is no guarantee that PERST will be asserted
while the core is performing a fundamental reset.
(E.g. if the bootloader would leave PERST deasserted.)

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index d6842141d384..a909e42b4273 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -240,7 +240,7 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
 		return PTR_ERR(rockchip->apb_base);
 
 	rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
-						     GPIOD_OUT_HIGH);
+						     GPIOD_OUT_LOW);
 	if (IS_ERR(rockchip->rst_gpio))
 		return PTR_ERR(rockchip->rst_gpio);
 
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: dw-rockchip: Fix GPIO initialization flag

[Jianfeng Liu]

Test on rock5b with RTL8822CE connected to M.2 E slot.
Before this patch RTL8822CE is not detected sometimes, and I tried to
fix it by a patch[1] but that could not solve it. And in that thread
Sebastian mentioned this fix. Now with this patch my pcie wifi can get
detected normally.

[1] https://lore.kernel.org/all/20240401081302.942742-1-liujianfeng1994@gmail.com/

Tested-by: Jianfeng Liu <liujianfeng1994@gmail.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: dw-rockchip: Fix GPIO initialization flag

[Manivannan Sadhasivam]

On Wed, Mar 27, 2024 at 04:25:31PM +0100, Niklas Cassel wrote:
> PERST is active low according to the PCIe specification.
> 
> However, the existing pcie-dw-rockchip.c driver does:
> gpiod_set_value(..., 0); msleep(100); gpiod_set_value(..., 1);
> When asserting + deasserting PERST.
> 
> This is of course wrong, but because all the device trees for this
> compatible string have also incorrectly marked this GPIO as ACTIVE_HIGH:
> $ git grep -B 10 reset-gpios arch/arm64/boot/dts/rockchip/rk3568*
> $ git grep -B 10 reset-gpios arch/arm64/boot/dts/rockchip/rk3588*
> 
> The actual toggling of PERST is correct.
> (And we cannot change it anyway, since that would break device tree
> compatibility.)
> 
> However, this driver does request the GPIO to be initialized as
> GPIOD_OUT_HIGH, which does cause a silly sequence where PERST gets
> toggled back and forth for no good reason.
> 
> Fix this by requesting the GPIO to be initialized as GPIOD_OUT_LOW
> (which for this driver means PERST asserted).
> 
> This will avoid an unnecessary signal change where PERST gets deasserted
> (by devm_gpiod_get_optional()) and then gets asserted
> (by rockchip_pcie_start_link()) just a few instructions later.
> 
> Before patch, debug prints on EP side, when booting RC:
> [  845.606810] pci: PERST asserted by host!
> [  852.483985] pci: PERST de-asserted by host!
> [  852.503041] pci: PERST asserted by host!
> [  852.610318] pci: PERST de-asserted by host!
> 
> After patch, debug prints on EP side, when booting RC:
> [  125.107921] pci: PERST asserted by host!
> [  132.111429] pci: PERST de-asserted by host!
> 
> Without this change, there is no guarantee that PERST will be asserted
> while the core is performing a fundamental reset.

There is no 'core' here, are you referring to the device?

> (E.g. if the bootloader would leave PERST deasserted.)
> 

I don't follow this last sentence. But even without that, the commit message
itself is descriptive enough.

> Signed-off-by: Niklas Cassel <cassel@kernel.org>

This is a legitimate bug fix. So you should add the fixes tag and CC stable to
get it backported.

But the patch itself looks fine to me.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index d6842141d384..a909e42b4273 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -240,7 +240,7 @@ static int rockchip_pcie_resource_get(struct platform_device *pdev,
>  		return PTR_ERR(rockchip->apb_base);
>  
>  	rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> -						     GPIOD_OUT_HIGH);
> +						     GPIOD_OUT_LOW);
>  	if (IS_ERR(rockchip->rst_gpio))
>  		return PTR_ERR(rockchip->rst_gpio);
>  
> -- 
> 2.44.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: dw-rockchip: Fix GPIO initialization flag

[Niklas Cassel]

On Tue, Apr 16, 2024 at 04:58:07PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 27, 2024 at 04:25:31PM +0100, Niklas Cassel wrote:
> > PERST is active low according to the PCIe specification.
> > 
> > However, the existing pcie-dw-rockchip.c driver does:
> > gpiod_set_value(..., 0); msleep(100); gpiod_set_value(..., 1);
> > When asserting + deasserting PERST.
> > 
> > This is of course wrong, but because all the device trees for this
> > compatible string have also incorrectly marked this GPIO as ACTIVE_HIGH:
> > $ git grep -B 10 reset-gpios arch/arm64/boot/dts/rockchip/rk3568*
> > $ git grep -B 10 reset-gpios arch/arm64/boot/dts/rockchip/rk3588*
> > 
> > The actual toggling of PERST is correct.
> > (And we cannot change it anyway, since that would break device tree
> > compatibility.)
> > 
> > However, this driver does request the GPIO to be initialized as
> > GPIOD_OUT_HIGH, which does cause a silly sequence where PERST gets
> > toggled back and forth for no good reason.
> > 
> > Fix this by requesting the GPIO to be initialized as GPIOD_OUT_LOW
> > (which for this driver means PERST asserted).
> > 
> > This will avoid an unnecessary signal change where PERST gets deasserted
> > (by devm_gpiod_get_optional()) and then gets asserted
> > (by rockchip_pcie_start_link()) just a few instructions later.
> > 
> > Before patch, debug prints on EP side, when booting RC:
> > [  845.606810] pci: PERST asserted by host!
> > [  852.483985] pci: PERST de-asserted by host!
> > [  852.503041] pci: PERST asserted by host!
> > [  852.610318] pci: PERST de-asserted by host!
> > 
> > After patch, debug prints on EP side, when booting RC:
> > [  125.107921] pci: PERST asserted by host!
> > [  132.111429] pci: PERST de-asserted by host!
> > 
> > Without this change, there is no guarantee that PERST will be asserted
> > while the core is performing a fundamental reset.

> There is no 'core' here, are you referring to the device?

If you at pcie-qcom.c, it does:
1) PERST# assert
2) core reset (using reset_control_assert() in ops->init())
3) PERST# deassert

So the EP is held in reset while the RC resets.

Right now, for dw-rockchip driver, there is no guarantee that
EP is held in reset when the core on RC side resets, which seems bad.


> 
> > (E.g. if the bootloader would leave PERST deasserted.)
> > 
> 
> I don't follow this last sentence. But even without that, the commit message
> itself is descriptive enough.

When I flash u-boot on my board, and boot using TFTP,
I can see that when u-boot jumps to Linux, PERST# is not asserted
and will not be asserted when PCIe RC driver loads.

So the scenario mentioned above can happen.

(If I don't boot using TFTP, PERST# is deasserted when PCIe RC driver loads.)

Anyway, I will remove or clarify this sentence.


> 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> This is a legitimate bug fix. So you should add the fixes tag and CC stable to
> get it backported.

Ok, will do in V2.


> 
> But the patch itself looks fine to me.
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thank you.


Kind regards,
Niklas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel