linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Polling for MHI ready
@ 2021-02-24  3:44 Bhaumik Bhatt
  2021-02-24  3:44 ` [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Bhaumik Bhatt @ 2021-02-24  3:44 UTC (permalink / raw)
  To: manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, loic.poulain,
	carl.yin, naveen.kumar, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function
  2021-02-24  3:44 [PATCH v3 0/3] Polling for MHI ready Bhaumik Bhatt
@ 2021-02-24  3:44 ` Bhaumik Bhatt
  2021-03-05 13:37   ` Loic Poulain
  2021-03-10 18:57   ` Jeffrey Hugo
  2021-02-24  3:44 ` [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 11+ messages in thread
From: Bhaumik Bhatt @ 2021-02-24  3:44 UTC (permalink / raw)
  To: manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, loic.poulain,
	carl.yin, naveen.kumar, 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready
  2021-02-24  3:44 [PATCH v3 0/3] Polling for MHI ready Bhaumik Bhatt
  2021-02-24  3:44 ` [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt
@ 2021-02-24  3:44 ` Bhaumik Bhatt
  2021-03-10 19:02   ` Jeffrey Hugo
  2021-02-24  3:44 ` [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt
  2021-02-24  9:27 ` [PATCH v3 0/3] Polling for MHI ready Manivannan Sadhasivam
  3 siblings, 1 reply; 11+ messages in thread
From: Bhaumik Bhatt @ 2021-02-24  3:44 UTC (permalink / raw)
  To: manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, loic.poulain,
	carl.yin, naveen.kumar, 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download
  2021-02-24  3:44 [PATCH v3 0/3] Polling for MHI ready Bhaumik Bhatt
  2021-02-24  3:44 ` [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt
  2021-02-24  3:44 ` [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt
@ 2021-02-24  3:44 ` Bhaumik Bhatt
  2021-03-10 19:04   ` Jeffrey Hugo
  2021-02-24  9:27 ` [PATCH v3 0/3] Polling for MHI ready Manivannan Sadhasivam
  3 siblings, 1 reply; 11+ messages in thread
From: Bhaumik Bhatt @ 2021-02-24  3:44 UTC (permalink / raw)
  To: manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, loic.poulain,
	carl.yin, naveen.kumar, 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 0/3] Polling for MHI ready
  2021-02-24  3:44 [PATCH v3 0/3] Polling for MHI ready Bhaumik Bhatt
                   ` (2 preceding siblings ...)
  2021-02-24  3:44 ` [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt
@ 2021-02-24  9:27 ` Manivannan Sadhasivam
  3 siblings, 0 replies; 11+ messages in thread
From: Manivannan Sadhasivam @ 2021-02-24  9:27 UTC (permalink / raw)
  To: Bhaumik Bhatt
  Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, loic.poulain,
	carl.yin, naveen.kumar

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function
  2021-02-24  3:44 ` [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt
@ 2021-03-05 13:37   ` Loic Poulain
  2021-03-10 18:57   ` Jeffrey Hugo
  1 sibling, 0 replies; 11+ messages in thread
From: Loic Poulain @ 2021-03-05 13:37 UTC (permalink / raw)
  To: Bhaumik Bhatt
  Cc: Manivannan Sadhasivam, linux-arm-msm, Hemant Kumar, Jeffrey Hugo,
	open list, Carl Yin(殷张成),
	Naveen Kumar

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function
  2021-02-24  3:44 ` [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt
  2021-03-05 13:37   ` Loic Poulain
@ 2021-03-10 18:57   ` Jeffrey Hugo
  2021-03-10 22:56     ` Bhaumik Bhatt
  1 sibling, 1 reply; 11+ messages in thread
From: Jeffrey Hugo @ 2021-03-10 18:57 UTC (permalink / raw)
  To: Bhaumik Bhatt, manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, linux-kernel, loic.poulain, carl.yin,
	naveen.kumar

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready
  2021-02-24  3:44 ` [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt
@ 2021-03-10 19:02   ` Jeffrey Hugo
  2021-03-10 22:58     ` Bhaumik Bhatt
  0 siblings, 1 reply; 11+ messages in thread
From: Jeffrey Hugo @ 2021-03-10 19:02 UTC (permalink / raw)
  To: Bhaumik Bhatt, manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, linux-kernel, loic.poulain, carl.yin,
	naveen.kumar

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download
  2021-02-24  3:44 ` [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt
@ 2021-03-10 19:04   ` Jeffrey Hugo
  0 siblings, 0 replies; 11+ messages in thread
From: Jeffrey Hugo @ 2021-03-10 19:04 UTC (permalink / raw)
  To: Bhaumik Bhatt, manivannan.sadhasivam
  Cc: linux-arm-msm, hemantk, linux-kernel, loic.poulain, carl.yin,
	naveen.kumar

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function
  2021-03-10 18:57   ` Jeffrey Hugo
@ 2021-03-10 22:56     ` Bhaumik Bhatt
  0 siblings, 0 replies; 11+ messages in thread
From: Bhaumik Bhatt @ 2021-03-10 22:56 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: manivannan.sadhasivam, linux-arm-msm, hemantk, linux-kernel,
	loic.poulain, carl.yin, naveen.kumar, jhugo=codeaurora.org

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready
  2021-03-10 19:02   ` Jeffrey Hugo
@ 2021-03-10 22:58     ` Bhaumik Bhatt
  0 siblings, 0 replies; 11+ messages in thread
From: Bhaumik Bhatt @ 2021-03-10 22:58 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: manivannan.sadhasivam, linux-arm-msm, hemantk, linux-kernel,
	loic.poulain, carl.yin, naveen.kumar

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-03-10 22:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-24  3:44 [PATCH v3 0/3] Polling for MHI ready Bhaumik Bhatt
2021-02-24  3:44 ` [PATCH v3 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt
2021-03-05 13:37   ` Loic Poulain
2021-03-10 18:57   ` Jeffrey Hugo
2021-03-10 22:56     ` Bhaumik Bhatt
2021-02-24  3:44 ` [PATCH v3 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt
2021-03-10 19:02   ` Jeffrey Hugo
2021-03-10 22:58     ` Bhaumik Bhatt
2021-02-24  3:44 ` [PATCH v3 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt
2021-03-10 19:04   ` Jeffrey Hugo
2021-02-24  9:27 ` [PATCH v3 0/3] Polling for MHI ready Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).