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

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

[Niklas Cassel]

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;
+
+	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);
+}
+
+static void rockchip_pcie_ep_pre_init(struct dw_pcie_ep *ep)
+{
+	rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
+}
+
 static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
@@ -314,6 +359,7 @@ rockchip_pcie_get_features(struct dw_pcie_ep *ep)
 
 static const struct dw_pcie_ep_ops rockchip_pcie_ep_ops = {
 	.init = rockchip_pcie_ep_init,
+	.pre_init = rockchip_pcie_ep_pre_init,
 	.raise_irq = rockchip_pcie_raise_irq,
 	.get_features = rockchip_pcie_get_features,
 };
-- 
2.48.1

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

[Bjorn Helgaas]

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]

Maybe add a blank line and indent the message since it's quoted
material?  E.g.,

  When running the rk3588 in endpoint mode, with an Intel IOMMU, the
  host side prints:

    DMAR: VT-d detected Invalidation Time-out Error: SID 0

  When running the rk3588 in endpoint mode, with an AMD ...

    iommu ivhd0: AMD-Vi: Event logged [IOTLB_INV_TIMEOUT device=63:00.0 address=0x42b5b01a0]

Too bad DMAR isn't smart enough to include a device ID in its message ;)

Can you include something here about what the issue is?  Based on the
subject line and the patch, I assume something is wrong with the ATS
Capability?  I guess this is some kind of rk3588 defect, right?

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

s/capabilties/capabilities/

Bjorn

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

[Krzysztof Wilczyński]

Hello,

> + * ATS does not work on rk3588 when running in EP mode.

Would it be OK if we started to style "rk3588" as "RK3588", unless the
lower-case is preferred?  I had a look at Rockchip's own datasheet, and the
product code names seem to be styled with upper-case.  That said, I am not
a Rockchip expert.  Just curious for the sake of consistency.

> +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;
> +
> +	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);
> +}

To keep things consistent, it would be nice to capitalise sentences in code
comments, and end them with a full-stop where appropriate (e.g., longer
sentences, etc.).  That said, this is something I can after applying, to
save you the hassle of sending another versions.

Thank you!

	Krzysztof

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

[Krzysztof Wilczyński]

Hello,

> > 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]
> 
> Maybe add a blank line and indent the message since it's quoted
> material?  E.g.,
> 
>   When running the rk3588 in endpoint mode, with an Intel IOMMU, the
>   host side prints:
> 
>     DMAR: VT-d detected Invalidation Time-out Error: SID 0
> 
>   When running the rk3588 in endpoint mode, with an AMD ...
> 
>     iommu ivhd0: AMD-Vi: Event logged [IOTLB_INV_TIMEOUT device=63:00.0 address=0x42b5b01a0]
> 
> Too bad DMAR isn't smart enough to include a device ID in its message ;)
> 
> Can you include something here about what the issue is?  Based on the
> subject line and the patch, I assume something is wrong with the ATS
> Capability?  I guess this is some kind of rk3588 defect, right?
> 
> > 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.
> 
> s/capabilties/capabilities/

If there are no code changes here, then I can update the commit log
directly on the branch itself once applied.

Thank you!

	Krzysztof

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

[Manivannan Sadhasivam]

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.

> +
> +	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);

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.

- Mani

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

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

[Krzysztof Wilczyński]

Hello,

[...]
> Can you include something here about what the issue is?  Based on the
> subject line and the patch, I assume something is wrong with the ATS
> Capability?  I guess this is some kind of rk3588 defect, right?
> 
> > 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.

Rockchip folks, anything to add about this issue?  Perhaps there is an
erratum about this?  Any code reviews?  Anything?

Western Digital folks are doing you a lot of favour with all the upstream
work they do maintaining drivers for your platforms.  But it would be nice
if Rockchip took some ownership.  I have seen none recently.  No reviews,
not even an Acked-by, nothing.  A bit of a letdown, if you ask me.

