> Subject: RE: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook > > Quoting Kandpal, Suraj (2025-12-31 02:10:59-03:00) > >> > >> Quoting Suraj Kandpal (2025-12-30 05:31:41-03:00) > >> >Move the step to toggle powerdown sequence change for HDMI to > enable > >> >clock hook where it belongs according to its sequence. > >> >Do the required changes to make that work. > >> > >> This should probably be a squash into the previous patch? > > > >So reason for separate patch is that this requires me changing the > >argument of clock enable which is not because of the same logical > >reason that changes are being done in patch 1, hence a separate patch > >for changes that were brought about due to another reason. Had this been > just movement of step 12 then I would have squashed them. > > Hm... The previous patch is introducing intel_cx0pll_enable_clock() and says > it > is splitting the sequence in two, but then it ended up is leaving step 12 > behind. > If it is introducing intel_cx0pll_enable_clock(), it could as well have done > it > with a signature that allows step 12 to be done. > > IMO, here we are modifying that function to "make it right". This looks like > a > good fixup candidate to me. >
Let's see how the discussion in patch 1 goes then see how this can be dealt with Regards, Suraj Kandpal > -- > Gustavo Sousa > > > > >Regards, > >Suraj Kandpal > > > >> > >> -- > >> Gustavo Sousa > >> > >> > > >> >Signed-off-by: Suraj Kandpal <[email protected]> > >> >--- > >> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 37 > >> >++++++++++---------- > >> > 1 file changed, 19 insertions(+), 18 deletions(-) > >> > > >> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >> >index f3baba264e88..5edd293b533b 100644 > >> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >> >@@ -3281,21 +3281,6 @@ static void intel_cx0pll_enable(struct > >> intel_encoder *encoder, > >> > */ > >> > intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), > >> >port_clock); > >> > > >> >- /* > >> >- * 12. Toggle powerdown if HDMI is enabled on C10 PHY. > >> >- * > >> >- * Wa_13013502646: > >> >- * Fixes: HDMI lane to lane skew violations on C10 display PHYs. > >> >- * Workaround: Toggle powerdown value by setting first to P0 and > then > >> to P2, for both > >> >- * PHY lanes. > >> >- */ > >> >- if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) { > >> >- intel_cx0_powerdown_change_sequence(encoder, > >> INTEL_CX0_BOTH_LANES, > >> >- > >> >XELPDP_P0_STATE_ACTIVE); > >> >- intel_cx0_powerdown_change_sequence(encoder, > >> INTEL_CX0_BOTH_LANES, > >> >- > >> >XELPDP_P2_STATE_READY); > >> >- } > >> >- > >> > intel_cx0_phy_transaction_end(encoder, wakeref); } > >> > > >> >@@ -3379,7 +3364,8 @@ static int intel_mtl_tbt_clock_select(struct > >> intel_display *display, > >> > } > >> > } > >> > > >> >-static void intel_cx0pll_enable_clock(struct intel_encoder > >> >*encoder) > >> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder, > >> >+ const struct > >> >+intel_cx0pll_state > >> >+*pll_state) > >> > { > >> > struct intel_display *display = to_intel_display(encoder); > >> > enum phy phy = intel_encoder_to_phy(encoder); @@ -3412,6 > >> >+3398,21 @@ static void intel_cx0pll_enable_clock(struct > >> >+intel_encoder > >> *encoder) > >> > * Frequency Change. We handle this step in bxt_set_cdclk(). > >> > */ > >> > > >> >+ /* > >> >+ * 12. Toggle powerdown if HDMI is enabled on C10 PHY. > >> >+ * > >> >+ * Wa_13013502646: > >> >+ * Fixes: HDMI lane to lane skew violations on C10 display PHYs. > >> >+ * Workaround: Toggle powerdown value by setting first to > >> >+ P0 and then > >> to P2, for both > >> >+ * PHY lanes. > >> >+ */ > >> >+ if (!cx0pll_state_is_dp(pll_state) && pll_state->use_c10) { > >> >+ intel_cx0_powerdown_change_sequence(encoder, > >> INTEL_CX0_BOTH_LANES, > >> >+ > >> >XELPDP_P0_STATE_ACTIVE); > >> >+ intel_cx0_powerdown_change_sequence(encoder, > >> INTEL_CX0_BOTH_LANES, > >> >+ > >> >XELPDP_P2_STATE_READY); > >> >+ } > >> >+ > >> > intel_cx0_phy_transaction_end(encoder, wakeref); } > >> > > >> >@@ -3485,7 +3486,7 @@ 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); > >> >+ intel_cx0pll_enable_clock(encoder, > >> >+ &crtc_state->dpll_hw_state.cx0pll); > >> > } > >> > > >> > /* > >> >@@ -3808,7 +3809,7 @@ 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_enable_clock(encoder, &pll_state); > >> > intel_cx0pll_disable(encoder); > >> > intel_cx0pll_disable_clock(encoder); > >> > } > >> >-- > >> >2.34.1 > >> >
