Re: [PATCH v3] PCI: rcar-gen4: Limit Max_Read_Request_Size and Max_Payload_Size to 256 Bytes

[Marek Vasut <marek.vasut@mailbox.org>]

Hello Bjorn,

On 9/3/26 10:27 PM, Bjorn Helgaas wrote:
> On Thu, Sep 03, 2026 at 08:51:06PM +0200, Marek Vasut wrote:
>> Hello Bjorn,
>>
>> On 9/3/26 7:32 PM, Bjorn Helgaas wrote:
>>> [+cc Ziyao, Rong, Huacai for similar Loongson MRRS issue]
>>>
>>> On Fri, Aug 21, 2026 at 04:05:51AM +0200, Marek Vasut wrote:
>>>> R-Car Gen4 PCIe controller has a hardware limitation of 256 Bytes
>>>> Max_Payload_Size (MPS). PCIe specification indicates that the MPS
>>>> must not exceed minimum MPS of any element along the packet path.
>>>> Force limit Max_Payload_Size to at most 256 Bytes for each device
>>>> connected to this PCIe controller.
>>>
>>> IIUC the PCI core already enforces this limit, and what this patch
>>> does is double-check that this limit is observed with this check,
>>> right?
>>
>> That is correct, this was changed in V3, I missed the commit message update,
>> sorry.
>>
>> Would you like me to respin the patch one more time with an updated commit
>> message, or would you be willing to fix it up in tree ?
> 
> I fixed the commit log, no problem.

Thank you.

>>>     +	WARN_ON(pcie_get_mps(dev) > 256);
>>>
>>> More below.
>>
>>
>> [...]
>>
>>>> +	bridge->no_inc_mrrs = 1;
>>>> +	if (pcie_get_readrq(dev) > 256) {
>>>> +		pci_info(dev, "Limiting MRRS to 256 bytes\n");
>>>> +		pcie_set_readrq(dev, 256);
>>>> +	}
>>>
>>> It would be nice if all the platforms that need no_inc_mrrs could
>>> apply it the same way, but I assume you saw loongson_mrrs_quirk() and
>>> loongson_set_min_mrrs_quirk() in the process of finding no_inc_mrrs,
>>> and chose a different implementation strategy for some reason, e.g.,
>>> this way doesn't have to include device IDs for all the Root Ports?
>>
>> The loongson quirk won't work if the PCIe controller driver is built as a
>> module, which the R-Car Gen4 PCIe driver can be, and in fact is often built
>> as a module, because it depends on firmware which is loaded from filesystem.
>>
>> If the controller driver is built as a module, then
>> DECLARE_PCI_FIXUP_ENABLE() is not applied, the DECLARE_PCI_FIXUP_ENABLE() is
>> applied only on boot and therefore only for built-in drivers.
>>
>> I got burnt by DECLARE_PCI_FIXUP_ENABLE() in V1 of this patch.
> 
> Ouch, that does hurt.

I also mentioned this to TI a while back, because I think the 
pci-keystone.c has the same (module) issue. ( +CC Nishanth here too )

>> However, there is also another part to this -- the
>> rcar_gen4_pcie_enable_device() is called for every device on the bus and
>> applies the MRRS limitation to every device on the bus that is downstream of
>> the controller, not only the controller. This is necessary on this
>> controller variant, else hardware like PCIe SSDs with MRRS higher than the
>> controller break.
>>
>>> Maybe we should rework no_inc_mrrs in such a way that drivers could
>>> set a max MRRS in the struct pci_host_bridge and make
>>> pcie_write_mrrs() and pcie_set_readrq() pay attention to it?  That
>>> might let us get rid of the FIXUP approach.
>>
>> In light of the last paragraph above, that the MRRS has to be limited also
>> on all devices downstream of this particular controller, I would like to ask
>> -- does the Loongson controller have the same limitation or not ? If not,
>> then I would argue this quirk should be isolated to this controller variant
>> ; else, I am happy to start on the core patches.
> 
> I don't know if we'll get a real answer for Loongson (there's no
> maintainer listed for it, hint hint :)), but my guess is that it does
> apply to all devices downstream of the Loongson controller.
> 
> I think MRRS is mostly interesting for DMA because MMIO from CPUs is
> usually small sizes, far below the 128-byte or larger transfers that
> devices may do.

I agree with that, and DMA is what triggers the fault in my case.

Looking at the TI ks_pcie_quirk() FIXUP, I wonder if that might be a 
third instance of the same behavior. TI uses it to work around errata 
i2037 PCIe: PCI-Express May Corrupt Inbound Data [1] page 19 . But I now 
wonder, whether this behavior might be some common behavior of the DWC 
PCIe controller core ? Is there someone from Synopsys who might comment 
on that ?

[1] https://www.ti.com/lit/er/sprz452i/sprz452i.pdf