Re: [PATCH] drm/i915/ptl: Use everywhere the correct DDI port clock select mask

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

Quoting Imre Deak (2025-05-12 11:26:00-03:00)
>The PTL XELPDP_PORT_CLOCK_CTL register XELPDP_DDI_CLOCK_SELECT field's
>size is 5 bits vs. the earlier platforms where its size is 4 bits. Make
>sure the field is read-out/programmed everywhere correctly, according to
>the above.
>
>Cc: Mika Kahola <mika.kahola@intel.com>
>Cc: stable@vger.kernel.org # v6.13+
>Signed-off-by: Imre Deak <imre.deak@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 27 +++++++------------
> .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 15 ++++++++---
> 2 files changed, 21 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index b09f724c3046b..a82b93cbc81d2 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -2763,9 +2763,9 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
>         val |= XELPDP_FORWARD_CLOCK_UNGATE;
> 
>         if (!is_dp && is_hdmi_frl(port_clock))
>-                val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_DIV18CLK);
>+                val |= XELPDP_DDI_CLOCK_SELECT_PREP(display, XELPDP_DDI_CLOCK_SELECT_DIV18CLK);
>         else
>-                val |= XELPDP_DDI_CLOCK_SELECT(XELPDP_DDI_CLOCK_SELECT_MAXPCLK);
>+                val |= XELPDP_DDI_CLOCK_SELECT_PREP(display, XELPDP_DDI_CLOCK_SELECT_MAXPCLK);
> 
>         /* TODO: HDMI FRL */
>         /* DP2.0 10G and 20G rates enable MPLLA*/
>@@ -2776,7 +2776,7 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
> 
>         intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>                      XELPDP_LANE1_PHY_CLOCK_SELECT | XELPDP_FORWARD_CLOCK_UNGATE |
>-                     XELPDP_DDI_CLOCK_SELECT_MASK | XELPDP_SSC_ENABLE_PLLA |
>+                     XELPDP_DDI_CLOCK_SELECT_MASK(display) | XELPDP_SSC_ENABLE_PLLA |
>                      XELPDP_SSC_ENABLE_PLLB, val);
> }
> 
>@@ -3099,10 +3099,7 @@ int intel_mtl_tbt_calc_port_clock(struct intel_encoder *encoder)
> 
>         val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port));
> 
>-        if (DISPLAY_VER(display) >= 30)
>-                clock = REG_FIELD_GET(XE3_DDI_CLOCK_SELECT_MASK, val);
>-        else
>-                clock = REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val);
>+        clock = XELPDP_DDI_CLOCK_SELECT_GET(display, val);
> 
>         drm_WARN_ON(display->drm, !(val & XELPDP_FORWARD_CLOCK_UNGATE));
>         drm_WARN_ON(display->drm, !(val & XELPDP_TBT_CLOCK_REQUEST));
>@@ -3170,13 +3167,9 @@ static void intel_mtl_tbt_pll_enable(struct intel_encoder *encoder,
>          * clock muxes, gating and SSC
>          */
> 
>-        if (DISPLAY_VER(display) >= 30) {
>-                mask = XE3_DDI_CLOCK_SELECT_MASK;
>-                val |= XE3_DDI_CLOCK_SELECT(intel_mtl_tbt_clock_select(display, crtc_state->port_clock));
>-        } else {
>-                mask = XELPDP_DDI_CLOCK_SELECT_MASK;
>-                val |= XELPDP_DDI_CLOCK_SELECT(intel_mtl_tbt_clock_select(display, crtc_state->port_clock));
>-        }
>+        mask = XELPDP_DDI_CLOCK_SELECT_MASK(display);
>+        val |= XELPDP_DDI_CLOCK_SELECT_PREP(display,
>+                                            intel_mtl_tbt_clock_select(display, crtc_state->port_clock));
> 
>         mask |= XELPDP_FORWARD_CLOCK_UNGATE;
>         val |= XELPDP_FORWARD_CLOCK_UNGATE;
>@@ -3289,7 +3282,7 @@ static void intel_cx0pll_disable(struct intel_encoder *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, 0);
>+                     XELPDP_DDI_CLOCK_SELECT_MASK(display), 0);
>         intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>                      XELPDP_FORWARD_CLOCK_UNGATE, 0);
> 
>@@ -3338,7 +3331,7 @@ static void intel_mtl_tbt_pll_disable(struct intel_encoder *encoder)
>          * 5. Program PORT CLOCK CTRL register to disable and gate clocks
>          */
>         intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port),
>-                     XELPDP_DDI_CLOCK_SELECT_MASK |
>+                     XELPDP_DDI_CLOCK_SELECT_MASK(display) |
>                      XELPDP_FORWARD_CLOCK_UNGATE, 0);
> 
>         /* 6. Program DDI_CLK_VALFREQ to 0. */
>@@ -3367,7 +3360,7 @@ intel_mtl_port_pll_type(struct intel_encoder *encoder,
>          * handling is done via the standard shared DPLL framework.
>          */
>         val = intel_de_read(display, XELPDP_PORT_CLOCK_CTL(display, encoder->port));
>-        clock = REG_FIELD_GET(XELPDP_DDI_CLOCK_SELECT_MASK, val);
>+        clock = XELPDP_DDI_CLOCK_SELECT_GET(display, val);
> 
>         if (clock == XELPDP_DDI_CLOCK_SELECT_MAXPCLK ||
>             clock == XELPDP_DDI_CLOCK_SELECT_DIV18CLK)
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>index 960f7f778fb81..59c22beaf1de5 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>@@ -192,10 +192,17 @@
> 
> #define   XELPDP_TBT_CLOCK_REQUEST                        REG_BIT(19)
> #define   XELPDP_TBT_CLOCK_ACK                                REG_BIT(18)
>-#define   XELPDP_DDI_CLOCK_SELECT_MASK                        REG_GENMASK(15, 12)
>-#define   XE3_DDI_CLOCK_SELECT_MASK                        REG_GENMASK(16, 12)
>-#define   XELPDP_DDI_CLOCK_SELECT(val)                        REG_FIELD_PREP(XELPDP_DDI_CLOCK_SELECT_MASK, val)
>-#define   XE3_DDI_CLOCK_SELECT(val)                        REG_FIELD_PREP(XE3_DDI_CLOCK_SELECT_MASK, val)
>+#define   _XELPDP_DDI_CLOCK_SELECT_MASK                        REG_GENMASK(15, 12)
>+#define   _XE3_DDI_CLOCK_SELECT_MASK                        REG_GENMASK(16, 12)

Since bit 16 is unused for pre-Xe3 display IPs, I wonder if we should
simply redefine XELPDP_DDI_CLOCK_SELECT_MASK to be REG_GENMASK(16, 12)
and add a comment noting changes by display IP? Are we aware of
something that would be wired to bit 16 that would prevent us from doing
that?

I remember something similar was done to other register fields in the
past. Some examples I found:

    3fe856180c94 ("drm/i915/xe3lpd: Add new bit range of MAX swing setup")
    247bdac958fc ("drm/i915/adl_p: Add ddb allocation support")
    d7e449a858ec ("drm/i915: Just use icl+ definition for PLANE_WM blocks field")

--
Gustavo Sousa

>+#define   XELPDP_DDI_CLOCK_SELECT_MASK(display)                (DISPLAY_VER(display) >= 30 ? \
>+                                                         _XE3_DDI_CLOCK_SELECT_MASK : _XELPDP_DDI_CLOCK_SELECT_MASK)
>+#define   XELPDP_DDI_CLOCK_SELECT_PREP(display, val)        (DISPLAY_VER(display) >= 30 ? \
>+                                                         REG_FIELD_PREP(_XE3_DDI_CLOCK_SELECT_MASK, (val)) : \
>+                                                         REG_FIELD_PREP(_XELPDP_DDI_CLOCK_SELECT_MASK, (val)))
>+#define   XELPDP_DDI_CLOCK_SELECT_GET(display, val)        (DISPLAY_VER(display) >= 30 ? \
>+                                                         REG_FIELD_GET(_XE3_DDI_CLOCK_SELECT_MASK, (val)) : \
>+                                                         REG_FIELD_GET(_XELPDP_DDI_CLOCK_SELECT_MASK, (val)))
>+
> #define   XELPDP_DDI_CLOCK_SELECT_NONE                        0x0
> #define   XELPDP_DDI_CLOCK_SELECT_MAXPCLK                0x8
> #define   XELPDP_DDI_CLOCK_SELECT_DIV18CLK                0x9
>-- 
>2.44.2
>