[PATCH v8 0/4] Add support for the eDP panel over aux_bus

[PATCH v8 0/4] Add support for the eDP panel over aux_bus

[Sankeerth Billakanti]

This series adds support for generic eDP panel over aux_bus.

These changes are dependent on the following patches:
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-5-dmitry.baryshkov@linaro.org/
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-6-dmitry.baryshkov@linaro.org/

Sankeerth Billakanti (4):
  drm/msm/dp: Add eDP support via aux_bus
  drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
  drm/msm/dp: wait for hpd high before aux transaction
  drm/msm/dp: Support the eDP modes given by panel

 drivers/gpu/drm/msm/dp/dp_aux.c     |  21 +++++++-
 drivers/gpu/drm/msm/dp/dp_aux.h     |   3 +-
 drivers/gpu/drm/msm/dp/dp_catalog.c |  31 ++++++++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 101 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |   1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     |  21 ++++++--
 drivers/gpu/drm/msm/dp/dp_parser.c  |  23 +-------
 drivers/gpu/drm/msm/dp/dp_parser.h  |  13 ++++-
 9 files changed, 175 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH v8 0/4] Add support for the eDP panel over aux_bus

[Sankeerth Billakanti]

This series adds support for generic eDP panel over aux_bus.

These changes are dependent on the following patches:
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-5-dmitry.baryshkov@linaro.org/
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-6-dmitry.baryshkov@linaro.org/

Sankeerth Billakanti (4):
  drm/msm/dp: Add eDP support via aux_bus
  drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
  drm/msm/dp: wait for hpd high before aux transaction
  drm/msm/dp: Support the eDP modes given by panel

 drivers/gpu/drm/msm/dp/dp_aux.c     |  21 +++++++-
 drivers/gpu/drm/msm/dp/dp_aux.h     |   3 +-
 drivers/gpu/drm/msm/dp/dp_catalog.c |  31 ++++++++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 101 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |   1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     |  21 ++++++--
 drivers/gpu/drm/msm/dp/dp_parser.c  |  23 +-------
 drivers/gpu/drm/msm/dp/dp_parser.h  |  13 ++++-
 9 files changed, 175 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti]

This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.

The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v8:
  - handle corner cases
  - add comment for the bridge ops

Changes in v7:
  - aux_bus is mandatory for eDP
  - connector type check modified to just check for eDP

Changes in v6:
  - Remove initialization
  - Fix aux_bus node leak
  - Split the patches

 drivers/gpu/drm/msm/dp/dp_display.c | 69 ++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     | 21 ++++++++---
 drivers/gpu/drm/msm/dp/dp_parser.c  | 23 ++-----------
 drivers/gpu/drm/msm/dp/dp_parser.h  | 13 ++++++-
 5 files changed, 97 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..3d03c77 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
 #include <linux/component.h>
 #include <linux/of_irq.h>
 #include <linux/delay.h>
+#include <drm/dp/drm_dp_aux_bus.h>
 
 #include "msm_drv.h"
 #include "msm_kms.h"
@@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
-	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
+	rc = dp->parser->parse(dp->parser);
 	if (rc) {
 		DRM_ERROR("device tree parsing failed\n");
 		goto end;
 	}
 
-	dp->dp_display.next_bridge = dp->parser->next_bridge;
-
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
 	if (rc) {
@@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev)
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
 	dp->dp_display.connector_type = desc->connector_type;
+	dp->dp_display.is_edp =
+		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
 
 	rc = dp_init_sub_modules(dp);
 	if (rc) {
@@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 
 	dp_hpd_event_setup(dp);
 
-	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+	if (!dp_display->is_edp)
+		dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
 }
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
@@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 	}
 }
 
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+	int rc;
+	struct dp_display_private *dp_priv;
+	struct device_node *aux_bus;
+	struct device *dev;
+
+	dp_priv = container_of(dp, struct dp_display_private, dp_display);
+	dev = &dp_priv->pdev->dev;
+	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+	if (aux_bus && dp->is_edp) {
+		dp_display_host_init(dp_priv);
+		dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+		dp_display_host_phy_init(dp_priv);
+		enable_irq(dp_priv->irq);
+
+		rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+		of_node_put(aux_bus);
+		if (rc)
+			goto edp_error;
+	} else if (dp->is_edp) {
+		DRM_ERROR("eDP aux_bus not found\n");
+		rc = -ENODEV;
+		goto error;
+	}
+
+	/*
+	 * External bridges are mandatory for eDP interfaces: one has to
+	 * provide at least an eDP panel (which gets wrapped into panel-bridge).
+	 *
+	 * For DisplayPort interfaces external bridges are optional, so
+	 * silently ignore an error if one is not present (-ENODEV).
+	 */
+	rc = dp_parser_find_next_bridge(dp_priv->parser);
+	if (rc && dp->is_edp) {
+		DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
+		goto edp_error;
+	} else if (rc && rc != -ENODEV) {
+		DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
+		goto error;
+	}
+
+	dp->next_bridge = dp_priv->parser->next_bridge;
+
+	return 0;
+
+edp_error:
+	disable_irq(dp_priv->irq);
+	dp_display_host_phy_exit(dp_priv);
+	dp_display_host_deinit(dp_priv);
+error:
+	return rc;
+}
+
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
@@ -1553,6 +1610,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 
 	dp_display->encoder = encoder;
 
