| Message ID | 1604031057-32820-11-git-send-email-bbhatt@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bug fixes and improvements for MHI power operations | expand |
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 > 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 >
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
Hi Bhaumik, On 31 October 2020 1:04:07 AM IST, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote: >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. > If that happens then do it but now, please no. Thanks, Mani >>> 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
Hi Mani, On 2020-10-30 23:54, Manivannan Sadhasivam wrote: > Hi Bhaumik, > > On 31 October 2020 1:04:07 AM IST, Bhaumik Bhatt > <bbhatt@codeaurora.org> wrote: >> 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. >> > > If that happens then do it but now, please no. > > Thanks, > Mani > It had to be done for 11/12 and 12/12 (last two) patches of the series. It's a much cleaner approach and allows us to handle states better. We should be moving the dev_state and EE to "Reset" and "Disable" states soon enough when a shutdown is initiated and we can resolve bugs such as freeing the IRQs for a shutdown sooner as well. Since these differences in shutdown versus SYS_ERROR processing are already increasingly apparent and more could come, this patch had to step in. bus: mhi: core: Mark and maintain device states early on after power down bus: mhi: core: Remove MHI event ring IRQ handlers when powering down >>>> 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 Thanks, Bhaumik
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
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. Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> --- drivers/bus/mhi/core/pm.c | 159 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 137 insertions(+), 22 deletions(-)