[ 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