[PATCH v3 0/3] Polling for MHI ready

[PATCH v3 0/3] Polling for MHI ready

[Bhaumik Bhatt]

v3:
-Removed config changes that crept in in the first patch

v2:
-Addressed review comments
-Introduce new patch for to use controller defined read_reg() for polling
-Add usage in RDDM download panic path as well

Use polling instead of interrupt driven approach to wait for MHI ready state.

In certain devices, it is likely that there is no incoming MHI
interrupt for a transition to MHI READY state. One such example
is the move from Pass Through to an SBL or AMSS execution
environment. In order to facilitate faster bootup times as there
is no need to wait until timeout_ms completes, MHI host can poll
every 25 milliseconds to check if device has entered MHI READY
until a maximum timeout of twice the timeout_ms is reached.

This patch series has been tested on an arm64 device.

Bhaumik Bhatt (3):
  bus: mhi: core: Introduce internal register poll helper function
  bus: mhi: core: Move to polling method to wait for MHI ready
  bus: mhi: core: Use poll register read API for RDDM download

 drivers/bus/mhi/core/boot.c     | 20 ++++++--------------
 drivers/bus/mhi/core/internal.h |  3 +++
 drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
 drivers/bus/mhi/core/pm.c       | 31 ++++++++++++++-----------------
 4 files changed, 46 insertions(+), 31 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function

[Bhaumik Bhatt]

Introduce helper function to allow MHI core driver to poll for
a value in a register field. This helps reach a common path to
read and poll register values along with a retry time interval.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/internal.h |  3 +++
 drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 6f80ec3..005286b 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
 int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
 				    void __iomem *base, u32 offset, u32 mask,
 				    u32 shift, u32 *out);
+int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
+				    void __iomem *base, u32 offset, u32 mask,
+				    u32 shift, u32 val, u32 delayus);
 void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
 		   u32 offset, u32 val);
 void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 4e0131b..249ae26 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -4,6 +4,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
@@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
 	return 0;
 }
 
+int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
+				    void __iomem *base, u32 offset,
+				    u32 mask, u32 shift, u32 val, u32 delayus)
+{
+	int ret = -ENOENT;
+	u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
+
+	while (retry--) {
+		ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift,
+					 &out);
+		if (ret)
+			return -EIO;
+
+		if (out == val)
+			return 0;
+
+		udelay(delayus);
+	}
+
+	return ret;
+}
+
 void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
 		   u32 offset, u32 val)
 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function

[Loic Poulain]