+	ret = dp_display_get_next_bridge(dp_display);
+	if (ret)
+		return ret;
+
 	dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
 	if (IS_ERR(dp_display->bridge)) {
 		ret = PTR_ERR(dp_display->bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 49a1d89..1377cc3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -21,6 +21,7 @@ struct msm_dp {
 	bool audio_enabled;
 	bool power_on;
 	unsigned int connector_type;
+	bool is_edp;
 
 	hdmi_codec_plugged_cb plugged_cb;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7ce1aca..8a75c55 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,23 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
 	bridge->funcs = &dp_bridge_ops;
 	bridge->type = dp_display->connector_type;
 
-	bridge->ops =
-		DRM_BRIDGE_OP_DETECT |
-		DRM_BRIDGE_OP_HPD |
-		DRM_BRIDGE_OP_MODES;
+	/*
+	 * Many ops only make sense for DP. Why?
+	 * - Detect/HPD are used by DRM to know if a display is _physically_
+	 *   there, not whether the display is powered on / finished initting.
+	 *   On eDP we assume the display is always there because you can't
+	 *   know until power is applied. If we don't implement the ops DRM will
+	 *   assume our display is always there.
+	 * - Currently eDP mode reading is driven by the panel driver. This
+	 *   allows the panel driver to properly power itself on to read the
+	 *   modes.
+	 */
+	if (!dp_display->is_edp) {
+		bridge->ops =
+			DRM_BRIDGE_OP_DETECT |
+			DRM_BRIDGE_OP_HPD |
+			DRM_BRIDGE_OP_MODES;
+	}
 
 	rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 1056b8d..4bdbf91 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
 	return 0;
 }
 
-static int dp_parser_find_next_bridge(struct dp_parser *parser)
+int dp_parser_find_next_bridge(struct dp_parser *parser)
 {
 	struct device *dev = &parser->pdev->dev;
 	struct drm_bridge *bridge;
@@ -279,7 +279,7 @@ static int dp_parser_find_next_bridge(struct dp_parser *parser)
 	return 0;
 }
 
-static int dp_parser_parse(struct dp_parser *parser, int connector_type)
+static int dp_parser_parse(struct dp_parser *parser)
 {
 	int rc = 0;
 
@@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type)
 	if (rc)
 		return rc;
 
-	/*
-	 * External bridges are mandatory for eDP interfaces: one has to
-	 * provide at least an eDP panel (which gets wrapped into panel-bridge).
-	 *
-	 * For DisplayPort interfaces external bridges are optional, so
-	 * silently ignore an error if one is not present (-ENODEV).
-	 */
-	rc = dp_parser_find_next_bridge(parser);
-	if (rc == -ENODEV) {
-		if (connector_type == DRM_MODE_CONNECTOR_eDP) {
-			DRM_ERROR("eDP: next bridge is not present\n");
-			return rc;
-		}
-	} else if (rc) {
-		if (rc != -EPROBE_DEFER)
-			DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
-		return rc;
-	}
-
 	/* Map the corresponding regulator information according to
 	 * version. Currently, since we only have one supported platform,
 	 * mapping the regulator directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index d371bae..950416c 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -125,7 +125,7 @@ struct dp_parser {
 	u32 max_dp_lanes;
 	struct drm_bridge *next_bridge;
 
-	int (*parse)(struct dp_parser *parser, int connector_type);
+	int (*parse)(struct dp_parser *parser);
 };
 
 /**
@@ -141,4 +141,15 @@ struct dp_parser {
  */
 struct dp_parser *dp_parser_get(struct platform_device *pdev);
 
+/**
+ * dp_parser_find_next_bridge() - find an additional bridge to DP
+ *
+ * @parser: dp_parser data from client
+ * return: 0 if able to get the bridge else return an error code
+ *
+ * This function is used to find any additional bridge attached to
+ * the DP controller. The eDP interface requires a panel bridge.
+ */
+int dp_parser_find_next_bridge(struct dp_parser *parser);
+
 #endif
-- 
2.7.4

[PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti]

This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.

The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v8:
  - handle corner cases
  - add comment for the bridge ops

Changes in v7:
  - aux_bus is mandatory for eDP
  - connector type check modified to just check for eDP

Changes in v6:
  - Remove initialization
  - Fix aux_bus node leak
  - Split the patches

 drivers/gpu/drm/msm/dp/dp_display.c | 69 ++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     | 21 ++++++++---
 drivers/gpu/drm/msm/dp/dp_parser.c  | 23 ++-----------
 drivers/gpu/drm/msm/dp/dp_parser.h  | 13 ++++++-
 5 files changed, 97 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..3d03c77 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
 #include <linux/component.h>
 #include <linux/of_irq.h>
 #include <linux/delay.h>
+#include <drm/dp/drm_dp_aux_bus.h>
 
 #include "msm_drv.h"
 #include "msm_kms.h"
@@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
-	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
+	rc = dp->parser->parse(dp->parser);
 	if (rc) {
 		DRM_ERROR("device tree parsing failed\n");
 		goto end;
 	}
 
-	dp->dp_display.next_bridge = dp->parser->next_bridge;
-
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
 	if (rc) {
@@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev)
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
 	dp->dp_display.connector_type = desc->connector_type;
+	dp->dp_display.is_edp =
+		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
 
 	rc = dp_init_sub_modules(dp);
 	if (rc) {
@@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 
 	dp_hpd_event_setup(dp);
 
-	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+	if (!dp_display->is_edp)
+		dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
 }
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
@@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 	}
 }
 
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+	int rc;
+	struct dp_display_private *dp_priv;
+	struct device_node *aux_bus;
+	struct device *dev;
+
+	dp_priv = container_of(dp, struct dp_display_private, dp_display);
+	dev = &dp_priv->pdev->dev;
+	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+	if (aux_bus && dp->is_edp) {
+		dp_display_host_init(dp_priv);
+		dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+		dp_display_host_phy_init(dp_priv);
+		enable_irq(dp_priv->irq);
+
+		rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+		of_node_put(aux_bus);
+		if (rc)
+			goto edp_error;
+	} else if (dp->is_edp) {
+		DRM_ERROR("eDP aux_bus not found\n");
+		rc = -ENODEV;
+		goto error;
+	}
+
+	/*
+	 * External bridges are mandatory for eDP interfaces: one has to
+	 * provide at least an eDP panel (which gets wrapped into panel-bridge).
+	 *
+	 * For DisplayPort interfaces external bridges are optional, so
+	 * silently ignore an error if one is not present (-ENODEV).
+	 */
+	rc = dp_parser_find_next_bridge(dp_priv->parser);
+	if (rc && dp->is_edp) {
+		DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
+		goto edp_error;
+	} else if (rc && rc != -ENODEV) {
+		DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
+		goto error;
+	}
+
+	dp->next_bridge = dp_priv->parser->next_bridge;
+
+	return 0;
+
+edp_error:
+	disable_irq(dp_priv->irq);
+	dp_display_host_phy_exit(dp_priv);
+	dp_display_host_deinit(dp_priv);
+error:
+	return rc;
+}
+
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
@@ -1553,6 +1610,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 
 	dp_display->encoder = encoder;
 
+	ret = dp_display_get_next_bridge(dp_display);
+	if (ret)
+		return ret;
+
 	dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
 	if (IS_ERR(dp_display->bridge)) {
 		ret = PTR_ERR(dp_display->bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 49a1d89..1377cc3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -21,6 +21,7 @@ struct msm_dp {
 	bool audio_enabled;
 	bool power_on;
 	unsigned int connector_type;
+	bool is_edp;
 
 	hdmi_codec_plugged_cb plugged_cb;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7ce1aca..8a75c55 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,23 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
 	bridge->funcs = &dp_bridge_ops;
 	bridge->type = dp_display->connector_type;
 
-	bridge->ops =
-		DRM_BRIDGE_OP_DETECT |
-		DRM_BRIDGE_OP_HPD |
-		DRM_BRIDGE_OP_MODES;
+	/*
+	 * Many ops only make sense for DP. Why?
+	 * - Detect/HPD are used by DRM to know if a display is _physically_
+	 *   there, not whether the display is powered on / finished initting.
+	 *   On eDP we assume the display is always there because you can't
+	 *   know until power is applied. If we don't implement the ops DRM will
+	 *   assume our display is always there.
+	 * - Currently eDP mode reading is driven by the panel driver. This
+	 *   allows the panel driver to properly power itself on to read the
+	 *   modes.
+	 */
+	if (!dp_display->is_edp) {
+		bridge->ops =
+			DRM_BRIDGE_OP_DETECT |
+			DRM_BRIDGE_OP_HPD |
+			DRM_BRIDGE_OP_MODES;
+	}
 
 	rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 1056b8d..4bdbf91 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
 	return 0;
 }
 
-static int dp_parser_find_next_bridge(struct dp_parser *parser)
+int dp_parser_find_next_bridge(struct dp_parser *parser)
 {
 	struct device *dev = &parser->pdev->dev;
 	struct drm_bridge *bridge;
@@ -279,7 +279,7 @@ static int dp_parser_find_next_bridge(struct dp_parser *parser)
 	return 0;
 }
 
-static int dp_parser_parse(struct dp_parser *parser, int connector_type)
+static int dp_parser_parse(struct dp_parser *parser)
 {
 	int rc = 0;
 
@@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type)
 	if (rc)
 		return rc;
 
-	/*
-	 * External bridges are mandatory for eDP interfaces: one has to
-	 * provide at least an eDP panel (which gets wrapped into panel-bridge).
-	 *
-	 * For DisplayPort interfaces external bridges are optional, so
-	 * silently ignore an error if one is not present (-ENODEV).
-	 */
-	rc = dp_parser_find_next_bridge(parser);
-	if (rc == -ENODEV) {
-		if (connector_type == DRM_MODE_CONNECTOR_eDP) {
-			DRM_ERROR("eDP: next bridge is not present\n");
-			return rc;
-		}
-	} else if (rc) {
-		if (rc != -EPROBE_DEFER)
-			DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
-		return rc;
-	}
-
 	/* Map the corresponding regulator information according to
 	 * version. Currently, since we only have one supported platform,
 	 * mapping the regulator directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index d371bae..950416c 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -125,7 +125,7 @@ struct dp_parser {
 	u32 max_dp_lanes;
 	struct drm_bridge *next_bridge;
 
-	int (*parse)(struct dp_parser *parser, int connector_type);
+	int (*parse)(struct dp_parser *parser);
 };
 
 /**
@@ -141,4 +141,15 @@ struct dp_parser {
  */
 struct dp_parser *dp_parser_get(struct platform_device *pdev);
 
+/**
+ * dp_parser_find_next_bridge() - find an additional bridge to DP
+ *
+ * @parser: dp_parser data from client
+ * return: 0 if able to get the bridge else return an error code
+ *
+ * This function is used to find any additional bridge attached to
+ * the DP controller. The eDP interface requires a panel bridge.
+ */
+int dp_parser_find_next_bridge(struct dp_parser *parser);
+
 #endif
-- 
2.7.4

[PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti]

The panel-edp enables the eDP panel power during probe, get_modes
and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.

So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v8:
  - add comment explaining the interrupt status return

Changes in v7:
  - reordered the patch in the series
  - modified the return statement for isr
  - connector check modified to just check for eDP

 drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
 drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..3a298e9 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
 
 	u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
 
-	/* enable HPD plug and unplug interrupts */
-	dp_catalog_hpd_config_intr(dp_catalog,
-		DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
 	/* Configure REFTIMER and enable it */
 	reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
 	dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
@@ -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
 {
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 				struct dp_catalog_private, dp_catalog);
-	int isr = 0;
+	int isr, mask;
 
 	isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
 	dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
 				 (isr & DP_DP_HPD_INT_MASK));
+	mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
 
-	return isr;
+	/*
+	 * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
+	 * irrespective of their masked status. The HW interrupt will not be
+	 * generated if an interrupt is masked. However, the interrupt status
+	 * bit in the register will still be set. The eDP controller driver
+	 * masks the plug and unplug interrupts unlike DP controller which
+	 * unmasks all the interrupts. So, do not report the status of the
+	 * masked interrupts.
+	 */
+	return isr & (mask | ~DP_DP_HPD_INT_MASK);
 }
 
 int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3d03c77..874258d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	dp_display_handle_plugged_change(&dp->dp_display, false);
 
 	/* enable HDP plug interrupt to prepare for next plugin */
-	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+	if (!dp->dp_display.is_edp)
+		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
 
 	DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
 			dp->dp_display.connector_type, state);
