[PATCH 0/3] drm/msm/dp: Simplify aux code

[PATCH 0/3] drm/msm/dp: Simplify aux code

[Stephen Boyd]

Here's a few patches that simplify the aux handling code and bubble up
timeouts and nacks to the upper DRM layers. The goal is to get DRM to
know that the other side isn't there or that there's been a timeout,
instead of saying that everything is fine and putting some error message
into the logs.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <abhinavk@codeaurora.org>
Cc: Kuogee Hsieh <khsieh@codeaurora.org>
Cc: aravindh@codeaurora.org
Cc: Sean Paul <sean@poorly.run>

Stephen Boyd (3):
  drm/msm/dp: Simplify aux irq handling code
  drm/msm/dp: Shrink locking area of dp_aux_transfer()
  drm/msm/dp: Handle aux timeouts, nacks, defers

 drivers/gpu/drm/msm/dp/dp_aux.c     | 181 ++++++++++++----------------
 drivers/gpu/drm/msm/dp/dp_aux.h     |   8 --
 drivers/gpu/drm/msm/dp/dp_catalog.c |   2 +-
 drivers/gpu/drm/msm/dp/dp_catalog.h |   2 +-
 4 files changed, 80 insertions(+), 113 deletions(-)


base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
-- 
https://chromeos.dev

[PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

[Stephen Boyd]

We don't need to stash away 'isr' in the aux structure to pass to two
functions. Let's use a local variable instead. And we can complete the
completion variable in one place instead of two to simplify the code.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <abhinavk@codeaurora.org>
Cc: Kuogee Hsieh <khsieh@codeaurora.org>
Cc: aravindh@codeaurora.org
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     | 22 ++++++++--------------
 drivers/gpu/drm/msm/dp/dp_catalog.c |  2 +-
 drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 7c22bfe0fc7d..91188466cece 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -27,7 +27,6 @@ struct dp_aux_private {
 	bool no_send_stop;
 	u32 offset;
 	u32 segment;
-	u32 isr;
 
 	struct drm_dp_aux dp_aux;
 };
@@ -181,10 +180,8 @@ static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
 	}
 }
 
-static void dp_aux_native_handler(struct dp_aux_private *aux)
+static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
 {
-	u32 isr = aux->isr;
-
 	if (isr & DP_INTR_AUX_I2C_DONE)
 		aux->aux_error_num = DP_AUX_ERR_NONE;
 	else if (isr & DP_INTR_WRONG_ADDR)
@@ -197,14 +194,10 @@ static void dp_aux_native_handler(struct dp_aux_private *aux)
 		aux->aux_error_num = DP_AUX_ERR_PHY;
 		dp_catalog_aux_clear_hw_interrupts(aux->catalog);
 	}
-
-	complete(&aux->comp);
 }
 
-static void dp_aux_i2c_handler(struct dp_aux_private *aux)
+static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
 {
-	u32 isr = aux->isr;
-
 	if (isr & DP_INTR_AUX_I2C_DONE) {
 		if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
 			aux->aux_error_num = DP_AUX_ERR_NACK;
@@ -226,8 +219,6 @@ static void dp_aux_i2c_handler(struct dp_aux_private *aux)
 			dp_catalog_aux_clear_hw_interrupts(aux->catalog);
 		}
 	}
-
-	complete(&aux->comp);
 }
 
 static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
@@ -412,6 +403,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 
 void dp_aux_isr(struct drm_dp_aux *dp_aux)
 {
+	u32 isr;
 	struct dp_aux_private *aux;
 
 	if (!dp_aux) {
@@ -421,15 +413,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
 
 	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
 
-	aux->isr = dp_catalog_aux_get_irq(aux->catalog);
+	isr = dp_catalog_aux_get_irq(aux->catalog);
 
 	if (!aux->cmd_busy)
 		return;
 
 	if (aux->native)
-		dp_aux_native_handler(aux);
+		dp_aux_native_handler(aux, isr);
 	else
-		dp_aux_i2c_handler(aux);
+		dp_aux_i2c_handler(aux, isr);
+
+	complete(&aux->comp);
 }
 
 void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b98f5f..a70c238f34b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -292,7 +292,7 @@ void dp_catalog_dump_regs(struct dp_catalog *dp_catalog)
 	dump_regs(catalog->io->dp_controller.base + offset, len);
 }
 
