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

[Imre Deak <imre.deak@intel.com>]

On Wed, Jan 14, 2026 at 09:12:57AM +0530, 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.

This is missing the rationale for the change. As I understand it
appeares to fix a PLL enabling/disabling timeout, if so that should be
described here in the commit log and also explain how the change fixes
the timeout, IOW wrt. what are the enabling/disabling steps gets
reordered?

Also, what about CMTG? It may require an output/DDI's PLL for another
ouput/DDI even if the former output/DDI is disabled.

Also, there is an ordering issue in the change, see below.

> 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.
> 
> While at it also add the necessary argument to cx0_enable_clock
> so that we can move step 12 of the enable sequence.
> 
> v2:
> - Move state verification to enable_clock() function for
> MTL+ platforms
> - Squash patch 1 & 2 (Gustavo)
> - Use correct Bspec references (Gustavo)
> - Fix build error (Michal)
> 
> Bspec: 65448, 68849
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> Reviewed-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 120 +++++++++++-------
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   7 +-
>  2 files changed, 80 insertions(+), 47 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..3418a3ed28fd 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,42 +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.
> -	 *
> -	 * 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);
>  }
>  
> @@ -3403,6 +3364,56 @@ static int intel_mtl_tbt_clock_select(struct intel_display *display,
>  	}
>  }
>  
> +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);
> +	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);
> +
> +	/*
> +	 * 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.
> +	 *
> +	 * 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);
> +}
> +
>  void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int port_clock)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
> @@ -3468,10 +3479,16 @@ void intel_mtl_pll_enable(struct intel_encoder *encoder,
>  void intel_mtl_pll_enable_clock(struct intel_encoder *encoder,
>  				const struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct intel_dpll *pll = crtc_state->intel_dpll;
>  	struct intel_digital_port *dig_port = enc_to_dig_port(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, &crtc_state->dpll_hw_state.cx0pll);
> +
> +	assert_dpll_enabled(display, pll);
>  }
>  
>  /*
> @@ -3567,12 +3584,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 +3597,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);

During a modeset disable sequence encoder::disable_clock() is called
first and only afterwards intel_dpll_funcs::disable() is called. So the
above will incorrectly reorder the clearing of the clock-select and
forward-clock-ungate flags wrt. the rest of the steps
intel_cx0pll_disable(), i.e. not matching now bspec at all.

> +}
> +
>  void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)
>  {
>  	struct intel_display *display = to_intel_display(encoder);
> @@ -3635,6 +3660,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 +3811,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, &pll_state);
>  		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..040c97d81302 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -298,7 +298,8 @@ void intel_dpll_enable(const struct intel_crtc_state *crtc_state)
>  
>  	if (old_mask) {
>  		drm_WARN_ON(display->drm, !pll->on);
> -		assert_dpll_enabled(display, pll);
> +		if (DISPLAY_VER(display) < 14)
> +			assert_dpll_enabled(display, pll);
>  		goto out;
>  	}
>  	drm_WARN_ON(display->drm, pll->on);
> @@ -342,7 +343,9 @@ void intel_dpll_disable(const struct intel_crtc_state *crtc_state)
>  		    pll->info->name, pll->active_mask, pll->on,
>  		    crtc->base.base.id, crtc->base.name);
>  
> -	assert_dpll_enabled(display, pll);
> +	if (DISPLAY_VER(display) < 14)
> +		assert_dpll_enabled(display, pll);
> +
>  	drm_WARN_ON(display->drm, !pll->on);
>  
>  	pll->active_mask &= ~pipe_mask;
> -- 
> 2.34.1
>