Thank you!

	Krzysztof

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

[Shawn Lin]

On 2025/2/22 4:26, 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,

Niklas, Thanks for reporting this issue. It's been a while before
getting confirmation from the design team. Now I can confirm the ATS 
support for RK3588 is only available running as RC but I'm still
requesting erratum about this issue if possible.

Acked-by: Shawn Lin <shawn.lin@rock-chips.com>

> 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;
> +
> +	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);
> +}
> +
> +static void rockchip_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> +{
> +	rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
> +}
> +
>   static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
>   {
>   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> @@ -314,6 +359,7 @@ rockchip_pcie_get_features(struct dw_pcie_ep *ep)
>   
>   static const struct dw_pcie_ep_ops rockchip_pcie_ep_ops = {
>   	.init = rockchip_pcie_ep_init,
> +	.pre_init = rockchip_pcie_ep_pre_init,
>   	.raise_irq = rockchip_pcie_raise_irq,
>   	.get_features = rockchip_pcie_get_features,
>   };

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

[Niklas Cassel]

Hello Shawn,

On Tue, Feb 25, 2025 at 09:35:22AM +0800, Shawn Lin wrote:
> On 2025/2/22 4:26, 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,
> 
> Niklas, Thanks for reporting this issue. It's been a while before
> getting confirmation from the design team. Now I can confirm the ATS support
> for RK3588 is only available running as RC but I'm still
> requesting erratum about this issue if possible.
> 
> Acked-by: Shawn Lin <shawn.lin@rock-chips.com>

Thank you for confirming!

Considering that rock5b running in RC mode:
# lspci -vvvs 0000:00:00.0 | grep Capa
        Capabilities: [40] Power Management version 3
        Capabilities: [50] MSI: Enable+ Count=16/32 Maskable+ 64bit+
        Capabilities: [70] Express (v2) Root Port (Slot-), IntMsgNum 8
        Capabilities: [b0] MSI-X: Enable- Count=128 Masked-
        Capabilities: [100 v2] Advanced Error Reporting
        Capabilities: [148 v1] Secondary PCI Express
        Capabilities: [190 v1] L1 PM Substates
        Capabilities: [1d0 v1] Vendor Specific Information: ID=0002 Rev=4 Len=100 <?>
        Capabilities: [2d0 v1] Vendor Specific Information: ID=0006 Rev=0 Len=018 <?>

and rock5b running in EP mode:
# lspci -vvvs 0000:01:00.0 | grep Capa
        Capabilities: [40] Power Management version 3
        Capabilities: [50] MSI: Enable+ Count=32/32 Maskable+ 64bit+
        Capabilities: [70] Express (v2) Endpoint, IntMsgNum 8
        Capabilities: [b0] MSI-X: Enable- Count=2048 Masked-
        Capabilities: [100 v2] Advanced Error Reporting
        Capabilities: [148 v1] Secondary PCI Express
        Capabilities: [178 v1] Page Request Interface (PRI)
                Page Request Capacity: 00000001, Page Request Allocation: 00000000
        Capabilities: [188 v1] Latency Tolerance Reporting
        Capabilities: [190 v1] L1 PM Substates
        Capabilities: [1a0 v1] Dynamic Power Allocation <?>
        Capabilities: [1d0 v1] Vendor Specific Information: ID=0002 Rev=4 Len=100 <?>
        Capabilities: [2d0 v1] Vendor Specific Information: ID=0006 Rev=0 Len=018 <?>
        Capabilities: [2e8 v1] Physical Resizable BAR

already exposes different Capabilities (depending on the mode the PCIe
controller is running in), I would say that it slightly confusing that
Synopsys chose not to hide the ATS Capability when the PCIe controller
is running in EP mode.
So, I would guess that there is an errata for this.

But I think that your confirmation is enough.
Will take a while before I can send out a V2 though, but quite confident
that we can get something merged in time for 6.15.


Kind regards,
Niklas

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

[Niklas Cassel]

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