[PATCH 0/3] Fix Cx0 Suspend Resume issue

[PATCH 0/3] Fix Cx0 Suspend Resume issue

[Suraj Kandpal]

CX0 PHY currently has two issues which cause a hang when we try
to suspend resume machine with a delay of 15mins and 1+ hour.
This happens due to two reasons:
1) We do not follow the Enablement sequence where we need to
enable our clock after PPS Enablement cycle
2) We do not make sure response ready and error bit are cleared
in P2M_MSGBUS_STATUS before writing the transaction pending bit.
This series aims to solve this.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>

Mika Kahola (1):
  drm/i915/cx0: Split PLL enabling/disabling in two parts

Suraj Kandpal (2):
  drm/i915/cx0: Move step 12 to enable clock hook
  drm/i915/cx0: Clear response ready & error bit

 drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 122 +++++++++++-------
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  12 +-
 2 files changed, 84 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Suraj Kandpal]

From: Mika Kahola <mika.kahola@intel.com>

Split PLL enabling/disabling in two parts - one for pll setting
pll dividers and second one to enable/disable pll clock. PLL
clock enabling/disbling happens via encoder->enable_clock/disable_clock
function hook. The reason for doing this is that we need to make sure
the clock enablement happens after PPS ON step to be inline with the
sequences which we end up violating otherwise. As a result of this
violation we end up in a hanged state if machine stays idle for more
that 15 mins.

PLL state verification happens now earlier than the clock is enabled
which causes a drm warn to be thrown. Silence this warning by
allowing this check for only earlier platforms than MeteorLake.

Bspec: 49190
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87 ++++++++++++-------
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +--
 2 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 7288065d2461..f3baba264e88 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 {
 	int port_clock = pll_state->use_c10 ? pll_state->c10.clock : pll_state->c20.clock;
 	struct intel_display *display = to_intel_display(encoder);
-	enum phy phy = intel_encoder_to_phy(encoder);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	bool lane_reversal = dig_port->lane_reversal;
-	u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
-					  INTEL_CX0_LANE0;
 	struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
 
 	/*
@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 	 */
 	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
 
-	/*
-	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
-	 * LN<Lane for maxPCLK> to "1" to enable PLL.
-	 */
-	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
-		     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
-		     intel_cx0_get_pclk_pll_request(maxpclk_lane));
-
-	/* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
-	if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
-			     intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
-			     intel_cx0_get_pclk_pll_ack(maxpclk_lane),
-			     XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
-		drm_warn(display->drm, "Port %c PLL not locked\n",
-			 phy_name(phy));
-
-	/*
-	 * 11. Follow the Display Voltage Frequency Switching Sequence After
-	 * Frequency Change. We handle this step in bxt_set_cdclk().
-	 */
-
 	/*
 	 * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
 	 *
@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct intel_display *display,
 	}
 }
 
+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
+{
+	struct intel_display *display = to_intel_display(encoder);
+	enum phy phy = intel_encoder_to_phy(encoder);
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	bool lane_reversal = dig_port->lane_reversal;
+					  INTEL_CX0_LANE0;
+	u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
+					INTEL_CX0_LANE0;
+
+	struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
+
+	/*
+	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
+	 * LN<Lane for maxPCLK> to "1" to enable PLL.
+	 */
+	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
+		     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
+		     intel_cx0_get_pclk_pll_request(maxpclk_lane));
+
+	/* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
+	if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
+			     intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
+			     intel_cx0_get_pclk_pll_ack(maxpclk_lane),
+			     XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
+		drm_warn(display->drm, "Port %c PLL not locked\n",
+			 phy_name(phy));
+
+	/*
+	 * 11. Follow the Display Voltage Frequency Switching Sequence After
+	 * Frequency Change. We handle this step in bxt_set_cdclk().
+	 */
+
+	intel_cx0_phy_transaction_end(encoder, wakeref);
+}
+
 void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int port_clock)
 {
 	struct intel_display *display = to_intel_display(encoder);
@@ -3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct intel_encoder *encoder,
 
 	if (intel_tc_port_in_tbt_alt_mode(dig_port))
 		intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
+	else
+		intel_cx0pll_enable_clock(encoder);
 }
 
 /*
@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct intel_encoder *encoder)
 	 * Frequency Change. We handle this step in bxt_set_cdclk().
 	 */
 
-	/* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
-	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
-		     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
-	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
-		     XELPDP_FORWARD_CLOCK_UNGATE, 0);
-
 	intel_cx0_phy_transaction_end(encoder, wakeref);
 }
 
@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct intel_encoder *encoder)
 			     intel_cx0_get_pclk_pll_request(lane);
 }
 
+static void intel_cx0pll_disable_clock(struct intel_encoder *encoder)
+{
+	struct intel_display *display = to_intel_display(encoder);
+	struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
+
+	/* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
+	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
+		     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
+	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
+		     XELPDP_FORWARD_CLOCK_UNGATE, 0);
+
+	intel_cx0_phy_transaction_end(encoder, wakeref);
+}
+
 void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)
 {
 	struct intel_display *display = to_intel_display(encoder);
@@ -3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct intel_encoder *encoder)
 
 	if (intel_tc_port_in_tbt_alt_mode(dig_port))
 		intel_mtl_tbt_pll_disable_clock(encoder);
+	else
+		intel_cx0pll_disable_clock(encoder);
+
 }
 
 enum icl_port_dpll_id
@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
 			    encoder->base.base.id, encoder->base.name);
 
 		intel_cx0pll_enable(encoder, &pll_state);
+		intel_cx0pll_enable_clock(encoder);
 		intel_cx0pll_disable(encoder);
+		intel_cx0pll_disable_clock(encoder);
 	}
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 9aa84a430f09..59395076103c 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
 		     "asserting DPLL %s with no DPLL\n", str_on_off(state)))
 		return;
 
-	cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
-	INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
-				 "%s assertion failure (expected %s, current %s)\n",
-				 pll->info->name, str_on_off(state),
-				 str_on_off(cur_state));
+	if (DISPLAY_VER(display) < 14) {
+		cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
+		INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
+					 "%s assertion failure (expected %s, current %s)\n",
+					 pll->info->name, str_on_off(state),
+					 str_on_off(cur_state));
+	}
 }
 
 static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
-- 
2.34.1

[PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook

[Suraj Kandpal]

Move the step to toggle powerdown sequence change for HDMI to enable
clock hook where it belongs according to its sequence.
Do the required changes to make that work.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 37 ++++++++++----------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index f3baba264e88..5edd293b533b 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -3281,21 +3281,6 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
 	 */
 	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
 
-	/*
-	 * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
-	 *
-	 * Wa_13013502646:
-	 * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
-	 * Workaround: Toggle powerdown value by setting first to P0 and then to P2, for both
-	 * PHY lanes.
-	 */
-	if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
-		intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
-						    XELPDP_P0_STATE_ACTIVE);
-		intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
-						    XELPDP_P2_STATE_READY);
-	}
-
 	intel_cx0_phy_transaction_end(encoder, wakeref);
 }
 
@@ -3379,7 +3364,8 @@ static int intel_mtl_tbt_clock_select(struct intel_display *display,
 	}
 }
 
-static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder,
+				      const struct intel_cx0pll_state *pll_state)
 {
 	struct intel_display *display = to_intel_display(encoder);
 	enum phy phy = intel_encoder_to_phy(encoder);
@@ -3412,6 +3398,21 @@ static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
 	 * Frequency Change. We handle this step in bxt_set_cdclk().
 	 */
 
+	/*
+	 * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
+	 *
+	 * Wa_13013502646:
+	 * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
+	 * Workaround: Toggle powerdown value by setting first to P0 and then to P2, for both
+	 * PHY lanes.
+	 */
+	if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
+		intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
+						    XELPDP_P0_STATE_ACTIVE);
+		intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
+						    XELPDP_P2_STATE_READY);
+	}
+
 	intel_cx0_phy_transaction_end(encoder, wakeref);
 }
 
