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

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

[Vidya Sagar]

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 <vidyas@nvidia.com>
---
 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

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

[Bjorn Helgaas]

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

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

[Vidya Sagar]

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

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

[Jon Hunter]

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 <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

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

[Vidya Sagar]

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 <vidyas@nvidia.com>
---
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

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

[kernel test robot]

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/202306191113.M2RDKBvQ-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230619/202306191113.M2RDKBvQ-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306191113.M2RDKBvQ-lkp@intel.com/

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

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

[Vidya Sagar]

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 <vidyas@nvidia.com>
---
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

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

[Krzysztof Wilczyński]

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

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

[Bjorn Helgaas]

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 <vidyas@nvidia.com>
> ---
> 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
> 

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

[Vidya Sagar]

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@nvidia.com>
>> ---
>> 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
>>

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

[Bjorn Helgaas]

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

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

[Vidya Sagar]

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

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

[Bjorn Helgaas]

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

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

[Vidya Sagar]

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 <vidyas@nvidia.com>
---
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

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

[Jon Hunter]

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 <vidyas@nvidia.com>
> ---
> 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 <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic

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

[Bjorn Helgaas]

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.

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

[Jon Hunter]

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

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

[Bjorn Helgaas]

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 <vidyas@nvidia.com>
> ---
> 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
> 

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

[Vidya Sagar]

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@nvidia.com>
>> ---
>> 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
>>

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

[Krzysztof Wilczyński]

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

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

[Manivannan Sadhasivam]

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 <vidyas@nvidia.com>

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
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Vidya Sagar]

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 <vidyas@nvidia.com>
> 
> 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
>>
> 
> --
> மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

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 <vidyas@nvidia.com>
> > 
> > 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
> > > 
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

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

[Vidya Sagar]

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 <vidyas@nvidia.com>
>>>
>>> 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
>>>>
>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
> 
> --
> மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

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 <vidyas@nvidia.com>
> > > > 
> > > > 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
> > > > > 
> > > > 
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்