On Wed, 24 Feb 2021 at 04:44, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote:
>
> Introduce helper function to allow MHI core driver to poll for
> a value in a register field. This helps reach a common path to
> read and poll register values along with a retry time interval.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/internal.h |  3 +++
>  drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 6f80ec3..005286b 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
>  int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
>                                     void __iomem *base, u32 offset, u32 mask,
>                                     u32 shift, u32 *out);
> +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> +                                   void __iomem *base, u32 offset, u32 mask,
> +                                   u32 shift, u32 val, u32 delayus);
>  void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
>                    u32 offset, u32 val);
>  void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 4e0131b..249ae26 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -4,6 +4,7 @@
>   *
>   */
>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/dma-direction.h>
>  #include <linux/dma-mapping.h>
> @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
>         return 0;
>  }
>
> +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> +                                   void __iomem *base, u32 offset,
> +                                   u32 mask, u32 shift, u32 val, u32 delayus)
> +{
> +       int ret = -ENOENT;
> +       u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus;

Can we get the timeout from parameter, not sure all callers will want
to wait the controller timeout_ms in the future. In case of PCI the
mhi_cntrl->timeout_ms can be really huge given the device can take up
to 15 seconds to completely start.

> +
> +       while (retry--) {
> +               ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift,
> +                                        &out);
> +               if (ret)
> +                       return -EIO;
> +
> +               if (out == val)
> +                       return 0;
> +
> +               udelay(delayus);

I would use a sleep variant (msleep) and millisecond parameter for the function.

Regards,
Loic

Re: [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function

[Jeffrey Hugo]

On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote:
> Introduce helper function to allow MHI core driver to poll for
> a value in a register field. This helps reach a common path to
> read and poll register values along with a retry time interval.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>   drivers/bus/mhi/core/internal.h |  3 +++
>   drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
>   2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 6f80ec3..005286b 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
>   int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
>   				    void __iomem *base, u32 offset, u32 mask,
>   				    u32 shift, u32 *out);
> +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> +				    void __iomem *base, u32 offset, u32 mask,
> +				    u32 shift, u32 val, u32 delayus);
>   void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
>   		   u32 offset, u32 val);
>   void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 4e0131b..249ae26 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -4,6 +4,7 @@
>    *
>    */
>   
> +#include <linux/delay.h>
>   #include <linux/device.h>
>   #include <linux/dma-direction.h>
>   #include <linux/dma-mapping.h>
> @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
>   	return 0;
>   }
>   
> +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> +				    void __iomem *base, u32 offset,
> +				    u32 mask, u32 shift, u32 val, u32 delayus)
> +{
> +	int ret = -ENOENT;
> +	u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
> +
> +	while (retry--) {
> +		ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift,
> +					 &out);
> +		if (ret)
> +			return -EIO;

I generally dislike recoding return codes.  Do you believe it adds value 
here?  I'm concerned that if I'm debugging an error, I'll get EIO, which 
I trace to here, but then I don't know what the actual error from 
mhi_read_reg_field() was.

> +
> +		if (out == val)
> +			return 0;
> +
> +		udelay(delayus);
> +	}
> +
> +	return ret;
> +}
> +
>   void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
>   		   u32 offset, u32 val)
>   {
> 


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

Re: [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function

[Bhaumik Bhatt]

On 2021-03-10 10:57 AM, Jeffrey Hugo wrote:
> On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote:
>> Introduce helper function to allow MHI core driver to poll for
>> a value in a register field. This helps reach a common path to
>> read and poll register values along with a retry time interval.
>> 
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/internal.h |  3 +++
>>   drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>> 
>> diff --git a/drivers/bus/mhi/core/internal.h 
>> b/drivers/bus/mhi/core/internal.h
>> index 6f80ec3..005286b 100644
>> --- a/drivers/bus/mhi/core/internal.h
>> +++ b/drivers/bus/mhi/core/internal.h
>> @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct 
>> mhi_controller *mhi_cntrl,
>>   int __must_check mhi_read_reg_field(struct mhi_controller 
>> *mhi_cntrl,
>>   				    void __iomem *base, u32 offset, u32 mask,
>>   				    u32 shift, u32 *out);
>> +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
>> +				    void __iomem *base, u32 offset, u32 mask,
>> +				    u32 shift, u32 val, u32 delayus);
>>   void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem 
>> *base,
>>   		   u32 offset, u32 val);
>>   void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void 
>> __iomem *base,
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 4e0131b..249ae26 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -4,6 +4,7 @@
>>    *
>>    */
>>   +#include <linux/delay.h>
>>   #include <linux/device.h>
>>   #include <linux/dma-direction.h>
>>   #include <linux/dma-mapping.h>
>> @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct 
>> mhi_controller *mhi_cntrl,
>>   	return 0;
>>   }
>>   +int __must_check mhi_poll_reg_field(struct mhi_controller 
>> *mhi_cntrl,
>> +				    void __iomem *base, u32 offset,
>> +				    u32 mask, u32 shift, u32 val, u32 delayus)
>> +{
>> +	int ret = -ENOENT;
Make this int ret;
>> +	u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
>> +
>> +	while (retry--) {
>> +		ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift,
>> +					 &out);
>> +		if (ret)
>> +			return -EIO;
> 
> I generally dislike recoding return codes.  Do you believe it adds
> value here?  I'm concerned that if I'm debugging an error, I'll get
> EIO, which I trace to here, but then I don't know what the actual
> error from mhi_read_reg_field() was.
> 
Thanks for pointing out. I don't think its necessary to recode and I can 
go back
to returning whatever the reg_field API returns. I have added the 
proposed changes
here which help fix a potential bug that we'd return 0 if read is 
successful but
polling fails.
>> +
>> +		if (out == val)
>> +			return 0;
>> +
>> +		udelay(delayus);
>> +	}
>> +
>> +	return ret;
return -ENOENT; to signify failure to find the entry after poll duration 
completes
>> +}
>> +
>>   void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem 
>> *base,
>>   		   u32 offset, u32 val)
>>   {
>> 

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready

[Bhaumik Bhatt]

In certain devices, it is likely that there is no incoming MHI
interrupt for a transition to MHI READY state. One such example
is the move from Pass Through to an SBL or AMSS execution
environment. In order to facilitate faster bootup times as there
is no need to wait until timeout_ms completes, MHI host can poll
every 25 milliseconds to check if device has entered MHI READY
until a maximum timeout of twice the timeout_ms is reached.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 681960c..5fe33d4 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -153,34 +153,31 @@ static void mhi_toggle_dev_wake(struct mhi_controller *mhi_cntrl)
 /* Handle device ready state transition */
 int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl)
 {
-	void __iomem *base = mhi_cntrl->regs;
 	struct mhi_event *mhi_event;
 	enum mhi_pm_state cur_state;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
-	u32 reset = 1, ready = 0;
 	int ret, i;
 
-	/* Wait for RESET to be cleared and READY bit to be set by the device */
-	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) ||
-			   mhi_read_reg_field(mhi_cntrl, base, MHISTATUS,
-					      MHISTATUS_READY_MASK,
-					      MHISTATUS_READY_SHIFT, &ready) ||
-			   (!reset && ready),
-			   msecs_to_jiffies(mhi_cntrl->timeout_ms));
-
 	/* Check if device entered error state */
 	if (MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) {
 		dev_err(dev, "Device link is not accessible\n");
 		return -EIO;
 	}
 
