[PATCH 00/21] drm/bridge: tc358767: DP support

[PATCH 00/21] drm/bridge: tc358767: DP support

[Tomi Valkeinen]

Hi,

tc358767 bridge was originally implemented for eDP use with an embedded
panel. I've been working to add DP support, and this series is the
result. I did have a lot of issues with link training, but with these,
it's been working reliably with my devices.

A downside on my hardware is that there's no HPD interrupt, so this uses
polling. I believe the polling support could be improved by using the
INTSTS_G register to find out it there has been plug-ins/outs, but for
now this does the HPD handling the naive way.

 Tomi

Tomi Valkeinen (21):
  drm/bridge: tc358767: fix tc_aux_get_status error handling
  drm/bridge: tc358767: reset voltage-swing & pre-emphasis
  drm/bridge: tc358767: fix ansi 8b10b use
  drm/bridge: tc358767: cleanup spread & scrambler_dis
  drm/bridge: tc358767: remove unused swing & preemp
  drm/bridge: tc358767: cleanup aux_link_setup
  drm/bridge: tc358767: move video stream setup to tc_main_link_stream
  drm/bridge: tc358767: split stream enable/disable
  drm/bridge: tc358767: move PXL PLL enable/disable to stream
    enable/disable
  drm/bridge: tc358767: add link disable function
  drm/bridge: tc358767: ensure DP is disabled before LT
  drm/bridge: tc358767: remove unnecessary msleep
  drm/bridge: tc358767: use more reliable seq when finishing LT
  drm/bridge: tc358767: cleanup LT result check
  drm/bridge: tc358767: clean-up link training
  drm/bridge: tc358767: remove check for video mode in link enable
  drm/bridge: tc358767: use bridge mode_valid
  drm/bridge: tc358767: remove tc_connector_best_encoder
  drm/bridge: tc358767: copy the mode data, instead of storing the
    pointer
  drm/bridge: tc358767: add GPIO & interrupt registers
  drm/bridge: tc358767: implement naive HPD handling

 drivers/gpu/drm/bridge/tc358767.c | 457 ++++++++++++++++--------------
 1 file changed, 249 insertions(+), 208 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/21] drm/bridge: tc358767: fix tc_aux_get_status error handling

[Tomi Valkeinen]

tc_aux_get_status() does not report AUX_TIMEOUT correctly, as it only
checks the AUX_TIMEOUT if aux is still busy. Fix this by always checking
for AUX_TIMEOUT.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 888980d4bc74..7031c4f52c57 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -286,14 +286,17 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
 	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
 	if (ret < 0)
 		return ret;
+
 	if (value & AUX_BUSY) {
-		if (value & AUX_TIMEOUT) {
-			dev_err(tc->dev, "i2c access timeout!\n");
-			return -ETIMEDOUT;
-		}
+		dev_err(tc->dev, "aux busy!\n");
 		return -EBUSY;
 	}
 
+	if (value & AUX_TIMEOUT) {
+		dev_err(tc->dev, "aux access timeout!\n");
+		return -ETIMEDOUT;
+	}
+
 	*reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
 	return 0;
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/21] drm/bridge: tc358767: reset voltage-swing & pre-emphasis

[Tomi Valkeinen]

We need to reset DPCD voltage-swing & pre-emphasis before starting the
link training, as otherwise tc358767 will use the previous values as
minimums.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7031c4f52c57..11a50f7bb4be 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
+	// Reset voltage-swing & pre-emphasis
+	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
+	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
+	if (ret < 0)
+		goto err_dpcd_write;
+
 	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
 	if (ret)
 		goto err;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/21] drm/bridge: tc358767: fix ansi 8b10b use

[Tomi Valkeinen]

DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
the ANSI 8B10B bit set in DPCD, even if it should always be set.

The tc358767 driver currently respects that flag, and turns the encoding
off if the monitor does not have the bit set, which then results in the
monitor not working.

This patch makes the driver to always use ANSI 8B10B encoding, and drops
the 'coding8b10b' field which is no longer used.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 11a50f7bb4be..163c594fa6ac 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -188,7 +188,6 @@ struct tc_edp_link {
 	u8			assr;
 	int			scrambler_dis;
 	int			spread;
-	int			coding8b10b;
 	u8			swing;
 	u8			preemp;
 };
@@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
 	 * No training pattern, skew lane 1 data by two LSCLK cycles with
 	 * respect to lane 0 data, AutoCorrect Mode = 0
 	 */
-	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
+	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
 
 	if (tc->link.scrambler_dis)
 		reg |= DP0_SRCCTRL_SCRMBLDIS;	/* Scrambler Disabled */
-	if (tc->link.coding8b10b)
-		/* Enable 8/10B Encoder (TxData[19:16] not used) */
-		reg |= DP0_SRCCTRL_EN810B;
 	if (tc->link.spread)
 		reg |= DP0_SRCCTRL_SSCG;	/* Spread Spectrum Enable */
 	if (tc->link.base.num_lanes == 2)
@@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.coding8b10b = tmp[0] & BIT(0);
+
 	tc->link.scrambler_dis = 0;
 	/* read assr */
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
@@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.num_lanes,
 		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
 		"enhanced" : "non-enhanced");
-	dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
 	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
 		tc->link.assr, tc->assr);
 
@@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
 	/* DOWNSPREAD_CTRL */
 	tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
 	/* MAIN_LINK_CHANNEL_CODING_SET */
-	tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
+	tmp[1] =  DP_SET_ANSI_8B10B;
 	ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
 	if (ret < 0)
 		goto err_dpcd_write;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/21] drm/bridge: tc358767: cleanup spread & scrambler_dis

[Tomi Valkeinen]

Minor cleanups:
- Use bool for boolean fields
- Use DP_MAX_DOWNSPREAD_0_5 instead of BIT(0)
- debug print down-spread and scrambler status

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 163c594fa6ac..8e53073f0e35 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -186,8 +186,8 @@ module_param_named(test, tc_test_pattern, bool, 0644);
 struct tc_edp_link {
 	struct drm_dp_link	base;
 	u8			assr;
-	int			scrambler_dis;
-	int			spread;
+	bool			scrambler_dis;
+	bool			spread;
 	u8			swing;
 	u8			preemp;
 };
@@ -626,13 +626,13 @@ static int tc_get_display_props(struct tc_data *tc)
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.spread = tmp[0] & BIT(0); /* 0.5% down spread */
+	tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
 
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
 
-	tc->link.scrambler_dis = 0;
+	tc->link.scrambler_dis = false;
 	/* read assr */
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
 	if (ret < 0)
