| Message ID | 20240327152531.814392-1-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: dw-rockchip: Fix GPIO initialization flag | expand |
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>
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 > >
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
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);
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(-)