________________________________________ From: Kandpal, Suraj <[email protected]> Sent: Wednesday, March 11, 2026 8:15 AM To: Kandpal, Suraj; Kahola, Mika; [email protected]; [email protected] Cc: Kahola, Mika Subject: RE: [PATCH v3 04/24] drm/i915/lt_phy: Refactor LT PHY PLL handling to use explicit PLL state
> > Subject: [PATCH v3 04/24] drm/i915/lt_phy: Refactor LT PHY PLL > > handling to use explicit PLL state > > > > The LT PHY implementation currently pulls PLL and port_clock > > information directly from the CRTC state. This ties the PHY > > programming logic too tightly to the CRTC state and makes it harder to > > clearly express the PHY’s own PLL configuration. > > > > Introduce an explicit "struct intel_lt_phy_pll_state" argument for the > > PHY functions and update callers accordingly. > > > > No functional change is intended — this is a preparatory cleanup for > > to bring LT PHY PLL handling as part of PLL framework. > > > > v2: DP, HDMI 2.0, and HDMI FRL modes are port of the VDR configuration 0 > > register. These modes are defined by bits 2:0. Decode these to > > differentiate DP and HDMI modes when programming PLL's. (Imre, > > Suraj) > > > > BSpec: 744921 > > No new line needed here Right, I will remove the new line. > > > > > Signed-off-by: Mika Kahola <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_lt_phy.c | 67 > > ++++++++++++++------- > > 1 file changed, 46 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.c > > b/drivers/gpu/drm/i915/display/intel_lt_phy.c > > index 8fe61cfdb706..76acffb2e840 100644 > > --- a/drivers/gpu/drm/i915/display/intel_lt_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_lt_phy.c > > @@ -32,6 +32,7 @@ > > INTEL_LT_PHY_LANE0) > > #define MODE_DP 3 > > #define MODE_HDMI_20 4 > > +#define MODE_HDMI_FRL 5 > > #define Q32_TO_INT(x) ((x) >> 32) > > #define Q32_TO_FRAC(x) ((x) & 0xFFFFFFFF) > > #define DCO_MIN_FREQ_MHZ 11850 > > @@ -1176,9 +1177,30 @@ intel_lt_phy_lane_reset(struct intel_encoder > > *encoder, > > intel_de_rmw(display, XELPDP_PORT_BUF_CTL2(display, port), > > lane_phy_pulse_status, 0); } > > > > +static bool intel_lt_phy_is_hdmi(const struct intel_lt_phy_pll_state > > +*ltpll) { > > + u8 mode = > REG_FIELD_GET8(LT_PHY_VDR_MODE_ENCODING_MASK, > > +ltpll->config[0]); > > + > > + if (mode == MODE_HDMI_20 || mode == MODE_HDMI_FRL) > > + return true; > > + > > + return false; > > +} > > + > > +static bool intel_lt_phy_is_dp(const struct intel_lt_phy_pll_state > > +*ltpll) { > > + u8 mode = > REG_FIELD_GET8(LT_PHY_VDR_MODE_ENCODING_MASK, > > +ltpll->config[0]); > > + > > + if (mode == MODE_DP) > > + return true; > > + > > + return false; > > +} > > + > > static void > > intel_lt_phy_program_port_clock_ctl(struct intel_encoder *encoder, > > - const struct intel_crtc_state *crtc_state, > > + const struct intel_lt_phy_pll_state *ltpll, > > + int port_clock, > > bool lane_reversal) > > { > > struct intel_display *display = to_intel_display(encoder); @@ > > -1195,17 > > +1217,16 @@ intel_lt_phy_program_port_clock_ctl(struct intel_encoder > > *encoder, > > * but since the register bits still remain the same we use > > * the same definition > > */ > > - if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) && > > - intel_hdmi_is_frl(crtc_state->port_clock)) > > + if (intel_lt_phy_is_hdmi(ltpll) && intel_hdmi_is_frl(port_clock)) > > val |= XELPDP_DDI_CLOCK_SELECT_PREP(display, > > XELPDP_DDI_CLOCK_SELECT_DIV18CLK); > > else > > val |= XELPDP_DDI_CLOCK_SELECT_PREP(display, > > XELPDP_DDI_CLOCK_SELECT_MAXPCLK); > > > > /* DP2.0 10G and 20G rates enable MPLLA*/ > > - if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == > > 2000000) > > + if (port_clock == 1000000 || port_clock == 2000000) > > val |= XELPDP_SSC_ENABLE_PLLA; > > else > > - val |= crtc_state->dpll_hw_state.ltpll.ssc_enabled ? > > XELPDP_SSC_ENABLE_PLLB : 0; > > + val |= ltpll->ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0; > > > > intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- > > >port), > > XELPDP_LANE1_PHY_CLOCK_SELECT | > XELPDP_FORWARD_CLOCK_UNGATE | > > @@ -1248,10 +1269,12 @@ static u32 > > intel_lt_phy_get_dp_clock(u8 rate) > > > > static bool > > intel_lt_phy_config_changed(struct intel_encoder *encoder, > > - const struct intel_crtc_state *crtc_state) > > + const struct intel_lt_phy_pll_state *ltpll) > > Why are we changing this? crtc_state is removed just that we don't need to pass around this large structure. We did similarly with the Cx0 series. > > > { > > + struct intel_display *display = to_intel_display(encoder); > > u8 val, rate; > > u32 clock; > > + u32 port_clock = intel_lt_phy_calc_port_clock(display, ltpll); > > No need to have this recalculated again > If we really want to change the arguments to send ltpll state Then I > recommend sending the crtc_state->port_clock as an argument > > Had a look at this part seems like you need it in later patches when you > switch to the dpll framework since > Crtc_state is not available so then it makes sense to recalculate. Port clock could still be passed as argument. Maybe that would be a cleaner solution rather than recalculate port clock. Thanks! -Mika- > > But please move the calculation right after intel_display declaration. > >Regards, >Suraj Kandpal > > Otherwise rest looks good to me here > > Regards, > Suraj Kandpal > > > > > val = intel_lt_phy_read(encoder, INTEL_LT_PHY_LANE0, > > LT_PHY_VDR_0_CONFIG); > > @@ -1262,9 +1285,9 @@ intel_lt_phy_config_changed(struct > intel_encoder > > *encoder, > > * using 1.62 Gbps clock since PHY PLL defaults to that > > * otherwise we always need to reconfigure it. > > */ > > - if (intel_crtc_has_dp_encoder(crtc_state)) { > > + if (intel_lt_phy_is_dp(ltpll)) { > > clock = intel_lt_phy_get_dp_clock(rate); > > - if (crtc_state->port_clock == 1620000 && crtc_state- > > >port_clock == clock) > > + if (port_clock == 1620000 && port_clock == clock) > > return false; > > } > > > > @@ -1759,41 +1782,41 @@ intel_lt_phy_pll_calc_state(struct > > intel_crtc_state *crtc_state, > > > > static void > > intel_lt_phy_program_pll(struct intel_encoder *encoder, > > - const struct intel_crtc_state *crtc_state) > > + const struct intel_lt_phy_pll_state *ltpll) > > { > > u8 owned_lane_mask = > intel_lt_phy_get_owned_lane_mask(encoder); > > int i, j, k; > > > > intel_lt_phy_write(encoder, owned_lane_mask, > LT_PHY_VDR_0_CONFIG, > > - crtc_state->dpll_hw_state.ltpll.config[0], > > MB_WRITE_COMMITTED); > > + ltpll->config[0], MB_WRITE_COMMITTED); > > intel_lt_phy_write(encoder, INTEL_LT_PHY_LANE0, > LT_PHY_VDR_1_CONFIG, > > - crtc_state->dpll_hw_state.ltpll.config[1], > > MB_WRITE_COMMITTED); > > + ltpll->config[1], MB_WRITE_COMMITTED); > > intel_lt_phy_write(encoder, owned_lane_mask, > LT_PHY_VDR_2_CONFIG, > > - crtc_state->dpll_hw_state.ltpll.config[2], > > MB_WRITE_COMMITTED); > > + ltpll->config[2], MB_WRITE_COMMITTED); > > > > for (i = 0; i <= 12; i++) { > > intel_lt_phy_write(encoder, INTEL_LT_PHY_LANE0, > > LT_PHY_VDR_X_ADDR_MSB(i), > > - crtc_state->dpll_hw_state.ltpll.addr_msb[i], > > + ltpll->addr_msb[i], > > MB_WRITE_COMMITTED); > > intel_lt_phy_write(encoder, INTEL_LT_PHY_LANE0, > > LT_PHY_VDR_X_ADDR_LSB(i), > > - crtc_state->dpll_hw_state.ltpll.addr_lsb[i], > > + ltpll->addr_lsb[i], > > MB_WRITE_COMMITTED); > > > > for (j = 3, k = 0; j >= 0; j--, k++) > > intel_lt_phy_write(encoder, INTEL_LT_PHY_LANE0, > > LT_PHY_VDR_X_DATAY(i, j), > > - crtc_state- > > >dpll_hw_state.ltpll.data[i][k], > > + ltpll->data[i][k], > > MB_WRITE_COMMITTED); > > } > > } > > > > static void > > intel_lt_phy_enable_disable_tx(struct intel_encoder *encoder, > > - const struct intel_crtc_state *crtc_state) > > + const struct intel_lt_phy_pll_state *ltpll, > > + u8 lane_count) > > { > > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > bool lane_reversal = dig_port->lane_reversal; > > - u8 lane_count = crtc_state->lane_count; > > bool is_dp_alt = > > intel_tc_port_in_dp_alt_mode(dig_port); > > enum intel_tc_pin_assignment tc_pin = @@ -1895,7 +1918,8 @@ > void > > intel_lt_phy_pll_enable(struct intel_encoder *encoder, > > intel_lt_phy_lane_reset(encoder, crtc_state->lane_count); > > > > /* 2. Program PORT_CLOCK_CTL register to configure clock muxes, > > gating, and SSC. */ > > - intel_lt_phy_program_port_clock_ctl(encoder, crtc_state, > > lane_reversal); > > + intel_lt_phy_program_port_clock_ctl(encoder, &crtc_state- > > >dpll_hw_state.ltpll, > > + crtc_state->port_clock, > > lane_reversal); > > > > /* 3. Change owned PHY lanes power to Ready state. */ > > intel_lt_phy_powerdown_change_sequence(encoder, > > owned_lane_mask, @@ -1905,12 +1929,12 @@ void > > intel_lt_phy_pll_enable(struct intel_encoder *encoder, > > * 4. Read the PHY message bus VDR register PHY_VDR_0_Config > check > > enabled PLL type, > > * encoded rate and encoded mode. > > */ > > - if (intel_lt_phy_config_changed(encoder, crtc_state)) { > > + if (intel_lt_phy_config_changed(encoder, > > +&crtc_state->dpll_hw_state.ltpll)) { > > /* > > * 5. Program the PHY internal PLL registers over PHY > message bus > > for the desired > > * frequency and protocol type > > */ > > - intel_lt_phy_program_pll(encoder, crtc_state); > > + intel_lt_phy_program_pll(encoder, &crtc_state- > > >dpll_hw_state.ltpll); > > > > /* 6. Use the P2P transaction flow */ > > /* > > @@ -2001,7 +2025,8 @@ void intel_lt_phy_pll_enable(struct > > intel_encoder *encoder, > > intel_lt_phy_powerdown_change_sequence(encoder, > > owned_lane_mask, > > XELPDP_P0_STATE_ACTIVE); > > > > - intel_lt_phy_enable_disable_tx(encoder, crtc_state); > > + intel_lt_phy_enable_disable_tx(encoder, &crtc_state- > > >dpll_hw_state.ltpll, > > + crtc_state->lane_count); > > intel_lt_phy_transaction_end(encoder, wakeref); } > > > > -- > > 2.43.0