@@ -645,6 +645,9 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.num_lanes,
 		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
 		"enhanced" : "non-enhanced");
+	dev_dbg(tc->dev, "Downspread: %s, scrambler: %s\n",
+		tc->link.spread ? "0.5%" : "0.0%",
+		tc->link.scrambler_dis ? "disabled" : "enabled");
 	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
 		tc->link.assr, tc->assr);
 
@@ -934,7 +937,7 @@ static int tc_main_link_setup(struct tc_data *tc)
 			dev_dbg(dev, "Failed to switch display ASSR to %d, falling back to unscrambled mode\n",
 				 tc->assr);
 			/* trying with disabled scrambler */
-			tc->link.scrambler_dis = 1;
+			tc->link.scrambler_dis = true;
 		}
 	}
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/21] drm/bridge: tc358767: remove unused swing & preemp

[Tomi Valkeinen]

swing and preemp fields are not used. Remove them.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8e53073f0e35..7ef8d754b4ff 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -188,8 +188,6 @@ struct tc_edp_link {
 	u8			assr;
 	bool			scrambler_dis;
 	bool			spread;
-	u8			swing;
-	u8			preemp;
 };
 
 struct tc_data {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/21] drm/bridge: tc358767: cleanup aux_link_setup

[Tomi Valkeinen]

Modify aux_link_setup so that it does not use tc->link, and thus makes
aux setup independent of the link probing.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7ef8d754b4ff..f5c232a9064e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	unsigned long rate;
 	u32 value;
 	int ret;
-	u32 dp_phy_ctrl;
 
 	rate = clk_get_rate(tc->refclk);
 	switch (rate) {
@@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
 	tc_write(SYS_PLLPARAM, value);
 
-	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
-	if (tc->link.base.num_lanes == 2)
-		dp_phy_ctrl |= PHY_2LANE;
-	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
+	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
 
 	/*
 	 * Initially PLLs are in bypass. Force PLL parameter update,
@@ -587,8 +583,9 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	if (ret == -ETIMEDOUT) {
 		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
 		return ret;
-	} else if (ret)
+	} else if (ret) {
 		goto err;
+	}
 
 	/* Setup AUX link */
 	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/21] drm/bridge: tc358767: move video stream setup to tc_main_link_stream

[Tomi Valkeinen]

The driver currently sets the video stream registers in
tc_main_link_setup. One should be able to establish the DP link without
any video stream, so a more logical place is to configure the stream in
the tc_main_link_stream. So move them there.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f5c232a9064e..86b272422281 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc)
 		return -EAGAIN;
 	}
 
-	ret = tc_set_video_mode(tc, tc->mode);
-	if (ret)
-		goto err;
-
-	/* Set M/N */
-	ret = tc_stream_clock_calc(tc);
-	if (ret)
-		goto err;
-
 	return 0;
 err_dpcd_read:
 	dev_err(tc->dev, "Failed to read DPCD: %d\n", ret);
@@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state)
 	dev_dbg(tc->dev, "stream: %d\n", state);
 
 	if (state) {
+		ret = tc_set_video_mode(tc, tc->mode);
+		if (ret)
+			goto err;
+
+		/* Set M/N */
+		ret = tc_stream_clock_calc(tc);
+		if (ret)
+			goto err;
+
 		value = VID_MN_GEN | DP_EN;
 		if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
 			value |= EF_EN;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/21] drm/bridge: tc358767: split stream enable/disable

[Tomi Valkeinen]

It is nicer to have enable/disable functions instead of set(bool enable)
style function.

Split tc_main_link_stream into tc_stream_enable and tc_stream_disable.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++--------------
 1 file changed, 45 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 86b272422281..bfc673bd5986 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc)
 	return ret;
 }
 
-static int tc_main_link_stream(struct tc_data *tc, int state)
+static int tc_stream_enable(struct tc_data *tc)
 {
 	int ret;
 	u32 value;
 
-	dev_dbg(tc->dev, "stream: %d\n", state);
+	dev_dbg(tc->dev, "stream enable\n");
 
-	if (state) {
-		ret = tc_set_video_mode(tc, tc->mode);
-		if (ret)
-			goto err;
+	ret = tc_set_video_mode(tc, tc->mode);
+	if (ret)
+		goto err;
 
-		/* Set M/N */
-		ret = tc_stream_clock_calc(tc);
-		if (ret)
-			goto err;
+	/* Set M/N */
+	ret = tc_stream_clock_calc(tc);
+	if (ret)
+		goto err;
 
-		value = VID_MN_GEN | DP_EN;
-		if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
-			value |= EF_EN;
-		tc_write(DP0CTL, value);
-		/*
-		 * VID_EN assertion should be delayed by at least N * LSCLK
-		 * cycles from the time VID_MN_GEN is enabled in order to
-		 * generate stable values for VID_M. LSCLK is 270 MHz or
-		 * 162 MHz, VID_N is set to 32768 in  tc_stream_clock_calc(),
-		 * so a delay of at least 203 us should suffice.
-		 */
-		usleep_range(500, 1000);
-		value |= VID_EN;
-		tc_write(DP0CTL, value);
-		/* Set input interface */
-		value = DP0_AUDSRC_NO_INPUT;
-		if (tc_test_pattern)
-			value |= DP0_VIDSRC_COLOR_BAR;
-		else
-			value |= DP0_VIDSRC_DPI_RX;
-		tc_write(SYSCTRL, value);
-	} else {
-		tc_write(DP0CTL, 0);
-	}
+	value = VID_MN_GEN | DP_EN;
+	if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+		value |= EF_EN;
+	tc_write(DP0CTL, value);
+	/*
+	 * VID_EN assertion should be delayed by at least N * LSCLK
+	 * cycles from the time VID_MN_GEN is enabled in order to
+	 * generate stable values for VID_M. LSCLK is 270 MHz or
+	 * 162 MHz, VID_N is set to 32768 in  tc_stream_clock_calc(),
+	 * so a delay of at least 203 us should suffice.
+	 */
+	usleep_range(500, 1000);
+	value |= VID_EN;
+	tc_write(DP0CTL, value);
+	/* Set input interface */
+	value = DP0_AUDSRC_NO_INPUT;
+	if (tc_test_pattern)
+		value |= DP0_VIDSRC_COLOR_BAR;
+	else
+		value |= DP0_VIDSRC_DPI_RX;
+	tc_write(SYSCTRL, value);
+
+	return 0;
+err:
+	return ret;
+}
+
+static int tc_stream_disable(struct tc_data *tc)
+{
+	int ret;
+
+	dev_dbg(tc->dev, "stream disable\n");
+
+	tc_write(DP0CTL, 0);
 
 	return 0;
 err:
@@ -1078,7 +1087,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 		return;
 	}
 
