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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Fri, Mar 06, 2026 at 11:29:09AM -0600, Andrew Davis wrote:
> 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 <afd@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().
>

You are correct - I have applied this patch.
 
> 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
> > > 
>