Re: [PATCH v2 12/26] drm/i915/ltphy: Add function to calculate LT PHY port clock

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 10/24/2025 3:36 PM, Suraj Kandpal wrote:
> The current algorithm is very wrong and was made wrose with
> changes in algorithm that were done. It needs to be rewritten
> to be able to extract the correct values and get the right port clock.


I think you mean previous versions of the algorithm here.

Since the algorithm is introduced first time in this patch, the commit 
message should reflect that.

As I understand, the function intel_lt_phy_calc_hdmi_port_clock() helps 
to derive the port clock from the LT phy register values which helps in 
readout and compare the LT phyold/new states.

Few of the things that should be mentioned in the commit message:

-Why this is needed for HDMI.

-The fact  that the function to calculate LT Phy port clock is the 
inverse of the function provided in Bspec: 74667.


>
> Bspec: 74667
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
> V1 -> V2: Correct comment grammar
> ---
>   drivers/gpu/drm/i915/display/intel_dpll.c   |  2 +
>   drivers/gpu/drm/i915/display/intel_lt_phy.c | 74 +++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_lt_phy.h |  3 +
>   3 files changed, 79 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
> index 8c3ef5867a12..2e1f67be8eda 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> @@ -1247,6 +1247,8 @@ static int xe3plpd_crtc_compute_clock(struct intel_atomic_state *state,
>   		return ret;
>   
>   	/* TODO: Do the readback via intel_compute_shared_dplls() */
> +	crtc_state->port_clock =
> +			intel_lt_phy_calc_port_clock(encoder, crtc_state);
>   
>   	crtc_state->hw.adjusted_mode.crtc_clock = intel_crtc_dotclock(crtc_state);
>   
> diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.c b/drivers/gpu/drm/i915/display/intel_lt_phy.c
> index 0b1b320f5c3a..c7a109e4422c 100644
> --- a/drivers/gpu/drm/i915/display/intel_lt_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_lt_phy.c
> @@ -1237,6 +1237,80 @@ intel_lt_phy_pll_is_ssc_enabled(struct intel_crtc_state *crtc_state,
>   	return false;
>   }
>   
> +static int
> +intel_lt_phy_calc_hdmi_port_clock(const struct intel_lt_phy_pll_state *lt_state)
> +{
> +#define DIV_CONST 10000000

This is not used.


> +#define REF_CLK 38400


Since this is 38400Khz, REF_CLK_KHZ would be better.


> +#define REGVAL(i) (				\
> +	(lt_state->data[i][3])		|	\
> +	(lt_state->data[i][2] << 8)	|	\
> +	(lt_state->data[i][1] << 16)	|	\
> +	(lt_state->data[i][0] << 24)		\
> +)
> +
> +	int clk = 0;
> +	u32 d8, pll_reg_5, pll_reg_3, pll_reg_57, m2div_frac, m2div_int;
> +	u64 temp0, temp1;
> +
> +	/*
> +	 * d7 max val can be 10 so 4 bits.
> +	 * postdiv can be max 9 hence it needs 4 bits.
> +	 * d8 = loop_cnt / 2 and loop count can be max 255
> +	 * hence we it needs only 7 bits to but 8 bits is given to it.
> +	 * PLL_reg57 = ((D7 << 24) + (postdiv << 15) + (D8 << 7) + D6_new);
> +	 * d4 max val can be 256 so 9 bits.
> +	 * d3 can be max 9 hence needs 4 bits.
> +	 * d1 can be max 2 hence needs 2 bits.
> +	 * m2div can never be > 511 hence m2div_int
> +	 * needs up to 9 bits but it is given 10.
> +	 * PLL_reg3 = (uint32_t)((D4 << 21) + (D3 << 18) + (D1 << 15)+ (m2div_int << 5));

The algorithm uses + in the formulae above but as per my understanding 
the intention is to combine the non-overlapping bits.

So I agree with the above reasoning and the method to derive `d8` and 
`m2div_int` from the register values.

Since this is not very explicit, the comment can be bit improved to 
mention the formulae first and then the reasoning about the bits each 
constituent takes, something like:

         /*
          * The algorithm uses '+' to combine bitfields when 
constructing PLL_reg3 and PLL_reg57:
          * PLL_reg57 = (D7 << 24) + (postdiv << 15) + (D8 << 7) + D6_new;
          * PLL_reg3 = (D4 << 21) + (D3 << 18) + (D1 << 15) + (m2div_int 
<< 5);
          *
          * However, this is likely intended to be a bitwise OR operation,
          * as each field occupies distinct, non-overlapping bits in the 
register.
          *
          * PLL_reg57 is composed of following fields packed into a 
32-bit value:
          * - D7: max value 10 -> fits in 4 bits -> placed at bits 24-27
          * - postdiv: max value 9 -> fits in 4 bits -> placed at bits 15-18
          * - D8: derived from loop_cnt / 2, max 127 -> fits in 7 bits 
(though 8 bits are given to it) -> placed at bits 7-14
          * - D6_new: fits in lower 7 bits -> placed at bits 0-6
          * PLL_reg57 = (D7 << 24) | (postdiv << 15) | (D8 << 7) | D6_new;
          *
          * Similarly, PLL_reg3 is packed as:
          * - D4: max value 256 -> fits in 9 bits -> placed at bits 21-29
          * - D3: max value 9 -> fits in 4 bits -> placed at bits 18-21
          * - D1: max value 2 -> fits in 2 bits -> placed at bits 15-16
          * - m2div_int: max value 511 -> fits in 9 bits (10 bits 
allocated) -> placed at bits 5-14
          * PLL_reg3 = (D4 << 21) | (D3 << 18) | (D1 << 15) | (m2div_int 
<< 5);
          */

> +	 */
> +	pll_reg_5 = REGVAL(2);
> +	pll_reg_3 = REGVAL(1);
> +	pll_reg_57 = REGVAL(3);
> +	m2div_frac = pll_reg_5;
> +
> +	d8 = (pll_reg_57 & REG_GENMASK(14, 7)) >> 7;
> +	m2div_int = (pll_reg_3  & REG_GENMASK(14, 5)) >> 5;
> +	temp0 = ((u64)m2div_frac * REF_CLK) >> 32;
> +	temp1 = (u64)m2div_int * REF_CLK;
> +	if (d8 == 0)
> +		return 0;
> +
> +	clk = div_u64((temp1 + temp0), d8 * 10);

temp1 + temp0 is effectively m2div. Since  m2div = val / 2 / refclk_mhz 
and val  = d8 * clk * 10; m2div should be multiplied with a factor of 2.

Perhaps I am missing something? It would be good to document how this is 
derived.


Regards,

Ankit

> +
> +	return clk;
> +}
> +
> +int
> +intel_lt_phy_calc_port_clock(struct intel_encoder *encoder,
> +			     const struct intel_crtc_state *crtc_state)
> +{
> +	int clk;
> +	const struct intel_lt_phy_pll_state *lt_state =
> +		&crtc_state->dpll_hw_state.ltpll;
> +	u8 mode, rate;
> +
> +	mode = REG_FIELD_GET8(LT_PHY_VDR_MODE_ENCODING_MASK,
> +			      lt_state->config[0]);
> +	/*
> +	 * For edp/dp read the clock value from the tables
> +	 * and return the clock as the algorithm used for
> +	 * calculating the port clock does not exactly matches
> +	 * with edp/dp clock.
> +	 */
> +	if (mode == MODE_DP) {
> +		rate = REG_FIELD_GET8(LT_PHY_VDR_RATE_ENCODING_MASK,
> +				      lt_state->config[0]);
> +		clk = intel_lt_phy_get_dp_clock(rate);
> +	} else {
> +		clk = intel_lt_phy_calc_hdmi_port_clock(lt_state);
> +	}
> +
> +	return clk;
> +}
> +
>   int
>   intel_lt_phy_pll_calc_state(struct intel_crtc_state *crtc_state,
>   			    struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.h b/drivers/gpu/drm/i915/display/intel_lt_phy.h
> index 3f255c9b0f96..5b4e0d9c940f 100644
> --- a/drivers/gpu/drm/i915/display/intel_lt_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_lt_phy.h
> @@ -10,12 +10,15 @@
>   
>   struct intel_encoder;
>   struct intel_crtc_state;
> +struct intel_lt_phy_pll_state;
>   
>   void intel_lt_phy_pll_enable(struct intel_encoder *encoder,
>   			     const struct intel_crtc_state *crtc_state);
>   int
>   intel_lt_phy_pll_calc_state(struct intel_crtc_state *crtc_state,
>   			    struct intel_encoder *encoder);
> +int intel_lt_phy_calc_port_clock(struct intel_encoder *encoder,
> +				 const struct intel_crtc_state *crtc_state);
>   
>   #define HAS_LT_PHY(display) (DISPLAY_VER(display) >= 35)
>