> -----Original Message-----
> From: Kandpal, Suraj <[email protected]>
> Sent: Tuesday, 10 March 2026 6.03
> To: Kahola, Mika <[email protected]>; Deak, Imre <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: RE: [PATCH v2 04/24] drm/i915/lt_phy: Refactor LT PHY PLL handling
> to use explicit PLL state
>
> > Subject: RE: [PATCH v2 04/24] drm/i915/lt_phy: Refactor LT PHY PLL
> > handling to use explicit PLL state
> >
> > > -----Original Message-----
> > > From: Deak, Imre <[email protected]>
> > > Sent: Wednesday, 4 March 2026 16.05
> > > To: Kahola, Mika <[email protected]>
> > > Cc: [email protected]; [email protected]
> > > Subject: Re: [PATCH v2 04/24] drm/i915/lt_phy: Refactor LT PHY PLL
> > > handling to use explicit PLL state
> > >
> > > On Wed, Mar 04, 2026 at 01:14:03PM +0000, Mika Kahola wrote:
> > > > 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.
> > > >
> > > > Signed-off-by: Mika Kahola <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_lt_phy.c | 48
> > > > ++++++++++++---------
> > > > 1 file changed, 27 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..ebdcab58df4a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_lt_phy.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_lt_phy.c
> > > > @@ -1178,7 +1178,8 @@ intel_lt_phy_lane_reset(struct intel_encoder
> > > > *encoder,
> > > >
> > > > 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 +1196,17 @@ 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 (encoder->type == INTEL_OUTPUT_HDMI &&
> > >
> > > This was discussed already elsewhere: encoder->type can't be used to
> > > determine if the encoder is in HDMI or DP mode. In fact on LT PHY
> > > platforms encoder->type will be never INTEL_OUTPUT_HDMI, rather it's
> > INTEL_OUTPUT_DDI, where the actual mode of the DDI encoder could be
> > either HDMI or DP.
> > >
> > > The ideal would be to deduct the DP/HDMI mode from the
> > > intel_lt_phy_pll_state instead. AFAICS ltpll->config[0] is
> > > explicitly set to
> > > 0x84 for both the computed and table-specified HDMI PLL
> > > configurations
> > and config[0] is not 0x84 for any DP PLL configurations.
> > > Could be used that instead to distinguish the HDMI and DP configs?
> >
> > Right. I keep forgetting this that "encoder->type" can be both DP and HDMI.
> > The naming to me seems a bit misleading and hence I keep forgetting this.
> > Anyway, this approach cannot be used here but I must come up with
> > better solution.
> >
> > I could switch to use config[0] to distinguish HDMI.
>
> We could also use intel_encoder_is_dp() that makes sure we skip HDMI
> connectors in that case.
>
> Otherwise,
> mode = REG_FIELD_GET8(LT_PHY_VDR_MODE_ENCODING_MASK, val); mode & MODE_DP
> will also work
I have this latter one implemented but I haven't sent this patch to mailing
list yet. It seems that for VDR configuration 0 register the bits 2:0 define
the mode and for HDMI we either have 0x4 for HDMI and 0x5 for HDMI FRL.
-Mika-
>
> Regards,
> Suraj Kandpal
>
> >
> > Thanks for a review!
> >
> > -Mika-
> >
> > >
> > > > + 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 +1249,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)
> > > > {
> > > > + struct intel_display *display = to_intel_display(encoder);
> > > > u8 val, rate;
> > > > u32 clock;
> > > > + u32 port_clock = intel_lt_phy_calc_port_clock(display, ltpll);
> > > >
> > > > val = intel_lt_phy_read(encoder, INTEL_LT_PHY_LANE0,
> > > > LT_PHY_VDR_0_CONFIG);
> > > > @@ -1262,9 +1265,10 @@ 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 (encoder->type == INTEL_OUTPUT_DP ||
> > > > + encoder->type == INTEL_OUTPUT_EDP) {
> > >
> > > Same as above, encoder->type can't be used here.
> > >
> > > > 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 +1763,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 +1899,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 +1910,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 +2006,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
> > > >