-	/* Timeout if device did not transition to ready state */
-	if (reset || !ready) {
-		dev_err(dev, "Device Ready timeout\n");
+	/* Wait for RESET to be cleared and READY bit to be set by the device */
+	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
+				 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
+				 25000);
+	if (ret) {
+		dev_err(dev, "Device failed to clear MHI Reset\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS,
+				 MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1,
+				 25000);
+	if (ret) {
+		dev_err(dev, "Device failed to enter MHI Ready\n");
 		return -ETIMEDOUT;
 	}
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready

[Jeffrey Hugo]

On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote:
> In certain devices, it is likely that there is no incoming MHI
> interrupt for a transition to MHI READY state. One such example
> is the move from Pass Through to an SBL or AMSS execution
> environment. In order to facilitate faster bootup times as there
> is no need to wait until timeout_ms completes, MHI host can poll
> every 25 milliseconds to check if device has entered MHI READY
> until a maximum timeout of twice the timeout_ms is reached.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>   drivers/bus/mhi/core/pm.c | 31 ++++++++++++++-----------------
>   1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 681960c..5fe33d4 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -153,34 +153,31 @@ static void mhi_toggle_dev_wake(struct mhi_controller *mhi_cntrl)
>   /* Handle device ready state transition */
>   int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl)
>   {
> -	void __iomem *base = mhi_cntrl->regs;
>   	struct mhi_event *mhi_event;
>   	enum mhi_pm_state cur_state;
>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> -	u32 reset = 1, ready = 0;
>   	int ret, i;
>   
> -	/* Wait for RESET to be cleared and READY bit to be set by the device */
> -	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) ||
> -			   mhi_read_reg_field(mhi_cntrl, base, MHISTATUS,
> -					      MHISTATUS_READY_MASK,
> -					      MHISTATUS_READY_SHIFT, &ready) ||
> -			   (!reset && ready),
> -			   msecs_to_jiffies(mhi_cntrl->timeout_ms));
> -
>   	/* Check if device entered error state */
>   	if (MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) {
>   		dev_err(dev, "Device link is not accessible\n");
>   		return -EIO;
>   	}
>   
> -	/* Timeout if device did not transition to ready state */
> -	if (reset || !ready) {
> -		dev_err(dev, "Device Ready timeout\n");
> +	/* Wait for RESET to be cleared and READY bit to be set by the device */
> +	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> +				 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
> +				 25000);
> +	if (ret) {
> +		dev_err(dev, "Device failed to clear MHI Reset\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS,
> +				 MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1,
> +				 25000);

You use the magic number "25000" twice here.  Its my understanding that 
the preference is to inline a magic number if its used in one spot, but 
use a macro if its used more than that.

Both uses are confined to this function, and in close proximity, so 
chances that one gets updated without the other seem minimal, so this 
feels like a borderline case.  I don't know if Mani has an opinion here.

I'd probably err on the side of making a macro or a single variable.  If 
not, I think some comments explaining the value are warranted (should 
comment the macro as well).

> +	if (ret) {
> +		dev_err(dev, "Device failed to enter MHI Ready\n");
>   		return -ETIMEDOUT;
>   	}
>   
> 


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

Re: [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready

[Bhaumik Bhatt]

On 2021-03-10 11:02 AM, Jeffrey Hugo wrote:
> On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote:
>> In certain devices, it is likely that there is no incoming MHI
>> interrupt for a transition to MHI READY state. One such example
>> is the move from Pass Through to an SBL or AMSS execution
>> environment. In order to facilitate faster bootup times as there
>> is no need to wait until timeout_ms completes, MHI host can poll
>> every 25 milliseconds to check if device has entered MHI READY
>> until a maximum timeout of twice the timeout_ms is reached.
>> 
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/pm.c | 31 ++++++++++++++-----------------
>>   1 file changed, 14 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 681960c..5fe33d4 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -153,34 +153,31 @@ static void mhi_toggle_dev_wake(struct 
>> mhi_controller *mhi_cntrl)
>>   /* Handle device ready state transition */
>>   int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl)
>>   {
>> -	void __iomem *base = mhi_cntrl->regs;
>>   	struct mhi_event *mhi_event;
>>   	enum mhi_pm_state cur_state;
>>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> -	u32 reset = 1, ready = 0;
>>   	int ret, i;
>>   -	/* Wait for RESET to be cleared and READY bit to be set by the 
>> device */
>> -	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) ||
>> -			   mhi_read_reg_field(mhi_cntrl, base, MHISTATUS,
>> -					      MHISTATUS_READY_MASK,
>> -					      MHISTATUS_READY_SHIFT, &ready) ||
>> -			   (!reset && ready),
>> -			   msecs_to_jiffies(mhi_cntrl->timeout_ms));
>> -
>>   	/* Check if device entered error state */
>>   	if (MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) {
>>   		dev_err(dev, "Device link is not accessible\n");
>>   		return -EIO;
>>   	}
>>   -	/* Timeout if device did not transition to ready state */
>> -	if (reset || !ready) {
>> -		dev_err(dev, "Device Ready timeout\n");
>> +	/* Wait for RESET to be cleared and READY bit to be set by the 
>> device */
>> +	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
>> +				 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
>> +				 25000);
>> +	if (ret) {
>> +		dev_err(dev, "Device failed to clear MHI Reset\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS,
>> +				 MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1,
>> +				 25000);
> 
> You use the magic number "25000" twice here.  Its my understanding
> that the preference is to inline a magic number if its used in one
> spot, but use a macro if its used more than that.
> 
> Both uses are confined to this function, and in close proximity, so
> chances that one gets updated without the other seem minimal, so this
> feels like a borderline case.  I don't know if Mani has an opinion
> here.
> 
> I'd probably err on the side of making a macro or a single variable.
> If not, I think some comments explaining the value are warranted
> (should comment the macro as well).
> 
I think just using a variable would be good enough here. I can add a 
comment
when defining the interval variable.
>> +	if (ret) {
>> +		dev_err(dev, "Device failed to enter MHI Ready\n");
>>   		return -ETIMEDOUT;
>>   	}
>> 

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download

[Bhaumik Bhatt]

Make use of mhi_poll_reg_field() API in order to poll for RDDM
download in panic path to employ a common approach throughout the
driver.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index c2546bf..b9c44e0 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -60,7 +60,6 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
 	u32 rx_status;
 	enum mhi_ee_type ee;
 	const u32 delayus = 2000;
-	u32 retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
 	const u32 rddm_timeout_us = 200000;
 	int rddm_retry = rddm_timeout_us / delayus;
 	void __iomem *base = mhi_cntrl->bhie;
@@ -125,19 +124,12 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
 		"Waiting for RDDM image download via BHIe, current EE:%s\n",
 		TO_MHI_EXEC_STR(ee));
 
