[PATCH v2 1/2] bus: mhi: core: Allow shared IRQ for event rings

[PATCH v2 1/2] bus: mhi: core: Allow shared IRQ for event rings

[Loic Poulain]

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>
---
 v2: keep nr_irqs check early in init sequence

 drivers/bus/mhi/core/init.c | 7 +++++++
 drivers/bus/mhi/core/pm.c   | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 19ade8a..31961af 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -125,6 +125,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..3de7b16 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -918,7 +918,7 @@ 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)
+	if (mhi_cntrl->nr_irqs < 1)
 		return -EINVAL;
 
 	/* Supply default wake routines if not provided by controller driver */
-- 
2.7.4

[PATCH v2 2/2] bus: mhi: Remove unused nr_irqs_req variable

[Loic Poulain]

nr_irqs_req is unused in MHI stack.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 v2: add this patch to the series

 drivers/bus/mhi/core/init.c | 3 ---
 include/linux/mhi.h         | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 31961af..8798deb 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -643,9 +643,6 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
 		mhi_event++;
 	}
 
-	/* We need IRQ for each event ring + additional one for BHI */
-	mhi_cntrl->nr_irqs_req = mhi_cntrl->total_ev_rings + 1;
-
 	return 0;
 
 error_ev_cfg:
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 742dabe..0d277c7 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -309,7 +309,6 @@ struct mhi_controller_config {
  * @total_ev_rings: Total # of event rings allocated
  * @hw_ev_rings: Number of hardware event rings
  * @sw_ev_rings: Number of software event rings
- * @nr_irqs_req: Number of IRQs required to operate (optional)
  * @nr_irqs: Number of IRQ allocated by bus master (required)
  * @family_number: MHI controller family number
  * @device_number: MHI controller device number
@@ -389,7 +388,6 @@ struct mhi_controller {
 	u32 total_ev_rings;
 	u32 hw_ev_rings;
 	u32 sw_ev_rings;
-	u32 nr_irqs_req;
 	u32 nr_irqs;
 	u32 family_number;
 	u32 device_number;
-- 
2.7.4

Re: [PATCH v2 1/2] bus: mhi: core: Allow shared IRQ for event rings

[Manivannan Sadhasivam]

On Mon, Sep 21, 2020 at 09:36:58AM +0200, 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  v2: keep nr_irqs check early in init sequence
> 
>  drivers/bus/mhi/core/init.c | 7 +++++++
>  drivers/bus/mhi/core/pm.c   | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 19ade8a..31961af 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -125,6 +125,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..3de7b16 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -918,7 +918,7 @@ 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)
> +	if (mhi_cntrl->nr_irqs < 1)
>  		return -EINVAL;
>  
>  	/* Supply default wake routines if not provided by controller driver */
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/2] bus: mhi: Remove unused nr_irqs_req variable

[Manivannan Sadhasivam]