@@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
 	dp_display_host_init(dp);
 	dp_catalog_ctrl_hpd_config(dp->catalog);
 
+	/* Enable plug and unplug interrupts only for external DisplayPort */
+	if (!dp->dp_display.is_edp)
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+
 	/* Enable interrupt first time
 	 * we are leaving dp clocks on during disconnect
 	 * and never disable interrupt
@@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
 	dp_catalog_ctrl_hpd_config(dp->catalog);
 
 
+	if (!dp->dp_display.is_edp)
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+
 	if (dp_catalog_link_is_connected(dp->catalog)) {
 		/*
 		 * set sink to normal operation mode -- D0
@@ -1655,6 +1669,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
 		return;
 	}
 
+	if (dp->is_edp)
+		dp_hpd_plug_handle(dp_display, 0);
+
 	mutex_lock(&dp_display->event_mutex);
 
 	/* stop sentinel checking */
@@ -1719,6 +1736,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
+	if (dp->is_edp)
+		dp_hpd_unplug_handle(dp_display, 0);
+
 	mutex_lock(&dp_display->event_mutex);
 
 	/* stop sentinel checking */
-- 
2.7.4

[PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti]

The panel-edp enables the eDP panel power during probe, get_modes
and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.

So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v8:
  - add comment explaining the interrupt status return

Changes in v7:
  - reordered the patch in the series
  - modified the return statement for isr
  - connector check modified to just check for eDP

 drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
 drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..3a298e9 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
 
 	u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
 
-	/* enable HPD plug and unplug interrupts */
-	dp_catalog_hpd_config_intr(dp_catalog,
-		DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
 	/* Configure REFTIMER and enable it */
 	reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
 	dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
@@ -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
 {
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 				struct dp_catalog_private, dp_catalog);
-	int isr = 0;
+	int isr, mask;
 
 	isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
 	dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
 				 (isr & DP_DP_HPD_INT_MASK));
+	mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
 
-	return isr;
+	/*
+	 * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
+	 * irrespective of their masked status. The HW interrupt will not be
+	 * generated if an interrupt is masked. However, the interrupt status
+	 * bit in the register will still be set. The eDP controller driver
+	 * masks the plug and unplug interrupts unlike DP controller which
+	 * unmasks all the interrupts. So, do not report the status of the
+	 * masked interrupts.
+	 */
+	return isr & (mask | ~DP_DP_HPD_INT_MASK);
 }
 
 int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3d03c77..874258d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	dp_display_handle_plugged_change(&dp->dp_display, false);
 
 	/* enable HDP plug interrupt to prepare for next plugin */
-	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+	if (!dp->dp_display.is_edp)
+		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
 
 	DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
 			dp->dp_display.connector_type, state);
@@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
 	dp_display_host_init(dp);
 	dp_catalog_ctrl_hpd_config(dp->catalog);
 
+	/* Enable plug and unplug interrupts only for external DisplayPort */
+	if (!dp->dp_display.is_edp)
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+
 	/* Enable interrupt first time
 	 * we are leaving dp clocks on during disconnect
 	 * and never disable interrupt
@@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
 	dp_catalog_ctrl_hpd_config(dp->catalog);
 
 
+	if (!dp->dp_display.is_edp)
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+
 	if (dp_catalog_link_is_connected(dp->catalog)) {
 		/*
 		 * set sink to normal operation mode -- D0
@@ -1655,6 +1669,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
 		return;
 	}
 
+	if (dp->is_edp)
+		dp_hpd_plug_handle(dp_display, 0);
+
 	mutex_lock(&dp_display->event_mutex);
 
 	/* stop sentinel checking */
@@ -1719,6 +1736,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
+	if (dp->is_edp)
+		dp_hpd_unplug_handle(dp_display, 0);
+
 	mutex_lock(&dp_display->event_mutex);
 
 	/* stop sentinel checking */
-- 
2.7.4

[PATCH v8 3/4] drm/msm/dp: wait for hpd high before aux transaction

[Sankeerth Billakanti]

The source device should ensure the sink is ready before proceeding to
read the sink capability or perform any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
it performs any aux transactions.

The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.

Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
These changes may be handled in is_hpd_asserted when
https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/
is accepted upstream.

Changes in v8:
  - correct the indentation

Changes in v7:
  - add a comment to say why the wait is done for eDP
  - correct the commit text

Changes in v6:
  - Wait for hpd high only for eDP
  - Split into smaller patches

 drivers/gpu/drm/msm/dp/dp_aux.c     | 21 ++++++++++++++++++++-
 drivers/gpu/drm/msm/dp/dp_aux.h     |  3 ++-
 drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +++++++++++++
 drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c |  2 +-
 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..d030a93 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -34,6 +34,7 @@ struct dp_aux_private {
 	bool no_send_addr;
 	bool no_send_stop;
 	bool initted;
+	bool is_edp;
 	u32 offset;
 	u32 segment;
 
@@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		goto exit;
 	}
 
+	/*
+	 * For eDP it's important to give a reasonably long wait here for HPD
+	 * to be asserted. This is because the panel driver may have _just_
+	 * turned on the panel and then tried to do an AUX transfer. The panel
+	 * driver has no way of knowing when the panel is ready, so it's up
+	 * to us to wait. For DP we never get into this situation so let's
+	 * avoid ever doing the extra long wait for DP.
+	 */
+	if (aux->is_edp) {
+		ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+		if (ret) {
+			DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+			goto exit;
+		}
+	}
+
 	dp_aux_update_offset_and_segment(aux, msg);
 	dp_aux_transfer_helper(aux, msg, true);
 
