| Message ID | 1600414128-5510-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Accepted |
| Commit | b0fc0167f25401a5dd0325a935e3a20d1da2aa0b |
| Headers | show |
| Series | bus: mhi: core: Allow shared IRQ for event rings | expand |
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'
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 --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) {
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(-)