RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two parts

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

Quoting Kandpal, Suraj (2025-12-31 02:07:35-03:00)
>> Subject: Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two
>> parts
>> 
>> Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00)
>> >From: Mika Kahola <mika.kahola@intel.com>
>> >
>> >Split PLL enabling/disabling in two parts - one for pll setting pll
>> >dividers and second one to enable/disable pll clock. PLL clock
>> >enabling/disbling happens via encoder->enable_clock/disable_clock
>> >function hook. The reason for doing this is that we need to make sure
>> >the clock enablement happens after PPS ON step to be inline with the
>> >sequences which we end up violating otherwise. As a result of this
>> >violation we end up in a hanged state if machine stays idle for more
>> >that 15 mins.
>> 
>> So, it appears this started happening when we Cx0 code was integrated into
>> the DPLL framework and then the driver started enabling the PHY PLL/clock
>> too early, right?
>> 
>> I am lacking some context/background here due to my unfamiliarity with pre-
>> MTL platforms, but why I exactly do we program the PLLs before the modeset
>> sequence?  Is it related to the shared nature of PLLs for platforms pre-
>> C10/pre-C20?  If so, do we really need to do the same for
>> C10/C20 PHYs, since we have dedicated PLLs for them?
>> 
>> (Sorry for asking here and a bit too late.  Probably the better place to ask this
>> was in series that integrated Cx0 into the DPLL framework.)
>
>
>Right it used to be actually because of the shared nature of PLL's. With c10 c20 we moved
>to a different framework where we called the the sequence together using hooks like enable_clock
>and disable_clock since there was not a lot of time of time to refactor the dpll_shared_framework to
>a framework with supported individual ones.
>Now that we had time we shifted cx0 back to the previous framework but missed defer the clock enablement

Then, if we move forward with this, perhaps this patch deserves a
"Fixes:" trailer.

>To later during enable clock time so that we honor the sequence, why we had to do this is even though its not shared PLL anymore is
>To make sure this framework is backward compatible too.

I see.

If the requirement for programming PLL parameters early was only because
of shared PLLs and we do not have that same requirements for C10/C20, I
would argue that doing the whole programming at once and only during the
"enable clock" phase of the encoder would make the driver more compliant
with the Bspec.

I also noticed that, for the older displays, the "enable clock" thing is
the part that selects the PLL (which is already enabled) as the port's
"clock source".  With C10/C20 we are actually deferring the PLL enabling
to the "enable clock" phase of the port while, I believe, the
expectation of intel_dpll_funcs::enable() is that the PLL would be
enabled when the function returned, which would not be exactly true for
C10/C20 after this patch.

What if, in intel_dpll_mgr, we made the distinction between enabling the
PLL early and enabling it at "intel_ddi_enable_clock()" time?

--
Gustavo Sousa

