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

[Rong Zhang <i@rong.moe>]

Hi Bjorn, Marek,

Thanks for reaching me.

On Thu, 2026-09-03 at 15:27 -0500, 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.
> 
> > >    +	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.
> 
> > 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.

ACK. I agreed that it should make our life easier.

> 
> I don't know if we'll get a real answer for Loongson (there's no
> maintainer listed for it, hint hint :)), 
> 

(+CC Jiaxun)

The driver was introduced by Jiaxun without updating MAINTAINERS. I guess
he'd be willing to be listed as a maintainer.

I don't work for Loongson, but I do maintain several MIPS-based Loongson
devices for the Golang community with my colleagues and personally own a
MIPS-based Loongson-LS3A4000-7A1000-NUC-SE mini PC. I do some PCIe
experiments on it from time to time for fun. So I am OK if someone wants
to list me as a maintainer or reviewer :)

> but my guess is that it does
> apply to all devices downstream of the Loongson controller.

I believe this is the case. Maybe Jiaxun can shed a light on it too.

Just checked the kmsg log from April, the firmware seemed to only clamp
MRRS for devices directly connected to the root ports. IOW, it seemed to
only clamp MRRS for the upstream port of a PCIe switch, so
loongson_set_min_mrrs_quirk() had to fix up downstream ports.

If you need more information I can do some more experiments with the PCIe
switch card.

Thanks,
Rong

> 
> 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.