On 7/14/2023 3:09 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Mon, Jun 19, 2023 at 03:56:04PM +0530, Vidya Sagar wrote:
This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable
support for 256 Byte payload").
Consider a PCIe hierarchy with a PCIe switch and a device connected
downstream of the switch that has support for MPS which is the minimum in
the hierarchy, and root port programmed with an MPS in its DevCtl register
that is greater than the minimum. In this scenario, the default bus
configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't configure the
MPS settings in the hierarchy correctly resulting in the device with
support for minimum MPS in the hierarchy receiving the TLPs of size more
than that. Although this can be addressed by appending "pci=pcie_bus_safe"
to the kernel command line, it doesn't seem to be a good idea to always
have this commandline argument even for the basic functionality to work.
I think this has some irrelevant detail (IIUC the problem should
happen even without a switch) and could be more specific (I think the
problem case is RP MPS=256, EP only supports MPS=128).
The issue is present only if there is a switch.
Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256
Byte payload") avoids this requirement and ensures that the basic
functionality of the devices irrespective of the hierarchy and the MPS of
the devices in the hierarchy.
"Ensure" is a transitive verb, so "... ensures that the basic
functionality ..." is missing whatever the object should be.
I think I missed to add 'work' in the end after 'hierarchy'. My bad.
Maybe something like the following?
After 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.
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 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.
Thanks, I'll send a new patch with the above commit message.
To reap the benefits of having support for higher MPS, optionally, one can
always append the kernel command line with "pci=pcie_bus_perf".
Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload")
Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
---
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
