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

[Niklas Cassel <cassel@kernel.org>]

On Thu, Apr 17, 2025 at 02:47:23PM +0800, Hans Zhang wrote:
> On 2025/4/17 14:01, Niklas Cassel wrote:
> > 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@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?
> > 
> 
> Dear Niklas,
> 
> Do you have an Endpoint with MPS=128? If so, you can also help verify the
> logic of the pci_configure_mps function. I don't have an Endpoint with
> MPS=128 here.

I imagine that it would be trivial to test with a PCIe controller running
in endpoint mode with the PCI endpoint subsystem in the kernel.
(As you should be able to set CAP.MPS before starting link training.)


> The processing logic of the pci_configure_mps function has been verified on
> our own SOC platform. Please refer to the following log.
> Our Root Port will set MPS=512.

(snip)

Ok, since it works to downgrade 512B to 256B, I would imagine that it also
would downgrade to 128B properly.


Kind regards,
Niklas