> -----Original Message-----
> From: Kandpal, Suraj <[email protected]>
> Sent: Tuesday, 18 November 2025 6.33
> To: Kahola, Mika <[email protected]>; [email protected];
> [email protected]
> Cc: Kahola, Mika <[email protected]>; Deak, Imre <[email protected]>
> Subject: RE: [PATCH v2 30/32] drm/i915/cx0: Get encoder configuration for C10
> and C20 PHY PLLs
>
> > Subject: [PATCH v2 30/32] drm/i915/cx0: Get encoder configuration for
> > C10 and C20 PHY PLLs
> >
> > For DDI initialization get configuration for C10 and C20 chips.
> >
> > v2: Getting configuration either for a C10 or on the PTL port B
> > eDP on TypeC PHY case for a C20 PHY PLL. Hence refer to this
> > case as "non_tc_phy" instead of "c10phy".
> >
> > Signed-off-by: Imre Deak <[email protected]>
> > Signed-off-by: Mika Kahola <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 81
> > ++++++++++++++++++++++--
> > 1 file changed, 75 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index be25a6fdd491..689bd3224919 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -4273,6 +4273,77 @@ static void mtl_ddi_get_config(struct
> > intel_encoder *encoder,
> > intel_ddi_get_config(encoder, crtc_state); }
> >
> > +static bool icl_ddi_tc_pll_is_tbt(const struct intel_dpll *pll) {
> > + return pll->info->id == DPLL_ID_ICL_TBTPLL; }
> > +
> > +static void mtl_ddi_cx0_get_config(struct intel_encoder *encoder,
> > + struct intel_crtc_state *crtc_state,
> > + enum icl_port_dpll_id port_dpll_id,
> > + enum intel_dpll_id pll_id)
> > +{
> > + struct intel_display *display = to_intel_display(encoder);
> > + struct icl_port_dpll *port_dpll;
> > + struct intel_dpll *pll;
> > + bool pll_active;
> > +
> > + port_dpll = &crtc_state->icl_port_dplls[port_dpll_id];
> > + pll = intel_get_dpll_by_id(display, pll_id);
> > +
> > + if (drm_WARN_ON(display->drm, !pll))
> > + return;
> > +
> > + port_dpll->pll = pll;
> > + pll_active = intel_dpll_get_hw_state(display, pll,
> > &port_dpll->hw_state);
> > + drm_WARN_ON(display->drm, !pll_active);
> > +
> > + icl_set_active_port_dpll(crtc_state, port_dpll_id);
> > +
> > + if (icl_ddi_tc_pll_is_tbt(crtc_state->intel_dpll))
> > + crtc_state->port_clock = intel_mtl_tbt_calc_port_clock(encoder);
> > + else
> > + crtc_state->port_clock = intel_dpll_get_freq(display,
> > crtc_state-
> > >intel_dpll,
> > + &crtc_state-
> > >dpll_hw_state);
> > +
> > + intel_ddi_get_config(encoder, crtc_state); }
> > +
> > +/*
> > + * Get the configuration for either a port using a C10 PHY PLL, or in
> > +the case of
> > + * the PTL port B eDP on TypeC PHY case the configuration of a port
> > +using a C20
> > + * PHY PLL.
> > + */
> > +static void mtl_ddi_non_tc_phy_get_config(struct intel_encoder *encoder,
> > + struct intel_crtc_state
> > *crtc_state) {
> > + struct intel_display *display = to_intel_display(encoder);
> > +
> > + /* TODO: Remove when the PLL manager is in place. */
>
> Is the comment needed anymore
At this point of patch series, we don't have pll manager yet in place so we can
keep this comment for a while. The last patch that enables pll manager and
framework will remove this comment.
>
> > + mtl_ddi_get_config(encoder, crtc_state);
> > + return;
>
> Why the early return code after this point then serves no purpose.
It serves a purpose that in this way the patch series is bisectable if we need
to do that one day. This will be removed by that last patch of the series.
>
> > +
> > + mtl_ddi_cx0_get_config(encoder, crtc_state, ICL_PORT_DPLL_DEFAULT,
> > + mtl_port_to_pll_id(display, encoder->port)); }
>
> Have the pll id in its own variable.
I think this change would come down to code readability. In my taste the
function call to mtl_port_pll_id() is not too confusing and hence would be ok
to use as is.
>
> > +
> > +static void mtl_ddi_tc_phy_get_config(struct intel_encoder *encoder,
> > + struct intel_crtc_state *crtc_state) {
> > + struct intel_display *display = to_intel_display(encoder);
> > +
> > + /* TODO: Remove when the PLL manager is in place. */
>
> No need for this comment
This is removed by the last patch.
>
> > + mtl_ddi_get_config(encoder, crtc_state);
> > + return;
>
> Same question why the early return ?
This is again for bisectablity of the patch series.
>
> > +
> > + if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
> > + mtl_ddi_cx0_get_config(encoder, crtc_state,
> > ICL_PORT_DPLL_DEFAULT,
> > + DPLL_ID_ICL_TBTPLL);
> > + else
> > + mtl_ddi_cx0_get_config(encoder, crtc_state,
> > ICL_PORT_DPLL_MG_PHY,
> > + mtl_port_to_pll_id(display, encoder-
> > >port)); }
>
> You can have the pll id as its one variable In fact you can call
> mtl_ddi_cx0_get_config just once if you have both port and pll id
> variables assigned After checking if intel_tc_port_in_tbt_alt_mode
This could be written that way we set these pll and port id's as variables
after checking if intel_tc_port_in_tbt_alt_mode(). However, to me this change
wouldn't improve code readability but simply would be written differently.
Thanks,
Mika
>
> Regards,
> Suraj Kandpal
>
> > +
> > static void dg2_ddi_get_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *crtc_state) { @@ -
> > 4310,11 +4381,6 @@ static void icl_ddi_combo_get_config(struct
> > intel_encoder *encoder,
> > intel_ddi_get_config(encoder, crtc_state); }
> >
> > -static bool icl_ddi_tc_pll_is_tbt(const struct intel_dpll *pll) -{
> > - return pll->info->id == DPLL_ID_ICL_TBTPLL;
> > -}
> > -
> > static enum icl_port_dpll_id
> > icl_ddi_tc_port_pll_type(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state) @@ -5260,7
> > +5326,10 @@ void intel_ddi_init(struct intel_display *display,
> > encoder->enable_clock = intel_mtl_pll_enable_clock;
> > encoder->disable_clock = intel_mtl_pll_disable_clock;
> > encoder->port_pll_type = intel_mtl_port_pll_type;
> > - encoder->get_config = mtl_ddi_get_config;
> > + if (intel_encoder_is_tc(encoder))
> > + encoder->get_config = mtl_ddi_tc_phy_get_config;
> > + else
> > + encoder->get_config = mtl_ddi_non_tc_phy_get_config;
> > } else if (display->platform.dg2) {
> > encoder->enable_clock = intel_mpllb_enable;
> > encoder->disable_clock = intel_mpllb_disable;
> > --
> > 2.34.1