-	ret = tc_main_link_stream(tc, 1);
+	ret = tc_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
 		return;
@@ -1094,7 +1103,7 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 
 	drm_panel_disable(tc->panel);
 
-	ret = tc_main_link_stream(tc, 0);
+	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/21] drm/bridge: tc358767: move PXL PLL enable/disable to stream enable/disable

[Tomi Valkeinen]

We set up the PXL PLL inside tc_main_link_setup. This is unnecessary,
and makes tc_main_link_setup depend on the video-mode, which should not
be the case. As PXL PLL is used only for the video stream (and only when
using the HW test pattern), let's move the PXL PLL setup into
tc_stream_enable.

Also, currently the PXL PLL is only disabled if the driver if removed.
Let's disable the PXL PLL when the stream is disabled.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index bfc673bd5986..f8039149a4e8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -877,14 +877,6 @@ static int tc_main_link_setup(struct tc_data *tc)
 	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
 	tc_wait_pll_lock(tc);
 
-	/* PXL PLL setup */
-	if (tc_test_pattern) {
-		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
-				    1000 * tc->mode->clock);
-		if (ret)
-			goto err;
-	}
-
 	/* Reset/Enable Main Links */
 	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
 	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
@@ -1020,6 +1012,14 @@ static int tc_stream_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "stream enable\n");
 
+	/* PXL PLL setup */
+	if (tc_test_pattern) {
+		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
+				    1000 * tc->mode->clock);
+		if (ret)
+			goto err;
+	}
+
 	ret = tc_set_video_mode(tc, tc->mode);
 	if (ret)
 		goto err;
@@ -1064,6 +1064,8 @@ static int tc_stream_disable(struct tc_data *tc)
 
 	tc_write(DP0CTL, 0);
 
+	tc_pxl_pll_dis(tc);
+
 	return 0;
 err:
 	return ret;
@@ -1390,8 +1392,6 @@ static int tc_remove(struct i2c_client *client)
 	drm_bridge_remove(&tc->bridge);
 	drm_dp_aux_unregister(&tc->aux);
 
-	tc_pxl_pll_dis(tc);
-
 	return 0;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/21] drm/bridge: tc358767: add link disable function

[Tomi Valkeinen]

Currently we have tc_main_link_setup(), which configures and enabled the
link, but we have no counter-part for disabling the link.

Add tc_main_link_disable, and rename tc_main_link_setup to
tc_main_link_enable.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f8039149a4e8..fe5fd7db0247 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -822,7 +822,7 @@ static int tc_link_training(struct tc_data *tc, int pattern)
 	return ret;
 }
 
-static int tc_main_link_setup(struct tc_data *tc)
+static int tc_main_link_enable(struct tc_data *tc)
 {
 	struct drm_dp_aux *aux = &tc->aux;
 	struct device *dev = tc->dev;
@@ -837,6 +837,8 @@ static int tc_main_link_setup(struct tc_data *tc)
 	if (!tc->mode)
 		return -EINVAL;
 
+	dev_dbg(tc->dev, "link enable\n");
+
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
 	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
 	tc_write(DP1_SRCCTRL,
@@ -1005,6 +1007,20 @@ static int tc_main_link_setup(struct tc_data *tc)
 	return ret;
 }
 
+static int tc_main_link_disable(struct tc_data *tc)
+{
+	int ret;
+
+	dev_dbg(tc->dev, "link disable\n");
+
+	tc_write(DP0_SRCCTRL, 0);
+	tc_write(DP0CTL, 0);
+
+	return 0;
+err:
+	return ret;
+}
+
 static int tc_stream_enable(struct tc_data *tc)
 {
 	int ret;
@@ -1083,15 +1099,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
-	ret = tc_main_link_setup(tc);
+	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
-		dev_err(tc->dev, "main link setup error: %d\n", ret);
+		dev_err(tc->dev, "main link enable error: %d\n", ret);
 		return;
 	}
 
 	ret = tc_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
+		tc_main_link_disable(tc);
 		return;
 	}
 
@@ -1108,6 +1125,10 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
+
+	ret = tc_main_link_disable(tc);
+	if (ret < 0)
+		dev_err(tc->dev, "main link disable error: %d\n", ret);
 }
 
 static void tc_bridge_post_disable(struct drm_bridge *bridge)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/21] drm/bridge: tc358767: ensure DP is disabled before LT

[Tomi Valkeinen]

Link training will sometimes fail if the DP link is, for some whatever
reason, enabled when tc_main_link_enable() is called.

Ensure that the DP link is disabled by setting DP0CTL to 0 as the first
thing.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index fe5fd7db0247..f628575c9de9 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -839,6 +839,8 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "link enable\n");
 
+	tc_write(DP0CTL, 0);
+
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
 	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
 	tc_write(DP1_SRCCTRL,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/21] drm/bridge: tc358767: remove unnecessary msleep

[Tomi Valkeinen]

For some reason the driver has a msleep(100) after writing to
DP_PHY_CTRL. Toshiba's documentation doesn't suggest any delay is
needed, and I have not seen any issues with the sleep removed.

Drop it, as msleep(100) is a rather big one.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f628575c9de9..2a6c0c0d47a6 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -872,7 +872,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (tc->link.base.num_lanes == 2)
 		dp_phy_ctrl |= PHY_2LANE;
 	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
-	msleep(100);
 
 	/* PLL setup */
 	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/21] drm/bridge: tc358767: use more reliable seq when finishing LT

[Tomi Valkeinen]

At the end of the link training, two steps have to be taken: 1)
tc358767's LT mode is disabled by a write to DP0_SRCCTRL, and 2) Remove
LT flag in DPCD 0x102.

Toshiba's documentation tells to first write the DPCD, then modify
DP0_SRCCTRL. In my testing this often causes issues, and the link
disconnects right after those steps.

If I reverse the steps, it works every time. There's a chance that this
is DP sink specific, though, but as my testing shows this sequence to be
much more reliable, let's change it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 2a6c0c0d47a6..700e161015af 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -958,6 +958,9 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret)
 		goto err;
 
+	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
+
 	/* Clear DPCD 0x102 */
 	/* Note: Can Not use DP0_SNKLTCTRL (0x06E4) short cut */
 	tmp[0] = tc->link.scrambler_dis ? DP_LINK_SCRAMBLING_DISABLE : 0x00;
