Re: [PATCH] remoteproc: keystone: Request IRQs in probe()

[Andrew Davis]

On 3/6/26 11:19 AM, Mathieu Poirier wrote:

Good day,

On Mon, Mar 02, 2026 at 02:17:34PM -0600, Andrew Davis wrote:

IRQs can be registered in probe and only need to be enabled/disabled
during remoteproc start/stop. This lets us catch IRQ issues early
and simplify remoteproc start/stop.

Signed-off-by: Andrew Davis <a...@ti.com>
---
  drivers/remoteproc/keystone_remoteproc.c | 41 +++++++++---------------
  1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/remoteproc/keystone_remoteproc.c 
b/drivers/remoteproc/keystone_remoteproc.c
index 4d6550b485675..e7fde55097866 100644
--- a/drivers/remoteproc/keystone_remoteproc.c
+++ b/drivers/remoteproc/keystone_remoteproc.c
@@ -173,35 +173,16 @@ static int keystone_rproc_start(struct rproc *rproc)

INIT_WORK(&ksproc->workqueue, handle_event); - ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,

-                         dev_name(ksproc->dev), ksproc);
-       if (ret) {
-               dev_err(ksproc->dev, "failed to enable vring interrupt, ret = 
%d\n",
-                       ret);
-               goto out;
-       }
+       enable_irq(ksproc->irq_ring);
+       enable_irq(ksproc->irq_fault);

- ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,

-                         0, dev_name(ksproc->dev), ksproc);
+       ret = keystone_rproc_dsp_boot(ksproc, rproc->bootaddr);
        if (ret) {
-               dev_err(ksproc->dev, "failed to enable exception interrupt, ret = 
%d\n",
-                       ret);
-               goto free_vring_irq;
+               flush_work(&ksproc->workqueue);
+               return ret;
        }

- ret = keystone_rproc_dsp_boot(ksproc, rproc->bootaddr);

-       if (ret)
-               goto free_exc_irq;
-
        return 0;
-
-free_exc_irq:
-       free_irq(ksproc->irq_fault, ksproc);
-free_vring_irq:
-       free_irq(ksproc->irq_ring, ksproc);
-       flush_work(&ksproc->workqueue);
-out:
-       return ret;
  }

/*

@@ -215,8 +196,8 @@ static int keystone_rproc_stop(struct rproc *rproc)
        struct keystone_rproc *ksproc = rproc->priv;

keystone_rproc_dsp_reset(ksproc);

-       free_irq(ksproc->irq_fault, ksproc);
-       free_irq(ksproc->irq_ring, ksproc);
+       disable_irq(ksproc->irq_fault);
+       disable_irq(ksproc->irq_ring);
        flush_work(&ksproc->workqueue);

return 0;

@@ -427,10 +408,18 @@ static int keystone_rproc_probe(struct platform_device 
*pdev)
        ksproc->irq_ring = platform_get_irq_byname(pdev, "vring");
        if (ksproc->irq_ring < 0)
                return ksproc->irq_ring;
+       ret = devm_request_irq(dev, ksproc->irq_ring, 
keystone_rproc_vring_interrupt,
+                              IRQF_NO_AUTOEN, dev_name(dev), ksproc);
+       if (ret)
+               return dev_err_probe(dev, ret, "failed to request vring 
interrupt\n");

ksproc->irq_fault = platform_get_irq_byname(pdev, "exception");

        if (ksproc->irq_fault < 0)
                return ksproc->irq_fault;
+       ret = devm_request_irq(dev, ksproc->irq_fault, 
keystone_rproc_exception_interrupt,
+                              IRQF_NO_AUTOEN, dev_name(dev), ksproc);

request_irq() sets irqflags IRQF_COND_ONESHOT, something that is not done here.
Are you sure this is what you want?

devm_request_irq() looks to also set IRQF_COND_ONESHOT matching request_irq().

Not sure it would matter anyway as these IRQs are not shared, behavior would
be unchanged.

Andrew

Thanks,
Mathieu

+       if (ret)
+               return dev_err_probe(dev, ret, "failed to enable exception 
interrupt\n");

ksproc->kick_gpio = devm_gpiod_get(dev, "kick", GPIOD_ASIS);

        ret = PTR_ERR_OR_ZERO(ksproc->kick_gpio);
--
2.39.2