On Mon, Sep 21, 2020 at 09:36:59AM +0200, Loic Poulain wrote:
> nr_irqs_req is unused in MHI stack.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  v2: add this patch to the series
> 
>  drivers/bus/mhi/core/init.c | 3 ---
>  include/linux/mhi.h         | 2 --
>  2 files changed, 5 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 31961af..8798deb 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -643,9 +643,6 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
>  		mhi_event++;
>  	}
>  
> -	/* We need IRQ for each event ring + additional one for BHI */
> -	mhi_cntrl->nr_irqs_req = mhi_cntrl->total_ev_rings + 1;
> -
>  	return 0;
>  
>  error_ev_cfg:
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 742dabe..0d277c7 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -309,7 +309,6 @@ struct mhi_controller_config {
>   * @total_ev_rings: Total # of event rings allocated
>   * @hw_ev_rings: Number of hardware event rings
>   * @sw_ev_rings: Number of software event rings
> - * @nr_irqs_req: Number of IRQs required to operate (optional)
>   * @nr_irqs: Number of IRQ allocated by bus master (required)
>   * @family_number: MHI controller family number
>   * @device_number: MHI controller device number
> @@ -389,7 +388,6 @@ struct mhi_controller {
>  	u32 total_ev_rings;
>  	u32 hw_ev_rings;
>  	u32 sw_ev_rings;
> -	u32 nr_irqs_req;
>  	u32 nr_irqs;
>  	u32 family_number;
>  	u32 device_number;
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/2] bus: mhi: Remove unused nr_irqs_req variable

[Manivannan Sadhasivam]

On Mon, Sep 21, 2020 at 09:36:59AM +0200, Loic Poulain wrote:
> nr_irqs_req is unused in MHI stack.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Applied to mhi-next!

Thanks,
Mani

> ---
>  v2: add this patch to the series
> 
>  drivers/bus/mhi/core/init.c | 3 ---
>  include/linux/mhi.h         | 2 --
>  2 files changed, 5 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 31961af..8798deb 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -643,9 +643,6 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
>  		mhi_event++;
>  	}
>  
> -	/* We need IRQ for each event ring + additional one for BHI */
> -	mhi_cntrl->nr_irqs_req = mhi_cntrl->total_ev_rings + 1;
> -
>  	return 0;
>  
>  error_ev_cfg:
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 742dabe..0d277c7 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -309,7 +309,6 @@ struct mhi_controller_config {
>   * @total_ev_rings: Total # of event rings allocated
>   * @hw_ev_rings: Number of hardware event rings
>   * @sw_ev_rings: Number of software event rings
> - * @nr_irqs_req: Number of IRQs required to operate (optional)
>   * @nr_irqs: Number of IRQ allocated by bus master (required)
>   * @family_number: MHI controller family number
>   * @device_number: MHI controller device number
> @@ -389,7 +388,6 @@ struct mhi_controller {
>  	u32 total_ev_rings;
>  	u32 hw_ev_rings;
>  	u32 sw_ev_rings;
> -	u32 nr_irqs_req;
>  	u32 nr_irqs;
>  	u32 family_number;
>  	u32 device_number;
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] bus: mhi: core: Allow shared IRQ for event rings

[Manivannan Sadhasivam]

On Mon, Sep 21, 2020 at 09:36:58AM +0200, 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>

Applied to mhi-next!

Thanks,
Mani

> ---
>  v2: keep nr_irqs check early in init sequence
> 
>  drivers/bus/mhi/core/init.c | 7 +++++++
>  drivers/bus/mhi/core/pm.c   | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 19ade8a..31961af 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -125,6 +125,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..3de7b16 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -918,7 +918,7 @@ 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)
> +	if (mhi_cntrl->nr_irqs < 1)
>  		return -EINVAL;
>  
>  	/* Supply default wake routines if not provided by controller driver */
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] bus: mhi: core: Allow shared IRQ for event rings

[bbhatt]

On 2020-09-21 00:36, 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>
> ---
>  v2: keep nr_irqs check early in init sequence
> 
>  drivers/bus/mhi/core/init.c | 7 +++++++
>  drivers/bus/mhi/core/pm.c   | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 19ade8a..31961af 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -125,6 +125,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..3de7b16 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -918,7 +918,7 @@ 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)
> +	if (mhi_cntrl->nr_irqs < 1)
>  		return -EINVAL;
> 
>  	/* Supply default wake routines if not provided by controller driver 
> */
It would be better if we can remove this check altogether from the 
mhi_async_power_up()
function and instead place it as one of the checks in 
mhi_register_controller() in init.c.

That way, we don't have to wait until power up attempt to determine 
whether the provided
controller configuration is acceptable and can bail out early from the
mhi_register_controller() function itself.

if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
     !mhi_cntrl->status_cb || !mhi_cntrl->read_reg ||
-    !mhi_cntrl->write_reg)
+    !mhi_cntrl->write_reg || !mhi_cntrl->nr_irqs)

Thanks,
Bhaumik