@@ -965,9 +968,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
-	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
-
 	/* Wait */
 	timeout = 100;
 	do {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/21] drm/bridge: tc358767: cleanup LT result check

[Tomi Valkeinen]

The driver has a loop after ending link training, where it reads the
DPCD link status and prints an error if that status is not ok.

The loop is unnecessary, as far as I can understand from DP specs, so
let's remove it. We can also print the more specific errors to help
debugging.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 62 +++++++++++++++++--------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 700e161015af..220408db82f7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -968,34 +968,42 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	/* Wait */
-	timeout = 100;
-	do {
-		udelay(1);
-		/* Read DPCD 0x202-0x207 */
-		ret = drm_dp_dpcd_read_link_status(aux, tmp + 2);
-		if (ret < 0)
-			goto err_dpcd_read;
-	} while ((--timeout) &&
-		 !(drm_dp_channel_eq_ok(tmp + 2,  tc->link.base.num_lanes)));
+	/* Check link status */
+	ret = drm_dp_dpcd_read_link_status(aux, tmp);
+	if (ret < 0)
+		goto err_dpcd_read;
 
-	if (timeout == 0) {
-		/* Read DPCD 0x200-0x201 */
-		ret = drm_dp_dpcd_read(aux, DP_SINK_COUNT, tmp, 2);
-		if (ret < 0)
-			goto err_dpcd_read;
-		dev_err(dev, "channel(s) EQ not ok\n");
-		dev_info(dev, "0x0200 SINK_COUNT: 0x%02x\n", tmp[0]);
-		dev_info(dev, "0x0201 DEVICE_SERVICE_IRQ_VECTOR: 0x%02x\n",
-			 tmp[1]);
-		dev_info(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[2]);
-		dev_info(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n",
-			 tmp[4]);
-		dev_info(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[5]);
-		dev_info(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n",
-			 tmp[6]);
-
-		return -EAGAIN;
+	ret = 0;
+
+	value = tmp[0] & DP_CHANNEL_EQ_BITS;
+
+	if (value != DP_CHANNEL_EQ_BITS) {
+		dev_err(tc->dev, "Lane 0 failed: %x\n", value);
+		ret = -ENODEV;
+	}
+
+	if (tc->link.base.num_lanes == 2) {
+		value = (tmp[0] >> 4) & DP_CHANNEL_EQ_BITS;
+
+		if (value != DP_CHANNEL_EQ_BITS) {
+			dev_err(tc->dev, "Lane 1 failed: %x\n", value);
+			ret = -ENODEV;
+		}
+
+		if (!(tmp[2] & DP_INTERLANE_ALIGN_DONE)) {
+			dev_err(tc->dev, "Interlane align failed\n");
+			ret = -ENODEV;
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "0x0202 LANE0_1_STATUS:            0x%02x\n", tmp[0]);
+		dev_err(dev, "0x0203 LANE2_3_STATUS             0x%02x\n", tmp[1]);
+		dev_err(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", tmp[2]);
+		dev_err(dev, "0x0205 SINK_STATUS:               0x%02x\n", tmp[3]);
+		dev_err(dev, "0x0206 ADJUST_REQUEST_LANE0_1:    0x%02x\n", tmp[4]);
+		dev_err(dev, "0x0207 ADJUST_REQUEST_LANE2_3:    0x%02x\n", tmp[5]);
+		goto err;
 	}
 
 	return 0;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 15/21] drm/bridge: tc358767: clean-up link training

[Tomi Valkeinen]

The current link training code does unnecessary retry-loops, and does
extra writes to the registers. It is easier to follow the flow and
ensure it's similar to Toshiba's documentation if we deal with LT inside
tc_main_link_enable() function.

This patch adds tc_wait_link_training() which handles waiting for the LT
phase to finish, and does the necessary LT register setups in
tc_main_link_enable, without extra loops.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
 1 file changed, 57 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 220408db82f7..1c61f6205e43 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
 	return ret;
 }
 
-static int tc_link_training(struct tc_data *tc, int pattern)
+static int tc_wait_link_training(struct tc_data *tc, u32 *error)
 {
-	const char * const *errors;
-	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
-		      DP0_SRCCTRL_AUTOCORRECT;
-	int timeout;
-	int retry;
+	u32 timeout = 1000;
 	u32 value;
 	int ret;
 
-	if (pattern == DP_TRAINING_PATTERN_1) {
-		srcctrl |= DP0_SRCCTRL_TP1;
-		errors = training_pattern1_errors;
-	} else {
-		srcctrl |= DP0_SRCCTRL_TP2;
-		errors = training_pattern2_errors;
-	}
-
-	/* Set DPCD 0x102 for Training Part 1 or 2 */
-	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
-
-	tc_write(DP0_LTLOOPCTRL,
-		 (0x0f << 28) |	/* Defer Iteration Count */
-		 (0x0f << 24) |	/* Loop Iteration Count */
-		 (0x0d << 0));	/* Loop Timer Delay */
-
-	retry = 5;
 	do {
-		/* Set DP0 Training Pattern */
-		tc_write(DP0_SRCCTRL, srcctrl);
-
-		/* Enable DP0 to start Link Training */
-		tc_write(DP0CTL, DP_EN);
-
-		/* wait */
-		timeout = 1000;
-		do {
-			tc_read(DP0_LTSTAT, &value);
-			udelay(1);
-		} while ((!(value & LT_LOOPDONE)) && (--timeout));
-		if (timeout == 0) {
-			dev_err(tc->dev, "Link training timeout!\n");
-		} else {
-			int pattern = (value >> 11) & 0x3;
-			int error = (value >> 8) & 0x7;
-
-			dev_dbg(tc->dev,
-				"Link training phase %d done after %d uS: %s\n",
-				pattern, 1000 - timeout, errors[error]);
-			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
-				break;
-			if (pattern == DP_TRAINING_PATTERN_2) {
-				value &= LT_CHANNEL1_EQ_BITS |
-					 LT_INTERLANE_ALIGN_DONE |
-					 LT_CHANNEL0_EQ_BITS;
-				/* in case of two lanes */
-				if ((tc->link.base.num_lanes == 2) &&
-				    (value == (LT_CHANNEL1_EQ_BITS |
-					       LT_INTERLANE_ALIGN_DONE |
-					       LT_CHANNEL0_EQ_BITS)))
-					break;
-				/* in case of one line */
-				if ((tc->link.base.num_lanes == 1) &&
-				    (value == (LT_INTERLANE_ALIGN_DONE |
-					       LT_CHANNEL0_EQ_BITS)))
-					break;
-			}
-		}
-		/* restart */
-		tc_write(DP0CTL, 0);
-		usleep_range(10, 20);
-	} while (--retry);
-	if (retry == 0) {
-		dev_err(tc->dev, "Failed to finish training phase %d\n",
-			pattern);
+		udelay(1);
+		tc_read(DP0_LTSTAT, &value);
+	} while ((!(value & LT_LOOPDONE)) && (--timeout));
+
+	if (timeout == 0) {
+		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
+		ret = -ETIMEDOUT;
+		goto err;
 	}
 
+	*error = (value >> 8) & 0x7;
+
 	return 0;
 err:
 	return ret;
@@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 	u32 value;
 	int ret;
 	u8 tmp[8];
+	u32 error;
 
 	/* display mode should be set at this point */
 	if (!tc->mode)
@@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
+	/* LINK TRAINING PATTERN 1 */
+
+	/* Set DPCD 0x102 for Training Pattern 1 */
+	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
+
+	tc_write(DP0_LTLOOPCTRL,
+		 (15 << 28) |	/* Defer Iteration Count */
+		 (15 << 24) |	/* Loop Iteration Count */
+		 (0xd << 0));	/* Loop Timer Delay */
+
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+		 DP0_SRCCTRL_TP1);
+
+	/* Enable DP0 to start Link Training */
+	tc_write(DP0CTL,
+		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
+		 DP_EN);
+
+	/* wait */
+	ret = tc_wait_link_training(tc, &error);
 	if (ret)
 		goto err;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
