Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Sam Ravnborg <sam@ravnborg.org>, Wolfram Sang <wsa@kernel.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	robdclark@chromium.org,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
	Steev Klimaszewski <steev@kali.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-arm-msm@vger.kernel.org, Linus W <linus.walleij@linaro.org>,
	Douglas Anderson <dianders@chromium.org>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Robert Foss <robert.foss@linaro.org>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: [PATCH v4 19/27] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable
Date: Fri, 16 Apr 2021 15:39:42 -0700
Message-ID: <20210416153909.v4.19.Ie8cf556114953c6e7634564cc0d3ddbd103cb96c@changeid> (raw)
In-Reply-To: <20210416223950.3586967-1-dianders@chromium.org>

Let's reorganize how we init and turn on the reference clock in the
code to allow us to turn it on early (even before pre_enable()) so
that we can read the EDID early. This is handy for eDP because:
- We always assume that a panel is there.
- Once we report that a panel is there we get asked to read the EDID.
- Pre-enable isn't called until we know what pixel clock we want to
  use and we're ready to turn everything on. That's _after_ we get
  asked to read the EDID.

NOTE: the above only works out OK if we "refclk" is provided. Though I
don't have access to any hardware that uses ti-sn65dsi86 and _doesn't_
provide a "refclk", I believe that we'll have trouble reading the EDID
at bootup in that case. Specifically I believe that if there's no
"refclk" we need the MIPI source clock to be active before we can
successfully read the EDID. My evidence here is that, in testing, I
couldn't read the EDID until I turned on the DPPLL in the bridge chip
and that the DPPLL needs the input clock to be active.

Since this is hard to support, let's punt trying to handle this case
if there's no "refclk". In that case we'll enable comms in
pre_enable() like we always did.

I don't believe there are any users of the ti-sn65dsi86 bridge chip
that _don't_ use "refclk". The bridge chip is _very_ inflexible in
that mode. The only time I've seen that mode used was for some really
early prototype hardware that was thrown in the e-waste bin years ago
when we realized how inflexible it was.

Even if someone is using the bridge chip without the "refclk" they're
in no worse shape than they were before the (fairly recent) commit
58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index b3c699da7724..875e5dbe6594 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -132,6 +132,8 @@
  * @dp_lanes:     Count of dp_lanes we're using.
  * @ln_assign:    Value to program to the LN_ASSIGN register.
  * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
+ * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @comms_mutex:   Protects modification of comms_enabled.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -162,6 +164,8 @@ struct ti_sn65dsi86 {
 	int				dp_lanes;
 	u8				ln_assign;
 	u8				ln_polrs;
+	bool				comms_enabled;
+	struct mutex			comms_mutex;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -250,6 +254,47 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
 			   REFCLK_FREQ(i));
 }
 
+static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
+{
+	mutex_lock(&pdata->comms_mutex);
+
+	/* configure bridge ref_clk */
+	ti_sn_bridge_set_refclk_freq(pdata);
+
+	/*
+	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
+	 * so the HPD is an internal signal that's only there to signal that
+	 * the panel is done powering up.  ...but the bridge chip debounces
+	 * this signal by between 100 ms and 400 ms (depending on process,
+	 * voltage, and temperate--I measured it at about 200 ms).  One
+	 * particular panel asserted HPD 84 ms after it was powered on meaning
+	 * that we saw HPD 284 ms after power on.  ...but the same panel said
+	 * that instead of looking at HPD you could just hardcode a delay of
+	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
+	 * delay in its prepare and always disable HPD.
+	 *
+	 * If HPD somehow makes sense on some future panel we'll have to
+	 * change this to be conditional on someone specifying that HPD should
+	 * be used.
+	 */
+	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
+			   HPD_DISABLE);
+
+	pdata->comms_enabled = true;
+
+	mutex_unlock(&pdata->comms_mutex);
+}
+
+static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
+{
+	mutex_lock(&pdata->comms_mutex);
+
+	pdata->comms_enabled = false;
+	clk_disable_unprepare(pdata->refclk);
+
+	mutex_unlock(&pdata->comms_mutex);
+}
+
 static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