@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
 	drm_dp_aux_unregister(dp_aux);
 }
 
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+			      bool is_edp)
 {
 	struct dp_aux_private *aux;
 
@@ -506,6 +524,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
 
 	init_completion(&aux->comp);
 	aux->cmd_busy = false;
+	aux->is_edp = is_edp;
 	mutex_init(&aux->mutex);
 
 	aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..5a50c08 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
 void dp_aux_deinit(struct drm_dp_aux *dp_aux);
 void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
 
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+			      bool is_edp);
 void dp_aux_put(struct drm_dp_aux *aux);
 
 #endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 3a298e9..67a650a 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
 	phy_calibrate(phy);
 }
 
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+	u32 state;
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+				struct dp_catalog_private, dp_catalog);
+
+	/* poll for hpd connected status every 2ms and timeout after 500ms */
+	return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+				REG_DP_DP_HPD_INT_STATUS,
+				state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
+				2000, 500000);
+}
+
 static void dump_regs(void __iomem *base, int len)
 {
 	int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
 u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 874258d..86948d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -805,7 +805,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
-	dp->aux = dp_aux_get(dev, dp->catalog);
+	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
 		DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
-- 
2.7.4

[PATCH v8 3/4] drm/msm/dp: wait for hpd high before aux transaction

[Sankeerth Billakanti]

The source device should ensure the sink is ready before proceeding to
read the sink capability or perform any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
it performs any aux transactions.

The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.

Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
These changes may be handled in is_hpd_asserted when
https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/
is accepted upstream.

Changes in v8:
  - correct the indentation

Changes in v7:
  - add a comment to say why the wait is done for eDP
  - correct the commit text

Changes in v6:
  - Wait for hpd high only for eDP
  - Split into smaller patches

 drivers/gpu/drm/msm/dp/dp_aux.c     | 21 ++++++++++++++++++++-
 drivers/gpu/drm/msm/dp/dp_aux.h     |  3 ++-
 drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +++++++++++++
 drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c |  2 +-
 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..d030a93 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -34,6 +34,7 @@ struct dp_aux_private {
 	bool no_send_addr;
 	bool no_send_stop;
 	bool initted;
+	bool is_edp;
 	u32 offset;
 	u32 segment;
 
@@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		goto exit;
 	}
 
+	/*
+	 * For eDP it's important to give a reasonably long wait here for HPD
+	 * to be asserted. This is because the panel driver may have _just_
+	 * turned on the panel and then tried to do an AUX transfer. The panel
+	 * driver has no way of knowing when the panel is ready, so it's up
+	 * to us to wait. For DP we never get into this situation so let's
+	 * avoid ever doing the extra long wait for DP.
+	 */
+	if (aux->is_edp) {
+		ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+		if (ret) {
+			DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+			goto exit;
+		}
+	}
+
 	dp_aux_update_offset_and_segment(aux, msg);
 	dp_aux_transfer_helper(aux, msg, true);
 
@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
 	drm_dp_aux_unregister(dp_aux);
 }
 
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+			      bool is_edp)
 {
 	struct dp_aux_private *aux;
 
@@ -506,6 +524,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
 
 	init_completion(&aux->comp);
 	aux->cmd_busy = false;
+	aux->is_edp = is_edp;
 	mutex_init(&aux->mutex);
 
 	aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..5a50c08 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
 void dp_aux_deinit(struct drm_dp_aux *dp_aux);
 void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
 
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+			      bool is_edp);
 void dp_aux_put(struct drm_dp_aux *aux);
 
 #endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 3a298e9..67a650a 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
 	phy_calibrate(phy);
 }
 
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+	u32 state;
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+				struct dp_catalog_private, dp_catalog);
+
+	/* poll for hpd connected status every 2ms and timeout after 500ms */
+	return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+				REG_DP_DP_HPD_INT_STATUS,
+				state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
+				2000, 500000);
+}
+
 static void dump_regs(void __iomem *base, int len)
 {
 	int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
 u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 874258d..86948d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -805,7 +805,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
-	dp->aux = dp_aux_get(dev, dp->catalog);
+	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
 		DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
-- 
2.7.4

[PATCH v8 4/4] drm/msm/dp: Support the eDP modes given by panel

[Sankeerth Billakanti]

The eDP controller does not have a reliable way keep panel
powered on to read the sink capabilities. So, the controller
driver cannot validate if a mode can be supported by the
source. We will rely on the panel driver to populate only
the supported modes for now.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v8:
  - add the drm/msm/dp tag in the commit title

 drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 86948d6..7d71bdc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
+	/*
+	 * The eDP controller currently does not have a reliable way of
+	 * enabling panel power to read sink capabilities. So, we rely
+	 * on the panel driver to populate only supported modes for now.
+	 */
+	if (dp->is_edp)
+		return MODE_OK;
+
 	if ((dp->max_pclk_khz <= 0) ||
 			(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
 			(mode->clock > dp->max_pclk_khz))
-- 
2.7.4

[PATCH v8 4/4] drm/msm/dp: Support the eDP modes given by panel

[Sankeerth Billakanti]

The eDP controller does not have a reliable way keep panel
powered on to read the sink capabilities. So, the controller
driver cannot validate if a mode can be supported by the
source. We will rely on the panel driver to populate only
the supported modes for now.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v8:
  - add the drm/msm/dp tag in the commit title

 drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 86948d6..7d71bdc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
+	/*
+	 * The eDP controller currently does not have a reliable way of
+	 * enabling panel power to read sink capabilities. So, we rely
+	 * on the panel driver to populate only supported modes for now.
+	 */
+	if (dp->is_edp)
+		return MODE_OK;
+
 	if ((dp->max_pclk_khz <= 0) ||
 			(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
 			(mode->clock > dp->max_pclk_khz))
-- 
2.7.4

Re: [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> @@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>         }
>  }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> +       int rc;
> +       struct dp_display_private *dp_priv;
> +       struct device_node *aux_bus;
> +       struct device *dev;
> +
> +       dp_priv = container_of(dp, struct dp_display_private, dp_display);
> +       dev = &dp_priv->pdev->dev;
> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> +       if (aux_bus && dp->is_edp) {
> +               dp_display_host_init(dp_priv);
> +               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> +               dp_display_host_phy_init(dp_priv);
> +               enable_irq(dp_priv->irq);
> +
> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);

I think a comment was requested above that line saying something like:

/*
 * The code below assumes that the panel will finish probing
 * by the time devm_of_dp_aux_populate_ep_devices() returns.
 * This isn't a great assumption since it will fail if the
 * panel driver is probed asynchronously but is the best we
 * can do without a bigger driver reorganization.
 */


> +               of_node_put(aux_bus);
> +               if (rc)
> +                       goto edp_error;
> +       } else if (dp->is_edp) {
> +               DRM_ERROR("eDP aux_bus not found\n");
> +               rc = -ENODEV;
> +               goto error;

This goto doesn't add much. Just leave the above like it was in v7.
return -ENODEV w/ no goto.


> +       }
> +
> +       /*
> +        * External bridges are mandatory for eDP interfaces: one has to
> +        * provide at least an eDP panel (which gets wrapped into panel-bridge).
> +        *
> +        * For DisplayPort interfaces external bridges are optional, so
> +        * silently ignore an error if one is not present (-ENODEV).
> +        */
> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
> +       if (rc && dp->is_edp) {
> +               DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
> +               goto edp_error;
> +       } else if (rc && rc != -ENODEV) {
> +               DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
> +               goto error;
> +       }

The above wouldn't be my favorite way of doing this. Instead, I would
have written:

  if (rc) {
    DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
    goto err;
  }
  ...

err:
  if (dp->is_edp) {
    disable_irq(...);
    dp_display_host_phy_exit(...);
    dp_display_host_deinit(...);
  }
  return rc;

> +
> +       dp->next_bridge = dp_priv->parser->next_bridge;
> +
> +       return 0;
> +
> +edp_error:
> +       disable_irq(dp_priv->irq);
> +       dp_display_host_phy_exit(dp_priv);
> +       dp_display_host_deinit(dp_priv);
> +error:
> +       return rc;
> +}
> +
>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>                         struct drm_encoder *encoder)
>  {

With the above fixes, I'd be happy enough for my Reviewed-by tag with
the expectation that continued work will happen to continue cleaning
this up.


-Doug

Re: [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> @@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>         }
>  }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> +       int rc;
> +       struct dp_display_private *dp_priv;
> +       struct device_node *aux_bus;
> +       struct device *dev;
> +
> +       dp_priv = container_of(dp, struct dp_display_private, dp_display);
> +       dev = &dp_priv->pdev->dev;
> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> +       if (aux_bus && dp->is_edp) {
> +               dp_display_host_init(dp_priv);
> +               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> +               dp_display_host_phy_init(dp_priv);
> +               enable_irq(dp_priv->irq);
> +
> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);

I think a comment was requested above that line saying something like:

