> > 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
>
> >
> > 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?
>
> > {
> > + 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.
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
