[PATCH v2] drm/bridge: ti-sn65dsi86: Decouple DP output lanes from DSI input lanes

[PATCH v2] drm/bridge: ti-sn65dsi86: Decouple DP output lanes from DSI input lanes

[Jeffrey Hugo]

Based on work by Bjorn Andersson <bjorn.andersson@linaro.org>

The bridge can be configured to use 1, 2, or 4 DP lanes.  This
configuration is independent of the input DSI lanes.  Right now, the
driver assumes that there is 1:1 mapping of input lanes to output lanes
which is not correct and does not work for manu devices such as the
Lenovo Miix 630 and Lenovo Yoga C630 laptops.

The bridge can also be configured to use one of a number of data rates on
the DP lanes.  Currently any of the supported rates is considered valid,
however the configured rate must also be supported by the connected panel,
and not all rates are supported or even valid for any particular panel.

Luckily, we can determine what we need at runtime by reading the DPCD from
the attached panel.  DPCD will tell us the maximum number of supported
lanes, and the supported data rates.

Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---

Bjorn, I think this should address the issue you pointed out concerning
the data rate glitch I missed in your origional work.  Would you kindly
give this a test and let me know if it appears to address all of the
issues you were working around?

v2:
-Use DPCD instead of DT to address the issue of some panels not
supporting all the rates

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 97 ++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 43abf01ebd4c..72bacca8d49a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -100,6 +100,7 @@ struct ti_sn_bridge {
 	struct drm_panel		*panel;
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
+	int				dp_lanes;
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -432,6 +433,8 @@ static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
 	unsigned int val, i;
 	struct drm_display_mode *mode =
 		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+	u8 dpcd_val;
+	u8 rate_valid[8] = {0};
 
 	/* set DSIA clk frequency */
 	bit_rate_mhz = (mode->clock / 1000) *
@@ -444,10 +447,91 @@ static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
 	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
 
 	/* set DP data rate */
-	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
+	dp_rate_mhz = ((bit_rate_mhz / pdata->dp_lanes) * DP_CLK_FUDGE_NUM) /
 							DP_CLK_FUDGE_DEN;
+
+	/* read the panel capabilities to determine valid supported rates */
+	val = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &dpcd_val);
+	if (!val) {
+		DRM_ERROR("Reading max link rate from DPCD failed\n");
+		return;
+	}
+
+	if (dpcd_val) {
+		/* cap to the max rate supported by the bridge */
+		if (dpcd_val > 0x14)
+			dpcd_val = 0x14;
+
+		switch (dpcd_val) {
+		case 0x14:
+			rate_valid[7] = 1;
+			/* fall through */
+		case 0xa:
+			rate_valid[4] = 1;
+			/* fall through */
+		case 0x6:
+			rate_valid[1] = 1;
+			break;
+		default:
+			DRM_ERROR("Invalid max link rate from DPCD:%x\n",
+				  dpcd_val);
+			return;
+		}
+	} else {
+		/* eDP 1.4 devices can provide a custom table */
+		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
+
+		val = drm_dp_dpcd_readb(&pdata->aux, DP_EDP_DPCD_REV, &dpcd_val);
+		if (!val) {
+			DRM_ERROR("Reading eDP rev from DPCD failed\n");
+			return;
+		}
+
+		if (dpcd_val < DP_EDP_14) {
+			DRM_ERROR("eDP 1.4 supported link rates specified from non-1.4 device\n");
+			return;
+		}
+
+		drm_dp_dpcd_read(&pdata->aux, DP_SUPPORTED_LINK_RATES,
+			      sink_rates, sizeof(sink_rates));
+
+		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
+			val = le16_to_cpu(sink_rates[i]);
+
+			if (!val)
+				break;
+
+			switch (val) {
+			case 27000:
+				rate_valid[7] = 1;
+				break;
+			case 21600:
+				rate_valid[6] = 1;
+				break;
+			case 16200:
+				rate_valid[5] = 1;
+				break;
+			case 13500:
+				rate_valid[4] = 1;
+				break;
+			case 12150:
+				rate_valid[3] = 1;
+				break;
+			case 10800:
+				rate_valid[2] = 1;
+				break;
+			case 8100:
+				rate_valid[1] = 1;
+				break;
+			default:
+				/* unsupported */
+				break;
+			}
+		}
+	}
+
 	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
