On Thu, 20 Feb 2025, Matt Roper <matthew.d.roper@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
