Received-SPF: pass (google.com: domain of linux-kernel+bounces-13140-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 29 Dec 2023 16:29:55 +0100
From: Johan Hovold <johan@kernel.org>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	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>,
	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
Message-ID: <ZY7l828-mSGXVwrk@hovoldconsulting.com>
References: <20231227-topic-8280_pcie-v1-0-095491baf9e4@linaro.org>
 <20231227-topic-8280_pcie-v1-1-095491baf9e4@linaro.org>
 <ZY7R581pgn3uO6kk@hovoldconsulting.com>
 <fa0fbadc-a7c3-4bea-bed7-0006db0616dc@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fa0fbadc-a7c3-4bea-bed7-0006db0616dc@linaro.org>

[ Again, please remember to add a newline before you inline comments to
make you replies readable. ]

On Fri, Dec 29, 2023 at 04:01:27PM +0100, Konrad Dybcio wrote:
> 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.

What does that even mean? I'm telling you that this reset is asserted on
each boot, on all sc8280xp platforms I have access to, and never have I
seen the aux clk stuck at off.

So clearly your claim above is too broad and the commit message is
incorrect or incomplete.
 
> >> 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.

How can it do something without power and clocks? And leaving reset
asserted for non-powered devices is generally not a good idea.
 
> > so should at least go in a separate patch if it should
> > be done at all.

> I'll grumpily comply..

I suggest you leave it deasserted unless you have documentation
suggesting that the opposite is safe and recommended for this piece of
hardware.
 
> >> 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.

Yes, but this is all very hand-wavy so far. With a complete commit
message I may agree, but you still haven't convinced me that this is a
bug and not just a workaround from some not fully-understood issue on
one particular platform.
 
> > 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.

Right, as I mentioned.
 
> > 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.

The commit message does not even mention suspend, it just makes a
clearly false general claim about a clock being stuck unless you reorder
things.

Johan