-	while (retry--) {
-		ret = mhi_read_reg_field(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS,
-					 BHIE_RXVECSTATUS_STATUS_BMSK,
-					 BHIE_RXVECSTATUS_STATUS_SHFT,
-					 &rx_status);
-		if (ret)
-			return -EIO;
-
-		if (rx_status == BHIE_RXVECSTATUS_STATUS_XFER_COMPL)
-			return 0;
-
-		udelay(delayus);
-	}
+	ret = mhi_poll_reg_field(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS,
+				 BHIE_RXVECSTATUS_STATUS_BMSK,
+				 BHIE_RXVECSTATUS_STATUS_SHFT,
+				 BHIE_RXVECSTATUS_STATUS_XFER_COMPL, delayus);
+	if (!ret)
+		return 0;
 
 	ee = mhi_get_exec_env(mhi_cntrl);
 	ret = mhi_read_reg(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS, &rx_status);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download

[Jeffrey Hugo]

On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote:
> Make use of mhi_poll_reg_field() API in order to poll for RDDM
> download in panic path to employ a common approach throughout the
> driver.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

Seems ok to me.

Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>


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

Re: [PATCH v3 0/3] Polling for MHI ready

[Manivannan Sadhasivam]

On Tue, Feb 23, 2021 at 07:44:27PM -0800, Bhaumik Bhatt wrote:
> v3:
> -Removed config changes that crept in in the first patch
> 
> v2:
> -Addressed review comments
> -Introduce new patch for to use controller defined read_reg() for polling
> -Add usage in RDDM download panic path as well
> 
> Use polling instead of interrupt driven approach to wait for MHI ready state.
> 
> In certain devices, it is likely that there is no incoming MHI
> interrupt for a transition to MHI READY state. One such example
> is the move from Pass Through to an SBL or AMSS execution
> environment. In order to facilitate faster bootup times as there
> is no need to wait until timeout_ms completes, MHI host can poll
> every 25 milliseconds to check if device has entered MHI READY
> until a maximum timeout of twice the timeout_ms is reached.
> 
> This patch series has been tested on an arm64 device.
> 

