diff mbox series

bus: mhi: core: Allow shared IRQ for event rings

Message ID 1600414128-5510-1-git-send-email-loic.poulain@linaro.org (mailing list archive)
State Superseded
Headers show
Series bus: mhi: core: Allow shared IRQ for event rings | expand

Commit Message

Loic Poulain Sept. 18, 2020, 7:28 a.m. UTC
There is no requirement for using a dedicated IRQ per event ring.
Some systems does not support multiple MSI vectors (e.g. intel
without CONFIG_IRQ_REMAP), In that case the MHI controller can
configure all the event rings to use the same interrupt (as fallback).

Allow this by removing the nr_irqs = ev_ring test and add extra check
in the irq_setup function.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/core/init.c | 10 ++++++++++
 drivers/bus/mhi/core/pm.c   |  3 ---
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Bhaumik Bhatt Sept. 19, 2020, 2:43 a.m. UTC | #1
On 2020-09-18 00:28, Loic Poulain wrote:
> There is no requirement for using a dedicated IRQ per event ring.
> Some systems does not support multiple MSI vectors (e.g. intel
> without CONFIG_IRQ_REMAP), In that case the MHI controller can
> configure all the event rings to use the same interrupt (as fallback).
> 
> Allow this by removing the nr_irqs = ev_ring test and add extra check
> in the irq_setup function.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/bus/mhi/core/init.c | 10 ++++++++++
>  drivers/bus/mhi/core/pm.c   |  3 ---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index d232938..ac19067 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -113,6 +113,9 @@ int mhi_init_irq_setup(struct mhi_controller 
> *mhi_cntrl)
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	int i, ret;
> 
> +	if (mhi_cntrl->nr_irqs < 1)
> +		return -EINVAL;
> +

It would be better to move this check earlier in 
mhi_register_controller() because if
the resource is not available, we do not have to proceed to even allow 
power up.

>  	/* Setup BHI_INTVEC IRQ */
>  	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
>  				   mhi_intvec_threaded_handler,
> @@ -125,6 +128,13 @@ int mhi_init_irq_setup(struct mhi_controller 
> *mhi_cntrl)
>  		if (mhi_event->offload_ev)
>  			continue;
> 
> +		if (mhi_event->irq >= mhi_cntrl->nr_irqs) {
> +			dev_err(dev, "irq %d not available for event ring\n",
> +				mhi_event->irq);
> +			ret = -EINVAL;
> +			goto error_request;
> +		}
> +
>  		ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
>  				  mhi_irq_handler,
>  				  IRQF_SHARED | IRQF_NO_SUSPEND,
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index ce4d969..07efdbc 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -918,9 +918,6 @@ int mhi_async_power_up(struct mhi_controller 
> *mhi_cntrl)
> 
>  	dev_info(dev, "Requested to power ON\n");
> 
> -	if (mhi_cntrl->nr_irqs < mhi_cntrl->total_ev_rings)
> -		return -EINVAL;
> -
>  	/* Supply default wake routines if not provided by controller driver 
> */
>  	if (!mhi_cntrl->wake_get || !mhi_cntrl->wake_put ||
>  	    !mhi_cntrl->wake_toggle) {

Maybe another clean-up patch is also good to remove usage of 
"mhi_cntrl->nr_irqs_req"
as it is deemed optional anyway and is unused in the driver.

Thanks,
Bhaumik

'The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project'
Loic Poulain Sept. 21, 2020, 7:07 a.m. UTC | #2
Hi Bhaumik,

On Sat, 19 Sep 2020 at 04:43, <bbhatt@codeaurora.org> wrote:
>
> On 2020-09-18 00:28, Loic Poulain wrote:
> > There is no requirement for using a dedicated IRQ per event ring.
> > Some systems does not support multiple MSI vectors (e.g. intel
> > without CONFIG_IRQ_REMAP), In that case the MHI controller can
> > configure all the event rings to use the same interrupt (as fallback).
> >
> > Allow this by removing the nr_irqs = ev_ring test and add extra check
> > in the irq_setup function.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> >  drivers/bus/mhi/core/init.c | 10 ++++++++++
> >  drivers/bus/mhi/core/pm.c   |  3 ---
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index d232938..ac19067 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -113,6 +113,9 @@ int mhi_init_irq_setup(struct mhi_controller
> > *mhi_cntrl)
> >       struct device *dev = &mhi_cntrl->mhi_dev->dev;
> >       int i, ret;
> >
> > +     if (mhi_cntrl->nr_irqs < 1)
> > +             return -EINVAL;
> > +
>
> It would be better to move this check earlier in
> mhi_register_controller() because if
> the resource is not available, we do not have to proceed to even allow
> power up.


Ok, will do in V2.

>
>
> >       /* Setup BHI_INTVEC IRQ */
> >       ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
> >                                  mhi_intvec_threaded_handler,
> > @@ -125,6 +128,13 @@ int mhi_init_irq_setup(struct mhi_controller
> > *mhi_cntrl)
> >               if (mhi_event->offload_ev)
> >                       continue;
> >
> > +             if (mhi_event->irq >= mhi_cntrl->nr_irqs) {
> > +                     dev_err(dev, "irq %d not available for event ring\n",
> > +                             mhi_event->irq);
> > +                     ret = -EINVAL;
> > +                     goto error_request;
> > +             }
> > +
> >               ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
> >                                 mhi_irq_handler,
> >                                 IRQF_SHARED | IRQF_NO_SUSPEND,
> > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > index ce4d969..07efdbc 100644
> > --- a/drivers/bus/mhi/core/pm.c
> > +++ b/drivers/bus/mhi/core/pm.c
> > @@ -918,9 +918,6 @@ int mhi_async_power_up(struct mhi_controller
> > *mhi_cntrl)
> >
> >       dev_info(dev, "Requested to power ON\n");
> >
> > -     if (mhi_cntrl->nr_irqs < mhi_cntrl->total_ev_rings)
> > -             return -EINVAL;
> > -
> >       /* Supply default wake routines if not provided by controller driver
> > */
> >       if (!mhi_cntrl->wake_get || !mhi_cntrl->wake_put ||
> >           !mhi_cntrl->wake_toggle) {
>
> Maybe another clean-up patch is also good to remove usage of
> "mhi_cntrl->nr_irqs_req"
> as it is deemed optional anyway and is unused in the driver.


OK, I'll submit an additional patch for that.

Regards,
Loic
diff mbox series

Patch

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index d232938..ac19067 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -113,6 +113,9 @@  int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	int i, ret;
 
+	if (mhi_cntrl->nr_irqs < 1)
+		return -EINVAL;
+
 	/* Setup BHI_INTVEC IRQ */
 	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
 				   mhi_intvec_threaded_handler,
@@ -125,6 +128,13 @@  int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
 		if (mhi_event->offload_ev)
 			continue;
 
+		if (mhi_event->irq >= mhi_cntrl->nr_irqs) {
+			dev_err(dev, "irq %d not available for event ring\n",
+				mhi_event->irq);
+			ret = -EINVAL;
+			goto error_request;
+		}
+
 		ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
 				  mhi_irq_handler,
 				  IRQF_SHARED | IRQF_NO_SUSPEND,
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index ce4d969..07efdbc 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -918,9 +918,6 @@  int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
 
 	dev_info(dev, "Requested to power ON\n");
 
-	if (mhi_cntrl->nr_irqs < mhi_cntrl->total_ev_rings)
-		return -EINVAL;
-
 	/* Supply default wake routines if not provided by controller driver */
 	if (!mhi_cntrl->wake_get || !mhi_cntrl->wake_put ||
 	    !mhi_cntrl->wake_toggle) {