@@ -263,6 +308,16 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 
 	gpiod_set_value(pdata->enable_gpio, 1);
 
+	/*
+	 * If we have a reference clock we can enable communication w/ the
+	 * panel (including the aux channel) w/out any need for an input clock
+	 * so we can do it in resume which lets us read the EDID before
+	 * pre_enable(). Without a reference clock we need the MIPI reference
+	 * clock so reading early doesn't work.
+	 */
+	if (pdata->refclk)
+		ti_sn65dsi86_enable_comms(pdata);
+
 	return ret;
 }
 
@@ -271,6 +326,9 @@ static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev)
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
 	int ret;
 
+	if (pdata->refclk)
+		ti_sn65dsi86_disable_comms(pdata);
+
 	gpiod_set_value(pdata->enable_gpio, 0);
 
 	ret = regulator_bulk_disable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -843,27 +901,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 
 	pm_runtime_get_sync(pdata->dev);
 
-	/* configure bridge ref_clk */
-	ti_sn_bridge_set_refclk_freq(pdata);
-
-	/*
-	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
-	 * so the HPD is an internal signal that's only there to signal that
-	 * the panel is done powering up.  ...but the bridge chip debounces
-	 * this signal by between 100 ms and 400 ms (depending on process,
-	 * voltage, and temperate--I measured it at about 200 ms).  One
-	 * particular panel asserted HPD 84 ms after it was powered on meaning
-	 * that we saw HPD 284 ms after power on.  ...but the same panel said
-	 * that instead of looking at HPD you could just hardcode a delay of
-	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
-	 * delay in its prepare and always disable HPD.
-	 *
-	 * If HPD somehow makes sense on some future panel we'll have to
-	 * change this to be conditional on someone specifying that HPD should
-	 * be used.
-	 */
-	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
-			   HPD_DISABLE);
+	if (!pdata->refclk)
+		ti_sn65dsi86_enable_comms(pdata);
 
 	drm_panel_prepare(pdata->panel);
 }
@@ -874,7 +913,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 
 	drm_panel_unprepare(pdata->panel);
 
-	clk_disable_unprepare(pdata->refclk);
+	if (!pdata->refclk)
+		ti_sn65dsi86_disable_comms(pdata);
 
 	pm_runtime_put_sync(pdata->dev);
 }
@@ -908,6 +948,20 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	if (len > SN_AUX_MAX_PAYLOAD_BYTES)
 		return -EINVAL;
 
+	pm_runtime_get_sync(pdata->dev);
+	mutex_lock(&pdata->comms_mutex);
+
+	/*
+	 * If someone tries to do a DDC over AUX transaction before pre_enable()
+	 * on a device without a dedicated reference clock then we just can't
+	 * do it. Fail right away. This prevents non-refclk users from reading
+	 * the EDID before enabling the panel but such is life.
+	 */
+	if (!pdata->comms_enabled) {
+		ret = -EIO;
+		goto exit;
+	}
+
 	switch (request) {
 	case DP_AUX_NATIVE_WRITE:
 	case DP_AUX_I2C_WRITE:
@@ -918,7 +972,8 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		msg->reply = 0;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32));
@@ -942,11 +997,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val,
 				       !(val & AUX_CMD_SEND), 0, 50 * 1000);
 	if (ret)
-		return ret;
+		goto exit;
 
 	ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
 	if (ret)
-		return ret;
+		goto exit;
 
 	if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
 		/*
@@ -954,13 +1009,14 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 		 * but it hit a timeout. We ignore defers here because they're
 		 * handled in hardware.
 		 */
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
+		goto exit;
 	}
 
 	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
 		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
 		if (ret)
-			return ret;
+			goto exit;
 	} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
 		switch (request) {
 		case DP_AUX_I2C_WRITE:
@@ -972,18 +1028,19 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
 			break;
 		}
-		return 0;
+		len = 0;
+		goto exit;
 	}
 
