Re: [PATCH V4] Revert "PCI: tegra194: Enable support for 256 Byte payload"

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

 





On 8/2/2023 2:10 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments


On Tue, Jul 18, 2023 at 08:22:21AM +0530, Vidya Sagar wrote:
After commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte
payload"), we set MPS=256 for tegra194 Root Ports.

By default (CONFIG_PCIE_BUS_DEFAULT set and no "pci=pcie_bus_*"
parameter), Linux configures the MPS of every device to match the
upstream bridge, which is impossible if the Root Port has MPS=256
and a device only supports MPS=128.

Thanks for pointing out that I broke this log by omitting the mention
of a switch.  Is the rewording below better?  If so, Krzysztof can
amend the commit.
Yes. The below rewording looks good.

Thanks,
Vidya Sagar


   After commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte
   payload"), we initialize MPS=256 for tegra194 Root Ports before enumerating
   the hierarchy.

   Consider an Endpoint that supports only MPS=128.  In the default situation
   (CONFIG_PCIE_BUS_DEFAULT set and no "pci=pcie_bus_*" parameter), Linux
   tries to configure the MPS of every device to match the upstream bridge.
   If the Endpoint is directly below the Root Port, Linux can reduce the Root
   Port MPS to 128 to match the Endpoint.  But if there's a switch in the
   middle, Linux doesn't reduce the Root Port MPS because other devices below
   the switch may already be configured with MPS larger than 128.

This scenario results in uncorrectable Malformed TLP errors if the
Root Port sends TLPs with payloads larger than 128 bytes.  These
errors can be avoided by using the "pci=pcie_bus_safe" parameter,
but it doesn't seem to be a good idea to always have this parameter
even for basic functionality to work.

Revert commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte
payload") so the Root Ports default to MPS=128, which all devices
support.

If peer-to-peer DMA is not required, one can use "pci=pcie_bus_perf"
to get the benefit of larger MPS settings.

[ rewrote commit message based on Bjorn's suggestion ]

Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload")

4fb8e46c1bc4 appeared in v6.0-rc1, so this wouldn't be a candidate for
v6.5, but it does sound like it should be tagged for stable?  If so,
Krzysztof can probably add that as well.

Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
---
V4:
* Rewrote commit message based on Bjorn's suggestion

V3:
* Fixed a build issue

V2:
* Addressed review comments from Bjorn

  drivers/pci/controller/dwc/pcie-tegra194.c | 14 ++------------
  1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 4fdadc7b045f..a772faff14b5 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -900,11 +900,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
               pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
                                                             PCI_CAP_ID_EXP);

-     val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL);
-     val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD;
-     val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B;
-     dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16);
-
       val = dw_pcie_readl_dbi(pci, PCI_IO_BASE);
       val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8);
       dw_pcie_writel_dbi(pci, PCI_IO_BASE, val);
@@ -1756,7 +1751,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
       struct device *dev = pcie->dev;
       u32 val;
       int ret;
-     u16 val_16;

       if (pcie->ep_state == EP_STATE_ENABLED)
               return;
@@ -1887,20 +1881,16 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
       pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
                                                     PCI_CAP_ID_EXP);

-     val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL);
-     val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD;
-     val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B;
-     dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16);
-
       /* Clear Slot Clock Configuration bit if SRNS configuration */
       if (pcie->enable_srns) {
+             u16 val_16;
+
               val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
                                          PCI_EXP_LNKSTA);
               val_16 &= ~PCI_EXP_LNKSTA_SLC;
               dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA,
                                  val_16);
       }
-
       clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);

       val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK);
--
2.25.1




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux