mhi.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Manivannan Sadhasivam <mani@kernel.org>
To: Qiang Yu <quic_qianyu@quicinc.com>
Cc: quic_hemantk@quicinc.com, loic.poulain@linaro.org,
	quic_jhugo@quicinc.com, mhi@lists.linux.dev,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	quic_cang@quicinc.com
Subject: Re: [PATCH v3 1/1] bus: mhi: host: Move IRQ allocation to controller registration phase
Date: Wed, 22 Jun 2022 13:48:51 +0530	[thread overview]
Message-ID: <20220622081851.GB6263@thinkpad> (raw)
In-Reply-To: <1655868106-79456-1-git-send-email-quic_qianyu@quicinc.com>

On Wed, Jun 22, 2022 at 11:21:46AM +0800, Qiang Yu wrote:
> During runtime, the MHI endpoint may be powered up/down several times.
> So instead of allocating and destroying the IRQs all the time, let's just
> enable/disable IRQs during power up/down.
> 
> The IRQs will be allocated during mhi_register_controller() and freed
> during mhi_unregister_controller(). This works well for things like PCI
> hotplug also as once the PCI device gets removed, the controller will
> get unregistered. And once it comes back, it will get registered back
> and even if the IRQ configuration changes (MSI), that will get accounted.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>

One comment below. With that addressed,

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

> ---
> v2->v3: change commit text and comments. 
> v1->v2: Rewrite commit text. Remove a random change. Use
>         inline enables.
> 
>  drivers/bus/mhi/host/init.c | 15 +++++++++++++++
>  drivers/bus/mhi/host/pm.c   | 19 +++++++++++++------
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index cbb86b2..e2acf66 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -179,6 +179,12 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
>  				   "bhi", mhi_cntrl);
>  	if (ret)
>  		return ret;
> +	/*
> +	 * IRQs should be enabled during mhi_async_power_up(), so disable them explicitly here.
> +	 * Due to the use of IRQF_SHARED flag as default while requesting IRQs, we assume that
> +	 * IRQ_NOAUTOEN is not applicable.
> +	 */
> +	disable_irq(mhi_cntrl->irq[0]);
>  
>  	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>  		if (mhi_event->offload_ev)
> @@ -200,6 +206,8 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
>  				mhi_cntrl->irq[mhi_event->irq], i);
>  			goto error_request;
>  		}
> +
> +		disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>  	}
>  
>  	return 0;
> @@ -1003,8 +1011,14 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  
>  	mhi_create_debugfs(mhi_cntrl);
>  
> +	ret = mhi_init_irq_setup(mhi_cntrl);

This can be moved above mhi_alloc_device() to fail early if there is any problem
with IRQ allocation.

Thanks,
Mani

> +	if (ret)
> +		goto error_setup_irq;
> +
>  	return 0;
>  
> +error_setup_irq:
> +	mhi_destroy_debugfs(mhi_cntrl);
>  err_release_dev:
>  	put_device(&mhi_dev->dev);
>  err_ida_free:
> @@ -1027,6 +1041,7 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>  	struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>  	unsigned int i;
>  
> +	mhi_deinit_free_irq(mhi_cntrl);
>  	mhi_destroy_debugfs(mhi_cntrl);
>  
>  	destroy_workqueue(mhi_cntrl->hiprio_wq);
> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> index dc2e8ff..4a42186 100644
> --- a/drivers/bus/mhi/host/pm.c
> +++ b/drivers/bus/mhi/host/pm.c
> @@ -500,7 +500,7 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
>  	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>  		if (mhi_event->offload_ev)
>  			continue;
> -		free_irq(mhi_cntrl->irq[mhi_event->irq], mhi_event);
> +		disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>  		tasklet_kill(&mhi_event->task);
>  	}
>  
> @@ -1060,12 +1060,13 @@ static void mhi_deassert_dev_wake(struct mhi_controller *mhi_cntrl,
>  
>  int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>  {
> +	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
>  	enum mhi_state state;
>  	enum mhi_ee_type current_ee;
>  	enum dev_st_transition next_state;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	u32 interval_us = 25000; /* poll register field every 25 milliseconds */
> -	int ret;
> +	int ret, i;
>  
>  	dev_info(dev, "Requested to power ON\n");
>  
> @@ -1117,9 +1118,15 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>  		mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
>  	}
>  
> -	ret = mhi_init_irq_setup(mhi_cntrl);
> -	if (ret)
> -		goto error_exit;
> +	/* IRQs have been requested during probe, so we just need to enable them. */
> +	enable_irq(mhi_cntrl->irq[0]);
> +
> +	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
> +		if (mhi_event->offload_ev)
> +			continue;
> +
> +		enable_irq(mhi_cntrl->irq[mhi_event->irq]);
> +	}
>  
>  	/* Transition to next state */
>  	next_state = MHI_IN_PBL(current_ee) ?
> @@ -1182,7 +1189,7 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  	/* Wait for shutdown to complete */
>  	flush_work(&mhi_cntrl->st_worker);
>  
> -	free_irq(mhi_cntrl->irq[0], mhi_cntrl);
> +	disable_irq(mhi_cntrl->irq[0]);
>  }
>  EXPORT_SYMBOL_GPL(mhi_power_down);
>  
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

-- 
மணிவண்ணன் சதாசிவம்

      reply	other threads:[~2022-06-22  8:19 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-22  3:21 [PATCH v3 1/1] bus: mhi: host: Move IRQ allocation to controller registration phase Qiang Yu
2022-06-22  8:18 ` Manivannan Sadhasivam [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220622081851.GB6263@thinkpad \
    --to=mani@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=mhi@lists.linux.dev \
    --cc=quic_cang@quicinc.com \
    --cc=quic_hemantk@quicinc.com \
    --cc=quic_jhugo@quicinc.com \
    --cc=quic_qianyu@quicinc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).