Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Jul 2023 15:33:02 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Vidya Sagar <vidyas@nvidia.com>
Cc:     lpieralisi@kernel.org, kw@linux.com, robh@kernel.org,
        bhelgaas@google.com, thierry.reding@gmail.com,
        jonathanh@nvidia.com, Sergey.Semin@baikalelectronics.ru,
        linux-pci@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org, kthota@nvidia.com,
        mmaddireddy@nvidia.com, sagar.tv@gmail.com
Subject: Re: [PATCH V1] Revert "PCI: tegra194: Enable support for 256 Byte
 payload"
Message-ID: <20230725100302.GD22139@thinkpad>
References: <20230608093652.1409485-1-vidyas@nvidia.com>
 <20230725075159.GB22139@thinkpad>
 <815d102d-be0c-8e5d-ac12-1500d90628da@nvidia.com>
 <20230725083030.GC22139@thinkpad>
 <770da5a3-ea02-ce87-8515-beb246082de8@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <770da5a3-ea02-ce87-8515-beb246082de8@nvidia.com>
Precedence: bulk

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
> > > > > 
> > > > 
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

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