@@ -3485,7 +3486,7 @@ void intel_mtl_pll_enable_clock(struct intel_encoder *encoder,
 	if (intel_tc_port_in_tbt_alt_mode(dig_port))
 		intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
 	else
-		intel_cx0pll_enable_clock(encoder);
+		intel_cx0pll_enable_clock(encoder, &crtc_state->dpll_hw_state.cx0pll);
 }
 
 /*
@@ -3808,7 +3809,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
 			    encoder->base.base.id, encoder->base.name);
 
 		intel_cx0pll_enable(encoder, &pll_state);
-		intel_cx0pll_enable_clock(encoder);
+		intel_cx0pll_enable_clock(encoder, &pll_state);
 		intel_cx0pll_disable(encoder);
 		intel_cx0pll_disable_clock(encoder);
 	}
-- 
2.34.1

[PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Suraj Kandpal]

Clear the response ready and error bit of PORT_P2M_MESSAGE_BUS_STATUS
before writing the transaction pending bit of
PORT_M2P_MSGBUS_CTL as that is a hard requirement. If not done
we find that the PHY hangs since it ends up in a weird state if left
idle for more than 1 hour.

Bspec: 65101
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 5edd293b533b..5ebc3404eee2 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct intel_encoder *encoder,
 		return -ETIMEDOUT;
 	}
 
+	intel_clear_response_ready_flag(encoder, lane);
+
 	intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port, lane),
 		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
 		       XELPDP_PORT_M2P_COMMAND_READ |
@@ -293,6 +295,8 @@ static int __intel_cx0_write_once(struct intel_encoder *encoder,
 		return -ETIMEDOUT;
 	}
 
+	intel_clear_response_ready_flag(encoder, lane);
+
 	intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port, lane),
 		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
 		       (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
-- 
2.34.1

✗ Fi.CI.BUILD: failure for Fix Cx0 Suspend Resume issue

[Patchwork]

== Series Details ==

Series: Fix Cx0 Suspend Resume issue
URL   : https://patchwork.freedesktop.org/series/159540/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/intel_cx0_phy.o
In file included from ./include/linux/bits.h:5,
                 from ./include/linux/bitops.h:6,
                 from ./include/linux/log2.h:12,
                 from drivers/gpu/drm/i915/display/intel_cx0_phy.c:6:
drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘intel_cx0pll_enable_clock’:
./include/vdso/bits.h:7:33: error: statement with no effect [-Werror=unused-value]
    7 | #define BIT(nr)                 (UL(1) << (nr))
      |                                 ^
drivers/gpu/drm/i915/display/intel_cx0_phy.c:32:33: note: in expansion of macro ‘BIT’
   32 | #define INTEL_CX0_LANE0         BIT(0)
      |                                 ^~~
drivers/gpu/drm/i915/display/intel_cx0_phy.c:3378:43: note: in expansion of macro ‘INTEL_CX0_LANE0’
 3378 |                                           INTEL_CX0_LANE0;
      |                                           ^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[6]: *** [scripts/Makefile.build:287: drivers/gpu/drm/i915/display/intel_cx0_phy.o] Error 1
make[5]: *** [scripts/Makefile.build:556: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:556: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:556: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:556: drivers] Error 2
make[1]: *** [/home/kbuild2/kernel/Makefile:2054: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2
Build failed, no error log produced

Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Gustavo Sousa]

Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00)
>From: Mika Kahola <mika.kahola@intel.com>
>
>Split PLL enabling/disabling in two parts - one for pll setting
>pll dividers and second one to enable/disable pll clock. PLL
>clock enabling/disbling happens via encoder->enable_clock/disable_clock
>function hook. The reason for doing this is that we need to make sure
>the clock enablement happens after PPS ON step to be inline with the
>sequences which we end up violating otherwise. As a result of this
>violation we end up in a hanged state if machine stays idle for more
>that 15 mins.

So, it appears this started happening when we Cx0 code was integrated
into the DPLL framework and then the driver started enabling the PHY
PLL/clock too early, right?

I am lacking some context/background here due to my unfamiliarity with
pre-MTL platforms, but why I exactly do we program the PLLs before the
modeset sequence?  Is it related to the shared nature of PLLs for
platforms pre-C10/pre-C20?  If so, do we really need to do the same for
C10/C20 PHYs, since we have dedicated PLLs for them?

(Sorry for asking here and a bit too late.  Probably the better place to
ask this was in series that integrated Cx0 into the DPLL framework.)

>
>PLL state verification happens now earlier than the clock is enabled
>which causes a drm warn to be thrown. Silence this warning by
>allowing this check for only earlier platforms than MeteorLake.
>
>Bspec: 49190

This Bspec page is not invalid for platforms using C10/C20 PHYs.

We probably want to use these instead:

Bspec: 65448, 68849

--
Gustavo Sousa

>Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87 ++++++++++++-------
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +--
> 2 files changed, 64 insertions(+), 35 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index 7288065d2461..f3baba264e88 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> {
>         int port_clock = pll_state->use_c10 ? pll_state->c10.clock : pll_state->c20.clock;
>         struct intel_display *display = to_intel_display(encoder);
>-        enum phy phy = intel_encoder_to_phy(encoder);
>         struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>         bool lane_reversal = dig_port->lane_reversal;
>-        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
>-                                          INTEL_CX0_LANE0;
>         struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
> 
>         /*
>@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>          */
>         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
> 
>-        /*
>-         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>-         * LN<Lane for maxPCLK> to "1" to enable PLL.
>-         */
>-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>-                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>-                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
>-
>-        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
>-        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>-                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>-                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>-                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
>-                drm_warn(display->drm, "Port %c PLL not locked\n",
>-                         phy_name(phy));
>-
>-        /*
>-         * 11. Follow the Display Voltage Frequency Switching Sequence After
>-         * Frequency Change. We handle this step in bxt_set_cdclk().
>-         */
>-
>         /*
>          * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>          *
>@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct intel_display *display,
>         }
> }
> 
>+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
>+{
>+        struct intel_display *display = to_intel_display(encoder);
>+        enum phy phy = intel_encoder_to_phy(encoder);
>+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>+        bool lane_reversal = dig_port->lane_reversal;
>+                                          INTEL_CX0_LANE0;
>+        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
>+                                        INTEL_CX0_LANE0;
>+
>+        struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
>+
>+        /*
>+         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>+         * LN<Lane for maxPCLK> to "1" to enable PLL.
>+         */
>+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>+                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>+                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
>+
>+        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
>+        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>+                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>+                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>+                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
>+                drm_warn(display->drm, "Port %c PLL not locked\n",
>+                         phy_name(phy));
>+
>+        /*
>+         * 11. Follow the Display Voltage Frequency Switching Sequence After
>+         * Frequency Change. We handle this step in bxt_set_cdclk().
>+         */
>+
>+        intel_cx0_phy_transaction_end(encoder, wakeref);
>+}
>+
> void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int port_clock)
> {
>         struct intel_display *display = to_intel_display(encoder);
>@@ -3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct intel_encoder *encoder,
> 
>         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>                 intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
>+        else
>+                intel_cx0pll_enable_clock(encoder);
> }
> 
> /*
>@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct intel_encoder *encoder)
>          * Frequency Change. We handle this step in bxt_set_cdclk().
>          */
> 
>-        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
>-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>-                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>-                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
>-
>         intel_cx0_phy_transaction_end(encoder, wakeref);
> }
> 
>@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct intel_encoder *encoder)
>                              intel_cx0_get_pclk_pll_request(lane);
> }
> 
>+static void intel_cx0pll_disable_clock(struct intel_encoder *encoder)
>+{
>+        struct intel_display *display = to_intel_display(encoder);
>+        struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
>+
>+        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
>+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>+                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
>+
>+        intel_cx0_phy_transaction_end(encoder, wakeref);
>+}
>+
> void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)
> {
>         struct intel_display *display = to_intel_display(encoder);
>@@ -3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct intel_encoder *encoder)
> 
>         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>                 intel_mtl_tbt_pll_disable_clock(encoder);
>+        else
>+                intel_cx0pll_disable_clock(encoder);
>+
> }
> 
> enum icl_port_dpll_id
>@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
>                             encoder->base.base.id, encoder->base.name);
> 
>                 intel_cx0pll_enable(encoder, &pll_state);
>+                intel_cx0pll_enable_clock(encoder);
>                 intel_cx0pll_disable(encoder);
>+                intel_cx0pll_disable_clock(encoder);
>         }
> }
>diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>index 9aa84a430f09..59395076103c 100644
>--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
>                      "asserting DPLL %s with no DPLL\n", str_on_off(state)))
>                 return;
> 
>-        cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
>-        INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
>-                                 "%s assertion failure (expected %s, current %s)\n",
>-                                 pll->info->name, str_on_off(state),
>-                                 str_on_off(cur_state));
>+        if (DISPLAY_VER(display) < 14) {
>+                cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
>+                INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
>+                                         "%s assertion failure (expected %s, current %s)\n",
>+                                         pll->info->name, str_on_off(state),
>+                                         str_on_off(cur_state));
>+        }
> }
> 
> static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>-- 
>2.34.1
>

Re: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook

[Gustavo Sousa]

Quoting Suraj Kandpal (2025-12-30 05:31:41-03:00)
>Move the step to toggle powerdown sequence change for HDMI to enable
>clock hook where it belongs according to its sequence.
>Do the required changes to make that work.

This should probably be a squash into the previous patch?

--
Gustavo Sousa