+	if (error) {
+		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
+			training_pattern1_errors[error]);
+		ret = -ENODEV;
+		goto err;
+	}
+
+	/* LINK TRAINING PATTERN 2 */
+
+	/* Set DPCD 0x102 for Training Pattern 2 */
+	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
+
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+		 DP0_SRCCTRL_TP2);
+
+	/* wait */
+	ret = tc_wait_link_training(tc, &error);
 	if (ret)
 		goto err;
 
+	if (error) {
+		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
+			training_pattern2_errors[error]);
+		ret = -ENODEV;
+		goto err;
+	}
+
 	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 16/21] drm/bridge: tc358767: remove check for video mode in link enable

[Tomi Valkeinen]

tc_main_link_enable() checks if videomode has been set, and fails if
there's no videomode. As tc_main_link_enable() no longer depends on the
videomode, we can drop the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 1c61f6205e43..ece330c05b9f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -776,10 +776,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	u8 tmp[8];
 	u32 error;
 
-	/* display mode should be set at this point */
-	if (!tc->mode)
-		return -EINVAL;
-
 	dev_dbg(tc->dev, "link enable\n");
 
 	tc_write(DP0CTL, 0);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 17/21] drm/bridge: tc358767: use bridge mode_valid

[Tomi Valkeinen]

We have tc_connector_mode_valid() to filter out videomdoes that the
tc358767 cannot support. As it is a bridge limitation, change the code
to use drm_bridge_funcs's mode_valid instead.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index ece330c05b9f..9fbda370a4c2 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1140,10 +1140,10 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
 	return true;
 }
 
-static enum drm_mode_status tc_connector_mode_valid(struct drm_connector *connector,
-				   struct drm_display_mode *mode)
+static enum drm_mode_status tc_mode_valid(struct drm_bridge *bridge,
+					  const struct drm_display_mode *mode)
 {
-	struct tc_data *tc = connector_to_tc(connector);
+	struct tc_data *tc = bridge_to_tc(bridge);
 	u32 req, avail;
 	u32 bits_per_pixel = 24;
 
@@ -1212,7 +1212,6 @@ tc_connector_best_encoder(struct drm_connector *connector)
 
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
-	.mode_valid = tc_connector_mode_valid,
 	.best_encoder = tc_connector_best_encoder,
 };
 
@@ -1255,6 +1254,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 
 static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.attach = tc_bridge_attach,
+	.mode_valid = tc_mode_valid,
 	.mode_set = tc_bridge_mode_set,
 	.pre_enable = tc_bridge_pre_enable,
 	.enable = tc_bridge_enable,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 18/21] drm/bridge: tc358767: remove tc_connector_best_encoder

[Tomi Valkeinen]

As far as I know, drm_connector_helper_funcs.best_encoder is not needed
in a trivial case as we have here. So remove tc_connector_best_encoder.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 9fbda370a4c2..114d535c296b 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1202,17 +1202,8 @@ static void tc_connector_set_polling(struct tc_data *tc,
 			    DRM_CONNECTOR_POLL_DISCONNECT;
 }
 
-static struct drm_encoder *
-tc_connector_best_encoder(struct drm_connector *connector)
-{
-	struct tc_data *tc = connector_to_tc(connector);
-
-	return tc->bridge.encoder;
-}
-
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
-	.best_encoder = tc_connector_best_encoder,
 };
 
 static const struct drm_connector_funcs tc_connector_funcs = {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 19/21] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Tomi Valkeinen]

In tc_bridge_mode_set callback, we store the pointer to the given
drm_display_mode, and use the mode later. Storing a pointer in such a
way looks very suspicious to me, and I have observed odd issues where
the timings were apparently (at least mostly) zero.

Do a copy of the drm_display_mode instead to ensure we don't refer to
freed/modified data.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 114d535c296b..8732d5b05453 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -205,7 +205,7 @@ struct tc_data {
 	/* display edid */
 	struct edid		*edid;
 	/* current mode */
-	const struct drm_display_mode	*mode;
+	struct drm_display_mode	mode;
 
 	u32			rev;
 	u8			assr;
@@ -1021,12 +1021,12 @@ static int tc_stream_enable(struct tc_data *tc)
 	/* PXL PLL setup */
 	if (tc_test_pattern) {
 		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
-				    1000 * tc->mode->clock);
+				    1000 * tc->mode.clock);
 		if (ret)
 			goto err;
 	}
 
-	ret = tc_set_video_mode(tc, tc->mode);
+	ret = tc_set_video_mode(tc, &tc->mode);
 	if (ret)
 		goto err;
 
@@ -1166,7 +1166,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 
-	tc->mode = mode;
+	tc->mode = *mode;
 }
 
 static int tc_connector_get_modes(struct drm_connector *connector)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 20/21] drm/bridge: tc358767: add GPIO & interrupt registers

[Tomi Valkeinen]

