| Message ID | 20220212182117.49438-20-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add initial support for MHI endpoint stack | expand |
On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote: > Add support for handling SYS_ERR (System Error) condition in the MHI > endpoint stack. The SYS_ERR flag will be asserted by the endpoint device > when it detects an internal error. The host will then issue reset and > reinitializes MHI to recover from the error state. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> I have a few small comments, but this look good enough for me. Reviewed-by: Alex Elder <elder@linaro.org> > --- > drivers/bus/mhi/ep/internal.h | 1 + > drivers/bus/mhi/ep/main.c | 24 ++++++++++++++++++++++++ > drivers/bus/mhi/ep/sm.c | 2 ++ > 3 files changed, 27 insertions(+) > > diff --git a/drivers/bus/mhi/ep/internal.h b/drivers/bus/mhi/ep/internal.h > index ee8c5974f0c0..8654af7caf40 100644 > --- a/drivers/bus/mhi/ep/internal.h > +++ b/drivers/bus/mhi/ep/internal.h > @@ -241,6 +241,7 @@ int mhi_ep_set_mhi_state(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_state mhi_stat > int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl); > int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl); > int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl); > +void mhi_ep_handle_syserr(struct mhi_ep_cntrl *mhi_cntrl); > > /* MHI EP memory management functions */ > int mhi_ep_alloc_map(struct mhi_ep_cntrl *mhi_cntrl, u64 pci_addr, size_t size, > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index ddedd0fb19aa..6378ac5c7e37 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -611,6 +611,30 @@ static void mhi_ep_reset_worker(struct work_struct *work) > } > } > > +/* > + * We don't need to do anything special other than setting the MHI SYS_ERR > + * state. The host issue will reset all contexts and issue MHI RESET so that we s/host issue/host/ > + * could also recover from error state. > + */ > +void mhi_ep_handle_syserr(struct mhi_ep_cntrl *mhi_cntrl) > +{ > + struct device *dev = &mhi_cntrl->mhi_dev->dev; > + int ret; > + > + /* If MHI EP is not enabled, nothing to do */ > + if (!mhi_cntrl->is_enabled) Is this an expected condition? SYS_ERR with the endpoint disabled? > + return; > + > + ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_SYS_ERR); > + if (ret) > + return; > + > + /* Signal host that the device went to SYS_ERR state */ > + ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_SYS_ERR); > + if (ret) > + dev_err(dev, "Failed sending SYS_ERR state change event: %d\n", ret); > +} > + > int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) > { > struct device *dev = &mhi_cntrl->mhi_dev->dev; > diff --git a/drivers/bus/mhi/ep/sm.c b/drivers/bus/mhi/ep/sm.c > index 68e7f99b9137..9a75ecfe1adf 100644 > --- a/drivers/bus/mhi/ep/sm.c > +++ b/drivers/bus/mhi/ep/sm.c > @@ -93,6 +93,7 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl) > > ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M0); > if (ret) { > + mhi_ep_handle_syserr(mhi_cntrl); > spin_unlock_bh(&mhi_cntrl->state_lock); > return ret; > } > @@ -128,6 +129,7 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl) > spin_lock_bh(&mhi_cntrl->state_lock); > ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3); Are there any other spots that should do this? For example, in mhi_ep_set_ready_state() you don't check the return value of the call to mhi_ep_set_mhi_state(). It seems to me it should be possible to preclude bogus state changes anyway, but I'm not completely sure. > if (ret) { > + mhi_ep_handle_syserr(mhi_cntrl); > spin_unlock_bh(&mhi_cntrl->state_lock); > return ret; > }
On Tue, Feb 15, 2022 at 04:39:55PM -0600, Alex Elder wrote: > On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote: > > Add support for handling SYS_ERR (System Error) condition in the MHI > > endpoint stack. The SYS_ERR flag will be asserted by the endpoint device > > when it detects an internal error. The host will then issue reset and > > reinitializes MHI to recover from the error state. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > I have a few small comments, but this look good enough for me. > > Reviewed-by: Alex Elder <elder@linaro.org> > > > --- > > drivers/bus/mhi/ep/internal.h | 1 + > > drivers/bus/mhi/ep/main.c | 24 ++++++++++++++++++++++++ > > drivers/bus/mhi/ep/sm.c | 2 ++ > > 3 files changed, 27 insertions(+) > > > > diff --git a/drivers/bus/mhi/ep/internal.h b/drivers/bus/mhi/ep/internal.h > > index ee8c5974f0c0..8654af7caf40 100644 > > --- a/drivers/bus/mhi/ep/internal.h > > +++ b/drivers/bus/mhi/ep/internal.h > > @@ -241,6 +241,7 @@ int mhi_ep_set_mhi_state(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_state mhi_stat > > int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl); > > int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl); > > int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl); > > +void mhi_ep_handle_syserr(struct mhi_ep_cntrl *mhi_cntrl); > > /* MHI EP memory management functions */ > > int mhi_ep_alloc_map(struct mhi_ep_cntrl *mhi_cntrl, u64 pci_addr, size_t size, > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > > index ddedd0fb19aa..6378ac5c7e37 100644 > > --- a/drivers/bus/mhi/ep/main.c > > +++ b/drivers/bus/mhi/ep/main.c > > @@ -611,6 +611,30 @@ static void mhi_ep_reset_worker(struct work_struct *work) > > } > > } > > +/* > > + * We don't need to do anything special other than setting the MHI SYS_ERR > > + * state. The host issue will reset all contexts and issue MHI RESET so that we > > s/host issue/host/ > > > + * could also recover from error state. > > + */ > > +void mhi_ep_handle_syserr(struct mhi_ep_cntrl *mhi_cntrl) > > +{ > > + struct device *dev = &mhi_cntrl->mhi_dev->dev; > > + int ret; > > + > > + /* If MHI EP is not enabled, nothing to do */ > > + if (!mhi_cntrl->is_enabled) > > Is this an expected condition? SYS_ERR with the endpoint > disabled? > I hit a case during bringup but I don't exactly remember where. So I'll probably remove this check. > > + return; > > + > > + ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_SYS_ERR); > > + if (ret) > > + return; > > + > > + /* Signal host that the device went to SYS_ERR state */ > > + ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_SYS_ERR); > > + if (ret) > > + dev_err(dev, "Failed sending SYS_ERR state change event: %d\n", ret); > > +} > > + > > int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) > > { > > struct device *dev = &mhi_cntrl->mhi_dev->dev; > > diff --git a/drivers/bus/mhi/ep/sm.c b/drivers/bus/mhi/ep/sm.c > > index 68e7f99b9137..9a75ecfe1adf 100644 > > --- a/drivers/bus/mhi/ep/sm.c > > +++ b/drivers/bus/mhi/ep/sm.c > > @@ -93,6 +93,7 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl) > > ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M0); > > if (ret) { > > + mhi_ep_handle_syserr(mhi_cntrl); > > spin_unlock_bh(&mhi_cntrl->state_lock); > > return ret; > > } > > @@ -128,6 +129,7 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl) > > spin_lock_bh(&mhi_cntrl->state_lock); > > ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3); > > Are there any other spots that should do this? For example, in > mhi_ep_set_ready_state() you don't check the return value of > the call to mhi_ep_set_mhi_state(). It seems to me it should > be possible to preclude bogus state changes anyway, but I'm > not completely sure. > The check should be there, I will add it to ready_state() also. Thanks, Mani > > if (ret) { > > + mhi_ep_handle_syserr(mhi_cntrl); > > spin_unlock_bh(&mhi_cntrl->state_lock); > > return ret; > > } >
diff --git a/drivers/bus/mhi/ep/internal.h b/drivers/bus/mhi/ep/internal.h index ee8c5974f0c0..8654af7caf40 100644 --- a/drivers/bus/mhi/ep/internal.h +++ b/drivers/bus/mhi/ep/internal.h @@ -241,6 +241,7 @@ int mhi_ep_set_mhi_state(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_state mhi_stat int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl); int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl); int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl); +void mhi_ep_handle_syserr(struct mhi_ep_cntrl *mhi_cntrl); /* MHI EP memory management functions */ int mhi_ep_alloc_map(struct mhi_ep_cntrl *mhi_cntrl, u64 pci_addr, size_t size, diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index ddedd0fb19aa..6378ac5c7e37 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -611,6 +611,30 @@ static void mhi_ep_reset_worker(struct work_struct *work) } } +/* + * We don't need to do anything special other than setting the MHI SYS_ERR + * state. The host issue will reset all contexts and issue MHI RESET so that we + * could also recover from error state. + */ +void mhi_ep_handle_syserr(struct mhi_ep_cntrl *mhi_cntrl) +{ + struct device *dev = &mhi_cntrl->mhi_dev->dev; + int ret; + + /* If MHI EP is not enabled, nothing to do */ + if (!mhi_cntrl->is_enabled) + return; + + ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_SYS_ERR); + if (ret) + return; + + /* Signal host that the device went to SYS_ERR state */ + ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_SYS_ERR); + if (ret) + dev_err(dev, "Failed sending SYS_ERR state change event: %d\n", ret); +} + int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) { struct device *dev = &mhi_cntrl->mhi_dev->dev; diff --git a/drivers/bus/mhi/ep/sm.c b/drivers/bus/mhi/ep/sm.c index 68e7f99b9137..9a75ecfe1adf 100644 --- a/drivers/bus/mhi/ep/sm.c +++ b/drivers/bus/mhi/ep/sm.c @@ -93,6 +93,7 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl) ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M0); if (ret) { + mhi_ep_handle_syserr(mhi_cntrl); spin_unlock_bh(&mhi_cntrl->state_lock); return ret; } @@ -128,6 +129,7 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl) spin_lock_bh(&mhi_cntrl->state_lock); ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3); if (ret) { + mhi_ep_handle_syserr(mhi_cntrl); spin_unlock_bh(&mhi_cntrl->state_lock); return ret; }
Add support for handling SYS_ERR (System Error) condition in the MHI endpoint stack. The SYS_ERR flag will be asserted by the endpoint device when it detects an internal error. The host will then issue reset and reinitializes MHI to recover from the error state. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/bus/mhi/ep/internal.h | 1 + drivers/bus/mhi/ep/main.c | 24 ++++++++++++++++++++++++ drivers/bus/mhi/ep/sm.c | 2 ++ 3 files changed, 27 insertions(+)