| Message ID | 20240312163639.172321-3-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable LNL display | expand |
On Tue, Mar 12, 2024 at 01:36:33PM -0300, Gustavo Sousa wrote: > Currently, only Xe2LPD uses CDCLK PLL as the source of MDCLK and > previous display IPs use the CD2XCLK. There will be changes in code > paths common to those platforms that will rely on which source is being > used. As such, let's make that information explicit with the addition of > the predicate function mdclk_source_is_cdclk_pll(). > > Arguably, an enum could be created, but using a boolean should suffice > here, since we there are only two possible sources and the logic that > will rely on it will be very localized. > > In order to get the code into a more consistent state, let's also take > this opportunity to hook the selection of CDCLK_CTL's "MDCLK Source > Select" to that new function. Even though currently only > MDCLK_SOURCE_SEL_CDCLK_PLL will be returned, having this extra logic is > arguably better than keeping stuff untied and prone to bugs. > > v2: > - Extract mdclk_source_is_cdclk_pll() out of xe2lpd_mdclk_source_sel() > to make latter only about the register's field. > > Bspec: 69090 You might also add 68861 here since that's where we find out that Xe2 and beyond should always use the CDCLK PLL as the MDCLK source (rather than the CD2X which would still be the register field's default value if we didn't touch it). Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++++++++++++- > drivers/gpu/drm/i915/i915_reg.h | 4 +++- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index be268c6abe21..ad0f03e51e4a 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1876,6 +1876,19 @@ static bool cdclk_pll_is_unknown(unsigned int vco) > return vco == ~0; > } > > +static bool mdclk_source_is_cdclk_pll(struct drm_i915_private *i915) > +{ > + return DISPLAY_VER(i915) >= 20; > +} > + > +static u32 xe2lpd_mdclk_source_sel(struct drm_i915_private *i915) > +{ > + if (mdclk_source_is_cdclk_pll(i915)) > + return MDCLK_SOURCE_SEL_CDCLK_PLL; > + > + return MDCLK_SOURCE_SEL_CD2XCLK; > +} > + > static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i915, > const struct intel_cdclk_config *old_cdclk_config, > const struct intel_cdclk_config *new_cdclk_config, > @@ -1980,7 +1993,7 @@ static u32 bxt_cdclk_ctl(struct drm_i915_private *i915, > val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE; > > if (DISPLAY_VER(i915) >= 20) > - val |= MDCLK_SOURCE_SEL_CDCLK_PLL; > + val |= xe2lpd_mdclk_source_sel(i915); > else > val |= skl_cdclk_decimal(cdclk); > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8823531b8770..d6193c858a74 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5891,7 +5891,9 @@ enum skl_power_gate { > #define CDCLK_FREQ_540 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 1) > #define CDCLK_FREQ_337_308 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 2) > #define CDCLK_FREQ_675_617 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 3) > -#define MDCLK_SOURCE_SEL_CDCLK_PLL REG_BIT(25) > +#define MDCLK_SOURCE_SEL_MASK REG_GENMASK(25, 25) > +#define MDCLK_SOURCE_SEL_CD2XCLK REG_FIELD_PREP(MDCLK_SOURCE_SEL_MASK, 0) > +#define MDCLK_SOURCE_SEL_CDCLK_PLL REG_FIELD_PREP(MDCLK_SOURCE_SEL_MASK, 1) > #define BXT_CDCLK_CD2X_DIV_SEL_MASK REG_GENMASK(23, 22) > #define BXT_CDCLK_CD2X_DIV_SEL_1 REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 0) > #define BXT_CDCLK_CD2X_DIV_SEL_1_5 REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 1) > -- > 2.44.0 >
On Tue, Mar 12, 2024 at 09:49:05AM -0700, Matt Roper wrote: >On Tue, Mar 12, 2024 at 01:36:33PM -0300, Gustavo Sousa wrote: >> Currently, only Xe2LPD uses CDCLK PLL as the source of MDCLK and >> previous display IPs use the CD2XCLK. There will be changes in code >> paths common to those platforms that will rely on which source is being >> used. As such, let's make that information explicit with the addition of >> the predicate function mdclk_source_is_cdclk_pll(). >> >> Arguably, an enum could be created, but using a boolean should suffice >> here, since we there are only two possible sources and the logic that >> will rely on it will be very localized. >> >> In order to get the code into a more consistent state, let's also take >> this opportunity to hook the selection of CDCLK_CTL's "MDCLK Source >> Select" to that new function. Even though currently only >> MDCLK_SOURCE_SEL_CDCLK_PLL will be returned, having this extra logic is >> arguably better than keeping stuff untied and prone to bugs. >> >> v2: >> - Extract mdclk_source_is_cdclk_pll() out of xe2lpd_mdclk_source_sel() >> to make latter only about the register's field. >> >> Bspec: 69090 > >You might also add 68861 here since that's where we find out that Xe2 I added this while applying. thanks Lucas De Marchi
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index be268c6abe21..ad0f03e51e4a 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -1876,6 +1876,19 @@ static bool cdclk_pll_is_unknown(unsigned int vco) return vco == ~0; } +static bool mdclk_source_is_cdclk_pll(struct drm_i915_private *i915) +{ + return DISPLAY_VER(i915) >= 20; +} + +static u32 xe2lpd_mdclk_source_sel(struct drm_i915_private *i915) +{ + if (mdclk_source_is_cdclk_pll(i915)) + return MDCLK_SOURCE_SEL_CDCLK_PLL; + + return MDCLK_SOURCE_SEL_CD2XCLK; +} + static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i915, const struct intel_cdclk_config *old_cdclk_config, const struct intel_cdclk_config *new_cdclk_config, @@ -1980,7 +1993,7 @@ static u32 bxt_cdclk_ctl(struct drm_i915_private *i915, val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE; if (DISPLAY_VER(i915) >= 20) - val |= MDCLK_SOURCE_SEL_CDCLK_PLL; + val |= xe2lpd_mdclk_source_sel(i915); else val |= skl_cdclk_decimal(cdclk); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8823531b8770..d6193c858a74 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5891,7 +5891,9 @@ enum skl_power_gate { #define CDCLK_FREQ_540 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 1) #define CDCLK_FREQ_337_308 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 2) #define CDCLK_FREQ_675_617 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 3) -#define MDCLK_SOURCE_SEL_CDCLK_PLL REG_BIT(25) +#define MDCLK_SOURCE_SEL_MASK REG_GENMASK(25, 25) +#define MDCLK_SOURCE_SEL_CD2XCLK REG_FIELD_PREP(MDCLK_SOURCE_SEL_MASK, 0) +#define MDCLK_SOURCE_SEL_CDCLK_PLL REG_FIELD_PREP(MDCLK_SOURCE_SEL_MASK, 1) #define BXT_CDCLK_CD2X_DIV_SEL_MASK REG_GENMASK(23, 22) #define BXT_CDCLK_CD2X_DIV_SEL_1 REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 0) #define BXT_CDCLK_CD2X_DIV_SEL_1_5 REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 1)
Currently, only Xe2LPD uses CDCLK PLL as the source of MDCLK and previous display IPs use the CD2XCLK. There will be changes in code paths common to those platforms that will rely on which source is being used. As such, let's make that information explicit with the addition of the predicate function mdclk_source_is_cdclk_pll(). Arguably, an enum could be created, but using a boolean should suffice here, since we there are only two possible sources and the logic that will rely on it will be very localized. In order to get the code into a more consistent state, let's also take this opportunity to hook the selection of CDCLK_CTL's "MDCLK Source Select" to that new function. Even though currently only MDCLK_SOURCE_SEL_CDCLK_PLL will be returned, having this extra logic is arguably better than keeping stuff untied and prone to bugs. v2: - Extract mdclk_source_is_cdclk_pll() out of xe2lpd_mdclk_source_sel() to make latter only about the register's field. Bspec: 69090 Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 4 +++- 2 files changed, 17 insertions(+), 2 deletions(-)