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);

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);

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);

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.