-	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
-	    len == 0)
-		return len;
+	if (request != DP_AUX_NATIVE_WRITE && request != DP_AUX_I2C_WRITE && len != 0)
+		ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
 
-	ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
-	if (ret)
-		return ret;
+exit:
+	mutex_unlock(&pdata->comms_mutex);
+	pm_runtime_mark_last_busy(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
 
-	return len;
+	return ret ? ret : len;
 }
 
 static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
@@ -1380,6 +1437,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
+	mutex_init(&pdata->comms_mutex);
+
 	pdata->regmap = devm_regmap_init_i2c(client,
 					     &ti_sn65dsi86_regmap_config);
 	if (IS_ERR(pdata->regmap)) {
-- 
2.31.1.368.gbe11c130af-goog


  parent reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-16 22:39 [PATCH v4 00/27] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 01/27] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 02/27] drm/bridge: ti-sn65dsi86: Simplify refclk handling Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 03/27] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 04/27] drm/bridge: ti-sn65dsi86: Reorder remove() Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 05/27] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 06/27] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 07/27] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare Douglas Anderson
2021-04-16 22:39 ` [PATCH v4 08/27] drm/bridge: ti-sn65dsi86: Rename the main driver data structure Douglas Anderson
2021-04-23 14:28   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 09/27] drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices Douglas Anderson
2021-04-23 14:30   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 10/27] drm/bridge: ti-sn65dsi86: Clean debugfs code Douglas Anderson
2021-04-23 14:35   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 11/27] drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe Douglas Anderson
2021-04-23 14:38   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 12/27] drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata Douglas Anderson
2021-04-23 14:38   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 13/27] drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable Douglas Anderson
2021-04-23 14:39   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 14/27] drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start Douglas Anderson
2021-04-23 14:41   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 15/27] drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers Douglas Anderson
2021-04-23 14:49   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 16/27] drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code Douglas Anderson
2021-04-23 14:50   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 17/27] drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend Douglas Anderson
2021-04-23 14:51   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 18/27] drm/bridge: ti-sn65dsi86: Code motion of refclk management functions Douglas Anderson
2021-04-23 14:51   ` Bjorn Andersson
2021-04-16 22:39 ` Douglas Anderson [this message]
2021-04-23 14:56   ` [PATCH v4 19/27] drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 20/27] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev Douglas Anderson
2021-04-23 14:59   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 21/27] i2c: i2c-core-of: Fix corner case of finding adapter by node Douglas Anderson
2021-04-23 15:12   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 22/27] drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
2021-04-23 15:15   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 23/27] drm/panel: panel-simple: Power the panel when reading the EDID Douglas Anderson
2021-04-23 15:16   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 24/27] drm/panel: panel-simple: Cache the EDID as long as we retain power Douglas Anderson
2021-04-23 16:12   ` Bjorn Andersson
2021-04-23 16:30     ` Doug Anderson
2021-04-16 22:39 ` [PATCH v4 25/27] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC Douglas Anderson
2021-04-23 16:13   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 26/27] arm64: dts: qcom: Link the panel to the bridge's DDC bus Douglas Anderson
2021-04-23 16:16   ` Bjorn Andersson
2021-04-16 22:39 ` [PATCH v4 27/27] drm/panel: panel-simple: Prepare/unprepare are refcounted, not forced Douglas Anderson
2021-04-23 16:16   ` Bjorn Andersson
2021-04-23 16:46   ` Doug Anderson
2021-04-20 16:42 ` [PATCH v4 00/27] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210416153909.v4.19.Ie8cf556114953c6e7634564cc0d3ddbd103cb96c@changeid \
    --to=dianders@chromium.org \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=narmstrong@baylibre.com \
    --cc=robdclark@chromium.org \
    --cc=robert.foss@linaro.org \
    --cc=sam@ravnborg.org \
    --cc=stanislav.lisovskiy@intel.com \
    --cc=steev@kali.org \
    --cc=swboyd@chromium.org \
    --cc=wsa@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git