[PATCH 1/4] drm/i915/display: Make refclk fetching logic reusable

[Jani Nikula]

On Thu, 20 Feb 2025, Matt Roper <matthew.d.roper at intel.com> wrote:
> There's cdclk-specific code to obtain the display reference clock,
> either by reading a strap register, or by using a platform-specific
> hardcoded value.  There's at least one other place in our drivers that
> potentially needs this clock frequency, so refactor the logic to make it
> more generally usable.
>
> While we're at it, change the fallback frequency we assume if the strap
> readout gives us something unrecognizable to 38.4MHz for platforms with
> display version 14 and above.  38.4MHz seems to be the sole frequency
> that's actually been used in recent history (since MTL), so this is
> probably the safest guess to make going forward.

Refactoring and functional changes should be kept separate.

>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 44 +++++++++++++---------
>  drivers/gpu/drm/i915/display/intel_cdclk.h |  1 +
>  2 files changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index c6cfc57a0346..57b01f8a7be8 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1636,38 +1636,48 @@ static u8 xe3lpd_calc_voltage_level(int cdclk)
>  	return 0;
>  }
>  
> -static void icl_readout_refclk(struct intel_display *display,
> -			       struct intel_cdclk_config *cdclk_config)
> +static u32 icl_readout_refclk(struct intel_display *display)

Wondering about the use of u32 for this. The cdclk_config->ref member is
unsigned int.

>  {
>  	u32 dssm = intel_de_read(display, SKL_DSSM) & ICL_DSSM_CDCLK_PLL_REFCLK_MASK;
>  
>  	switch (dssm) {
> -	default:
> -		MISSING_CASE(dssm);
> -		fallthrough;
>  	case ICL_DSSM_CDCLK_PLL_REFCLK_24MHz:
> -		cdclk_config->ref = 24000;
> -		break;
> +		return 24000;
>  	case ICL_DSSM_CDCLK_PLL_REFCLK_19_2MHz:
> -		cdclk_config->ref = 19200;
> -		break;
> +		return 19200;
>  	case ICL_DSSM_CDCLK_PLL_REFCLK_38_4MHz:
> -		cdclk_config->ref = 38400;
> -		break;
> +		return 38400;
> +	default:
> +		MISSING_CASE(dssm);
> +		return DISPLAY_VER(display) >= 14 ? 38400 : 24000;
>  	}
>  }
>  
> +/**
> + * intel_display_get_refclk - Returns the display reference clock
> + * @display: display instance
> + *
> + * Returns the display reference clock in KHz.  The display reference clock
> + * is defined by the SoC; on some platforms the proper value should be read
> + * from a hardware strap register, while on others there's only a single
> + * possible value.
> + */

The comment is useful in general, but I don't think there's any need to
make it a kernel-doc comment.

> +u32 intel_display_get_refclk(struct intel_display *display)

The file's intel_cdclk.c, the name should be intel_cdclk_<something>.


BR,
Jani.

> +{
> +	if (display->platform.dg2)
> +		return 38400;
> +	else if (DISPLAY_VER(display) >= 11)
> +		return icl_readout_refclk(display);
> +	else
> +		return 19200;
> +}
> +
>  static void bxt_de_pll_readout(struct intel_display *display,
>  			       struct intel_cdclk_config *cdclk_config)
>  {
>  	u32 val, ratio;
>  
> -	if (display->platform.dg2)
> -		cdclk_config->ref = 38400;
> -	else if (DISPLAY_VER(display) >= 11)
> -		icl_readout_refclk(display, cdclk_config);
> -	else
> -		cdclk_config->ref = 19200;
> +	cdclk_config->ref = intel_display_get_refclk(display);
>  
>  	val = intel_de_read(display, BXT_DE_PLL_ENABLE);
>  	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0 ||
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 6b0e7a41eba3..3cfbe1f2b6b5 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -65,6 +65,7 @@ void intel_init_cdclk_hooks(struct intel_display *display);
>  void intel_update_max_cdclk(struct intel_display *display);
>  void intel_update_cdclk(struct intel_display *display);
>  u32 intel_read_rawclk(struct intel_display *display);
> +u32 intel_display_get_refclk(struct intel_display *display);
>  bool intel_cdclk_clock_changed(const struct intel_cdclk_config *a,
>  			       const struct intel_cdclk_config *b);
>  int intel_mdclk_cdclk_ratio(struct intel_display *display,

-- 
Jani Nikula, Intel