-		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
+		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz && rate_valid[i])
 			break;
 
 	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
@@ -505,7 +589,14 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 			   CHA_DSI_LANES_MASK, val);
 
 	/* DP lane config */
-	val = DP_NUM_LANES(pdata->dsi->lanes - 1);
+	ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LANE_COUNT, (u8 *)&val);
+	if (!ret) {
+		DRM_ERROR("Reading lane count from DPCD failed\n");
+		return;
+	}
+	pdata->dp_lanes = val & DP_MAX_LANE_COUNT_MASK;
+	/* 4 lanes are encoded with the value "3" */
+	val = DP_NUM_LANES(pdata->dp_lanes == 4 ? 3 : pdata->dp_lanes);
 	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
 			   val);
 
-- 
2.17.1

Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Decouple DP output lanes from DSI input lanes

[Doug Anderson]

Hi,

On Tue, Oct 22, 2019 at 12:01 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>
> Based on work by Bjorn Andersson <bjorn.andersson@linaro.org>
>
> The bridge can be configured to use 1, 2, or 4 DP lanes.  This
> configuration is independent of the input DSI lanes.  Right now, the
> driver assumes that there is 1:1 mapping of input lanes to output lanes
> which is not correct and does not work for manu devices such as the
> Lenovo Miix 630 and Lenovo Yoga C630 laptops.
>
> The bridge can also be configured to use one of a number of data rates on
> the DP lanes.  Currently any of the supported rates is considered valid,
> however the configured rate must also be supported by the connected panel,
> and not all rates are supported or even valid for any particular panel.
>
> Luckily, we can determine what we need at runtime by reading the DPCD from
> the attached panel.  DPCD will tell us the maximum number of supported
> lanes, and the supported data rates.
>
> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> ---
>
> Bjorn, I think this should address the issue you pointed out concerning
> the data rate glitch I missed in your origional work.  Would you kindly
> give this a test and let me know if it appears to address all of the
> issues you were working around?
>
> v2:
> -Use DPCD instead of DT to address the issue of some panels not
> supporting all the rates
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 97 ++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 3 deletions(-)

Overall I'm suggesting moving over to my series and I've taken the
best stuff from your patch and put it atop my series.  Please yell if
you disagree.  You can find the cover letter for my v2 at:

https://lore.kernel.org/r/20191218004741.102067-1-dianders@chromium.org

A few misc comments below in any case.


> @@ -444,10 +447,91 @@ static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
>         regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>
>         /* set DP data rate */
> -       dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
> +       dp_rate_mhz = ((bit_rate_mhz / pdata->dp_lanes) * DP_CLK_FUDGE_NUM) /
>                                                         DP_CLK_FUDGE_DEN;

One note is that "bit_rate_mhz" is still calculated using the MIPI
pixel format, which is wrong.  It happens that (at the moment) we have
24 bits per pixel for both cases, though.


> +       /* read the panel capabilities to determine valid supported rates */
> +       val = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &dpcd_val);
> +       if (!val) {
> +               DRM_ERROR("Reading max link rate from DPCD failed\n");
> +               return;
> +       }
> +
> +       if (dpcd_val) {

I think your patch is assuming that the only case you want to use the
table is if dpcd_val is 0.  This doesn't appear to be the case.  In
Table 4-24 of the spec it states that you can have a non-zero value
here and still provide a table.  That might be useful if you want to
be backward compatible with an eDP 1.3 source but also provide an
optimized rate for a eDP 1.4 source.

We should be checking the eDP revision first and always using the
table if it's eDP 1.4.


> +               /* cap to the max rate supported by the bridge */
> +               if (dpcd_val > 0x14)
> +                       dpcd_val = 0x14;
> +
> +               switch (dpcd_val) {
> +               case 0x14:

There are constants.  Like DP_LINK_BW_5_4


> @@ -505,7 +589,14 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>                            CHA_DSI_LANES_MASK, val);
>
>         /* DP lane config */
> -       val = DP_NUM_LANES(pdata->dsi->lanes - 1);
> +       ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LANE_COUNT, (u8 *)&val);

This is an iffy cast.  The function is only guaranteed to set the
first byte of val which could be the most or least significant byte.
Other bytes will remain as-is.