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

[Manivannan Sadhasivam]

On Wed, May 13, 2026 at 12:57:18AM +0200, Marek Vasut wrote:
> On 5/11/26 4:34 PM, Manivannan Sadhasivam wrote:
> 
> Hello Manivannan,
> 
> > >   drivers/pci/controller/dwc/pcie-rcar-gen4.c | 56 +++++++++++++++++++++
> > >   1 file changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > index 8b03c42f8c84c..82f0a074a71da 100644
> > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > @@ -576,6 +576,7 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> > >   static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
> > >   {
> > >   	struct dw_pcie *dw = &rcar->dw;
> > > +	u16 offset = dw_pcie_find_capability(dw, PCI_CAP_ID_EXP);
> > >   	u32 val;
> > >   	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
> > > @@ -584,11 +585,66 @@ static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
> > >   		val |= BIT(6);
> > >   	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
> > > +	val = dw_pcie_readl_dbi(dw, offset + PCI_EXP_DEVCTL);
> > > +	val &= ~(PCI_EXP_DEVCTL_PAYLOAD | PCI_EXP_DEVCTL_READRQ);
> > > +	val |= PCI_EXP_DEVCTL_PAYLOAD_256B | PCI_EXP_DEVCTL_READRQ_256B;
> > > +	dw_pcie_writel_dbi(dw, offset + PCI_EXP_DEVCTL, val);
> > 
> > Instead of limiting the MRRS/MPS values for all devices through quirks, why
> > can't you just limit the Root Port's MPSS value in PCI_EXP_DEVCAP?
> The root port MPSS is already 3'b001 = 256 Bytes and is read-only for
> EXPCAP1F0 (PCI_EXP_DEVCAP) .
> 
> The controller is limited to MPS 256 Bytes according to V4H rev.1.30
> documentation. There is no explicitly spelled out MRRS limitation in the
> documentation to my knowledge, except for the DMA hint, but please read on.
> 
> The root port EXPCAP2F0 MPS is 128 Bytes and MRRS is 512 Bytes .
> 
> I now noticed that in V4H rev.1.30 documentation, the EXPCAP2F0 MRRS field
> is default set to 3'b010 = 512 Bytes, but that value is "Reserved" and only
> two non-reserved values are 3'b000 and 3'b001 which are MRRS 128 Bytes and
> 256 Bytes respectively. That means MRRS has to be trimmed to maximum 256
> Bytes in software to avoid "Reserved" settings. I will also ask the hardware
> and documentation team about this.
> 
> As a result, I adjust EXPCAP2F0:
> 
> - I raise MPS from 128 Bytes to 256 Bytes
> - I reduce MRRS from 512 Bytes to 256 Bytes (this is important to prevent
> data corruption)
> 
> However, the downstream devices (in my case, PCIe SSD) can still be
> configured with MRRS > 256 (in my case, Crucial P5 Plus 1 TiB has MRRS=512
> and MPS=128), which is where the quirk kicks in and reconfigures MRRS for
> those downstream devices.
> 
> The pci_configure_mps() does propagate MPS from root port EXPCAP2F0 to
> downstream devices, but there is no equivalent for MRRS as far as I can find
> ?

Sorry for the late reply!

I'm now confused about the issue itself. In your v1 patch you said:

"R-Car Gen4 PCIe controller has a hardware limitation of 256 Bytes
maximum payload size. The PCIe DMA generates requests of size up
to minimum(Max_Read_Request_Size, Max_Payload_Size). Force limit
both Max_Read_Request_Size and Max_Payload_Size to 256 Bytes and
propagate this limit to all downstream devices."

The 256B limitation of Root Port MPS is clear. And you said, Root Port's MPSS is
already 256B. So this will prevent the endpoints from sending > 256B payload.

Also, the DMA calculation of min(MRRS,MPS) will also satisfy with 256B MPS even
if MRRS is higher i.e, 512B.

If MRRS is 512B, the endpoint should send two 256B TLPs based on MPS and that
shouldn't be a problem on the host. So I'm not sure how the higher MRRS value
induces data corruption here. And why you want to limit MRRS on all downstream
devices and not just the Root Port? Since you are using NVMe, the host will be
the one sending MRd TLP to the device. So I'm not sure how the endpoint MRRS
comes into play.

Am I missing something?

- Mani

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