From: Bhaumik Bhatt <bbhatt@codeaurora.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: linux-arm-msm@vger.kernel.org, hemantk@codeaurora.org,
jhugo@codeaurora.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 10/12] bus: mhi: core: Separate system error and power down handling
Date: Fri, 30 Oct 2020 12:34:07 -0700 [thread overview]
Message-ID: <a02c31409d696075b155ef2d6ee33009@codeaurora.org> (raw)
In-Reply-To: <20201030140656.GL3818@Mani-XPS-13-9360>
Hi Mani,
On 2020-10-30 07:06, Manivannan Sadhasivam wrote:
> On Thu, Oct 29, 2020 at 09:10:55PM -0700, Bhaumik Bhatt wrote:
>> Currently, there exist a set of if...else statements in the
>> mhi_pm_disable_transition() function which make handling system
>> error and disable transitions differently complex. To make that
>> cleaner and facilitate differences in behavior, separate these
>> two transitions for MHI host.
>>
>
> And this results in a lot of duplicated code :/
>
> Thanks,
> Mani
>
I knew this was coming. Mainly, we can avoid adding confusing if...else
statements that plague the current mhi_pm_disable_transition() function
and in
return for some duplicate code, we can make handling separate use cases
easier
as they could pop-up anytime in the future.
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>> drivers/bus/mhi/core/pm.c | 159
>> +++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 137 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 1d04e401..347ae7d 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -444,7 +444,7 @@ static int mhi_pm_mission_mode_transition(struct
>> mhi_controller *mhi_cntrl)
>> return ret;
>> }
>>
>> -/* Handle SYS_ERR and Shutdown transitions */
>> +/* Handle shutdown transitions */
>> static void mhi_pm_disable_transition(struct mhi_controller
>> *mhi_cntrl,
>> enum mhi_pm_state transition_state)
>> {
>> @@ -460,10 +460,6 @@ static void mhi_pm_disable_transition(struct
>> mhi_controller *mhi_cntrl,
>> to_mhi_pm_state_str(mhi_cntrl->pm_state),
>> to_mhi_pm_state_str(transition_state));
>>
>> - /* We must notify MHI control driver so it can clean up first */
>> - if (transition_state == MHI_PM_SYS_ERR_PROCESS)
>> - mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_SYS_ERROR);
>> -
>> mutex_lock(&mhi_cntrl->pm_mutex);
>> write_lock_irq(&mhi_cntrl->pm_lock);
>> prev_state = mhi_cntrl->pm_state;
>> @@ -502,11 +498,8 @@ static void mhi_pm_disable_transition(struct
>> mhi_controller *mhi_cntrl,
>> MHICTRL_RESET_SHIFT,
>> &in_reset) ||
>> !in_reset, timeout);
>> - if ((!ret || in_reset) && cur_state == MHI_PM_SYS_ERR_PROCESS) {
>> + if (!ret || in_reset)
>> dev_err(dev, "Device failed to exit MHI Reset state\n");
>> - mutex_unlock(&mhi_cntrl->pm_mutex);
>> - return;
>> - }
>>
>> /*
>> * Device will clear BHI_INTVEC as a part of RESET processing,
>> @@ -566,19 +559,142 @@ static void mhi_pm_disable_transition(struct
>> mhi_controller *mhi_cntrl,
>> er_ctxt->wp = er_ctxt->rbase;
>> }
>>
>> - if (cur_state == MHI_PM_SYS_ERR_PROCESS) {
>> - mhi_ready_state_transition(mhi_cntrl);
>> - } else {
>> - /* Move to disable state */
>> - write_lock_irq(&mhi_cntrl->pm_lock);
>> - cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_DISABLE);
>> - write_unlock_irq(&mhi_cntrl->pm_lock);
>> - if (unlikely(cur_state != MHI_PM_DISABLE))
>> - dev_err(dev, "Error moving from PM state: %s to: %s\n",
>> - to_mhi_pm_state_str(cur_state),
>> - to_mhi_pm_state_str(MHI_PM_DISABLE));
>> + /* Move to disable state */
>> + write_lock_irq(&mhi_cntrl->pm_lock);
>> + cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_DISABLE);
>> + write_unlock_irq(&mhi_cntrl->pm_lock);
>> + if (unlikely(cur_state != MHI_PM_DISABLE))
>> + dev_err(dev, "Error moving from PM state: %s to: %s\n",
>> + to_mhi_pm_state_str(cur_state),
>> + to_mhi_pm_state_str(MHI_PM_DISABLE));
>> +
>> + dev_dbg(dev, "Exiting with PM state: %s, MHI state: %s\n",
>> + to_mhi_pm_state_str(mhi_cntrl->pm_state),
>> + TO_MHI_STATE_STR(mhi_cntrl->dev_state));
>> +
>> + mutex_unlock(&mhi_cntrl->pm_mutex);
>> +}
>> +
>> +/* Handle system error transitions */
>> +static void mhi_pm_sys_error_transition(struct mhi_controller
>> *mhi_cntrl)
>> +{
>> + enum mhi_pm_state cur_state, prev_state;
>> + struct mhi_event *mhi_event;
>> + struct mhi_cmd_ctxt *cmd_ctxt;
>> + struct mhi_cmd *mhi_cmd;
>> + struct mhi_event_ctxt *er_ctxt;
>> + struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> + int ret, i;
>> +
>> + dev_dbg(dev, "Transitioning from PM state: %s to: %s\n",
>> + to_mhi_pm_state_str(mhi_cntrl->pm_state),
>> + to_mhi_pm_state_str(MHI_PM_SYS_ERR_PROCESS));
>> +
>> + /* We must notify MHI control driver so it can clean up first */
>> + mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_SYS_ERROR);
>> +
>> + mutex_lock(&mhi_cntrl->pm_mutex);
>> + write_lock_irq(&mhi_cntrl->pm_lock);
>> + prev_state = mhi_cntrl->pm_state;
>> + cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
>> + write_unlock_irq(&mhi_cntrl->pm_lock);
>> +
>> + if (cur_state != MHI_PM_SYS_ERR_PROCESS) {
>> + dev_err(dev, "Failed to transition from PM state: %s to: %s\n",
>> + to_mhi_pm_state_str(cur_state),
>> + to_mhi_pm_state_str(MHI_PM_SYS_ERR_PROCESS));
>> + goto exit_sys_error_transition;
>> + }
>> +
>> + mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
>> + mhi_cntrl->dev_state = MHI_STATE_RESET;
>> +
>> + /* Wake up threads waiting for state transition */
>> + wake_up_all(&mhi_cntrl->state_event);
>> +
>> + /* Trigger MHI RESET so that the device will not access host memory
>> */
>> + if (MHI_REG_ACCESS_VALID(prev_state)) {
>> + u32 in_reset = -1;
>> + unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms);
>> +
>> + dev_dbg(dev, "Triggering MHI Reset in device\n");
>> + mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
>> +
>> + /* Wait for the reset bit to be cleared by the device */
>> + ret = wait_event_timeout(mhi_cntrl->state_event,
>> + mhi_read_reg_field(mhi_cntrl,
>> + mhi_cntrl->regs,
>> + MHICTRL,
>> + MHICTRL_RESET_MASK,
>> + MHICTRL_RESET_SHIFT,
>> + &in_reset) ||
>> + !in_reset, timeout);
>> + if (!ret || in_reset) {
>> + dev_err(dev, "Device failed to exit MHI Reset state\n");
>> + goto exit_sys_error_transition;
>> + }
>> +
>> + /*
>> + * Device will clear BHI_INTVEC as a part of RESET processing,
>> + * hence re-program it
>> + */
>> + mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
>> + }
>> +
>> + dev_dbg(dev,
>> + "Waiting for all pending event ring processing to complete\n");
>> + mhi_event = mhi_cntrl->mhi_event;
>> + for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>> + if (mhi_event->offload_ev)
>> + continue;
>> + tasklet_kill(&mhi_event->task);
>> }
>>
>> + /* Release lock and wait for all pending threads to complete */
>> + mutex_unlock(&mhi_cntrl->pm_mutex);
>> + dev_dbg(dev, "Waiting for all pending threads to complete\n");
>> + wake_up_all(&mhi_cntrl->state_event);
>> +
>> + dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
>> + device_for_each_child(mhi_cntrl->cntrl_dev, NULL,
>> mhi_destroy_device);
>> +
>> + mutex_lock(&mhi_cntrl->pm_mutex);
>> +
>> + WARN_ON(atomic_read(&mhi_cntrl->dev_wake));
>> + WARN_ON(atomic_read(&mhi_cntrl->pending_pkts));
>> +
>> + /* Reset the ev rings and cmd rings */
>> + dev_dbg(dev, "Resetting EV CTXT and CMD CTXT\n");
>> + mhi_cmd = mhi_cntrl->mhi_cmd;
>> + cmd_ctxt = mhi_cntrl->mhi_ctxt->cmd_ctxt;
>> + for (i = 0; i < NR_OF_CMD_RINGS; i++, mhi_cmd++, cmd_ctxt++) {
>> + struct mhi_ring *ring = &mhi_cmd->ring;
>> +
>> + ring->rp = ring->base;
>> + ring->wp = ring->base;
>> + cmd_ctxt->rp = cmd_ctxt->rbase;
>> + cmd_ctxt->wp = cmd_ctxt->rbase;
>> + }
>> +
>> + mhi_event = mhi_cntrl->mhi_event;
>> + er_ctxt = mhi_cntrl->mhi_ctxt->er_ctxt;
>> + for (i = 0; i < mhi_cntrl->total_ev_rings; i++, er_ctxt++,
>> + mhi_event++) {
>> + struct mhi_ring *ring = &mhi_event->ring;
>> +
>> + /* Skip offload events */
>> + if (mhi_event->offload_ev)
>> + continue;
>> +
>> + ring->rp = ring->base;
>> + ring->wp = ring->base;
>> + er_ctxt->rp = er_ctxt->rbase;
>> + er_ctxt->wp = er_ctxt->rbase;
>> + }
>> +
>> + mhi_ready_state_transition(mhi_cntrl);
>> +
>> +exit_sys_error_transition:
>> dev_dbg(dev, "Exiting with PM state: %s, MHI state: %s\n",
>> to_mhi_pm_state_str(mhi_cntrl->pm_state),
>> TO_MHI_STATE_STR(mhi_cntrl->dev_state));
>> @@ -666,8 +782,7 @@ void mhi_pm_st_worker(struct work_struct *work)
>> mhi_ready_state_transition(mhi_cntrl);
>> break;
>> case DEV_ST_TRANSITION_SYS_ERR:
>> - mhi_pm_disable_transition
>> - (mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
>> + mhi_pm_sys_error_transition(mhi_cntrl);
>> break;
>> case DEV_ST_TRANSITION_DISABLE:
>> mhi_pm_disable_transition
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2020-10-30 19:35 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-30 4:10 [PATCH v3 00/12] Bug fixes and improvements for MHI power operations Bhaumik Bhatt
2020-10-30 4:10 ` [PATCH v3 01/12] bus: mhi: core: Use appropriate names for firmware load functions Bhaumik Bhatt
2020-10-30 13:29 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 02/12] bus: mhi: core: Move to using high priority workqueue Bhaumik Bhatt
2020-10-30 13:35 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 03/12] bus: mhi: core: Skip device wake in error or shutdown states Bhaumik Bhatt
2020-10-30 4:10 ` [PATCH v3 04/12] bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability Bhaumik Bhatt
2020-10-30 13:52 ` Manivannan Sadhasivam
2020-10-30 19:29 ` Bhaumik Bhatt
2020-10-30 4:10 ` [PATCH v3 05/12] bus: mhi: core: Prevent sending multiple RDDM entry callbacks Bhaumik Bhatt
2020-10-30 13:56 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 06/12] bus: mhi: core: Move to an error state on any firmware load failure Bhaumik Bhatt
2020-10-30 14:00 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 07/12] bus: mhi: core: Use appropriate label in firmware load handler API Bhaumik Bhatt
2020-10-30 14:00 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 08/12] bus: mhi: core: Move to an error state on mission mode failure Bhaumik Bhatt
2020-10-30 4:10 ` [PATCH v3 09/12] bus: mhi: core: Check for IRQ availability during registration Bhaumik Bhatt
2020-10-30 14:02 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 10/12] bus: mhi: core: Separate system error and power down handling Bhaumik Bhatt
2020-10-30 14:06 ` Manivannan Sadhasivam
2020-10-30 19:34 ` Bhaumik Bhatt [this message]
2020-10-31 6:54 ` Manivannan Sadhasivam
2020-11-02 16:52 ` Bhaumik Bhatt
2020-10-30 4:10 ` [PATCH v3 11/12] bus: mhi: core: Mark and maintain device states early on after power down Bhaumik Bhatt
2020-10-30 14:10 ` Manivannan Sadhasivam
2020-10-30 4:10 ` [PATCH v3 12/12] bus: mhi: core: Remove MHI event ring IRQ handlers when powering down Bhaumik Bhatt
2020-10-30 14:11 ` Manivannan Sadhasivam
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=a02c31409d696075b155ef2d6ee33009@codeaurora.org \
--to=bbhatt@codeaurora.org \
--cc=hemantk@codeaurora.org \
--cc=jhugo@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.