>
>Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 37 ++++++++++----------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index f3baba264e88..5edd293b533b 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -3281,21 +3281,6 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>          */
>         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
> 
>-        /*
>-         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>-         *
>-         * Wa_13013502646:
>-         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
>-         * Workaround: Toggle powerdown value by setting first to P0 and then to P2, for both
>-         * PHY lanes.
>-         */
>-        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
>-                intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
>-                                                    XELPDP_P0_STATE_ACTIVE);
>-                intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
>-                                                    XELPDP_P2_STATE_READY);
>-        }
>-
>         intel_cx0_phy_transaction_end(encoder, wakeref);
> }
> 
>@@ -3379,7 +3364,8 @@ static int intel_mtl_tbt_clock_select(struct intel_display *display,
>         }
> }
> 
>-static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
>+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder,
>+                                      const struct intel_cx0pll_state *pll_state)
> {
>         struct intel_display *display = to_intel_display(encoder);
>         enum phy phy = intel_encoder_to_phy(encoder);
>@@ -3412,6 +3398,21 @@ static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
>          * Frequency Change. We handle this step in bxt_set_cdclk().
>          */
> 
>+        /*
>+         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>+         *
>+         * Wa_13013502646:
>+         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
>+         * Workaround: Toggle powerdown value by setting first to P0 and then to P2, for both
>+         * PHY lanes.
>+         */
>+        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
>+                intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
>+                                                    XELPDP_P0_STATE_ACTIVE);
>+                intel_cx0_powerdown_change_sequence(encoder, INTEL_CX0_BOTH_LANES,
>+                                                    XELPDP_P2_STATE_READY);
>+        }
>+
>         intel_cx0_phy_transaction_end(encoder, wakeref);
> }
> 
>@@ -3485,7 +3486,7 @@ void intel_mtl_pll_enable_clock(struct intel_encoder *encoder,
>         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>                 intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
>         else
>-                intel_cx0pll_enable_clock(encoder);
>+                intel_cx0pll_enable_clock(encoder, &crtc_state->dpll_hw_state.cx0pll);
> }
> 
> /*
>@@ -3808,7 +3809,7 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
>                             encoder->base.base.id, encoder->base.name);
> 
>                 intel_cx0pll_enable(encoder, &pll_state);
>-                intel_cx0pll_enable_clock(encoder);
>+                intel_cx0pll_enable_clock(encoder, &pll_state);
>                 intel_cx0pll_disable(encoder);
>                 intel_cx0pll_disable_clock(encoder);
>         }
>-- 
>2.34.1
>

Re: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Jani Nikula]

On Tue, 30 Dec 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Clear the response ready and error bit of PORT_P2M_MESSAGE_BUS_STATUS
> before writing the transaction pending bit of
> PORT_M2P_MSGBUS_CTL as that is a hard requirement. If not done
> we find that the PHY hangs since it ends up in a weird state if left
> idle for more than 1 hour.
>
> Bspec: 65101
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 5edd293b533b..5ebc3404eee2 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct intel_encoder *encoder,
>  		return -ETIMEDOUT;
>  	}
>  
> +	intel_clear_response_ready_flag(encoder, lane);
> +

Please rename the non-static function. Nobody knows what that's related
to without looking at the function.

BR,
Jani.

>  	intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port, lane),
>  		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
>  		       XELPDP_PORT_M2P_COMMAND_READ |
> @@ -293,6 +295,8 @@ static int __intel_cx0_write_once(struct intel_encoder *encoder,
>  		return -ETIMEDOUT;
>  	}
>  
> +	intel_clear_response_ready_flag(encoder, lane);
> +
>  	intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port, lane),
>  		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
>  		       (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :

-- 
Jani Nikula, Intel

Re: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Gustavo Sousa]

Quoting Suraj Kandpal (2025-12-30 05:31:42-03:00)
>Clear the response ready and error bit of PORT_P2M_MESSAGE_BUS_STATUS
>before writing the transaction pending bit of
>PORT_M2P_MSGBUS_CTL as that is a hard requirement. If not done
>we find that the PHY hangs since it ends up in a weird state if left
>idle for more than 1 hour.

Since the series title refers to suspend/resume, is there an easy way of
reproducing this via some power state transition?

I'm wondering if we are looking at a driver issue here or if this is
really something else.  I see that we usually call intel_cx0_bus_reset()
in error paths, which contains a call to
intel_clear_response_ready_flag(), but it could end up being not called
if the reset times out.

Do we see error messages from the driver when the PHY hangs?

--
Gustavo Sousa

>
>Bspec: 65101
>Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index 5edd293b533b..5ebc3404eee2 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct intel_encoder *encoder,
>                 return -ETIMEDOUT;
>         }
> 
>+        intel_clear_response_ready_flag(encoder, lane);
>+
>         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port, lane),
>                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
>                        XELPDP_PORT_M2P_COMMAND_READ |
>@@ -293,6 +295,8 @@ static int __intel_cx0_write_once(struct intel_encoder *encoder,
>                 return -ETIMEDOUT;
>         }
> 
>+        intel_clear_response_ready_flag(encoder, lane);
>+
>         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port, lane),
>                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
>                        (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
>-- 
>2.34.1
>

Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-i915/for-linux-next]
[also build test WARNING on drm-tip/drm-tip]
[cannot apply to drm-i915/for-linux-next-fixes linus/master v6.19-rc3 next-20251219]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-cx0-Split-PLL-enabling-disabling-in-two-parts/20251230-163308
base:   https://gitlab.freedesktop.org/drm/i915/kernel.git for-linux-next
patch link:    https://lore.kernel.org/r/20251230083142.70064-2-suraj.kandpal%40intel.com
patch subject: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts
config: x86_64-rhel-9.4-kunit (https://download.01.org/0day-ci/archive/20251230/202512302233.vcqoghOz-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251230/202512302233.vcqoghOz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512302233.vcqoghOz-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/bits.h:5,
                    from include/linux/bitops.h:6,
                    from include/linux/log2.h:12,
                    from drivers/gpu/drm/i915/display/intel_cx0_phy.c:6:
   drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function 'intel_cx0pll_enable_clock':
>> include/vdso/bits.h:7:33: warning: statement with no effect [-Wunused-value]
       7 | #define BIT(nr)                 (UL(1) << (nr))
         |                                 ^
   drivers/gpu/drm/i915/display/intel_cx0_phy.c:32:33: note: in expansion of macro 'BIT'
      32 | #define INTEL_CX0_LANE0         BIT(0)
         |                                 ^~~
   drivers/gpu/drm/i915/display/intel_cx0_phy.c:3388:43: note: in expansion of macro 'INTEL_CX0_LANE0'
    3388 |                                           INTEL_CX0_LANE0;
         |                                           ^~~~~~~~~~~~~~~
--
   In file included from include/linux/bits.h:5,
                    from include/linux/bitops.h:6,
                    from include/linux/log2.h:12,
                    from display/intel_cx0_phy.c:6:
   display/intel_cx0_phy.c: In function 'intel_cx0pll_enable_clock':
>> include/vdso/bits.h:7:33: warning: statement with no effect [-Wunused-value]
       7 | #define BIT(nr)                 (UL(1) << (nr))
         |                                 ^
   display/intel_cx0_phy.c:32:33: note: in expansion of macro 'BIT'
      32 | #define INTEL_CX0_LANE0         BIT(0)
         |                                 ^~~
   display/intel_cx0_phy.c:3388:43: note: in expansion of macro 'INTEL_CX0_LANE0'
    3388 |                                           INTEL_CX0_LANE0;
         |                                           ^~~~~~~~~~~~~~~


vim +7 include/vdso/bits.h

3945ff37d2f48d Vincenzo Frascino 2020-03-20  6  
3945ff37d2f48d Vincenzo Frascino 2020-03-20 @7  #define BIT(nr)			(UL(1) << (nr))
cbdb1f163af2bb Andy Shevchenko   2022-11-28  8  #define BIT_ULL(nr)		(ULL(1) << (nr))
3945ff37d2f48d Vincenzo Frascino 2020-03-20  9  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Kandpal, Suraj]

> Subject: Re: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit
> 
> Quoting Suraj Kandpal (2025-12-30 05:31:42-03:00)
> >Clear the response ready and error bit of PORT_P2M_MESSAGE_BUS_STATUS
> >before writing the transaction pending bit of PORT_M2P_MSGBUS_CTL as
> >that is a hard requirement. If not done we find that the PHY hangs
> >since it ends up in a weird state if left idle for more than 1 hour.
> 
> Since the series title refers to suspend/resume, is there an easy way of
> reproducing this via some power state transition?

Sadly no we did try to no avail went as low as DC9 too. Tried sleep then wake,
Tried hibernating too.

> 
> I'm wondering if we are looking at a driver issue here or if this is really
> something else.  I see that we usually call intel_cx0_bus_reset() in error paths,
> which contains a call to intel_clear_response_ready_flag(), but it could end up
> being not called if the reset times out.

Yes the reset times out and this is because PHY is in hanged state here

> 
> Do we see error messages from the driver when the PHY hangs?

Yes we do see a PHY hang when we are at the stage of swing programming setting
And the error happens when we try to read the msg access bit itself which is because the PHY ends up in
Weird state where it has a write ack but no response ready bit getting set.
But with my observation I can certainly say this is PHY issue.
I also double confirmed this behavior with windows folks and their code where they said they saw an issue in the same lines.
Also the bspec basically adds this as a disclaimer that response ready and error bit should be cleared no matter what before writing
The transaction pending bit.

Regards,
Suraj Kandpal

> 
> --
> Gustavo Sousa
> 
> >
> >Bspec: 65101
> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >index 5edd293b533b..5ebc3404eee2 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >@@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct intel_encoder
> *encoder,
> >                 return -ETIMEDOUT;
> >         }
> >
> >+        intel_clear_response_ready_flag(encoder, lane);
> >+
> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port,
> lane),
> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >                        XELPDP_PORT_M2P_COMMAND_READ | @@ -293,6 +295,8
> >@@ static int __intel_cx0_write_once(struct intel_encoder *encoder,
> >                 return -ETIMEDOUT;
> >         }
> >
> >+        intel_clear_response_ready_flag(encoder, lane);
> >+
> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port,
> lane),
> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >                        (committed ?
> XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
> >--
> >2.34.1
> >

RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Kandpal, Suraj]

> Subject: Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two
> parts
> 
> Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00)
> >From: Mika Kahola <mika.kahola@intel.com>
> >
> >Split PLL enabling/disabling in two parts - one for pll setting pll
> >dividers and second one to enable/disable pll clock. PLL clock
> >enabling/disbling happens via encoder->enable_clock/disable_clock
> >function hook. The reason for doing this is that we need to make sure
> >the clock enablement happens after PPS ON step to be inline with the
> >sequences which we end up violating otherwise. As a result of this
> >violation we end up in a hanged state if machine stays idle for more
> >that 15 mins.
> 
> So, it appears this started happening when we Cx0 code was integrated into
> the DPLL framework and then the driver started enabling the PHY PLL/clock
> too early, right?
> 
> I am lacking some context/background here due to my unfamiliarity with pre-
> MTL platforms, but why I exactly do we program the PLLs before the modeset
> sequence?  Is it related to the shared nature of PLLs for platforms pre-
> C10/pre-C20?  If so, do we really need to do the same for
> C10/C20 PHYs, since we have dedicated PLLs for them?
> 
> (Sorry for asking here and a bit too late.  Probably the better place to ask this
> was in series that integrated Cx0 into the DPLL framework.)


Right it used to be actually because of the shared nature of PLL's. With c10 c20 we moved
to a different framework where we called the the sequence together using hooks like enable_clock
and disable_clock since there was not a lot of time of time to refactor the dpll_shared_framework to
a framework with supported individual ones.
Now that we had time we shifted cx0 back to the previous framework but missed defer the clock enablement
To later during enable clock time so that we honor the sequence, why we had to do this is even though its not shared PLL anymore is
To make sure this framework is backward compatible too.
Also we had to move cx0 pll framework back to dpll framework because the previous can work well as long as the ports are static hence aren’t
As future proof , we plan to move LT PHY back here too once this ages well.

> 
> >
> >PLL state verification happens now earlier than the clock is enabled
> >which causes a drm warn to be thrown. Silence this warning by allowing
> >this check for only earlier platforms than MeteorLake.
> >
> >Bspec: 49190
> 
> This Bspec page is not invalid for platforms using C10/C20 PHYs.
> 
> We probably want to use these instead:
> 
> Bspec: 65448, 68849
>

Sure will replace them.

Regards,
Suraj Kandpal
 
> --
> Gustavo Sousa
> 
> >Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87 ++++++++++++-------
> >drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +--
> > 2 files changed, 64 insertions(+), 35 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >index 7288065d2461..f3baba264e88 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct
> >intel_encoder *encoder,  {
> >         int port_clock = pll_state->use_c10 ? pll_state->c10.clock : pll_state-
> >c20.clock;
> >         struct intel_display *display = to_intel_display(encoder);
> >-        enum phy phy = intel_encoder_to_phy(encoder);
> >         struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >         bool lane_reversal = dig_port->lane_reversal;
> >-        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> >-                                          INTEL_CX0_LANE0;
> >         struct ref_tracker *wakeref =
> >intel_cx0_phy_transaction_begin(encoder);
> >
> >         /*
> >@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct
> intel_encoder *encoder,
> >          */
> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> >port_clock);
> >
> >-        /*
> >-         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> >-         * LN<Lane for maxPCLK> to "1" to enable PLL.
> >-         */
> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >-                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> >-                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> >-
> >-        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
> == "1". */
> >-        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
> encoder->port),
> >-                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> >-                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> >-                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> >-                drm_warn(display->drm, "Port %c PLL not locked\n",
> >-                         phy_name(phy));
> >-
> >-        /*
> >-         * 11. Follow the Display Voltage Frequency Switching Sequence After
> >-         * Frequency Change. We handle this step in bxt_set_cdclk().
> >-         */
> >-
> >         /*
> >          * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >          *
> >@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct
> intel_display *display,
> >         }
> > }
> >
> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder) {
> >+        struct intel_display *display = to_intel_display(encoder);
> >+        enum phy phy = intel_encoder_to_phy(encoder);
> >+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >+        bool lane_reversal = dig_port->lane_reversal;
> >+                                          INTEL_CX0_LANE0;
> >+        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> >+                                        INTEL_CX0_LANE0;
> >+
> >+        struct ref_tracker *wakeref =
> >+ intel_cx0_phy_transaction_begin(encoder);
> >+
> >+        /*
> >+         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> >+         * LN<Lane for maxPCLK> to "1" to enable PLL.
> >+         */
> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >+                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> >+                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> >+
> >+        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
> == "1". */
> >+        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
> encoder->port),
> >+                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> >+                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> >+                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> >+                drm_warn(display->drm, "Port %c PLL not locked\n",
> >+                         phy_name(phy));
> >+
> >+        /*
> >+         * 11. Follow the Display Voltage Frequency Switching Sequence After
> >+         * Frequency Change. We handle this step in bxt_set_cdclk().
> >+         */
> >+
> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
> >+
> > void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int
> >port_clock)  {
> >         struct intel_display *display = to_intel_display(encoder); @@
> >-3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct intel_encoder
> >*encoder,
> >
> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >                 intel_mtl_tbt_pll_enable_clock(encoder,
> > crtc_state->port_clock);
> >+        else
> >+                intel_cx0pll_enable_clock(encoder);
> > }
> >
> > /*
> >@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct
> intel_encoder *encoder)
> >          * Frequency Change. We handle this step in bxt_set_cdclk().
> >          */
> >
> >-        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >-                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >-                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> >-
> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
> >
> >@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct
> intel_encoder *encoder)
> >                              intel_cx0_get_pclk_pll_request(lane);
> > }
> >
> >+static void intel_cx0pll_disable_clock(struct intel_encoder *encoder)
> >+{
> >+        struct intel_display *display = to_intel_display(encoder);
> >+        struct ref_tracker *wakeref =
> >+intel_cx0_phy_transaction_begin(encoder);
> >+
> >+        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >port),
> >+                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> >+
> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
> >+
> > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)  {
> >         struct intel_display *display = to_intel_display(encoder); @@
> >-3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct
> >intel_encoder *encoder)
> >
> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >                 intel_mtl_tbt_pll_disable_clock(encoder);
> >+        else
> >+                intel_cx0pll_disable_clock(encoder);
> >+
> > }
> >
> > enum icl_port_dpll_id
> >@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct
> intel_display *display)
> >                             encoder->base.base.id,
> >encoder->base.name);
> >
> >                 intel_cx0pll_enable(encoder, &pll_state);
> >+                intel_cx0pll_enable_clock(encoder);
> >                 intel_cx0pll_disable(encoder);
> >+                intel_cx0pll_disable_clock(encoder);
> >         }
> > }
> >diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >index 9aa84a430f09..59395076103c 100644
> >--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
> >                      "asserting DPLL %s with no DPLL\n", str_on_off(state)))
> >                 return;
> >
> >-        cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> >-        INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> >-                                 "%s assertion failure (expected %s, current %s)\n",
> >-                                 pll->info->name, str_on_off(state),
> >-                                 str_on_off(cur_state));
> >+        if (DISPLAY_VER(display) < 14) {
> >+                cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> >+                INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> >+                                         "%s assertion failure (expected %s, current %s)\n",
> >+                                         pll->info->name, str_on_off(state),
> >+                                         str_on_off(cur_state));
> >+        }
> > }
> >
> > static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
> >--
> >2.34.1
> >