Add GPIO and interrupt related registers for HPD work. Mark INTSTS_G and
GPIOI as volatile.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8732d5b05453..8606de29c9b2 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -78,6 +78,12 @@
 #define DP0_VIDSRC_DSI_RX		(1 << 0)
 #define DP0_VIDSRC_DPI_RX		(2 << 0)
 #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
+#define GPIOM			0x0540
+#define GPIOI			0x054c
+#define INTCTL_G		0x0560
+#define INTSTS_G		0x0564
+#define INT_GP0_LCNT		0x0584
+#define INT_GP1_LCNT		0x0588
 
 /* Control */
 #define DP0CTL			0x0600
@@ -1265,6 +1271,8 @@ static const struct regmap_range tc_volatile_ranges[] = {
 	regmap_reg_range(DP_PHY_CTRL, DP_PHY_CTRL),
 	regmap_reg_range(DP0_PLLCTRL, PXL_PLLCTRL),
 	regmap_reg_range(VFUEN0, VFUEN0),
+	regmap_reg_range(INTSTS_G, INTSTS_G),
+	regmap_reg_range(GPIOI, GPIOI),
 };
 
 static const struct regmap_access_table tc_volatile_table = {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 21/21] drm/bridge: tc358767: implement naive HPD handling

[Tomi Valkeinen]

tc358767 driver doesn't have any HPD handling at the moment, as it was
originally developed to support only eDP.

This patch implements a naive, polling-based HPD handling, which is used
when the driver is in DP mode.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8606de29c9b2..2b252f7ac070 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
+	ret = tc_get_display_props(tc);
+	if (ret < 0) {
+		dev_err(tc->dev, "failed to read display props: %d\n", ret);
+		return;
+	}
+
 	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link enable error: %d\n", ret);
@@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector)
 	return count;
 }
 
-static void tc_connector_set_polling(struct tc_data *tc,
-				     struct drm_connector *connector)
-{
-	/* TODO: add support for HPD */
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			    DRM_CONNECTOR_POLL_DISCONNECT;
-}
-
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
 };
 
+static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tc_data *tc = connector_to_tc(connector);
+	u32 val;
+	int ret;
+	bool conn;
+
+	tc_read(GPIOI, &val);
+
+	conn = val & BIT(0);
+
+	if (force && conn)
+		tc_get_display_props(tc);
+
+	if (conn)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+
+err:
+	return connector_status_unknown;
+}
+
 static const struct drm_connector_funcs tc_connector_funcs = {
+	.detect = tc_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
 	.reset = drm_atomic_helper_connector_reset,
@@ -1227,7 +1249,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
-	/* Create eDP connector */
+	/* Create DP/eDP connector */
 	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
 	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
 				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
@@ -1235,6 +1257,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 	if (ret)
 		return ret;
 
+	/* Don't poll if we have eDP panel */
+	if (!tc->panel) {
+		/* TODO: add support for HPD */
+		tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+				       DRM_CONNECTOR_POLL_DISCONNECT;
+	}
+
 	if (tc->panel)
 		drm_panel_attach(tc->panel, &tc->connector);
 
@@ -1377,12 +1406,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
-	ret = tc_get_display_props(tc);
-	if (ret)
-		goto err_unregister_aux;
-
-	tc_connector_set_polling(tc, &tc->connector);
-
 	tc->bridge.funcs = &tc_bridge_funcs;
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
@@ -1390,9 +1413,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	i2c_set_clientdata(client, tc);
 
 	return 0;
-err_unregister_aux:
-	drm_dp_aux_unregister(&tc->aux);
-	return ret;
 }
 
 static int tc_remove(struct i2c_client *client)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Andrey Smirnov]

> tc358767 driver doesn't have any HPD handling at the moment, as it was
> originally developed to support only eDP.

> This patch implements a naive, polling-based HPD handling, which is used
> when the driver is in DP mode.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 18 deletions(-)

> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 8606de29c9b2..2b252f7ac070 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
 
> +	ret = tc_get_display_props(tc);
> +	if (ret < 0) {
> +		dev_err(tc->dev, "failed to read display props: %d\n", ret);
> +		return;
> +	}
> +
>  	ret = tc_main_link_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link enable error: %d\n", ret);
> @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  	return count;
>  }
 
> -static void tc_connector_set_polling(struct tc_data *tc,
> -				     struct drm_connector *connector)
> -{
> -	/* TODO: add support for HPD */
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> -			    DRM_CONNECTOR_POLL_DISCONNECT;
> -}
> -
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
>  };
 
> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct tc_data *tc = connector_to_tc(connector);
> +	u32 val;
> +	int ret;
> +	bool conn;
> +
> +	tc_read(GPIOI, &val);
> +
> +	conn = val & BIT(0);

TC358767 has two GPIO pins that can be used for HPD signal. I think
instead of hardcoding GPIO0 here it would be more flexible to expose
boths gpios as a gpiochip and use gpiolib API to query the value of
HPD as well as use "hpd-gpios" binidng in DT to select which input to
use.

Another argument in favour of this solution is that Toshiba's FAEs (at
least some) recommend thier customers to connect HPD signal to SoC's
GPIOs and bypass TC358767 entirely. Their reasoning being that
TC358767 implements a generic GPIO contoller and there's no advantage
in going through TC358767 if you could use your "normal" GPIOs.

Using gpiolib API would allow us to handle both usecase with the same
code.

Lastly, by treating "hpd-gpios" as an optional property, we can
preserve old driver behaviour regardless if it is connected to DP or
eDP panel. Not saying that this is really worth doing, just pointing
out that this option would be on the table as well.


Thanks,
Andrey Smirnov
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Andrey Smirnov]

On Tue, Mar 19, 2019 at 11:18 AM Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
>
> > tc358767 driver doesn't have any HPD handling at the moment, as it was
> > originally developed to support only eDP.
>
> > This patch implements a naive, polling-based HPD handling, which is used
> > when the driver is in DP mode.
>
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++----------
> >  1 file changed, 38 insertions(+), 18 deletions(-)
>
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 8606de29c9b2..2b252f7ac070 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> >       struct tc_data *tc = bridge_to_tc(bridge);
> >       int ret;
>
> > +     ret = tc_get_display_props(tc);
> > +     if (ret < 0) {
> > +             dev_err(tc->dev, "failed to read display props: %d\n", ret);
> > +             return;
> > +     }
> > +
> >       ret = tc_main_link_enable(tc);
> >       if (ret < 0) {
> >               dev_err(tc->dev, "main link enable error: %d\n", ret);
> > @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector)
> >       return count;
> >  }
>
> > -static void tc_connector_set_polling(struct tc_data *tc,
> > -                                  struct drm_connector *connector)
> > -{
> > -     /* TODO: add support for HPD */
> > -     connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> > -                         DRM_CONNECTOR_POLL_DISCONNECT;
> > -}
> > -
> >  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
> >       .get_modes = tc_connector_get_modes,
> >  };
>
> > +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +     struct tc_data *tc = connector_to_tc(connector);
> > +     u32 val;
> > +     int ret;
> > +     bool conn;
> > +
> > +     tc_read(GPIOI, &val);
> > +
> > +     conn = val & BIT(0);

