[PATCH] PCI: dw-rockchip: Configure max payload size on host init

[Niklas Cassel]

On Thu, Apr 17, 2025 at 10:19:10AM +0800, Hans Zhang wrote:
> On 2025/4/17 04:40, Bjorn Helgaas wrote:
> > On Wed, Apr 16, 2025 at 11:19:26PM +0800, Hans Zhang wrote:
> > > The RK3588's PCIe controller defaults to a 128-byte max payload size,
> > > but its hardware capability actually supports 256 bytes. This results
> > > in suboptimal performance with devices that support larger payloads.
> > > 
> > > Signed-off-by: Hans Zhang <18255117159 at 163.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > index c624b7ebd118..5bbb536a2576 100644
> > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > @@ -477,6 +477,22 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> > >   	return IRQ_HANDLED;
> > >   }
> > > +static void rockchip_pcie_set_max_payload(struct rockchip_pcie *rockchip)
> > > +{
> > > +	struct dw_pcie *pci = &rockchip->pci;
> > > +	u32 dev_cap, dev_ctrl;
> > > +	u16 offset;
> > > +
> > > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > +	dev_cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCAP);
> > > +	dev_cap &= PCI_EXP_DEVCAP_PAYLOAD;
> > > +
> > > +	dev_ctrl = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> > > +	dev_ctrl &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > > +	dev_ctrl |= dev_cap << 5;
> > > +	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, dev_ctrl);
> > > +}
> > 
> > I can't really complain too much about this since meson does basically
> > the same thing, but there are some things I don't like about this:
> > 
> >    - I don't think it's safe to set MPS higher in all cases.  If we set
> >      the Root Port MPS=256, and an Endpoint only supports MPS=128, the
> >      Endpoint may do a 256-byte DMA read (assuming its MRRS>=256).  In
> >      that case the RP may respond with a 256-byte payload the Endpoint
> >      can't handle.  The generic code in pci_configure_mps() might be
> >      smart enough to avoid that situation, but I'm not confident about
> >      it.  Maybe I could be convinced.
> > 
> 
> Dear Bjorn,
> 
> Thank you very much for your reply. If we set the Root Port MPS=256, and an
> Endpoint only supports MPS=128. Finally, Root Port is also set to MPS=128 in
> pci_configure_mps.

In you example below, the Endpoint has:
 DevCap: MaxPayload 512 bytes

So at least your example can't be used to prove this specific point.
But perhaps you just wanted to show that your Max Payload Size increase
actually works?


Kind regards,
Niklas