From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Niklas Cassel <cassel@kernel.org>
Cc: "Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Rob Herring" <robh@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Kishon Vijay Abraham I" <kishon@kernel.org>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Jonathan Hunter" <jonathanh@nvidia.com>,
"Jingoo Han" <jingoohan1@gmail.com>,
"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org, mhi@lists.linux.dev,
linux-tegra@vger.kernel.org
Subject: Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert
Date: Tue, 26 Mar 2024 16:40:21 +0530 [thread overview]
Message-ID: <20240326111021.GA13849@thinkpad> (raw)
In-Reply-To: <ZgKiUogkgrMwV1uD@x1-carbon>
On Tue, Mar 26, 2024 at 11:24:18AM +0100, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 01:14:29PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 22, 2024 at 05:08:22PM +0100, Niklas Cassel wrote:
> > > On Thu, Mar 14, 2024 at 08:53:40PM +0530, Manivannan Sadhasivam wrote:
> > > > All EP specific resources are enabled during PERST# deassert. As a counter
> > > > operation, all resources should be disabled during PERST# assert. There is
> > > > no point in skipping that if the link was not enabled.
> > > >
> > > > This will also result in enablement of the resources twice if PERST# got
> > > > deasserted again. So remove the check from qcom_pcie_perst_assert() and
> > > > disable all the resources unconditionally.
> > > >
> > > > Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
> > > > 1 file changed, 6 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > index 2fb8c15e7a91..50b1635e3cbb 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > @@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> > > > static void qcom_pcie_perst_assert(struct dw_pcie *pci)
> > > > {
> > > > struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > > > - struct device *dev = pci->dev;
> > > > -
> > > > - if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> > > > - dev_dbg(dev, "Link is already disabled\n");
> > > > - return;
> > > > - }
> > > >
> > > > dw_pcie_ep_cleanup(&pci->ep);
> > > > qcom_pcie_disable_resources(pcie_ep);
> > >
> > > Are you really sure that this is safe?
> > >
> > > I think I remember seeing some splat in dmesg if some clks, or maybe it
> > > was regulators, got disabled while already being disabled.
> > >
> > > Perhaps you could test it by simply calling:
> > > qcom_pcie_disable_resources();
> > > twice here, and see if you see and splat in dmesg.
> > >
> >
> > Calling the disable_resources() function twice will definitely result in the
> > splat. But here PERST# is level triggered, so I don't see how the EP can see
> > assert twice.
> >
> > Am I missing something?
>
> I think I remember now, I was developing a driver using a .core_init_notifier,
> but I followed the pcie-tegra model, which does not enable any resources in
> probe() (it only gets them), so I got the splat because when PERST got
> asserted, resources would get disabled even though they were already disabled.
>
> pcie-qcom:
> -gets resources in .probe()
> -enables resources in .probe()
> -sets no default state in .probe()
>
> pcie-tegra:
> -gets resources in .probe()
> -enables resources in perst_deassert()
> -sets default state to EP_STATE_DISABLED in probe()
>
> So pcie-qcom does not seem to be following the same pattern like pcie-tegra,
> because pcie-qcom actually does enable resources for the first time in
> probe(), while tegra will enable resources for the first time in
> perst_deassert().
>
> Sorry for the noise.
>
I was planning to drop enable_resources() from Qcom driver once the DBI rework
series gets merged. Because, the resource enablement during probe is currently
done to avoid the crash that is bound to happen if registers are accessed during
probe.
But what your observation reveals is that it is possible to get PERST# assert
during the EP boot up itself which I was not accounting for. I always assumed
that the EP will receive PERST# deassert first. If that is not the case, then
this patch needs to be dropped.
- Mani
--
மணிவண்ணன் சதாசிவம்
next prev parent reply other threads:[~2024-03-26 11:10 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 15:23 [PATCH 00/11] PCI: endpoint: Make host reboot handling more robust Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert Manivannan Sadhasivam
2024-03-22 16:08 ` Niklas Cassel
2024-03-26 7:44 ` Manivannan Sadhasivam
2024-03-26 10:24 ` Niklas Cassel
2024-03-26 11:10 ` Manivannan Sadhasivam [this message]
2024-03-26 13:47 ` Niklas Cassel
2024-03-26 13:55 ` Manivannan Sadhasivam
2024-03-26 10:37 ` Niklas Cassel
2024-03-14 15:23 ` [PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events Manivannan Sadhasivam
2024-03-22 16:08 ` Niklas Cassel
2024-03-26 7:49 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 03/11] PCI: endpoint: Rename core_init() callback in 'struct pci_epc_event_ops' to init() Manivannan Sadhasivam
2024-03-22 16:08 ` Niklas Cassel
2024-03-26 7:56 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 04/11] PCI: epf-test: Refactor pci_epf_test_unbind() function Manivannan Sadhasivam
2024-03-22 16:09 ` Niklas Cassel
2024-03-26 7:58 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-26 8:26 ` Manivannan Sadhasivam
2024-03-26 11:05 ` Niklas Cassel
2024-03-26 14:27 ` Niklas Cassel
2024-03-27 6:20 ` Manivannan Sadhasivam
2024-03-27 6:18 ` Manivannan Sadhasivam
2024-03-27 11:39 ` Niklas Cassel
2024-03-28 18:42 ` Vinod Koul
2024-04-04 8:44 ` Niklas Cassel
2024-04-22 7:55 ` Manivannan Sadhasivam
2024-04-22 9:30 ` Niklas Cassel
2024-03-14 15:23 ` [PATCH 06/11] PCI: endpoint: Introduce EPC 'deinit' event and notify the EPF drivers Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-26 8:31 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 07/11] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle Link Down event Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-27 18:06 ` Niklas Cassel
2024-04-01 16:34 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 08/11] PCI: qcom-ep: Use the " Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 09/11] PCI: epf-test: Handle " Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-14 15:23 ` [PATCH 10/11] PCI: qcom-ep: Rework {start/stop}_link() callbacks implementation Manivannan Sadhasivam
2024-03-22 16:10 ` Niklas Cassel
2024-03-26 8:33 ` Manivannan Sadhasivam
2024-03-14 15:23 ` [PATCH 11/11] PCI: tegra194: " Manivannan Sadhasivam
2024-03-22 16:11 ` Niklas Cassel
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=20240326111021.GA13849@thinkpad \
--to=manivannan.sadhasivam@linaro.org \
--cc=bhelgaas@google.com \
--cc=cassel@kernel.org \
--cc=gustavo.pimentel@synopsys.com \
--cc=jingoohan1@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=kishon@kernel.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=linux-tegra@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=mhi@lists.linux.dev \
--cc=robh@kernel.org \
--cc=thierry.reding@gmail.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).