Another thing I noticed when trying this patch is that
tc_connector_detect() will get called via drm_helper_probe_detect()
even if tc->panel is not NULL and tc->connector.polled is zero, which
creates a problem for eDP use-case.

Thanks,
Andrey Smirnov
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Tomi Valkeinen]

On 20/03/2019 07:55, Andrey Smirnov wrote:
> On Tue, Mar 19, 2019 at 11:18 AM Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>>
>>> tc358767 driver doesn't have any HPD handling at the moment, as it was
>>> originally developed to support only eDP.
>>
>>> This patch implements a naive, polling-based HPD handling, which is used
>>> when the driver is in DP mode.
>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>  drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++----------
>>>  1 file changed, 38 insertions(+), 18 deletions(-)
>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>>> index 8606de29c9b2..2b252f7ac070 100644
>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>> @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>>>       struct tc_data *tc = bridge_to_tc(bridge);
>>>       int ret;
>>
>>> +     ret = tc_get_display_props(tc);
>>> +     if (ret < 0) {
>>> +             dev_err(tc->dev, "failed to read display props: %d\n", ret);
>>> +             return;
>>> +     }
>>> +
>>>       ret = tc_main_link_enable(tc);
>>>       if (ret < 0) {
>>>               dev_err(tc->dev, "main link enable error: %d\n", ret);
>>> @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>>>       return count;
>>>  }
>>
>>> -static void tc_connector_set_polling(struct tc_data *tc,
>>> -                                  struct drm_connector *connector)
>>> -{
>>> -     /* TODO: add support for HPD */
>>> -     connector->polled = DRM_CONNECTOR_POLL_CONNECT |
>>> -                         DRM_CONNECTOR_POLL_DISCONNECT;
>>> -}
>>> -
>>>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>>>       .get_modes = tc_connector_get_modes,
>>>  };
>>
>>> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
>>> +{
>>> +     struct tc_data *tc = connector_to_tc(connector);
>>> +     u32 val;
>>> +     int ret;
>>> +     bool conn;
>>> +
>>> +     tc_read(GPIOI, &val);
>>> +
>>> +     conn = val & BIT(0);
> 
> Another thing I noticed when trying this patch is that
> tc_connector_detect() will get called via drm_helper_probe_detect()
> even if tc->panel is not NULL and tc->connector.polled is zero, which
> creates a problem for eDP use-case.

Ok, thanks for testing. So we need to return "connected" from the detect
function for eDP cases.

I think I need to create a fake setup where I can also run with an eDP
panel to test this series properly. Probably I can create a "panel"
driver for my DP monitor, so I would still get a picture.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Tomi Valkeinen]

On 19/03/2019 20:18, Andrey Smirnov wrote:

> TC358767 has two GPIO pins that can be used for HPD signal. I think
> instead of hardcoding GPIO0 here it would be more flexible to expose
> boths gpios as a gpiochip and use gpiolib API to query the value of
> HPD as well as use "hpd-gpios" binidng in DT to select which input to
> use.
> 
> Another argument in favour of this solution is that Toshiba's FAEs (at
> least some) recommend thier customers to connect HPD signal to SoC's
> GPIOs and bypass TC358767 entirely. Their reasoning being that
> TC358767 implements a generic GPIO contoller and there's no advantage
> in going through TC358767 if you could use your "normal" GPIOs.
> 
> Using gpiolib API would allow us to handle both usecase with the same
> code.
> 
> Lastly, by treating "hpd-gpios" as an optional property, we can
> preserve old driver behaviour regardless if it is connected to DP or
> eDP panel. Not saying that this is really worth doing, just pointing
> out that this option would be on the table as well.

I think that's a good point.

There's one thing that TC358767 offers, which may not be available on
most generic GPIO controllers, though: it can detect short (programmable
length) pulses, thus it's possible to easily implement the DisplayPort
IRQ mechanism. I'm not sure if it's possible to implement reliable DP
IRQ detection without HW support.

Still, I think using standard gpios makes sense.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Tomi Valkeinen]

On 20/03/2019 08:57, Tomi Valkeinen wrote:
> On 19/03/2019 20:18, Andrey Smirnov wrote:
> 
>> TC358767 has two GPIO pins that can be used for HPD signal. I think
>> instead of hardcoding GPIO0 here it would be more flexible to expose
>> boths gpios as a gpiochip and use gpiolib API to query the value of
>> HPD as well as use "hpd-gpios" binidng in DT to select which input to
>> use.
>>
>> Another argument in favour of this solution is that Toshiba's FAEs (at
>> least some) recommend thier customers to connect HPD signal to SoC's
>> GPIOs and bypass TC358767 entirely. Their reasoning being that
>> TC358767 implements a generic GPIO contoller and there's no advantage
>> in going through TC358767 if you could use your "normal" GPIOs.
>>
>> Using gpiolib API would allow us to handle both usecase with the same
>> code.
>>
>> Lastly, by treating "hpd-gpios" as an optional property, we can
>> preserve old driver behaviour regardless if it is connected to DP or
>> eDP panel. Not saying that this is really worth doing, just pointing
>> out that this option would be on the table as well.
> 
> I think that's a good point.
> 
> There's one thing that TC358767 offers, which may not be available on
> most generic GPIO controllers, though: it can detect short (programmable
> length) pulses, thus it's possible to easily implement the DisplayPort
> IRQ mechanism. I'm not sure if it's possible to implement reliable DP
> IRQ detection without HW support.
> 
> Still, I think using standard gpios makes sense.

After implementing the gpiochip (it works), I started to wonder...

If TC358767 is used as a gpio expander, for whatever purpose, outside
the TC358767 driver, then obviously we need the gpiochip driver. But I
don't think anyone needs that.

Then we have two cases 1) HPD connected to TC358767, 2) HPD goes
directly to the SoC, or worded differently, HPD is handled by something
else than TC358767.