/*
 * The code below assumes that the panel will finish probing
 * by the time devm_of_dp_aux_populate_ep_devices() returns.
 * This isn't a great assumption since it will fail if the
 * panel driver is probed asynchronously but is the best we
 * can do without a bigger driver reorganization.
 */


> +               of_node_put(aux_bus);
> +               if (rc)
> +                       goto edp_error;
> +       } else if (dp->is_edp) {
> +               DRM_ERROR("eDP aux_bus not found\n");
> +               rc = -ENODEV;
> +               goto error;

This goto doesn't add much. Just leave the above like it was in v7.
return -ENODEV w/ no goto.


> +       }
> +
> +       /*
> +        * External bridges are mandatory for eDP interfaces: one has to
> +        * provide at least an eDP panel (which gets wrapped into panel-bridge).
> +        *
> +        * For DisplayPort interfaces external bridges are optional, so
> +        * silently ignore an error if one is not present (-ENODEV).
> +        */
> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
> +       if (rc && dp->is_edp) {
> +               DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
> +               goto edp_error;
> +       } else if (rc && rc != -ENODEV) {
> +               DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
> +               goto error;
> +       }

The above wouldn't be my favorite way of doing this. Instead, I would
have written:

  if (rc) {
    DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
    goto err;
  }
  ...

err:
  if (dp->is_edp) {
    disable_irq(...);
    dp_display_host_phy_exit(...);
    dp_display_host_deinit(...);
  }
  return rc;

> +
> +       dp->next_bridge = dp_priv->parser->next_bridge;
> +
> +       return 0;
> +
> +edp_error:
> +       disable_irq(dp_priv->irq);
> +       dp_display_host_phy_exit(dp_priv);
> +       dp_display_host_deinit(dp_priv);
> +error:
> +       return rc;
> +}
> +
>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>                         struct drm_encoder *encoder)
>  {

With the above fixes, I'd be happy enough for my Reviewed-by tag with
the expectation that continued work will happen to continue cleaning
this up.


-Doug

Re: [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v8:
>   - add comment explaining the interrupt status return
>
> Changes in v7:
>   - reordered the patch in the series
>   - modified the return statement for isr
>   - connector check modified to just check for eDP
>
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
>  drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..3a298e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>
>         u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> -       /* enable HPD plug and unplug interrupts */
> -       dp_catalog_hpd_config_intr(dp_catalog,
> -               DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
> -
>         /* Configure REFTIMER and enable it */
>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> @@ -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
>  {
>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>                                 struct dp_catalog_private, dp_catalog);
> -       int isr = 0;
> +       int isr, mask;
>
>         isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>                                  (isr & DP_DP_HPD_INT_MASK));
> +       mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>
> -       return isr;
> +       /*
> +        * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
> +        * irrespective of their masked status. The HW interrupt will not be
> +        * generated if an interrupt is masked. However, the interrupt status
> +        * bit in the register will still be set. The eDP controller driver
> +        * masks the plug and unplug interrupts unlike DP controller which
> +        * unmasks all the interrupts. So, do not report the status of the
> +        * masked interrupts.
> +        */
> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);

What's still missing in your comments is why you aren't just doing
"return isr & mask;". In other words, why is the API for HPD bits
different from the API for non-HPD bits? What code out there wants to
know about non-HPD interrupts even if they are masked?

Actually, thinking about this more, my preference would be this:

a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()

b) Create a new function called dp_catalog_hpd_get_intr_status() whose
implementation is:

  return dp_catalog_hpd_get_intr_status_raw() & mask;

Then any callers who care about the raw status can be changed to call
dp_catalog_hpd_get_intr_status_raw(). If no callers need
dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
create this function.

If you make that change then all of a sudden the API isn't weird/wonky
and you can just get rid of the comment I asked you to add.


-Doug

Re: [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v8:
>   - add comment explaining the interrupt status return
>
> Changes in v7:
>   - reordered the patch in the series
>   - modified the return statement for isr
>   - connector check modified to just check for eDP
>
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
>  drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..3a298e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>
>         u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> -       /* enable HPD plug and unplug interrupts */
> -       dp_catalog_hpd_config_intr(dp_catalog,
> -               DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
> -
>         /* Configure REFTIMER and enable it */
>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> @@ -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
>  {
>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>                                 struct dp_catalog_private, dp_catalog);
> -       int isr = 0;
> +       int isr, mask;
>
>         isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>                                  (isr & DP_DP_HPD_INT_MASK));
> +       mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>
> -       return isr;
> +       /*
> +        * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
> +        * irrespective of their masked status. The HW interrupt will not be
> +        * generated if an interrupt is masked. However, the interrupt status
> +        * bit in the register will still be set. The eDP controller driver
> +        * masks the plug and unplug interrupts unlike DP controller which
> +        * unmasks all the interrupts. So, do not report the status of the
> +        * masked interrupts.
> +        */
> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);

What's still missing in your comments is why you aren't just doing
"return isr & mask;". In other words, why is the API for HPD bits
different from the API for non-HPD bits? What code out there wants to
know about non-HPD interrupts even if they are masked?

Actually, thinking about this more, my preference would be this:

a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()

b) Create a new function called dp_catalog_hpd_get_intr_status() whose
implementation is:

  return dp_catalog_hpd_get_intr_status_raw() & mask;

Then any callers who care about the raw status can be changed to call
dp_catalog_hpd_get_intr_status_raw(). If no callers need
dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
create this function.

If you make that change then all of a sudden the API isn't weird/wonky
and you can just get rid of the comment I asked you to add.


-Doug

RE: [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti (QUIC)]

Hi Doug,

