iommu/rockchip: Request irqs in rk_iommu_probe()

Move request_irq to the end of rk_iommu_probe().

Suggested-by Robin Murphy <[email protected]>

Signed-off-by Jeffy Chen <[email protected]>

Acked-by Robin Murphy <[email protected]>

---

Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3:
Loop platform_get_irq() as Robin suggested.

Changes in v2: None

 drivers/iommu/rockchip-iommu.c | 38 +++++++++-----------------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

drivers/iommu/rockchip-iommu.c +9 -30

index 73117dbe839e..ec3ff936aa60 100644
@@ -90,8 +90,6 @@ struct rk_iommu {
        struct device *dev;
        void __iomem **bases;
        int num_mmu;
-       int *irq;
-       int num_irq;
        bool reset_disabled;
        struct iommu_device iommu;
        struct list_head node; /* entry in rk_iommu_domain.iommus */

@@ -830,13 +828,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 
        iommu->domain = domain;
 
-       for (i = 0; i < iommu->num_irq; i++) {
-               ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-                                      IRQF_SHARED, dev_name(dev), iommu);
-               if (ret)
-                       return ret;
-       }
-
        for (i = 0; i < iommu->num_mmu; i++) {
                rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
                               rk_domain->dt_dma);

@@ -885,9 +876,6 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
        }
        rk_iommu_disable_stall(iommu);
 
-       for (i = 0; i < iommu->num_irq; i++)
-               devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
        iommu->domain = NULL;
 
        dev_dbg(dev, "Detached from iommu domain\n");

@@ -1138,7 +1126,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
        struct rk_iommu *iommu;
        struct resource *res;
        int num_res = pdev->num_resources;
-       int err, i;
+       int err, i, irq;
 
        iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
        if (!iommu)

@@ -1165,23 +1153,15 @@ static int rk_iommu_probe(struct platform_device *pdev)
        if (iommu->num_mmu == 0)
                return PTR_ERR(iommu->bases[0]);
 
-       iommu->num_irq = platform_irq_count(pdev);
-       if (iommu->num_irq < 0)
-               return iommu->num_irq;
-       if (iommu->num_irq == 0)
-               return -ENXIO;
+       i = 0;
+       while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
+               if (irq < 0)
+                       return irq;
 
-       iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
-                                 GFP_KERNEL);
-       if (!iommu->irq)
-               return -ENOMEM;
-
-       for (i = 0; i < iommu->num_irq; i++) {
-               iommu->irq[i] = platform_get_irq(pdev, i);
-               if (iommu->irq[i] < 0) {
-                       dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
-                       return -ENXIO;
-               }
+               err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+                                      IRQF_SHARED, dev_name(dev), iommu);
+               if (err)
+                       return err;
        }
 
        iommu->reset_disabled = device_property_read_bool(dev,
--

2.11.0

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case.

So remove the enable/disable handling and add appropriate condition to the irq handler.

Signed-off-by Sandy Huang <[email protected]>

[added an actual commit message]

Signed-off-by Heiko Stuebner <[email protected]>

---
Hi Ezequiel,

this patch came from a discussion I had with Rockchip people over the iommu changes and resulting issues back in april, but somehow was forgotten and not posted to the lists. Correcting that now.

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case.

So remove the enable/disable handling and add appropriate condition to the irq handler.

Signed-off-by Sandy Huang <[email protected]>

[added an actual commit message]

Signed-off-by Heiko Stuebner <[email protected]>

---
Hi Ezequiel,

this patch came from a discussion I had with Rockchip people over the iommu changes and resulting issues back in april, but somehow was forgotten and not posted to the lists. Correcting that now.

So removing the enable/disable voodoo on the shared interrupt is the preferred way.

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 ++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -4

index 510cdf0..61493d4 100644
@@ -549,8 +549,6 @@ static int vop_enable(struct drm_crtc *crtc)
 
 	spin_unlock(&vop->reg_lock);
 
-	enable_irq(vop->irq);
-
 	drm_crtc_vblank_on(crtc);
 
 	return 0;

@@ -596,8 +594,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	vop_dsp_hold_valid_irq_disable(vop);
 
-	disable_irq(vop->irq);
-
 	vop->is_enabled = false;
 
 	/*

[Thanks Robin for pointing me to that patch.]

Hi Heiko,

Quoted 52 lines · Heiko Stübner

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case.

So remove the enable/disable handling and add appropriate condition to the irq handler.

Signed-off-by Sandy Huang <[email protected]>

[added an actual commit message]

Signed-off-by Heiko Stuebner <[email protected]>

---
Hi Ezequiel,

this patch came from a discussion I had with Rockchip people over the iommu changes and resulting issues back in april, but somehow was forgotten and not posted to the lists. Correcting that now.

So removing the enable/disable voodoo on the shared interrupt is the preferred way.

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 ++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -4

index 510cdf0..61493d4 100644
@@ -549,8 +549,6 @@ static int vop_enable(struct drm_crtc *crtc)
 
 	spin_unlock(&vop->reg_lock);
 
-	enable_irq(vop->irq);
-
 	drm_crtc_vblank_on(crtc);
 
 	return 0;

@@ -596,8 +594,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	vop_dsp_hold_valid_irq_disable(vop);
 
-	disable_irq(vop->irq);
-
 	vop->is_enabled = false;
 
 	/*

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static irqreturn_t vop_isr(int irq, void *data) · line 1172

Quoted 9 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +6 -0

@@ -1168,6 +1164,13 @@ static irqreturn_t vop_isr(int irq, void *data)
 	int ret = IRQ_NONE;
 
 	/*
+	 * since the irq is shared with iommu, iommu irq is enabled before vop
+	 * enable, so before vop enable we do nothing.
+	 */
+	if (!vop->is_enabled)
+		return IRQ_NONE;
+

What guarantee do we have that the IOMMU will actually service that interrupt? Can't we get into a situation where the interrupt gets ignored by both drivers and subsequently disabled by the core irq code as being spurious?

From what I understood of the way things are plugged together on the rockchip platforms, each individual VOP is behind a single IOMMU, hence there there is hardly any point in handling IOMMU interrupts if the VOP is off.

But I'm missing the actual reason behind this patch. Could you enlighten me?