From: Manivannan Sadhasivam <mani@kernel.org>
To: Niklas Cassel <cassel@kernel.org>
Cc: "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
"Jingoo Han" <jingoohan1@gmail.com>,
"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Rob Herring" <robh@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Marek Vasut" <marek.vasut+renesas@gmail.com>,
"Yoshihiro Shimoda" <yoshihiro.shimoda.uh@renesas.com>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Jonathan Hunter" <jonathanh@nvidia.com>,
"Kishon Vijay Abraham I" <kishon@ti.com>,
"Vidya Sagar" <vidyas@nvidia.com>,
"Vignesh Raghavendra" <vigneshr@ti.com>,
"Richard Zhu" <hongxing.zhu@nxp.com>,
"Lucas Stach" <l.stach@pengutronix.de>,
"Shawn Guo" <shawnguo@kernel.org>,
"Sascha Hauer" <s.hauer@pengutronix.de>,
"Pengutronix Kernel Team" <kernel@pengutronix.de>,
"Fabio Estevam" <festevam@gmail.com>,
"NXP Linux Team" <linux-imx@nxp.com>,
"Minghuan Lian" <minghuan.Lian@nxp.com>,
"Mingkai Hu" <mingkai.hu@nxp.com>, "Roy Zang" <roy.zang@nxp.com>,
"Kunihiko Hayashi" <hayashi.kunihiko@socionext.com>,
"Masami Hiramatsu" <mhiramat@kernel.org>,
"Kishon Vijay Abraham I" <kishon@kernel.org>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-renesas-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-tegra@vger.kernel.org, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v8 03/10] PCI: dwc: ep: Introduce dw_pcie_ep_cleanup() API for drivers supporting PERST#
Date: Mon, 4 Mar 2024 20:34:17 +0530 [thread overview]
Message-ID: <20240304150417.GK2647@thinkpad> (raw)
In-Reply-To: <ZeWnmLjS0O8CYQYg@fedora>
On Mon, Mar 04, 2024 at 11:51:04AM +0100, Niklas Cassel wrote:
> On Mon, Mar 04, 2024 at 01:47:13PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Feb 29, 2024 at 01:40:29PM +0100, Niklas Cassel wrote:
> > > On Sat, Feb 24, 2024 at 12:24:09PM +0530, Manivannan Sadhasivam wrote:
> > >
> > > Since e.g. qcom-ep.c does a reset_control_assert() during perst
> > > assert/deassert, which should clear sticky registers, I think that
> > > you should let dw_pcie_ep_cleanup() clean up the BARs using
> > > dw_pcie_ep_clear_bar().
> > >
> >
> > As I mentioned earlier, it is the job of the EPF drivers to clear the BARs since
> > they allocate them. I'm trying to reduce the implicit resetting wherever we
> > could.
> >
> > The proper fix is to add the LINK_DOWN callback to EPF drivers and do cleanup.
> > I'm planning to submit a series for that after this one.
>
> Currently, pci-epf-test allocates memory for the BARs in .bind().
> Likewise it frees the memory for the BARs in .unbind().
>
> AFAICT, most iATU registers, and most BAR registers are sticky registers,
> so they will not get reset on link down.
> (The currently selected BAR size, in case of Resizable BAR is an exception.)
>
> That means that even on link down, we do not need to free the memory,
> or change the iATU settings. (This applies to all drivers.)
>
>
>
> However, on PERST (for the drivers call dw_pcie_ep_cleanup()), they call
> reset_control_assert(), so they will clear sticky registers, which means
> that they need to at least re-write the iATU and BAR registers.
> (I guess they could free + allocate the memory for the BARs again,
> but I don't think that is strictly necessary.)
> That is why I suggested that you call dw_pcie_ep_clear_bar() from
> dw_pcie_ep_cleanup().
>
Sorry, I keep assuming the flow w.r.t PERST# supported platforms :/
My bad!
>
>
> If you free the memory for the BARs in link_down() (this callback exists
> for many drivers, even drivers without a PERST handler), where are you
> supposted to alloc the memory for the BARs again?
>
> Allocating them at link_up() is too late (because as soon as the link is
> up, the host is allowed to enumerate the EP BARs.) The proper place is to
> allocate them when receiving PERST, but not all drivers have a PERST handler.
>
> (My understanding is that 1) PERST assert 2) PERST deassert 3) link is up.)
>
>
>
> unbind() undos what was done in bind(), so shouldn't link_down() undo what was
> done in link_up()? With that logic, if you move the alloc to .core_init(),
> should we perhaps have a .core_deinit() callback for EPF drivers?
> (I guess only drivers which perform a reset during PERST would call this.)
>
> But considering that free+alloc is not strictly needed, why not just keep
> the allocation + free in .bind()/.unbind() ?
> (To avoid the need to create a .core_deinit()), and let dw_pcie_ep_cleanup()
> call dw_pcie_ep_clear_bar() ?
>
> I guess my point is that it seems a bit pointless for drivers that do not
> clear sticky registers to free+alloc memory on link down, for no good
> reason. (Memory might get fragmented over time, so it might not be possible
> to perform a big allocation after the device has been running for a really
> long time.)
>
>
>
> So I'm thinking that we either
> 1) Keep the alloc/free in bind/unbind, and let dw_pcie_ep_cleanup() call
> dw_pcie_ep_clear_bar(),
> or
> 2) Introduce a .deinit_core() callback which will free the BARs.
> (Because I don't see how you will (re-)allocate memory for all drivers
> if you free the memory in link_down().)
>
I think option 2 is the better solution. In my view, calling
dw_pcie_ep_clear_bar() from EPC drivers is a layering violation since the
allocation happens from EPF drivers.
So clearing the BARs during the deinit() callback that gets called when PERST#
assert happens is the way to go.
- Mani
--
மணிவண்ணன் சதாசிவம்
next prev parent reply other threads:[~2024-03-04 15:04 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-24 6:54 [PATCH v8 00/10] PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host Manivannan Sadhasivam
2024-02-24 6:54 ` [PATCH v8 01/10] PCI: dwc: ep: Remove deinit() callback from struct dw_pcie_ep_ops Manivannan Sadhasivam
2024-02-26 16:49 ` Frank Li
2024-02-24 6:54 ` [PATCH v8 02/10] PCI: dwc: ep: Rename dw_pcie_ep_exit() to dw_pcie_ep_deinit() Manivannan Sadhasivam
2024-02-26 16:51 ` Frank Li
2024-02-24 6:54 ` [PATCH v8 03/10] PCI: dwc: ep: Introduce dw_pcie_ep_cleanup() API for drivers supporting PERST# Manivannan Sadhasivam
2024-02-26 16:53 ` Frank Li
2024-02-29 12:40 ` Niklas Cassel
2024-03-04 8:17 ` Manivannan Sadhasivam
2024-03-04 10:51 ` Niklas Cassel
2024-03-04 15:04 ` Manivannan Sadhasivam [this message]
2024-02-24 6:54 ` [PATCH v8 04/10] PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host Manivannan Sadhasivam
2024-02-26 16:57 ` Frank Li
2024-02-24 6:54 ` [PATCH v8 05/10] PCI: dwc: ep: Rename dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers() Manivannan Sadhasivam
2024-02-26 16:58 ` Frank Li
2024-02-24 6:54 ` [PATCH v8 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers Manivannan Sadhasivam
2024-02-26 17:04 ` Frank Li
2024-02-27 12:21 ` Manivannan Sadhasivam
2024-02-27 17:28 ` Frank Li
2024-02-27 18:42 ` Manivannan Sadhasivam
2024-02-24 6:54 ` [PATCH v8 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag Manivannan Sadhasivam
2024-02-26 17:09 ` Frank Li
2024-02-29 11:23 ` Niklas Cassel
2024-03-04 6:26 ` Manivannan Sadhasivam
2024-02-24 6:54 ` [PATCH v8 08/10] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event Manivannan Sadhasivam
2024-02-26 17:18 ` Frank Li
2024-02-27 12:30 ` Manivannan Sadhasivam
2024-02-27 17:26 ` Frank Li
2024-02-27 18:50 ` Manivannan Sadhasivam
2024-02-24 6:54 ` [PATCH v8 09/10] PCI: qcom-ep: Use the " Manivannan Sadhasivam
2024-02-26 17:20 ` Frank Li
2024-02-27 12:32 ` Manivannan Sadhasivam
2024-02-27 17:34 ` Frank Li
2024-02-27 18:47 ` Manivannan Sadhasivam
2024-02-24 6:54 ` [PATCH v8 10/10] PCI: dwc: ep: Add Kernel-doc comments for APIs Manivannan Sadhasivam
2024-02-26 17:21 ` Frank Li
2024-02-29 12:56 ` Niklas Cassel
2024-03-04 8:18 ` 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=20240304150417.GK2647@thinkpad \
--to=mani@kernel.org \
--cc=bhelgaas@google.com \
--cc=cassel@kernel.org \
--cc=festevam@gmail.com \
--cc=gustavo.pimentel@synopsys.com \
--cc=hayashi.kunihiko@socionext.com \
--cc=hongxing.zhu@nxp.com \
--cc=jingoohan1@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=kernel@pengutronix.de \
--cc=kishon@kernel.org \
--cc=kishon@ti.com \
--cc=kw@linux.com \
--cc=l.stach@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lpieralisi@kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=marek.vasut+renesas@gmail.com \
--cc=mhiramat@kernel.org \
--cc=minghuan.Lian@nxp.com \
--cc=mingkai.hu@nxp.com \
--cc=robh@kernel.org \
--cc=roy.zang@nxp.com \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=vidyas@nvidia.com \
--cc=vigneshr@ti.com \
--cc=yoshihiro.shimoda.uh@renesas.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).