> -----Original Message-----
> From: Deak, Imre <imre.d...@intel.com>
> Sent: Monday, 12 May 2025 23.26
> To: Sousa, Gustavo <gustavo.so...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; intel...@lists.freedesktop.org; Kahola,
> Mika
> <mika.kah...@intel.com>; sta...@vger.kernel.org
> Subject: Re: [PATCH] drm/i915/ptl: Use everywhere the correct DDI port clock
> select mask
>
> On Mon, May 12, 2025 at 05:03:52PM -0300, Gustavo Sousa wrote:
> > Quoting Imre Deak (2025-05-12 11:26:00-03:00)
> >
> > [...]
> >
> > >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?
>
> Not sure if a register bit is not used, unless it's defined as reserved.
> For pre-Xe3 (pre-PTL) bits 16-17 are not defined as reserved.
Bits 16-17 are not defined or marked as reserved, so Imre's approach seems safe.
Tested-by: Mika Kahola <mika.kah...@intel.com>
Reviewed-by: Mika Kahola <mika.kah...@intel.com>
>
> > 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
> > >
