Re: [PATCH 2/2] PCI: dw-rockchip: hide broken ATS capability

[Niklas Cassel <cassel@kernel.org>]

Hello Mani,

On Sat, Feb 22, 2025 at 09:38:37PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Feb 21, 2025 at 09:26:48PM +0100, Niklas Cassel wrote:
> > When running the rk3588 in endpoint mode, with an Intel host with IOMMU
> > enabled, the host side prints:
> > DMAR: VT-d detected Invalidation Time-out Error: SID 0
> > 
> > When running the rk3588 in endpoint mode, with an AMD host with IOMMU
> > enabled, the host side prints:
> > iommu ivhd0: AMD-Vi: Event logged [IOTLB_INV_TIMEOUT device=63:00.0 address=0x42b5b01a0]
> > 
> > Usually, to handle these issues, we add a quirk for the PCI vendor and
> > device ID in drivers/pci/quirks.c with quirk_no_ats(). That is because
> > we cannot usually modify the capabilities on the EP side.
> > 
> > In this case, we can modify the capabilties on the EP side. Thus, hide the
> > broken ATS capability on rk3588 when running in EP mode. That way,
> > we don't need any quirk on the host side, and we see no errors on the host
> > side, and we can run pci_endpoint_test successfully, with the IOMMU
> > enabled on the host side.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 836ea10eafbb..2be005c1a161 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -242,6 +242,51 @@ static const struct dw_pcie_host_ops rockchip_pcie_host_ops = {
> >  	.init = rockchip_pcie_host_init,
> >  };
> >  
> > +/*
> > + * ATS does not work on rk3588 when running in EP mode.
> > + * After a host has enabled ATS on the EP side, it will send an IOTLB
> > + * invalidation request to the EP side. The rk3588 will never send a completion
> > + * back and eventually the host will print an IOTLB_INV_TIMEOUT error, and the
> > + * EP will not be operational. If we hide the ATS cap, things work as expected.
> > + */
> > +static void rockchip_pcie_ep_hide_broken_ats_cap_rk3588(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct device *dev = pci->dev;
> > +	unsigned int spcie_cap_offset, next_cap_offset;
> > +	u32 spcie_cap_header, next_cap_header;
> > +
> > +	/* only hide the ATS cap for rk3588 running in EP mode */
> > +	if (!of_device_is_compatible(dev->of_node, "rockchip,rk3588-pcie-ep"))
> > +		return;
> 
> Compatible checks always tend to extend. So please use a boolean flag to
> identify the quirk in 'struct rockchip_pcie_of_data' and set it in
> 'rockchip_pcie_ep_of_data_rk3588'. This way, other SoCs could also reuse the
> flag if required.

I agree that in many cases, compatible checks tend to extend.

However, the ATS capability is broken only for rk3588, thus we need to provide
the ATS cap_id and the cap_id for the CAP immediately preceding the ATS cap_id.

The linked list of capabilities will be different for every SoC.
If we look at the only other SoC supported by this EPC, rk3568, it does not
even have the ATS cap in the linked list.

Thus, I think that creating a boolean flag for this is wrong.
Even in the unlikely scenario that there will come a Rockchip SoC in the
future with an identical linked list of capabilities, I think that what you
are proposing this is premature optimization (AKA the root of all evil :P)


> 
> > +
> > +	spcie_cap_offset = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_SECPCI);
> > +	if (!spcie_cap_offset)
> > +		return;
> > +
> > +	spcie_cap_header = dw_pcie_readl_dbi(pci, spcie_cap_offset);
> > +	next_cap_offset = PCI_EXT_CAP_NEXT(spcie_cap_header);
> > +
> > +	next_cap_header = dw_pcie_readl_dbi(pci, next_cap_offset);
> > +	if (PCI_EXT_CAP_ID(next_cap_header) != PCI_EXT_CAP_ID_ATS)
> > +		return;
> > +
> > +	/* clear next ptr */
> > +	spcie_cap_header &= ~GENMASK(31, 20);
> > +
> > +	/* set next ptr to next ptr of ATS_CAP */
> > +	spcie_cap_header |= next_cap_header & GENMASK(31, 20);
> > +
> > +	dw_pcie_dbi_ro_wr_en(pci);
> > +	dw_pcie_writel_dbi(pci, spcie_cap_offset, spcie_cap_header);
> > +	dw_pcie_dbi_ro_wr_dis(pci);
> 
> This code is mostly generic. So please move it to pcie-designware-ep.c. The
> function should just accept two CAP IDs (prev and target) and hide the target
> capability. Like,
> 
> 	dw_pcie_hide_ext_capability(pci, PCI_EXT_CAP_ID_SECPCI,
> 				    PCI_EXT_CAP_ID_ATS);

Sure, I will create a generic function in pcie-designware-ep.c.


> 
> Its too bad that we cannot traverse back from the target capability. We could
> open code dw_pcie_ep_find_ext_capability() to keep track of the prev_cap_offset
> though.

I'd rather skip this. Seems like too many special cases.
E.g. what should dw_pcie_ep_find_ext_capability() return if there is no prev?
It seems easier to just need to provide prev_cap_id as well.


Kind regards,
Niklas