So this is a spinoff from "Execution environment updates from MHI"?
Please mention it in the cover letter and which one is the latest.

Thanks,
Mani

> Bhaumik Bhatt (3):
>   bus: mhi: core: Introduce internal register poll helper function
>   bus: mhi: core: Move to polling method to wait for MHI ready
>   bus: mhi: core: Use poll register read API for RDDM download
> 
>  drivers/bus/mhi/core/boot.c     | 20 ++++++--------------
>  drivers/bus/mhi/core/internal.h |  3 +++
>  drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
>  drivers/bus/mhi/core/pm.c       | 31 ++++++++++++++-----------------
>  4 files changed, 46 insertions(+), 31 deletions(-)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 9514 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <1614138270-2374-3-git-send-email-bbhatt@codeaurora.org>
References: <1614138270-2374-3-git-send-email-bbhatt@codeaurora.org>
TO: Bhaumik Bhatt <bbhatt@codeaurora.org>
TO: manivannan.sadhasivam(a)linaro.org
CC: linux-arm-msm(a)vger.kernel.org
CC: hemantk(a)codeaurora.org
CC: jhugo(a)codeaurora.org
CC: linux-kernel(a)vger.kernel.org
CC: loic.poulain(a)linaro.org
CC: carl.yin(a)quectel.com
CC: naveen.kumar(a)quectel.com
CC: Bhaumik Bhatt <bbhatt@codeaurora.org>

Hi Bhaumik,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11 next-20210224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bhaumik-Bhatt/Polling-for-MHI-ready/20210224-114954
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git c03c21ba6f4e95e406a1a7b4c34ef334b977c194
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: riscv-randconfig-m031-20210224 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/bus/mhi/core/pm.c:239 mhi_ready_state_transition() warn: inconsistent returns '&mhi_cntrl->pm_lock'.

Old smatch warnings:
drivers/bus/mhi/core/pm.c:308 mhi_pm_m0_transition() warn: inconsistent returns '&mhi_cntrl->pm_lock'.
drivers/bus/mhi/core/pm.c:442 mhi_pm_mission_mode_transition() warn: inconsistent returns '&mhi_cntrl->pm_lock'.
drivers/bus/mhi/core/pm.c:859 mhi_pm_suspend() warn: inconsistent returns '&mhi_cntrl->pm_lock'.
arch/riscv/include/asm/atomic.h:204 atomic_fetch_add_unless() warn: inconsistent indenting

vim +239 drivers/bus/mhi/core/pm.c