-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
 {
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 				struct dp_catalog_private, dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 176a9020a520..502bc0dc7787 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -80,7 +80,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
 void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
-- 
https://chromeos.dev

Re: [PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:
> We don't need to stash away 'isr' in the aux structure to pass to two
> functions. Let's use a local variable instead. And we can complete the
> completion variable in one place instead of two to simplify the code.
> 
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Cc: Kuogee Hsieh <khsieh@codeaurora.org>
> Cc: aravindh@codeaurora.org
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 22 ++++++++--------------
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  2 +-
>  drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
>  3 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 7c22bfe0fc7d..91188466cece 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -27,7 +27,6 @@ struct dp_aux_private {
>  	bool no_send_stop;
>  	u32 offset;
>  	u32 segment;
> -	u32 isr;
> 
>  	struct drm_dp_aux dp_aux;
>  };
> @@ -181,10 +180,8 @@ static void dp_aux_cmd_fifo_rx(struct 
> dp_aux_private *aux,
>  	}
>  }
> 
> -static void dp_aux_native_handler(struct dp_aux_private *aux)
> +static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
>  {
> -	u32 isr = aux->isr;
> -
>  	if (isr & DP_INTR_AUX_I2C_DONE)
>  		aux->aux_error_num = DP_AUX_ERR_NONE;
>  	else if (isr & DP_INTR_WRONG_ADDR)
> @@ -197,14 +194,10 @@ static void dp_aux_native_handler(struct
> dp_aux_private *aux)
>  		aux->aux_error_num = DP_AUX_ERR_PHY;
>  		dp_catalog_aux_clear_hw_interrupts(aux->catalog);
>  	}
> -
> -	complete(&aux->comp);
>  }
> 
> -static void dp_aux_i2c_handler(struct dp_aux_private *aux)
> +static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
>  {
> -	u32 isr = aux->isr;
> -
>  	if (isr & DP_INTR_AUX_I2C_DONE) {
>  		if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
>  			aux->aux_error_num = DP_AUX_ERR_NACK;
> @@ -226,8 +219,6 @@ static void dp_aux_i2c_handler(struct 
> dp_aux_private *aux)
>  			dp_catalog_aux_clear_hw_interrupts(aux->catalog);
>  		}
>  	}
> -
> -	complete(&aux->comp);
>  }
> 
>  static void dp_aux_update_offset_and_segment(struct dp_aux_private 
> *aux,
> @@ -412,6 +403,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> *dp_aux,
> 
>  void dp_aux_isr(struct drm_dp_aux *dp_aux)
>  {
> +	u32 isr;
>  	struct dp_aux_private *aux;
> 
>  	if (!dp_aux) {
> @@ -421,15 +413,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
> 
>  	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> 
> -	aux->isr = dp_catalog_aux_get_irq(aux->catalog);
> +	isr = dp_catalog_aux_get_irq(aux->catalog);
> 
>  	if (!aux->cmd_busy)
>  		return;
> 
>  	if (aux->native)
> -		dp_aux_native_handler(aux);
> +		dp_aux_native_handler(aux, isr);
>  	else
> -		dp_aux_i2c_handler(aux);
> +		dp_aux_i2c_handler(aux, isr);
> +
> +	complete(&aux->comp);
>  }
> 
>  void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index b1a9b1b98f5f..a70c238f34b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -292,7 +292,7 @@ void dp_catalog_dump_regs(struct dp_catalog 
> *dp_catalog)
>  	dump_regs(catalog->io->dp_controller.base + offset, len);
>  }
> 
> -int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
> +u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
>  {
>  	struct dp_catalog_private *catalog = container_of(dp_catalog,
>  				struct dp_catalog_private, dp_catalog);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
> b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 176a9020a520..502bc0dc7787 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -80,7 +80,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct
> dp_catalog *dp_catalog);
>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
>  void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool 
> enable);
>  void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
> -int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> +u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> 
>  /* DP Controller APIs */
>  void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 
> state);

[PATCH 2/3] drm/msm/dp: Shrink locking area of dp_aux_transfer()

[Stephen Boyd]

We don't need to hold the lock to inspect the message we're going to
transfer, and we don't need to clear the busy flag either. Take the lock
later and bail out earlier if conditions aren't met.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <abhinavk@codeaurora.org>
Cc: Kuogee Hsieh <khsieh@codeaurora.org>
Cc: aravindh@codeaurora.org
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_aux.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 91188466cece..b49810396513 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -329,30 +329,29 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	ssize_t ret;
 	int const aux_cmd_native_max = 16;
 	int const aux_cmd_i2c_max = 128;
-	struct dp_aux_private *aux = container_of(dp_aux,
-		struct dp_aux_private, dp_aux);
+	struct dp_aux_private *aux;
 
-	mutex_lock(&aux->mutex);
+	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
 
 	aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
 
 	/* Ignore address only message */
