RE: [PATCH 2/3] drm/i915/cx0: Move step 12 to enable clock hook

[Gustavo Sousa <gustavo.sousa@intel.com>]

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.

--
Gustavo Sousa

>
>Regards,
>Suraj Kandpal
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >---
>> > 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
>> >