1) was implemented in this current patch, and there's no real benefit
with the gpiochip. It's somewhat confusing that the driver provides a
gpiochip which the same driver then uses, for its internal functionality.

2) should actually not involve TC358767 driver at all as it's totally
outside TC358767.

If the HPD goes from the DP connector to the SoC, we should have the DP
connector driver handle it. Currently that connector is in the TC358767
driver, but it should really be separated.

So... Obviously what's missing from the current patch is that we need to
be able to say which of the two GPIOs are used for the HPD (if any). But
I'm debating with myself whether gpiochip here is a sane choice or not.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Andrey Smirnov]

On Wed, Mar 20, 2019 at 6:03 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 20/03/2019 08:57, Tomi Valkeinen wrote:
> > On 19/03/2019 20:18, Andrey Smirnov wrote:
> >
> >> TC358767 has two GPIO pins that can be used for HPD signal. I think
> >> instead of hardcoding GPIO0 here it would be more flexible to expose
> >> boths gpios as a gpiochip and use gpiolib API to query the value of
> >> HPD as well as use "hpd-gpios" binidng in DT to select which input to
> >> use.
> >>
> >> Another argument in favour of this solution is that Toshiba's FAEs (at
> >> least some) recommend thier customers to connect HPD signal to SoC's
> >> GPIOs and bypass TC358767 entirely. Their reasoning being that
> >> TC358767 implements a generic GPIO contoller and there's no advantage
> >> in going through TC358767 if you could use your "normal" GPIOs.
> >>
> >> Using gpiolib API would allow us to handle both usecase with the same
> >> code.
> >>
> >> Lastly, by treating "hpd-gpios" as an optional property, we can
> >> preserve old driver behaviour regardless if it is connected to DP or
> >> eDP panel. Not saying that this is really worth doing, just pointing
> >> out that this option would be on the table as well.
> >
> > I think that's a good point.
> >
> > There's one thing that TC358767 offers, which may not be available on
> > most generic GPIO controllers, though: it can detect short (programmable
> > length) pulses, thus it's possible to easily implement the DisplayPort
> > IRQ mechanism. I'm not sure if it's possible to implement reliable DP
> > IRQ detection without HW support.
> >
> > Still, I think using standard gpios makes sense.
>
> After implementing the gpiochip (it works), I started to wonder...
>
> If TC358767 is used as a gpio expander, for whatever purpose, outside
> the TC358767 driver, then obviously we need the gpiochip driver. But I
> don't think anyone needs that.
>

Regardless of how it's going to be implemented in the end, there'd
have to be a way to specify which HPD input is being used. Which means
a either a new DT binding or re-using already existing to be agreed on
by DT folks. It just seems to me that there exists a much stronger
case to solve this using existing "language" of GPIO references as
opposed to introducing some vendor specific binding serving just this
single purpose. If DT is supposed to be used to describe the HW, then,
IMHO, it might be the other way around, TC358767 is also a GPIO
expander and has to be modeled/implemented as such, whether anyone
would ever use it in such capacity fully isn't that significant.

> Then we have two cases 1) HPD connected to TC358767, 2) HPD goes
> directly to the SoC, or worded differently, HPD is handled by something
> else than TC358767.
>
> 1) was implemented in this current patch, and there's no real benefit
> with the gpiochip. It's somewhat confusing that the driver provides a
> gpiochip which the same driver then uses, for its internal functionality.
>
> 2) should actually not involve TC358767 driver at all as it's totally
> outside TC358767.
>

There's already precedent for such usage in ti-tfp410.c, analogix/ and
andanalogix-anx78xx.c, so it's not unheard of.

> If the HPD goes from the DP connector to the SoC, we should have the DP
> connector driver handle it. Currently that connector is in the TC358767
> driver, but it should really be separated.
>

Sure, there's definitely more than one way to solve this.

> So... Obviously what's missing from the current patch is that we need to
> be able to say which of the two GPIOs are used for the HPD (if any). But
> I'm debating with myself whether gpiochip here is a sane choice or not.

Yeah, maybe it'd be best to submit a patch to DT mailing list and see
what they have to say?

Thanks,
Andrey Smirnov
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [21/21] drm/bridge: tc358767: implement naive HPD handling

[Tomi Valkeinen]

On 21/03/2019 00:58, Andrey Smirnov wrote:

> Regardless of how it's going to be implemented in the end, there'd
> have to be a way to specify which HPD input is being used. Which means

True.

> a either a new DT binding or re-using already existing to be agreed on
> by DT folks. It just seems to me that there exists a much stronger
> case to solve this using existing "language" of GPIO references as
> opposed to introducing some vendor specific binding serving just this
> single purpose. If DT is supposed to be used to describe the HW, then,
> IMHO, it might be the other way around, TC358767 is also a GPIO
> expander and has to be modeled/implemented as such, whether anyone
> would ever use it in such capacity fully isn't that significant.

Yep. But few points:

- TC358767 node will expose gpios and then it uses them itself. It does
look slightly silly in the DT data =). It's not often when you create a
reference from a node to itself.

- This also needs irqchip implemention to support HPD irq. I have never
written one, but I presume it's not too complex, but not trivial either.

- All this adds quite a big amount of code, compared to only few lines
of code if this is done internally.

>> Then we have two cases 1) HPD connected to TC358767, 2) HPD goes
>> directly to the SoC, or worded differently, HPD is handled by something
>> else than TC358767.
>>
>> 1) was implemented in this current patch, and there's no real benefit
>> with the gpiochip. It's somewhat confusing that the driver provides a
>> gpiochip which the same driver then uses, for its internal functionality.
>>
>> 2) should actually not involve TC358767 driver at all as it's totally
>> outside TC358767.
>>
> 
> There's already precedent for such usage in ti-tfp410.c, analogix/ and
> andanalogix-anx78xx.c, so it's not unheard of.

Yes, but I believe the direction has been to move away from that.

>> If the HPD goes from the DP connector to the SoC, we should have the DP
>> connector driver handle it. Currently that connector is in the TC358767
>> driver, but it should really be separated.
>>
> 
> Sure, there's definitely more than one way to solve this.
> 
>> So... Obviously what's missing from the current patch is that we need to
>> be able to say which of the two GPIOs are used for the HPD (if any). But
>> I'm debating with myself whether gpiochip here is a sane choice or not.
> 
> Yeah, maybe it'd be best to submit a patch to DT mailing list and see
> what they have to say?

Yep. I'll write the irchip support too, out of interest, and see what it
looks like. But this has the feel of over-engineering.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel