[PATCHv3 00/23] drm/bridge: tc358767: DP support

[PATCHv3 00/23] 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 and HPD 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.

Changes in v2
* Drop "implement naive HPD handling", and implement HPD interrupt handling.

Changes in v3
* Various small comment, description and formatting changes
* 'hpd-num' DT property renamed to 'toshiba,hpd-pin'
* Check DP0CTL == 0 at the beginning of tc_main_link_enable
* Disable only the video stream in tc_stream_disable()
* Fix tc_connector_detect for eDP panels

 Tomi

Tomi Valkeinen (23):
  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: disable only video stream in tc_stream_disable
  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: add IRQ and HPD support
  dt-bindings: tc358767: add HPD support

 .../display/bridge/toshiba,tc358767.txt       |   1 +
 drivers/gpu/drm/bridge/tc358767.c             | 589 +++++++++++-------
 2 files changed, 378 insertions(+), 212 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

[PATCHv3 01/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.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

[PATCHv3 02/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7031c4f52c57..03da47cff182 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -956,6 +956,13 @@ 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

[PATCHv3 03/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.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 03da47cff182..664cc691a841 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

[PATCHv3 04/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 664cc691a841..87b838ffe084 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

[PATCHv3 05/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 87b838ffe084..f54918b0594c 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

[PATCHv3 06/23] drm/bridge: tc358767: cleanup aux_link_setup

[Tomi Valkeinen]

The driver sets up AUX link at probe time, but, for some reason, also
sets the main link's number of lanes using tc->link.base.num_lanes. This
is not needed nor correct, as the number of lanes has not been decided
yet. The number of lanes will be set later during main link setup.

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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 f54918b0594c..f125727ac98e 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

[PATCHv3 07/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 f125727ac98e..8dc63de39e73 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1004,15 +1004,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);
@@ -1031,6 +1022,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

[PATCHv3 08/23] 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 8dc63de39e73..641295ac2c0e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1014,47 +1014,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, "enable video stream\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)
+		return ret;
 
-		/* Set M/N */
-		ret = tc_stream_clock_calc(tc);
-		if (ret)
-			goto err;
+	/* Set M/N */
+	ret = tc_stream_clock_calc(tc);
+	if (ret)
+		return ret;
 
-		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, "disable video stream\n");
+
+	tc_write(DP0CTL, 0);
 
 	return 0;
 err:
@@ -1079,7 +1088,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;
@@ -1095,7 +1104,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

Re: [PATCHv3 08/23] drm/bridge: tc358767: split stream enable/disable

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

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

[PATCHv3 09/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 641295ac2c0e..887b78128f29 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);
@@ -1021,6 +1013,14 @@ static int tc_stream_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "enable video stream\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)
 		return ret;
@@ -1065,6 +1065,8 @@ static int tc_stream_disable(struct tc_data *tc)
 
 	tc_write(DP0CTL, 0);
 
+	tc_pxl_pll_dis(tc);
+
 	return 0;
 err:
 	return ret;
@@ -1391,8 +1393,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

[PATCHv3 10/23] 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 887b78128f29..b22f5fe7ccfd 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,
@@ -1006,6 +1008,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;
@@ -1084,15 +1100,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;
 	}
 
@@ -1109,6 +1126,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

Re: [PATCHv3 10/23] drm/bridge: tc358767: add link disable function

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

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

[PATCHv3 11/23] drm/bridge: tc358767: disable only video stream in tc_stream_disable

[Tomi Valkeinen]

Currently the code writes 0 to DP0CTL in tc_stream_disable(), which
disables the whole DP link instead of just the video stream. We always
disable the link and the stream together from tc_bridge_disable(), so
this doesn't cause any issues.

Nevertheless, fix this by only clearing VID_EN in tc_stream_disable to
stop the video stream while keeping the link enabled.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index b22f5fe7ccfd..86e2e16c5a90 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1076,10 +1076,13 @@ static int tc_stream_enable(struct tc_data *tc)
 static int tc_stream_disable(struct tc_data *tc)
 {
 	int ret;
+	u32 val;
 
 	dev_dbg(tc->dev, "disable video stream\n");
 
-	tc_write(DP0CTL, 0);
+	tc_read(DP0CTL, &val);
+	val &= ~VID_EN;
+	tc_write(DP0CTL, val);
 
 	tc_pxl_pll_dis(tc);
 
-- 
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: [PATCHv3 11/23] drm/bridge: tc358767: disable only video stream in tc_stream_disable

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> Currently the code writes 0 to DP0CTL in tc_stream_disable(), which
> disables the whole DP link instead of just the video stream. We always
> disable the link and the stream together from tc_bridge_disable(), so
> this doesn't cause any issues.
>
> Nevertheless, fix this by only clearing VID_EN in tc_stream_disable to
> stop the video stream while keeping the link enabled.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

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

[PATCHv3 12/23] drm/bridge: tc358767: ensure DP is disabled before LT

[Tomi Valkeinen]

Link training will sometimes fail if the DP link is enabled when
tc_main_link_enable() is called. The driver makes sure the DP link is
disabled when the DP output is disabled, and we never enable the DP
without first disabling it, so this should never happen.

However, as the HW behavior seems to be somewhat random if DP link has
erroneously been left enabled, let's add a WARN_ON() for the case and
set DP0CTL to 0.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 86e2e16c5a90..cec825600158 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -839,6 +839,10 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "link enable\n");
 
+	tc_read(DP0CTL, &value);
+	if (WARN_ON(value != 0))
+		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

Re: [PATCHv3 12/23] drm/bridge: tc358767: ensure DP is disabled before LT

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> Link training will sometimes fail if the DP link is enabled when
> tc_main_link_enable() is called. The driver makes sure the DP link is
> disabled when the DP output is disabled, and we never enable the DP
> without first disabling it, so this should never happen.
>
> However, as the HW behavior seems to be somewhat random if DP link has
> erroneously been left enabled, let's add a WARN_ON() for the case and
> set DP0CTL to 0.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

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

[PATCHv3 13/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Andrey Gusakov <andrey.gusakov@cogentembedded.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 cec825600158..ed348e09576a 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -874,7 +874,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

[PATCHv3 14/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index ed348e09576a..87eb875e1174 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -961,6 +961,18 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret)
 		goto err;
 
+	/*
+	 * Toshiba's documentation suggests to first clear DPCD 0x102, then
+	 * clear the training pattern bit in DP0_SRCCTRL. Testing shows
+	 * that the link sometimes drops if those steps are done in that order,
+	 * but if the steps are done in reverse order, the link stays up.
+	 *
+	 * So we do the steps differently than documented here.
+	 */
+
+	/* 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;
@@ -968,9 +980,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

[PATCHv3 15/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.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 87eb875e1174..2cec06520fcf 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -980,34 +980,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

[PATCHv3 16/23] 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 | 136 +++++++++++++-----------------
 1 file changed, 60 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 2cec06520fcf..86175e8d01b3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -740,84 +740,24 @@ 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)
 {
-	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");
+		return -ETIMEDOUT;
 	}
 
-	return 0;
+	return (value >> 8) & 0x7;
+
 err:
 	return ret;
 }
@@ -927,7 +867,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 		if (tmp[0] != tc->assr) {
 			dev_dbg(dev, "Failed to switch display ASSR to %d, falling back to unscrambled mode\n",
-				 tc->assr);
+				tc->assr);
 			/* trying with disabled scrambler */
 			tc->link.scrambler_dis = true;
 		}
@@ -953,13 +893,57 @@ 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);
-	if (ret)
+	/* Clock-Recovery */
+
+	/* 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);
+	if (ret < 0)
 		goto err;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
-	if (ret)
+	if (ret) {
+		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
+			training_pattern1_errors[ret]);
+		ret = -ENODEV;
 		goto err;
+	}
+
+	/* Channel Equalization */
+
+	/* 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);
+	if (ret < 0)
+		goto err;
+
+	if (ret) {
+		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
+			training_pattern2_errors[ret]);
+		ret = -ENODEV;
+		goto err;
+	}
 
 	/*
 	 * Toshiba's documentation suggests to first clear DPCD 0x102, then
-- 
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: [PATCHv3 16/23] drm/bridge: tc358767: clean-up link training

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> 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 | 136 +++++++++++++-----------------
>  1 file changed, 60 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 2cec06520fcf..86175e8d01b3 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -740,84 +740,24 @@ 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)
>  {
> -	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;
> -			}


As I understand all above checks can be dropped.

> -		}
> -		/* 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");
> +		return -ETIMEDOUT;
>  	}
>  
> -	return 0;
> +	return (value >> 8) & 0x7;
> +
>  err:
>  	return ret;
>  }
> @@ -927,7 +867,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>  
>  		if (tmp[0] != tc->assr) {
>  			dev_dbg(dev, "Failed to switch display ASSR to %d, falling back to unscrambled mode\n",
> -				 tc->assr);
> +				tc->assr);
>  			/* trying with disabled scrambler */
>  			tc->link.scrambler_dis = true;
>  		}
> @@ -953,13 +893,57 @@ 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);
> -	if (ret)
> +	/* Clock-Recovery */
> +
> +	/* 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);
> +	if (ret < 0)
>  		goto err;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
> -	if (ret)
> +	if (ret) {
> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
> +			training_pattern1_errors[ret]);
> +		ret = -ENODEV;
>  		goto err;
> +	}
> +
> +	/* Channel Equalization */
> +
> +	/* 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);
> +	if (ret < 0)
> +		goto err;
> +
> +	if (ret) {
> +		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
> +			training_pattern2_errors[ret]);
> +		ret = -ENODEV;
> +		goto err;
> +	}


In general it seems to be correct, but I could miss something - too many
changes for me :)

Anyway:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>


But it would be good to have Tested-by tag, if possible?

With that I guess the patchset can be merged.


Laurent are you OK with it?


 --
Regards
Andrzej


>  
>  	/*
>  	 * Toshiba's documentation suggests to first clear DPCD 0x102, then


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

[PATCHv3 17/23] 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.

Also, while tc_stream_enable() does depend on the videomode, we can
expect that a mode has been set before drm_bridge_funcs.enable is
called, so we don't need the check there either.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.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 86175e8d01b3..81e12f17b162 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -773,10 +773,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	int ret;
 	u8 tmp[8];
 
-	/* display mode should be set at this point */
-	if (!tc->mode)
-		return -EINVAL;
-
 	dev_dbg(tc->dev, "link enable\n");
 
 	tc_read(DP0CTL, &value);
-- 
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

[PATCHv3 18/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 81e12f17b162..f39528dbde22 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1154,10 +1154,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;
 
@@ -1226,7 +1226,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,
 };
 
@@ -1269,6 +1268,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

[PATCHv3 19/23] drm/bridge: tc358767: remove tc_connector_best_encoder

[Tomi Valkeinen]

drm_connector_helper_funcs.best_encoder is only needed when the
connector can have more than one encoder, and that is never the case
here.

So remove tc_connector_best_encoder.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 f39528dbde22..591bf64ac916 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1216,17 +1216,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

[PATCHv3 20/23] 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 591bf64ac916..8f6d601def3f 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;
@@ -1032,12 +1032,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)
 		return ret;
 
@@ -1180,7 +1180,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

Re: [PATCHv3 20/23] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
> ---
>  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 591bf64ac916..8f6d601def3f 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;
> @@ -1032,12 +1032,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)
>  		return ret;
>  
> @@ -1180,7 +1180,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)


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

[PATCHv3 21/23] 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>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.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 8f6d601def3f..7c275b8bbabc 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
@@ -1279,6 +1285,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

[PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

Add support for interrupt and hotplug handling. Both are optional.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7c275b8bbabc..b8cfeb2335cb 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -71,6 +71,7 @@
 
 /* System */
 #define TC_IDREG		0x0500
+#define SYSSTAT			0x0508
 #define SYSCTRL			0x0510
 #define DP0_AUDSRC_NO_INPUT		(0 << 3)
 #define DP0_AUDSRC_I2S_RX		(1 << 3)
@@ -79,9 +80,16 @@
 #define DP0_VIDSRC_DPI_RX		(2 << 0)
 #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
 #define GPIOM			0x0540
+#define GPIOC			0x0544
+#define GPIOO			0x0548
 #define GPIOI			0x054c
 #define INTCTL_G		0x0560
 #define INTSTS_G		0x0564
+
+#define INT_SYSERR		BIT(16)
+#define INT_GPIO_H(x)		(1 << (x == 0 ? 2 : 10))
+#define INT_GPIO_LC(x)		(1 << (x == 0 ? 3 : 11))
+
 #define INT_GP0_LCNT		0x0584
 #define INT_GP1_LCNT		0x0588
 
@@ -219,6 +227,12 @@ struct tc_data {
 	struct gpio_desc	*sd_gpio;
 	struct gpio_desc	*reset_gpio;
 	struct clk		*refclk;
+
+	/* do we have IRQ */
+	bool			have_irq;
+
+	/* HPD pin number (0 or 1) or -ENODEV */
+	int			hpd_pin;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -1109,6 +1123,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);
@@ -1214,19 +1234,43 @@ 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);
+	bool conn;
+	u32 val;
+	int ret;
+
+	if (tc->hpd_pin < 0) {
+		if (!tc->panel)
+			return connector_status_unknown;
+
+		conn = true;
+	} else {
+		tc_read(GPIOI, &val);
+
+		conn = val & BIT(tc->hpd_pin);
+	}
+
+	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,
@@ -1241,7 +1285,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 :
@@ -1249,6 +1293,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 	if (ret)
 		return ret;
 
+	/* Don't poll if don't have HPD connected */
+	if (tc->hpd_pin >= 0) {
+		if (tc->have_irq)
+			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
+		else
+			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+					       DRM_CONNECTOR_POLL_DISCONNECT;
+	}
+
 	if (tc->panel)
 		drm_panel_attach(tc->panel, &tc->connector);
 
@@ -1315,6 +1368,49 @@ static const struct regmap_config tc_regmap_config = {
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };
 
+static irqreturn_t tc_irq_handler(int irq, void *arg)
+{
+	struct tc_data *tc = arg;
+	u32 val;
+	int r;
+
+	r = regmap_read(tc->regmap, INTSTS_G, &val);
+	if (r)
+		return IRQ_NONE;
+
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & INT_SYSERR) {
+		u32 stat = 0;
+
+		regmap_read(tc->regmap, SYSSTAT, &stat);
+
+		dev_err(tc->dev, "syserr %x\n", stat);
+	}
+
+	if (tc->hpd_pin >= 0 && tc->bridge.dev) {
+		/*
+		 * H is triggered when the GPIO goes high.
+		 *
+		 * LC is triggered when the GPIO goes low and stays low for
+		 * the duration of LCNT
+		 */
+		bool h = val & INT_GPIO_H(tc->hpd_pin);
+		bool lc = val & INT_GPIO_LC(tc->hpd_pin);
+
+		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_pin,
+			h ? "H" : "", lc ? "LC" : "");
+
+		if (h || lc)
+			drm_kms_helper_hotplug_event(tc->bridge.dev);
+	}
+
+	regmap_write(tc->regmap, INTSTS_G, val);
+
+	return IRQ_HANDLED;
+}
+
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
@@ -1366,6 +1462,33 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "toshiba,hpd-pin",
+				   &tc->hpd_pin);
+	if (ret) {
+		tc->hpd_pin = -ENODEV;
+	} else {
+		if (tc->hpd_pin < 0 || tc->hpd_pin > 1) {
+			dev_err(dev, "failed to parse HPD number\n");
+			return ret;
+		}
+	}
+
+	if (client->irq > 0) {
+		/* enable SysErr */
+		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
+
+		ret = devm_request_threaded_irq(dev, client->irq,
+						NULL, tc_irq_handler,
+						IRQF_ONESHOT,
+						"tc358767-irq", tc);
+		if (ret) {
+			dev_err(dev, "failed to register dp interrupt\n");
+			return ret;
+		}
+
+		tc->have_irq = true;
+	}
+
 	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
 	if (ret) {
 		dev_err(tc->dev, "can not read device ID: %d\n", ret);
@@ -1379,6 +1502,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
 
+	if (tc->hpd_pin >= 0) {
+		u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
+		u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin);
+
+		/* Set LCNT to 2ms */
+		regmap_write(tc->regmap, lcnt_reg,
+			     clk_get_rate(tc->refclk) * 2 / 1000);
+		/* We need the "alternate" mode for HPD */
+		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin));
+
+		if (tc->have_irq) {
+			/* enable H & LC */
+			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
+		}
+	}
+
 	ret = tc_aux_link_setup(tc);
 	if (ret)
 		return ret;
@@ -1391,12 +1530,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);
@@ -1404,9 +1537,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: [PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Andrzej Hajda]

On 03.05.2019 14:29, Tomi Valkeinen wrote:
> Add support for interrupt and hotplug handling. Both are optional.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 166 ++++++++++++++++++++++++++----
>  1 file changed, 148 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7c275b8bbabc..b8cfeb2335cb 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -71,6 +71,7 @@
>  
>  /* System */
>  #define TC_IDREG		0x0500
> +#define SYSSTAT			0x0508
>  #define SYSCTRL			0x0510
>  #define DP0_AUDSRC_NO_INPUT		(0 << 3)
>  #define DP0_AUDSRC_I2S_RX		(1 << 3)
> @@ -79,9 +80,16 @@
>  #define DP0_VIDSRC_DPI_RX		(2 << 0)
>  #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
>  #define GPIOM			0x0540
> +#define GPIOC			0x0544
> +#define GPIOO			0x0548
>  #define GPIOI			0x054c
>  #define INTCTL_G		0x0560
>  #define INTSTS_G		0x0564
> +
> +#define INT_SYSERR		BIT(16)
> +#define INT_GPIO_H(x)		(1 << (x == 0 ? 2 : 10))
> +#define INT_GPIO_LC(x)		(1 << (x == 0 ? 3 : 11))
> +
>  #define INT_GP0_LCNT		0x0584
>  #define INT_GP1_LCNT		0x0588
>  
> @@ -219,6 +227,12 @@ struct tc_data {
>  	struct gpio_desc	*sd_gpio;
>  	struct gpio_desc	*reset_gpio;
>  	struct clk		*refclk;
> +
> +	/* do we have IRQ */
> +	bool			have_irq;
> +
> +	/* HPD pin number (0 or 1) or -ENODEV */
> +	int			hpd_pin;
>  };
>  
>  static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -1109,6 +1123,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);
> @@ -1214,19 +1234,43 @@ 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);
> +	bool conn;
> +	u32 val;
> +	int ret;
unused var
> +
> +	if (tc->hpd_pin < 0) {
> +		if (!tc->panel)
> +			return connector_status_unknown;
> +
> +		conn = true;
> +	} else {
> +		tc_read(GPIOI, &val);
> +
> +		conn = val & BIT(tc->hpd_pin);
> +	}
> +
> +	if (force && conn)
> +		tc_get_display_props(tc);


Why do you call tc_get_display_props here? It is called already in .enable.

If you need it for get_modes you can call it there. Here it looks unrelated.

Removing the call from here should also simplify function flow:

    if (tc->hpd_pin < 0)

        return tc->panel ? connector_status_connected :
connector_status_disconnected;

    tc_read(GPIOI, &val);

    return (val & BIT(tc->hpd_pin))? ? connector_status_connected :
connector_status_disconnected;


> +
> +	if (conn)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
> +
> +err:


unused label/code?


> +	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,
> @@ -1241,7 +1285,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 :
> @@ -1249,6 +1293,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>  	if (ret)
>  		return ret;
>  
> +	/* Don't poll if don't have HPD connected */
> +	if (tc->hpd_pin >= 0) {
> +		if (tc->have_irq)
> +			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +		else
> +			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
> +					       DRM_CONNECTOR_POLL_DISCONNECT;


Bikeshedding: wouldn't be more clear to use ?:  operator?


Regards

Andrzej


> +	}
> +
>  	if (tc->panel)
>  		drm_panel_attach(tc->panel, &tc->connector);
>  
> @@ -1315,6 +1368,49 @@ static const struct regmap_config tc_regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>  };
>  
> +static irqreturn_t tc_irq_handler(int irq, void *arg)
> +{
> +	struct tc_data *tc = arg;
> +	u32 val;
> +	int r;
> +
> +	r = regmap_read(tc->regmap, INTSTS_G, &val);
> +	if (r)
> +		return IRQ_NONE;
> +
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & INT_SYSERR) {
> +		u32 stat = 0;
> +
> +		regmap_read(tc->regmap, SYSSTAT, &stat);
> +
> +		dev_err(tc->dev, "syserr %x\n", stat);
> +	}
> +
> +	if (tc->hpd_pin >= 0 && tc->bridge.dev) {
> +		/*
> +		 * H is triggered when the GPIO goes high.
> +		 *
> +		 * LC is triggered when the GPIO goes low and stays low for
> +		 * the duration of LCNT
> +		 */
> +		bool h = val & INT_GPIO_H(tc->hpd_pin);
> +		bool lc = val & INT_GPIO_LC(tc->hpd_pin);
> +
> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_pin,
> +			h ? "H" : "", lc ? "LC" : "");
> +
> +		if (h || lc)
> +			drm_kms_helper_hotplug_event(tc->bridge.dev);
> +	}
> +
> +	regmap_write(tc->regmap, INTSTS_G, val);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> @@ -1366,6 +1462,33 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  	}
>  
> +	ret = of_property_read_u32(dev->of_node, "toshiba,hpd-pin",
> +				   &tc->hpd_pin);
> +	if (ret) {
> +		tc->hpd_pin = -ENODEV;
> +	} else {
> +		if (tc->hpd_pin < 0 || tc->hpd_pin > 1) {
> +			dev_err(dev, "failed to parse HPD number\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (client->irq > 0) {
> +		/* enable SysErr */
> +		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
> +
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						NULL, tc_irq_handler,
> +						IRQF_ONESHOT,
> +						"tc358767-irq", tc);
> +		if (ret) {
> +			dev_err(dev, "failed to register dp interrupt\n");
> +			return ret;
> +		}
> +
> +		tc->have_irq = true;
> +	}
> +
>  	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
>  	if (ret) {
>  		dev_err(tc->dev, "can not read device ID: %d\n", ret);
> @@ -1379,6 +1502,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
>  
> +	if (tc->hpd_pin >= 0) {
> +		u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
> +		u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin);
> +
> +		/* Set LCNT to 2ms */
> +		regmap_write(tc->regmap, lcnt_reg,
> +			     clk_get_rate(tc->refclk) * 2 / 1000);
> +		/* We need the "alternate" mode for HPD */
> +		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin));
> +
> +		if (tc->have_irq) {
> +			/* enable H & LC */
> +			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
> +		}
> +	}
> +
>  	ret = tc_aux_link_setup(tc);
>  	if (ret)
>  		return ret;
> @@ -1391,12 +1530,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);
> @@ -1404,9 +1537,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)


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

Re: [PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

On 21/05/2019 10:07, Andrzej Hajda wrote:

>> @@ -1214,19 +1234,43 @@ 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);
>> +	bool conn;
>> +	u32 val;
>> +	int ret;
> unused var

Needed for tc_write/read... =( Cleaning these up will be the next step.

>> +
>> +	if (tc->hpd_pin < 0) {
>> +		if (!tc->panel)
>> +			return connector_status_unknown;
>> +
>> +		conn = true;
>> +	} else {
>> +		tc_read(GPIOI, &val);
>> +
>> +		conn = val & BIT(tc->hpd_pin);
>> +	}
>> +
>> +	if (force && conn)
>> +		tc_get_display_props(tc);
> 
> 
> Why do you call tc_get_display_props here? It is called already in .enable.
> 
> If you need it for get_modes you can call it there. Here it looks unrelated.

Yes, it's needed for get_modes. Or more specifically, for tc_mode_valid. I agree it 
doesn't quite feel right, but I wouldn't say it's unrelated, or that this is a wrong place.

Afaics, we need tc_get_display_props in bridge_enable, for the case where we don't have 
hpd. We could call tc_get_display_props in get_modes, but I don't know if we always get a 
get_modes call. Or maybe we get multiple get_modes calls, and we do unnecessary 
tc_get_display_props calls.

Now that I wrote the above, it makes me wonder whether the get_modes works in the current 
patches if we don't have hpd...

We could cache tc_get_display_props results, too, but I'm not sure when to clear the 
cache, especially if we don't have hpd.

DisplayPort spec talks about doing the display-props reading and EDID reading when 
handling HPD.

I think it would be best to change the code so that we read display props and EDID in HPD, 
but so that we also can read them later (when needed, probably bridge enable and 
get_modes) if we haven't done the reads already. I've had this in mind since I started the 
series, but as it didn't feel like a simple change, I left it for later.

> Removing the call from here should also simplify function flow:
> 
>      if (tc->hpd_pin < 0)
> 
>          return tc->panel ? connector_status_connected :
> connector_status_disconnected;
> 
>      tc_read(GPIOI, &val);
> 
>      return (val & BIT(tc->hpd_pin))? ? connector_status_connected :
> connector_status_disconnected;
> 
> 
>> +
>> +	if (conn)
>> +		return connector_status_connected;
>> +	else
>> +		return connector_status_disconnected;
>> +
>> +err:
> 
> 
> unused label/code?

Needed for tc_write/read too.

>> @@ -1249,6 +1293,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>>   	if (ret)
>>   		return ret;
>>   
>> +	/* Don't poll if don't have HPD connected */
>> +	if (tc->hpd_pin >= 0) {
>> +		if (tc->have_irq)
>> +			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
>> +		else
>> +			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
>> +					       DRM_CONNECTOR_POLL_DISCONNECT;
> 
> 
> Bikeshedding: wouldn't be more clear to use ?:  operator?

Depends on the reader, I guess. I like ?: when the parameters are relatively simple (say, 
a single variable). Here it's a bit so-and-so with the second case's bitwise-or.

  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: [PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Andrzej Hajda]

On 21.05.2019 13:34, Tomi Valkeinen wrote:
> On 21/05/2019 10:07, Andrzej Hajda wrote:
>
>>> @@ -1214,19 +1234,43 @@ 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);
>>> +	bool conn;
>>> +	u32 val;
>>> +	int ret;
>> unused var
> Needed for tc_write/read... =( Cleaning these up will be the next step.


aah, I forgot about this pattern :)


>
>>> +
>>> +	if (tc->hpd_pin < 0) {
>>> +		if (!tc->panel)
>>> +			return connector_status_unknown;
>>> +
>>> +		conn = true;
>>> +	} else {
>>> +		tc_read(GPIOI, &val);
>>> +
>>> +		conn = val & BIT(tc->hpd_pin);
>>> +	}
>>> +
>>> +	if (force && conn)
>>> +		tc_get_display_props(tc);
>>
>> Why do you call tc_get_display_props here? It is called already in .enable.
>>
>> If you need it for get_modes you can call it there. Here it looks unrelated.
> Yes, it's needed for get_modes. Or more specifically, for tc_mode_valid. I agree it 
> doesn't quite feel right, but I wouldn't say it's unrelated, or that this is a wrong place.
>
> Afaics, we need tc_get_display_props in bridge_enable, for the case where we don't have 
> hpd. We could call tc_get_display_props in get_modes, but I don't know if we always get a 
> get_modes call. Or maybe we get multiple get_modes calls, and we do unnecessary 
> tc_get_display_props calls.


.detect can be also called multiple times.


>
> Now that I wrote the above, it makes me wonder whether the get_modes works in the current 
> patches if we don't have hpd...
>
> We could cache tc_get_display_props results, too, but I'm not sure when to clear the 
> cache, especially if we don't have hpd.


If I remember correctly without hpd userspace 'informs' driver that sink
is connected (via status sysfs property), in such case
.fill_modes/.get_modes is called on change.


>
> DisplayPort spec talks about doing the display-props reading and EDID reading when 
> handling HPD.
>
> I think it would be best to change the code so that we read display props and EDID in HPD, 
> but so that we also can read them later (when needed, probably bridge enable and 
> get_modes) if we haven't done the reads already. I've had this in mind since I started the 
> series, but as it didn't feel like a simple change, I left it for later.


My approach and experience suggest that detect, should be rather
lightweight and should not modify state, I am not even sure if it is
called at all on forced connector.


Regards

Andrzej


>
>> Removing the call from here should also simplify function flow:
>>
>>      if (tc->hpd_pin < 0)
>>
>>          return tc->panel ? connector_status_connected :
>> connector_status_disconnected;
>>
>>      tc_read(GPIOI, &val);
>>
>>      return (val & BIT(tc->hpd_pin))? ? connector_status_connected :
>> connector_status_disconnected;
>>
>>
>>> +
>>> +	if (conn)
>>> +		return connector_status_connected;
>>> +	else
>>> +		return connector_status_disconnected;
>>> +
>>> +err:
>>
>> unused label/code?
> Needed for tc_write/read too.
>
>>> @@ -1249,6 +1293,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> +	/* Don't poll if don't have HPD connected */
>>> +	if (tc->hpd_pin >= 0) {
>>> +		if (tc->have_irq)
>>> +			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
>>> +		else
>>> +			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
>>> +					       DRM_CONNECTOR_POLL_DISCONNECT;
>>
>> Bikeshedding: wouldn't be more clear to use ?:  operator?
> Depends on the reader, I guess. I like ?: when the parameters are relatively simple (say, 
> a single variable). Here it's a bit so-and-so with the second case's bitwise-or.
>
>   Tomi
>

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

Re: [PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

On 21/05/2019 17:18, Andrzej Hajda wrote:

>> DisplayPort spec talks about doing the display-props reading and EDID reading when
>> handling HPD.
>>
>> I think it would be best to change the code so that we read display props and EDID in HPD,
>> but so that we also can read them later (when needed, probably bridge enable and
>> get_modes) if we haven't done the reads already. I've had this in mind since I started the
>> series, but as it didn't feel like a simple change, I left it for later.
> 
> 
> My approach and experience suggest that detect, should be rather
> lightweight and should not modify state, I am not even sure if it is
> called at all on forced connector.

I just realized that this is not exactly perfect...

Link training can adjust the link speed and/or number of lanes, although 
the driver doesn't support this at the moment. The speed and number of 
lanes affect the video modes that are possible, so they affect get_modes.

So... I think the driver should set up the link fully before get_modes 
get called, instead of leaving the link setup to the point where we 
enable the bridge. Maybe... This is not exactly clear to me =).

In any case, I think that's future work. I have changed the code to read 
the display props in get_modes(), and I have another small fix too. I'll 
send v4 this week, and hopefully we can get this merged.

  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: [PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Andrzej Hajda]

On 27.05.2019 17:11, Tomi Valkeinen wrote:
> On 21/05/2019 17:18, Andrzej Hajda wrote:
>
>>> DisplayPort spec talks about doing the display-props reading and EDID reading when
>>> handling HPD.
>>>
>>> I think it would be best to change the code so that we read display props and EDID in HPD,
>>> but so that we also can read them later (when needed, probably bridge enable and
>>> get_modes) if we haven't done the reads already. I've had this in mind since I started the
>>> series, but as it didn't feel like a simple change, I left it for later.
>>
>> My approach and experience suggest that detect, should be rather
>> lightweight and should not modify state, I am not even sure if it is
>> called at all on forced connector.
> I just realized that this is not exactly perfect...
>
> Link training can adjust the link speed and/or number of lanes, although 
> the driver doesn't support this at the moment. The speed and number of 
> lanes affect the video modes that are possible, so they affect get_modes.
>
> So... I think the driver should set up the link fully before get_modes 
> get called, instead of leaving the link setup to the point where we 
> enable the bridge. Maybe... This is not exactly clear to me =).


Moreover link state can change during work, so full implementation
should handle HPD pulses (below 1ms if I remember correctly) re-train if
necessary and use drm_connector_set_link_status_property as well :)


Regards

Andrzej


>
> In any case, I think that's future work. I have changed the code to read 
> the display props in get_modes(), and I have another small fix too. I'll 
> send v4 this week, and hopefully we can get this merged.
>
>   Tomi
>

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

[PATCHv3 23/23] dt-bindings: tc358767: add HPD support

[Tomi Valkeinen]

Add DT property for defining the pin used for HPD.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/display/bridge/toshiba,tc358767.txt      | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
index e3f6aa6a214d..583c5e9dbe6b 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
@@ -12,6 +12,7 @@ Optional properties:
                    (active high shutdown input)
  - reset-gpios: OF device-tree gpio specification for RSTX pin
                 (active low system reset)
+ - toshiba,hpd-pin: TC358767 GPIO pin number to which HPD is connected to (0 or 1)
  - ports: the ports node can contain video interface port nodes to connect
    to a DPI/DSI source and to an eDP/DP sink according to [1][2]:
     - port@0: DSI input port
-- 
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