>On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
><quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp
>*dp_display, struct drm_minor *minor)
>>         }
>>  }
>>
>> +static int dp_display_get_next_bridge(struct msm_dp *dp) {
>> +       int rc;
>> +       struct dp_display_private *dp_priv;
>> +       struct device_node *aux_bus;
>> +       struct device *dev;
>> +
>> +       dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> +       dev = &dp_priv->pdev->dev;
>> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> +
>> +       if (aux_bus && dp->is_edp) {
>> +               dp_display_host_init(dp_priv);
>> +               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> +               dp_display_host_phy_init(dp_priv);
>> +               enable_irq(dp_priv->irq);
>> +
>> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>
>I think a comment was requested above that line saying something like:
>
>/*
> * The code below assumes that the panel will finish probing
> * by the time devm_of_dp_aux_populate_ep_devices() returns.
> * This isn't a great assumption since it will fail if the
> * panel driver is probed asynchronously but is the best we
> * can do without a bigger driver reorganization.
> */
>
>

Will add the comment

>> +               of_node_put(aux_bus);
>> +               if (rc)
>> +                       goto edp_error;
>> +       } else if (dp->is_edp) {
>> +               DRM_ERROR("eDP aux_bus not found\n");
>> +               rc = -ENODEV;
>> +               goto error;
>
>This goto doesn't add much. Just leave the above like it was in v7.
>return -ENODEV w/ no goto.
>
>

Okay

>> +       }
>> +
>> +       /*
>> +        * External bridges are mandatory for eDP interfaces: one has to
>> +        * provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> +        *
>> +        * For DisplayPort interfaces external bridges are optional, so
>> +        * silently ignore an error if one is not present (-ENODEV).
>> +        */
>> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
>> +       if (rc && dp->is_edp) {
>> +               DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
>> +               goto edp_error;
>> +       } else if (rc && rc != -ENODEV) {
>> +               DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
>> +               goto error;
>> +       }
>
>The above wouldn't be my favorite way of doing this. Instead, I would have
>written:
>
>  if (rc) {
>    DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
>    goto err;
>  }
>  ...
>
>err:
>  if (dp->is_edp) {
>    disable_irq(...);
>    dp_display_host_phy_exit(...);
>    dp_display_host_deinit(...);
>  }
>  return rc;
>

If rc is ENODEV for DP, then we need to return 0. Shall I add like below ?

err:
  if (dp->is_edp) {
    disable_irq(...);
    dp_display_host_phy_exit(...);
    dp_display_host_deinit(...);
  } else
        If (rc == -ENODEV)
            rc = 0;
  return rc;

>> +
>> +       dp->next_bridge = dp_priv->parser->next_bridge;
>> +
>> +       return 0;
>> +
>> +edp_error:
>> +       disable_irq(dp_priv->irq);
>> +       dp_display_host_phy_exit(dp_priv);
>> +       dp_display_host_deinit(dp_priv);
>> +error:
>> +       return rc;
>> +}
>> +
>>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>>                         struct drm_encoder *encoder)  {
>
>With the above fixes, I'd be happy enough for my Reviewed-by tag with the
>expectation that continued work will happen to continue cleaning this up.
>
>
>-Doug

RE: [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti (QUIC)]

Hi Doug,

>On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
><quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp
>*dp_display, struct drm_minor *minor)
>>         }
>>  }
>>
>> +static int dp_display_get_next_bridge(struct msm_dp *dp) {
>> +       int rc;
>> +       struct dp_display_private *dp_priv;
>> +       struct device_node *aux_bus;
>> +       struct device *dev;
>> +
>> +       dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> +       dev = &dp_priv->pdev->dev;
>> +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> +
>> +       if (aux_bus && dp->is_edp) {
>> +               dp_display_host_init(dp_priv);
>> +               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> +               dp_display_host_phy_init(dp_priv);
>> +               enable_irq(dp_priv->irq);
>> +
>> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>
>I think a comment was requested above that line saying something like:
>
>/*
> * The code below assumes that the panel will finish probing
> * by the time devm_of_dp_aux_populate_ep_devices() returns.
> * This isn't a great assumption since it will fail if the
> * panel driver is probed asynchronously but is the best we
> * can do without a bigger driver reorganization.
> */
>
>

Will add the comment

>> +               of_node_put(aux_bus);
>> +               if (rc)
>> +                       goto edp_error;
>> +       } else if (dp->is_edp) {
>> +               DRM_ERROR("eDP aux_bus not found\n");
>> +               rc = -ENODEV;
>> +               goto error;
>
>This goto doesn't add much. Just leave the above like it was in v7.
>return -ENODEV w/ no goto.
>
>

Okay

>> +       }
>> +
>> +       /*
>> +        * External bridges are mandatory for eDP interfaces: one has to
>> +        * provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> +        *
>> +        * For DisplayPort interfaces external bridges are optional, so
>> +        * silently ignore an error if one is not present (-ENODEV).
>> +        */
>> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
>> +       if (rc && dp->is_edp) {
>> +               DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
>> +               goto edp_error;
>> +       } else if (rc && rc != -ENODEV) {
>> +               DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
>> +               goto error;
>> +       }
>
>The above wouldn't be my favorite way of doing this. Instead, I would have
>written:
>
>  if (rc) {
>    DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
>    goto err;
>  }
>  ...
>
>err:
>  if (dp->is_edp) {
>    disable_irq(...);
>    dp_display_host_phy_exit(...);
>    dp_display_host_deinit(...);
>  }
>  return rc;
>

If rc is ENODEV for DP, then we need to return 0. Shall I add like below ?

err:
  if (dp->is_edp) {
    disable_irq(...);
    dp_display_host_phy_exit(...);
    dp_display_host_deinit(...);
  } else
        If (rc == -ENODEV)
            rc = 0;
  return rc;

>> +
>> +       dp->next_bridge = dp_priv->parser->next_bridge;
>> +
>> +       return 0;
>> +
>> +edp_error:
>> +       disable_irq(dp_priv->irq);
>> +       dp_display_host_phy_exit(dp_priv);
>> +       dp_display_host_deinit(dp_priv);
>> +error:
>> +       return rc;
>> +}
>> +
>>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>>                         struct drm_encoder *encoder)  {
>
>With the above fixes, I'd be happy enough for my Reviewed-by tag with the
>expectation that continued work will happen to continue cleaning this up.
>
>
>-Doug

Re: [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 9:00 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> >> +       }
> >> +
> >> +       /*
> >> +        * External bridges are mandatory for eDP interfaces: one has to
> >> +        * provide at least an eDP panel (which gets wrapped into panel-
> >bridge).
> >> +        *
> >> +        * For DisplayPort interfaces external bridges are optional, so
> >> +        * silently ignore an error if one is not present (-ENODEV).
> >> +        */
> >> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
> >> +       if (rc && dp->is_edp) {
> >> +               DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
> >> +               goto edp_error;
> >> +       } else if (rc && rc != -ENODEV) {
> >> +               DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
> >> +               goto error;
> >> +       }
> >
> >The above wouldn't be my favorite way of doing this. Instead, I would have
> >written:
> >
> >  if (rc) {
> >    DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
> >    goto err;
> >  }
> >  ...
> >
> >err:
> >  if (dp->is_edp) {
> >    disable_irq(...);
> >    dp_display_host_phy_exit(...);
> >    dp_display_host_deinit(...);
> >  }
> >  return rc;
> >
>
> If rc is ENODEV for DP, then we need to return 0. Shall I add like below ?
>
> err:
>   if (dp->is_edp) {
>     disable_irq(...);
>     dp_display_host_phy_exit(...);
>     dp_display_host_deinit(...);
>   } else
>         If (rc == -ENODEV)
>             rc = 0;
>   return rc;

I wouldn't. Then you're essentially going to "err" for a case that you
don't consider an error. I would instead have just handled it right
away.

rc = dp_parser_find_next_bridge(dp_priv->parser);
if (!dp->is_edp && rc == -ENODEV)
  return 0;

This also is better IMO because it means you aren't assuming that
`dp_priv->parser->next_bridge` is "valid" (or at least NULL) after
dp_parser_find_next_bridge() returned an error.

-Doug

Re: [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 9:00 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> >> +       }
> >> +
> >> +       /*
> >> +        * External bridges are mandatory for eDP interfaces: one has to
> >> +        * provide at least an eDP panel (which gets wrapped into panel-
> >bridge).
> >> +        *
> >> +        * For DisplayPort interfaces external bridges are optional, so
> >> +        * silently ignore an error if one is not present (-ENODEV).
> >> +        */
> >> +       rc = dp_parser_find_next_bridge(dp_priv->parser);
> >> +       if (rc && dp->is_edp) {
> >> +               DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
> >> +               goto edp_error;
> >> +       } else if (rc && rc != -ENODEV) {
> >> +               DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
> >> +               goto error;
> >> +       }
> >
> >The above wouldn't be my favorite way of doing this. Instead, I would have
> >written:
> >
> >  if (rc) {
> >    DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
> >    goto err;
> >  }
> >  ...
> >
> >err:
> >  if (dp->is_edp) {
> >    disable_irq(...);
> >    dp_display_host_phy_exit(...);
> >    dp_display_host_deinit(...);
> >  }
> >  return rc;
> >
>
> If rc is ENODEV for DP, then we need to return 0. Shall I add like below ?
>
> err:
>   if (dp->is_edp) {
>     disable_irq(...);
>     dp_display_host_phy_exit(...);
>     dp_display_host_deinit(...);
>   } else
>         If (rc == -ENODEV)
>             rc = 0;
>   return rc;

I wouldn't. Then you're essentially going to "err" for a case that you
don't consider an error. I would instead have just handled it right
away.

rc = dp_parser_find_next_bridge(dp_priv->parser);
if (!dp->is_edp && rc == -ENODEV)
  return 0;

This also is better IMO because it means you aren't assuming that
`dp_priv->parser->next_bridge` is "valid" (or at least NULL) after
dp_parser_find_next_bridge() returned an error.

-Doug

RE: [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti (QUIC)]

Hi Doug,

>On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
><quic_sbillaka@quicinc.com> wrote:
>>
>> The panel-edp enables the eDP panel power during probe, get_modes and
>> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can
>> be assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>> Changes in v8:
>>   - add comment explaining the interrupt status return
>>
>> Changes in v7:
>>   - reordered the patch in the series
>>   - modified the return statement for isr
>>   - connector check modified to just check for eDP
>>
>>  drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
>> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>>  2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index fac815f..3a298e9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
>> *dp_catalog)
>>
>>         u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>>
>> -       /* enable HPD plug and unplug interrupts */
>> -       dp_catalog_hpd_config_intr(dp_catalog,
>> -               DP_DP_HPD_PLUG_INT_MASK |
>DP_DP_HPD_UNPLUG_INT_MASK, true);
>> -
>>         /* Configure REFTIMER and enable it */
>>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@
>> -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct
>> dp_catalog *dp_catalog)  {
>>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>>                                 struct dp_catalog_private, dp_catalog);
>> -       int isr = 0;
>> +       int isr, mask;
>>
>>         isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>>                                  (isr & DP_DP_HPD_INT_MASK));
>> +       mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>>
>> -       return isr;
>> +       /*
>> +        * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
>> +        * irrespective of their masked status. The HW interrupt will not be
>> +        * generated if an interrupt is masked. However, the interrupt status
>> +        * bit in the register will still be set. The eDP controller driver
>> +        * masks the plug and unplug interrupts unlike DP controller which
>> +        * unmasks all the interrupts. So, do not report the status of the
>> +        * masked interrupts.
>> +        */
>> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);
>
>What's still missing in your comments is why you aren't just doing "return isr &
>mask;". In other words, why is the API for HPD bits different from the API for
>non-HPD bits? What code out there wants to know about non-HPD interrupts
>even if they are masked?

