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 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: Tue, 2 Jan 2024 11:17:38 +0100 [thread overview]
Message-ID: <ZZPiwk1pbhLyfthB@hovoldconsulting.com> (raw)
In-Reply-To: <598ede70-bc01-4137-b68b-981c3d420735@linaro.org>
On Sat, Dec 30, 2023 at 02:16:18AM +0100, Konrad Dybcio wrote:
> On 29.12.2023 16:29, Johan Hovold wrote:
> > [ 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.
>
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index 0b7801971dc1..6650bd6af5e3 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -7566,6 +7566,18 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
> if (ret)
> goto err_put_rpm;
>
> + int val;
> + regmap_read(regmap, 0xa0000, &val);
> + pr_err("GCC_PCIE_3A_BCR = 0x%x\n", val);
> + regmap_read(regmap, 0xa00f0, &val);
> + pr_err("GCC_PCIE_3A_LINK_DOWN_BCR = 0x%x\n", val);
> + regmap_read(regmap, 0xa00fc, &val);
> + pr_err("GCC_PCIE_3A_NOCSR_COM_PHY_BCR = 0x%x\n", val);
> + regmap_read(regmap, 0xa00e0, &val);
> + pr_err("GCC_PCIE_3A_PHY_BCR = 0x%x\n", val);
> + regmap_read(regmap, 0xa00e4, &val);
> + pr_err("GCC_PCIE_3A_PHY_NOCSR_COM_PHY_BCR = 0x%x\n", val);
> +
> pm_runtime_put(&pdev->dev);
>
> return 0;
>
>
> [root@sc8280xp-crd ~]# dmesg | grep BCR
> [ 2.500245] GCC_PCIE_3A_BCR = 0x0
> [ 2.500250] GCC_PCIE_3A_LINK_DOWN_BCR = 0x0
> [ 2.500253] GCC_PCIE_3A_NOCSR_COM_PHY_BCR = 0x0
> [ 2.500255] GCC_PCIE_3A_PHY_BCR = 0x0
> [ 2.500257] GCC_PCIE_3A_PHY_NOCSR_COM_PHY_BCR = 0x0
>
>
> 0 meaning "not asserted".
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.
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.
> PCIE3A is used for WLAN on the CRD, btw.
You meant to say WWAN (modem).
> >>>> 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?
>
> Fair point.
>
> As far as power goes, the RC hangs off CX, which is on whenever the
> system is not in power collapse. As for clocks, at least parts of it
> use the crystal oscillator, not sure if directly.
>
> > And leaving reset
> > asserted for non-powered devices is generally not a good idea.
>
> Depends on the hw.
That's why I said "generally".
> > 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.
>
> No, I insist that this general statement, while indeed lacking a full
> description of the problem, is provably true. The AUX clock will not
> turn on if the PCIe reset is asserted, at least on SC8280XP.
I agree, I was misinterpreting the commit message.
Johan
next prev parent reply other threads:[~2024-01-02 10:17 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-27 22:17 [PATCH 0/4] Qualcomm PCIe RC shutdown & reinit Konrad Dybcio
2023-12-27 22:17 ` [PATCH 1/4] PCI: qcom: Reshuffle reset logic in 2_7_0 .init Konrad Dybcio
2023-12-29 14:04 ` Johan Hovold
2023-12-29 15:01 ` Konrad Dybcio
2023-12-29 15:29 ` Johan Hovold
2023-12-30 1:16 ` Konrad Dybcio
2024-01-01 17:31 ` Manivannan Sadhasivam
2024-01-02 10:17 ` Johan Hovold [this message]
2024-01-02 17:03 ` Konrad Dybcio
2024-01-03 10:40 ` Johan Hovold
2024-01-03 12:11 ` Konrad Dybcio
2023-12-29 15:46 ` Bjorn Helgaas
2023-12-30 22:11 ` Konrad Dybcio
2023-12-27 22:17 ` [PATCH 2/4] PCI: qcom: Cache last icc bandwidth Konrad Dybcio
2023-12-27 22:17 ` [PATCH 3/4] PCI: qcom: Read back PARF_LTSSM register Konrad Dybcio
2024-01-02 17:05 ` Manivannan Sadhasivam
2023-12-27 22:17 ` [PATCH 4/4] PCI: qcom: Implement RC shutdown/power up Konrad Dybcio
2023-12-28 15:12 ` kernel test robot
2023-12-29 3:42 ` kernel test robot
2024-01-02 17:07 ` Manivannan Sadhasivam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZZPiwk1pbhLyfthB@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=amurray@thegoodpenguin.co.uk \
--cc=andersson@kernel.org \
--cc=bhelgaas@google.com \
--cc=konrad.dybcio@linaro.org \
--cc=kw@linux.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=mani@kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=p.zabel@pengutronix.de \
--cc=robh@kernel.org \
--cc=svarbanov@mm-sol.com \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).