Re: [PATCH v2 1/5] bus: mhi: core: Handle syserr during power_up

[Jeffrey Hugo]

On 4/9/2020 6:55 PM, Hemant Kumar wrote:
> On 4/7/20 9:50 AM, Jeffrey Hugo wrote:
> > The MHI device may be in the syserr state when we attempt to init it in
> > power_up(). Since we have no local state, the handling is simple -
> > reset the device and wait for it to transition out of the reset state.
> >
> > Signed-off-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
> > ---
> > Â drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
> > Â 1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > index 52690cb..3285c9e 100644
> > --- a/drivers/bus/mhi/core/pm.c
> > +++ b/drivers/bus/mhi/core/pm.c
> > @@ -9,6 +9,7 @@
> > Â #include <linux/dma-direction.h>
> > Â #include <linux/dma-mapping.h>
> > Â #include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > Â #include <linux/list.h>
> > Â #include <linux/mhi.h>
> > Â #include <linux/module.h>
> > @@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct 
> > mhi_controller *mhi_cntrl,
> > Â int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
> > Â {
> > +ÂÂÂ enum mhi_state state;
> > ÂÂÂÂÂ enum mhi_ee_type current_ee;
> > ÂÂÂÂÂ enum dev_st_transition next_state;
> > ÂÂÂÂÂ struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > @@ -829,6 +831,24 @@ int mhi_async_power_up(struct mhi_controller 
> > *mhi_cntrl)
> > ÂÂÂÂÂÂÂÂÂ goto error_bhi_offset;
> > ÂÂÂÂÂ }
> > +ÂÂÂ state = mhi_get_mhi_state(mhi_cntrl);
> > +ÂÂÂ if (state == MHI_STATE_SYS_ERR) {
> > +ÂÂÂÂÂÂÂ mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
> > +ÂÂÂÂÂÂÂ ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ !(val & MHICTRL_RESET_MASK), 1000,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mhi_cntrl->timeout_ms * 1000);
> can we use this instead of polling because MSI is configures and int_vec 
> handler is registered
>
>  ÂÂÂ wait_event_timeout(mhi_cntrl->state_event,
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) ||
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂ mhi_read_reg_field(mhi_cntrl, base, MHICTRL,
>  ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂÂÂ MHICTRL_RESET_MASK,
>  ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂÂÂ MHICTRL_RESET_SHIFT, &reset) || !reset ,
>  ÂÂÂ ÂÂÂ ÂÂÂ ÂÂ msecs_to_jiffies(mhi_cntrl->timeout_ms));
>
> 1) In case of MHI_PM_IN_FATAL_STATE we would not be accessing MHI reg
> 2) Consistent with current MHI driver code.

I'm not sure this works in the way you intend.

state_event is linked to the intvec, which is the BHI interrupt.  I 
don't see that the state_event is triggered in the MHI interrupt path 
(mhi_irq_handler).  So, if we are in the PBL EE, we would expect to see 
the BHI interrupt, but if we are in the AMSS EE, we would expect to see 
a MHI interrupt.

Now, for my concerned usecase, those two interrupts happen to be the 
same interrupt, so both will get triggered, but I don't expect that to 
be the same for all usecases.

So, with the solution I propose, we exit the wait (poll loop) as soon as 
we see the register change values.

With the solution you propose, if we only get the MHI interrupt, we'll 
have to wait out the entire timeout value, and then check the register. 
In this scenario, we are almost guaranteed to wait for longer than 
necessary.

Did I miss something?

> > +ÂÂÂÂÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_info(dev, "Failed to reset MHI due to syserr state\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto error_bhi_offset;
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ /*
> > +ÂÂÂÂÂÂÂÂ * device cleares INTVEC as part of RESET processing,
> > +ÂÂÂÂÂÂÂÂ * re-program it
> > +ÂÂÂÂÂÂÂÂ */
> > +ÂÂÂÂÂÂÂ mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
> > +ÂÂÂ }
> > +
> > ÂÂÂÂÂ /* Transition to next state */
> > ÂÂÂÂÂ next_state = MHI_IN_PBL(current_ee) ?
> > ÂÂÂÂÂÂÂÂÂ DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;


--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.