Message-ID: <161d6789-4e7f-456a-91ed-9ef682187eae@163.com>
Date: Wed, 5 Nov 2025 00:32:14 +0800
From: Hans Zhang <18255117159@....com>
To: Niklas Cassel <cassel@...nel.org>, Bjorn Helgaas <helgaas@...nel.org>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
 heiko@...ech.de, mani@...nel.org, yue.wang@...ogic.com, pali@...nel.org,
 neil.armstrong@...aro.org, robh@...nel.org, jingoohan1@...il.com,
 khilman@...libre.com, jbrunet@...libre.com,
 martin.blumenstingl@...glemail.com, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v5 1/2] PCI: Configure root port MPS during host probing



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

Powered by blists - more mailing lists