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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 02, 2024 at 06:03:36PM +0100, Konrad Dybcio wrote:
> On 2.01.2024 11:17, Johan Hovold wrote:
> > On Sat, Dec 30, 2023 at 02:16:18AM +0100, Konrad Dybcio wrote:
> >> On 29.12.2023 16:29, Johan Hovold wrote:
> >>> 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.

> > We're clearly talking past each other. When I'm saying reset is asserted
> > on each boot, I'm referring to reset being asserted in
> > qcom_pcie_init_2_7_0(), whereas you appear to be referring to whether
> > the reset has been left asserted by the bootloader when the driver
> > probes.
> 
> OK, "boot" meant "booting the device" to me, not the PCIe controller.

Still not getting across to you apparently.

Again, the code in question is exercised on every boot and not once have
I seen a stuck clock due to reset being asserted *in*
qcom_pcie_init_2_7_0().

Now that's not what you were trying to describe as you were thinking of
reset having been left asserted *before* the driver probes (or before
qcom_pcie_init_2_7_0() is called).

See? Do you understand now what I was trying to say and why my
misinterpretation of your terse commit message lead me to claim that it
was clearly false?

> > I understand what you meant to say now, but I think you should rephrase:
> > 
> > 	At least on SC8280XP, if the PCIe reset is asserted, the
> > 	corresponding AUX_CLK will be stuck at 'off'.
> > 
> > because as it stands, it sounds like the problem happens when the driver
> > asserts reset.
> 
> Does this sound good?
> 
> "At least on SC8280XP, trying to enable the AUX_CLK associated with
> a PCIe host fails if the corresponding PCIe reset is asserted."

Yes, but you need to also say something about how this would happen, for
example, your hypothetical bootloader leaving it asserted and your actual
motivation for this change which is that it appears to be needed after
suspend. 

A commit message should be clear and self-contained and not force
reviewers to have to try to interpret what it means and guess what the
motivation for the change really is.

Johan




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux