| Message ID | 20250221202646.395252-4-cassel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [1/2] PCI: dwc: ep: Remove superfluous function dw_pcie_ep_find_ext_capability() | expand |
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
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
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
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
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, };
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(+)