[1/3] bus: mhi: core: Handle syserr during power_up
diff mbox series

Message ID 1586207077-22361-2-git-send-email-jhugo@codeaurora.org
State New
Headers show
Series
  • Misc MHI fixes
Related show

Commit Message

Jeffrey Hugo April 6, 2020, 9:04 p.m. UTC
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@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Manivannan Sadhasivam April 7, 2020, 6:26 a.m. UTC | #1
On Mon, Apr 06, 2020 at 03:04:35PM -0600, 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@codeaurora.org>
> ---
>  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..cd6ba23 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,

Hmm. Do we really need a max 1ms delay between each read? I'd prefer to have
100ns to reduce the wait time.

> +					 mhi_cntrl->timeout_ms * 1000);
> +		if (ret) {
> +			dev_info(dev, "Failed to reset syserr\n");

dev_info(dev, "Failed to reset MHI due to syserr state\n"); ?

Thanks,
Mani

> +			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;
> -- 
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
Jeffrey Hugo April 7, 2020, 3:11 p.m. UTC | #2
On 4/7/2020 12:26 AM, Manivannan Sadhasivam wrote:
> On Mon, Apr 06, 2020 at 03:04:35PM -0600, 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@codeaurora.org>
>> ---
>>   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..cd6ba23 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,
> 
> Hmm. Do we really need a max 1ms delay between each read? I'd prefer to have
> 100ns to reduce the wait time.


I assume you mean 100us since that's the units of the parameter, and 
usleep_range is the actual delay mechanism.  Please correct me if that 
is a bad assumption.

I chose 1ms to try to avoid flooding the bus, since on one system we 
care about, the round trip time was observed to be ~1ms.  However, that 
is fairly arbitrary, so a factor of 10 reduction don't seem like a 
significant issue.

> 
>> +					 mhi_cntrl->timeout_ms * 1000);
>> +		if (ret) {
>> +			dev_info(dev, "Failed to reset syserr\n");
> 
> dev_info(dev, "Failed to reset MHI due to syserr state\n"); ?
> 

Ah yes, that is clearer.  Thanks
Manivannan Sadhasivam April 7, 2020, 3:32 p.m. UTC | #3
On Tue, Apr 07, 2020 at 09:11:33AM -0600, Jeffrey Hugo wrote:
> On 4/7/2020 12:26 AM, Manivannan Sadhasivam wrote:
> > On Mon, Apr 06, 2020 at 03:04:35PM -0600, 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@codeaurora.org>
> > > ---
> > >   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..cd6ba23 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,
> > 
> > Hmm. Do we really need a max 1ms delay between each read? I'd prefer to have
> > 100ns to reduce the wait time.
> 
> 
> I assume you mean 100us since that's the units of the parameter, and
> usleep_range is the actual delay mechanism.  Please correct me if that is a
> bad assumption.
> 

Yep, it will get extended to:

usleep_range((__sleep_us >> 2) + 1, __sleep_us)

So the max delay (range) here would be 100ns. Anyway, I'm okay with 1ms. Please
see below.

> I chose 1ms to try to avoid flooding the bus, since on one system we care
> about, the round trip time was observed to be ~1ms.  However, that is fairly
> arbitrary, so a factor of 10 reduction don't seem like a significant issue.
> 

Hmm. I thought 1ms is too much wait time but just looked into some downstream
controller implementation and they seem to be allowing the timeout value upto
2000ms. So this is fine. Sorry for the noise!

Thanks,
Mani

> > 
> > > +					 mhi_cntrl->timeout_ms * 1000);
> > > +		if (ret) {
> > > +			dev_info(dev, "Failed to reset syserr\n");
> > 
> > dev_info(dev, "Failed to reset MHI due to syserr state\n"); ?
> > 
> 
> Ah yes, that is clearer.  Thanks
> 
> -- 
> Jeffrey Hugo
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Patch
diff mbox series

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 52690cb..cd6ba23 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);
+		if (ret) {
+			dev_info(dev, "Failed to reset syserr\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;