>Also we had to move cx0 pll framework back to dpll framework because the previous can work well as long as the ports are static hence aren’t
>As future proof , we plan to move LT PHY back here too once this ages well.
>
>> 
>> >
>> >PLL state verification happens now earlier than the clock is enabled
>> >which causes a drm warn to be thrown. Silence this warning by allowing
>> >this check for only earlier platforms than MeteorLake.
>> >
>> >Bspec: 49190
>> 
>> This Bspec page is not invalid for platforms using C10/C20 PHYs.
>> 
>> We probably want to use these instead:
>> 
>> Bspec: 65448, 68849
>>
>
>Sure will replace them.
>
>Regards,
>Suraj Kandpal
> 
>> --
>> Gustavo Sousa
>> 
>> >Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 87 ++++++++++++-------
>> >drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +--
>> > 2 files changed, 64 insertions(+), 35 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >index 7288065d2461..f3baba264e88 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct
>> >intel_encoder *encoder,  {
>> >         int port_clock = pll_state->use_c10 ? pll_state->c10.clock : pll_state-
>> >c20.clock;
>> >         struct intel_display *display = to_intel_display(encoder);
>> >-        enum phy phy = intel_encoder_to_phy(encoder);
>> >         struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> >         bool lane_reversal = dig_port->lane_reversal;
>> >-        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
>> >-                                          INTEL_CX0_LANE0;
>> >         struct ref_tracker *wakeref =
>> >intel_cx0_phy_transaction_begin(encoder);
>> >
>> >         /*
>> >@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct
>> intel_encoder *encoder,
>> >          */
>> >         intel_de_write(display, DDI_CLK_VALFREQ(encoder->port),
>> >port_clock);
>> >
>> >-        /*
>> >-         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>> >-         * LN<Lane for maxPCLK> to "1" to enable PLL.
>> >-         */
>> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >-                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>> >-                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
>> >-
>> >-        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
>> == "1". */
>> >-        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
>> encoder->port),
>> >-                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>> >-                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>> >-                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
>> >-                drm_warn(display->drm, "Port %c PLL not locked\n",
>> >-                         phy_name(phy));
>> >-
>> >-        /*
>> >-         * 11. Follow the Display Voltage Frequency Switching Sequence After
>> >-         * Frequency Change. We handle this step in bxt_set_cdclk().
>> >-         */
>> >-
>> >         /*
>> >          * 12. Toggle powerdown if HDMI is enabled on C10 PHY.
>> >          *
>> >@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct
>> intel_display *display,
>> >         }
>> > }
>> >
>> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder) {
>> >+        struct intel_display *display = to_intel_display(encoder);
>> >+        enum phy phy = intel_encoder_to_phy(encoder);
>> >+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> >+        bool lane_reversal = dig_port->lane_reversal;
>> >+                                          INTEL_CX0_LANE0;
>> >+        u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 :
>> >+                                        INTEL_CX0_LANE0;
>> >+
>> >+        struct ref_tracker *wakeref =
>> >+ intel_cx0_phy_transaction_begin(encoder);
>> >+
>> >+        /*
>> >+         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>> >+         * LN<Lane for maxPCLK> to "1" to enable PLL.
>> >+         */
>> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >+                     intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>> >+                     intel_cx0_get_pclk_pll_request(maxpclk_lane));
>> >+
>> >+        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK>
>> == "1". */
>> >+        if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display,
>> encoder->port),
>> >+                             intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>> >+                             intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>> >+                             XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL))
>> >+                drm_warn(display->drm, "Port %c PLL not locked\n",
>> >+                         phy_name(phy));
>> >+
>> >+        /*
>> >+         * 11. Follow the Display Voltage Frequency Switching Sequence After
>> >+         * Frequency Change. We handle this step in bxt_set_cdclk().
>> >+         */
>> >+
>> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
>> >+
>> > void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int
>> >port_clock)  {
>> >         struct intel_display *display = to_intel_display(encoder); @@
>> >-3472,6 +3484,8 @@ 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);
>> > }
>> >
>> > /*
>> >@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct
>> intel_encoder *encoder)
>> >          * Frequency Change. We handle this step in bxt_set_cdclk().
>> >          */
>> >
>> >-        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
>> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >-                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>> >-        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >-                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
>> >-
>> >         intel_cx0_phy_transaction_end(encoder, wakeref);  }
>> >
>> >@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct
>> intel_encoder *encoder)
>> >                              intel_cx0_get_pclk_pll_request(lane);
>> > }
>> >
>> >+static void intel_cx0pll_disable_clock(struct intel_encoder *encoder)
>> >+{
>> >+        struct intel_display *display = to_intel_display(encoder);
>> >+        struct ref_tracker *wakeref =
>> >+intel_cx0_phy_transaction_begin(encoder);
>> >+
>> >+        /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. */
>> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>> >+        intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder-
>> >port),
>> >+                     XELPDP_FORWARD_CLOCK_UNGATE, 0);
>> >+
>> >+        intel_cx0_phy_transaction_end(encoder, wakeref); }
>> >+
>> > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder)  {
>> >         struct intel_display *display = to_intel_display(encoder); @@
>> >-3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct
>> >intel_encoder *encoder)
>> >
>> >         if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> >                 intel_mtl_tbt_pll_disable_clock(encoder);
>> >+        else
>> >+                intel_cx0pll_disable_clock(encoder);
>> >+
>> > }
>> >
>> > enum icl_port_dpll_id
>> >@@ -3783,6 +3808,8 @@ 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_disable(encoder);
>> >+                intel_cx0pll_disable_clock(encoder);
>> >         }
>> > }
>> >diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >index 9aa84a430f09..59395076103c 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
>> >@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display,
>> >                      "asserting DPLL %s with no DPLL\n", str_on_off(state)))
>> >                 return;
>> >
>> >-        cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
>> >-        INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
>> >-                                 "%s assertion failure (expected %s, current %s)\n",
>> >-                                 pll->info->name, str_on_off(state),
>> >-                                 str_on_off(cur_state));
>> >+        if (DISPLAY_VER(display) < 14) {
>> >+                cur_state = intel_dpll_get_hw_state(display, pll, &hw_state);
>> >+                INTEL_DISPLAY_STATE_WARN(display, cur_state != state,
>> >+                                         "%s assertion failure (expected %s, current %s)\n",
>> >+                                         pll->info->name, str_on_off(state),
>> >+                                         str_on_off(cur_state));
>> >+        }
>> > }
>> >
>> > static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>> >--
>> >2.34.1
>> >