RE: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook

[Kandpal, Suraj]

> 
> Quoting Suraj Kandpal (2025-12-30 05:31:41-03:00)
> >Move the step to toggle powerdown sequence change for HDMI to enable
> >clock hook where it belongs according to its sequence.
> >Do the required changes to make that work.
> 
> This should probably be a squash into the previous patch?

So reason for separate patch is that this requires me changing the argument of
clock enable which is not because of the same logical reason that changes are being done in patch 1,
hence a separate patch for changes that were brought about due to another reason. Had this been just movement
of step 12 then I would have squashed them.

Regards,
Suraj Kandpal

> 
> --
> Gustavo Sousa
> 
> >
> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 37 ++++++++++----------
> > 1 file changed, 19 insertions(+), 18 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >index f3baba264e88..5edd293b533b 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >@@ -3281,21 +3281,6 @@ static void intel_cx0pll_enable(struct
> intel_encoder *encoder,
> >          */
> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> >port_clock);
> >
> >-        /*
> >-         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >-         *
> >-         * Wa_13013502646:
> >-         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
> >-         * Workaround: Toggle powerdown value by setting first to P0 and then
> to P2, for both
> >-         * PHY lanes.
> >-         */
> >-        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
> >-                intel_cx0_powerdown_change_sequence(encoder,
> INTEL_CX0_BOTH_LANES,
> >-                                                    XELPDP_P0_STATE_ACTIVE);
> >-                intel_cx0_powerdown_change_sequence(encoder,
> INTEL_CX0_BOTH_LANES,
> >-                                                    XELPDP_P2_STATE_READY);
> >-        }
> >-
> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
> >
> >@@ -3379,7 +3364,8 @@ static int intel_mtl_tbt_clock_select(struct
> intel_display *display,
> >         }
> > }
> >
> >-static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder,
> >+                                      const struct intel_cx0pll_state
> >+*pll_state)
> > {
> >         struct intel_display *display = to_intel_display(encoder);
> >         enum phy phy = intel_encoder_to_phy(encoder); @@ -3412,6
> >+3398,21 @@ static void intel_cx0pll_enable_clock(struct intel_encoder
> *encoder)
> >          * Frequency Change. We handle this step in bxt_set_cdclk().
> >          */
> >
> >+        /*
> >+         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >+         *
> >+         * Wa_13013502646:
> >+         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
> >+         * Workaround: Toggle powerdown value by setting first to P0 and then
> to P2, for both
> >+         * PHY lanes.
> >+         */
> >+        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
> >+                intel_cx0_powerdown_change_sequence(encoder,
> INTEL_CX0_BOTH_LANES,
> >+                                                    XELPDP_P0_STATE_ACTIVE);
> >+                intel_cx0_powerdown_change_sequence(encoder,
> INTEL_CX0_BOTH_LANES,
> >+                                                    XELPDP_P2_STATE_READY);
> >+        }
> >+
> >         intel_cx0_phy_transaction_end(encoder, wakeref); }
> >
> >@@ -3485,7 +3486,7 @@ void intel_mtl_pll_enable_clock(struct
> intel_encoder *encoder,
> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >                 intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
> >         else
> >-                intel_cx0pll_enable_clock(encoder);
> >+                intel_cx0pll_enable_clock(encoder,
> >+ &crtc_state->dpll_hw_state.cx0pll);
> > }
> >
> > /*
> >@@ -3808,7 +3809,7 @@ void intel_cx0_pll_power_save_wa(struct
> intel_display *display)
> >                             encoder->base.base.id,
> >encoder->base.name);
> >
> >                 intel_cx0pll_enable(encoder, &pll_state);
> >-                intel_cx0pll_enable_clock(encoder);
> >+                intel_cx0pll_enable_clock(encoder, &pll_state);
> >                 intel_cx0pll_disable(encoder);
> >                 intel_cx0pll_disable_clock(encoder);
> >         }
> >--
> >2.34.1
> >

RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Gustavo Sousa]

Quoting Kandpal, Suraj (2025-12-31 02:07:35-03:00)
>> Subject: Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two
>> parts
>> 
>> Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00)
>> >From: Mika Kahola <mika.kahola@intel.com>
>> >
>> >Split PLL enabling/disabling in two parts - one for pll setting pll
>> >dividers and second one to enable/disable pll clock. PLL clock
>> >enabling/disbling happens via encoder->enable_clock/disable_clock
>> >function hook. The reason for doing this is that we need to make sure
>> >the clock enablement happens after PPS ON step to be inline with the
>> >sequences which we end up violating otherwise. As a result of this
>> >violation we end up in a hanged state if machine stays idle for more
>> >that 15 mins.
>> 
>> So, it appears this started happening when we Cx0 code was integrated into
>> the DPLL framework and then the driver started enabling the PHY PLL/clock
>> too early, right?
>> 
>> I am lacking some context/background here due to my unfamiliarity with pre-
>> MTL platforms, but why I exactly do we program the PLLs before the modeset
>> sequence?  Is it related to the shared nature of PLLs for platforms pre-
>> C10/pre-C20?  If so, do we really need to do the same for
>> C10/C20 PHYs, since we have dedicated PLLs for them?
>> 
>> (Sorry for asking here and a bit too late.  Probably the better place to ask this
>> was in series that integrated Cx0 into the DPLL framework.)
>
>
>Right it used to be actually because of the shared nature of PLL's. With c10 c20 we moved
>to a different framework where we called the the sequence together using hooks like enable_clock
>and disable_clock since there was not a lot of time of time to refactor the dpll_shared_framework to
>a framework with supported individual ones.
>Now that we had time we shifted cx0 back to the previous framework but missed defer the clock enablement

Then, if we move forward with this, perhaps this patch deserves a
"Fixes:" trailer.

>To later during enable clock time so that we honor the sequence, why we had to do this is even though its not shared PLL anymore is
>To make sure this framework is backward compatible too.

I see.

If the requirement for programming PLL parameters early was only because
of shared PLLs and we do not have that same requirements for C10/C20, I
would argue that doing the whole programming at once and only during the
"enable clock" phase of the encoder would make the driver more compliant
with the Bspec.

I also noticed that, for the older displays, the "enable clock" thing is
the part that selects the PLL (which is already enabled) as the port's
"clock source".  With C10/C20 we are actually deferring the PLL enabling
to the "enable clock" phase of the port while, I believe, the
expectation of intel_dpll_funcs::enable() is that the PLL would be
enabled when the function returned, which would not be exactly true for
C10/C20 after this patch.

What if, in intel_dpll_mgr, we made the distinction between enabling the
PLL early and enabling it at "intel_ddi_enable_clock()" time?

--
Gustavo Sousa

>Also we had to move cx0 pll framework back to dpll framework because the previous can work well as long as the ports are static hence aren’t
>As future proof , we plan to move LT PHY back here too once this ages well.
>
>> 
>> >
>> >PLL state verification happens now earlier than the clock is enabled
>> >which causes a drm warn to be thrown. Silence this warning by allowing
>> >this check for only earlier platforms than MeteorLake.
>> >
>> >Bspec: 49190
>> 
>> This Bspec page is not invalid for platforms using C10/C20 PHYs.
>> 
>> We probably want to use these instead:
>> 
>> Bspec: 65448, 68849
>>
>
>Sure will replace them.
>
>Regards,
>Suraj Kandpal
> 
>> --
>> Gustavo Sousa
>> 
>> >Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87 ++++++++++++-------
>> >drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +--
>> > 2 files changed, 64 insertions(+), 35 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >index 7288065d2461..f3baba264e88 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct
>> >intel_encoder *encoder,  {
>> >         int port_clock = pll_state->use_c10 ? pll_state->c10.clock : pll_state-
>> >c20.clock;
>> >         struct intel_display *display = to_intel_display(encoder);
>> >-        enum phy phy = intel_encoder_to_phy(encoder);
>> >         struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> >         bool lane_reversal = dig_port->lane_reversal;
>> >-        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
>> >-                                          INTEL_CX0_LANE0;
>> >         struct ref_tracker *wakeref =
>> >intel_cx0_phy_transaction_begin(encoder);
>> >
>> >         /*
>> >@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct
>> intel_encoder *encoder,
>> >          */
>> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
>> >port_clock);
>> >
>> >-        /*
>> >-         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>> >-         * LN<Lane for maxPCLK> to "1" to enable PLL.
>> >-         */
>> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >-                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>> >-                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
>> >-
>> >-        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
>> == "1". */
>> >-        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
>> encoder->port),
>> >-                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>> >-                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>> >-                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
>> >-                drm_warn(display->drm, "Port %c PLL not locked\n",
>> >-                         phy_name(phy));
>> >-
>> >-        /*
>> >-         * 11. Follow the Display Voltage Frequency Switching Sequence After
>> >-         * Frequency Change. We handle this step in bxt_set_cdclk().
>> >-         */
>> >-
>> >         /*
>> >          * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>> >          *
>> >@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct
>> intel_display *display,
>> >         }
>> > }
>> >
>> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder) {
>> >+        struct intel_display *display = to_intel_display(encoder);
>> >+        enum phy phy = intel_encoder_to_phy(encoder);
>> >+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> >+        bool lane_reversal = dig_port->lane_reversal;
>> >+                                          INTEL_CX0_LANE0;
>> >+        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
>> >+                                        INTEL_CX0_LANE0;
>> >+
>> >+        struct ref_tracker *wakeref =
>> >+ intel_cx0_phy_transaction_begin(encoder);
>> >+
>> >+        /*
>> >+         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>> >+         * LN<Lane for maxPCLK> to "1" to enable PLL.
>> >+         */
>> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >+                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>> >+                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
>> >+
>> >+        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
>> == "1". */
>> >+        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
>> encoder->port),
>> >+                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>> >+                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>> >+                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
>> >+                drm_warn(display->drm, "Port %c PLL not locked\n",
>> >+                         phy_name(phy));
>> >+
>> >+        /*
>> >+         * 11. Follow the Display Voltage Frequency Switching Sequence After
>> >+         * Frequency Change. We handle this step in bxt_set_cdclk().
>> >+         */
>> >+
>> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
>> >+
>> > void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int
>> >port_clock)  {
>> >         struct intel_display *display = to_intel_display(encoder); @@
>> >-3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct intel_encoder
>> >*encoder,
>> >
>> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> >                 intel_mtl_tbt_pll_enable_clock(encoder,
>> > crtc_state->port_clock);
>> >+        else
>> >+                intel_cx0pll_enable_clock(encoder);
>> > }
>> >
>> > /*
>> >@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct
>> intel_encoder *encoder)
>> >          * Frequency Change. We handle this step in bxt_set_cdclk().
>> >          */
>> >
>> >-        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
>> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >-                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >-                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
>> >-
>> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
>> >
>> >@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct
>> intel_encoder *encoder)
>> >                              intel_cx0_get_pclk_pll_request(lane);
>> > }
>> >
>> >+static void intel_cx0pll_disable_clock(struct intel_encoder *encoder)
>> >+{
>> >+        struct intel_display *display = to_intel_display(encoder);
>> >+        struct ref_tracker *wakeref =
>> >+intel_cx0_phy_transaction_begin(encoder);
>> >+
>> >+        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
>> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >+                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
>> >+
>> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
>> >+
>> > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)  {
>> >         struct intel_display *display = to_intel_display(encoder); @@
>> >-3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct
>> >intel_encoder *encoder)
>> >
>> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> >                 intel_mtl_tbt_pll_disable_clock(encoder);
>> >+        else
>> >+                intel_cx0pll_disable_clock(encoder);
>> >+
>> > }
>> >
>> > enum icl_port_dpll_id
>> >@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct
>> intel_display *display)
>> >                             encoder->base.base.id,
>> >encoder->base.name);
>> >
>> >                 intel_cx0pll_enable(encoder, &pll_state);
>> >+                intel_cx0pll_enable_clock(encoder);
>> >                 intel_cx0pll_disable(encoder);
>> >+                intel_cx0pll_disable_clock(encoder);
>> >         }
>> > }
>> >diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >index 9aa84a430f09..59395076103c 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
>> >                      "asserting DPLL %s with no DPLL\n", str_on_off(state)))
>> >                 return;
>> >
>> >-        cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
>> >-        INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
>> >-                                 "%s assertion failure (expected %s, current %s)\n",
>> >-                                 pll->info->name, str_on_off(state),
>> >-                                 str_on_off(cur_state));
>> >+        if (DISPLAY_VER(display) < 14) {
>> >+                cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
>> >+                INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
>> >+                                         "%s assertion failure (expected %s, current %s)\n",
>> >+                                         pll->info->name, str_on_off(state),
>> >+                                         str_on_off(cur_state));
>> >+        }
>> > }
>> >
>> > static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>> >--
>> >2.34.1
>> >

RE: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook

[Gustavo Sousa]

Quoting Kandpal, Suraj (2025-12-31 02:10:59-03:00)
>> 
>> Quoting Suraj Kandpal (2025-12-30 05:31:41-03:00)
>> >Move the step to toggle powerdown sequence change for HDMI to enable
>> >clock hook where it belongs according to its sequence.
>> >Do the required changes to make that work.
>> 
>> This should probably be a squash into the previous patch?
>
>So reason for separate patch is that this requires me changing the argument of
>clock enable which is not because of the same logical reason that changes are being done in patch 1,
>hence a separate patch for changes that were brought about due to another reason. Had this been just movement
>of step 12 then I would have squashed them.

Hm... The previous patch is introducing intel_cx0pll_enable_clock() and
says it is splitting the sequence in two, but then it ended up is
leaving step 12 behind.  If it is introducing
intel_cx0pll_enable_clock(), it could as well have done it with a
signature that allows step 12 to be done.

IMO, here we are modifying that function to "make it right".  This looks
like a good fixup candidate to me.

--
Gustavo Sousa

>
>Regards,
>Suraj Kandpal
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 37 ++++++++++----------
>> > 1 file changed, 19 insertions(+), 18 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >index f3baba264e88..5edd293b533b 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >@@ -3281,21 +3281,6 @@ static void intel_cx0pll_enable(struct
>> intel_encoder *encoder,
>> >          */
>> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
>> >port_clock);
>> >
>> >-        /*
>> >-         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>> >-         *
>> >-         * Wa_13013502646:
>> >-         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
>> >-         * Workaround: Toggle powerdown value by setting first to P0 and then
>> to P2, for both
>> >-         * PHY lanes.
>> >-         */
>> >-        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
>> >-                intel_cx0_powerdown_change_sequence(encoder,
>> INTEL_CX0_BOTH_LANES,
>> >-                                                    XELPDP_P0_STATE_ACTIVE);
>> >-                intel_cx0_powerdown_change_sequence(encoder,
>> INTEL_CX0_BOTH_LANES,
>> >-                                                    XELPDP_P2_STATE_READY);
>> >-        }
>> >-
>> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
>> >
>> >@@ -3379,7 +3364,8 @@ static int intel_mtl_tbt_clock_select(struct
>> intel_display *display,
>> >         }
>> > }
>> >
>> >-static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
>> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder,
>> >+                                      const struct intel_cx0pll_state
>> >+*pll_state)
>> > {
>> >         struct intel_display *display = to_intel_display(encoder);
>> >         enum phy phy = intel_encoder_to_phy(encoder); @@ -3412,6
>> >+3398,21 @@ static void intel_cx0pll_enable_clock(struct intel_encoder
>> *encoder)
>> >          * Frequency Change. We handle this step in bxt_set_cdclk().
>> >          */
>> >
>> >+        /*
>> >+         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>> >+         *
>> >+         * Wa_13013502646:
>> >+         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
>> >+         * Workaround: Toggle powerdown value by setting first to P0 and then
>> to P2, for both
>> >+         * PHY lanes.
>> >+         */
>> >+        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
>> >+                intel_cx0_powerdown_change_sequence(encoder,
>> INTEL_CX0_BOTH_LANES,
>> >+                                                    XELPDP_P0_STATE_ACTIVE);
>> >+                intel_cx0_powerdown_change_sequence(encoder,
>> INTEL_CX0_BOTH_LANES,
>> >+                                                    XELPDP_P2_STATE_READY);
>> >+        }
>> >+
>> >         intel_cx0_phy_transaction_end(encoder, wakeref); }
>> >
>> >@@ -3485,7 +3486,7 @@ void intel_mtl_pll_enable_clock(struct
>> intel_encoder *encoder,
>> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> >                 intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
>> >         else
>> >-                intel_cx0pll_enable_clock(encoder);
>> >+                intel_cx0pll_enable_clock(encoder,
>> >+ &crtc_state->dpll_hw_state.cx0pll);
>> > }
>> >
>> > /*
>> >@@ -3808,7 +3809,7 @@ void intel_cx0_pll_power_save_wa(struct
>> intel_display *display)
>> >                             encoder->base.base.id,
>> >encoder->base.name);
>> >
>> >                 intel_cx0pll_enable(encoder, &pll_state);
>> >-                intel_cx0pll_enable_clock(encoder);
>> >+                intel_cx0pll_enable_clock(encoder, &pll_state);
>> >                 intel_cx0pll_disable(encoder);
>> >                 intel_cx0pll_disable_clock(encoder);
>> >         }
>> >--
>> >2.34.1
>> >

RE: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Gustavo Sousa]

Quoting Kandpal, Suraj (2025-12-31 01:59:29-03:00)
>> Subject: Re: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit
>> 
>> Quoting Suraj Kandpal (2025-12-30 05:31:42-03:00)
>> >Clear the response ready and error bit of PORT_P2M_MESSAGE_BUS_STATUS
>> >before writing the transaction pending bit of PORT_M2P_MSGBUS_CTL as
>> >that is a hard requirement. If not done we find that the PHY hangs
>> >since it ends up in a weird state if left idle for more than 1 hour.
>> 
>> Since the series title refers to suspend/resume, is there an easy way of
>> reproducing this via some power state transition?
>
>Sadly no we did try to no avail went as low as DC9 too. Tried sleep then wake,
>Tried hibernating too.

I see.  I was wondering if DMC could be at play here.

So the only known way of reproducing this is to leave it idle for more
than 1 hour?  Do you know if that happens if we load the driver with
DC5/6 disabled (i.e. enable_dc=0)?

>
>> 
>> I'm wondering if we are looking at a driver issue here or if this is really
>> something else.  I see that we usually call intel_cx0_bus_reset() in error paths,
>> which contains a call to intel_clear_response_ready_flag(), but it could end up
>> being not called if the reset times out.
>
>Yes the reset times out and this is because PHY is in hanged state here
>
>> 
>> Do we see error messages from the driver when the PHY hangs?
>
>Yes we do see a PHY hang when we are at the stage of swing programming setting
>And the error happens when we try to read the msg access bit itself which is because the PHY ends up in
>Weird state where it has a write ack but no response ready bit getting set.
>But with my observation I can certainly say this is PHY issue.
>I also double confirmed this behavior with windows folks and their code where they said they saw an issue in the same lines.
>Also the bspec basically adds this as a disclaimer that response ready and error bit should be cleared no matter what before writing
>The transaction pending bit.

Agreed on the need to clear the response ready bit before starting
another transaction, so the patch looks fine.  I believe Jani's
request to rename the function wouldn't necessarily change the semantics
of this patch, so

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

in case the rename gets done as a follow-up.

>
>Regards,
>Suraj Kandpal
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >Bspec: 65101
>> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >index 5edd293b533b..5ebc3404eee2 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >@@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct intel_encoder
>> *encoder,
>> >                 return -ETIMEDOUT;
>> >         }
>> >
>> >+        intel_clear_response_ready_flag(encoder, lane);
>> >+
>> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port,
>> lane),
>> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
>> >                        XELPDP_PORT_M2P_COMMAND_READ | @@ -293,6 +295,8
>> >@@ static int __intel_cx0_write_once(struct intel_encoder *encoder,
>> >                 return -ETIMEDOUT;
>> >         }
>> >
>> >+        intel_clear_response_ready_flag(encoder, lane);
>> >+
>> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port,
>> lane),
>> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
>> >                        (committed ?
>> XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
>> >--
>> >2.34.1
>> >

RE: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook

[Kandpal, Suraj]

> Subject: RE: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook
> 
> Quoting Kandpal, Suraj (2025-12-31 02:10:59-03:00)
> >>
> >> Quoting Suraj Kandpal (2025-12-30 05:31:41-03:00)
> >> >Move the step to toggle powerdown sequence change for HDMI to
> enable
> >> >clock hook where it belongs according to its sequence.
> >> >Do the required changes to make that work.
> >>
> >> This should probably be a squash into the previous patch?
> >
> >So reason for separate patch is that this requires me changing the
> >argument of clock enable which is not because of the same logical
> >reason that changes are being done in patch 1, hence a separate patch
> >for changes that were brought about due to another reason. Had this been
> just movement of step 12 then I would have squashed them.
> 
> Hm... The previous patch is introducing intel_cx0pll_enable_clock() and says it
> is splitting the sequence in two, but then it ended up is leaving step 12 behind.
> If it is introducing intel_cx0pll_enable_clock(), it could as well have done it
> with a signature that allows step 12 to be done.
> 
> IMO, here we are modifying that function to "make it right".  This looks like a
> good fixup candidate to me.
> 

Let's see how the discussion in patch 1 goes then see how this can be dealt with

Regards,
Suraj Kandpal
> --
> Gustavo Sousa
> 
> >
> >Regards,
> >Suraj Kandpal
> >
> >>
> >> --
> >> Gustavo Sousa
> >>
> >> >
> >> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 37
> >> >++++++++++----------
> >> > 1 file changed, 19 insertions(+), 18 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >index f3baba264e88..5edd293b533b 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >@@ -3281,21 +3281,6 @@ static void intel_cx0pll_enable(struct
> >> intel_encoder *encoder,
> >> >          */
> >> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> >> >port_clock);
> >> >
> >> >-        /*
> >> >-         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >> >-         *
> >> >-         * Wa_13013502646:
> >> >-         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
> >> >-         * Workaround: Toggle powerdown value by setting first to P0 and
> then
> >> to P2, for both
> >> >-         * PHY lanes.
> >> >-         */
> >> >-        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
> >> >-                intel_cx0_powerdown_change_sequence(encoder,
> >> INTEL_CX0_BOTH_LANES,
> >> >-                                                    XELPDP_P0_STATE_ACTIVE);
> >> >-                intel_cx0_powerdown_change_sequence(encoder,
> >> INTEL_CX0_BOTH_LANES,
> >> >-                                                    XELPDP_P2_STATE_READY);
> >> >-        }
> >> >-
> >> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
> >> >
> >> >@@ -3379,7 +3364,8 @@ static int intel_mtl_tbt_clock_select(struct
> >> intel_display *display,
> >> >         }
> >> > }
> >> >
> >> >-static void intel_cx0pll_enable_clock(struct intel_encoder
> >> >*encoder)
> >> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder,
> >> >+                                      const struct
> >> >+intel_cx0pll_state
> >> >+*pll_state)
> >> > {
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >         enum phy phy = intel_encoder_to_phy(encoder); @@ -3412,6
> >> >+3398,21 @@ static void intel_cx0pll_enable_clock(struct
> >> >+intel_encoder
> >> *encoder)
> >> >          * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >          */
> >> >
> >> >+        /*
> >> >+         * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >> >+         *
> >> >+         * Wa_13013502646:
> >> >+         * Fixes: HDMI lane to lane skew violations on C10 display PHYs.
> >> >+         * Workaround: Toggle powerdown value by setting first to
> >> >+ P0 and then
> >> to P2, for both
> >> >+         * PHY lanes.
> >> >+         */
> >> >+        if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) {
> >> >+                intel_cx0_powerdown_change_sequence(encoder,
> >> INTEL_CX0_BOTH_LANES,
> >> >+                                                    XELPDP_P0_STATE_ACTIVE);
> >> >+                intel_cx0_powerdown_change_sequence(encoder,
> >> INTEL_CX0_BOTH_LANES,
> >> >+                                                    XELPDP_P2_STATE_READY);
> >> >+        }
> >> >+
> >> >         intel_cx0_phy_transaction_end(encoder, wakeref); }
> >> >
> >> >@@ -3485,7 +3486,7 @@ void intel_mtl_pll_enable_clock(struct
> >> intel_encoder *encoder,
> >> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >> >                 intel_mtl_tbt_pll_enable_clock(encoder, crtc_state-
> >port_clock);
> >> >         else
> >> >-                intel_cx0pll_enable_clock(encoder);
> >> >+                intel_cx0pll_enable_clock(encoder,
> >> >+ &crtc_state->dpll_hw_state.cx0pll);
> >> > }
> >> >
> >> > /*
> >> >@@ -3808,7 +3809,7 @@ void intel_cx0_pll_power_save_wa(struct
> >> intel_display *display)
> >> >                             encoder->base.base.id,
> >> >encoder->base.name);
> >> >
> >> >                 intel_cx0pll_enable(encoder, &pll_state);
> >> >-                intel_cx0pll_enable_clock(encoder);
> >> >+                intel_cx0pll_enable_clock(encoder, &pll_state);
> >> >                 intel_cx0pll_disable(encoder);
> >> >                 intel_cx0pll_disable_clock(encoder);
> >> >         }
> >> >--
> >> >2.34.1
> >> >

RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Kandpal, Suraj]

> Subject: RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two
> parts
> 
> Quoting Kandpal, Suraj (2025-12-31 02:07:35-03:00)
> >> Subject: Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling
> >> in two parts
> >>
> >> Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00)
> >> >From: Mika Kahola <mika.kahola@intel.com>
> >> >
> >> >Split PLL enabling/disabling in two parts - one for pll setting pll
> >> >dividers and second one to enable/disable pll clock. PLL clock
> >> >enabling/disbling happens via encoder->enable_clock/disable_clock
> >> >function hook. The reason for doing this is that we need to make
> >> >sure the clock enablement happens after PPS ON step to be inline
> >> >with the sequences which we end up violating otherwise. As a result
> >> >of this violation we end up in a hanged state if machine stays idle
> >> >for more that 15 mins.
> >>
> >> So, it appears this started happening when we Cx0 code was integrated
> >> into the DPLL framework and then the driver started enabling the PHY
> >> PLL/clock too early, right?
> >>
> >> I am lacking some context/background here due to my unfamiliarity
> >> with pre- MTL platforms, but why I exactly do we program the PLLs
> >> before the modeset sequence?  Is it related to the shared nature of
> >> PLLs for platforms pre- C10/pre-C20?  If so, do we really need to do
> >> the same for
> >> C10/C20 PHYs, since we have dedicated PLLs for them?
> >>
> >> (Sorry for asking here and a bit too late.  Probably the better place
> >> to ask this was in series that integrated Cx0 into the DPLL
> >> framework.)
> >
> >
> >Right it used to be actually because of the shared nature of PLL's.
> >With c10 c20 we moved to a different framework where we called the the
> >sequence together using hooks like enable_clock and disable_clock since
> >there was not a lot of time of time to refactor the dpll_shared_framework to
> a framework with supported individual ones.
> >Now that we had time we shifted cx0 back to the previous framework but
> >missed defer the clock enablement
> 
> Then, if we move forward with this, perhaps this patch deserves a "Fixes:"
> trailer.
> 
> >To later during enable clock time so that we honor the sequence, why we
> >had to do this is even though its not shared PLL anymore is To make sure this
> framework is backward compatible too.
> 
> I see.
> 
> If the requirement for programming PLL parameters early was only because of
> shared PLLs and we do not have that same requirements for C10/C20, I would
> argue that doing the whole programming at once and only during the "enable
> clock" phase of the encoder would make the driver more compliant with the
> Bspec.
> 
> I also noticed that, for the older displays, the "enable clock" thing is the part
> that selects the PLL (which is already enabled) as the port's "clock source".
> With C10/C20 we are actually deferring the PLL enabling to the "enable clock"
> phase of the port while, I believe, the expectation of intel_dpll_funcs::enable()
> is that the PLL would be enabled when the function returned, which would not
> be exactly true for
> C10/C20 after this patch.
> 
> What if, in intel_dpll_mgr, we made the distinction between enabling the PLL
> early and enabling it at "intel_ddi_enable_clock()" time?
> 

Any inputs here @Kahola, Mika @Deak, Imre

Regards,
Suraj Kandpal

> --
> Gustavo Sousa
> 
> >Also we had to move cx0 pll framework back to dpll framework because
> >the previous can work well as long as the ports are static hence aren’t As
> future proof , we plan to move LT PHY back here too once this ages well.
> >
> >>
> >> >
> >> >PLL state verification happens now earlier than the clock is enabled
> >> >which causes a drm warn to be thrown. Silence this warning by
> >> >allowing this check for only earlier platforms than MeteorLake.
> >> >
> >> >Bspec: 49190
> >>
> >> This Bspec page is not invalid for platforms using C10/C20 PHYs.
> >>
> >> We probably want to use these instead:
> >>
> >> Bspec: 65448, 68849
> >>
> >
> >Sure will replace them.
> >
> >Regards,
> >Suraj Kandpal
> >
> >> --
> >> Gustavo Sousa
> >>
> >> >Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87
> >> >++++++++++++------- drivers/gpu/drm/i915/display/intel_dpll_mgr.c |
> >> >12 +--
> >> > 2 files changed, 64 insertions(+), 35 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >index 7288065d2461..f3baba264e88 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct
> >> >intel_encoder *encoder,  {
> >> >         int port_clock = pll_state->use_c10 ? pll_state->c10.clock
> >> >: pll_state- c20.clock;
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >-        enum phy phy = intel_encoder_to_phy(encoder);
> >> >         struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >> >         bool lane_reversal = dig_port->lane_reversal;
> >> >-        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> >> >-                                          INTEL_CX0_LANE0;
> >> >         struct ref_tracker *wakeref =
> >> >intel_cx0_phy_transaction_begin(encoder);
> >> >
> >> >         /*
> >> >@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct
> >> intel_encoder *encoder,
> >> >          */
> >> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> >> >port_clock);
> >> >
> >> >-        /*
> >> >-         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> >> >-         * LN<Lane for maxPCLK> to "1" to enable PLL.
> >> >-         */
> >> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >> >port),
> >> >-                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> >> >-                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> >> >-
> >> >-        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for
> maxPCLK>
> >> == "1". */
> >> >-        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
> >> encoder->port),
> >> >-                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> >> >-                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> >> >-                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> >> >-                drm_warn(display->drm, "Port %c PLL not locked\n",
> >> >-                         phy_name(phy));
> >> >-
> >> >-        /*
> >> >-         * 11. Follow the Display Voltage Frequency Switching Sequence
> After
> >> >-         * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >-         */
> >> >-
> >> >         /*
> >> >          * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >> >          *
> >> >@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct
> >> intel_display *display,
> >> >         }
> >> > }
> >> >
> >> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder) {
> >> >+        struct intel_display *display = to_intel_display(encoder);
> >> >+        enum phy phy = intel_encoder_to_phy(encoder);
> >> >+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >> >+        bool lane_reversal = dig_port->lane_reversal;
> >> >+                                          INTEL_CX0_LANE0;
> >> >+        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> >> >+                                        INTEL_CX0_LANE0;
> >> >+
> >> >+        struct ref_tracker *wakeref =
> >> >+ intel_cx0_phy_transaction_begin(encoder);
> >> >+
> >> >+        /*
> >> >+         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> >> >+         * LN<Lane for maxPCLK> to "1" to enable PLL.
> >> >+         */
> >> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display,
> >> >+ encoder-
> >> >port),
> >> >+                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> >> >+                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> >> >+
> >> >+        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for
> >> >+ maxPCLK>
> >> == "1". */
> >> >+        if (intel_de_wait_us(display,
> >> >+ XELPDP_PORT_CLOCK_CTL(display,
> >> encoder->port),
> >> >+                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> >> >+                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> >> >+                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> >> >+                drm_warn(display->drm, "Port %c PLL not locked\n",
> >> >+                         phy_name(phy));
> >> >+
> >> >+        /*
> >> >+         * 11. Follow the Display Voltage Frequency Switching Sequence
> After
> >> >+         * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >+         */
> >> >+
> >> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
> >> >+
> >> > void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder,
> >> >int
> >> >port_clock)  {
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >@@
> >> >-3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct
> >> >intel_encoder *encoder,
> >> >
> >> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >> >                 intel_mtl_tbt_pll_enable_clock(encoder,
> >> > crtc_state->port_clock);
> >> >+        else
> >> >+                intel_cx0pll_enable_clock(encoder);
> >> > }
> >> >
> >> > /*
> >> >@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct
> >> intel_encoder *encoder)
> >> >          * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >          */
> >> >
> >> >-        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks.
> */
> >> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >> >port),
> >> >-                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> >> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >> >port),
> >> >-                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> >> >-
> >> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
> >> >
> >> >@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct
> >> intel_encoder *encoder)
> >> >                              intel_cx0_get_pclk_pll_request(lane);
> >> > }
> >> >
> >> >+static void intel_cx0pll_disable_clock(struct intel_encoder
> >> >+*encoder) {
> >> >+        struct intel_display *display = to_intel_display(encoder);
> >> >+        struct ref_tracker *wakeref =
> >> >+intel_cx0_phy_transaction_begin(encoder);
> >> >+
> >> >+        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks.
> */
> >> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display,
> >> >+ encoder-
> >> >port),
> >> >+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> >> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display,
> >> >+ encoder-
> >> >port),
> >> >+                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> >> >+
> >> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
> >> >+
> >> > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)  {
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >@@
> >> >-3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct
> >> >intel_encoder *encoder)
> >> >
> >> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >> >                 intel_mtl_tbt_pll_disable_clock(encoder);
> >> >+        else
> >> >+                intel_cx0pll_disable_clock(encoder);
> >> >+
> >> > }
> >> >
> >> > enum icl_port_dpll_id
> >> >@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct
> >> intel_display *display)
> >> >                             encoder->base.base.id,
> >> >encoder->base.name);
> >> >
> >> >                 intel_cx0pll_enable(encoder, &pll_state);
> >> >+                intel_cx0pll_enable_clock(encoder);
> >> >                 intel_cx0pll_disable(encoder);
> >> >+                intel_cx0pll_disable_clock(encoder);
> >> >         }
> >> > }
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >index 9aa84a430f09..59395076103c 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
> >> >                      "asserting DPLL %s with no DPLL\n", str_on_off(state)))
> >> >                 return;
> >> >
> >> >-        cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> >> >-        INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> >> >-                                 "%s assertion failure (expected %s, current %s)\n",
> >> >-                                 pll->info->name, str_on_off(state),
> >> >-                                 str_on_off(cur_state));
> >> >+        if (DISPLAY_VER(display) < 14) {
> >> >+                cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> >> >+                INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> >> >+                                         "%s assertion failure (expected %s, current
> %s)\n",
> >> >+                                         pll->info->name, str_on_off(state),
> >> >+                                         str_on_off(cur_state));
> >> >+        }
> >> > }
> >> >
> >> > static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
> >> >--
> >> >2.34.1
> >> >

RE: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Kandpal, Suraj]

> Subject: RE: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit
> 
> Quoting Kandpal, Suraj (2025-12-31 01:59:29-03:00)
> >> Subject: Re: [PATCH 3/3] drm/i915/cx0: Clear response ready & error
> >> bit
> >>
> >> Quoting Suraj Kandpal (2025-12-30 05:31:42-03:00)
> >> >Clear the response ready and error bit of
> >> >PORT_P2M_MESSAGE_BUS_STATUS before writing the transaction
> pending
> >> >bit of PORT_M2P_MSGBUS_CTL as that is a hard requirement. If not
> >> >done we find that the PHY hangs since it ends up in a weird state if left
> idle for more than 1 hour.
> >>
> >> Since the series title refers to suspend/resume, is there an easy way
> >> of reproducing this via some power state transition?
> >
> >Sadly no we did try to no avail went as low as DC9 too. Tried sleep
> >then wake, Tried hibernating too.
> 
> I see.  I was wondering if DMC could be at play here.
> 
> So the only known way of reproducing this is to leave it idle for more than 1
> hour?  Do you know if that happens if we load the driver with
> DC5/6 disabled (i.e. enable_dc=0)?
> 
> >
> >>
> >> I'm wondering if we are looking at a driver issue here or if this is
> >> really something else.  I see that we usually call
> >> intel_cx0_bus_reset() in error paths, which contains a call to
> >> intel_clear_response_ready_flag(), but it could end up being not called if
> the reset times out.
> >
> >Yes the reset times out and this is because PHY is in hanged state here
> >
> >>
> >> Do we see error messages from the driver when the PHY hangs?
> >
> >Yes we do see a PHY hang when we are at the stage of swing programming
> >setting And the error happens when we try to read the msg access bit
> >itself which is because the PHY ends up in Weird state where it has a write
> ack but no response ready bit getting set.
> >But with my observation I can certainly say this is PHY issue.
> >I also double confirmed this behavior with windows folks and their code
> where they said they saw an issue in the same lines.
> >Also the bspec basically adds this as a disclaimer that response ready
> >and error bit should be cleared no matter what before writing The
> transaction pending bit.
> 
> Agreed on the need to clear the response ready bit before starting another
> transaction, so the patch looks fine.  I believe Jani's request to rename the
> function wouldn't necessarily change the semantics of this patch, so
> 
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
> 
> in case the rename gets done as a follow-up.
> 

