Re: [Intel-gfx] [PATCH 1/3] drm/i915/display/adl_s: Fix dpclka_cfgcr0_clk_off mapping

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 12, 2021 at 10:21:59AM -0800, José Roberto de Souza wrote:
> The cfgcr0/1_clk_off mapping is wrong for adl-s what could cause
> the wrong clock being disabled and leaving a not needed clock
> running consuming more power than needed.
> 
> Bspec: 50287
> Bspec: 53812
> Bspec: 53723
> Fixes: d6d2bc996e45 ("drm/i915/adl_s: Configure Port clock registers for ADL-S")
> Cc: Aditya Swarup <aditya.swarup@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c |  4 +++-
>  drivers/gpu/drm/i915/i915_reg.h          | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 2d6906f6995f..7631e080349d 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1585,7 +1585,9 @@ hsw_set_signal_levels(struct intel_dp *intel_dp,
>  static u32 icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
>  				     enum phy phy)
>  {
> -	if (IS_ROCKETLAKE(dev_priv)) {
> +	if (IS_ALDERLAKE_S(dev_priv)) {
> +		return ADLS_DPCLKA_CFGCR_DDI_CLK_OFF(phy);
> +	} else if (IS_ROCKETLAKE(dev_priv)) {
>  		return RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy);
>  	} else if (intel_phy_is_combo(dev_priv, phy)) {
>  		return ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 224ad897af34..7c69b50ccc5c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -10416,6 +10416,18 @@ enum skl_power_gate {
>  							ADLS_DPCLKA_DDIJ_SEL_MASK, \
>  							ADLS_DPCLKA_DDIK_SEL_MASK)
>  
> +#define _ADLS_DPCLKA_DDIA_CLK_OFF			REG_BIT(10)
> +#define _ADLS_DPCLKA_DDIB_CLK_OFF			REG_BIT(11)
> +#define _ADLS_DPCLKA_DDII_CLK_OFF			REG_BIT(24)
> +#define _ADLS_DPCLKA_DDIJ_CLK_OFF			REG_BIT(4)
> +#define _ADLS_DPCLKA_DDIK_CLK_OFF			REG_BIT(5)

So shose are apparently split between the two registers. Why aren't
we defining these so that it would be obvious which register they
live in? This stuff is confusing enough with the hw folks churning
the bits around randomly on every platform, so I don't think we
should add to the confusion by obfuscating the bit defines. I do
like that you named the bits, which isn't case for the other
platforms. Would be nice to fix it all up actually.

Hmm. However, this new defintion seem to match 
ICL_DPCLKA_CFGCR0_DDI_CLK_OFF() 100%. So how can this be fixing
something? Also ICL for sure can't have that many combo PHYs can
it? We should nuke the extra stuff from the ICL defintion if it's
no longer used.

> +#define ADLS_DPCLKA_CFGCR_DDI_CLK_OFF(phy)		_PICK((phy), \
> +							      _ADLS_DPCLKA_DDIA_CLK_OFF, \
> +							      _ADLS_DPCLKA_DDIB_CLK_OFF, \
> +							      _ADLS_DPCLKA_DDII_CLK_OFF, \
> +							      _ADLS_DPCLKA_DDIJ_CLK_OFF, \
> +							      _ADLS_DPCLKA_DDIK_CLK_OFF)
> +
>  /* CNL PLL */
>  #define DPLL0_ENABLE		0x46010
>  #define DPLL1_ENABLE		0x46014
> -- 
> 2.30.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx