* Re: [PATCH V3] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30
2021-09-01 20:40 ` [PATCH V3] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30 Bjorn Helgaas
@ 2021-09-02 14:07 ` Bjorn Helgaas
0 siblings, 0 replies; 2+ messages in thread
From: Bjorn Helgaas @ 2021-09-02 14:07 UTC (permalink / raw)
To: Vidya Sagar
Cc: bhelgaas, lorenzo.pieralisi, treding, swarren, jonathanh,
linux-tegra, linux-pci, kthota, mmaddireddy, sagar.tv,
Keith Busch
[+cc Keith]
On Wed, Sep 01, 2021 at 03:40:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Jul 04, 2019 at 08:34:28PM +0530, Vidya Sagar wrote:
> > Currently Relaxed Ordering bit in the configuration space is enabled for
> > all PCIe devices as the quirk uses PCI_ANY_ID for both Vendor-ID and
> > Device-ID, but, as per the Technical Reference Manual of Tegra20 which is
> > available at https://developer.nvidia.com/embedded/downloads#?search=tegra%202
> > in Sec 34.1, it is mentioned that Relaxed Ordering bit needs to be enabled in
> > its root ports to avoid deadlock in hardware. The same is applicable for
> > Tegra30 as well though it is not explicitly mentioned in Tegra30 TRM document,
> > but the same must not be extended to root ports of other Tegra SoCs or
> > other hosts as the same issue doesn't exist there.
>
> While researching another thread about RO [1], I got concerned about
> setting RO for root ports.
>
> Setting RO for *endpoints* makes sense: that allows (but does not
> require) the endpoint to issue writes that don't require strong
> ordering.
>
> Setting RO for *root ports* seems more problematic. It allows the
> root port to issue PCIe writes that don't require strong ordering.
> These would be CPU MMIO writes to devices. But Linux currently does
> not have a way for drivers to indicate that some MMIO writes need to
> be ordered while others do not, and I think drivers assume that all
> MMIO writes are performed in order.
>
> We merged this patch as 7be142caabc4 ("PCI: tegra: Enable Relaxed
> Ordering only for Tegra20 & Tegra30") [2], so Tegra20 and Tegra30 root
> ports are allowed (but again, not required) to set the RO bit for MMIO
> writes initiated by a CPU.
>
> Because I think drivers *rely* on MMIO writes being strongly ordered,
> this is a potential problem. I think we should probably consider
> explicitly *disabling* RO in all root ports (with exceptions for
> quirks like this) in case it's set by any firmware.
>
> So the question is, how do Tegra20 and Tegra30 actually work? Do they
> ever actually set the RO bit for these MMIO writes? If so, I think
> drivers are really at risk, and we probably should log some kind of
> warning.
Keith reminded me that writel_relaxed() is a way for drivers to
specify that they don't need strong ordering. So I think the question
really is whether Tegra20 and Tegra30 set the RO bit for normal
writel() writes. If they set RO for writel_relaxed() writes, that
should be perfectly fine.
> But if Tegra20 and Tegra30 just need "Enable Relaxed Ordering" set as
> a bug workaround and they never actually initiate PCIe writes with the
> RO bit set, maybe we should add a comment to that effect, but there
> should be no actual problem.
>
> [1] https://lore.kernel.org/r/20210830123704.221494-2-verdre@v0yd.nl
> [2] https://git.kernel.org/linus/7be142caabc4
>
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> > V3:
> > * Modified commit message to make it more precise and explicit
> >
> > V2:
> > * Modified commit message to include reference to Tegra20 TRM document.
> >
> > drivers/pci/controller/pci-tegra.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> > index 9cc03a2549c0..241760aa15bd 100644
> > --- a/drivers/pci/controller/pci-tegra.c
> > +++ b/drivers/pci/controller/pci-tegra.c
> > @@ -787,12 +787,15 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
> > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
> > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);
> >
> > -/* Tegra PCIE requires relaxed ordering */
> > +/* Tegra20 and Tegra30 PCIE requires relaxed ordering */
> > static void tegra_pcie_relax_enable(struct pci_dev *dev)
> > {
> > pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
> > }
> > -DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf0, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_relax_enable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_relax_enable);
> >
> > static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
> > {
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 2+ messages in thread