[PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Changes in v3:
 - Added DPTX AUX block initialization before trying to communicate
   to stop relying on the bootloader keeping it initialized before
   booting Linux.
 - Fixed commit description for patch [09/09] and removed commented
   out code (that slipped from dev phase.. sorry!).

This series adds "real" support for eDP in the mtk-dp DisplayPort driver.

Explaining the "real":
Before this change, the DisplayPort driver did support eDP to some
extent, but it was treating it entirely like a regular DP interface
which is partially fine, after all, embedded DisplayPort *is* actually
DisplayPort, but there might be some differences to account for... and
this is for both small performance improvements and, more importantly,
for correct functionality in some systems.

Functionality first:

One of the common differences found in various boards implementing eDP
and machines using an eDP panel is that many times the HPD line is not
connected. This *must* be accounted for: at startup, this specific IP
will raise a HPD interrupt (which should maybe be ignored... as it does
not appear to be a "real" event...) that will make the eDP panel to be
detected and to actually work but, after a suspend-resume cycle, there
will be no HPD interrupt (as there's no HPD line in my case!) producing
a functionality issue - specifically, the DP Link Training fails because
the panel doesn't get powered up, then it stays black and won't work
until rebooting the machine (or removing and reinserting the module I
think, but I haven't tried that).

Now for.. both:
eDP panels are *e*DP because they are *not* removable (in the sense that
you can't unplug the cable without disassembling the machine, in which
case, the machine shall be powered down..!): this (correct) assumption
makes us able to solve some issues and to also gain a little performance
during PM operations.

What was done here is:
 - Caching the EDID if the panel is eDP: we're always going to read the
   same data everytime, so we can just cache that (as it's small enough)
   shortening PM resume times for the eDP driver instance;
 - Always return connector_status_connected if it's eDP: non-removable
   means connector_status_disconnected can't happen during runtime...
   this also saves us some time and even power, as we won't have to
   perform yet another power cycle of the HW;
 - Added aux-bus support!
   This makes us able to rely on panel autodetection from the EDID,
   avoiding to add more and more panel timings to panel-edp and, even
   better, allowing to use one panel node in devicetrees for multiple
   variants of the same machine since, at that point, it's not important
   to "preventively know" what panel we have (eh, it's autodetected...!).

This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)


P.S.: For your own testing commodity, here's a reference devicetree:
&edp_tx {
	status = "okay";

	pinctrl-names = "default";
	pinctrl-0 = <&edptx_pins_default>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			edp_in: endpoint {
				remote-endpoint = <&dp_intf0_out>;
			};
		};

		port@1 {
			reg = <1>;
			edp_out: endpoint {
				data-lanes = <0 1 2 3>;
				remote-endpoint = <&panel_in>;
			};
		};
	};

	aux-bus {
		panel: panel {
			compatible = "edp-panel";
			power-supply = <&pp3300_disp_x>;
			backlight = <&backlight_lcd0>;
			port {
				panel_in: endpoint {
					remote-endpoint = <&edp_out>;
				};
			};
		};
	};
};

AngeloGioacchino Del Regno (9):
  drm/mediatek: dp: Cache EDID for eDP panel
  drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
  drm/mediatek: dp: Always return connected status for eDP in .detect()
  drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
  drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
  drm/mediatek: dp: Enable event interrupt only when bridge attached
  drm/mediatek: dp: Use devm variant of drm_bridge_add()
  drm/mediatek: dp: Move AUX_P0 setting to
    mtk_dp_initialize_aux_settings()
  drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

 drivers/gpu/drm/mediatek/mtk_dp.c | 186 +++++++++++++++++++-----------
 1 file changed, 116 insertions(+), 70 deletions(-)

-- 
2.40.0

[PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Changes in v3:
 - Added DPTX AUX block initialization before trying to communicate
   to stop relying on the bootloader keeping it initialized before
   booting Linux.
 - Fixed commit description for patch [09/09] and removed commented
   out code (that slipped from dev phase.. sorry!).

This series adds "real" support for eDP in the mtk-dp DisplayPort driver.

Explaining the "real":
Before this change, the DisplayPort driver did support eDP to some
extent, but it was treating it entirely like a regular DP interface
which is partially fine, after all, embedded DisplayPort *is* actually
DisplayPort, but there might be some differences to account for... and
this is for both small performance improvements and, more importantly,
for correct functionality in some systems.

Functionality first:

One of the common differences found in various boards implementing eDP
and machines using an eDP panel is that many times the HPD line is not
connected. This *must* be accounted for: at startup, this specific IP
will raise a HPD interrupt (which should maybe be ignored... as it does
not appear to be a "real" event...) that will make the eDP panel to be
detected and to actually work but, after a suspend-resume cycle, there
will be no HPD interrupt (as there's no HPD line in my case!) producing
a functionality issue - specifically, the DP Link Training fails because
the panel doesn't get powered up, then it stays black and won't work
until rebooting the machine (or removing and reinserting the module I
think, but I haven't tried that).

Now for.. both:
eDP panels are *e*DP because they are *not* removable (in the sense that
you can't unplug the cable without disassembling the machine, in which
case, the machine shall be powered down..!): this (correct) assumption
makes us able to solve some issues and to also gain a little performance
during PM operations.

What was done here is:
 - Caching the EDID if the panel is eDP: we're always going to read the
   same data everytime, so we can just cache that (as it's small enough)
   shortening PM resume times for the eDP driver instance;
 - Always return connector_status_connected if it's eDP: non-removable
   means connector_status_disconnected can't happen during runtime...
   this also saves us some time and even power, as we won't have to
   perform yet another power cycle of the HW;
 - Added aux-bus support!
   This makes us able to rely on panel autodetection from the EDID,
   avoiding to add more and more panel timings to panel-edp and, even
   better, allowing to use one panel node in devicetrees for multiple
   variants of the same machine since, at that point, it's not important
   to "preventively know" what panel we have (eh, it's autodetected...!).

This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)


P.S.: For your own testing commodity, here's a reference devicetree:
&edp_tx {
	status = "okay";

	pinctrl-names = "default";
	pinctrl-0 = <&edptx_pins_default>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			edp_in: endpoint {
				remote-endpoint = <&dp_intf0_out>;
			};
		};

		port@1 {
			reg = <1>;
			edp_out: endpoint {
				data-lanes = <0 1 2 3>;
				remote-endpoint = <&panel_in>;
			};
		};
	};

	aux-bus {
		panel: panel {
			compatible = "edp-panel";
			power-supply = <&pp3300_disp_x>;
			backlight = <&backlight_lcd0>;
			port {
				panel_in: endpoint {
					remote-endpoint = <&edp_out>;
				};
			};
		};
	};
};

AngeloGioacchino Del Regno (9):
  drm/mediatek: dp: Cache EDID for eDP panel
  drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
  drm/mediatek: dp: Always return connected status for eDP in .detect()
  drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
  drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
  drm/mediatek: dp: Enable event interrupt only when bridge attached
  drm/mediatek: dp: Use devm variant of drm_bridge_add()
  drm/mediatek: dp: Move AUX_P0 setting to
    mtk_dp_initialize_aux_settings()
  drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

 drivers/gpu/drm/mediatek/mtk_dp.c | 186 +++++++++++++++++++-----------
 1 file changed, 116 insertions(+), 70 deletions(-)

-- 
2.40.0

[PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Changes in v3:
 - Added DPTX AUX block initialization before trying to communicate
   to stop relying on the bootloader keeping it initialized before
   booting Linux.
 - Fixed commit description for patch [09/09] and removed commented
   out code (that slipped from dev phase.. sorry!).

This series adds "real" support for eDP in the mtk-dp DisplayPort driver.

Explaining the "real":
Before this change, the DisplayPort driver did support eDP to some
extent, but it was treating it entirely like a regular DP interface
which is partially fine, after all, embedded DisplayPort *is* actually
DisplayPort, but there might be some differences to account for... and
this is for both small performance improvements and, more importantly,
for correct functionality in some systems.

Functionality first:

One of the common differences found in various boards implementing eDP
and machines using an eDP panel is that many times the HPD line is not
connected. This *must* be accounted for: at startup, this specific IP
will raise a HPD interrupt (which should maybe be ignored... as it does
not appear to be a "real" event...) that will make the eDP panel to be
detected and to actually work but, after a suspend-resume cycle, there
will be no HPD interrupt (as there's no HPD line in my case!) producing
a functionality issue - specifically, the DP Link Training fails because
the panel doesn't get powered up, then it stays black and won't work
until rebooting the machine (or removing and reinserting the module I
think, but I haven't tried that).

Now for.. both:
eDP panels are *e*DP because they are *not* removable (in the sense that
you can't unplug the cable without disassembling the machine, in which
case, the machine shall be powered down..!): this (correct) assumption
makes us able to solve some issues and to also gain a little performance
during PM operations.

What was done here is:
 - Caching the EDID if the panel is eDP: we're always going to read the
   same data everytime, so we can just cache that (as it's small enough)
   shortening PM resume times for the eDP driver instance;
 - Always return connector_status_connected if it's eDP: non-removable
   means connector_status_disconnected can't happen during runtime...
   this also saves us some time and even power, as we won't have to
   perform yet another power cycle of the HW;
 - Added aux-bus support!
   This makes us able to rely on panel autodetection from the EDID,
   avoiding to add more and more panel timings to panel-edp and, even
   better, allowing to use one panel node in devicetrees for multiple
   variants of the same machine since, at that point, it's not important
   to "preventively know" what panel we have (eh, it's autodetected...!).

This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)


P.S.: For your own testing commodity, here's a reference devicetree:
&edp_tx {
	status = "okay";

	pinctrl-names = "default";
	pinctrl-0 = <&edptx_pins_default>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			edp_in: endpoint {
				remote-endpoint = <&dp_intf0_out>;
			};
		};

		port@1 {
			reg = <1>;
			edp_out: endpoint {
				data-lanes = <0 1 2 3>;
				remote-endpoint = <&panel_in>;
			};
		};
	};

	aux-bus {
		panel: panel {
			compatible = "edp-panel";
			power-supply = <&pp3300_disp_x>;
			backlight = <&backlight_lcd0>;
			port {
				panel_in: endpoint {
					remote-endpoint = <&edp_out>;
				};
			};
		};
	};
};

AngeloGioacchino Del Regno (9):
  drm/mediatek: dp: Cache EDID for eDP panel
  drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
  drm/mediatek: dp: Always return connected status for eDP in .detect()
  drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
  drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
  drm/mediatek: dp: Enable event interrupt only when bridge attached
  drm/mediatek: dp: Use devm variant of drm_bridge_add()
  drm/mediatek: dp: Move AUX_P0 setting to
    mtk_dp_initialize_aux_settings()
  drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

 drivers/gpu/drm/mediatek/mtk_dp.c | 186 +++++++++++++++++++-----------
 1 file changed, 116 insertions(+), 70 deletions(-)

-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Since eDP panels are not removable it is safe to cache the EDID:
this will avoid a relatively long read transaction at every PM
resume that is unnecessary only in the "special" case of eDP,
hence speeding it up a little, as from now on, as resume operation,
we will perform only link training.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 1f94fcc144d3..84f82cc68672 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -118,6 +118,7 @@ struct mtk_dp {
 	const struct mtk_dp_data *data;
 	struct mtk_dp_info info;
 	struct mtk_dp_train_info train_info;
+	struct edid *edid;
 
 	struct platform_device *phy_dev;
 	struct phy *phy;
@@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 		usleep_range(2000, 5000);
 	}
 
-	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
+	/* eDP panels aren't removable, so we can return a cached EDID. */
+	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		new_edid = drm_edid_duplicate(mtk_dp->edid);
+	else
+		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
 
 	/*
 	 * Parse capability here to let atomic_get_input_bus_fmts and
@@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 	}
 
+	/* If this is an eDP panel and the read EDID is good, cache it for later */
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
+		mtk_dp->edid = drm_edid_duplicate(new_edid);
+
 	return new_edid;
 }
 
-- 
2.40.0

[PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Since eDP panels are not removable it is safe to cache the EDID:
this will avoid a relatively long read transaction at every PM
resume that is unnecessary only in the "special" case of eDP,
hence speeding it up a little, as from now on, as resume operation,
we will perform only link training.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 1f94fcc144d3..84f82cc68672 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -118,6 +118,7 @@ struct mtk_dp {
 	const struct mtk_dp_data *data;
 	struct mtk_dp_info info;
 	struct mtk_dp_train_info train_info;
+	struct edid *edid;
 
 	struct platform_device *phy_dev;
 	struct phy *phy;
@@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 		usleep_range(2000, 5000);
 	}
 
-	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
+	/* eDP panels aren't removable, so we can return a cached EDID. */
+	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		new_edid = drm_edid_duplicate(mtk_dp->edid);
+	else
+		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
 
 	/*
 	 * Parse capability here to let atomic_get_input_bus_fmts and
@@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 	}
 
+	/* If this is an eDP panel and the read EDID is good, cache it for later */
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
+		mtk_dp->edid = drm_edid_duplicate(new_edid);
+
 	return new_edid;
 }
 
-- 
2.40.0

[PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Since eDP panels are not removable it is safe to cache the EDID:
this will avoid a relatively long read transaction at every PM
resume that is unnecessary only in the "special" case of eDP,
hence speeding it up a little, as from now on, as resume operation,
we will perform only link training.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 1f94fcc144d3..84f82cc68672 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -118,6 +118,7 @@ struct mtk_dp {
 	const struct mtk_dp_data *data;
 	struct mtk_dp_info info;
 	struct mtk_dp_train_info train_info;
+	struct edid *edid;
 
 	struct platform_device *phy_dev;
 	struct phy *phy;
@@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 		usleep_range(2000, 5000);
 	}
 
-	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
+	/* eDP panels aren't removable, so we can return a cached EDID. */
+	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		new_edid = drm_edid_duplicate(mtk_dp->edid);
+	else
+		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
 
 	/*
 	 * Parse capability here to let atomic_get_input_bus_fmts and
@@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 	}
 
+	/* If this is an eDP panel and the read EDID is good, cache it for later */
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
+		mtk_dp->edid = drm_edid_duplicate(new_edid);
+
 	return new_edid;
 }
 
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[AngeloGioacchino Del Regno]

Everytime we run bridge detection and/or EDID read we run a poweron
and poweroff sequence for both the AUX and the panel; moreover, this
is also done when enabling the bridge in the .atomic_enable() callback.

Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
function as to commonize it.
Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
the AUX was getting powered on but the panel was left powered off if
the DP cable wasn't plugged in while now we unconditionally send a D0
request and this is done for two reasons:
 - First, whether this request fails or not, it takes the same time
   and anyway the DP hardware won't produce any error (or, if it
   does, it's ignorable because it won't block further commands)
 - Second, training the link between a sleeping/standby/unpowered
   display makes little sense.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
 1 file changed, 30 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 84f82cc68672..76ea94167531 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
 			   val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
 }
 
+static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
+{
+	if (pwron) {
+		/* power on aux */
+		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
+				   DP_PWR_STATE_MASK);
+
+		/* power on panel */
+		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+		usleep_range(2000, 5000);
+	} else {
+		/* power off panel */
+		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
+		usleep_range(2000, 3000);
+
+		/* power off aux */
+		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+				   DP_PWR_STATE_BANDGAP_TPLL,
+				   DP_PWR_STATE_MASK);
+	}
+}
+
 static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
 {
 	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
@@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	if (!mtk_dp->train_info.cable_plugged_in)
 		return ret;
 
-	if (!enabled) {
-		/* power on aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-				   DP_PWR_STATE_MASK);
+	if (!enabled)
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
 
-		/* power on panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
-	}
 	/*
 	 * Some dongles still source HPD when they do not connect to any
 	 * sink device. To avoid this, we need to read the sink count
@@ -1958,16 +1974,8 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	if (DP_GET_SINK_COUNT(sink_count))
 		ret = connector_status_connected;
 
-	if (!enabled) {
-		/* power off panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
-		usleep_range(2000, 3000);
-
-		/* power off aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL,
-				   DP_PWR_STATE_MASK);
-	}
+	if (!enabled)
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
 
 	return ret;
 }
@@ -1983,15 +1991,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 
 	if (!enabled) {
 		drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
-
-		/* power on aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-				   DP_PWR_STATE_MASK);
-
-		/* power on panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
 	}
 
 	/* eDP panels aren't removable, so we can return a cached EDID. */
@@ -2015,15 +2015,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 	}
 
 	if (!enabled) {
-		/* power off panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
-		usleep_range(2000, 3000);
-
-		/* power off aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL,
-				   DP_PWR_STATE_MASK);
-
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
 		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 	}
 
@@ -2188,15 +2180,7 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 		return;
 	}
 
-	/* power on aux */
-	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-			   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-			   DP_PWR_STATE_MASK);
-
-	if (mtk_dp->train_info.cable_plugged_in) {
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
-	}
+	mtk_dp_aux_panel_poweron(mtk_dp, true);
 
 	/* Training */
 	ret = mtk_dp_training(mtk_dp);
-- 
2.40.0

[PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[AngeloGioacchino Del Regno]

Everytime we run bridge detection and/or EDID read we run a poweron
and poweroff sequence for both the AUX and the panel; moreover, this
is also done when enabling the bridge in the .atomic_enable() callback.

Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
function as to commonize it.
Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
the AUX was getting powered on but the panel was left powered off if
the DP cable wasn't plugged in while now we unconditionally send a D0
request and this is done for two reasons:
 - First, whether this request fails or not, it takes the same time
   and anyway the DP hardware won't produce any error (or, if it
   does, it's ignorable because it won't block further commands)
 - Second, training the link between a sleeping/standby/unpowered
   display makes little sense.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
 1 file changed, 30 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 84f82cc68672..76ea94167531 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
 			   val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
 }
 
+static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
+{
+	if (pwron) {
+		/* power on aux */
+		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
+				   DP_PWR_STATE_MASK);
+
+		/* power on panel */
+		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+		usleep_range(2000, 5000);
+	} else {
+		/* power off panel */
+		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
+		usleep_range(2000, 3000);
+
+		/* power off aux */
+		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+				   DP_PWR_STATE_BANDGAP_TPLL,
+				   DP_PWR_STATE_MASK);
+	}
+}
+
 static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
 {
 	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
@@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	if (!mtk_dp->train_info.cable_plugged_in)
 		return ret;
 
-	if (!enabled) {
-		/* power on aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-				   DP_PWR_STATE_MASK);
+	if (!enabled)
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
 
-		/* power on panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
-	}
 	/*
 	 * Some dongles still source HPD when they do not connect to any
 	 * sink device. To avoid this, we need to read the sink count
@@ -1958,16 +1974,8 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	if (DP_GET_SINK_COUNT(sink_count))
 		ret = connector_status_connected;
 
-	if (!enabled) {
-		/* power off panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
-		usleep_range(2000, 3000);
-
-		/* power off aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL,
-				   DP_PWR_STATE_MASK);
-	}
+	if (!enabled)
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
 
 	return ret;
 }
@@ -1983,15 +1991,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 
 	if (!enabled) {
 		drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
-
-		/* power on aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-				   DP_PWR_STATE_MASK);
-
-		/* power on panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
 	}
 
 	/* eDP panels aren't removable, so we can return a cached EDID. */
@@ -2015,15 +2015,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 	}
 
 	if (!enabled) {
-		/* power off panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
-		usleep_range(2000, 3000);
-
-		/* power off aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL,
-				   DP_PWR_STATE_MASK);
-
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
 		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 	}
 
@@ -2188,15 +2180,7 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 		return;
 	}
 
-	/* power on aux */
-	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-			   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-			   DP_PWR_STATE_MASK);
-
-	if (mtk_dp->train_info.cable_plugged_in) {
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
-	}
+	mtk_dp_aux_panel_poweron(mtk_dp, true);
 
 	/* Training */
 	ret = mtk_dp_training(mtk_dp);
-- 
2.40.0

[PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[AngeloGioacchino Del Regno]

Everytime we run bridge detection and/or EDID read we run a poweron
and poweroff sequence for both the AUX and the panel; moreover, this
is also done when enabling the bridge in the .atomic_enable() callback.

Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
function as to commonize it.
Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
the AUX was getting powered on but the panel was left powered off if
the DP cable wasn't plugged in while now we unconditionally send a D0
request and this is done for two reasons:
 - First, whether this request fails or not, it takes the same time
   and anyway the DP hardware won't produce any error (or, if it
   does, it's ignorable because it won't block further commands)
 - Second, training the link between a sleeping/standby/unpowered
   display makes little sense.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
 1 file changed, 30 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 84f82cc68672..76ea94167531 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
 			   val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
 }
 