The mask register bitfields are different from the INT_STATUS register.
The INT_STATUS register has additional bits [31:28] which indicates the HPD state machine
status and [3:0] indicates the actual generated/set interrupt. The fields [3:0] are similar to
the mask and ack interrupts.

#define DP_DP_HPD_STATE_STATUS_CONNECTED        (0x40000000)
#define DP_DP_HPD_STATE_STATUS_PENDING          (0x20000000)
#define DP_DP_HPD_STATE_STATUS_DISCONNECTED     (0x00000000)
#define DP_DP_HPD_STATE_STATUS_MASK             (0xE0000000)

So, earlier I returned isr & (mask | DP_DP_HPD_STATE_STATUS_MASK);

Actually, there is no reason to do this. We can just do the below:
return isr & mask;

>
>Actually, thinking about this more, my preference would be this:
>
>a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
>
>b) Create a new function called dp_catalog_hpd_get_intr_status() whose
>implementation is:
>
>  return dp_catalog_hpd_get_intr_status_raw() & mask;
>
>Then any callers who care about the raw status can be changed to call
>dp_catalog_hpd_get_intr_status_raw(). If no callers need
>dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
>create this function.
>

There is no caller for raw status. All interrupts are unmasked for DP.
While handling the aux interrupts generated during the transfer call from panel probe,
we read that the HPD interrupt status [3:0] bit0 is set and proceed to handle connect like DP.
We experimented to find out that the connect interrupt is not generated but just the status
bit is set.

As the interrupts are generated over a single MDSS line, the controller driver has to read all the
interrupt status registers and handle all the set interrupt bits. So, while handling aux transfer
interrupts, we were proceeding to handle connect interrupt also as a consequence.

>If you make that change then all of a sudden the API isn't weird/wonky and
>you can just get rid of the comment I asked you to add.
>
>
>-Doug

RE: [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti (QUIC)]

Hi Doug,

>On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
><quic_sbillaka@quicinc.com> wrote:
>>
>> The panel-edp enables the eDP panel power during probe, get_modes and
>> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can
>> be assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>> Changes in v8:
>>   - add comment explaining the interrupt status return
>>
>> Changes in v7:
>>   - reordered the patch in the series
>>   - modified the return statement for isr
>>   - connector check modified to just check for eDP
>>
>>  drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
>> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>>  2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index fac815f..3a298e9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
>> *dp_catalog)
>>
>>         u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>>
>> -       /* enable HPD plug and unplug interrupts */
>> -       dp_catalog_hpd_config_intr(dp_catalog,
>> -               DP_DP_HPD_PLUG_INT_MASK |
>DP_DP_HPD_UNPLUG_INT_MASK, true);
>> -
>>         /* Configure REFTIMER and enable it */
>>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@
>> -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct
>> dp_catalog *dp_catalog)  {
>>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>>                                 struct dp_catalog_private, dp_catalog);
>> -       int isr = 0;
>> +       int isr, mask;
>>
>>         isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>>                                  (isr & DP_DP_HPD_INT_MASK));
>> +       mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>>
>> -       return isr;
>> +       /*
>> +        * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
>> +        * irrespective of their masked status. The HW interrupt will not be
>> +        * generated if an interrupt is masked. However, the interrupt status
>> +        * bit in the register will still be set. The eDP controller driver
>> +        * masks the plug and unplug interrupts unlike DP controller which
>> +        * unmasks all the interrupts. So, do not report the status of the
>> +        * masked interrupts.
>> +        */
>> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);
>
>What's still missing in your comments is why you aren't just doing "return isr &
>mask;". In other words, why is the API for HPD bits different from the API for
>non-HPD bits? What code out there wants to know about non-HPD interrupts
>even if they are masked?

The mask register bitfields are different from the INT_STATUS register.
The INT_STATUS register has additional bits [31:28] which indicates the HPD state machine
status and [3:0] indicates the actual generated/set interrupt. The fields [3:0] are similar to
the mask and ack interrupts.

#define DP_DP_HPD_STATE_STATUS_CONNECTED        (0x40000000)
#define DP_DP_HPD_STATE_STATUS_PENDING          (0x20000000)
#define DP_DP_HPD_STATE_STATUS_DISCONNECTED     (0x00000000)
#define DP_DP_HPD_STATE_STATUS_MASK             (0xE0000000)

So, earlier I returned isr & (mask | DP_DP_HPD_STATE_STATUS_MASK);

Actually, there is no reason to do this. We can just do the below:
return isr & mask;

>
>Actually, thinking about this more, my preference would be this:
>
>a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
>
>b) Create a new function called dp_catalog_hpd_get_intr_status() whose
>implementation is:
>
>  return dp_catalog_hpd_get_intr_status_raw() & mask;
>
>Then any callers who care about the raw status can be changed to call
>dp_catalog_hpd_get_intr_status_raw(). If no callers need
>dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
>create this function.
>

There is no caller for raw status. All interrupts are unmasked for DP.
While handling the aux interrupts generated during the transfer call from panel probe,
we read that the HPD interrupt status [3:0] bit0 is set and proceed to handle connect like DP.
We experimented to find out that the connect interrupt is not generated but just the status
bit is set.

As the interrupts are generated over a single MDSS line, the controller driver has to read all the
interrupt status registers and handle all the set interrupt bits. So, while handling aux transfer
interrupts, we were proceeding to handle connect interrupt also as a consequence.

>If you make that change then all of a sudden the API isn't weird/wonky and
>you can just get rid of the comment I asked you to add.
>
>
>-Doug

