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@xxxxxxxxxx> > > 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@xxxxxxxxxx> Thank you. Kind regards, Niklas