-	if ((msg->size == 0) || (msg->buffer == NULL)) {
+	if (msg->size == 0 || !msg->buffer) {
 		msg->reply = aux->native ?
 			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
-		ret = msg->size;
-		goto unlock_exit;
+		return msg->size;
 	}
 
 	/* msg sanity check */
-	if ((aux->native && (msg->size > aux_cmd_native_max)) ||
-		(msg->size > aux_cmd_i2c_max)) {
+	if ((aux->native && msg->size > aux_cmd_native_max) ||
+	    msg->size > aux_cmd_i2c_max) {
 		DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
 			__func__, msg->size, msg->request);
-		ret = -EINVAL;
-		goto unlock_exit;
+		return -EINVAL;
 	}
 
+	mutex_lock(&aux->mutex);
+
 	dp_aux_update_offset_and_segment(aux, msg);
 	dp_aux_transfer_helper(aux, msg, true);
 
-- 
https://chromeos.dev

Re: [PATCH 2/3] drm/msm/dp: Shrink locking area of dp_aux_transfer()

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:
> We don't need to hold the lock to inspect the message we're going to
> transfer, and we don't need to clear the busy flag either. Take the 
> lock
> later and bail out earlier if conditions aren't met.
> 
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Cc: Kuogee Hsieh <khsieh@codeaurora.org>
> Cc: aravindh@codeaurora.org
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 91188466cece..b49810396513 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -329,30 +329,29 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> *dp_aux,
>  	ssize_t ret;
>  	int const aux_cmd_native_max = 16;
>  	int const aux_cmd_i2c_max = 128;
> -	struct dp_aux_private *aux = container_of(dp_aux,
> -		struct dp_aux_private, dp_aux);
> +	struct dp_aux_private *aux;
> 
> -	mutex_lock(&aux->mutex);
> +	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> 
>  	aux->native = msg->request & (DP_AUX_NATIVE_WRITE & 
> DP_AUX_NATIVE_READ);
> 
>  	/* Ignore address only message */
> -	if ((msg->size == 0) || (msg->buffer == NULL)) {
> +	if (msg->size == 0 || !msg->buffer) {
>  		msg->reply = aux->native ?
>  			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> -		ret = msg->size;
> -		goto unlock_exit;
> +		return msg->size;
>  	}
> 
>  	/* msg sanity check */
> -	if ((aux->native && (msg->size > aux_cmd_native_max)) ||
> -		(msg->size > aux_cmd_i2c_max)) {
> +	if ((aux->native && msg->size > aux_cmd_native_max) ||
> +	    msg->size > aux_cmd_i2c_max) {
>  		DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>  			__func__, msg->size, msg->request);
> -		ret = -EINVAL;
> -		goto unlock_exit;
> +		return -EINVAL;
>  	}
> 
> +	mutex_lock(&aux->mutex);
> +
>  	dp_aux_update_offset_and_segment(aux, msg);
>  	dp_aux_transfer_helper(aux, msg, true);

[PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[Stephen Boyd]

Let's look at the irq status bits after a transfer and see if we got a
nack or a defer or a timeout, instead of telling drm layers that
everything was fine, while still printing an error message. I wasn't
sure about NACK+DEFER so I lumped all those various errors along with a
nack so that the drm core can figure out that things are just not going
well. The important thing is that we're now returning -ETIMEDOUT when
the message times out and nacks for bad addresses.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <abhinavk@codeaurora.org>
Cc: Kuogee Hsieh <khsieh@codeaurora.org>
Cc: aravindh@codeaurora.org
Cc: Sean Paul <sean@poorly.run>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++++++++++++++------------------
 drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
 2 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index b49810396513..4a3293b590b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -9,7 +9,15 @@
 #include "dp_reg.h"
 #include "dp_aux.h"
 
-#define DP_AUX_ENUM_STR(x)		#x
+enum msm_dp_aux_err {
+	DP_AUX_ERR_NONE,
+	DP_AUX_ERR_ADDR,
+	DP_AUX_ERR_TOUT,
+	DP_AUX_ERR_NACK,
+	DP_AUX_ERR_DEFER,
+	DP_AUX_ERR_NACK_DEFER,
+	DP_AUX_ERR_PHY,
+};
 
 struct dp_aux_private {
 	struct device *dev;
@@ -18,7 +26,7 @@ struct dp_aux_private {
 	struct mutex mutex;
 	struct completion comp;
 
-	u32 aux_error_num;
+	enum msm_dp_aux_err aux_error_num;
 	u32 retry_cnt;
 	bool cmd_busy;
 	bool native;
@@ -33,62 +41,45 @@ struct dp_aux_private {
 
 #define MAX_AUX_RETRIES			5
 
-static const char *dp_aux_get_error(u32 aux_error)
-{
-	switch (aux_error) {
-	case DP_AUX_ERR_NONE:
-		return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
-	case DP_AUX_ERR_ADDR:
-		return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
-	case DP_AUX_ERR_TOUT:
-		return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
-	case DP_AUX_ERR_NACK:
-		return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
-	case DP_AUX_ERR_DEFER:
-		return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
-	case DP_AUX_ERR_NACK_DEFER:
-		return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
-	default:
-		return "unknown";
-	}
-}
-
-static u32 dp_aux_write(struct dp_aux_private *aux,
+static ssize_t dp_aux_write(struct dp_aux_private *aux,
 			struct drm_dp_aux_msg *msg)
 {
-	u32 data[4], reg, len;
+	u8 data[4];
+	u32 reg;
+	ssize_t len;
 	u8 *msgdata = msg->buffer;
 	int const AUX_CMD_FIFO_LEN = 128;
 	int i = 0;
 
 	if (aux->read)
-		len = 4;
+		len = 0;
 	else
-		len = msg->size + 4;
+		len = msg->size;
 
 	/*
 	 * cmd fifo only has depth of 144 bytes
 	 * limit buf length to 128 bytes here
 	 */
-	if (len > AUX_CMD_FIFO_LEN) {
+	if (len > AUX_CMD_FIFO_LEN - 4) {
 		DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
-		return 0;
+		return -EINVAL;
 	}
 
 	/* Pack cmd and write to HW */
-	data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
+	data[0] = (msg->address >> 16) & 0xf;	/* addr[19:16] */
 	if (aux->read)
-		data[0] |=  BIT(4); /* R/W */
+		data[0] |=  BIT(4);		/* R/W */
 
-	data[1] = (msg->address >> 8) & 0xff;	/* addr[15:8] */
-	data[2] = msg->address & 0xff;		/* addr[7:0] */
-	data[3] = (msg->size - 1) & 0xff;	/* len[7:0] */
+	data[1] = msg->address >> 8;		/* addr[15:8] */
+	data[2] = msg->address;			/* addr[7:0] */
+	data[3] = msg->size - 1;		/* len[7:0] */
 
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len + 4; i++) {
 		reg = (i < 4) ? data[i] : msgdata[i - 4];
+		reg <<= DP_AUX_DATA_OFFSET;
+		reg &= DP_AUX_DATA_MASK;
+		reg |= DP_AUX_DATA_WRITE;
 		/* index = 0, write */
-		reg = (((reg) << DP_AUX_DATA_OFFSET)
-		       & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
 		if (i == 0)
 			reg |= DP_AUX_DATA_INDEX_WRITE;
 		aux->catalog->aux_data = reg;
@@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private *aux,
 	return len;
 }
 
-static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
 			      struct drm_dp_aux_msg *msg)
 {
-	u32 ret, len, timeout;
-	int aux_timeout_ms = HZ/4;
+	ssize_t ret;
+	unsigned long time_left;
 
 	reinit_completion(&aux->comp);
 
-	len = dp_aux_write(aux, msg);
-	if (len == 0) {
-		DRM_ERROR("DP AUX write failed\n");
-		return -EINVAL;
-	}
+	ret = dp_aux_write(aux, msg);
+	if (ret < 0)
+		return ret;
 
-	timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms);
-	if (!timeout) {
-		DRM_ERROR("aux %s timeout\n", (aux->read ? "read" : "write"));
+	time_left = wait_for_completion_timeout(&aux->comp,
+						msecs_to_jiffies(250));
+	if (!time_left)
 		return -ETIMEDOUT;
-	}
-
-	if (aux->aux_error_num == DP_AUX_ERR_NONE) {
-		ret = len;
-	} else {
-		DRM_ERROR_RATELIMITED("aux err: %s\n",
-			dp_aux_get_error(aux->aux_error_num));
-
-		ret = -EINVAL;
-	}
 
 	return ret;
 }
 
-static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
 		struct drm_dp_aux_msg *msg)
 {
 	u32 data;
@@ -175,9 +154,10 @@ static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
 
 		actual_i = (data >> DP_AUX_DATA_INDEX_OFFSET) & 0xFF;
 		if (i != actual_i)
-			DRM_ERROR("Index mismatch: expected %d, found %d\n",
-				i, actual_i);
+			break;
 	}
+
+	return i;
 }
 
 static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
@@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	}
 
 	ret = dp_aux_cmd_fifo_tx(aux, msg);
-
 	if (ret < 0) {
 		if (aux->native) {
 			aux->retry_cnt++;
 			if (!(aux->retry_cnt % MAX_AUX_RETRIES))
 				dp_catalog_aux_update_cfg(aux->catalog);
 		}
-		usleep_range(400, 500); /* at least 400us to next try */
-		goto unlock_exit;
-	}
-
-	if (aux->aux_error_num == DP_AUX_ERR_NONE) {
-		if (aux->read)
-			dp_aux_cmd_fifo_rx(aux, msg);
-
-		msg->reply = aux->native ?
-			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
 	} else {
-		/* Reply defer to retry */
-		msg->reply = aux->native ?
-			DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
+		aux->retry_cnt = 0;
+		switch (aux->aux_error_num) {
+		case DP_AUX_ERR_NONE:
+			if (aux->read)
+				ret = dp_aux_cmd_fifo_rx(aux, msg);
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
+			break;
+		case DP_AUX_ERR_DEFER:
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
+			break;
+		case DP_AUX_ERR_PHY:
+		case DP_AUX_ERR_ADDR:
+		case DP_AUX_ERR_NACK:
+		case DP_AUX_ERR_NACK_DEFER:
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
+			break;
+		case DP_AUX_ERR_TOUT:
+			ret = -ETIMEDOUT;
+			break;
+		}
 	}
 
-	/* Return requested size for success or retry */
-	ret = msg->size;
-	aux->retry_cnt = 0;
-
-unlock_exit:
 	aux->cmd_busy = false;
 	mutex_unlock(&aux->mutex);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index f8b8ba919465..0728cc09c9ec 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -9,14 +9,6 @@
 #include "dp_catalog.h"
 #include <drm/drm_dp_helper.h>
 
-#define DP_AUX_ERR_NONE		0
-#define DP_AUX_ERR_ADDR		-1
-#define DP_AUX_ERR_TOUT		-2
-#define DP_AUX_ERR_NACK		-3
-#define DP_AUX_ERR_DEFER	-4
-#define DP_AUX_ERR_NACK_DEFER	-5
-#define DP_AUX_ERR_PHY		-6
-
 int dp_aux_register(struct drm_dp_aux *dp_aux);
 void dp_aux_unregister(struct drm_dp_aux *dp_aux);
 void dp_aux_isr(struct drm_dp_aux *dp_aux);
-- 
https://chromeos.dev

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:
> Let's look at the irq status bits after a transfer and see if we got a
> nack or a defer or a timeout, instead of telling drm layers that
> everything was fine, while still printing an error message. I wasn't
> sure about NACK+DEFER so I lumped all those various errors along with a
> nack so that the drm core can figure out that things are just not going
> well. The important thing is that we're now returning -ETIMEDOUT when
> the message times out and nacks for bad addresses.
> 
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Cc: Kuogee Hsieh <khsieh@codeaurora.org>
> Cc: aravindh@codeaurora.org
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++++++++++++++------------------
>  drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
>  2 files changed, 61 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index b49810396513..4a3293b590b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -9,7 +9,15 @@
>  #include "dp_reg.h"
>  #include "dp_aux.h"
> 
> -#define DP_AUX_ENUM_STR(x)		#x
> +enum msm_dp_aux_err {
> +	DP_AUX_ERR_NONE,
> +	DP_AUX_ERR_ADDR,
> +	DP_AUX_ERR_TOUT,
> +	DP_AUX_ERR_NACK,
> +	DP_AUX_ERR_DEFER,
> +	DP_AUX_ERR_NACK_DEFER,
> +	DP_AUX_ERR_PHY,
> +};
> 
>  struct dp_aux_private {
>  	struct device *dev;
> @@ -18,7 +26,7 @@ struct dp_aux_private {
>  	struct mutex mutex;
>  	struct completion comp;
> 
> -	u32 aux_error_num;
> +	enum msm_dp_aux_err aux_error_num;
>  	u32 retry_cnt;
>  	bool cmd_busy;
>  	bool native;
> @@ -33,62 +41,45 @@ struct dp_aux_private {
> 
>  #define MAX_AUX_RETRIES			5
> 
> -static const char *dp_aux_get_error(u32 aux_error)
> -{
> -	switch (aux_error) {
> -	case DP_AUX_ERR_NONE:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
> -	case DP_AUX_ERR_ADDR:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
> -	case DP_AUX_ERR_TOUT:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
> -	case DP_AUX_ERR_NACK:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
> -	case DP_AUX_ERR_DEFER:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
> -	case DP_AUX_ERR_NACK_DEFER:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
> -	default:
> -		return "unknown";
> -	}
> -}
> -
> -static u32 dp_aux_write(struct dp_aux_private *aux,
> +static ssize_t dp_aux_write(struct dp_aux_private *aux,
>  			struct drm_dp_aux_msg *msg)
>  {
> -	u32 data[4], reg, len;
> +	u8 data[4];
> +	u32 reg;
> +	ssize_t len;
>  	u8 *msgdata = msg->buffer;
>  	int const AUX_CMD_FIFO_LEN = 128;
>  	int i = 0;
> 
>  	if (aux->read)
> -		len = 4;
> +		len = 0;
>  	else
> -		len = msg->size + 4;
> +		len = msg->size;
> 
>  	/*
>  	 * cmd fifo only has depth of 144 bytes
>  	 * limit buf length to 128 bytes here
>  	 */
> -	if (len > AUX_CMD_FIFO_LEN) {
> +	if (len > AUX_CMD_FIFO_LEN - 4) {
>  		DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
> -		return 0;
> +		return -EINVAL;
>  	}
> 
>  	/* Pack cmd and write to HW */
> -	data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
> +	data[0] = (msg->address >> 16) & 0xf;	/* addr[19:16] */
>  	if (aux->read)
> -		data[0] |=  BIT(4); /* R/W */
> +		data[0] |=  BIT(4);		/* R/W */
> 
> -	data[1] = (msg->address >> 8) & 0xff;	/* addr[15:8] */
> -	data[2] = msg->address & 0xff;		/* addr[7:0] */
> -	data[3] = (msg->size - 1) & 0xff;	/* len[7:0] */
> +	data[1] = msg->address >> 8;		/* addr[15:8] */
> +	data[2] = msg->address;			/* addr[7:0] */
> +	data[3] = msg->size - 1;		/* len[7:0] */
> 
> -	for (i = 0; i < len; i++) {
> +	for (i = 0; i < len + 4; i++) {
>  		reg = (i < 4) ? data[i] : msgdata[i - 4];
> +		reg <<= DP_AUX_DATA_OFFSET;
> +		reg &= DP_AUX_DATA_MASK;
> +		reg |= DP_AUX_DATA_WRITE;
>  		/* index = 0, write */
> -		reg = (((reg) << DP_AUX_DATA_OFFSET)
> -		       & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
>  		if (i == 0)
>  			reg |= DP_AUX_DATA_INDEX_WRITE;
>  		aux->catalog->aux_data = reg;
> @@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private 
> *aux,
>  	return len;
>  }
> 
> -static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>  			      struct drm_dp_aux_msg *msg)
>  {
> -	u32 ret, len, timeout;
> -	int aux_timeout_ms = HZ/4;
> +	ssize_t ret;
> +	unsigned long time_left;
> 
>  	reinit_completion(&aux->comp);
> 
> -	len = dp_aux_write(aux, msg);
> -	if (len == 0) {
> -		DRM_ERROR("DP AUX write failed\n");
> -		return -EINVAL;
> -	}
> +	ret = dp_aux_write(aux, msg);
> +	if (ret < 0)
> +		return ret;
> 
> -	timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms);
> -	if (!timeout) {
> -		DRM_ERROR("aux %s timeout\n", (aux->read ? "read" : "write"));
> +	time_left = wait_for_completion_timeout(&aux->comp,
> +						msecs_to_jiffies(250));
> +	if (!time_left)
>  		return -ETIMEDOUT;
> -	}
> -
> -	if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> -		ret = len;
> -	} else {
> -		DRM_ERROR_RATELIMITED("aux err: %s\n",
> -			dp_aux_get_error(aux->aux_error_num));
> -
> -		ret = -EINVAL;
> -	}
> 
>  	return ret;
>  }
> 
> -static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
>  		struct drm_dp_aux_msg *msg)
>  {
>  	u32 data;
> @@ -175,9 +154,10 @@ static void dp_aux_cmd_fifo_rx(struct 
> dp_aux_private *aux,
> 
>  		actual_i = (data >> DP_AUX_DATA_INDEX_OFFSET) & 0xFF;
>  		if (i != actual_i)
> -			DRM_ERROR("Index mismatch: expected %d, found %d\n",
> -				i, actual_i);
> +			break;
>  	}
> +
> +	return i;
>  }
> 
>  static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> *dp_aux,
>  	}
> 
>  	ret = dp_aux_cmd_fifo_tx(aux, msg);
> -
>  	if (ret < 0) {
>  		if (aux->native) {
>  			aux->retry_cnt++;
>  			if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>  				dp_catalog_aux_update_cfg(aux->catalog);
>  		}
> -		usleep_range(400, 500); /* at least 400us to next try */
> -		goto unlock_exit;
> -	}

1) dp_catalog_aux_update_cfg(aux->catalog) will not work without  
dp_catalog_aux_reset(aux->catalog);
dp_catalog_aux_reset(aux->catalog) will reset hpd control block and 
potentially cause pending hpd interrupts got lost.
Therefore I think we should not do 
dp_catalog_aux_update_cfg(aux->catalog) for now.
reset aux controller will reset hpd control block probolem will be fixed 
at next chipset.
after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed 
by dp_catalog_aux_reset(aux->catalog) back at next chipset.

2) according to DP specification, aux read/write failed have to wait at 
least 400us before next try can start.
Otherwise, DP compliant test will failed


> -
> -	if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> -		if (aux->read)
> -			dp_aux_cmd_fifo_rx(aux, msg);
> -
> -		msg->reply = aux->native ?
> -			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>  	} else {
> -		/* Reply defer to retry */
> -		msg->reply = aux->native ?
> -			DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> +		aux->retry_cnt = 0;
> +		switch (aux->aux_error_num) {
> +		case DP_AUX_ERR_NONE:
> +			if (aux->read)
> +				ret = dp_aux_cmd_fifo_rx(aux, msg);
> +			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : 
> DP_AUX_I2C_REPLY_ACK;
> +			break;
> +		case DP_AUX_ERR_DEFER:
> +			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_DEFER :
> DP_AUX_I2C_REPLY_DEFER;
> +			break;
> +		case DP_AUX_ERR_PHY:
> +		case DP_AUX_ERR_ADDR:
> +		case DP_AUX_ERR_NACK:
> +		case DP_AUX_ERR_NACK_DEFER:
> +			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : 
> DP_AUX_I2C_REPLY_NACK;
> +			break;
> +		case DP_AUX_ERR_TOUT:
> +			ret = -ETIMEDOUT;
> +			break;
> +		}
>  	}
> 
> -	/* Return requested size for success or retry */
> -	ret = msg->size;
> -	aux->retry_cnt = 0;
> -
> -unlock_exit:
>  	aux->cmd_busy = false;
>  	mutex_unlock(&aux->mutex);
> +
>  	return ret;
>  }
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h 
> b/drivers/gpu/drm/msm/dp/dp_aux.h
> index f8b8ba919465..0728cc09c9ec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -9,14 +9,6 @@
>  #include "dp_catalog.h"
>  #include <drm/drm_dp_helper.h>
> 
> -#define DP_AUX_ERR_NONE		0
> -#define DP_AUX_ERR_ADDR		-1
> -#define DP_AUX_ERR_TOUT		-2
> -#define DP_AUX_ERR_NACK		-3
> -#define DP_AUX_ERR_DEFER	-4
> -#define DP_AUX_ERR_NACK_DEFER	-5
> -#define DP_AUX_ERR_PHY		-6
> -
>  int dp_aux_register(struct drm_dp_aux *dp_aux);
>  void dp_aux_unregister(struct drm_dp_aux *dp_aux);
>  void dp_aux_isr(struct drm_dp_aux *dp_aux);

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-05-24 09:33:49)
> On 2021-05-07 14:25, Stephen Boyd wrote:
> > @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> > *dp_aux,
> >       }
> >
> >       ret = dp_aux_cmd_fifo_tx(aux, msg);
> > -
> >       if (ret < 0) {
> >               if (aux->native) {
> >                       aux->retry_cnt++;
> >                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> >                               dp_catalog_aux_update_cfg(aux->catalog);
> >               }
> > -             usleep_range(400, 500); /* at least 400us to next try */
> > -             goto unlock_exit;
> > -     }
>
> 1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
> dp_catalog_aux_reset(aux->catalog);
> dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
> potentially cause pending hpd interrupts got lost.
> Therefore I think we should not do
> dp_catalog_aux_update_cfg(aux->catalog) for now.
> reset aux controller will reset hpd control block probolem will be fixed
> at next chipset.
> after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
> by dp_catalog_aux_reset(aux->catalog) back at next chipset.

Hmm ok. So the phy calibration logic that tweaks the tuning values is
never used? Why can't the phy be tuned while it is active? I don't
understand why we would ever want to reset the aux phy when changing the
settings for a retry. Either way, this is not actually changing in this
patch so it would be another patch to remove this code.

>
> 2) according to DP specification, aux read/write failed have to wait at
> least 400us before next try can start.
> Otherwise, DP compliant test will failed

Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
already

                if (ret != 0 && ret != -ETIMEDOUT) {
                        usleep_range(AUX_RETRY_INTERVAL,
                                     AUX_RETRY_INTERVAL + 100);
                }

