Re: [PATCH 1/4] PCI: qcom: Reshuffle reset logic in 2_7_0 .init

From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Johan Hovold <johan@kernel.org>
Cc: "Manivannan Sadhasivam" <mani@kernel.org>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Stanimir Varbanov" <svarbanov@mm-sol.com>,
	"Andrew Murray" <amurray@thegoodpenguin.co.uk>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Marijn Suijten" <marijn.suijten@somainline.org>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] PCI: qcom: Reshuffle reset logic in 2_7_0 .init
Date: Fri, 29 Dec 2023 16:01:27 +0100	[thread overview]
Message-ID: <fa0fbadc-a7c3-4bea-bed7-0006db0616dc@linaro.org> (raw)
In-Reply-To: <ZY7R581pgn3uO6kk@hovoldconsulting.com>

On 29.12.2023 15:04, Johan Hovold wrote:
> On Wed, Dec 27, 2023 at 11:17:19PM +0100, Konrad Dybcio wrote:
>> At least on SC8280XP, if the PCIe reset is asserted, the corresponding
>> AUX_CLK will be stuck at 'off'.
> 
> No, this path is exercised on every boot without the aux clock ever
> being stuck at off. So something is clearly missing in this description.
That's likely because the hardware has been initialized and not cleanly
shut down by your bootloader. When you reset it, or your bootloader
wasn't so kind, you need to start initialization from scratch.

>> Assert the reset (which may end up being a NOP if it was previously
>> asserted) and de-assert it back *before* turning on the clocks to avoid
>> such cases.
>>
>> In addition to that, in case the clock bulk enable fails, assert the
>> RC reset back, as the hardware is in an unknown state at best.
> 
> This is arguably a separate change, and not necessarily one that is
> correct either
If the clock enable fails, the PCIe hw is not in reset state, ergo it
may be doing "something", and that "something" would eat non-zero power.
It's just cleaning up after yourself.

> so should at least go in a separate patch if it should
> be done at all.
I'll grumpily comply..

>> Fixes: ed8cc3b1fc84 ("PCI: qcom: Add support for SDM845 PCIe controller")
> 
> I think you're being way to liberal with your use of Fixes tags. To
> claim that this is a bug, you need to make a more convincing case for
> why you think so.
The first paragraph describes the issue that this patch fixes.

> Also note Qualcomm's vendor driver is similarly asserting reset after
> enabling the clocks.
It's also not asserting the reset on suspend, see below.

> That driver does not seem to reset the controller on resume, though, in
> case that is relevant for your current experiments.
I know, the vendor driver doesn't fully shut down the controller. This
is however the only sequence that we (partially) have upstream, and the
only one that is going to work on SC8280XP (due to hw design).

On other platforms, a "soft shutdown" (i.e. dropping the link, cutting
clocks but not fully resetting the RC state) should be possible, but
that's not what this patchset concerns.

Konrad