+static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
+{
+	if (pwron) {
+		/* power on aux */
+		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
+				   DP_PWR_STATE_MASK);
+
+		/* power on panel */
+		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+		usleep_range(2000, 5000);
+	} else {
+		/* power off panel */
+		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
+		usleep_range(2000, 3000);
+
+		/* power off aux */
+		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+				   DP_PWR_STATE_BANDGAP_TPLL,
+				   DP_PWR_STATE_MASK);
+	}
+}
+
 static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
 {
 	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
@@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	if (!mtk_dp->train_info.cable_plugged_in)
 		return ret;
 
-	if (!enabled) {
-		/* power on aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-				   DP_PWR_STATE_MASK);
+	if (!enabled)
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
 
-		/* power on panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
-	}
 	/*
 	 * Some dongles still source HPD when they do not connect to any
 	 * sink device. To avoid this, we need to read the sink count
@@ -1958,16 +1974,8 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	if (DP_GET_SINK_COUNT(sink_count))
 		ret = connector_status_connected;
 
-	if (!enabled) {
-		/* power off panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
-		usleep_range(2000, 3000);
-
-		/* power off aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL,
-				   DP_PWR_STATE_MASK);
-	}
+	if (!enabled)
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
 
 	return ret;
 }
@@ -1983,15 +1991,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 
 	if (!enabled) {
 		drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
-
-		/* power on aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-				   DP_PWR_STATE_MASK);
-
-		/* power on panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
 	}
 
 	/* eDP panels aren't removable, so we can return a cached EDID. */
@@ -2015,15 +2015,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
 	}
 
 	if (!enabled) {
-		/* power off panel */
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
-		usleep_range(2000, 3000);
-
-		/* power off aux */
-		mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-				   DP_PWR_STATE_BANDGAP_TPLL,
-				   DP_PWR_STATE_MASK);
-
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
 		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 	}
 
@@ -2188,15 +2180,7 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 		return;
 	}
 
-	/* power on aux */
-	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
-			   DP_PWR_STATE_BANDGAP_TPLL_LANE,
-			   DP_PWR_STATE_MASK);
-
-	if (mtk_dp->train_info.cable_plugged_in) {
-		drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-		usleep_range(2000, 5000);
-	}
+	mtk_dp_aux_panel_poweron(mtk_dp, true);
 
 	/* Training */
 	ret = mtk_dp_training(mtk_dp);
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/9] drm/mediatek: dp: Always return connected status for eDP in .detect()

[AngeloGioacchino Del Regno]

If this is an eDP panel it's not removable, hence it's always connected:
as a shortcut, always return connector_status_connected in the .detect()
callback for eDP connector, avoiding a poweron, a check for sink count
and a poweroff.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 76ea94167531..c82dd1f0675d 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1957,6 +1957,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	bool enabled = mtk_dp->enabled;
 	u8 sink_count = 0;
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		return connector_status_connected;
+
 	if (!mtk_dp->train_info.cable_plugged_in)
 		return ret;
 
-- 
2.40.0

[PATCH v3 3/9] drm/mediatek: dp: Always return connected status for eDP in .detect()

[AngeloGioacchino Del Regno]

If this is an eDP panel it's not removable, hence it's always connected:
as a shortcut, always return connector_status_connected in the .detect()
callback for eDP connector, avoiding a poweron, a check for sink count
and a poweroff.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 76ea94167531..c82dd1f0675d 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1957,6 +1957,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	bool enabled = mtk_dp->enabled;
 	u8 sink_count = 0;
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		return connector_status_connected;
+
 	if (!mtk_dp->train_info.cable_plugged_in)
 		return ret;
 
-- 
2.40.0

[PATCH v3 3/9] drm/mediatek: dp: Always return connected status for eDP in .detect()

[AngeloGioacchino Del Regno]

If this is an eDP panel it's not removable, hence it's always connected:
as a shortcut, always return connector_status_connected in the .detect()
callback for eDP connector, avoiding a poweron, a check for sink count
and a poweroff.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 76ea94167531..c82dd1f0675d 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1957,6 +1957,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
 	bool enabled = mtk_dp->enabled;
 	u8 sink_count = 0;
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		return connector_status_connected;
+
 	if (!mtk_dp->train_info.cable_plugged_in)
 		return ret;
 
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/9] drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel

[AngeloGioacchino Del Regno]

eDP panels are not removable: at PM resume, the cable will obviously
still be plugged in.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index c82dd1f0675d..ac21eca0e20e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -2607,6 +2607,9 @@ static int mtk_dp_resume(struct device *dev)
 	mtk_dp_hwirq_enable(mtk_dp, true);
 	mtk_dp_power_enable(mtk_dp);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		mtk_dp->train_info.cable_plugged_in = true;
+
 	return 0;
 }
 #endif
-- 
2.40.0

[PATCH v3 4/9] drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel

[AngeloGioacchino Del Regno]

eDP panels are not removable: at PM resume, the cable will obviously
still be plugged in.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index c82dd1f0675d..ac21eca0e20e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -2607,6 +2607,9 @@ static int mtk_dp_resume(struct device *dev)
 	mtk_dp_hwirq_enable(mtk_dp, true);
 	mtk_dp_power_enable(mtk_dp);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		mtk_dp->train_info.cable_plugged_in = true;
+
 	return 0;
 }
 #endif
-- 
2.40.0

[PATCH v3 4/9] drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel

[AngeloGioacchino Del Regno]

eDP panels are not removable: at PM resume, the cable will obviously
still be plugged in.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index c82dd1f0675d..ac21eca0e20e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -2607,6 +2607,9 @@ static int mtk_dp_resume(struct device *dev)
 	mtk_dp_hwirq_enable(mtk_dp, true);
 	mtk_dp_power_enable(mtk_dp);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
+		mtk_dp->train_info.cable_plugged_in = true;
+
 	return 0;
 }
 #endif
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/9] drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()

[AngeloGioacchino Del Regno]

Change logging from drm_{err,info}() to dev_{err,info}() in functions
mtk_dp_aux_transfer() and mtk_dp_aux_do_transfer(): this will be
essential to avoid getting NULL pointer kernel panics if any kind
of error happens during AUX transfers happening before the bridge
is attached.

This may potentially start happening in a later commit implementing
aux-bus support, as AUX transfers will be triggered from the panel
driver (for EDID) before the mtk-dp bridge gets attached, and it's
done in preparation for the same.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index ac21eca0e20e..19b145cf2e05 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -849,7 +849,7 @@ static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool is_read, u8 cmd,
 		u32 phy_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3628) &
 				 AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
 		if (phy_status != AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
-			drm_err(mtk_dp->drm_dev,
+			dev_err(mtk_dp->dev,
 				"AUX Rx Aux hang, need SW reset\n");
 			return -EIO;
 		}
@@ -2061,7 +2061,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 		is_read = true;
 		break;
 	default:
-		drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
+		dev_err(mtk_dp->dev, "invalid aux cmd = %d\n",
 			msg->request);
 		ret = -EINVAL;
 		goto err;
@@ -2077,7 +2077,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 					     to_access);
 
 		if (ret) {
-			drm_info(mtk_dp->drm_dev,
+			dev_info(mtk_dp->dev,
 				 "Failed to do AUX transfer: %d\n", ret);
 			goto err;
 		}
-- 
2.40.0

[PATCH v3 5/9] drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()

[AngeloGioacchino Del Regno]

Change logging from drm_{err,info}() to dev_{err,info}() in functions
mtk_dp_aux_transfer() and mtk_dp_aux_do_transfer(): this will be
essential to avoid getting NULL pointer kernel panics if any kind
of error happens during AUX transfers happening before the bridge
is attached.

This may potentially start happening in a later commit implementing
aux-bus support, as AUX transfers will be triggered from the panel
driver (for EDID) before the mtk-dp bridge gets attached, and it's
done in preparation for the same.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index ac21eca0e20e..19b145cf2e05 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -849,7 +849,7 @@ static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool is_read, u8 cmd,
 		u32 phy_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3628) &
 				 AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
 		if (phy_status != AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
-			drm_err(mtk_dp->drm_dev,
+			dev_err(mtk_dp->dev,
 				"AUX Rx Aux hang, need SW reset\n");
 			return -EIO;
 		}
@@ -2061,7 +2061,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 		is_read = true;
 		break;
 	default:
-		drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
+		dev_err(mtk_dp->dev, "invalid aux cmd = %d\n",
 			msg->request);
 		ret = -EINVAL;
 		goto err;
@@ -2077,7 +2077,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 					     to_access);
 
 		if (ret) {
-			drm_info(mtk_dp->drm_dev,
+			dev_info(mtk_dp->dev,
 				 "Failed to do AUX transfer: %d\n", ret);
 			goto err;
 		}
-- 
2.40.0

[PATCH v3 5/9] drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()

[AngeloGioacchino Del Regno]

Change logging from drm_{err,info}() to dev_{err,info}() in functions
mtk_dp_aux_transfer() and mtk_dp_aux_do_transfer(): this will be
essential to avoid getting NULL pointer kernel panics if any kind
of error happens during AUX transfers happening before the bridge
is attached.

This may potentially start happening in a later commit implementing
aux-bus support, as AUX transfers will be triggered from the panel
driver (for EDID) before the mtk-dp bridge gets attached, and it's
done in preparation for the same.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index ac21eca0e20e..19b145cf2e05 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -849,7 +849,7 @@ static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool is_read, u8 cmd,
 		u32 phy_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3628) &
 				 AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
 		if (phy_status != AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
-			drm_err(mtk_dp->drm_dev,
+			dev_err(mtk_dp->dev,
 				"AUX Rx Aux hang, need SW reset\n");
 			return -EIO;
 		}
@@ -2061,7 +2061,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 		is_read = true;
 		break;
 	default:
-		drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
+		dev_err(mtk_dp->dev, "invalid aux cmd = %d\n",
 			msg->request);
 		ret = -EINVAL;
 		goto err;
@@ -2077,7 +2077,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 					     to_access);
 
 		if (ret) {
-			drm_info(mtk_dp->drm_dev,
+			dev_info(mtk_dp->dev,
 				 "Failed to do AUX transfer: %d\n", ret);
 			goto err;
 		}
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 6/9] drm/mediatek: dp: Enable event interrupt only when bridge attached

[AngeloGioacchino Del Regno]

In preparation for implementing support for aux-bus in this driver,
add a IRQ_NOAUTOEN flag to the event interrupt that we request at
probe time and manage the enablement of the ISR at bridge_attach
and detach time.

When aux-bus will be implemented, enabling the interrupt before
attaching the bridge will create an event storm and hang the kernel
during boot.
In any case, the interrupt handler anyway requires resources that
are initialized by mtk_dp_bridge_attach(), so it cannot do anything
meaningful without... and even crash, but that's not happening in
the current code because the HW remains unpowered until resources
are made available.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 19b145cf2e05..6aaf162a6bfe 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -100,6 +100,7 @@ struct mtk_dp_efuse_fmt {
 struct mtk_dp {
 	bool enabled;
 	bool need_debounce;
+	int irq;
 	u8 max_lanes;
 	u8 max_linkrate;
 	u8 rx_cap[DP_RECEIVER_CAP_SIZE];
@@ -2148,6 +2149,8 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
 
 	mtk_dp->drm_dev = bridge->dev;
 
+	irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
+	enable_irq(mtk_dp->irq);
 	mtk_dp_hwirq_enable(mtk_dp, true);
 
 	return 0;
@@ -2164,6 +2167,7 @@ static void mtk_dp_bridge_detach(struct drm_bridge *bridge)
 	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
 
 	mtk_dp_hwirq_enable(mtk_dp, false);
+	disable_irq(mtk_dp->irq);
 	mtk_dp->drm_dev = NULL;
 	mtk_dp_poweroff(mtk_dp);
 	drm_dp_aux_unregister(&mtk_dp->aux);
@@ -2482,7 +2486,7 @@ static int mtk_dp_probe(struct platform_device *pdev)
 {
 	struct mtk_dp *mtk_dp;
 	struct device *dev = &pdev->dev;
-	int ret, irq_num;
+	int ret;
 
 	mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL);
 	if (!mtk_dp)
@@ -2491,9 +2495,9 @@ static int mtk_dp_probe(struct platform_device *pdev)
 	mtk_dp->dev = dev;
 	mtk_dp->data = (struct mtk_dp_data *)of_device_get_match_data(dev);
 
-	irq_num = platform_get_irq(pdev, 0);
-	if (irq_num < 0)
-		return dev_err_probe(dev, irq_num,
+	mtk_dp->irq = platform_get_irq(pdev, 0);
+	if (mtk_dp->irq < 0)
+		return dev_err_probe(dev, mtk_dp->irq,
 				     "failed to request dp irq resource\n");
 
 	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
@@ -2514,7 +2518,8 @@ static int mtk_dp_probe(struct platform_device *pdev)
 
 	spin_lock_init(&mtk_dp->irq_thread_lock);
 
-	ret = devm_request_threaded_irq(dev, irq_num, mtk_dp_hpd_event,
+	irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(dev, mtk_dp->irq, mtk_dp_hpd_event,
 					mtk_dp_hpd_event_thread,
 					IRQ_TYPE_LEVEL_HIGH, dev_name(dev),
 					mtk_dp);
-- 
2.40.0

[PATCH v3 6/9] drm/mediatek: dp: Enable event interrupt only when bridge attached

[AngeloGioacchino Del Regno]

In preparation for implementing support for aux-bus in this driver,
add a IRQ_NOAUTOEN flag to the event interrupt that we request at
probe time and manage the enablement of the ISR at bridge_attach
and detach time.

When aux-bus will be implemented, enabling the interrupt before
attaching the bridge will create an event storm and hang the kernel
during boot.
In any case, the interrupt handler anyway requires resources that
are initialized by mtk_dp_bridge_attach(), so it cannot do anything
meaningful without... and even crash, but that's not happening in
the current code because the HW remains unpowered until resources
are made available.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 19b145cf2e05..6aaf162a6bfe 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -100,6 +100,7 @@ struct mtk_dp_efuse_fmt {
 struct mtk_dp {
 	bool enabled;
 	bool need_debounce;
+	int irq;
 	u8 max_lanes;
 	u8 max_linkrate;
 	u8 rx_cap[DP_RECEIVER_CAP_SIZE];
@@ -2148,6 +2149,8 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
 
 	mtk_dp->drm_dev = bridge->dev;
 
+	irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
+	enable_irq(mtk_dp->irq);
 	mtk_dp_hwirq_enable(mtk_dp, true);
 
 	return 0;
@@ -2164,6 +2167,7 @@ static void mtk_dp_bridge_detach(struct drm_bridge *bridge)
 	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
 
 	mtk_dp_hwirq_enable(mtk_dp, false);
+	disable_irq(mtk_dp->irq);
 	mtk_dp->drm_dev = NULL;
 	mtk_dp_poweroff(mtk_dp);
 	drm_dp_aux_unregister(&mtk_dp->aux);
@@ -2482,7 +2486,7 @@ static int mtk_dp_probe(struct platform_device *pdev)
 {
 	struct mtk_dp *mtk_dp;
 	struct device *dev = &pdev->dev;
-	int ret, irq_num;
+	int ret;
 
 	mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL);
 	if (!mtk_dp)
@@ -2491,9 +2495,9 @@ static int mtk_dp_probe(struct platform_device *pdev)
 	mtk_dp->dev = dev;
 	mtk_dp->data = (struct mtk_dp_data *)of_device_get_match_data(dev);
 
-	irq_num = platform_get_irq(pdev, 0);
-	if (irq_num < 0)
-		return dev_err_probe(dev, irq_num,
+	mtk_dp->irq = platform_get_irq(pdev, 0);
+	if (mtk_dp->irq < 0)
+		return dev_err_probe(dev, mtk_dp->irq,
 				     "failed to request dp irq resource\n");
 
 	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
@@ -2514,7 +2518,8 @@ static int mtk_dp_probe(struct platform_device *pdev)
 
 	spin_lock_init(&mtk_dp->irq_thread_lock);
 
-	ret = devm_request_threaded_irq(dev, irq_num, mtk_dp_hpd_event,
+	irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(dev, mtk_dp->irq, mtk_dp_hpd_event,
 					mtk_dp_hpd_event_thread,
 					IRQ_TYPE_LEVEL_HIGH, dev_name(dev),
 					mtk_dp);
-- 
2.40.0

[PATCH v3 6/9] drm/mediatek: dp: Enable event interrupt only when bridge attached

[AngeloGioacchino Del Regno]

In preparation for implementing support for aux-bus in this driver,
add a IRQ_NOAUTOEN flag to the event interrupt that we request at
probe time and manage the enablement of the ISR at bridge_attach
and detach time.

When aux-bus will be implemented, enabling the interrupt before
attaching the bridge will create an event storm and hang the kernel
during boot.
In any case, the interrupt handler anyway requires resources that
are initialized by mtk_dp_bridge_attach(), so it cannot do anything
meaningful without... and even crash, but that's not happening in
the current code because the HW remains unpowered until resources
are made available.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 19b145cf2e05..6aaf162a6bfe 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -100,6 +100,7 @@ struct mtk_dp_efuse_fmt {
 struct mtk_dp {
 	bool enabled;
 	bool need_debounce;
+	int irq;
 	u8 max_lanes;
 	u8 max_linkrate;
 	u8 rx_cap[DP_RECEIVER_CAP_SIZE];
@@ -2148,6 +2149,8 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
 
 	mtk_dp->drm_dev = bridge->dev;
 
+	irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
+	enable_irq(mtk_dp->irq);
 	mtk_dp_hwirq_enable(mtk_dp, true);
 
 	return 0;
@@ -2164,6 +2167,7 @@ static void mtk_dp_bridge_detach(struct drm_bridge *bridge)
 	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
 
 	mtk_dp_hwirq_enable(mtk_dp, false);
+	disable_irq(mtk_dp->irq);
 	mtk_dp->drm_dev = NULL;
 	mtk_dp_poweroff(mtk_dp);
 	drm_dp_aux_unregister(&mtk_dp->aux);
@@ -2482,7 +2486,7 @@ static int mtk_dp_probe(struct platform_device *pdev)
 {
 	struct mtk_dp *mtk_dp;
 	struct device *dev = &pdev->dev;
-	int ret, irq_num;
+	int ret;
 
 	mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL);
 	if (!mtk_dp)
@@ -2491,9 +2495,9 @@ static int mtk_dp_probe(struct platform_device *pdev)
 	mtk_dp->dev = dev;
 	mtk_dp->data = (struct mtk_dp_data *)of_device_get_match_data(dev);
 
-	irq_num = platform_get_irq(pdev, 0);
-	if (irq_num < 0)
-		return dev_err_probe(dev, irq_num,
+	mtk_dp->irq = platform_get_irq(pdev, 0);
+	if (mtk_dp->irq < 0)
+		return dev_err_probe(dev, mtk_dp->irq,
 				     "failed to request dp irq resource\n");
 
 	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
@@ -2514,7 +2518,8 @@ static int mtk_dp_probe(struct platform_device *pdev)
 
 	spin_lock_init(&mtk_dp->irq_thread_lock);
 
-	ret = devm_request_threaded_irq(dev, irq_num, mtk_dp_hpd_event,
+	irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(dev, mtk_dp->irq, mtk_dp_hpd_event,
 					mtk_dp_hpd_event_thread,
 					IRQ_TYPE_LEVEL_HIGH, dev_name(dev),
 					mtk_dp);
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 7/9] drm/mediatek: dp: Use devm variant of drm_bridge_add()

[AngeloGioacchino Del Regno]

In preparation for adding support for aux-bus, which will add a code
path that may fail after the drm_bridge_add() call, change that to
devm_drm_bridge_add() to simplify failure paths later.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 6aaf162a6bfe..62d53c4b3feb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -2565,7 +2565,7 @@ static int mtk_dp_probe(struct platform_device *pdev)
 		DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
 	mtk_dp->bridge.type = mtk_dp->data->bridge_type;
 
-	drm_bridge_add(&mtk_dp->bridge);
+	devm_drm_bridge_add(dev, &mtk_dp->bridge);
 
 	mtk_dp->need_debounce = true;
 	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
-- 
2.40.0

[PATCH v3 7/9] drm/mediatek: dp: Use devm variant of drm_bridge_add()

[AngeloGioacchino Del Regno]

In preparation for adding support for aux-bus, which will add a code
path that may fail after the drm_bridge_add() call, change that to
devm_drm_bridge_add() to simplify failure paths later.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 6aaf162a6bfe..62d53c4b3feb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -2565,7 +2565,7 @@ static int mtk_dp_probe(struct platform_device *pdev)
 		DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
 	mtk_dp->bridge.type = mtk_dp->data->bridge_type;
 
-	drm_bridge_add(&mtk_dp->bridge);
+	devm_drm_bridge_add(dev, &mtk_dp->bridge);
 
 	mtk_dp->need_debounce = true;
 	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
-- 
2.40.0

[PATCH v3 7/9] drm/mediatek: dp: Use devm variant of drm_bridge_add()

[AngeloGioacchino Del Regno]

In preparation for adding support for aux-bus, which will add a code
path that may fail after the drm_bridge_add() call, change that to
devm_drm_bridge_add() to simplify failure paths later.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 6aaf162a6bfe..62d53c4b3feb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -2565,7 +2565,7 @@ static int mtk_dp_probe(struct platform_device *pdev)
 		DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
 	mtk_dp->bridge.type = mtk_dp->data->bridge_type;
 
-	drm_bridge_add(&mtk_dp->bridge);
+	devm_drm_bridge_add(dev, &mtk_dp->bridge);
 
 	mtk_dp->need_debounce = true;
 	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 8/9] drm/mediatek: dp: Move AUX_P0 setting to mtk_dp_initialize_aux_settings()

[AngeloGioacchino Del Regno]

Move the register write to MTK_DP_AUX_P0_3690 to set the AUX reply mode
to function mtk_dp_initialize_aux_settings(), as this is effectively
part of the DPTX AUX setup sequence.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 62d53c4b3feb..a67143c22024 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1012,6 +1012,11 @@ static void mtk_dp_initialize_aux_settings(struct mtk_dp *mtk_dp)
 	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_37C8,
 			   MTK_ATOP_EN_AUX_TX_P0,
 			   MTK_ATOP_EN_AUX_TX_P0);
+
+	/* Set complete reply mode for AUX */
+	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3690,
+			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0,
+			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0);
 }
 
 static void mtk_dp_initialize_digital_settings(struct mtk_dp *mtk_dp)
@@ -1824,10 +1829,6 @@ static void mtk_dp_init_port(struct mtk_dp *mtk_dp)
 	mtk_dp_initialize_settings(mtk_dp);
 	mtk_dp_initialize_aux_settings(mtk_dp);
 	mtk_dp_initialize_digital_settings(mtk_dp);
-
-	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3690,
-			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0,
-			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0);
 	mtk_dp_initialize_hpd_detect_settings(mtk_dp);
 
 	mtk_dp_digital_sw_reset(mtk_dp);
-- 
2.40.0

[PATCH v3 8/9] drm/mediatek: dp: Move AUX_P0 setting to mtk_dp_initialize_aux_settings()

[AngeloGioacchino Del Regno]

Move the register write to MTK_DP_AUX_P0_3690 to set the AUX reply mode
to function mtk_dp_initialize_aux_settings(), as this is effectively
part of the DPTX AUX setup sequence.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 62d53c4b3feb..a67143c22024 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1012,6 +1012,11 @@ static void mtk_dp_initialize_aux_settings(struct mtk_dp *mtk_dp)
 	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_37C8,
 			   MTK_ATOP_EN_AUX_TX_P0,
 			   MTK_ATOP_EN_AUX_TX_P0);
+
+	/* Set complete reply mode for AUX */
+	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3690,
+			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0,
+			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0);
 }
 
 static void mtk_dp_initialize_digital_settings(struct mtk_dp *mtk_dp)
@@ -1824,10 +1829,6 @@ static void mtk_dp_init_port(struct mtk_dp *mtk_dp)
 	mtk_dp_initialize_settings(mtk_dp);
 	mtk_dp_initialize_aux_settings(mtk_dp);
 	mtk_dp_initialize_digital_settings(mtk_dp);
-
-	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3690,
-			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0,
-			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0);
 	mtk_dp_initialize_hpd_detect_settings(mtk_dp);
 
 	mtk_dp_digital_sw_reset(mtk_dp);
-- 
2.40.0

[PATCH v3 8/9] drm/mediatek: dp: Move AUX_P0 setting to mtk_dp_initialize_aux_settings()

[AngeloGioacchino Del Regno]

Move the register write to MTK_DP_AUX_P0_3690 to set the AUX reply mode
to function mtk_dp_initialize_aux_settings(), as this is effectively
part of the DPTX AUX setup sequence.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index 62d53c4b3feb..a67143c22024 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -1012,6 +1012,11 @@ static void mtk_dp_initialize_aux_settings(struct mtk_dp *mtk_dp)
 	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_37C8,
 			   MTK_ATOP_EN_AUX_TX_P0,
 			   MTK_ATOP_EN_AUX_TX_P0);
+
+	/* Set complete reply mode for AUX */
+	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3690,
+			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0,
+			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0);
 }
 
 static void mtk_dp_initialize_digital_settings(struct mtk_dp *mtk_dp)
@@ -1824,10 +1829,6 @@ static void mtk_dp_init_port(struct mtk_dp *mtk_dp)
 	mtk_dp_initialize_settings(mtk_dp);
 	mtk_dp_initialize_aux_settings(mtk_dp);
 	mtk_dp_initialize_digital_settings(mtk_dp);
-
-	mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3690,
-			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0,
-			   RX_REPLY_COMPLETE_MODE_AUX_TX_P0);
 	mtk_dp_initialize_hpd_detect_settings(mtk_dp);
 
 	mtk_dp_digital_sw_reset(mtk_dp);
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[AngeloGioacchino Del Regno]

For the eDP case we can support using aux-bus on MediaTek DP: this
gives us the possibility to declare our panel as generic "panel-edp"
which will automatically configure the timings and available modes
via the EDID that we read from it.

To do this, move the panel parsing at the end of the probe function
so that the hardware is initialized beforehand and also initialize
the DPTX AUX block and power both on as, when we populate the
aux-bus, the panel driver will trigger an EDID read to perform
panel detection.

Last but not least, since now the AUX transfers can happen in the
separated aux-bus, it was necessary to add an exclusion for the
cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
way to do this is to simply ignore checking that when the bridge
type is eDP.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index a67143c22024..8109f5b4392b 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2022 BayLibre
  */
 
+#include <drm/display/drm_dp_aux_bus.h>
 #include <drm/display/drm_dp.h>
 #include <drm/display/drm_dp_helper.h>
 #include <drm/drm_atomic_helper.h>
@@ -2042,7 +2043,8 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 
 	mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
 
-	if (!mtk_dp->train_info.cable_plugged_in) {
+	if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP &&
+	    !mtk_dp->train_info.cable_plugged_in) {
 		ret = -EAGAIN;
 		goto err;
 	}
@@ -2154,6 +2156,11 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
 	enable_irq(mtk_dp->irq);
 	mtk_dp_hwirq_enable(mtk_dp, true);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		mtk_dp->train_info.cable_plugged_in = true;
+		drm_helper_hpd_irq_event(mtk_dp->drm_dev);
+	}
+
 	return 0;
 
 err_bridge_attach:
@@ -2483,6 +2490,20 @@ static int mtk_dp_register_audio_driver(struct device *dev)
 	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);
 }
 
+static int mtk_dp_edp_link_panel(struct drm_dp_aux *mtk_aux)
+{
+	struct mtk_dp *mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
+	struct device *dev = mtk_aux->dev;
+	struct drm_bridge *panel_aux_bridge;
+
+	panel_aux_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	if (IS_ERR(panel_aux_bridge))
+		return PTR_ERR(panel_aux_bridge);
+
+	mtk_dp->next_bridge = panel_aux_bridge;
+	return 0;
+}
+
 static int mtk_dp_probe(struct platform_device *pdev)
 {
 	struct mtk_dp *mtk_dp;
@@ -2501,21 +2522,14 @@ static int mtk_dp_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, mtk_dp->irq,
 				     "failed to request dp irq resource\n");
 
-	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
-	if (IS_ERR(mtk_dp->next_bridge) &&
-	    PTR_ERR(mtk_dp->next_bridge) == -ENODEV)
-		mtk_dp->next_bridge = NULL;
-	else if (IS_ERR(mtk_dp->next_bridge))
-		return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge),
-				     "Failed to get bridge\n");
-
 	ret = mtk_dp_dt_parse(mtk_dp, pdev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to parse dt\n");
 
-	drm_dp_aux_init(&mtk_dp->aux);
 	mtk_dp->aux.name = "aux_mtk_dp";
+	mtk_dp->aux.dev = dev;
 	mtk_dp->aux.transfer = mtk_dp_aux_transfer;
+	drm_dp_aux_init(&mtk_dp->aux);
 
 	spin_lock_init(&mtk_dp->irq_thread_lock);
 
@@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
 	mtk_dp->need_debounce = true;
 	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		/* Initialize, reset and poweron the DPTX AUX block */
+		mtk_dp_initialize_aux_settings(mtk_dp);
+		mtk_dp_power_enable(mtk_dp);
+
+		/* Power on the panel to allow EDID read from aux-bus */
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
+
+		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);
+
+		/* If the panel is present, detection is done: power off! */
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
+		mtk_dp_power_disable(mtk_dp);
+
+		/* We ignore -ENODEV error, as the panel may not be on aux-bus */
+		if (ret && ret != -ENODEV)
+			return ret;
+
+		/*
+		 * Here we don't ignore any error, as if there's no panel to
+		 * link, eDP is not configured correctly and will be unusable.
+		 */
+		ret = mtk_dp_edp_link_panel(&mtk_dp->aux);
+		if (ret)
+			return ret;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
-- 
2.40.0

[PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[AngeloGioacchino Del Regno]

For the eDP case we can support using aux-bus on MediaTek DP: this
gives us the possibility to declare our panel as generic "panel-edp"
which will automatically configure the timings and available modes
via the EDID that we read from it.

To do this, move the panel parsing at the end of the probe function
so that the hardware is initialized beforehand and also initialize
the DPTX AUX block and power both on as, when we populate the
aux-bus, the panel driver will trigger an EDID read to perform
panel detection.

Last but not least, since now the AUX transfers can happen in the
separated aux-bus, it was necessary to add an exclusion for the
cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
way to do this is to simply ignore checking that when the bridge
type is eDP.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index a67143c22024..8109f5b4392b 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2022 BayLibre
  */
 
+#include <drm/display/drm_dp_aux_bus.h>
 #include <drm/display/drm_dp.h>
 #include <drm/display/drm_dp_helper.h>
 #include <drm/drm_atomic_helper.h>
@@ -2042,7 +2043,8 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 
 	mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
 
-	if (!mtk_dp->train_info.cable_plugged_in) {
+	if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP &&
+	    !mtk_dp->train_info.cable_plugged_in) {
 		ret = -EAGAIN;
 		goto err;
 	}
@@ -2154,6 +2156,11 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
 	enable_irq(mtk_dp->irq);
 	mtk_dp_hwirq_enable(mtk_dp, true);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		mtk_dp->train_info.cable_plugged_in = true;
+		drm_helper_hpd_irq_event(mtk_dp->drm_dev);
+	}
+
 	return 0;
 
 err_bridge_attach:
@@ -2483,6 +2490,20 @@ static int mtk_dp_register_audio_driver(struct device *dev)
 	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);
 }
 
+static int mtk_dp_edp_link_panel(struct drm_dp_aux *mtk_aux)
+{
+	struct mtk_dp *mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
+	struct device *dev = mtk_aux->dev;
+	struct drm_bridge *panel_aux_bridge;
+
+	panel_aux_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	if (IS_ERR(panel_aux_bridge))
+		return PTR_ERR(panel_aux_bridge);
+
+	mtk_dp->next_bridge = panel_aux_bridge;
+	return 0;
+}
+
 static int mtk_dp_probe(struct platform_device *pdev)
 {
 	struct mtk_dp *mtk_dp;
@@ -2501,21 +2522,14 @@ static int mtk_dp_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, mtk_dp->irq,
 				     "failed to request dp irq resource\n");
 
-	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
-	if (IS_ERR(mtk_dp->next_bridge) &&
-	    PTR_ERR(mtk_dp->next_bridge) == -ENODEV)
-		mtk_dp->next_bridge = NULL;
-	else if (IS_ERR(mtk_dp->next_bridge))
-		return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge),
-				     "Failed to get bridge\n");
-
 	ret = mtk_dp_dt_parse(mtk_dp, pdev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to parse dt\n");
 
-	drm_dp_aux_init(&mtk_dp->aux);
 	mtk_dp->aux.name = "aux_mtk_dp";
+	mtk_dp->aux.dev = dev;
 	mtk_dp->aux.transfer = mtk_dp_aux_transfer;
+	drm_dp_aux_init(&mtk_dp->aux);
 
 	spin_lock_init(&mtk_dp->irq_thread_lock);
 
@@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
 	mtk_dp->need_debounce = true;
 	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		/* Initialize, reset and poweron the DPTX AUX block */
+		mtk_dp_initialize_aux_settings(mtk_dp);
+		mtk_dp_power_enable(mtk_dp);
+
+		/* Power on the panel to allow EDID read from aux-bus */
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
+
+		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);
+
+		/* If the panel is present, detection is done: power off! */
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
+		mtk_dp_power_disable(mtk_dp);
+
+		/* We ignore -ENODEV error, as the panel may not be on aux-bus */
+		if (ret && ret != -ENODEV)
+			return ret;
+
+		/*
+		 * Here we don't ignore any error, as if there's no panel to
+		 * link, eDP is not configured correctly and will be unusable.
+		 */
+		ret = mtk_dp_edp_link_panel(&mtk_dp->aux);
+		if (ret)
+			return ret;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
-- 
2.40.0

[PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[AngeloGioacchino Del Regno]

For the eDP case we can support using aux-bus on MediaTek DP: this
gives us the possibility to declare our panel as generic "panel-edp"
which will automatically configure the timings and available modes
via the EDID that we read from it.

To do this, move the panel parsing at the end of the probe function
so that the hardware is initialized beforehand and also initialize
the DPTX AUX block and power both on as, when we populate the
aux-bus, the panel driver will trigger an EDID read to perform
panel detection.

Last but not least, since now the AUX transfers can happen in the
separated aux-bus, it was necessary to add an exclusion for the
cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
way to do this is to simply ignore checking that when the bridge
type is eDP.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index a67143c22024..8109f5b4392b 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2022 BayLibre
  */
 
+#include <drm/display/drm_dp_aux_bus.h>
 #include <drm/display/drm_dp.h>
 #include <drm/display/drm_dp_helper.h>
 #include <drm/drm_atomic_helper.h>
@@ -2042,7 +2043,8 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
 
 	mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
 
-	if (!mtk_dp->train_info.cable_plugged_in) {
+	if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP &&
+	    !mtk_dp->train_info.cable_plugged_in) {
 		ret = -EAGAIN;
 		goto err;
 	}
@@ -2154,6 +2156,11 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
 	enable_irq(mtk_dp->irq);
 	mtk_dp_hwirq_enable(mtk_dp, true);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		mtk_dp->train_info.cable_plugged_in = true;
+		drm_helper_hpd_irq_event(mtk_dp->drm_dev);
+	}
+
 	return 0;
 
 err_bridge_attach:
@@ -2483,6 +2490,20 @@ static int mtk_dp_register_audio_driver(struct device *dev)
 	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);
 }
 
+static int mtk_dp_edp_link_panel(struct drm_dp_aux *mtk_aux)
+{
+	struct mtk_dp *mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
+	struct device *dev = mtk_aux->dev;
+	struct drm_bridge *panel_aux_bridge;
+
+	panel_aux_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	if (IS_ERR(panel_aux_bridge))
+		return PTR_ERR(panel_aux_bridge);
+
+	mtk_dp->next_bridge = panel_aux_bridge;
+	return 0;
+}
+
 static int mtk_dp_probe(struct platform_device *pdev)
 {
 	struct mtk_dp *mtk_dp;
@@ -2501,21 +2522,14 @@ static int mtk_dp_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, mtk_dp->irq,
 				     "failed to request dp irq resource\n");
 
-	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
-	if (IS_ERR(mtk_dp->next_bridge) &&
-	    PTR_ERR(mtk_dp->next_bridge) == -ENODEV)
-		mtk_dp->next_bridge = NULL;
-	else if (IS_ERR(mtk_dp->next_bridge))
-		return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge),
-				     "Failed to get bridge\n");
-
 	ret = mtk_dp_dt_parse(mtk_dp, pdev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to parse dt\n");
 
-	drm_dp_aux_init(&mtk_dp->aux);
 	mtk_dp->aux.name = "aux_mtk_dp";
+	mtk_dp->aux.dev = dev;
 	mtk_dp->aux.transfer = mtk_dp_aux_transfer;
+	drm_dp_aux_init(&mtk_dp->aux);
 
 	spin_lock_init(&mtk_dp->irq_thread_lock);
 
@@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
 	mtk_dp->need_debounce = true;
 	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
 
+	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
+		/* Initialize, reset and poweron the DPTX AUX block */
+		mtk_dp_initialize_aux_settings(mtk_dp);
+		mtk_dp_power_enable(mtk_dp);
+
+		/* Power on the panel to allow EDID read from aux-bus */
+		mtk_dp_aux_panel_poweron(mtk_dp, true);
+
+		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);
+
+		/* If the panel is present, detection is done: power off! */
+		mtk_dp_aux_panel_poweron(mtk_dp, false);
+		mtk_dp_power_disable(mtk_dp);
+
+		/* We ignore -ENODEV error, as the panel may not be on aux-bus */
+		if (ret && ret != -ENODEV)
+			return ret;
+
+		/*
+		 * Here we don't ignore any error, as if there's no panel to
+		 * link, eDP is not configured correctly and will be unusable.
+		 */
+		ret = mtk_dp_edp_link_panel(&mtk_dp->aux);
+		if (ret)
+			return ret;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/9] drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Change logging from drm_{err,info}() to dev_{err,info}() in functions
> mtk_dp_aux_transfer() and mtk_dp_aux_do_transfer(): this will be
> essential to avoid getting NULL pointer kernel panics if any kind
> of error happens during AUX transfers happening before the bridge
> is attached.
>
> This may potentially start happening in a later commit implementing
> aux-bus support, as AUX transfers will be triggered from the panel
> driver (for EDID) before the mtk-dp bridge gets attached, and it's
> done in preparation for the same.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index ac21eca0e20e..19b145cf2e05 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -849,7 +849,7 @@ static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool is_read, u8 cmd,
>                 u32 phy_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3628) &
>                                  AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
>                 if (phy_status != AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
> -                       drm_err(mtk_dp->drm_dev,
> +                       dev_err(mtk_dp->dev,
>                                 "AUX Rx Aux hang, need SW reset\n");
>                         return -EIO;
>                 }
> @@ -2061,7 +2061,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                 is_read = true;
>                 break;
>         default:
> -               drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
> +               dev_err(mtk_dp->dev, "invalid aux cmd = %d\n",
>                         msg->request);
>                 ret = -EINVAL;
>                 goto err;
> @@ -2077,7 +2077,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                                              to_access);
>
>                 if (ret) {
> -                       drm_info(mtk_dp->drm_dev,
> +                       dev_info(mtk_dp->dev,
>                                  "Failed to do AUX transfer: %d\n", ret);

This part no longer applies cleanly due to

    drm/mediatek: dp: Change the aux retries times when receiving AUX_DEFER

Also I think all these calls could be collapsed into one line?

ChenYu

>                         goto err;
>                 }
> --
> 2.40.0
>

Re: [PATCH v3 5/9] drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Change logging from drm_{err,info}() to dev_{err,info}() in functions
> mtk_dp_aux_transfer() and mtk_dp_aux_do_transfer(): this will be
> essential to avoid getting NULL pointer kernel panics if any kind
> of error happens during AUX transfers happening before the bridge
> is attached.
>
> This may potentially start happening in a later commit implementing
> aux-bus support, as AUX transfers will be triggered from the panel
> driver (for EDID) before the mtk-dp bridge gets attached, and it's
> done in preparation for the same.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index ac21eca0e20e..19b145cf2e05 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -849,7 +849,7 @@ static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool is_read, u8 cmd,
>                 u32 phy_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3628) &
>                                  AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
>                 if (phy_status != AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
> -                       drm_err(mtk_dp->drm_dev,
> +                       dev_err(mtk_dp->dev,
>                                 "AUX Rx Aux hang, need SW reset\n");
>                         return -EIO;
>                 }
> @@ -2061,7 +2061,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                 is_read = true;
>                 break;
>         default:
> -               drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
> +               dev_err(mtk_dp->dev, "invalid aux cmd = %d\n",
>                         msg->request);
>                 ret = -EINVAL;
>                 goto err;
> @@ -2077,7 +2077,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                                              to_access);
>
>                 if (ret) {
> -                       drm_info(mtk_dp->drm_dev,
> +                       dev_info(mtk_dp->dev,
>                                  "Failed to do AUX transfer: %d\n", ret);

This part no longer applies cleanly due to

    drm/mediatek: dp: Change the aux retries times when receiving AUX_DEFER

Also I think all these calls could be collapsed into one line?

ChenYu

>                         goto err;
>                 }
> --
> 2.40.0
>

Re: [PATCH v3 5/9] drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Change logging from drm_{err,info}() to dev_{err,info}() in functions
> mtk_dp_aux_transfer() and mtk_dp_aux_do_transfer(): this will be
> essential to avoid getting NULL pointer kernel panics if any kind
> of error happens during AUX transfers happening before the bridge
> is attached.
>
> This may potentially start happening in a later commit implementing
> aux-bus support, as AUX transfers will be triggered from the panel
> driver (for EDID) before the mtk-dp bridge gets attached, and it's
> done in preparation for the same.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index ac21eca0e20e..19b145cf2e05 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -849,7 +849,7 @@ static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool is_read, u8 cmd,
>                 u32 phy_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3628) &
>                                  AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
>                 if (phy_status != AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
> -                       drm_err(mtk_dp->drm_dev,
> +                       dev_err(mtk_dp->dev,
>                                 "AUX Rx Aux hang, need SW reset\n");
>                         return -EIO;
>                 }
> @@ -2061,7 +2061,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                 is_read = true;
>                 break;
>         default:
> -               drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
> +               dev_err(mtk_dp->dev, "invalid aux cmd = %d\n",
>                         msg->request);
>                 ret = -EINVAL;
>                 goto err;
> @@ -2077,7 +2077,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                                              to_access);
>
>                 if (ret) {
> -                       drm_info(mtk_dp->drm_dev,
> +                       dev_info(mtk_dp->dev,
>                                  "Failed to do AUX transfer: %d\n", ret);

This part no longer applies cleanly due to

    drm/mediatek: dp: Change the aux retries times when receiving AUX_DEFER

Also I think all these calls could be collapsed into one line?

ChenYu

>                         goto err;
>                 }
> --
> 2.40.0
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Changes in v3:
>  - Added DPTX AUX block initialization before trying to communicate
>    to stop relying on the bootloader keeping it initialized before
>    booting Linux.
>  - Fixed commit description for patch [09/09] and removed commented
>    out code (that slipped from dev phase.. sorry!).
>
> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
>
> Explaining the "real":
> Before this change, the DisplayPort driver did support eDP to some
> extent, but it was treating it entirely like a regular DP interface
> which is partially fine, after all, embedded DisplayPort *is* actually
> DisplayPort, but there might be some differences to account for... and
> this is for both small performance improvements and, more importantly,
> for correct functionality in some systems.
>
> Functionality first:
>
> One of the common differences found in various boards implementing eDP
> and machines using an eDP panel is that many times the HPD line is not
> connected. This *must* be accounted for: at startup, this specific IP
> will raise a HPD interrupt (which should maybe be ignored... as it does
> not appear to be a "real" event...) that will make the eDP panel to be
> detected and to actually work but, after a suspend-resume cycle, there
> will be no HPD interrupt (as there's no HPD line in my case!) producing
> a functionality issue - specifically, the DP Link Training fails because
> the panel doesn't get powered up, then it stays black and won't work
> until rebooting the machine (or removing and reinserting the module I
> think, but I haven't tried that).
>
> Now for.. both:
> eDP panels are *e*DP because they are *not* removable (in the sense that
> you can't unplug the cable without disassembling the machine, in which
> case, the machine shall be powered down..!): this (correct) assumption
> makes us able to solve some issues and to also gain a little performance
> during PM operations.
>
> What was done here is:
>  - Caching the EDID if the panel is eDP: we're always going to read the
>    same data everytime, so we can just cache that (as it's small enough)
>    shortening PM resume times for the eDP driver instance;
>  - Always return connector_status_connected if it's eDP: non-removable
>    means connector_status_disconnected can't happen during runtime...
>    this also saves us some time and even power, as we won't have to
>    perform yet another power cycle of the HW;
>  - Added aux-bus support!
>    This makes us able to rely on panel autodetection from the EDID,
>    avoiding to add more and more panel timings to panel-edp and, even
>    better, allowing to use one panel node in devicetrees for multiple
>    variants of the same machine since, at that point, it's not important
>    to "preventively know" what panel we have (eh, it's autodetected...!).
>
> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
>
>
> P.S.: For your own testing commodity, here's a reference devicetree:
> &edp_tx {
>         status = "okay";
>
>         pinctrl-names = "default";
>         pinctrl-0 = <&edptx_pins_default>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>                         edp_in: endpoint {
>                                 remote-endpoint = <&dp_intf0_out>;
>                         };
>                 };
>
>                 port@1 {
>                         reg = <1>;
>                         edp_out: endpoint {
>                                 data-lanes = <0 1 2 3>;
>                                 remote-endpoint = <&panel_in>;
>                         };
>                 };
>         };
>
>         aux-bus {
>                 panel: panel {
>                         compatible = "edp-panel";
>                         power-supply = <&pp3300_disp_x>;
>                         backlight = <&backlight_lcd0>;
>                         port {
>                                 panel_in: endpoint {
>                                         remote-endpoint = <&edp_out>;
>                                 };
>                         };
>                 };
>         };
> };
>
> AngeloGioacchino Del Regno (9):
>   drm/mediatek: dp: Cache EDID for eDP panel
>   drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>   drm/mediatek: dp: Always return connected status for eDP in .detect()
>   drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>   drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>   drm/mediatek: dp: Enable event interrupt only when bridge attached
>   drm/mediatek: dp: Use devm variant of drm_bridge_add()
>   drm/mediatek: dp: Move AUX_P0 setting to
>     mtk_dp_initialize_aux_settings()
>   drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

Tested-by: Chen-Yu Tsai <wenst@chromium.org>

on MT8195 Tomato: eDP panel works if the display panel regulator is always
on. External DP works.

Maybe it has something to do with the driver not supporting .wait_hpd_asserted
and not using a GPIO for HPD?

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Changes in v3:
>  - Added DPTX AUX block initialization before trying to communicate
>    to stop relying on the bootloader keeping it initialized before
>    booting Linux.
>  - Fixed commit description for patch [09/09] and removed commented
>    out code (that slipped from dev phase.. sorry!).
>
> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
>
> Explaining the "real":
> Before this change, the DisplayPort driver did support eDP to some
> extent, but it was treating it entirely like a regular DP interface
> which is partially fine, after all, embedded DisplayPort *is* actually
> DisplayPort, but there might be some differences to account for... and
> this is for both small performance improvements and, more importantly,
> for correct functionality in some systems.
>
> Functionality first:
>
> One of the common differences found in various boards implementing eDP
> and machines using an eDP panel is that many times the HPD line is not
> connected. This *must* be accounted for: at startup, this specific IP
> will raise a HPD interrupt (which should maybe be ignored... as it does
> not appear to be a "real" event...) that will make the eDP panel to be
> detected and to actually work but, after a suspend-resume cycle, there
> will be no HPD interrupt (as there's no HPD line in my case!) producing
> a functionality issue - specifically, the DP Link Training fails because
> the panel doesn't get powered up, then it stays black and won't work
> until rebooting the machine (or removing and reinserting the module I
> think, but I haven't tried that).
>
> Now for.. both:
> eDP panels are *e*DP because they are *not* removable (in the sense that
> you can't unplug the cable without disassembling the machine, in which
> case, the machine shall be powered down..!): this (correct) assumption
> makes us able to solve some issues and to also gain a little performance
> during PM operations.
>
> What was done here is:
>  - Caching the EDID if the panel is eDP: we're always going to read the
>    same data everytime, so we can just cache that (as it's small enough)
>    shortening PM resume times for the eDP driver instance;
>  - Always return connector_status_connected if it's eDP: non-removable
>    means connector_status_disconnected can't happen during runtime...
>    this also saves us some time and even power, as we won't have to
>    perform yet another power cycle of the HW;
>  - Added aux-bus support!
>    This makes us able to rely on panel autodetection from the EDID,
>    avoiding to add more and more panel timings to panel-edp and, even
>    better, allowing to use one panel node in devicetrees for multiple
>    variants of the same machine since, at that point, it's not important
>    to "preventively know" what panel we have (eh, it's autodetected...!).
>
> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
>
>
> P.S.: For your own testing commodity, here's a reference devicetree:
> &edp_tx {
>         status = "okay";
>
>         pinctrl-names = "default";
>         pinctrl-0 = <&edptx_pins_default>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>                         edp_in: endpoint {
>                                 remote-endpoint = <&dp_intf0_out>;
>                         };
>                 };
>
>                 port@1 {
>                         reg = <1>;
>                         edp_out: endpoint {
>                                 data-lanes = <0 1 2 3>;
>                                 remote-endpoint = <&panel_in>;
>                         };
>                 };
>         };
>
>         aux-bus {
>                 panel: panel {
>                         compatible = "edp-panel";
>                         power-supply = <&pp3300_disp_x>;
>                         backlight = <&backlight_lcd0>;
>                         port {
>                                 panel_in: endpoint {
>                                         remote-endpoint = <&edp_out>;
>                                 };
>                         };
>                 };
>         };
> };
>
> AngeloGioacchino Del Regno (9):
>   drm/mediatek: dp: Cache EDID for eDP panel
>   drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>   drm/mediatek: dp: Always return connected status for eDP in .detect()
>   drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>   drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>   drm/mediatek: dp: Enable event interrupt only when bridge attached
>   drm/mediatek: dp: Use devm variant of drm_bridge_add()
>   drm/mediatek: dp: Move AUX_P0 setting to
>     mtk_dp_initialize_aux_settings()
>   drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

Tested-by: Chen-Yu Tsai <wenst@chromium.org>

on MT8195 Tomato: eDP panel works if the display panel regulator is always
on. External DP works.

Maybe it has something to do with the driver not supporting .wait_hpd_asserted
and not using a GPIO for HPD?

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Changes in v3:
>  - Added DPTX AUX block initialization before trying to communicate
>    to stop relying on the bootloader keeping it initialized before
>    booting Linux.
>  - Fixed commit description for patch [09/09] and removed commented
>    out code (that slipped from dev phase.. sorry!).
>
> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
>
> Explaining the "real":
> Before this change, the DisplayPort driver did support eDP to some
> extent, but it was treating it entirely like a regular DP interface
> which is partially fine, after all, embedded DisplayPort *is* actually
> DisplayPort, but there might be some differences to account for... and
> this is for both small performance improvements and, more importantly,
> for correct functionality in some systems.
>
> Functionality first:
>
> One of the common differences found in various boards implementing eDP
> and machines using an eDP panel is that many times the HPD line is not
> connected. This *must* be accounted for: at startup, this specific IP
> will raise a HPD interrupt (which should maybe be ignored... as it does
> not appear to be a "real" event...) that will make the eDP panel to be
> detected and to actually work but, after a suspend-resume cycle, there
> will be no HPD interrupt (as there's no HPD line in my case!) producing
> a functionality issue - specifically, the DP Link Training fails because
> the panel doesn't get powered up, then it stays black and won't work
> until rebooting the machine (or removing and reinserting the module I
> think, but I haven't tried that).
>
> Now for.. both:
> eDP panels are *e*DP because they are *not* removable (in the sense that
> you can't unplug the cable without disassembling the machine, in which
> case, the machine shall be powered down..!): this (correct) assumption
> makes us able to solve some issues and to also gain a little performance
> during PM operations.
>
> What was done here is:
>  - Caching the EDID if the panel is eDP: we're always going to read the
>    same data everytime, so we can just cache that (as it's small enough)
>    shortening PM resume times for the eDP driver instance;
>  - Always return connector_status_connected if it's eDP: non-removable
>    means connector_status_disconnected can't happen during runtime...
>    this also saves us some time and even power, as we won't have to
>    perform yet another power cycle of the HW;
>  - Added aux-bus support!
>    This makes us able to rely on panel autodetection from the EDID,
>    avoiding to add more and more panel timings to panel-edp and, even
>    better, allowing to use one panel node in devicetrees for multiple
>    variants of the same machine since, at that point, it's not important
>    to "preventively know" what panel we have (eh, it's autodetected...!).
>
> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
>
>
> P.S.: For your own testing commodity, here's a reference devicetree:
> &edp_tx {
>         status = "okay";
>
>         pinctrl-names = "default";
>         pinctrl-0 = <&edptx_pins_default>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>                         edp_in: endpoint {
>                                 remote-endpoint = <&dp_intf0_out>;
>                         };
>                 };
>
>                 port@1 {
>                         reg = <1>;
>                         edp_out: endpoint {
>                                 data-lanes = <0 1 2 3>;
>                                 remote-endpoint = <&panel_in>;
>                         };
>                 };
>         };
>
>         aux-bus {
>                 panel: panel {
>                         compatible = "edp-panel";
>                         power-supply = <&pp3300_disp_x>;
>                         backlight = <&backlight_lcd0>;
>                         port {
>                                 panel_in: endpoint {
>                                         remote-endpoint = <&edp_out>;
>                                 };
>                         };
>                 };
>         };
> };
>
> AngeloGioacchino Del Regno (9):
>   drm/mediatek: dp: Cache EDID for eDP panel
>   drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>   drm/mediatek: dp: Always return connected status for eDP in .detect()
>   drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>   drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>   drm/mediatek: dp: Enable event interrupt only when bridge attached
>   drm/mediatek: dp: Use devm variant of drm_bridge_add()
>   drm/mediatek: dp: Move AUX_P0 setting to
>     mtk_dp_initialize_aux_settings()
>   drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

Tested-by: Chen-Yu Tsai <wenst@chromium.org>

on MT8195 Tomato: eDP panel works if the display panel regulator is always
on. External DP works.

Maybe it has something to do with the driver not supporting .wait_hpd_asserted
and not using a GPIO for HPD?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Everytime we run bridge detection and/or EDID read we run a poweron
> and poweroff sequence for both the AUX and the panel; moreover, this
> is also done when enabling the bridge in the .atomic_enable() callback.
>
> Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
> function as to commonize it.
> Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
> the AUX was getting powered on but the panel was left powered off if
> the DP cable wasn't plugged in while now we unconditionally send a D0
> request and this is done for two reasons:
>  - First, whether this request fails or not, it takes the same time
>    and anyway the DP hardware won't produce any error (or, if it
>    does, it's ignorable because it won't block further commands)
>  - Second, training the link between a sleeping/standby/unpowered
>    display makes little sense.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
>  1 file changed, 30 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 84f82cc68672..76ea94167531 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
>                            val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
>  }
>
> +static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
> +{
> +       if (pwron) {
> +               /* power on aux */
> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> +                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> +                                  DP_PWR_STATE_MASK);
> +
> +               /* power on panel */
> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> +               usleep_range(2000, 5000);
> +       } else {
> +               /* power off panel */
> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> +               usleep_range(2000, 3000);
> +
> +               /* power off aux */
> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> +                                  DP_PWR_STATE_BANDGAP_TPLL,
> +                                  DP_PWR_STATE_MASK);
> +       }
> +}
> +
>  static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>  {
>         mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
> @@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>         if (!mtk_dp->train_info.cable_plugged_in)
>                 return ret;
>
> -       if (!enabled) {
> -               /* power on aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                                  DP_PWR_STATE_MASK);
> +       if (!enabled)
> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>
> -               /* power on panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);

I suspect the original code was somewhat wrong already? We shouldn't need
to pull the panel out of standby just for HPD or reading EDID.

This driver probably needs a lot more cleanup. :/

ChenYu

> -               usleep_range(2000, 5000);
> -       }
>         /*
>          * Some dongles still source HPD when they do not connect to any
>          * sink device. To avoid this, we need to read the sink count
> @@ -1958,16 +1974,8 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>         if (DP_GET_SINK_COUNT(sink_count))
>                 ret = connector_status_connected;
>
> -       if (!enabled) {
> -               /* power off panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> -               usleep_range(2000, 3000);
> -
> -               /* power off aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL,
> -                                  DP_PWR_STATE_MASK);
> -       }
> +       if (!enabled)
> +               mtk_dp_aux_panel_poweron(mtk_dp, false);
>
>         return ret;
>  }
> @@ -1983,15 +1991,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>
>         if (!enabled) {
>                 drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> -
> -               /* power on aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                                  DP_PWR_STATE_MASK);
> -
> -               /* power on panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -               usleep_range(2000, 5000);
> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>         }
>
>         /* eDP panels aren't removable, so we can return a cached EDID. */
> @@ -2015,15 +2015,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>         }
>
>         if (!enabled) {
> -               /* power off panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> -               usleep_range(2000, 3000);
> -
> -               /* power off aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL,
> -                                  DP_PWR_STATE_MASK);
> -
> +               mtk_dp_aux_panel_poweron(mtk_dp, false);
>                 drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>         }
>
> @@ -2188,15 +2180,7 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>                 return;
>         }
>
> -       /* power on aux */
> -       mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                          DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                          DP_PWR_STATE_MASK);
> -
> -       if (mtk_dp->train_info.cable_plugged_in) {
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -               usleep_range(2000, 5000);
> -       }
> +       mtk_dp_aux_panel_poweron(mtk_dp, true);
>
>         /* Training */
>         ret = mtk_dp_training(mtk_dp);
> --
> 2.40.0
>

Re: [PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Everytime we run bridge detection and/or EDID read we run a poweron
> and poweroff sequence for both the AUX and the panel; moreover, this
> is also done when enabling the bridge in the .atomic_enable() callback.
>
> Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
> function as to commonize it.
> Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
> the AUX was getting powered on but the panel was left powered off if
> the DP cable wasn't plugged in while now we unconditionally send a D0
> request and this is done for two reasons:
>  - First, whether this request fails or not, it takes the same time
>    and anyway the DP hardware won't produce any error (or, if it
>    does, it's ignorable because it won't block further commands)
>  - Second, training the link between a sleeping/standby/unpowered
>    display makes little sense.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
>  1 file changed, 30 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 84f82cc68672..76ea94167531 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
>                            val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
>  }
>
> +static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
> +{
> +       if (pwron) {
> +               /* power on aux */
> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> +                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> +                                  DP_PWR_STATE_MASK);
> +
> +               /* power on panel */
> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> +               usleep_range(2000, 5000);
> +       } else {
> +               /* power off panel */
> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> +               usleep_range(2000, 3000);
> +
> +               /* power off aux */
> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> +                                  DP_PWR_STATE_BANDGAP_TPLL,
> +                                  DP_PWR_STATE_MASK);
> +       }
> +}
> +
>  static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>  {
>         mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
> @@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>         if (!mtk_dp->train_info.cable_plugged_in)
>                 return ret;
>
> -       if (!enabled) {
> -               /* power on aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                                  DP_PWR_STATE_MASK);
> +       if (!enabled)
> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>
> -               /* power on panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);

I suspect the original code was somewhat wrong already? We shouldn't need
to pull the panel out of standby just for HPD or reading EDID.

This driver probably needs a lot more cleanup. :/

ChenYu

> -               usleep_range(2000, 5000);
> -       }
>         /*
>          * Some dongles still source HPD when they do not connect to any
>          * sink device. To avoid this, we need to read the sink count
> @@ -1958,16 +1974,8 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>         if (DP_GET_SINK_COUNT(sink_count))
>                 ret = connector_status_connected;
>
> -       if (!enabled) {
> -               /* power off panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> -               usleep_range(2000, 3000);
> -
> -               /* power off aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL,
> -                                  DP_PWR_STATE_MASK);
> -       }
> +       if (!enabled)
> +               mtk_dp_aux_panel_poweron(mtk_dp, false);
>
>         return ret;
>  }
> @@ -1983,15 +1991,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>
>         if (!enabled) {
>                 drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> -
> -               /* power on aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                                  DP_PWR_STATE_MASK);
> -
> -               /* power on panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -               usleep_range(2000, 5000);
> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>         }
>
>         /* eDP panels aren't removable, so we can return a cached EDID. */
> @@ -2015,15 +2015,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>         }
>
>         if (!enabled) {
> -               /* power off panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> -               usleep_range(2000, 3000);
> -
> -               /* power off aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL,
> -                                  DP_PWR_STATE_MASK);
> -
> +               mtk_dp_aux_panel_poweron(mtk_dp, false);
>                 drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>         }
>
> @@ -2188,15 +2180,7 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>                 return;
>         }
>
> -       /* power on aux */
> -       mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                          DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                          DP_PWR_STATE_MASK);
> -
> -       if (mtk_dp->train_info.cable_plugged_in) {
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -               usleep_range(2000, 5000);
> -       }
> +       mtk_dp_aux_panel_poweron(mtk_dp, true);
>
>         /* Training */
>         ret = mtk_dp_training(mtk_dp);
> --
> 2.40.0
>

Re: [PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[Chen-Yu Tsai]

On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Everytime we run bridge detection and/or EDID read we run a poweron
> and poweroff sequence for both the AUX and the panel; moreover, this
> is also done when enabling the bridge in the .atomic_enable() callback.
>
> Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
> function as to commonize it.
> Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
> the AUX was getting powered on but the panel was left powered off if
> the DP cable wasn't plugged in while now we unconditionally send a D0
> request and this is done for two reasons:
>  - First, whether this request fails or not, it takes the same time
>    and anyway the DP hardware won't produce any error (or, if it
>    does, it's ignorable because it won't block further commands)
>  - Second, training the link between a sleeping/standby/unpowered
>    display makes little sense.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
>  1 file changed, 30 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 84f82cc68672..76ea94167531 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
>                            val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
>  }
>
> +static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
> +{
> +       if (pwron) {
> +               /* power on aux */
> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> +                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> +                                  DP_PWR_STATE_MASK);
> +
> +               /* power on panel */
> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> +               usleep_range(2000, 5000);
> +       } else {
> +               /* power off panel */
> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> +               usleep_range(2000, 3000);
> +
> +               /* power off aux */
> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> +                                  DP_PWR_STATE_BANDGAP_TPLL,
> +                                  DP_PWR_STATE_MASK);
> +       }
> +}
> +
>  static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>  {
>         mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
> @@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>         if (!mtk_dp->train_info.cable_plugged_in)
>                 return ret;
>
> -       if (!enabled) {
> -               /* power on aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                                  DP_PWR_STATE_MASK);
> +       if (!enabled)
> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>
> -               /* power on panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);

I suspect the original code was somewhat wrong already? We shouldn't need
to pull the panel out of standby just for HPD or reading EDID.

This driver probably needs a lot more cleanup. :/

ChenYu

> -               usleep_range(2000, 5000);
> -       }
>         /*
>          * Some dongles still source HPD when they do not connect to any
>          * sink device. To avoid this, we need to read the sink count
> @@ -1958,16 +1974,8 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>         if (DP_GET_SINK_COUNT(sink_count))
>                 ret = connector_status_connected;
>
> -       if (!enabled) {
> -               /* power off panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> -               usleep_range(2000, 3000);
> -
> -               /* power off aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL,
> -                                  DP_PWR_STATE_MASK);
> -       }
> +       if (!enabled)
> +               mtk_dp_aux_panel_poweron(mtk_dp, false);
>
>         return ret;
>  }
> @@ -1983,15 +1991,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>
>         if (!enabled) {
>                 drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> -
> -               /* power on aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                                  DP_PWR_STATE_MASK);
> -
> -               /* power on panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -               usleep_range(2000, 5000);
> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>         }
>
>         /* eDP panels aren't removable, so we can return a cached EDID. */
> @@ -2015,15 +2015,7 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>         }
>
>         if (!enabled) {
> -               /* power off panel */
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
> -               usleep_range(2000, 3000);
> -
> -               /* power off aux */
> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                                  DP_PWR_STATE_BANDGAP_TPLL,
> -                                  DP_PWR_STATE_MASK);
> -
> +               mtk_dp_aux_panel_poweron(mtk_dp, false);
>                 drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>         }
>
> @@ -2188,15 +2180,7 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>                 return;
>         }
>
> -       /* power on aux */
> -       mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
> -                          DP_PWR_STATE_BANDGAP_TPLL_LANE,
> -                          DP_PWR_STATE_MASK);
> -
> -       if (mtk_dp->train_info.cable_plugged_in) {
> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -               usleep_range(2000, 5000);
> -       }
> +       mtk_dp_aux_panel_poweron(mtk_dp, true);
>
>         /* Training */
>         ret = mtk_dp_training(mtk_dp);
> --
> 2.40.0
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Il 06/04/23 09:20, Chen-Yu Tsai ha scritto:
> On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Changes in v3:
>>   - Added DPTX AUX block initialization before trying to communicate
>>     to stop relying on the bootloader keeping it initialized before
>>     booting Linux.
>>   - Fixed commit description for patch [09/09] and removed commented
>>     out code (that slipped from dev phase.. sorry!).
>>
>> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
>>
>> Explaining the "real":
>> Before this change, the DisplayPort driver did support eDP to some
>> extent, but it was treating it entirely like a regular DP interface
>> which is partially fine, after all, embedded DisplayPort *is* actually
>> DisplayPort, but there might be some differences to account for... and
>> this is for both small performance improvements and, more importantly,
>> for correct functionality in some systems.
>>
>> Functionality first:
>>
>> One of the common differences found in various boards implementing eDP
>> and machines using an eDP panel is that many times the HPD line is not
>> connected. This *must* be accounted for: at startup, this specific IP
>> will raise a HPD interrupt (which should maybe be ignored... as it does
>> not appear to be a "real" event...) that will make the eDP panel to be
>> detected and to actually work but, after a suspend-resume cycle, there
>> will be no HPD interrupt (as there's no HPD line in my case!) producing
>> a functionality issue - specifically, the DP Link Training fails because
>> the panel doesn't get powered up, then it stays black and won't work
>> until rebooting the machine (or removing and reinserting the module I
>> think, but I haven't tried that).
>>
>> Now for.. both:
>> eDP panels are *e*DP because they are *not* removable (in the sense that
>> you can't unplug the cable without disassembling the machine, in which
>> case, the machine shall be powered down..!): this (correct) assumption
>> makes us able to solve some issues and to also gain a little performance
>> during PM operations.
>>
>> What was done here is:
>>   - Caching the EDID if the panel is eDP: we're always going to read the
>>     same data everytime, so we can just cache that (as it's small enough)
>>     shortening PM resume times for the eDP driver instance;
>>   - Always return connector_status_connected if it's eDP: non-removable
>>     means connector_status_disconnected can't happen during runtime...
>>     this also saves us some time and even power, as we won't have to
>>     perform yet another power cycle of the HW;
>>   - Added aux-bus support!
>>     This makes us able to rely on panel autodetection from the EDID,
>>     avoiding to add more and more panel timings to panel-edp and, even
>>     better, allowing to use one panel node in devicetrees for multiple
>>     variants of the same machine since, at that point, it's not important
>>     to "preventively know" what panel we have (eh, it's autodetected...!).
>>
>> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
>>
>>
>> P.S.: For your own testing commodity, here's a reference devicetree:
>> &edp_tx {
>>          status = "okay";
>>
>>          pinctrl-names = "default";
>>          pinctrl-0 = <&edptx_pins_default>;
>>
>>          ports {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>>                  port@0 {
>>                          reg = <0>;
>>                          edp_in: endpoint {
>>                                  remote-endpoint = <&dp_intf0_out>;
>>                          };
>>                  };
>>
>>                  port@1 {
>>                          reg = <1>;
>>                          edp_out: endpoint {
>>                                  data-lanes = <0 1 2 3>;
>>                                  remote-endpoint = <&panel_in>;
>>                          };
>>                  };
>>          };
>>
>>          aux-bus {
>>                  panel: panel {
>>                          compatible = "edp-panel";
>>                          power-supply = <&pp3300_disp_x>;
>>                          backlight = <&backlight_lcd0>;
>>                          port {
>>                                  panel_in: endpoint {
>>                                          remote-endpoint = <&edp_out>;
>>                                  };
>>                          };
>>                  };
>>          };
>> };
>>
>> AngeloGioacchino Del Regno (9):
>>    drm/mediatek: dp: Cache EDID for eDP panel
>>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>>    drm/mediatek: dp: Always return connected status for eDP in .detect()
>>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>>    drm/mediatek: dp: Enable event interrupt only when bridge attached
>>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
>>    drm/mediatek: dp: Move AUX_P0 setting to
>>      mtk_dp_initialize_aux_settings()
>>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> 
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> 
> on MT8195 Tomato: eDP panel works if the display panel regulator is always
> on. External DP works.
> 
> Maybe it has something to do with the driver not supporting .wait_hpd_asserted
> and not using a GPIO for HPD?

Even before this change I couldn't get the panel to reliably work without keeping 
the regulator always on (just to point out that I'm not introducing regressions).

I am already trying to understand why this happens... and I'm still researching...
but there's what I've seen for now:
  * Set the panel regulator as regulator-boot-on;
  * Boot: edp-panel will correctly read the EDID, then will run the PM suspend
    handler
  * mtk-dp's .get_edid() callback gets called but, at that time, edp-panel will
    still be suspended (PM resume handler didn't get called)
    * Regulator is still down
  * Failure.

That's not right and probably the .get_edid() callback in mtk-dp has an abuse:
there, mtk_dp_parse_capabilities() gets called, which performs initialization
of some variables for DP link training (essential to get the DP going!).

The question that I am making to myself is whether I should move that elsewhere,
if so, what's the right place (making me able to remove the DRM_BRIDGE_OP_EDID
bridge flag when eDP + aux-bus), or if I should leave that and make sure that the
panel-edp's resume callback is called before .get_edid() from mtk-dp gets called.

That can get done in a separated series (or single patch?)... so that if we get
this one picked sooner than later, we can start upstreaming the panel nodes in
the Cherry devicetrees and only remove the regulator-always-on later.

Thoughts?

Cheers,
Angelo

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Il 06/04/23 09:20, Chen-Yu Tsai ha scritto:
> On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Changes in v3:
>>   - Added DPTX AUX block initialization before trying to communicate
>>     to stop relying on the bootloader keeping it initialized before
>>     booting Linux.
>>   - Fixed commit description for patch [09/09] and removed commented
>>     out code (that slipped from dev phase.. sorry!).
>>
>> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
>>
>> Explaining the "real":
>> Before this change, the DisplayPort driver did support eDP to some
>> extent, but it was treating it entirely like a regular DP interface
>> which is partially fine, after all, embedded DisplayPort *is* actually
>> DisplayPort, but there might be some differences to account for... and
>> this is for both small performance improvements and, more importantly,
>> for correct functionality in some systems.
>>
>> Functionality first:
>>
>> One of the common differences found in various boards implementing eDP
>> and machines using an eDP panel is that many times the HPD line is not
>> connected. This *must* be accounted for: at startup, this specific IP
>> will raise a HPD interrupt (which should maybe be ignored... as it does
>> not appear to be a "real" event...) that will make the eDP panel to be
>> detected and to actually work but, after a suspend-resume cycle, there
>> will be no HPD interrupt (as there's no HPD line in my case!) producing
>> a functionality issue - specifically, the DP Link Training fails because
>> the panel doesn't get powered up, then it stays black and won't work
>> until rebooting the machine (or removing and reinserting the module I
>> think, but I haven't tried that).
>>
>> Now for.. both:
>> eDP panels are *e*DP because they are *not* removable (in the sense that
>> you can't unplug the cable without disassembling the machine, in which
>> case, the machine shall be powered down..!): this (correct) assumption
>> makes us able to solve some issues and to also gain a little performance
>> during PM operations.
>>
>> What was done here is:
>>   - Caching the EDID if the panel is eDP: we're always going to read the
>>     same data everytime, so we can just cache that (as it's small enough)
>>     shortening PM resume times for the eDP driver instance;
>>   - Always return connector_status_connected if it's eDP: non-removable
>>     means connector_status_disconnected can't happen during runtime...
>>     this also saves us some time and even power, as we won't have to
>>     perform yet another power cycle of the HW;
>>   - Added aux-bus support!
>>     This makes us able to rely on panel autodetection from the EDID,
>>     avoiding to add more and more panel timings to panel-edp and, even
>>     better, allowing to use one panel node in devicetrees for multiple
>>     variants of the same machine since, at that point, it's not important
>>     to "preventively know" what panel we have (eh, it's autodetected...!).
>>
>> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
>>
>>
>> P.S.: For your own testing commodity, here's a reference devicetree:
>> &edp_tx {
>>          status = "okay";
>>
>>          pinctrl-names = "default";
>>          pinctrl-0 = <&edptx_pins_default>;
>>
>>          ports {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>>                  port@0 {
>>                          reg = <0>;
>>                          edp_in: endpoint {
>>                                  remote-endpoint = <&dp_intf0_out>;
>>                          };
>>                  };
>>
>>                  port@1 {
>>                          reg = <1>;
>>                          edp_out: endpoint {
>>                                  data-lanes = <0 1 2 3>;
>>                                  remote-endpoint = <&panel_in>;
>>                          };
>>                  };
>>          };
>>
>>          aux-bus {
>>                  panel: panel {
>>                          compatible = "edp-panel";
>>                          power-supply = <&pp3300_disp_x>;
>>                          backlight = <&backlight_lcd0>;
>>                          port {
>>                                  panel_in: endpoint {
>>                                          remote-endpoint = <&edp_out>;
>>                                  };
>>                          };
>>                  };
>>          };
>> };
>>
>> AngeloGioacchino Del Regno (9):
>>    drm/mediatek: dp: Cache EDID for eDP panel
>>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>>    drm/mediatek: dp: Always return connected status for eDP in .detect()
>>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>>    drm/mediatek: dp: Enable event interrupt only when bridge attached
>>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
>>    drm/mediatek: dp: Move AUX_P0 setting to
>>      mtk_dp_initialize_aux_settings()
>>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> 
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> 
> on MT8195 Tomato: eDP panel works if the display panel regulator is always
> on. External DP works.
> 
> Maybe it has something to do with the driver not supporting .wait_hpd_asserted
> and not using a GPIO for HPD?

Even before this change I couldn't get the panel to reliably work without keeping 
the regulator always on (just to point out that I'm not introducing regressions).

I am already trying to understand why this happens... and I'm still researching...
but there's what I've seen for now:
  * Set the panel regulator as regulator-boot-on;
  * Boot: edp-panel will correctly read the EDID, then will run the PM suspend
    handler
  * mtk-dp's .get_edid() callback gets called but, at that time, edp-panel will
    still be suspended (PM resume handler didn't get called)
    * Regulator is still down
  * Failure.

That's not right and probably the .get_edid() callback in mtk-dp has an abuse:
there, mtk_dp_parse_capabilities() gets called, which performs initialization
of some variables for DP link training (essential to get the DP going!).

The question that I am making to myself is whether I should move that elsewhere,
if so, what's the right place (making me able to remove the DRM_BRIDGE_OP_EDID
bridge flag when eDP + aux-bus), or if I should leave that and make sure that the
panel-edp's resume callback is called before .get_edid() from mtk-dp gets called.

That can get done in a separated series (or single patch?)... so that if we get
this one picked sooner than later, we can start upstreaming the panel nodes in
the Cherry devicetrees and only remove the regulator-always-on later.

Thoughts?

Cheers,
Angelo

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Il 06/04/23 09:20, Chen-Yu Tsai ha scritto:
> On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Changes in v3:
>>   - Added DPTX AUX block initialization before trying to communicate
>>     to stop relying on the bootloader keeping it initialized before
>>     booting Linux.
>>   - Fixed commit description for patch [09/09] and removed commented
>>     out code (that slipped from dev phase.. sorry!).
>>
>> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
>>
>> Explaining the "real":
>> Before this change, the DisplayPort driver did support eDP to some
>> extent, but it was treating it entirely like a regular DP interface
>> which is partially fine, after all, embedded DisplayPort *is* actually
>> DisplayPort, but there might be some differences to account for... and
>> this is for both small performance improvements and, more importantly,
>> for correct functionality in some systems.
>>
>> Functionality first:
>>
>> One of the common differences found in various boards implementing eDP
>> and machines using an eDP panel is that many times the HPD line is not
>> connected. This *must* be accounted for: at startup, this specific IP
>> will raise a HPD interrupt (which should maybe be ignored... as it does
>> not appear to be a "real" event...) that will make the eDP panel to be
>> detected and to actually work but, after a suspend-resume cycle, there
>> will be no HPD interrupt (as there's no HPD line in my case!) producing
>> a functionality issue - specifically, the DP Link Training fails because
>> the panel doesn't get powered up, then it stays black and won't work
>> until rebooting the machine (or removing and reinserting the module I
>> think, but I haven't tried that).
>>
>> Now for.. both:
>> eDP panels are *e*DP because they are *not* removable (in the sense that
>> you can't unplug the cable without disassembling the machine, in which
>> case, the machine shall be powered down..!): this (correct) assumption
>> makes us able to solve some issues and to also gain a little performance
>> during PM operations.
>>
>> What was done here is:
>>   - Caching the EDID if the panel is eDP: we're always going to read the
>>     same data everytime, so we can just cache that (as it's small enough)
>>     shortening PM resume times for the eDP driver instance;
>>   - Always return connector_status_connected if it's eDP: non-removable
>>     means connector_status_disconnected can't happen during runtime...
>>     this also saves us some time and even power, as we won't have to
>>     perform yet another power cycle of the HW;
>>   - Added aux-bus support!
>>     This makes us able to rely on panel autodetection from the EDID,
>>     avoiding to add more and more panel timings to panel-edp and, even
>>     better, allowing to use one panel node in devicetrees for multiple
>>     variants of the same machine since, at that point, it's not important
>>     to "preventively know" what panel we have (eh, it's autodetected...!).
>>
>> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
>>
>>
>> P.S.: For your own testing commodity, here's a reference devicetree:
>> &edp_tx {
>>          status = "okay";
>>
>>          pinctrl-names = "default";
>>          pinctrl-0 = <&edptx_pins_default>;
>>
>>          ports {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>>                  port@0 {
>>                          reg = <0>;
>>                          edp_in: endpoint {
>>                                  remote-endpoint = <&dp_intf0_out>;
>>                          };
>>                  };
>>
>>                  port@1 {
>>                          reg = <1>;
>>                          edp_out: endpoint {
>>                                  data-lanes = <0 1 2 3>;
>>                                  remote-endpoint = <&panel_in>;
>>                          };
>>                  };
>>          };
>>
>>          aux-bus {
>>                  panel: panel {
>>                          compatible = "edp-panel";
>>                          power-supply = <&pp3300_disp_x>;
>>                          backlight = <&backlight_lcd0>;
>>                          port {
>>                                  panel_in: endpoint {
>>                                          remote-endpoint = <&edp_out>;
>>                                  };
>>                          };
>>                  };
>>          };
>> };
>>
>> AngeloGioacchino Del Regno (9):
>>    drm/mediatek: dp: Cache EDID for eDP panel
>>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>>    drm/mediatek: dp: Always return connected status for eDP in .detect()
>>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>>    drm/mediatek: dp: Enable event interrupt only when bridge attached
>>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
>>    drm/mediatek: dp: Move AUX_P0 setting to
>>      mtk_dp_initialize_aux_settings()
>>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> 
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> 
> on MT8195 Tomato: eDP panel works if the display panel regulator is always
> on. External DP works.
> 
> Maybe it has something to do with the driver not supporting .wait_hpd_asserted
> and not using a GPIO for HPD?

Even before this change I couldn't get the panel to reliably work without keeping 
the regulator always on (just to point out that I'm not introducing regressions).

I am already trying to understand why this happens... and I'm still researching...
but there's what I've seen for now:
  * Set the panel regulator as regulator-boot-on;
  * Boot: edp-panel will correctly read the EDID, then will run the PM suspend
    handler
  * mtk-dp's .get_edid() callback gets called but, at that time, edp-panel will
    still be suspended (PM resume handler didn't get called)
    * Regulator is still down
  * Failure.

That's not right and probably the .get_edid() callback in mtk-dp has an abuse:
there, mtk_dp_parse_capabilities() gets called, which performs initialization
of some variables for DP link training (essential to get the DP going!).

The question that I am making to myself is whether I should move that elsewhere,
if so, what's the right place (making me able to remove the DRM_BRIDGE_OP_EDID
bridge flag when eDP + aux-bus), or if I should leave that and make sure that the
panel-edp's resume callback is called before .get_edid() from mtk-dp gets called.

That can get done in a separated series (or single patch?)... so that if we get
this one picked sooner than later, we can start upstreaming the panel nodes in
the Cherry devicetrees and only remove the regulator-always-on later.

Thoughts?

Cheers,
Angelo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[AngeloGioacchino Del Regno]

Il 06/04/23 10:20, Chen-Yu Tsai ha scritto:
> On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Everytime we run bridge detection and/or EDID read we run a poweron
>> and poweroff sequence for both the AUX and the panel; moreover, this
>> is also done when enabling the bridge in the .atomic_enable() callback.
>>
>> Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
>> function as to commonize it.
>> Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
>> the AUX was getting powered on but the panel was left powered off if
>> the DP cable wasn't plugged in while now we unconditionally send a D0
>> request and this is done for two reasons:
>>   - First, whether this request fails or not, it takes the same time
>>     and anyway the DP hardware won't produce any error (or, if it
>>     does, it's ignorable because it won't block further commands)
>>   - Second, training the link between a sleeping/standby/unpowered
>>     display makes little sense.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 84f82cc68672..76ea94167531 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
>>                             val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
>>   }
>>
>> +static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
>> +{
>> +       if (pwron) {
>> +               /* power on aux */
>> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> +                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
>> +                                  DP_PWR_STATE_MASK);
>> +
>> +               /* power on panel */
>> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
>> +               usleep_range(2000, 5000);
>> +       } else {
>> +               /* power off panel */
>> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
>> +               usleep_range(2000, 3000);
>> +
>> +               /* power off aux */
>> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> +                                  DP_PWR_STATE_BANDGAP_TPLL,
>> +                                  DP_PWR_STATE_MASK);
>> +       }
>> +}
>> +
>>   static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>>   {
>>          mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
>> @@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>>          if (!mtk_dp->train_info.cable_plugged_in)
>>                  return ret;
>>
>> -       if (!enabled) {
>> -               /* power on aux */
>> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
>> -                                  DP_PWR_STATE_MASK);
>> +       if (!enabled)
>> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>>
>> -               /* power on panel */
>> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> 
> I suspect the original code was somewhat wrong already? We shouldn't need
> to pull the panel out of standby just for HPD or reading EDID.
> 
> This driver probably needs a lot more cleanup. :/
> 

I believe the same... but I wanted to play safe, as I don't know if there's any
panel in particular that requires such quirk...

Angelo

Re: [PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[AngeloGioacchino Del Regno]

Il 06/04/23 10:20, Chen-Yu Tsai ha scritto:
> On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Everytime we run bridge detection and/or EDID read we run a poweron
>> and poweroff sequence for both the AUX and the panel; moreover, this
>> is also done when enabling the bridge in the .atomic_enable() callback.
>>
>> Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
>> function as to commonize it.
>> Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
>> the AUX was getting powered on but the panel was left powered off if
>> the DP cable wasn't plugged in while now we unconditionally send a D0
>> request and this is done for two reasons:
>>   - First, whether this request fails or not, it takes the same time
>>     and anyway the DP hardware won't produce any error (or, if it
>>     does, it's ignorable because it won't block further commands)
>>   - Second, training the link between a sleeping/standby/unpowered
>>     display makes little sense.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 84f82cc68672..76ea94167531 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
>>                             val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
>>   }
>>
>> +static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
>> +{
>> +       if (pwron) {
>> +               /* power on aux */
>> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> +                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
>> +                                  DP_PWR_STATE_MASK);
>> +
>> +               /* power on panel */
>> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
>> +               usleep_range(2000, 5000);
>> +       } else {
>> +               /* power off panel */
>> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
>> +               usleep_range(2000, 3000);
>> +
>> +               /* power off aux */
>> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> +                                  DP_PWR_STATE_BANDGAP_TPLL,
>> +                                  DP_PWR_STATE_MASK);
>> +       }
>> +}
>> +
>>   static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>>   {
>>          mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
>> @@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>>          if (!mtk_dp->train_info.cable_plugged_in)
>>                  return ret;
>>
>> -       if (!enabled) {
>> -               /* power on aux */
>> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
>> -                                  DP_PWR_STATE_MASK);
>> +       if (!enabled)
>> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>>
>> -               /* power on panel */
>> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> 
> I suspect the original code was somewhat wrong already? We shouldn't need
> to pull the panel out of standby just for HPD or reading EDID.
> 
> This driver probably needs a lot more cleanup. :/
> 

I believe the same... but I wanted to play safe, as I don't know if there's any
panel in particular that requires such quirk...

Angelo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/9] drm/mediatek: dp: Move AUX and panel poweron/off sequence to function

[AngeloGioacchino Del Regno]

Il 06/04/23 10:20, Chen-Yu Tsai ha scritto:
> On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Everytime we run bridge detection and/or EDID read we run a poweron
>> and poweroff sequence for both the AUX and the panel; moreover, this
>> is also done when enabling the bridge in the .atomic_enable() callback.
>>
>> Move this power on/off sequence to a new mtk_dp_aux_panel_poweron()
>> function as to commonize it.
>> Note that, before this commit, in mtk_dp_bridge_atomic_enable() only
>> the AUX was getting powered on but the panel was left powered off if
>> the DP cable wasn't plugged in while now we unconditionally send a D0
>> request and this is done for two reasons:
>>   - First, whether this request fails or not, it takes the same time
>>     and anyway the DP hardware won't produce any error (or, if it
>>     does, it's ignorable because it won't block further commands)
>>   - Second, training the link between a sleeping/standby/unpowered
>>     display makes little sense.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 76 ++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 84f82cc68672..76ea94167531 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -1253,6 +1253,29 @@ static void mtk_dp_audio_mute(struct mtk_dp *mtk_dp, bool mute)
>>                             val[2], AU_TS_CFG_DP_ENC0_P0_MASK);
>>   }
>>
>> +static void mtk_dp_aux_panel_poweron(struct mtk_dp *mtk_dp, bool pwron)
>> +{
>> +       if (pwron) {
>> +               /* power on aux */
>> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> +                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
>> +                                  DP_PWR_STATE_MASK);
>> +
>> +               /* power on panel */
>> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
>> +               usleep_range(2000, 5000);
>> +       } else {
>> +               /* power off panel */
>> +               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D3);
>> +               usleep_range(2000, 3000);
>> +
>> +               /* power off aux */
>> +               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> +                                  DP_PWR_STATE_BANDGAP_TPLL,
>> +                                  DP_PWR_STATE_MASK);
>> +       }
>> +}
>> +
>>   static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>>   {
>>          mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE,
>> @@ -1937,16 +1960,9 @@ static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
>>          if (!mtk_dp->train_info.cable_plugged_in)
>>                  return ret;
>>
>> -       if (!enabled) {
>> -               /* power on aux */
>> -               mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>> -                                  DP_PWR_STATE_BANDGAP_TPLL_LANE,
>> -                                  DP_PWR_STATE_MASK);
>> +       if (!enabled)
>> +               mtk_dp_aux_panel_poweron(mtk_dp, true);
>>
>> -               /* power on panel */
>> -               drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> 
> I suspect the original code was somewhat wrong already? We shouldn't need
> to pull the panel out of standby just for HPD or reading EDID.
> 
> This driver probably needs a lot more cleanup. :/
> 

I believe the same... but I wanted to play safe, as I don't know if there's any
panel in particular that requires such quirk...

Angelo

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Thu, Apr 6, 2023 at 4:25 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 06/04/23 09:20, Chen-Yu Tsai ha scritto:
> > On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Changes in v3:
> >>   - Added DPTX AUX block initialization before trying to communicate
> >>     to stop relying on the bootloader keeping it initialized before
> >>     booting Linux.
> >>   - Fixed commit description for patch [09/09] and removed commented
> >>     out code (that slipped from dev phase.. sorry!).
> >>
> >> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> >>
> >> Explaining the "real":
> >> Before this change, the DisplayPort driver did support eDP to some
> >> extent, but it was treating it entirely like a regular DP interface
> >> which is partially fine, after all, embedded DisplayPort *is* actually
> >> DisplayPort, but there might be some differences to account for... and
> >> this is for both small performance improvements and, more importantly,
> >> for correct functionality in some systems.
> >>
> >> Functionality first:
> >>
> >> One of the common differences found in various boards implementing eDP
> >> and machines using an eDP panel is that many times the HPD line is not
> >> connected. This *must* be accounted for: at startup, this specific IP
> >> will raise a HPD interrupt (which should maybe be ignored... as it does
> >> not appear to be a "real" event...) that will make the eDP panel to be
> >> detected and to actually work but, after a suspend-resume cycle, there
> >> will be no HPD interrupt (as there's no HPD line in my case!) producing
> >> a functionality issue - specifically, the DP Link Training fails because
> >> the panel doesn't get powered up, then it stays black and won't work
> >> until rebooting the machine (or removing and reinserting the module I
> >> think, but I haven't tried that).
> >>
> >> Now for.. both:
> >> eDP panels are *e*DP because they are *not* removable (in the sense that
> >> you can't unplug the cable without disassembling the machine, in which
> >> case, the machine shall be powered down..!): this (correct) assumption
> >> makes us able to solve some issues and to also gain a little performance
> >> during PM operations.
> >>
> >> What was done here is:
> >>   - Caching the EDID if the panel is eDP: we're always going to read the
> >>     same data everytime, so we can just cache that (as it's small enough)
> >>     shortening PM resume times for the eDP driver instance;
> >>   - Always return connector_status_connected if it's eDP: non-removable
> >>     means connector_status_disconnected can't happen during runtime...
> >>     this also saves us some time and even power, as we won't have to
> >>     perform yet another power cycle of the HW;
> >>   - Added aux-bus support!
> >>     This makes us able to rely on panel autodetection from the EDID,
> >>     avoiding to add more and more panel timings to panel-edp and, even
> >>     better, allowing to use one panel node in devicetrees for multiple
> >>     variants of the same machine since, at that point, it's not important
> >>     to "preventively know" what panel we have (eh, it's autodetected...!).
> >>
> >> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> >>
> >>
> >> P.S.: For your own testing commodity, here's a reference devicetree:
> >> &edp_tx {
> >>          status = "okay";
> >>
> >>          pinctrl-names = "default";
> >>          pinctrl-0 = <&edptx_pins_default>;
> >>
> >>          ports {
> >>                  #address-cells = <1>;
> >>                  #size-cells = <0>;
> >>
> >>                  port@0 {
> >>                          reg = <0>;
> >>                          edp_in: endpoint {
> >>                                  remote-endpoint = <&dp_intf0_out>;
> >>                          };
> >>                  };
> >>
> >>                  port@1 {
> >>                          reg = <1>;
> >>                          edp_out: endpoint {
> >>                                  data-lanes = <0 1 2 3>;
> >>                                  remote-endpoint = <&panel_in>;
> >>                          };
> >>                  };
> >>          };
> >>
> >>          aux-bus {
> >>                  panel: panel {
> >>                          compatible = "edp-panel";
> >>                          power-supply = <&pp3300_disp_x>;
> >>                          backlight = <&backlight_lcd0>;
> >>                          port {
> >>                                  panel_in: endpoint {
> >>                                          remote-endpoint = <&edp_out>;
> >>                                  };
> >>                          };
> >>                  };
> >>          };
> >> };
> >>
> >> AngeloGioacchino Del Regno (9):
> >>    drm/mediatek: dp: Cache EDID for eDP panel
> >>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
> >>    drm/mediatek: dp: Always return connected status for eDP in .detect()
> >>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
> >>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
> >>    drm/mediatek: dp: Enable event interrupt only when bridge attached
> >>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
> >>    drm/mediatek: dp: Move AUX_P0 setting to
> >>      mtk_dp_initialize_aux_settings()
> >>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> >
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> >
> > on MT8195 Tomato: eDP panel works if the display panel regulator is always
> > on. External DP works.
> >
> > Maybe it has something to do with the driver not supporting .wait_hpd_asserted
> > and not using a GPIO for HPD?
>
> Even before this change I couldn't get the panel to reliably work without keeping
> the regulator always on (just to point out that I'm not introducing regressions).
>
> I am already trying to understand why this happens... and I'm still researching...
> but there's what I've seen for now:
>   * Set the panel regulator as regulator-boot-on;

This simply means when the regulator driver grabs the GPIO, it will grab it
with output high. It will make no attempt to keep it on.

>   * Boot: edp-panel will correctly read the EDID, then will run the PM suspend
>     handler
>   * mtk-dp's .get_edid() callback gets called but, at that time, edp-panel will
>     still be suspended (PM resume handler didn't get called)
>     * Regulator is still down
>   * Failure.
> That's not right and probably the .get_edid() callback in mtk-dp has an abuse:
> there, mtk_dp_parse_capabilities() gets called, which performs initialization
> of some variables for DP link training (essential to get the DP going!).

I think that's wrong.

> The question that I am making to myself is whether I should move that elsewhere,
> if so, what's the right place (making me able to remove the DRM_BRIDGE_OP_EDID
> bridge flag when eDP + aux-bus), or if I should leave that and make sure that the
> panel-edp's resume callback is called before .get_edid() from mtk-dp gets called.

I think that's a proper change. Some of the callbacks in the DP driver are
doing suspicious things, as if the driver came from an era without bridge
drivers, and was not properly split up to fit the new bridge semantics.

Same probably goes for the HPD detection.

> That can get done in a separated series (or single patch?)... so that if we get
> this one picked sooner than later, we can start upstreaming the panel nodes in
> the Cherry devicetrees and only remove the regulator-always-on later.

I guess that works? Except for battery life lol. It's really up to CK.
And it's probably too late to upstream the panel nodes for the upcoming
cycle.

ChenYu

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Thu, Apr 6, 2023 at 4:25 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 06/04/23 09:20, Chen-Yu Tsai ha scritto:
> > On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Changes in v3:
> >>   - Added DPTX AUX block initialization before trying to communicate
> >>     to stop relying on the bootloader keeping it initialized before
> >>     booting Linux.
> >>   - Fixed commit description for patch [09/09] and removed commented
> >>     out code (that slipped from dev phase.. sorry!).
> >>
> >> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> >>
> >> Explaining the "real":
> >> Before this change, the DisplayPort driver did support eDP to some
> >> extent, but it was treating it entirely like a regular DP interface
> >> which is partially fine, after all, embedded DisplayPort *is* actually
> >> DisplayPort, but there might be some differences to account for... and
> >> this is for both small performance improvements and, more importantly,
> >> for correct functionality in some systems.
> >>
> >> Functionality first:
> >>
> >> One of the common differences found in various boards implementing eDP
> >> and machines using an eDP panel is that many times the HPD line is not
> >> connected. This *must* be accounted for: at startup, this specific IP
> >> will raise a HPD interrupt (which should maybe be ignored... as it does
> >> not appear to be a "real" event...) that will make the eDP panel to be
> >> detected and to actually work but, after a suspend-resume cycle, there
> >> will be no HPD interrupt (as there's no HPD line in my case!) producing
> >> a functionality issue - specifically, the DP Link Training fails because
> >> the panel doesn't get powered up, then it stays black and won't work
> >> until rebooting the machine (or removing and reinserting the module I
> >> think, but I haven't tried that).
> >>
> >> Now for.. both:
> >> eDP panels are *e*DP because they are *not* removable (in the sense that
> >> you can't unplug the cable without disassembling the machine, in which
> >> case, the machine shall be powered down..!): this (correct) assumption
> >> makes us able to solve some issues and to also gain a little performance
> >> during PM operations.
> >>
> >> What was done here is:
> >>   - Caching the EDID if the panel is eDP: we're always going to read the
> >>     same data everytime, so we can just cache that (as it's small enough)
> >>     shortening PM resume times for the eDP driver instance;
> >>   - Always return connector_status_connected if it's eDP: non-removable
> >>     means connector_status_disconnected can't happen during runtime...
> >>     this also saves us some time and even power, as we won't have to
> >>     perform yet another power cycle of the HW;
> >>   - Added aux-bus support!
> >>     This makes us able to rely on panel autodetection from the EDID,
> >>     avoiding to add more and more panel timings to panel-edp and, even
> >>     better, allowing to use one panel node in devicetrees for multiple
> >>     variants of the same machine since, at that point, it's not important
> >>     to "preventively know" what panel we have (eh, it's autodetected...!).
> >>
> >> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> >>
> >>
> >> P.S.: For your own testing commodity, here's a reference devicetree:
> >> &edp_tx {
> >>          status = "okay";
> >>
> >>          pinctrl-names = "default";
> >>          pinctrl-0 = <&edptx_pins_default>;
> >>
> >>          ports {
> >>                  #address-cells = <1>;
> >>                  #size-cells = <0>;
> >>
> >>                  port@0 {
> >>                          reg = <0>;
> >>                          edp_in: endpoint {
> >>                                  remote-endpoint = <&dp_intf0_out>;
> >>                          };
> >>                  };
> >>
> >>                  port@1 {
> >>                          reg = <1>;
> >>                          edp_out: endpoint {
> >>                                  data-lanes = <0 1 2 3>;
> >>                                  remote-endpoint = <&panel_in>;
> >>                          };
> >>                  };
> >>          };
> >>
> >>          aux-bus {
> >>                  panel: panel {
> >>                          compatible = "edp-panel";
> >>                          power-supply = <&pp3300_disp_x>;
> >>                          backlight = <&backlight_lcd0>;
> >>                          port {
> >>                                  panel_in: endpoint {
> >>                                          remote-endpoint = <&edp_out>;
> >>                                  };
> >>                          };
> >>                  };
> >>          };
> >> };
> >>
> >> AngeloGioacchino Del Regno (9):
> >>    drm/mediatek: dp: Cache EDID for eDP panel
> >>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
> >>    drm/mediatek: dp: Always return connected status for eDP in .detect()
> >>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
> >>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
> >>    drm/mediatek: dp: Enable event interrupt only when bridge attached
> >>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
> >>    drm/mediatek: dp: Move AUX_P0 setting to
> >>      mtk_dp_initialize_aux_settings()
> >>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> >
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> >
> > on MT8195 Tomato: eDP panel works if the display panel regulator is always
> > on. External DP works.
> >
> > Maybe it has something to do with the driver not supporting .wait_hpd_asserted
> > and not using a GPIO for HPD?
>
> Even before this change I couldn't get the panel to reliably work without keeping
> the regulator always on (just to point out that I'm not introducing regressions).
>
> I am already trying to understand why this happens... and I'm still researching...
> but there's what I've seen for now:
>   * Set the panel regulator as regulator-boot-on;

This simply means when the regulator driver grabs the GPIO, it will grab it
with output high. It will make no attempt to keep it on.

>   * Boot: edp-panel will correctly read the EDID, then will run the PM suspend
>     handler
>   * mtk-dp's .get_edid() callback gets called but, at that time, edp-panel will
>     still be suspended (PM resume handler didn't get called)
>     * Regulator is still down
>   * Failure.
> That's not right and probably the .get_edid() callback in mtk-dp has an abuse:
> there, mtk_dp_parse_capabilities() gets called, which performs initialization
> of some variables for DP link training (essential to get the DP going!).

I think that's wrong.

> The question that I am making to myself is whether I should move that elsewhere,
> if so, what's the right place (making me able to remove the DRM_BRIDGE_OP_EDID
> bridge flag when eDP + aux-bus), or if I should leave that and make sure that the
> panel-edp's resume callback is called before .get_edid() from mtk-dp gets called.

I think that's a proper change. Some of the callbacks in the DP driver are
doing suspicious things, as if the driver came from an era without bridge
drivers, and was not properly split up to fit the new bridge semantics.

Same probably goes for the HPD detection.

> That can get done in a separated series (or single patch?)... so that if we get
> this one picked sooner than later, we can start upstreaming the panel nodes in
> the Cherry devicetrees and only remove the regulator-always-on later.

I guess that works? Except for battery life lol. It's really up to CK.
And it's probably too late to upstream the panel nodes for the upcoming
cycle.

ChenYu

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Thu, Apr 6, 2023 at 4:25 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 06/04/23 09:20, Chen-Yu Tsai ha scritto:
> > On Tue, Apr 4, 2023 at 6:48 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Changes in v3:
> >>   - Added DPTX AUX block initialization before trying to communicate
> >>     to stop relying on the bootloader keeping it initialized before
> >>     booting Linux.
> >>   - Fixed commit description for patch [09/09] and removed commented
> >>     out code (that slipped from dev phase.. sorry!).
> >>
> >> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> >>
> >> Explaining the "real":
> >> Before this change, the DisplayPort driver did support eDP to some
> >> extent, but it was treating it entirely like a regular DP interface
> >> which is partially fine, after all, embedded DisplayPort *is* actually
> >> DisplayPort, but there might be some differences to account for... and
> >> this is for both small performance improvements and, more importantly,
> >> for correct functionality in some systems.
> >>
> >> Functionality first:
> >>
> >> One of the common differences found in various boards implementing eDP
> >> and machines using an eDP panel is that many times the HPD line is not
> >> connected. This *must* be accounted for: at startup, this specific IP
> >> will raise a HPD interrupt (which should maybe be ignored... as it does
> >> not appear to be a "real" event...) that will make the eDP panel to be
> >> detected and to actually work but, after a suspend-resume cycle, there
> >> will be no HPD interrupt (as there's no HPD line in my case!) producing
> >> a functionality issue - specifically, the DP Link Training fails because
> >> the panel doesn't get powered up, then it stays black and won't work
> >> until rebooting the machine (or removing and reinserting the module I
> >> think, but I haven't tried that).
> >>
> >> Now for.. both:
> >> eDP panels are *e*DP because they are *not* removable (in the sense that
> >> you can't unplug the cable without disassembling the machine, in which
> >> case, the machine shall be powered down..!): this (correct) assumption
> >> makes us able to solve some issues and to also gain a little performance
> >> during PM operations.
> >>
> >> What was done here is:
> >>   - Caching the EDID if the panel is eDP: we're always going to read the
> >>     same data everytime, so we can just cache that (as it's small enough)
> >>     shortening PM resume times for the eDP driver instance;
> >>   - Always return connector_status_connected if it's eDP: non-removable
> >>     means connector_status_disconnected can't happen during runtime...
> >>     this also saves us some time and even power, as we won't have to
> >>     perform yet another power cycle of the HW;
> >>   - Added aux-bus support!
> >>     This makes us able to rely on panel autodetection from the EDID,
> >>     avoiding to add more and more panel timings to panel-edp and, even
> >>     better, allowing to use one panel node in devicetrees for multiple
> >>     variants of the same machine since, at that point, it's not important
> >>     to "preventively know" what panel we have (eh, it's autodetected...!).
> >>
> >> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> >>
> >>
> >> P.S.: For your own testing commodity, here's a reference devicetree:
> >> &edp_tx {
> >>          status = "okay";
> >>
> >>          pinctrl-names = "default";
> >>          pinctrl-0 = <&edptx_pins_default>;
> >>
> >>          ports {
> >>                  #address-cells = <1>;
> >>                  #size-cells = <0>;
> >>
> >>                  port@0 {
> >>                          reg = <0>;
> >>                          edp_in: endpoint {
> >>                                  remote-endpoint = <&dp_intf0_out>;
> >>                          };
> >>                  };
> >>
> >>                  port@1 {
> >>                          reg = <1>;
> >>                          edp_out: endpoint {
> >>                                  data-lanes = <0 1 2 3>;
> >>                                  remote-endpoint = <&panel_in>;
> >>                          };
> >>                  };
> >>          };
> >>
> >>          aux-bus {
> >>                  panel: panel {
> >>                          compatible = "edp-panel";
> >>                          power-supply = <&pp3300_disp_x>;
> >>                          backlight = <&backlight_lcd0>;
> >>                          port {
> >>                                  panel_in: endpoint {
> >>                                          remote-endpoint = <&edp_out>;
> >>                                  };
> >>                          };
> >>                  };
> >>          };
> >> };
> >>
> >> AngeloGioacchino Del Regno (9):
> >>    drm/mediatek: dp: Cache EDID for eDP panel
> >>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
> >>    drm/mediatek: dp: Always return connected status for eDP in .detect()
> >>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
> >>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
> >>    drm/mediatek: dp: Enable event interrupt only when bridge attached
> >>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
> >>    drm/mediatek: dp: Move AUX_P0 setting to
> >>      mtk_dp_initialize_aux_settings()
> >>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> >
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> >
> > on MT8195 Tomato: eDP panel works if the display panel regulator is always
> > on. External DP works.
> >
> > Maybe it has something to do with the driver not supporting .wait_hpd_asserted
> > and not using a GPIO for HPD?
>
> Even before this change I couldn't get the panel to reliably work without keeping
> the regulator always on (just to point out that I'm not introducing regressions).
>
> I am already trying to understand why this happens... and I'm still researching...
> but there's what I've seen for now:
>   * Set the panel regulator as regulator-boot-on;

This simply means when the regulator driver grabs the GPIO, it will grab it
with output high. It will make no attempt to keep it on.

>   * Boot: edp-panel will correctly read the EDID, then will run the PM suspend
>     handler
>   * mtk-dp's .get_edid() callback gets called but, at that time, edp-panel will
>     still be suspended (PM resume handler didn't get called)
>     * Regulator is still down
>   * Failure.
> That's not right and probably the .get_edid() callback in mtk-dp has an abuse:
> there, mtk_dp_parse_capabilities() gets called, which performs initialization
> of some variables for DP link training (essential to get the DP going!).

I think that's wrong.

> The question that I am making to myself is whether I should move that elsewhere,
> if so, what's the right place (making me able to remove the DRM_BRIDGE_OP_EDID
> bridge flag when eDP + aux-bus), or if I should leave that and make sure that the
> panel-edp's resume callback is called before .get_edid() from mtk-dp gets called.

I think that's a proper change. Some of the callbacks in the DP driver are
doing suspicious things, as if the driver came from an era without bridge
drivers, and was not properly split up to fit the new bridge semantics.

Same probably goes for the HPD detection.

> That can get done in a separated series (or single patch?)... so that if we get
> this one picked sooner than later, we can start upstreaming the panel nodes in
> the Cherry devicetrees and only remove the regulator-always-on later.

I guess that works? Except for battery life lol. It's really up to CK.
And it's probably too late to upstream the panel nodes for the upcoming
cycle.

ChenYu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[Matthias Brugger]

On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
> Since eDP panels are not removable it is safe to cache the EDID:
> this will avoid a relatively long read transaction at every PM
> resume that is unnecessary only in the "special" case of eDP,
> hence speeding it up a little, as from now on, as resume operation,
> we will perform only link training.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 1f94fcc144d3..84f82cc68672 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -118,6 +118,7 @@ struct mtk_dp {
>   	const struct mtk_dp_data *data;
>   	struct mtk_dp_info info;
>   	struct mtk_dp_train_info train_info;
> +	struct edid *edid;
>   
>   	struct platform_device *phy_dev;
>   	struct phy *phy;
> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		usleep_range(2000, 5000);
>   	}
>   
> -	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> +	/* eDP panels aren't removable, so we can return a cached EDID. */
> +	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
> +		new_edid = drm_edid_duplicate(mtk_dp->edid);
> +	else
> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);

Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
DRM_MODE_CONNECTOR_eDP
it would make the code more readable.

>   
>   	/*
>   	 * Parse capability here to let atomic_get_input_bus_fmts and
> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>   	}
>   
> +	/* If this is an eDP panel and the read EDID is good, cache it for later */
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
> +		mtk_dp->edid = drm_edid_duplicate(new_edid);
> +

How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
least we could get rid of the check regarding if new_edid != NULL.

I was thinking on how to put both if statements in one block, but I think the 
problem is, that we would leak memory if the capability parsing failes due to 
the call to drm_edid_duplicate(). Correct?

Regards,
Matthais

>   	return new_edid;
>   }.	/
>   

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[Matthias Brugger]

On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
> Since eDP panels are not removable it is safe to cache the EDID:
> this will avoid a relatively long read transaction at every PM
> resume that is unnecessary only in the "special" case of eDP,
> hence speeding it up a little, as from now on, as resume operation,
> we will perform only link training.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 1f94fcc144d3..84f82cc68672 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -118,6 +118,7 @@ struct mtk_dp {
>   	const struct mtk_dp_data *data;
>   	struct mtk_dp_info info;
>   	struct mtk_dp_train_info train_info;
> +	struct edid *edid;
>   
>   	struct platform_device *phy_dev;
>   	struct phy *phy;
> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		usleep_range(2000, 5000);
>   	}
>   
> -	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> +	/* eDP panels aren't removable, so we can return a cached EDID. */
> +	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
> +		new_edid = drm_edid_duplicate(mtk_dp->edid);
> +	else
> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);

Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
DRM_MODE_CONNECTOR_eDP
it would make the code more readable.

>   
>   	/*
>   	 * Parse capability here to let atomic_get_input_bus_fmts and
> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>   	}
>   
> +	/* If this is an eDP panel and the read EDID is good, cache it for later */
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
> +		mtk_dp->edid = drm_edid_duplicate(new_edid);
> +

How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
least we could get rid of the check regarding if new_edid != NULL.

I was thinking on how to put both if statements in one block, but I think the 
problem is, that we would leak memory if the capability parsing failes due to 
the call to drm_edid_duplicate(). Correct?

Regards,
Matthais

>   	return new_edid;
>   }.	/
>   

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[Matthias Brugger]

On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
> Since eDP panels are not removable it is safe to cache the EDID:
> this will avoid a relatively long read transaction at every PM
> resume that is unnecessary only in the "special" case of eDP,
> hence speeding it up a little, as from now on, as resume operation,
> we will perform only link training.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 1f94fcc144d3..84f82cc68672 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -118,6 +118,7 @@ struct mtk_dp {
>   	const struct mtk_dp_data *data;
>   	struct mtk_dp_info info;
>   	struct mtk_dp_train_info train_info;
> +	struct edid *edid;
>   
>   	struct platform_device *phy_dev;
>   	struct phy *phy;
> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		usleep_range(2000, 5000);
>   	}
>   
> -	new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> +	/* eDP panels aren't removable, so we can return a cached EDID. */
> +	if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
> +		new_edid = drm_edid_duplicate(mtk_dp->edid);
> +	else
> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);

Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
DRM_MODE_CONNECTOR_eDP
it would make the code more readable.

>   
>   	/*
>   	 * Parse capability here to let atomic_get_input_bus_fmts and
> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
>   		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>   	}
>   
> +	/* If this is an eDP panel and the read EDID is good, cache it for later */
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
> +		mtk_dp->edid = drm_edid_duplicate(new_edid);
> +

How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
least we could get rid of the check regarding if new_edid != NULL.

I was thinking on how to put both if statements in one block, but I think the 
problem is, that we would leak memory if the capability parsing failes due to 
the call to drm_edid_duplicate(). Correct?

Regards,
Matthais

>   	return new_edid;
>   }.	/
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Il 12/04/23 09:08, Matthias Brugger ha scritto:
> 
> 
> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>> Since eDP panels are not removable it is safe to cache the EDID:
>> this will avoid a relatively long read transaction at every PM
>> resume that is unnecessary only in the "special" case of eDP,
>> hence speeding it up a little, as from now on, as resume operation,
>> we will perform only link training.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 1f94fcc144d3..84f82cc68672 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>       const struct mtk_dp_data *data;
>>       struct mtk_dp_info info;
>>       struct mtk_dp_train_info train_info;
>> +    struct edid *edid;
>>       struct platform_device *phy_dev;
>>       struct phy *phy;
>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           usleep_range(2000, 5000);
>>       }
>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>> +    else
>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> 
> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
> DRM_MODE_CONNECTOR_eDP
> it would make the code more readable.
> 

I had the same idea... but then avoided that because in most (if not all?) of the
DRM drivers (at least, the one I've read) this check is always open coded, so I
wrote it like that for consistency and nothing else.

I have no strong opinions on that though!

>>       /*
>>        * Parse capability here to let atomic_get_input_bus_fmts and
>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>       }
>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>> +
> 
> How about putting this in an else if branch of mtk_dp_parse_capabilities. At least 
> we could get rid of the check regarding if new_edid != NULL.
> 
> I was thinking on how to put both if statements in one block, but I think the 
> problem is, that we would leak memory if the capability parsing failes due to the 
> call to drm_edid_duplicate(). Correct?
> 

Correct. The only other "good" place would be in the `if (new_edid)` conditional,
but that wouldn't be as readable as it is right now...

Cheers,
Angelo

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Il 12/04/23 09:08, Matthias Brugger ha scritto:
> 
> 
> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>> Since eDP panels are not removable it is safe to cache the EDID:
>> this will avoid a relatively long read transaction at every PM
>> resume that is unnecessary only in the "special" case of eDP,
>> hence speeding it up a little, as from now on, as resume operation,
>> we will perform only link training.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 1f94fcc144d3..84f82cc68672 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>       const struct mtk_dp_data *data;
>>       struct mtk_dp_info info;
>>       struct mtk_dp_train_info train_info;
>> +    struct edid *edid;
>>       struct platform_device *phy_dev;
>>       struct phy *phy;
>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           usleep_range(2000, 5000);
>>       }
>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>> +    else
>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> 
> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
> DRM_MODE_CONNECTOR_eDP
> it would make the code more readable.
> 

I had the same idea... but then avoided that because in most (if not all?) of the
DRM drivers (at least, the one I've read) this check is always open coded, so I
wrote it like that for consistency and nothing else.

I have no strong opinions on that though!

>>       /*
>>        * Parse capability here to let atomic_get_input_bus_fmts and
>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>       }
>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>> +
> 
> How about putting this in an else if branch of mtk_dp_parse_capabilities. At least 
> we could get rid of the check regarding if new_edid != NULL.
> 
> I was thinking on how to put both if statements in one block, but I think the 
> problem is, that we would leak memory if the capability parsing failes due to the 
> call to drm_edid_duplicate(). Correct?
> 

Correct. The only other "good" place would be in the `if (new_edid)` conditional,
but that wouldn't be as readable as it is right now...

Cheers,
Angelo

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Il 12/04/23 09:08, Matthias Brugger ha scritto:
> 
> 
> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>> Since eDP panels are not removable it is safe to cache the EDID:
>> this will avoid a relatively long read transaction at every PM
>> resume that is unnecessary only in the "special" case of eDP,
>> hence speeding it up a little, as from now on, as resume operation,
>> we will perform only link training.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index 1f94fcc144d3..84f82cc68672 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>       const struct mtk_dp_data *data;
>>       struct mtk_dp_info info;
>>       struct mtk_dp_train_info train_info;
>> +    struct edid *edid;
>>       struct platform_device *phy_dev;
>>       struct phy *phy;
>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           usleep_range(2000, 5000);
>>       }
>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>> +    else
>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> 
> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
> DRM_MODE_CONNECTOR_eDP
> it would make the code more readable.
> 

I had the same idea... but then avoided that because in most (if not all?) of the
DRM drivers (at least, the one I've read) this check is always open coded, so I
wrote it like that for consistency and nothing else.

I have no strong opinions on that though!

>>       /*
>>        * Parse capability here to let atomic_get_input_bus_fmts and
>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>> *bridge,
>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>       }
>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>> +
> 
> How about putting this in an else if branch of mtk_dp_parse_capabilities. At least 
> we could get rid of the check regarding if new_edid != NULL.
> 
> I was thinking on how to put both if statements in one block, but I think the 
> problem is, that we would leak memory if the capability parsing failes due to the 
> call to drm_edid_duplicate(). Correct?
> 

Correct. The only other "good" place would be in the `if (new_edid)` conditional,
but that wouldn't be as readable as it is right now...

Cheers,
Angelo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[Matthias Brugger]

On 12/04/2023 10:06, AngeloGioacchino Del Regno wrote:
> Il 12/04/23 09:08, Matthias Brugger ha scritto:
>>
>>
>> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>>> Since eDP panels are not removable it is safe to cache the EDID:
>>> this will avoid a relatively long read transaction at every PM
>>> resume that is unnecessary only in the "special" case of eDP,
>>> hence speeding it up a little, as from now on, as resume operation,
>>> we will perform only link training.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c 
>>> b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> index 1f94fcc144d3..84f82cc68672 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>>       const struct mtk_dp_data *data;
>>>       struct mtk_dp_info info;
>>>       struct mtk_dp_train_info train_info;
>>> +    struct edid *edid;
>>>       struct platform_device *phy_dev;
>>>       struct phy *phy;
>>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           usleep_range(2000, 5000);
>>>       }
>>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)

Maybe better like this:
if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && mtk_dp->edid)

To in sync with the if statement below. Anyway we are only concerned if it's an 
eDP so check that first (and hope the compiler will do so as well ;)

With that:
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

>>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>>> +    else
>>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>
>> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type 
>> == DRM_MODE_CONNECTOR_eDP
>> it would make the code more readable.
>>
> 
> I had the same idea... but then avoided that because in most (if not all?) of the
> DRM drivers (at least, the one I've read) this check is always open coded, so I
> wrote it like that for consistency and nothing else.
> 
> I have no strong opinions on that though!
> 

I think the only reasonable solution would be a macro like:
DRM_CONNECTOR_MODE_IS(mtk_dp->bridge.type, eDP) which in the end is longer then 
open-code it, so probably just leave it as it is.

>>>       /*
>>>        * Parse capability here to let atomic_get_input_bus_fmts and
>>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>>       }
>>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && 
>>> new_edid)
>>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>>> +
>>
>> How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
>> least we could get rid of the check regarding if new_edid != NULL.
>>
>> I was thinking on how to put both if statements in one block, but I think the 
>> problem is, that we would leak memory if the capability parsing failes due to 
>> the call to drm_edid_duplicate(). Correct?
>>
> 
> Correct. The only other "good" place would be in the `if (new_edid)` conditional,
> but that wouldn't be as readable as it is right now...
> 
> Cheers,
> Angelo
> 

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[Matthias Brugger]

On 12/04/2023 10:06, AngeloGioacchino Del Regno wrote:
> Il 12/04/23 09:08, Matthias Brugger ha scritto:
>>
>>
>> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>>> Since eDP panels are not removable it is safe to cache the EDID:
>>> this will avoid a relatively long read transaction at every PM
>>> resume that is unnecessary only in the "special" case of eDP,
>>> hence speeding it up a little, as from now on, as resume operation,
>>> we will perform only link training.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c 
>>> b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> index 1f94fcc144d3..84f82cc68672 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>>       const struct mtk_dp_data *data;
>>>       struct mtk_dp_info info;
>>>       struct mtk_dp_train_info train_info;
>>> +    struct edid *edid;
>>>       struct platform_device *phy_dev;
>>>       struct phy *phy;
>>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           usleep_range(2000, 5000);
>>>       }
>>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)

Maybe better like this:
if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && mtk_dp->edid)

To in sync with the if statement below. Anyway we are only concerned if it's an 
eDP so check that first (and hope the compiler will do so as well ;)

With that:
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

>>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>>> +    else
>>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>
>> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type 
>> == DRM_MODE_CONNECTOR_eDP
>> it would make the code more readable.
>>
> 
> I had the same idea... but then avoided that because in most (if not all?) of the
> DRM drivers (at least, the one I've read) this check is always open coded, so I
> wrote it like that for consistency and nothing else.
> 
> I have no strong opinions on that though!
> 

I think the only reasonable solution would be a macro like:
DRM_CONNECTOR_MODE_IS(mtk_dp->bridge.type, eDP) which in the end is longer then 
open-code it, so probably just leave it as it is.

>>>       /*
>>>        * Parse capability here to let atomic_get_input_bus_fmts and
>>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>>       }
>>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && 
>>> new_edid)
>>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>>> +
>>
>> How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
>> least we could get rid of the check regarding if new_edid != NULL.
>>
>> I was thinking on how to put both if statements in one block, but I think the 
>> problem is, that we would leak memory if the capability parsing failes due to 
>> the call to drm_edid_duplicate(). Correct?
>>
> 
> Correct. The only other "good" place would be in the `if (new_edid)` conditional,
> but that wouldn't be as readable as it is right now...
> 
> Cheers,
> Angelo
> 

Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[Matthias Brugger]

On 12/04/2023 10:06, AngeloGioacchino Del Regno wrote:
> Il 12/04/23 09:08, Matthias Brugger ha scritto:
>>
>>
>> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
>>> Since eDP panels are not removable it is safe to cache the EDID:
>>> this will avoid a relatively long read transaction at every PM
>>> resume that is unnecessary only in the "special" case of eDP,
>>> hence speeding it up a little, as from now on, as resume operation,
>>> we will perform only link training.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c 
>>> b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> index 1f94fcc144d3..84f82cc68672 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> @@ -118,6 +118,7 @@ struct mtk_dp {
>>>       const struct mtk_dp_data *data;
>>>       struct mtk_dp_info info;
>>>       struct mtk_dp_train_info train_info;
>>> +    struct edid *edid;
>>>       struct platform_device *phy_dev;
>>>       struct phy *phy;
>>> @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           usleep_range(2000, 5000);
>>>       }
>>> -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>> +    /* eDP panels aren't removable, so we can return a cached EDID. */
>>> +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)

Maybe better like this:
if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && mtk_dp->edid)

To in sync with the if statement below. Anyway we are only concerned if it's an 
eDP so check that first (and hope the compiler will do so as well ;)

With that:
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

>>> +        new_edid = drm_edid_duplicate(mtk_dp->edid);
>>> +    else
>>> +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>>
>> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type 
>> == DRM_MODE_CONNECTOR_eDP
>> it would make the code more readable.
>>
> 
> I had the same idea... but then avoided that because in most (if not all?) of the
> DRM drivers (at least, the one I've read) this check is always open coded, so I
> wrote it like that for consistency and nothing else.
> 
> I have no strong opinions on that though!
> 

I think the only reasonable solution would be a macro like:
DRM_CONNECTOR_MODE_IS(mtk_dp->bridge.type, eDP) which in the end is longer then 
open-code it, so probably just leave it as it is.

>>>       /*
>>>        * Parse capability here to let atomic_get_input_bus_fmts and
>>> @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
>>> *bridge,
>>>           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>>>       }
>>> +    /* If this is an eDP panel and the read EDID is good, cache it for later */
>>> +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && 
>>> new_edid)
>>> +        mtk_dp->edid = drm_edid_duplicate(new_edid);
>>> +
>>
>> How about putting this in an else if branch of mtk_dp_parse_capabilities. At 
>> least we could get rid of the check regarding if new_edid != NULL.
>>
>> I was thinking on how to put both if statements in one block, but I think the 
>> problem is, that we would leak memory if the capability parsing failes due to 
>> the call to drm_edid_duplicate(). Correct?
>>
> 
> Correct. The only other "good" place would be in the `if (new_edid)` conditional,
> but that wouldn't be as readable as it is right now...
> 
> Cheers,
> Angelo
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Il 04/04/23 12:47, AngeloGioacchino Del Regno ha scritto:

Hello CK,

Gentle ping for this series.

Thanks,
Angelo

> Changes in v3:
>   - Added DPTX AUX block initialization before trying to communicate
>     to stop relying on the bootloader keeping it initialized before
>     booting Linux.
>   - Fixed commit description for patch [09/09] and removed commented
>     out code (that slipped from dev phase.. sorry!).
> 
> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> 
> Explaining the "real":
> Before this change, the DisplayPort driver did support eDP to some
> extent, but it was treating it entirely like a regular DP interface
> which is partially fine, after all, embedded DisplayPort *is* actually
> DisplayPort, but there might be some differences to account for... and
> this is for both small performance improvements and, more importantly,
> for correct functionality in some systems.
> 
> Functionality first:
> 
> One of the common differences found in various boards implementing eDP
> and machines using an eDP panel is that many times the HPD line is not
> connected. This *must* be accounted for: at startup, this specific IP
> will raise a HPD interrupt (which should maybe be ignored... as it does
> not appear to be a "real" event...) that will make the eDP panel to be
> detected and to actually work but, after a suspend-resume cycle, there
> will be no HPD interrupt (as there's no HPD line in my case!) producing
> a functionality issue - specifically, the DP Link Training fails because
> the panel doesn't get powered up, then it stays black and won't work
> until rebooting the machine (or removing and reinserting the module I
> think, but I haven't tried that).
> 
> Now for.. both:
> eDP panels are *e*DP because they are *not* removable (in the sense that
> you can't unplug the cable without disassembling the machine, in which
> case, the machine shall be powered down..!): this (correct) assumption
> makes us able to solve some issues and to also gain a little performance
> during PM operations.
> 
> What was done here is:
>   - Caching the EDID if the panel is eDP: we're always going to read the
>     same data everytime, so we can just cache that (as it's small enough)
>     shortening PM resume times for the eDP driver instance;
>   - Always return connector_status_connected if it's eDP: non-removable
>     means connector_status_disconnected can't happen during runtime...
>     this also saves us some time and even power, as we won't have to
>     perform yet another power cycle of the HW;
>   - Added aux-bus support!
>     This makes us able to rely on panel autodetection from the EDID,
>     avoiding to add more and more panel timings to panel-edp and, even
>     better, allowing to use one panel node in devicetrees for multiple
>     variants of the same machine since, at that point, it's not important
>     to "preventively know" what panel we have (eh, it's autodetected...!).
> 
> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> 
> 
> P.S.: For your own testing commodity, here's a reference devicetree:
> &edp_tx {
> 	status = "okay";
> 
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&edptx_pins_default>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 			edp_in: endpoint {
> 				remote-endpoint = <&dp_intf0_out>;
> 			};
> 		};
> 
> 		port@1 {
> 			reg = <1>;
> 			edp_out: endpoint {
> 				data-lanes = <0 1 2 3>;
> 				remote-endpoint = <&panel_in>;
> 			};
> 		};
> 	};
> 
> 	aux-bus {
> 		panel: panel {
> 			compatible = "edp-panel";
> 			power-supply = <&pp3300_disp_x>;
> 			backlight = <&backlight_lcd0>;
> 			port {
> 				panel_in: endpoint {
> 					remote-endpoint = <&edp_out>;
> 				};
> 			};
> 		};
> 	};
> };
> 
> AngeloGioacchino Del Regno (9):
>    drm/mediatek: dp: Cache EDID for eDP panel
>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>    drm/mediatek: dp: Always return connected status for eDP in .detect()
>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>    drm/mediatek: dp: Enable event interrupt only when bridge attached
>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
>    drm/mediatek: dp: Move AUX_P0 setting to
>      mtk_dp_initialize_aux_settings()
>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> 
>   drivers/gpu/drm/mediatek/mtk_dp.c | 186 +++++++++++++++++++-----------
>   1 file changed, 116 insertions(+), 70 deletions(-)
> 

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Il 04/04/23 12:47, AngeloGioacchino Del Regno ha scritto:

Hello CK,

Gentle ping for this series.

Thanks,
Angelo

> Changes in v3:
>   - Added DPTX AUX block initialization before trying to communicate
>     to stop relying on the bootloader keeping it initialized before
>     booting Linux.
>   - Fixed commit description for patch [09/09] and removed commented
>     out code (that slipped from dev phase.. sorry!).
> 
> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> 
> Explaining the "real":
> Before this change, the DisplayPort driver did support eDP to some
> extent, but it was treating it entirely like a regular DP interface
> which is partially fine, after all, embedded DisplayPort *is* actually
> DisplayPort, but there might be some differences to account for... and
> this is for both small performance improvements and, more importantly,
> for correct functionality in some systems.
> 
> Functionality first:
> 
> One of the common differences found in various boards implementing eDP
> and machines using an eDP panel is that many times the HPD line is not
> connected. This *must* be accounted for: at startup, this specific IP
> will raise a HPD interrupt (which should maybe be ignored... as it does
> not appear to be a "real" event...) that will make the eDP panel to be
> detected and to actually work but, after a suspend-resume cycle, there
> will be no HPD interrupt (as there's no HPD line in my case!) producing
> a functionality issue - specifically, the DP Link Training fails because
> the panel doesn't get powered up, then it stays black and won't work
> until rebooting the machine (or removing and reinserting the module I
> think, but I haven't tried that).
> 
> Now for.. both:
> eDP panels are *e*DP because they are *not* removable (in the sense that
> you can't unplug the cable without disassembling the machine, in which
> case, the machine shall be powered down..!): this (correct) assumption
> makes us able to solve some issues and to also gain a little performance
> during PM operations.
> 
> What was done here is:
>   - Caching the EDID if the panel is eDP: we're always going to read the
>     same data everytime, so we can just cache that (as it's small enough)
>     shortening PM resume times for the eDP driver instance;
>   - Always return connector_status_connected if it's eDP: non-removable
>     means connector_status_disconnected can't happen during runtime...
>     this also saves us some time and even power, as we won't have to
>     perform yet another power cycle of the HW;
>   - Added aux-bus support!
>     This makes us able to rely on panel autodetection from the EDID,
>     avoiding to add more and more panel timings to panel-edp and, even
>     better, allowing to use one panel node in devicetrees for multiple
>     variants of the same machine since, at that point, it's not important
>     to "preventively know" what panel we have (eh, it's autodetected...!).
> 
> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> 
> 
> P.S.: For your own testing commodity, here's a reference devicetree:
> &edp_tx {
> 	status = "okay";
> 
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&edptx_pins_default>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 			edp_in: endpoint {
> 				remote-endpoint = <&dp_intf0_out>;
> 			};
> 		};
> 
> 		port@1 {
> 			reg = <1>;
> 			edp_out: endpoint {
> 				data-lanes = <0 1 2 3>;
> 				remote-endpoint = <&panel_in>;
> 			};
> 		};
> 	};
> 
> 	aux-bus {
> 		panel: panel {
> 			compatible = "edp-panel";
> 			power-supply = <&pp3300_disp_x>;
> 			backlight = <&backlight_lcd0>;
> 			port {
> 				panel_in: endpoint {
> 					remote-endpoint = <&edp_out>;
> 				};
> 			};
> 		};
> 	};
> };
> 
> AngeloGioacchino Del Regno (9):
>    drm/mediatek: dp: Cache EDID for eDP panel
>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>    drm/mediatek: dp: Always return connected status for eDP in .detect()
>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>    drm/mediatek: dp: Enable event interrupt only when bridge attached
>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
>    drm/mediatek: dp: Move AUX_P0 setting to
>      mtk_dp_initialize_aux_settings()
>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> 
>   drivers/gpu/drm/mediatek/mtk_dp.c | 186 +++++++++++++++++++-----------
>   1 file changed, 116 insertions(+), 70 deletions(-)
> 

Re: [PATCH v3 0/9] MediaTek DisplayPort: support eDP and aux-bus

[AngeloGioacchino Del Regno]

Il 04/04/23 12:47, AngeloGioacchino Del Regno ha scritto:

Hello CK,

Gentle ping for this series.

Thanks,
Angelo

> Changes in v3:
>   - Added DPTX AUX block initialization before trying to communicate
>     to stop relying on the bootloader keeping it initialized before
>     booting Linux.
>   - Fixed commit description for patch [09/09] and removed commented
>     out code (that slipped from dev phase.. sorry!).
> 
> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> 
> Explaining the "real":
> Before this change, the DisplayPort driver did support eDP to some
> extent, but it was treating it entirely like a regular DP interface
> which is partially fine, after all, embedded DisplayPort *is* actually
> DisplayPort, but there might be some differences to account for... and
> this is for both small performance improvements and, more importantly,
> for correct functionality in some systems.
> 
> Functionality first:
> 
> One of the common differences found in various boards implementing eDP
> and machines using an eDP panel is that many times the HPD line is not
> connected. This *must* be accounted for: at startup, this specific IP
> will raise a HPD interrupt (which should maybe be ignored... as it does
> not appear to be a "real" event...) that will make the eDP panel to be
> detected and to actually work but, after a suspend-resume cycle, there
> will be no HPD interrupt (as there's no HPD line in my case!) producing
> a functionality issue - specifically, the DP Link Training fails because
> the panel doesn't get powered up, then it stays black and won't work
> until rebooting the machine (or removing and reinserting the module I
> think, but I haven't tried that).
> 
> Now for.. both:
> eDP panels are *e*DP because they are *not* removable (in the sense that
> you can't unplug the cable without disassembling the machine, in which
> case, the machine shall be powered down..!): this (correct) assumption
> makes us able to solve some issues and to also gain a little performance
> during PM operations.
> 
> What was done here is:
>   - Caching the EDID if the panel is eDP: we're always going to read the
>     same data everytime, so we can just cache that (as it's small enough)
>     shortening PM resume times for the eDP driver instance;
>   - Always return connector_status_connected if it's eDP: non-removable
>     means connector_status_disconnected can't happen during runtime...
>     this also saves us some time and even power, as we won't have to
>     perform yet another power cycle of the HW;
>   - Added aux-bus support!
>     This makes us able to rely on panel autodetection from the EDID,
>     avoiding to add more and more panel timings to panel-edp and, even
>     better, allowing to use one panel node in devicetrees for multiple
>     variants of the same machine since, at that point, it's not important
>     to "preventively know" what panel we have (eh, it's autodetected...!).
> 
> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> 
> 
> P.S.: For your own testing commodity, here's a reference devicetree:
> &edp_tx {
> 	status = "okay";
> 
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&edptx_pins_default>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 			edp_in: endpoint {
> 				remote-endpoint = <&dp_intf0_out>;
> 			};
> 		};
> 
> 		port@1 {
> 			reg = <1>;
> 			edp_out: endpoint {
> 				data-lanes = <0 1 2 3>;
> 				remote-endpoint = <&panel_in>;
> 			};
> 		};
> 	};
> 
> 	aux-bus {
> 		panel: panel {
> 			compatible = "edp-panel";
> 			power-supply = <&pp3300_disp_x>;
> 			backlight = <&backlight_lcd0>;
> 			port {
> 				panel_in: endpoint {
> 					remote-endpoint = <&edp_out>;
> 				};
> 			};
> 		};
> 	};
> };
> 
> AngeloGioacchino Del Regno (9):
>    drm/mediatek: dp: Cache EDID for eDP panel
>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
>    drm/mediatek: dp: Always return connected status for eDP in .detect()
>    drm/mediatek: dp: Always set cable_plugged_in at resume for eDP panel
>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
>    drm/mediatek: dp: Enable event interrupt only when bridge attached
>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
>    drm/mediatek: dp: Move AUX_P0 setting to
>      mtk_dp_initialize_aux_settings()
>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> 
>   drivers/gpu/drm/mediatek/mtk_dp.c | 186 +++++++++++++++++++-----------
>   1 file changed, 116 insertions(+), 70 deletions(-)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

On Tue, Apr 04, 2023 at 12:48:00PM +0200, AngeloGioacchino Del Regno wrote:
> For the eDP case we can support using aux-bus on MediaTek DP: this
> gives us the possibility to declare our panel as generic "panel-edp"
> which will automatically configure the timings and available modes
> via the EDID that we read from it.
> 
> To do this, move the panel parsing at the end of the probe function
> so that the hardware is initialized beforehand and also initialize
> the DPTX AUX block and power both on as, when we populate the
> aux-bus, the panel driver will trigger an EDID read to perform
> panel detection.
> 
> Last but not least, since now the AUX transfers can happen in the
> separated aux-bus, it was necessary to add an exclusion for the
> cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> way to do this is to simply ignore checking that when the bridge
> type is eDP.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index a67143c22024..8109f5b4392b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[..]
> @@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  	mtk_dp->need_debounce = true;
>  	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
>  
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
> +		/* Initialize, reset and poweron the DPTX AUX block */
> +		mtk_dp_initialize_aux_settings(mtk_dp);
> +		mtk_dp_power_enable(mtk_dp);
> +
> +		/* Power on the panel to allow EDID read from aux-bus */
> +		mtk_dp_aux_panel_poweron(mtk_dp, true);
> +
> +		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);

This patch causes

.../bin/aarch64-none-linux-gnu-ld: Unexpected GOT/PLT entries detected!
.../bin/aarch64-none-linux-gnu-ld: Unexpected run-time procedure linkages detected!
.../bin/aarch64-none-linux-gnu-ld: drivers/gpu/drm/mediatek/mtk_dp.o: in function `mtk_dp_probe':
.../drivers/gpu/drm/mediatek/mtk_dp.c:2595: undefined reference to `devm_of_dp_aux_populate_bus'

You need

diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig
index b451dee64d34..76cab28e010c 100644
--- a/drivers/gpu/drm/mediatek/Kconfig
+++ b/drivers/gpu/drm/mediatek/Kconfig
@@ -26,6 +26,7 @@ config DRM_MEDIATEK_DP
        select PHY_MTK_DP
        select DRM_DISPLAY_HELPER
        select DRM_DISPLAY_DP_HELPER
+       select DRM_DP_AUX_BUS
        help
          DRM/KMS Display Port driver for MediaTek SoCs.

Thanks,
Nícolas

Re: [PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

On Tue, Apr 04, 2023 at 12:48:00PM +0200, AngeloGioacchino Del Regno wrote:
> For the eDP case we can support using aux-bus on MediaTek DP: this
> gives us the possibility to declare our panel as generic "panel-edp"
> which will automatically configure the timings and available modes
> via the EDID that we read from it.
> 
> To do this, move the panel parsing at the end of the probe function
> so that the hardware is initialized beforehand and also initialize
> the DPTX AUX block and power both on as, when we populate the
> aux-bus, the panel driver will trigger an EDID read to perform
> panel detection.
> 
> Last but not least, since now the AUX transfers can happen in the
> separated aux-bus, it was necessary to add an exclusion for the
> cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> way to do this is to simply ignore checking that when the bridge
> type is eDP.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index a67143c22024..8109f5b4392b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[..]
> @@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  	mtk_dp->need_debounce = true;
>  	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
>  
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
> +		/* Initialize, reset and poweron the DPTX AUX block */
> +		mtk_dp_initialize_aux_settings(mtk_dp);
> +		mtk_dp_power_enable(mtk_dp);
> +
> +		/* Power on the panel to allow EDID read from aux-bus */
> +		mtk_dp_aux_panel_poweron(mtk_dp, true);
> +
> +		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);

This patch causes

.../bin/aarch64-none-linux-gnu-ld: Unexpected GOT/PLT entries detected!
.../bin/aarch64-none-linux-gnu-ld: Unexpected run-time procedure linkages detected!
.../bin/aarch64-none-linux-gnu-ld: drivers/gpu/drm/mediatek/mtk_dp.o: in function `mtk_dp_probe':
.../drivers/gpu/drm/mediatek/mtk_dp.c:2595: undefined reference to `devm_of_dp_aux_populate_bus'

You need

diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig
index b451dee64d34..76cab28e010c 100644
--- a/drivers/gpu/drm/mediatek/Kconfig
+++ b/drivers/gpu/drm/mediatek/Kconfig
@@ -26,6 +26,7 @@ config DRM_MEDIATEK_DP
        select PHY_MTK_DP
        select DRM_DISPLAY_HELPER
        select DRM_DISPLAY_DP_HELPER
+       select DRM_DP_AUX_BUS
        help
          DRM/KMS Display Port driver for MediaTek SoCs.

Thanks,
Nícolas

Re: [PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

On Tue, Apr 04, 2023 at 12:48:00PM +0200, AngeloGioacchino Del Regno wrote:
> For the eDP case we can support using aux-bus on MediaTek DP: this
> gives us the possibility to declare our panel as generic "panel-edp"
> which will automatically configure the timings and available modes
> via the EDID that we read from it.
> 
> To do this, move the panel parsing at the end of the probe function
> so that the hardware is initialized beforehand and also initialize
> the DPTX AUX block and power both on as, when we populate the
> aux-bus, the panel driver will trigger an EDID read to perform
> panel detection.
> 
> Last but not least, since now the AUX transfers can happen in the
> separated aux-bus, it was necessary to add an exclusion for the
> cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> way to do this is to simply ignore checking that when the bridge
> type is eDP.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index a67143c22024..8109f5b4392b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[..]
> @@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  	mtk_dp->need_debounce = true;
>  	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
>  
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
> +		/* Initialize, reset and poweron the DPTX AUX block */
> +		mtk_dp_initialize_aux_settings(mtk_dp);
> +		mtk_dp_power_enable(mtk_dp);
> +
> +		/* Power on the panel to allow EDID read from aux-bus */
> +		mtk_dp_aux_panel_poweron(mtk_dp, true);
> +
> +		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);

This patch causes

.../bin/aarch64-none-linux-gnu-ld: Unexpected GOT/PLT entries detected!
.../bin/aarch64-none-linux-gnu-ld: Unexpected run-time procedure linkages detected!
.../bin/aarch64-none-linux-gnu-ld: drivers/gpu/drm/mediatek/mtk_dp.o: in function `mtk_dp_probe':
.../drivers/gpu/drm/mediatek/mtk_dp.c:2595: undefined reference to `devm_of_dp_aux_populate_bus'

You need

diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig
index b451dee64d34..76cab28e010c 100644
--- a/drivers/gpu/drm/mediatek/Kconfig
+++ b/drivers/gpu/drm/mediatek/Kconfig
@@ -26,6 +26,7 @@ config DRM_MEDIATEK_DP
        select PHY_MTK_DP
        select DRM_DISPLAY_HELPER
        select DRM_DISPLAY_DP_HELPER
+       select DRM_DP_AUX_BUS
        help
          DRM/KMS Display Port driver for MediaTek SoCs.

Thanks,
Nícolas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

On Tue, Apr 04, 2023 at 12:48:00PM +0200, AngeloGioacchino Del Regno wrote:
> For the eDP case we can support using aux-bus on MediaTek DP: this
> gives us the possibility to declare our panel as generic "panel-edp"
> which will automatically configure the timings and available modes
> via the EDID that we read from it.
> 
> To do this, move the panel parsing at the end of the probe function
> so that the hardware is initialized beforehand and also initialize
> the DPTX AUX block and power both on as, when we populate the
> aux-bus, the panel driver will trigger an EDID read to perform
> panel detection.
> 
> Last but not least, since now the AUX transfers can happen in the
> separated aux-bus, it was necessary to add an exclusion for the
> cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> way to do this is to simply ignore checking that when the bridge
> type is eDP.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index a67143c22024..8109f5b4392b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[..]
> @@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  	mtk_dp->need_debounce = true;
>  	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
>  
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
> +		/* Initialize, reset and poweron the DPTX AUX block */
> +		mtk_dp_initialize_aux_settings(mtk_dp);
> +		mtk_dp_power_enable(mtk_dp);
> +
> +		/* Power on the panel to allow EDID read from aux-bus */
> +		mtk_dp_aux_panel_poweron(mtk_dp, true);
> +
> +		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);
> +
> +		/* If the panel is present, detection is done: power off! */
> +		mtk_dp_aux_panel_poweron(mtk_dp, false);
> +		mtk_dp_power_disable(mtk_dp);
> +
> +		/* We ignore -ENODEV error, as the panel may not be on aux-bus */
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +
> +		/*
> +		 * Here we don't ignore any error, as if there's no panel to
> +		 * link, eDP is not configured correctly and will be unusable.
> +		 */
> +		ret = mtk_dp_edp_link_panel(&mtk_dp->aux);

This call might return EDEFER_PROBE if the panel hasn't probed yet. That's a
problem, because during this probe you register a device for the dp-phy, so
you'll be retriggering defer probes every time you probe until the panel probes.
But if this driver was builtin and the panel a module, then this loop will go on
forever.

You should make use of the done_probing callback in
devm_of_dp_aux_populate_bus() and do the panel linking there. This way you can
exit successfully from this probe and avoid the loop. I had to do the same thing
for anx7625.c [1].

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20230518193902.891121-1-nfraprado@collabora.com/

Re: [PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

On Tue, Apr 04, 2023 at 12:48:00PM +0200, AngeloGioacchino Del Regno wrote:
> For the eDP case we can support using aux-bus on MediaTek DP: this
> gives us the possibility to declare our panel as generic "panel-edp"
> which will automatically configure the timings and available modes
> via the EDID that we read from it.
> 
> To do this, move the panel parsing at the end of the probe function
> so that the hardware is initialized beforehand and also initialize
> the DPTX AUX block and power both on as, when we populate the
> aux-bus, the panel driver will trigger an EDID read to perform
> panel detection.
> 
> Last but not least, since now the AUX transfers can happen in the
> separated aux-bus, it was necessary to add an exclusion for the
> cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> way to do this is to simply ignore checking that when the bridge
> type is eDP.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index a67143c22024..8109f5b4392b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[..]
> @@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  	mtk_dp->need_debounce = true;
>  	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
>  
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
> +		/* Initialize, reset and poweron the DPTX AUX block */
> +		mtk_dp_initialize_aux_settings(mtk_dp);
> +		mtk_dp_power_enable(mtk_dp);
> +
> +		/* Power on the panel to allow EDID read from aux-bus */
> +		mtk_dp_aux_panel_poweron(mtk_dp, true);
> +
> +		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);
> +
> +		/* If the panel is present, detection is done: power off! */
> +		mtk_dp_aux_panel_poweron(mtk_dp, false);
> +		mtk_dp_power_disable(mtk_dp);
> +
> +		/* We ignore -ENODEV error, as the panel may not be on aux-bus */
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +
> +		/*
> +		 * Here we don't ignore any error, as if there's no panel to
> +		 * link, eDP is not configured correctly and will be unusable.
> +		 */
> +		ret = mtk_dp_edp_link_panel(&mtk_dp->aux);

This call might return EDEFER_PROBE if the panel hasn't probed yet. That's a
problem, because during this probe you register a device for the dp-phy, so
you'll be retriggering defer probes every time you probe until the panel probes.
But if this driver was builtin and the panel a module, then this loop will go on
forever.

You should make use of the done_probing callback in
devm_of_dp_aux_populate_bus() and do the panel linking there. This way you can
exit successfully from this probe and avoid the loop. I had to do the same thing
for anx7625.c [1].

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20230518193902.891121-1-nfraprado@collabora.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 9/9] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

On Tue, Apr 04, 2023 at 12:48:00PM +0200, AngeloGioacchino Del Regno wrote:
> For the eDP case we can support using aux-bus on MediaTek DP: this
> gives us the possibility to declare our panel as generic "panel-edp"
> which will automatically configure the timings and available modes
> via the EDID that we read from it.
> 
> To do this, move the panel parsing at the end of the probe function
> so that the hardware is initialized beforehand and also initialize
> the DPTX AUX block and power both on as, when we populate the
> aux-bus, the panel driver will trigger an EDID read to perform
> panel detection.
> 
> Last but not least, since now the AUX transfers can happen in the
> separated aux-bus, it was necessary to add an exclusion for the
> cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> way to do this is to simply ignore checking that when the bridge
> type is eDP.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c | 61 ++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index a67143c22024..8109f5b4392b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[..]
> @@ -2571,6 +2585,33 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  	mtk_dp->need_debounce = true;
>  	timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0);
>  
> +	if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
> +		/* Initialize, reset and poweron the DPTX AUX block */
> +		mtk_dp_initialize_aux_settings(mtk_dp);
> +		mtk_dp_power_enable(mtk_dp);
> +
> +		/* Power on the panel to allow EDID read from aux-bus */
> +		mtk_dp_aux_panel_poweron(mtk_dp, true);
> +
> +		ret = devm_of_dp_aux_populate_bus(&mtk_dp->aux, NULL);
> +
> +		/* If the panel is present, detection is done: power off! */
> +		mtk_dp_aux_panel_poweron(mtk_dp, false);
> +		mtk_dp_power_disable(mtk_dp);
> +
> +		/* We ignore -ENODEV error, as the panel may not be on aux-bus */
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +
> +		/*
> +		 * Here we don't ignore any error, as if there's no panel to
> +		 * link, eDP is not configured correctly and will be unusable.
> +		 */
> +		ret = mtk_dp_edp_link_panel(&mtk_dp->aux);

This call might return EDEFER_PROBE if the panel hasn't probed yet. That's a
problem, because during this probe you register a device for the dp-phy, so
you'll be retriggering defer probes every time you probe until the panel probes.
But if this driver was builtin and the panel a module, then this loop will go on
forever.

You should make use of the done_probing callback in
devm_of_dp_aux_populate_bus() and do the panel linking there. This way you can
exit successfully from this probe and avoid the loop. I had to do the same thing
for anx7625.c [1].

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20230518193902.891121-1-nfraprado@collabora.com/