so this delay here is redundant.

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[khsieh]

On 2021-05-24 12:19, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-05-24 09:33:49)
>> On 2021-05-07 14:25, Stephen Boyd wrote:
>> > @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> > *dp_aux,
>> >       }
>> >
>> >       ret = dp_aux_cmd_fifo_tx(aux, msg);
>> > -
>> >       if (ret < 0) {
>> >               if (aux->native) {
>> >                       aux->retry_cnt++;
>> >                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>> >                               dp_catalog_aux_update_cfg(aux->catalog);
>> >               }
>> > -             usleep_range(400, 500); /* at least 400us to next try */
>> > -             goto unlock_exit;
>> > -     }
>> 
>> 1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
>> dp_catalog_aux_reset(aux->catalog);
>> dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
>> potentially cause pending hpd interrupts got lost.
>> Therefore I think we should not do
>> dp_catalog_aux_update_cfg(aux->catalog) for now.
>> reset aux controller will reset hpd control block probolem will be 
>> fixed
>> at next chipset.
>> after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
>> by dp_catalog_aux_reset(aux->catalog) back at next chipset.
> 
> Hmm ok. So the phy calibration logic that tweaks the tuning values is
> never used? Why can't the phy be tuned while it is active? I don't
> understand why we would ever want to reset the aux phy when changing 
> the
> settings for a retry. Either way, this is not actually changing in this
> patch so it would be another patch to remove this code.
ok,
> 
>> 
>> 2) according to DP specification, aux read/write failed have to wait 
>> at
>> least 400us before next try can start.
>> Otherwise, DP compliant test will failed
> 
> Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
> already
> 
>                 if (ret != 0 && ret != -ETIMEDOUT) {
>                         usleep_range(AUX_RETRY_INTERVAL,
>                                      AUX_RETRY_INTERVAL + 100);
>                 }
> 
> so this delay here is redundant.
yes, you are right. This is enough.

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:
> Let's look at the irq status bits after a transfer and see if we got a
> nack or a defer or a timeout, instead of telling drm layers that
> everything was fine, while still printing an error message. I wasn't
> sure about NACK+DEFER so I lumped all those various errors along with a
> nack so that the drm core can figure out that things are just not going
> well. The important thing is that we're now returning -ETIMEDOUT when
> the message times out and nacks for bad addresses.
> 
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Cc: Kuogee Hsieh <khsieh@codeaurora.org>
> Cc: aravindh@codeaurora.org
> Cc: Sean Paul <sean@poorly.run>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Kuogee Hsieh <khsieh@codeaurora.org>

> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++++++++++++++------------------
>  drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
>  2 files changed, 61 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> b/drivers/gpu/drm/msm/dp/dp_aux.c
> index b49810396513..4a3293b590b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -9,7 +9,15 @@
>  #include "dp_reg.h"
>  #include "dp_aux.h"
> 
> -#define DP_AUX_ENUM_STR(x)		#x
> +enum msm_dp_aux_err {
> +	DP_AUX_ERR_NONE,
> +	DP_AUX_ERR_ADDR,
> +	DP_AUX_ERR_TOUT,
> +	DP_AUX_ERR_NACK,
> +	DP_AUX_ERR_DEFER,
> +	DP_AUX_ERR_NACK_DEFER,
> +	DP_AUX_ERR_PHY,
> +};
> 
>  struct dp_aux_private {
>  	struct device *dev;
> @@ -18,7 +26,7 @@ struct dp_aux_private {
>  	struct mutex mutex;
>  	struct completion comp;
> 
> -	u32 aux_error_num;
> +	enum msm_dp_aux_err aux_error_num;
>  	u32 retry_cnt;
>  	bool cmd_busy;
>  	bool native;
> @@ -33,62 +41,45 @@ struct dp_aux_private {
> 
>  #define MAX_AUX_RETRIES			5
> 
> -static const char *dp_aux_get_error(u32 aux_error)
> -{
> -	switch (aux_error) {
> -	case DP_AUX_ERR_NONE:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
> -	case DP_AUX_ERR_ADDR:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
> -	case DP_AUX_ERR_TOUT:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
> -	case DP_AUX_ERR_NACK:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
> -	case DP_AUX_ERR_DEFER:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
> -	case DP_AUX_ERR_NACK_DEFER:
> -		return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
> -	default:
> -		return "unknown";
> -	}
> -}
> -
> -static u32 dp_aux_write(struct dp_aux_private *aux,
> +static ssize_t dp_aux_write(struct dp_aux_private *aux,
>  			struct drm_dp_aux_msg *msg)
>  {
> -	u32 data[4], reg, len;
> +	u8 data[4];
> +	u32 reg;
> +	ssize_t len;
>  	u8 *msgdata = msg->buffer;
>  	int const AUX_CMD_FIFO_LEN = 128;
>  	int i = 0;
> 
>  	if (aux->read)
> -		len = 4;
> +		len = 0;
>  	else
> -		len = msg->size + 4;
> +		len = msg->size;
> 
>  	/*
>  	 * cmd fifo only has depth of 144 bytes
>  	 * limit buf length to 128 bytes here
>  	 */
> -	if (len > AUX_CMD_FIFO_LEN) {
> +	if (len > AUX_CMD_FIFO_LEN - 4) {
>  		DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
> -		return 0;
> +		return -EINVAL;
>  	}
> 
>  	/* Pack cmd and write to HW */
> -	data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
> +	data[0] = (msg->address >> 16) & 0xf;	/* addr[19:16] */
>  	if (aux->read)
> -		data[0] |=  BIT(4); /* R/W */
> +		data[0] |=  BIT(4);		/* R/W */
> 
> -	data[1] = (msg->address >> 8) & 0xff;	/* addr[15:8] */
> -	data[2] = msg->address & 0xff;		/* addr[7:0] */
> -	data[3] = (msg->size - 1) & 0xff;	/* len[7:0] */
> +	data[1] = msg->address >> 8;		/* addr[15:8] */
> +	data[2] = msg->address;			/* addr[7:0] */
> +	data[3] = msg->size - 1;		/* len[7:0] */
> 
> -	for (i = 0; i < len; i++) {
> +	for (i = 0; i < len + 4; i++) {
>  		reg = (i < 4) ? data[i] : msgdata[i - 4];
> +		reg <<= DP_AUX_DATA_OFFSET;
> +		reg &= DP_AUX_DATA_MASK;
> +		reg |= DP_AUX_DATA_WRITE;
>  		/* index = 0, write */
> -		reg = (((reg) << DP_AUX_DATA_OFFSET)
> -		       & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
>  		if (i == 0)
>  			reg |= DP_AUX_DATA_INDEX_WRITE;
>  		aux->catalog->aux_data = reg;
> @@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private 
> *aux,
>  	return len;
>  }
> 
> -static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>  			      struct drm_dp_aux_msg *msg)
>  {
> -	u32 ret, len, timeout;
> -	int aux_timeout_ms = HZ/4;
> +	ssize_t ret;
> +	unsigned long time_left;
> 
>  	reinit_completion(&aux->comp);
> 
> -	len = dp_aux_write(aux, msg);
> -	if (len == 0) {
> -		DRM_ERROR("DP AUX write failed\n");
> -		return -EINVAL;
> -	}
> +	ret = dp_aux_write(aux, msg);
> +	if (ret < 0)
> +		return ret;
> 
> -	timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms);
> -	if (!timeout) {
> -		DRM_ERROR("aux %s timeout\n", (aux->read ? "read" : "write"));
> +	time_left = wait_for_completion_timeout(&aux->comp,
> +						msecs_to_jiffies(250));
> +	if (!time_left)
>  		return -ETIMEDOUT;
> -	}
> -
> -	if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> -		ret = len;
> -	} else {
> -		DRM_ERROR_RATELIMITED("aux err: %s\n",
> -			dp_aux_get_error(aux->aux_error_num));
> -
> -		ret = -EINVAL;
> -	}
> 
>  	return ret;
>  }
> 
> -static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
> +static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private *aux,
>  		struct drm_dp_aux_msg *msg)
>  {
>  	u32 data;
> @@ -175,9 +154,10 @@ static void dp_aux_cmd_fifo_rx(struct 
> dp_aux_private *aux,
> 
>  		actual_i = (data >> DP_AUX_DATA_INDEX_OFFSET) & 0xFF;
>  		if (i != actual_i)
> -			DRM_ERROR("Index mismatch: expected %d, found %d\n",
> -				i, actual_i);
> +			break;
>  	}
> +
> +	return i;
>  }
> 
>  static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> *dp_aux,
>  	}
> 
>  	ret = dp_aux_cmd_fifo_tx(aux, msg);
> -
>  	if (ret < 0) {
>  		if (aux->native) {
>  			aux->retry_cnt++;
>  			if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>  				dp_catalog_aux_update_cfg(aux->catalog);
>  		}
> -		usleep_range(400, 500); /* at least 400us to next try */
> -		goto unlock_exit;
> -	}
> -
> -	if (aux->aux_error_num == DP_AUX_ERR_NONE) {
> -		if (aux->read)
> -			dp_aux_cmd_fifo_rx(aux, msg);
> -
> -		msg->reply = aux->native ?
> -			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>  	} else {
> -		/* Reply defer to retry */
> -		msg->reply = aux->native ?
> -			DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> +		aux->retry_cnt = 0;
> +		switch (aux->aux_error_num) {
> +		case DP_AUX_ERR_NONE:
> +			if (aux->read)
> +				ret = dp_aux_cmd_fifo_rx(aux, msg);
> +			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : 
> DP_AUX_I2C_REPLY_ACK;
> +			break;
> +		case DP_AUX_ERR_DEFER:
> +			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_DEFER :
> DP_AUX_I2C_REPLY_DEFER;
> +			break;
> +		case DP_AUX_ERR_PHY:
> +		case DP_AUX_ERR_ADDR:
> +		case DP_AUX_ERR_NACK:
> +		case DP_AUX_ERR_NACK_DEFER:
> +			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : 
> DP_AUX_I2C_REPLY_NACK;
> +			break;
> +		case DP_AUX_ERR_TOUT:
> +			ret = -ETIMEDOUT;
> +			break;
> +		}
>  	}
> 
> -	/* Return requested size for success or retry */
> -	ret = msg->size;
> -	aux->retry_cnt = 0;
> -
> -unlock_exit:
>  	aux->cmd_busy = false;
>  	mutex_unlock(&aux->mutex);
> +
>  	return ret;
>  }
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h 
> b/drivers/gpu/drm/msm/dp/dp_aux.h
> index f8b8ba919465..0728cc09c9ec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -9,14 +9,6 @@
>  #include "dp_catalog.h"
>  #include <drm/drm_dp_helper.h>
> 
> -#define DP_AUX_ERR_NONE		0
> -#define DP_AUX_ERR_ADDR		-1
> -#define DP_AUX_ERR_TOUT		-2
> -#define DP_AUX_ERR_NACK		-3
> -#define DP_AUX_ERR_DEFER	-4
> -#define DP_AUX_ERR_NACK_DEFER	-5
> -#define DP_AUX_ERR_PHY		-6
> -
>  int dp_aux_register(struct drm_dp_aux *dp_aux);
>  void dp_aux_unregister(struct drm_dp_aux *dp_aux);
>  void dp_aux_isr(struct drm_dp_aux *dp_aux);

Re: [PATCH 0/3] drm/msm/dp: Simplify aux code

[Stephen Boyd]

Quoting Stephen Boyd (2021-05-07 14:25:02)
> Here's a few patches that simplify the aux handling code and bubble up
> timeouts and nacks to the upper DRM layers. The goal is to get DRM to
> know that the other side isn't there or that there's been a timeout,
> instead of saying that everything is fine and putting some error message
> into the logs.
>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Cc: Kuogee Hsieh <khsieh@codeaurora.org>
> Cc: aravindh@codeaurora.org
> Cc: Sean Paul <sean@poorly.run>
>

Kuogee, have you had a change to review this series?

> Stephen Boyd (3):
>   drm/msm/dp: Simplify aux irq handling code
>   drm/msm/dp: Shrink locking area of dp_aux_transfer()
>   drm/msm/dp: Handle aux timeouts, nacks, defers
>
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 181 ++++++++++++----------------
>  drivers/gpu/drm/msm/dp/dp_aux.h     |   8 --
>  drivers/gpu/drm/msm/dp/dp_catalog.c |   2 +-
>  drivers/gpu/drm/msm/dp/dp_catalog.h |   2 +-
>  4 files changed, 80 insertions(+), 113 deletions(-)
>
>
> base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
> --
> https://chromeos.dev
>

Re: [PATCH 0/3] drm/msm/dp: Simplify aux code

[khsieh]

On 2021-05-21 14:57, Stephen Boyd wrote:
> Quoting Stephen Boyd (2021-05-07 14:25:02)
>> Here's a few patches that simplify the aux handling code and bubble up
>> timeouts and nacks to the upper DRM layers. The goal is to get DRM to
>> know that the other side isn't there or that there's been a timeout,
>> instead of saying that everything is fine and putting some error 
>> message
>> into the logs.
>> 
>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
>> Cc: Kuogee Hsieh <khsieh@codeaurora.org>
>> Cc: aravindh@codeaurora.org
>> Cc: Sean Paul <sean@poorly.run>
>> 
> 
> Kuogee, have you had a change to review this series?
> 
Sorry  missed this one.
Will review it now.
>> Stephen Boyd (3):
>>   drm/msm/dp: Simplify aux irq handling code
>>   drm/msm/dp: Shrink locking area of dp_aux_transfer()
>>   drm/msm/dp: Handle aux timeouts, nacks, defers
>> 
>>  drivers/gpu/drm/msm/dp/dp_aux.c     | 181 
>> ++++++++++++----------------
>>  drivers/gpu/drm/msm/dp/dp_aux.h     |   8 --
>>  drivers/gpu/drm/msm/dp/dp_catalog.c |   2 +-
>>  drivers/gpu/drm/msm/dp/dp_catalog.h |   2 +-
>>  4 files changed, 80 insertions(+), 113 deletions(-)
>> 
>> 
>> base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
>> --
>> https://chromeos.dev
>>