> 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 <[email protected]> > > > >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 <[email protected]> > >Signed-off-by: Suraj Kandpal <[email protected]> > >--- > > 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 > >
