Received-SPF: pass (google.com: domain of linux-kernel+bounces-119093-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 26 Mar 2024 14:47:30 +0100
From: Niklas Cassel <cassel@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, mhi@lists.linux.dev,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally
 during PERST# assert
Message-ID: <ZgLR8jWfBcZB8laa@ryzen>
References: <20240314-pci-epf-rework-v1-0-6134e6c1d491@linaro.org>
 <20240314-pci-epf-rework-v1-1-6134e6c1d491@linaro.org>
 <Zf2s9kTMlZncldWx@ryzen>
 <20240326074429.GC9565@thinkpad>
 <ZgKiUogkgrMwV1uD@x1-carbon>
 <20240326111021.GA13849@thinkpad>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240326111021.GA13849@thinkpad>

On Tue, Mar 26, 2024 at 04:40:21PM +0530, Manivannan Sadhasivam wrote:
> 
> I was planning to drop enable_resources() from Qcom driver once the DBI rework
> series gets merged. Because, the resource enablement during probe is currently
> done to avoid the crash that is bound to happen if registers are accessed during
> probe.
> 
> But what your observation reveals is that it is possible to get PERST# assert
> during the EP boot up itself which I was not accounting for. I always assumed
> that the EP will receive PERST# deassert first. If that is not the case, then
> this patch needs to be dropped.

From what I saw when having debug prints from my old email to you:
https://lore.kernel.org/linux-pci/Zalu%2F%2FdNi5BhZlBU@x1-carbon/


## RC side:
# reboot

## EP side
[  845.606810] pci: PERST asserted by host!
[  852.483985] pci: PERST de-asserted by host!
[  852.503041] pci: PERST asserted by host!
[  852.522375] pci: link up! (LTSSM_STATUS: 0x230011)
[  852.610318] pci: PERST de-asserted by host!


So in my case, I assume that the RC asserts PERST during a SoC reset.

This is obviously from the RC driver asserting PERST + sleep 100 ms +
PERST deassert:
[  852.503041] pci: PERST asserted by host!
[  852.610318] pci: PERST de-asserted by host!

The two before that:
[  852.483985] pci: PERST de-asserted by host!
[  852.503041] pci: PERST asserted by host!

appears to be because the RC I am using, incorrectly sets the PERST gpio as
ACTIVE HIGH:
https://github.com/torvalds/linux/blob/v6.9-rc1/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L300

Well, at least they are bug compatible and sets the output to:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L170-L184

0 and the 1, which, since the DT binding is incorrect, will actually
do the right thing and assert and the deassert PERST.

The problem seems to be that the initial flags:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L242-L243
is: GPIOD_OUT_HIGH

which explains why I get the extra:
[  852.483985] pci: PERST de-asserted by host!
before
[  852.503041] pci: PERST asserted by host!

with basically no time between them..


I guess I should send a patch to set the initial value to
GPIOD_OUT_LOW, so that the RC driver does not trigger a
"spurious" PERST deassertion when requesting the IRQ.


So I think this patch should be fine if the RC is not buggy,
but as we can see, in reality there are at least one platform
in mainline that does manage to get this wrong.


Kind regards,
Niklas