Thanks will separate out this patch from the series since the first patch seems to need a little more discussion 

Regards,
Suraj Kandpal

> >
> >Regards,
> >Suraj Kandpal
> >
> >>
> >> --
> >> Gustavo Sousa
> >>
> >> >
> >> >Bspec: 65101
> >> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
> >> > 1 file changed, 4 insertions(+)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >index 5edd293b533b..5ebc3404eee2 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >@@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct
> >> >intel_encoder
> >> *encoder,
> >> >                 return -ETIMEDOUT;
> >> >         }
> >> >
> >> >+        intel_clear_response_ready_flag(encoder, lane);
> >> >+
> >> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display,
> >> > port,
> >> lane),
> >> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >> >                        XELPDP_PORT_M2P_COMMAND_READ | @@ -293,6
> >> >+295,8 @@ static int __intel_cx0_write_once(struct intel_encoder
> *encoder,
> >> >                 return -ETIMEDOUT;
> >> >         }
> >> >
> >> >+        intel_clear_response_ready_flag(encoder, lane);
> >> >+
> >> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display,
> >> > port,
> >> lane),
> >> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >> >                        (committed ?
> >> XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
> >> >--
> >> >2.34.1
> >> >

Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Michał Grzelak]

[-- Attachment #1: Type: text/plain, Size: 9599 bytes --]

Hi Suraj,

On Tue, 30 Dec 2025, Suraj Kandpal wrote:
> From: Mika Kahola <mika.kahola@intel.com>
>
> Split PLL enabling/disabling in two parts - one for pll setting
> pll dividers and second one to enable/disable pll clock. PLL
> clock enabling/disbling happens via encoder->enable_clock/disable_clock
> function hook. The reason for doing this is that we need to make sure
> the clock enablement happens after PPS ON step to be inline with the
> sequences which we end up violating otherwise. As a result of this
> violation we end up in a hanged state if machine stays idle for more
> that 15 mins.
>
> PLL state verification happens now earlier than the clock is enabled
> which causes a drm warn to be thrown. Silence this warning by
> allowing this check for only earlier platforms than MeteorLake.
>
> Bspec: 49190
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87 ++++++++++++-------
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +--
> 2 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 7288065d2461..f3baba264e88 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> {
> 	int port_clock = pll_state->use_c10 ? pll_state->c10.clock : pll_state->c20.clock;
> 	struct intel_display *display = to_intel_display(encoder);
> -	enum phy phy = intel_encoder_to_phy(encoder);
> 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> 	bool lane_reversal = dig_port->lane_reversal;
> -	u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> -					  INTEL_CX0_LANE0;
> 	struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
>
> 	/*
> @@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
> 	 */
> 	intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), port_clock);
>
> -	/*
> -	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> -	 * LN<Lane for maxPCLK> to "1" to enable PLL.
> -	 */
> -	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> -		     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> -		     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> -
> -	/* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
> -	if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> -			     intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> -			     intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> -			     XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> -		drm_warn(display->drm, "Port %c PLL not locked\n",
> -			 phy_name(phy));
> -
> -	/*
> -	 * 11. Follow the Display Voltage Frequency Switching Sequence After
> -	 * Frequency Change. We handle this step in bxt_set_cdclk().
> -	 */
> -
> 	/*
> 	 * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> 	 *
> @@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct intel_display *display,
> 	}
> }
>
> +static void intel_cx0pll_enable_clock(struct intel_encoder *encoder)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	enum phy phy = intel_encoder_to_phy(encoder);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	bool lane_reversal = dig_port->lane_reversal;
> +					  INTEL_CX0_LANE0;

Line above breaks the build. My current understanding is that it causes
compiler to expand INTEL_CX0_LANE0 into BIT(0), which expands into 1UL
<< 0. Finally, compiler parses it more or less as:

1;

Since it is a valid statement but doesn't do anything, it triggers
unused value warning, which is turned into error.

IOW, with the line removed build works.

The explanation seems consistent with the output:

In file included from ./include/linux/bits.h:5,
                  from ./include/linux/bitops.h:6,
                  from ./include/linux/log2.h:12,
                  from drivers/gpu/drm/i915/display/intel_cx0_phy.c:6:
drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘intel_cx0pll_enable_clock’:
./include/vdso/bits.h:7:33: error: statement with no effect [-Werror=unused-value]
     7 | #define BIT(nr)                 (UL(1) << (nr))
       |                                 ^
drivers/gpu/drm/i915/display/intel_cx0_phy.c:32:33: note: in expansion of macro ‘BIT’
    32 | #define INTEL_CX0_LANE0         BIT(0)
       |                                 ^~~
drivers/gpu/drm/i915/display/intel_cx0_phy.c:3378:43: note: in expansion of macro ‘INTEL_CX0_LANE0’
  3378 |                                           INTEL_CX0_LANE0;
       |                                           ^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Besides of this, for the whole series:
Reviewed-by: Michał Grzelak <michal.grzelak@intel.com>

BR,
Michał

> +	u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> +					INTEL_CX0_LANE0;
> +
> +	struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
> +
> +	/*
> +	 * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> +	 * LN<Lane for maxPCLK> to "1" to enable PLL.
> +	 */
> +	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> +		     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> +		     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> +
> +	/* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
> +	if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> +			     intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> +			     intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> +			     XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> +		drm_warn(display->drm, "Port %c PLL not locked\n",
> +			 phy_name(phy));
> +
> +	/*
> +	 * 11. Follow the Display Voltage Frequency Switching Sequence After
> +	 * Frequency Change. We handle this step in bxt_set_cdclk().
> +	 */
> +
> +	intel_cx0_phy_transaction_end(encoder, wakeref);
> +}
> +
> void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int port_clock)
> {
> 	struct intel_display *display = to_intel_display(encoder);
> @@ -3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct intel_encoder *encoder,
>
> 	if (intel_tc_port_in_tbt_alt_mode(dig_port))
> 		intel_mtl_tbt_pll_enable_clock(encoder, crtc_state->port_clock);
> +	else
> +		intel_cx0pll_enable_clock(encoder);
> }
>
> /*
> @@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct intel_encoder *encoder)
> 	 * Frequency Change. We handle this step in bxt_set_cdclk().
> 	 */
>
> -	/* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
> -	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> -		     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> -	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> -		     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> -
> 	intel_cx0_phy_transaction_end(encoder, wakeref);
> }
>
> @@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct intel_encoder *encoder)
> 			     intel_cx0_get_pclk_pll_request(lane);
> }
>
> +static void intel_cx0pll_disable_clock(struct intel_encoder *encoder)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct ref_tracker *wakeref = intel_cx0_phy_transaction_begin(encoder);
> +
> +	/* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
> +	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> +		     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> +	intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
> +		     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> +
> +	intel_cx0_phy_transaction_end(encoder, wakeref);
> +}
> +
> void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)
> {
> 	struct intel_display *display = to_intel_display(encoder);
> @@ -3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct intel_encoder *encoder)
>
> 	if (intel_tc_port_in_tbt_alt_mode(dig_port))
> 		intel_mtl_tbt_pll_disable_clock(encoder);
> +	else
> +		intel_cx0pll_disable_clock(encoder);
> +
> }
>
> enum icl_port_dpll_id
> @@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct intel_display *display)
> 			    encoder->base.base.id, encoder->base.name);
>
> 		intel_cx0pll_enable(encoder, &pll_state);
> +		intel_cx0pll_enable_clock(encoder);
> 		intel_cx0pll_disable(encoder);
> +		intel_cx0pll_disable_clock(encoder);
> 	}
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 9aa84a430f09..59395076103c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
> 		     "asserting DPLL %s with no DPLL\n", str_on_off(state)))
> 		return;
>
> -	cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> -	INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> -				 "%s assertion failure (expected %s, current %s)\n",
> -				 pll->info->name, str_on_off(state),
> -				 str_on_off(cur_state));
> +	if (DISPLAY_VER(display) < 14) {
> +		cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> +		INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> +					 "%s assertion failure (expected %s, current %s)\n",
> +					 pll->info->name, str_on_off(state),
> +					 str_on_off(cur_state));
> +	}
> }
>
> static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
> -- 
> 2.34.1
>
>

RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

[Kahola, Mika]

> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa@intel.com>
> Sent: Monday, 5 January 2026 16.14
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Kahola, Mika <mika.kahola@intel.com>
> Subject: RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts
> 
> Quoting Kandpal, Suraj (2025-12-31 02:07:35-03:00)
> >> Subject: Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling
> >> in two parts
> >>
> >> Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00)
> >> >From: Mika Kahola <mika.kahola@intel.com>
> >> >
> >> >Split PLL enabling/disabling in two parts - one for pll setting pll
> >> >dividers and second one to enable/disable pll clock. PLL clock
> >> >enabling/disbling happens via encoder->enable_clock/disable_clock
> >> >function hook. The reason for doing this is that we need to make
> >> >sure the clock enablement happens after PPS ON step to be inline
> >> >with the sequences which we end up violating otherwise. As a result
> >> >of this violation we end up in a hanged state if machine stays idle
> >> >for more that 15 mins.
> >>
> >> So, it appears this started happening when we Cx0 code was integrated
> >> into the DPLL framework and then the driver started enabling the PHY
> >> PLL/clock too early, right?
> >>
> >> I am lacking some context/background here due to my unfamiliarity
> >> with pre- MTL platforms, but why I exactly do we program the PLLs
> >> before the modeset sequence?  Is it related to the shared nature of
> >> PLLs for platforms pre- C10/pre-C20?  If so, do we really need to do
> >> the same for
> >> C10/C20 PHYs, since we have dedicated PLLs for them?
> >>
> >> (Sorry for asking here and a bit too late.  Probably the better place
> >> to ask this was in series that integrated Cx0 into the DPLL
> >> framework.)
> >
> >
> >Right it used to be actually because of the shared nature of PLL's.
> >With c10 c20 we moved to a different framework where we called the the
> >sequence together using hooks like enable_clock and disable_clock since
> >there was not a lot of time of time to refactor the dpll_shared_framework to a framework with supported individual ones.
> >Now that we had time we shifted cx0 back to the previous framework but
> >missed defer the clock enablement
> 
> Then, if we move forward with this, perhaps this patch deserves a "Fixes:" trailer.
> 
> >To later during enable clock time so that we honor the sequence, why we
> >had to do this is even though its not shared PLL anymore is To make sure this framework is backward compatible too.
> 
> I see.
> 
> If the requirement for programming PLL parameters early was only because of shared PLLs and we do not have that same
> requirements for C10/C20, I would argue that doing the whole programming at once and only during the "enable clock" phase of
> the encoder would make the driver more compliant with the Bspec.

Programming PLL parameters for C10/C20 as part of pll framework with .enable function hook seemed to work ok until we bumped into this ArrowLake case where system suspend resume after an hour of idling would cause C10/C20 chips to get stuck and hence unable to resume correctly. This behaviour hasn't been seen with other platforms. Tests with this platform showed that we would need to delay a bit the clock enabling. This patch proposed to split the sequence for C10/C20 in two parts. First part for PLL programming and second part to enable the clock. Like you mentioned we could also do all PLL programming and clock enabling in one go with .enable_clock function hook. 

> 
> I also noticed that, for the older displays, the "enable clock" thing is the part that selects the PLL (which is already enabled) as the
> port's "clock source".  With C10/C20 we are actually deferring the PLL enabling to the "enable clock" phase of the port while, I
> believe, the expectation of intel_dpll_funcs::enable() is that the PLL would be enabled when the function returned, which would
> not be exactly true for
> C10/C20 after this patch.
> 
> What if, in intel_dpll_mgr, we made the distinction between enabling the PLL early and enabling it at "intel_ddi_enable_clock()"
> time?

By this you mean, all PLL programming is done in intel_ddi_enable_clock() function leaving .enable() function hook in intel_dpll_mgr empty?

> 
> --
> Gustavo Sousa
> 
> >Also we had to move cx0 pll framework back to dpll framework because
> >the previous can work well as long as the ports are static hence aren’t As future proof , we plan to move LT PHY back here too
> once this ages well.
> >
> >>
> >> >
> >> >PLL state verification happens now earlier than the clock is enabled
> >> >which causes a drm warn to be thrown. Silence this warning by
> >> >allowing this check for only earlier platforms than MeteorLake.
> >> >
> >> >Bspec: 49190
> >>
> >> This Bspec page is not invalid for platforms using C10/C20 PHYs.
> >>
> >> We probably want to use these instead:
> >>
> >> Bspec: 65448, 68849
> >>
> >
> >Sure will replace them.
> >
> >Regards,
> >Suraj Kandpal
> >
> >> --
> >> Gustavo Sousa
> >>
> >> >Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87
> >> >++++++++++++------- drivers/gpu/drm/i915/display/intel_dpll_mgr.c |
> >> >12 +--
> >> > 2 files changed, 64 insertions(+), 35 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >index 7288065d2461..f3baba264e88 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> >@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct
> >> >intel_encoder *encoder,  {
> >> >         int port_clock = pll_state->use_c10 ? pll_state->c10.clock
> >> >: pll_state- c20.clock;
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >-        enum phy phy = intel_encoder_to_phy(encoder);
> >> >         struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >> >         bool lane_reversal = dig_port->lane_reversal;
> >> >-        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> >> >-                                          INTEL_CX0_LANE0;
> >> >         struct ref_tracker *wakeref =
> >> >intel_cx0_phy_transaction_begin(encoder);
> >> >
> >> >         /*
> >> >@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct
> >> intel_encoder *encoder,
> >> >          */
> >> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
> >> >port_clock);
> >> >
> >> >-        /*
> >> >-         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> >> >-         * LN<Lane for maxPCLK> to "1" to enable PLL.
> >> >-         */
> >> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >> >port),
> >> >-                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> >> >-                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> >> >-
> >> >-        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
> >> == "1". */
> >> >-        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
> >> encoder->port),
> >> >-                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> >> >-                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> >> >-                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> >> >-                drm_warn(display->drm, "Port %c PLL not locked\n",
> >> >-                         phy_name(phy));
> >> >-
> >> >-        /*
> >> >-         * 11. Follow the Display Voltage Frequency Switching Sequence After
> >> >-         * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >-         */
> >> >-
> >> >         /*
> >> >          * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
> >> >          *
> >> >@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct
> >> intel_display *display,
> >> >         }
> >> > }
> >> >
> >> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder) {
> >> >+        struct intel_display *display = to_intel_display(encoder);
> >> >+        enum phy phy = intel_encoder_to_phy(encoder);
> >> >+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >> >+        bool lane_reversal = dig_port->lane_reversal;
> >> >+                                          INTEL_CX0_LANE0;
> >> >+        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
> >> >+                                        INTEL_CX0_LANE0;
> >> >+
> >> >+        struct ref_tracker *wakeref =
> >> >+ intel_cx0_phy_transaction_begin(encoder);
> >> >+
> >> >+        /*
> >> >+         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
> >> >+         * LN<Lane for maxPCLK> to "1" to enable PLL.
> >> >+         */
> >> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display,
> >> >+ encoder-
> >> >port),
> >> >+                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
> >> >+                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
> >> >+
> >> >+        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for
> >> >+ maxPCLK>
> >> == "1". */
> >> >+        if (intel_de_wait_us(display,
> >> >+ XELPDP_PORT_CLOCK_CTL(display,
> >> encoder->port),
> >> >+                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
> >> >+                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
> >> >+                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
> >> >+                drm_warn(display->drm, "Port %c PLL not locked\n",
> >> >+                         phy_name(phy));
> >> >+
> >> >+        /*
> >> >+         * 11. Follow the Display Voltage Frequency Switching Sequence After
> >> >+         * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >+         */
> >> >+
> >> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
> >> >+
> >> > void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder,
> >> >int
> >> >port_clock)  {
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >@@
> >> >-3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct
> >> >intel_encoder *encoder,
> >> >
> >> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >> >                 intel_mtl_tbt_pll_enable_clock(encoder,
> >> > crtc_state->port_clock);
> >> >+        else
> >> >+                intel_cx0pll_enable_clock(encoder);
> >> > }
> >> >
> >> > /*
> >> >@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct
> >> intel_encoder *encoder)
> >> >          * Frequency Change. We handle this step in bxt_set_cdclk().
> >> >          */
> >> >
> >> >-        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
> >> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >> >port),
> >> >-                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> >> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
> >> >port),
> >> >-                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> >> >-
> >> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
> >> >
> >> >@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct
> >> intel_encoder *encoder)
> >> >                              intel_cx0_get_pclk_pll_request(lane);
> >> > }
> >> >
> >> >+static void intel_cx0pll_disable_clock(struct intel_encoder
> >> >+*encoder) {
> >> >+        struct intel_display *display = to_intel_display(encoder);
> >> >+        struct ref_tracker *wakeref =
> >> >+intel_cx0_phy_transaction_begin(encoder);
> >> >+
> >> >+        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
> >> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display,
> >> >+ encoder-
> >> >port),
> >> >+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
> >> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display,
> >> >+ encoder-
> >> >port),
> >> >+                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
> >> >+
> >> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
> >> >+
> >> > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)  {
> >> >         struct intel_display *display = to_intel_display(encoder);
> >> >@@
> >> >-3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct
> >> >intel_encoder *encoder)
> >> >
> >> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >> >                 intel_mtl_tbt_pll_disable_clock(encoder);
> >> >+        else
> >> >+                intel_cx0pll_disable_clock(encoder);
> >> >+
> >> > }
> >> >
> >> > enum icl_port_dpll_id
> >> >@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct
> >> intel_display *display)
> >> >                             encoder->base.base.id,
> >> >encoder->base.name);
> >> >
> >> >                 intel_cx0pll_enable(encoder, &pll_state);
> >> >+                intel_cx0pll_enable_clock(encoder);
> >> >                 intel_cx0pll_disable(encoder);
> >> >+                intel_cx0pll_disable_clock(encoder);
> >> >         }
> >> > }
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >index 9aa84a430f09..59395076103c 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >> >@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
> >> >                      "asserting DPLL %s with no DPLL\n", str_on_off(state)))
> >> >                 return;
> >> >
> >> >-        cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> >> >-        INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> >> >-                                 "%s assertion failure (expected %s, current %s)\n",
> >> >-                                 pll->info->name, str_on_off(state),
> >> >-                                 str_on_off(cur_state));
> >> >+        if (DISPLAY_VER(display) < 14) {
> >> >+                cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
> >> >+                INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
> >> >+                                         "%s assertion failure (expected %s, current %s)\n",
> >> >+                                         pll->info->name, str_on_off(state),
> >> >+                                         str_on_off(cur_state));
> >> >+        }
> >> > }
> >> >
> >> > static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
> >> >--
> >> >2.34.1
> >> >