Re: [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 9:39 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Doug,
>
> >On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
> ><quic_sbillaka@quicinc.com> wrote:
> >>
> >> The panel-edp enables the eDP panel power during probe, get_modes and
> >> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> >> controller are directly dependent on panel power. As eDP display can
> >> be assumed as always connected, the controller driver can skip the eDP
> >> connect and disconnect interrupts. Any disruption in the link status
> >> will be indicated via the IRQ_HPD interrupts.
> >>
> >> So, the eDP controller driver can just enable the IRQ_HPD and replug
> >> interrupts. The DP controller driver still needs to enable all the
> >> interrupts.
> >>
> >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >> ---
> >> Changes in v8:
> >>   - add comment explaining the interrupt status return
> >>
> >> Changes in v7:
> >>   - reordered the patch in the series
> >>   - modified the return statement for isr
> >>   - connector check modified to just check for eDP
> >>
> >>  drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
> >> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> >>  2 files changed, 33 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index fac815f..3a298e9 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> >> *dp_catalog)
> >>
> >>         u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
> >>
> >> -       /* enable HPD plug and unplug interrupts */
> >> -       dp_catalog_hpd_config_intr(dp_catalog,
> >> -               DP_DP_HPD_PLUG_INT_MASK |
> >DP_DP_HPD_UNPLUG_INT_MASK, true);
> >> -
> >>         /* Configure REFTIMER and enable it */
> >>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> >>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@
> >> -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct
> >> dp_catalog *dp_catalog)  {
> >>         struct dp_catalog_private *catalog = container_of(dp_catalog,
> >>                                 struct dp_catalog_private, dp_catalog);
> >> -       int isr = 0;
> >> +       int isr, mask;
> >>
> >>         isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> >>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> >>                                  (isr & DP_DP_HPD_INT_MASK));
> >> +       mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> >>
> >> -       return isr;
> >> +       /*
> >> +        * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
> >> +        * irrespective of their masked status. The HW interrupt will not be
> >> +        * generated if an interrupt is masked. However, the interrupt status
> >> +        * bit in the register will still be set. The eDP controller driver
> >> +        * masks the plug and unplug interrupts unlike DP controller which
> >> +        * unmasks all the interrupts. So, do not report the status of the
> >> +        * masked interrupts.
> >> +        */
> >> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);
> >
> >What's still missing in your comments is why you aren't just doing "return isr &
> >mask;". In other words, why is the API for HPD bits different from the API for
> >non-HPD bits? What code out there wants to know about non-HPD interrupts
> >even if they are masked?
>
> The mask register bitfields are different from the INT_STATUS register.
> The INT_STATUS register has additional bits [31:28] which indicates the HPD state machine
> status and [3:0] indicates the actual generated/set interrupt. The fields [3:0] are similar to
> the mask and ack interrupts.
>
> #define DP_DP_HPD_STATE_STATUS_CONNECTED        (0x40000000)
> #define DP_DP_HPD_STATE_STATUS_PENDING          (0x20000000)
> #define DP_DP_HPD_STATE_STATUS_DISCONNECTED     (0x00000000)
> #define DP_DP_HPD_STATE_STATUS_MASK             (0xE0000000)
>
> So, earlier I returned isr & (mask | DP_DP_HPD_STATE_STATUS_MASK);
>
> Actually, there is no reason to do this. We can just do the below:
> return isr & mask;
>
> >
> >Actually, thinking about this more, my preference would be this:
> >
> >a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
> >
> >b) Create a new function called dp_catalog_hpd_get_intr_status() whose
> >implementation is:
> >
> >  return dp_catalog_hpd_get_intr_status_raw() & mask;
> >
> >Then any callers who care about the raw status can be changed to call
> >dp_catalog_hpd_get_intr_status_raw(). If no callers need
> >dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
> >create this function.
> >
>
> There is no caller for raw status. All interrupts are unmasked for DP.
> While handling the aux interrupts generated during the transfer call from panel probe,
> we read that the HPD interrupt status [3:0] bit0 is set and proceed to handle connect like DP.
> We experimented to find out that the connect interrupt is not generated but just the status
> bit is set.
>
> As the interrupts are generated over a single MDSS line, the controller driver has to read all the
> interrupt status registers and handle all the set interrupt bits. So, while handling aux transfer
> interrupts, we were proceeding to handle connect interrupt also as a consequence.

Ah, I see. There aren't other _interrupts_ in this register. There's
just other status info that someone jammed into this register. Got it.

So with this comment I'm happy with my Reviewed-by:

/*
 * We only want to return interrupts that are unmasked to the caller.
 * However, the interrupt status field also contains other
 * informational bits about the HPD state status, so we only mask
 * out the part of the register that tells us about which interrupts
 * are pending.
 */

-Doug

Re: [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Thu, Apr 21, 2022 at 9:39 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Doug,
>
> >On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
> ><quic_sbillaka@quicinc.com> wrote:
> >>
> >> The panel-edp enables the eDP panel power during probe, get_modes and
> >> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> >> controller are directly dependent on panel power. As eDP display can
> >> be assumed as always connected, the controller driver can skip the eDP
> >> connect and disconnect interrupts. Any disruption in the link status
> >> will be indicated via the IRQ_HPD interrupts.
> >>
> >> So, the eDP controller driver can just enable the IRQ_HPD and replug
> >> interrupts. The DP controller driver still needs to enable all the
> >> interrupts.
> >>
> >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >> ---
> >> Changes in v8:
> >>   - add comment explaining the interrupt status return
> >>
> >> Changes in v7:
> >>   - reordered the patch in the series
> >>   - modified the return statement for isr
> >>   - connector check modified to just check for eDP
> >>
> >>  drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
> >> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> >>  2 files changed, 33 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index fac815f..3a298e9 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> >> *dp_catalog)
> >>
> >>         u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
> >>
> >> -       /* enable HPD plug and unplug interrupts */
> >> -       dp_catalog_hpd_config_intr(dp_catalog,
> >> -               DP_DP_HPD_PLUG_INT_MASK |
> >DP_DP_HPD_UNPLUG_INT_MASK, true);
> >> -
> >>         /* Configure REFTIMER and enable it */
> >>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> >>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@
> >> -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct
> >> dp_catalog *dp_catalog)  {
> >>         struct dp_catalog_private *catalog = container_of(dp_catalog,
> >>                                 struct dp_catalog_private, dp_catalog);
> >> -       int isr = 0;
> >> +       int isr, mask;
> >>
> >>         isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> >>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> >>                                  (isr & DP_DP_HPD_INT_MASK));
> >> +       mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> >>
> >> -       return isr;
> >> +       /*
> >> +        * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
> >> +        * irrespective of their masked status. The HW interrupt will not be
> >> +        * generated if an interrupt is masked. However, the interrupt status
> >> +        * bit in the register will still be set. The eDP controller driver
> >> +        * masks the plug and unplug interrupts unlike DP controller which
> >> +        * unmasks all the interrupts. So, do not report the status of the
> >> +        * masked interrupts.
> >> +        */
> >> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);
> >
> >What's still missing in your comments is why you aren't just doing "return isr &
> >mask;". In other words, why is the API for HPD bits different from the API for
> >non-HPD bits? What code out there wants to know about non-HPD interrupts
> >even if they are masked?
>
> The mask register bitfields are different from the INT_STATUS register.
> The INT_STATUS register has additional bits [31:28] which indicates the HPD state machine
> status and [3:0] indicates the actual generated/set interrupt. The fields [3:0] are similar to
> the mask and ack interrupts.
>
> #define DP_DP_HPD_STATE_STATUS_CONNECTED        (0x40000000)
> #define DP_DP_HPD_STATE_STATUS_PENDING          (0x20000000)
> #define DP_DP_HPD_STATE_STATUS_DISCONNECTED     (0x00000000)
> #define DP_DP_HPD_STATE_STATUS_MASK             (0xE0000000)
>
> So, earlier I returned isr & (mask | DP_DP_HPD_STATE_STATUS_MASK);
>
> Actually, there is no reason to do this. We can just do the below:
> return isr & mask;
>
> >
> >Actually, thinking about this more, my preference would be this:
> >
> >a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
> >
> >b) Create a new function called dp_catalog_hpd_get_intr_status() whose
> >implementation is:
> >
> >  return dp_catalog_hpd_get_intr_status_raw() & mask;
> >
> >Then any callers who care about the raw status can be changed to call
> >dp_catalog_hpd_get_intr_status_raw(). If no callers need
> >dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
> >create this function.
> >
>
> There is no caller for raw status. All interrupts are unmasked for DP.
> While handling the aux interrupts generated during the transfer call from panel probe,
> we read that the HPD interrupt status [3:0] bit0 is set and proceed to handle connect like DP.
> We experimented to find out that the connect interrupt is not generated but just the status
> bit is set.
>
> As the interrupts are generated over a single MDSS line, the controller driver has to read all the
> interrupt status registers and handle all the set interrupt bits. So, while handling aux transfer
> interrupts, we were proceeding to handle connect interrupt also as a consequence.

Ah, I see. There aren't other _interrupts_ in this register. There's
just other status info that someone jammed into this register. Got it.

So with this comment I'm happy with my Reviewed-by:

/*
 * We only want to return interrupts that are unmasked to the caller.
 * However, the interrupt status field also contains other
 * informational bits about the HPD state status, so we only mask
 * out the part of the register that tells us about which interrupts
 * are pending.
 */

-Doug