diff mbox series

[4/4] drm/bridge: ti-sn65dsi86: Update reply on aux failures

Message ID 20201029011154.1515687-5-swboyd@chromium.org
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Support EDID reading | expand

Commit Message

Stephen Boyd Oct. 29, 2020, 1:11 a.m. UTC
We should be setting the drm_dp_aux_msg::reply field if a NACK or a
SHORT reply happens. Update the error bit handling logic in
ti_sn_aux_transfer() to handle these cases and notify upper layers that
such errors have happened. This helps the retry logic understand that a
timeout has happened, or to shorten the read length if the panel isn't
able to handle the longest read possible.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 31 +++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

Comments

Doug Anderson Oct. 29, 2020, 4:22 p.m. UTC | #1
Hi,

On Wed, Oct 28, 2020 at 6:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> We should be setting the drm_dp_aux_msg::reply field if a NACK or a
> SHORT reply happens.

I don't think you update the "reply" field for SHORT, right?  You just
return a different size?


> Update the error bit handling logic in
> ti_sn_aux_transfer() to handle these cases and notify upper layers that
> such errors have happened. This helps the retry logic understand that a
> timeout has happened, or to shorten the read length if the panel isn't
> able to handle the longest read possible.
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 31 +++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 6b6e98ca2881..19737bc01b8f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -878,6 +878,7 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
>         case DP_AUX_NATIVE_READ:
>         case DP_AUX_I2C_READ:
>                 regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
> +               msg->reply = 0; /* Assume it's good */
>                 break;
>         default:
>                 return -EINVAL;
> @@ -909,10 +910,32 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
>         ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
>         if (ret)
>                 return ret;
> -       else if ((val & AUX_IRQ_STATUS_NAT_I2C_FAIL)
> -                || (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT)
> -                || (val & AUX_IRQ_STATUS_AUX_SHORT))
> -               return -ENXIO;
> +
> +       if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
> +               /*
> +                * The hardware tried the message seven times per the DP spec
> +                * but it hit a timeout. We ignore defers here because they're
> +                * handled in hardware.
> +                */
> +               return -ETIMEDOUT;
> +       }
> +       if (val & AUX_IRQ_STATUS_AUX_SHORT) {
> +               ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
> +               if (ret)
> +                       return ret;

IIUC, your digging through the code showed that in order to fully
handle the "SHORT" case you also needed to add support for
"DP_AUX_I2C_WRITE_STATUS_UPDATE", right?

Even without handling "DP_AUX_I2C_WRITE_STATUS_UPDATE" though, this
patch seems to be an improvement and I'd support landing it.

Oh, I guess one other thing: I think this is all from code inspection,
right?  You didn't manage to reproduce anything that would tickle one
of these code paths?  Might be worth mentioning, even if "after the
cut"?

-Doug
Stephen Boyd Oct. 29, 2020, 8:24 p.m. UTC | #2
Quoting Doug Anderson (2020-10-29 09:22:55)
> Hi,
> 
> On Wed, Oct 28, 2020 at 6:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > We should be setting the drm_dp_aux_msg::reply field if a NACK or a
> > SHORT reply happens.
> 
> I don't think you update the "reply" field for SHORT, right?  You just
> return a different size?

Correct.

> 
> 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 6b6e98ca2881..19737bc01b8f 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -909,10 +910,32 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
> >         ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
> >         if (ret)
> >                 return ret;
> > -       else if ((val & AUX_IRQ_STATUS_NAT_I2C_FAIL)
> > -                || (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT)
> > -                || (val & AUX_IRQ_STATUS_AUX_SHORT))
> > -               return -ENXIO;
> > +
> > +       if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
> > +               /*
> > +                * The hardware tried the message seven times per the DP spec
> > +                * but it hit a timeout. We ignore defers here because they're
> > +                * handled in hardware.
> > +                */
> > +               return -ETIMEDOUT;
> > +       }
> > +       if (val & AUX_IRQ_STATUS_AUX_SHORT) {
> > +               ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
> > +               if (ret)
> > +                       return ret;
> 
> IIUC, your digging through the code showed that in order to fully
> handle the "SHORT" case you also needed to add support for
> "DP_AUX_I2C_WRITE_STATUS_UPDATE", right?

Oh yeah. If a short reply happens and it is aux over i2c then
drm_dp_i2c_msg_write_status_update() is called and
DP_AUX_I2C_WRITE_STATUS_UPDATE is set and then we try a transfer again.
We need to handle that type of request in this ti_sn_aux_transfer()
function.

> 
> Even without handling "DP_AUX_I2C_WRITE_STATUS_UPDATE" though, this
> patch seems to be an improvement and I'd support landing it.
> 
> Oh, I guess one other thing: I think this is all from code inspection,
> right?  You didn't manage to reproduce anything that would tickle one
> of these code paths?  Might be worth mentioning, even if "after the
> cut"?
> 

Yes, just code inspection. I can add that detail to the commit text.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 6b6e98ca2881..19737bc01b8f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -878,6 +878,7 @@  static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	case DP_AUX_NATIVE_READ:
 	case DP_AUX_I2C_READ:
 		regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
+		msg->reply = 0; /* Assume it's good */
 		break;
 	default:
 		return -EINVAL;
@@ -909,10 +910,32 @@  static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
 	if (ret)
 		return ret;
-	else if ((val & AUX_IRQ_STATUS_NAT_I2C_FAIL)
-		 || (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT)
-		 || (val & AUX_IRQ_STATUS_AUX_SHORT))
-		return -ENXIO;
+
+	if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
+		/*
+		 * The hardware tried the message seven times per the DP spec
+		 * but it hit a timeout. We ignore defers here because they're
+		 * handled in hardware.
+		 */
+		return -ETIMEDOUT;
+	}
+	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
+		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
+		if (ret)
+			return ret;
+	} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
+		switch (request) {
+		case DP_AUX_I2C_WRITE:
+		case DP_AUX_I2C_READ:
+			msg->reply |= DP_AUX_I2C_REPLY_NACK;
+			break;
+		case DP_AUX_NATIVE_READ:
+		case DP_AUX_NATIVE_WRITE:
+			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
+			break;
+		}
+		return 0;
+	}
 
 	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
 	    len == 0)