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

[Johan Hovold <johan@kernel.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