3000f85b8f47b2 Manivannan Sadhasivam 2020-02-20  152  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  153  /* Handle device ready state transition */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  154  int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl)
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  155  {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  156  	struct mhi_event *mhi_event;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  157  	enum mhi_pm_state cur_state;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  158  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  159  	int ret, i;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  160  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  161  	/* Check if device entered error state */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  162  	if (MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  163  		dev_err(dev, "Device link is not accessible\n");
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  164  		return -EIO;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  165  	}
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  166  
112aa8cd374eda Bhaumik Bhatt         2021-02-23  167  	/* Wait for RESET to be cleared and READY bit to be set by the device */
112aa8cd374eda Bhaumik Bhatt         2021-02-23  168  	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
112aa8cd374eda Bhaumik Bhatt         2021-02-23  169  				 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
112aa8cd374eda Bhaumik Bhatt         2021-02-23  170  				 25000);
112aa8cd374eda Bhaumik Bhatt         2021-02-23  171  	if (ret) {
112aa8cd374eda Bhaumik Bhatt         2021-02-23  172  		dev_err(dev, "Device failed to clear MHI Reset\n");
112aa8cd374eda Bhaumik Bhatt         2021-02-23  173  		return -ETIMEDOUT;
112aa8cd374eda Bhaumik Bhatt         2021-02-23  174  	}
112aa8cd374eda Bhaumik Bhatt         2021-02-23  175  
112aa8cd374eda Bhaumik Bhatt         2021-02-23  176  	ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS,
112aa8cd374eda Bhaumik Bhatt         2021-02-23  177  				 MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1,
112aa8cd374eda Bhaumik Bhatt         2021-02-23  178  				 25000);
112aa8cd374eda Bhaumik Bhatt         2021-02-23  179  	if (ret) {
112aa8cd374eda Bhaumik Bhatt         2021-02-23  180  		dev_err(dev, "Device failed to enter MHI Ready\n");
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  181  		return -ETIMEDOUT;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  182  	}
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  183  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  184  	dev_dbg(dev, "Device in READY State\n");
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  185  	write_lock_irq(&mhi_cntrl->pm_lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  186  	cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_POR);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  187  	mhi_cntrl->dev_state = MHI_STATE_READY;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  188  	write_unlock_irq(&mhi_cntrl->pm_lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  189  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  190  	if (cur_state != MHI_PM_POR) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  191  		dev_err(dev, "Error moving to state %s from %s\n",
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  192  			to_mhi_pm_state_str(MHI_PM_POR),
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  193  			to_mhi_pm_state_str(cur_state));
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  194  		return -EIO;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  195  	}
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  196  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  197  	read_lock_bh(&mhi_cntrl->pm_lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  198  	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  199  		dev_err(dev, "Device registers not accessible\n");
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  200  		goto error_mmio;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  201  	}
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  202  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  203  	/* Configure MMIO registers */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  204  	ret = mhi_init_mmio(mhi_cntrl);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  205  	if (ret) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  206  		dev_err(dev, "Error configuring MMIO registers\n");
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  207  		goto error_mmio;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  208  	}
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  209  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  210  	/* Add elements to all SW event rings */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  211  	mhi_event = mhi_cntrl->mhi_event;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  212  	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  213  		struct mhi_ring *ring = &mhi_event->ring;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  214  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  215  		/* Skip if this is an offload or HW event */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  216  		if (mhi_event->offload_ev || mhi_event->hw_ring)
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  217  			continue;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  218  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  219  		ring->wp = ring->base + ring->len - ring->el_size;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  220  		*ring->ctxt_wp = ring->iommu_base + ring->len - ring->el_size;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  221  		/* Update all cores */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  222  		smp_wmb();
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  223  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  224  		/* Ring the event ring db */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  225  		spin_lock_irq(&mhi_event->lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  226  		mhi_ring_er_db(mhi_event);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  227  		spin_unlock_irq(&mhi_event->lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  228  	}
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  229  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  230  	/* Set MHI to M0 state */
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  231  	mhi_set_mhi_state(mhi_cntrl, MHI_STATE_M0);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  232  	read_unlock_bh(&mhi_cntrl->pm_lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  233  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  234  	return 0;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  235  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  236  error_mmio:
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  237  	read_unlock_bh(&mhi_cntrl->pm_lock);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  238  
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 @239  	return -EIO;
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  240  }
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20  241  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 26833 bytes --]