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 addresed 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.
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.
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 <[email protected]>
---
drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 4fdadc7b045f..877d81b13334 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
u32 val;
- u16 val_16;
pp->bridge->ops = &tegra_pci_ops;
@@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
@@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
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
On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
> 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.
> To reap the benefits of having support for higher MPS, optionally, one can
> always append the kernel command line with "pci=pcie_bus_perf".
Please add blank lines between paragraphs and wrap to fill 75 columns.
Also add a period at the end of the very first sentence.
s/addresed/addressed/
I guess that without 4fb8e46c1bc4, Linux configured everything with
128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to
allow 256 byte MPS.
If the Root Port advertises Max_Payload_Size Supported as 256 bytes in
DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd
argue that should be fixed in the PCI core without a driver change
like this.
Bjorn
On 6/8/2023 10:03 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
>> 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.
>> To reap the benefits of having support for higher MPS, optionally, one can
>> always append the kernel command line with "pci=pcie_bus_perf".
>
> Please add blank lines between paragraphs and wrap to fill 75 columns.
> Also add a period at the end of the very first sentence.
>
> s/addresed/addressed/
>
I'll address your comments in the next patch.
> I guess that without 4fb8e46c1bc4, Linux configured everything with
> 128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to
> allow 256 byte MPS.
Correct.
>
> If the Root Port advertises Max_Payload_Size Supported as 256 bytes in
> DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd
> argue that should be fixed in the PCI core without a driver change
> like this.
Well, kernel does configure MPS=256 but only if the 'perf' configuration
option is selected. 'perf' configuration option also changes the MRRS,
to extract the maximum performance. I'm not sure about the reasons for
not making 'perf' configuration as the default configuration though.
(IIUC, this is what you are coming to, right?)
The current patch which is a revert of an earlier patch is to keep
things working for different PCIe hierarchies given the default
configuration that kernel is using at the moment.
-Vidya Sagar
>
> Bjorn
Hi Bjorn,
On 09/06/2023 03:23, Vidya Sagar wrote:
>
>
> On 6/8/2023 10:03 PM, Bjorn Helgaas wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
>>> 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.
>>> To reap the benefits of having support for higher MPS, optionally,
>>> one can
>>> always append the kernel command line with "pci=pcie_bus_perf".
>>
>> Please add blank lines between paragraphs and wrap to fill 75 columns.
>> Also add a period at the end of the very first sentence.
>>
>> s/addresed/addressed/
>>
> I'll address your comments in the next patch.
>
>> I guess that without 4fb8e46c1bc4, Linux configured everything with
>> 128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to
>> allow 256 byte MPS.
> Correct.
>
>>
>> If the Root Port advertises Max_Payload_Size Supported as 256 bytes in
>> DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd
>> argue that should be fixed in the PCI core without a driver change
>> like this.
> Well, kernel does configure MPS=256 but only if the 'perf' configuration
> option is selected. 'perf' configuration option also changes the MRRS,
> to extract the maximum performance. I'm not sure about the reasons for
> not making 'perf' configuration as the default configuration though.
> (IIUC, this is what you are coming to, right?)
>
> The current patch which is a revert of an earlier patch is to keep
> things working for different PCIe hierarchies given the default
> configuration that kernel is using at the moment.
Any more feedback on this? If not, would be great to queue for v6.5.
Feel free to add my ...
Acked-by: Jon Hunter <[email protected]>
Cheers
Jon
--
nvpublic
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.
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.
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 <[email protected]>
---
V2:
* Addressed review comments from Bjorn
drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 4fdadc7b045f..877d81b13334 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
u32 val;
- u16 val_16;
pp->bridge->ops = &tegra_pci_ops;
@@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
@@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
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
Hi Vidya,
kernel test robot noticed the following build errors:
[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.4-rc7 next-20230616]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vidya-Sagar/Revert-PCI-tegra194-Enable-support-for-256-Byte-payload/20230619-094403
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20230619014218.1970846-1-vidyas%40nvidia.com
patch subject: [PATCH V2] Revert "PCI: tegra194: Enable support for 256 Byte payload"
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20230619/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230619/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/pci/controller/dwc/pcie-tegra194.c: In function 'tegra_pcie_dw_host_init':
>> drivers/pci/controller/dwc/pcie-tegra194.c:906:17: error: 'val_16' undeclared (first use in this function)
906 | val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
| ^~~~~~
drivers/pci/controller/dwc/pcie-tegra194.c:906:17: note: each undeclared identifier is reported only once for each function it appears in
drivers/pci/controller/dwc/pcie-tegra194.c: In function 'pex_ep_event_pex_rst_deassert':
drivers/pci/controller/dwc/pcie-tegra194.c:1865:17: error: 'val_16' undeclared (first use in this function)
1865 | val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
| ^~~~~~
vim +/val_16 +906 drivers/pci/controller/dwc/pcie-tegra194.c
56e15a238d9278 Vidya Sagar 2019-08-13 867
64451ac83fe6ab Bjorn Helgaas 2022-08-04 868 static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
56e15a238d9278 Vidya Sagar 2019-08-13 869 {
56e15a238d9278 Vidya Sagar 2019-08-13 870 struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
f1ab409d578752 Vidya Sagar 2022-07-21 871 struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
56e15a238d9278 Vidya Sagar 2019-08-13 872 u32 val;
56e15a238d9278 Vidya Sagar 2019-08-13 873
275e88b06a277c Rob Herring 2020-12-18 874 pp->bridge->ops = &tegra_pci_ops;
275e88b06a277c Rob Herring 2020-12-18 875
369b868f4a2ef8 Vidya Sagar 2020-11-26 876 if (!pcie->pcie_cap_base)
369b868f4a2ef8 Vidya Sagar 2020-11-26 877 pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
369b868f4a2ef8 Vidya Sagar 2020-11-26 878 PCI_CAP_ID_EXP);
369b868f4a2ef8 Vidya Sagar 2020-11-26 879
56e15a238d9278 Vidya Sagar 2019-08-13 880 val = dw_pcie_readl_dbi(pci, PCI_IO_BASE);
56e15a238d9278 Vidya Sagar 2019-08-13 881 val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8);
56e15a238d9278 Vidya Sagar 2019-08-13 882 dw_pcie_writel_dbi(pci, PCI_IO_BASE, val);
56e15a238d9278 Vidya Sagar 2019-08-13 883
56e15a238d9278 Vidya Sagar 2019-08-13 884 val = dw_pcie_readl_dbi(pci, PCI_PREF_MEMORY_BASE);
56e15a238d9278 Vidya Sagar 2019-08-13 885 val |= CFG_PREF_MEM_LIMIT_BASE_MEM_DECODE;
56e15a238d9278 Vidya Sagar 2019-08-13 886 val |= CFG_PREF_MEM_LIMIT_BASE_MEM_LIMIT_DECODE;
56e15a238d9278 Vidya Sagar 2019-08-13 887 dw_pcie_writel_dbi(pci, PCI_PREF_MEMORY_BASE, val);
56e15a238d9278 Vidya Sagar 2019-08-13 888
56e15a238d9278 Vidya Sagar 2019-08-13 889 dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
56e15a238d9278 Vidya Sagar 2019-08-13 890
56e15a238d9278 Vidya Sagar 2019-08-13 891 /* Enable as 0xFFFF0001 response for CRS */
56e15a238d9278 Vidya Sagar 2019-08-13 892 val = dw_pcie_readl_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT);
56e15a238d9278 Vidya Sagar 2019-08-13 893 val &= ~(AMBA_ERROR_RESPONSE_CRS_MASK << AMBA_ERROR_RESPONSE_CRS_SHIFT);
56e15a238d9278 Vidya Sagar 2019-08-13 894 val |= (AMBA_ERROR_RESPONSE_CRS_OKAY_FFFF0001 <<
56e15a238d9278 Vidya Sagar 2019-08-13 895 AMBA_ERROR_RESPONSE_CRS_SHIFT);
56e15a238d9278 Vidya Sagar 2019-08-13 896 dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
56e15a238d9278 Vidya Sagar 2019-08-13 897
56e15a238d9278 Vidya Sagar 2019-08-13 898 /* Configure Max lane width from DT */
56e15a238d9278 Vidya Sagar 2019-08-13 899 val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP);
56e15a238d9278 Vidya Sagar 2019-08-13 900 val &= ~PCI_EXP_LNKCAP_MLW;
56e15a238d9278 Vidya Sagar 2019-08-13 901 val |= (pcie->num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT);
56e15a238d9278 Vidya Sagar 2019-08-13 902 dw_pcie_writel_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP, val);
56e15a238d9278 Vidya Sagar 2019-08-13 903
a54e190737181c Vidya Sagar 2022-07-21 904 /* Clear Slot Clock Configuration bit if SRNS configuration */
a54e190737181c Vidya Sagar 2022-07-21 905 if (pcie->enable_srns) {
a54e190737181c Vidya Sagar 2022-07-21 @906 val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
a54e190737181c Vidya Sagar 2022-07-21 907 PCI_EXP_LNKSTA);
a54e190737181c Vidya Sagar 2022-07-21 908 val_16 &= ~PCI_EXP_LNKSTA_SLC;
a54e190737181c Vidya Sagar 2022-07-21 909 dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA,
a54e190737181c Vidya Sagar 2022-07-21 910 val_16);
a54e190737181c Vidya Sagar 2022-07-21 911 }
a54e190737181c Vidya Sagar 2022-07-21 912
56e15a238d9278 Vidya Sagar 2019-08-13 913 config_gen3_gen4_eq_presets(pcie);
56e15a238d9278 Vidya Sagar 2019-08-13 914
56e15a238d9278 Vidya Sagar 2019-08-13 915 init_host_aspm(pcie);
56e15a238d9278 Vidya Sagar 2019-08-13 916
6b6fafc1abc7c0 Vidya Sagar 2020-12-03 917 /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
6b6fafc1abc7c0 Vidya Sagar 2020-12-03 918 if (!pcie->supports_clkreq) {
6b6fafc1abc7c0 Vidya Sagar 2020-12-03 919 disable_aspm_l11(pcie);
6b6fafc1abc7c0 Vidya Sagar 2020-12-03 920 disable_aspm_l12(pcie);
6b6fafc1abc7c0 Vidya Sagar 2020-12-03 921 }
6b6fafc1abc7c0 Vidya Sagar 2020-12-03 922
a54e190737181c Vidya Sagar 2022-07-21 923 if (!pcie->of_data->has_l1ss_exit_fix) {
56e15a238d9278 Vidya Sagar 2019-08-13 924 val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
56e15a238d9278 Vidya Sagar 2019-08-13 925 val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
56e15a238d9278 Vidya Sagar 2019-08-13 926 dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
a54e190737181c Vidya Sagar 2022-07-21 927 }
56e15a238d9278 Vidya Sagar 2019-08-13 928
56e15a238d9278 Vidya Sagar 2019-08-13 929 if (pcie->update_fc_fixup) {
56e15a238d9278 Vidya Sagar 2019-08-13 930 val = dw_pcie_readl_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF);
56e15a238d9278 Vidya Sagar 2019-08-13 931 val |= 0x1 << CFG_TIMER_CTRL_ACK_NAK_SHIFT;
56e15a238d9278 Vidya Sagar 2019-08-13 932 dw_pcie_writel_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF, val);
56e15a238d9278 Vidya Sagar 2019-08-13 933 }
56e15a238d9278 Vidya Sagar 2019-08-13 934
56e15a238d9278 Vidya Sagar 2019-08-13 935 clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);
56e15a238d9278 Vidya Sagar 2019-08-13 936
275e88b06a277c Rob Herring 2020-12-18 937 return 0;
275e88b06a277c Rob Herring 2020-12-18 938 }
275e88b06a277c Rob Herring 2020-12-18 939
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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.
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.
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 <[email protected]>
---
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
Hello,
> 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.
>
> 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.
>
> To reap the benefits of having support for higher MPS, optionally, one can
> always append the kernel command line with "pci=pcie_bus_perf".
Applied to controller/tegra194, thank you!
[1/1] Revert "PCI: tegra194: Enable support for 256 Byte payload"
https://git.kernel.org/pci/pci/c/606295f16360
Krzysztof
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).
> 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.
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.
> 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 <[email protected]>
> ---
> 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
>
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.
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")
Signed-off-by: Vidya Sagar <[email protected]>
---
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
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 <[email protected]>
>> ---
>> 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
>>
On Tue, Jul 18, 2023 at 08:03:47AM +0530, Vidya Sagar wrote:
> On 7/14/2023 3:09 AM, Bjorn Helgaas wrote:
> > 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.
So if there's no switch, and an EP that only supports MPS=128, the PCI
core changes the RP MPS setting to 128? Just based on reading the
code, I thought we would leave RP MPS=256 and EP MPS=128, which would
be a problem. But maybe the PCI core changes the RP down to MPS=128?
Bjorn
On 7/18/2023 4:39 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, Jul 18, 2023 at 08:03:47AM +0530, Vidya Sagar wrote:
>> On 7/14/2023 3:09 AM, Bjorn Helgaas wrote:
>>> 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.
>
> So if there's no switch, and an EP that only supports MPS=128, the PCI
> core changes the RP MPS setting to 128? Just based on reading the
Yes. The code after the if condition here takes care of that.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/probe.c?h=v6.5-rc2#n2049
> code, I thought we would leave RP MPS=256 and EP MPS=128, which would
> be a problem. But maybe the PCI core changes the RP down to MPS=128?
>
> Bjorn
On Wed, Jul 19, 2023 at 04:31:13PM +0530, Vidya Sagar wrote:
> On 7/18/2023 4:39 PM, Bjorn Helgaas wrote:
> > On Tue, Jul 18, 2023 at 08:03:47AM +0530, Vidya Sagar wrote:
> > > On 7/14/2023 3:09 AM, Bjorn Helgaas wrote:
> > > > 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.
> >
> > So if there's no switch, and an EP that only supports MPS=128, the PCI
> > core changes the RP MPS setting to 128? Just based on reading the
>
> Yes. The code after the if condition here takes care of that.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/probe.c?h=v6.5-rc2#n2049
Oh, right, thanks. I vaguely remember the logic that if the immediate
parent is a Root Port, there are no other branches in the hierarchy to
worry about, so we can just configure the Root Port MPS to match the
device.
> > code, I thought we would leave RP MPS=256 and EP MPS=128, which would
> > be a problem. But maybe the PCI core changes the RP down to MPS=128?
> >
> > Bjorn
Hi Krzysztof, Bjorn,
On 18/07/2023 03:52, 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.
>
> 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")
> Signed-off-by: Vidya Sagar <[email protected]>
> ---
> 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);
I see a version of this patch here ...
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller/tegra194
However, I don't see this in -next yet. If you are happy with this
latest version, could we get this into -next?
FWIW ...
Acked-by: Jon Hunter <[email protected]>
Thanks!
Jon
--
nvpublic
On Fri, Jul 21, 2023 at 09:23:01AM +0100, Jon Hunter wrote:
> ...
> I see a version of this patch here ...
>
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller/tegra194
>
> However, I don't see this in -next yet. If you are happy with this latest
> version, could we get this into -next?
I'm on vacation until Tuesday; will build a new -next branch Tuesday
or Wednesday.
On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
> 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.
> 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 <[email protected]>
I know that this patch is merged. But I happen to test a similar change on Qcom
platform during a patch review and found that the PCI core changes MPS to 128
when a 128byte supported device is found:
[ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128)
[ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128)
This was just randomly tested on a platform whose Root port DEVCAP was 128, but
it shouldn't matter. And I didn't change the default bus configuration.
Wondering how you ended up facing issues with it.
- Mani
> ---
> drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 4fdadc7b045f..877d81b13334 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
> u32 val;
> - u16 val_16;
>
> pp->bridge->ops = &tegra_pci_ops;
>
> @@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
> val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
> @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
> 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
>
--
மணிவண்ணன் சதாசிவம்
On Tue, Jul 25, 2023 at 01:49:35PM +0530, Vidya Sagar wrote:
>
>
> On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
> > > 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.
> > > 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 <[email protected]>
> >
> > I know that this patch is merged. But I happen to test a similar change on Qcom
> > platform during a patch review and found that the PCI core changes MPS to 128
> > when a 128byte supported device is found:
> >
> > [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128)
> > [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128)
> >
> > This was just randomly tested on a platform whose Root port DEVCAP was 128, but
> > it shouldn't matter. And I didn't change the default bus configuration.
> >
> > Wondering how you ended up facing issues with it.
>
> If the endpiont device that has support only for 128byte MPS is connected
> directly to the root port, then, I agree that the PCIe sub-system takes care
> of changing the MPS to the common lowest MPS, but if the endpoint device is
> connected behind a PCIe switch, then the PCIe subsystem doesn't configure
> the MPS properly.
>
Ah, I missed the fact that your issue only happens with a PCIe switch. But
shouldn't this be fixed in the PCI core instead?
I mean, PCI core should use 128byte in your case for Root port unless it is not
allowed in the spec (which I doubt).
- Mani
> -Vidya Sagar
>
> >
> > - Mani
> >
> > > ---
> > > drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
> > > 1 file changed, 13 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > index 4fdadc7b045f..877d81b13334 100644
> > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
> > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
> > > u32 val;
> > > - u16 val_16;
> > >
> > > pp->bridge->ops = &tegra_pci_ops;
> > >
> > > @@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
> > > val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
> > > @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
> > > 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
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
>> 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.
>> 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 <[email protected]>
>
> I know that this patch is merged. But I happen to test a similar change on Qcom
> platform during a patch review and found that the PCI core changes MPS to 128
> when a 128byte supported device is found:
>
> [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128)
> [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128)
>
> This was just randomly tested on a platform whose Root port DEVCAP was 128, but
> it shouldn't matter. And I didn't change the default bus configuration.
>
> Wondering how you ended up facing issues with it.
If the endpiont device that has support only for 128byte MPS is
connected directly to the root port, then, I agree that the PCIe
sub-system takes care of changing the MPS to the common lowest MPS, but
if the endpoint device is connected behind a PCIe switch, then the PCIe
subsystem doesn't configure the MPS properly.
-Vidya Sagar
>
> - Mani
>
>> ---
>> drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
>> 1 file changed, 13 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index 4fdadc7b045f..877d81b13334 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
>> u32 val;
>> - u16 val_16;
>>
>> pp->bridge->ops = &tegra_pci_ops;
>>
>> @@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
>> val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
>> @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
>> 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
>>
>
> --
> மணிவண்ணன் சதாசிவம்
On 7/25/2023 2:00 PM, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, Jul 25, 2023 at 01:49:35PM +0530, Vidya Sagar wrote:
>>
>>
>> On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
>>>> 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.
>>>> 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 <[email protected]>
>>>
>>> I know that this patch is merged. But I happen to test a similar change on Qcom
>>> platform during a patch review and found that the PCI core changes MPS to 128
>>> when a 128byte supported device is found:
>>>
>>> [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128)
>>> [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128)
>>>
>>> This was just randomly tested on a platform whose Root port DEVCAP was 128, but
>>> it shouldn't matter. And I didn't change the default bus configuration.
>>>
>>> Wondering how you ended up facing issues with it.
>>
>> If the endpiont device that has support only for 128byte MPS is connected
>> directly to the root port, then, I agree that the PCIe sub-system takes care
>> of changing the MPS to the common lowest MPS, but if the endpoint device is
>> connected behind a PCIe switch, then the PCIe subsystem doesn't configure
>> the MPS properly.
>>
>
> Ah, I missed the fact that your issue only happens with a PCIe switch. But
> shouldn't this be fixed in the PCI core instead?
>
> I mean, PCI core should use 128byte in your case for Root port unless it is not
> allowed in the spec (which I doubt).
well, if the RP's DevCtl is set to 256MPS by default, then, the core
wouldn't do it automatically unless either 'pcie_bus_safe' or
'pcie_bus_perf' is passed.
>
> - Mani
>
>> -Vidya Sagar
>>
>>>
>>> - Mani
>>>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
>>>> 1 file changed, 13 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> index 4fdadc7b045f..877d81b13334 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
>>>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
>>>> u32 val;
>>>> - u16 val_16;
>>>>
>>>> pp->bridge->ops = &tegra_pci_ops;
>>>>
>>>> @@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
>>>> val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
>>>> @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
>>>> 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
>>>>
>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
>
> --
> மணிவண்ணன் சதாசிவம்
On Tue, Jul 25, 2023 at 02:51:10PM +0530, Vidya Sagar wrote:
>
>
> On 7/25/2023 2:00 PM, Manivannan Sadhasivam wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, Jul 25, 2023 at 01:49:35PM +0530, Vidya Sagar wrote:
> > >
> > >
> > > On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Thu, Jun 08, 2023 at 03:06:52PM +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 addresed 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.
> > > > > 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.
> > > > > 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 <[email protected]>
> > > >
> > > > I know that this patch is merged. But I happen to test a similar change on Qcom
> > > > platform during a patch review and found that the PCI core changes MPS to 128
> > > > when a 128byte supported device is found:
> > > >
> > > > [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128)
> > > > [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128)
> > > >
> > > > This was just randomly tested on a platform whose Root port DEVCAP was 128, but
> > > > it shouldn't matter. And I didn't change the default bus configuration.
> > > >
> > > > Wondering how you ended up facing issues with it.
> > >
> > > If the endpiont device that has support only for 128byte MPS is connected
> > > directly to the root port, then, I agree that the PCIe sub-system takes care
> > > of changing the MPS to the common lowest MPS, but if the endpoint device is
> > > connected behind a PCIe switch, then the PCIe subsystem doesn't configure
> > > the MPS properly.
> > >
> >
> > Ah, I missed the fact that your issue only happens with a PCIe switch. But
> > shouldn't this be fixed in the PCI core instead?
> >
> > I mean, PCI core should use 128byte in your case for Root port unless it is not
> > allowed in the spec (which I doubt).
> well, if the RP's DevCtl is set to 256MPS by default, then, the core
> wouldn't do it automatically unless either 'pcie_bus_safe' or
> 'pcie_bus_perf' is passed.
>
That's what I'm referring to. The default configuration shouldn't cause Root
port to send TLPs with unsupported payload. Moreover, this is not the case for
immediate children. So definitely the PCI core should exhibit the same behavior
for all downstream devices.
- Mani
>
> >
> > - Mani
> >
> > > -Vidya Sagar
> > >
> > > >
> > > > - Mani
> > > >
> > > > > ---
> > > > > drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
> > > > > 1 file changed, 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > > index 4fdadc7b045f..877d81b13334 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > > @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
> > > > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
> > > > > u32 val;
> > > > > - u16 val_16;
> > > > >
> > > > > pp->bridge->ops = &tegra_pci_ops;
> > > > >
> > > > > @@ -900,11 +899,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 +1750,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,11 +1880,6 @@ 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) {
> > > > > val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
> > > > > @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
> > > > > 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
> > > > >
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
Hi Bjorn,
On 21/07/2023 11:35, Bjorn Helgaas wrote:
> On Fri, Jul 21, 2023 at 09:23:01AM +0100, Jon Hunter wrote:
>> ...
>
>> I see a version of this patch here ...
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller/tegra194
>>
>> However, I don't see this in -next yet. If you are happy with this latest
>> version, could we get this into -next?
>
> I'm on vacation until Tuesday; will build a new -next branch Tuesday
> or Wednesday.
A friendly reminder on this. Can we queue this up for -next?
I know that there is further discussion on-going about if the core could
handle this, but right now PCIe is broken on our NVIDIA IGX Orin board
and we would like to merge this now in the short-term at least.
Thanks!
Jon
--
nvpublic
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.
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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
>>
Hello!
[...]
> > > 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.
Updated commit at:
https://git.kernel.org/pci/pci/c/ebfde1584d9f
Thank you everyone!
Krzysztof