Re: [PATCH v5 1/2] PCI: Configure root port MPS during host probing

[Hans Zhang]

On 2025/11/4 23:00, Niklas Cassel wrote:
> Hello Hans,
>
> Any chance that you have time to re-spin this series?
>
> I've seen some new DWC based drivers like NXP that also want to
> configure MPS.
>
> Most likely we know the reason... The PCI core does not change it
> without this series.

Hi Niklas,

I will modify a version based on Bjorn and your opinions and then send 
it out.

Best regards,
Hans

> Kind regards,
> Niklas
>
> On Thu, Sep 04, 2025 at 12:13:43PM +0200, Niklas Cassel wrote:
> > On Tue, Sep 02, 2025 at 12:48:28PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 20, 2025 at 11:55:06PM +0800, Hans Zhang wrote:
> > > > Current PCIe initialization logic may leave root ports operating with
> > > > non-optimal Maximum Payload Size (MPS) settings. While downstream device
> > > > configuration is handled during bus enumeration, root port MPS values
> > > > inherited from firmware or hardware defaults ...
> > >
> > > Apparently Root Port MPS configuration is different from that for
> > > downstream devices?
> >
> > pci_host_probe() will call pci_scan_root_bus_bridge(), which will call
> > pci_scan_single_device(), which will call pci_device_add(), which will
> > call pci_configure_device(), which will call pci_configure_mps().
> >
> > This will be done for both bridges and endpoints.
> >
> > The bridge will be scanned/added first, before devices behind the bridge.
> >
> >
> > While pci_configure_device()/pci_configure_mps() will be called for both
> > bridges and endpoints, pci_configure_mps() will do an early return for
> > devices where pci_upstream_bridge() returns NULL, i.e. for devices where
> > that does not have an upstream bridge, i.e. for the root bridge itself:
> > https://github.com/torvalds/linux/blob/v6.17-rc4/drivers/pci/probe.c#L2181-L2182
> >
> > So MPS will not be touched for root bridges.
> >
> > This patch ensures that MPS for root bridges gets initialized to MPSS
> > (Max supported MPS).
> >
> > Later, when pci_configure_device()/pci_configure_mps() is called for a
> > device behind the bridge, if the MPSS of the device behind the bridge is
> > smaller than the MPS of the bridge, the code reduces the MPS of the bridge:
> > https://github.com/torvalds/linux/blob/v6.17-rc4/drivers/pci/probe.c#L2205
> >
> >
> > My only question with this patch is if there is a bridge behind a bridge,
> > will the bridge behind the bridge still have pci_pcie_type() ==
> > PCI_EXP_TYPE_ROOT_PORT ?
> >
> > If so, perhaps we should modify this patch from:
> >
> > +       if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> > +           pcie_bus_config != PCIE_BUS_TUNE_OFF) {
> > +               pcie_write_mps(dev, 128 << dev->pcie_mpss);
> > +       }
> > +
> >          if (!bridge || !pci_is_pcie(bridge))
> >                  return;
> >
> >
> > to:
> >
> > +       if (!bridge && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> > +           pcie_bus_config != PCIE_BUS_TUNE_OFF) {
> > +               pcie_write_mps(dev, 128 << dev->pcie_mpss);
> > +       }
> > +
> >          if (!bridge || !pci_is_pcie(bridge))
> >                  return;
> >
> >
> >
> > > > During host controller probing phase, when PCIe bus tuning is enabled,
> > > > the implementation now configures root port MPS settings to their
> > > > hardware-supported maximum values. Specifically, when configuring the MPS
> > > > for a PCIe device, if the device is a root port and the bus tuning is not
> > > > disabled (PCIE_BUS_TUNE_OFF), the MPS is set to 128 << dev->pcie_mpss to
> > > > match the Root Port's maximum supported payload size. The Max Read Request
> > > > Size (MRRS) is subsequently adjusted through existing companion logic to
> > > > maintain compatibility with PCIe specifications.
> > > >
> > > > Note that this initial setting of the root port MPS to the maximum might
> > > > be reduced later during the enumeration of downstream devices if any of
> > > > those devices do not support the maximum MPS of the root port.
> > > >
> > > > Explicit initialization at host probing stage ensures consistent PCIe
> > > > topology configuration before downstream devices perform their own MPS
> > > > negotiations. This proactive approach addresses platform-specific
> > > > requirements where controller drivers depend on properly initialized
> > > > root port settings, while maintaining backward compatibility through
> > > > PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
> > > > utilized without altering existing device negotiation behaviors.
> > >
> > > This last paragraph seems kind of like marketing without any real
> > > content.  Is there something important in there?
> > >
> > > Nits:
> > > s/root port/Root Port/
> > >
> > > Reword "implementation now configures" to be clear about whether "now"
> > > refers to before this patch or after.
> > >
> > > Update the MRRS "to maintain compatibility" part.  I'm dubious about
> > > there being a spec compatibility issue with respect to MRRS.  Cite the
> > > relevant section if there is an issue.
> >
> > I'm not sure why the commit message mentions MRRS at all.
> >
> > Sure, pcie_write_mrrs() might set MRRS to MPS, but that is existing logic
> > and not really related to the change in this patch IMO.
> >
> >
> > Kind regards,
> > Niklas