diff mbox series

[1/5] drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2

Message ID 20240626050056.3996349-2-ankit.k.nautiyal@intel.com (mailing list archive)
State New
Headers show
Series Add HDMI PLL Algorithm for SNPS/C10PHY | expand

Commit Message

Nautiyal, Ankit K June 26, 2024, 5 a.m. UTC 
Add helper _intel_phy_compute_hdmi_tmds_pll to calculate the necessary
parameters for configuring the HDMI PLL for SNPS MPLLB and C10 PHY.

The pll parameters are computed for desired pixel clock, curve data
and other inputs used for interpolation and finally stored in the
pll_state. Bspec:54032

Currently the helper is used to compute PLLs for DG2 SNPS PHY.
Support for computing Plls for C10 PHY is added in subsequent patches.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 .../drm/i915/display/intel_pll_algorithm.c    | 252 ++++++++++++++++++
 .../drm/i915/display/intel_pll_algorithm.h    |  39 +++
 3 files changed, 292 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_pll_algorithm.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_pll_algorithm.h

Comments

Jani Nikula June 26, 2024, 10:04 a.m. UTC | #1 
On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Add helper _intel_phy_compute_hdmi_tmds_pll to calculate the necessary
> parameters for configuring the HDMI PLL for SNPS MPLLB and C10 PHY.
>
> The pll parameters are computed for desired pixel clock, curve data
> and other inputs used for interpolation and finally stored in the
> pll_state. Bspec:54032
>
> Currently the helper is used to compute PLLs for DG2 SNPS PHY.
> Support for computing Plls for C10 PHY is added in subsequent patches.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Okay, usually the high level review should come first, but there are so
many annoying details here that they distract review. See inline.

> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  .../drm/i915/display/intel_pll_algorithm.c    | 252 ++++++++++++++++++
>  .../drm/i915/display/intel_pll_algorithm.h    |  39 +++
>  3 files changed, 292 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_pll_algorithm.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_pll_algorithm.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 4022e4499382..f1a293110bc6 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -334,6 +334,7 @@ i915-y += \
>  	display/intel_lspcon.o \
>  	display/intel_lvds.o \
>  	display/intel_panel.o \
> +	display/intel_pll_algorithm.o \

We probably need to figure out a new name for this. That's too generic
for what it is.

>  	display/intel_pps.o \
>  	display/intel_qp_tables.o \
>  	display/intel_sdvo.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_pll_algorithm.c b/drivers/gpu/drm/i915/display/intel_pll_algorithm.c
> new file mode 100644
> index 000000000000..d935715bd3ab
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_pll_algorithm.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Synopsys, Inc., Intel Corporation
> + */
> +
> +#include <linux/math.h>

Blank line here.

> +#include "i915_reg.h"
> +#include "intel_ddi.h"
> +#include "intel_ddi_buf_trans.h"
> +#include "intel_de.h"
> +#include "intel_display_types.h"
> +#include "intel_snps_phy.h"
> +#include "intel_snps_phy_regs.h"
> +#include "intel_pll_algorithm.h"

Do you really need all of the includes here?

> +
> +#define INTEL_SNPS_PHY_HDMI_4999MHZ 4999999900ull
> +#define INTEL_SNPS_PHY_HDMI_16GHZ 16000000000ull
> +#define INTEL_SNPS_PHY_HDMI_9999MHZ (2 * INTEL_SNPS_PHY_HDMI_4999MHZ)
> +
> +#define CURVE0_MULTIPLIER 1000000000
> +#define CURVE1_MULTIPLIER 100
> +#define CURVE2_MULTIPLIER 1000000000000
> +
> +static int64_t interp(s64 x, s64 x1, s64 x2, s64 y1, s64 y2)

Please use kernel types instead of C99 types.

> +{
> +	s64 dydx;
> +
> +	dydx = DIV_ROUND_UP((y2 - y1) * 100000, (x2 - x1));
> +
> +	return (y1 + DIV_ROUND_UP(dydx * (x - x1), 100000));
> +}
> +
> +static void get_ana_cp_int_prop(u32 vco_clk,
> +				u32 refclk_postscalar,
> +				int mpll_ana_v2i,
> +				int c, int a,
> +				const u64 curve_freq_hz[2][8],
> +				const u64 curve_0[2][8],
> +				const u64 curve_1[2][8],
> +				const u64 curve_2[2][8],
> +				u32 *ana_cp_int,
> +				u32 *ana_cp_prop)
> +{
> +	u64 vco_div_refclk_float;
> +	u64 o_397ced90;
> +	u64 o_20c634d6;
> +	u64 o_20c634d4;
> +	u64 o_72019306;
> +	u64 o_6593e82b;
> +	u64 o_5cefc329;
> +	u64 o_49960328;
> +	u64 o_544adb37;
> +	u64 o_4ef74e66;

That's pretty obfuscated. Result of some automated conversion? Can we
have some more sensible names?

> +	u32 ana_cp_int_temp;
> +	u64 temp1, temp2, temp3, temp4;
> +
> +	vco_div_refclk_float = vco_clk * (1000000000000 / refclk_postscalar);
> +	o_397ced90 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
> +			    curve_0[c][a], curve_0[c][a + 1]);
> +
> +	o_20c634d6 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
> +			    curve_2[c][a], curve_2[c][a + 1]);
> +
> +	o_20c634d4 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
> +			    curve_1[c][a], curve_1[c][a + 1]);
> +
> +	o_20c634d4 /= CURVE1_MULTIPLIER;
> +
> +	temp1 = o_20c634d6 * (4 - mpll_ana_v2i);
> +
> +	o_72019306 = temp1 / 16000;
> +	o_6593e82b = temp1 / 160;
> +
> +	temp2 = ((112008301 * (vco_div_refclk_float / 100000)));
> +	o_5cefc329 = CURVE2_MULTIPLIER * (temp2 / (o_397ced90 * (o_20c634d4 / CURVE0_MULTIPLIER)));
> +
> +	ana_cp_int_temp = min(DIV_ROUND_CLOSEST(o_5cefc329 / o_72019306, CURVE2_MULTIPLIER), 127);
> +	ana_cp_int_temp = max(1, ana_cp_int_temp);
> +
> +	*ana_cp_int = ana_cp_int_temp;
> +
> +	o_49960328 = o_72019306 * ana_cp_int_temp;
> +
> +	temp3 = o_20c634d4 * (o_49960328 * o_397ced90 / CURVE0_MULTIPLIER);
> +	o_544adb37 = int_sqrt(DIV_ROUND_UP(temp3, vco_div_refclk_float) * (1000000000000 / 55));
> +
> +	temp4 = DIV_ROUND_UP(vco_div_refclk_float, 1000000);
> +	o_4ef74e66 = (1460281 * DIV_ROUND_UP(o_544adb37 * temp4, o_20c634d4));
> +
> +	*ana_cp_prop = max(1, min(DIV_ROUND_UP(o_4ef74e66, o_6593e82b), 127));
> +}
> +
> +static int _intel_phy_compute_hdmi_tmds_pll(u64 pixel_clock, u32 refclk,

What's with the underscore prefix? It's not a convention we have.

> +					    u32 ref_range,
> +					    u32 ana_cp_int_gs,
> +					    u32 ana_cp_prop_gs,
> +					    const u64 curve_freq_hz[2][8],
> +					    const u64 curve_0[2][8],
> +					    const u64 curve_1[2][8],
> +					    const u64 curve_2[2][8],
> +					    u32 prescaler_divider,
> +					    struct pll_output_params *pll_params)
> +{
> +	/*datarate 10khz */

Please put space around /* and */.

> +	u64 datarate = pixel_clock * 10000;
> +	u32 ssc_up_spread = 1;
> +	u32 mpll_div5_en = 1;
> +	u32 hdmi_div = 1;
> +	u32 ana_cp_int;
> +	u32 ana_cp_prop;
> +	u32 refclk_postscalar = refclk >> prescaler_divider;
> +	u32 tx_clk_div;
> +	u64 vco_clk;
> +	u32 vco_div_refclk_integer;
> +	u32 vco_div_refclk_fracn;
> +	u32 fracn_quot;
> +	u32 fracn_rem;
> +	u32 fracn_den;
> +	u32 fracn_en;
> +	u32 pmix_en;
> +	u32 multiplier;
> +	int mpll_ana_v2i;
> +	int ana_freq_vco;
> +	int c, a, j;
> +
> +	if (pixel_clock < 25175 || pixel_clock > 600000)
> +		return -EINVAL;
> +
> +	/* Select appropriate v2i point */
> +	if (datarate <= INTEL_SNPS_PHY_HDMI_9999MHZ) {
> +		mpll_ana_v2i = 2;
> +		tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_9999MHZ / datarate);
> +	} else {
> +		mpll_ana_v2i = 3;
> +		tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_16GHZ / datarate);
> +	}
> +	vco_clk = (datarate << tx_clk_div) >> 1;
> +
> +	/* Highly accurate division, calculate fraction to 32 bits of precision */
> +	vco_div_refclk_integer = vco_clk / refclk_postscalar;
> +	vco_div_refclk_fracn = ((vco_clk % refclk_postscalar) << 32) / refclk_postscalar;
> +	fracn_quot = vco_div_refclk_fracn >> 16;
> +	fracn_rem = vco_div_refclk_fracn & 0xffff;
> +	fracn_rem = fracn_rem - (fracn_rem >> 15);
> +	fracn_den = 0xffff;
> +	fracn_en = (fracn_quot != 0 || fracn_rem != 0) ? 1 : 0;
> +	pmix_en = fracn_en;
> +	multiplier = (vco_div_refclk_integer - 16) * 2;
> +	/* Curve selection for ana_cp_* calculations. One curve hardcoded per v2i range */
> +	c = mpll_ana_v2i - 2;
> +
> +	/* Find the right segment of the table */
> +	for (j = 0; j < 8; j += 2) {

i is the usual loop variable.

> +		if (vco_clk <= curve_freq_hz[c][j + 1]) {
> +			a = j;
> +			ana_freq_vco = 3 - (a >> 1);
> +			break;
> +		}
> +	}
> +
> +	get_ana_cp_int_prop(vco_clk, refclk_postscalar, mpll_ana_v2i, c, a,
> +			    curve_freq_hz, curve_0, curve_1, curve_2,
> +			    &ana_cp_int, &ana_cp_prop);
> +
> +	pll_params->ssc_up_spread = ssc_up_spread;
> +	pll_params->mpll_div5_en = mpll_div5_en;
> +	pll_params->hdmi_div = hdmi_div;
> +	pll_params->ana_cp_int = ana_cp_int;
> +	pll_params->refclk_postscalar = refclk_postscalar;
> +	pll_params->tx_clk_div = tx_clk_div;
> +	pll_params->fracn_quot = fracn_quot;
> +	pll_params->fracn_rem = fracn_rem;
> +	pll_params->fracn_den = fracn_den;
> +	pll_params->fracn_en = fracn_en;
> +	pll_params->pmix_en = pmix_en;
> +	pll_params->multiplier = multiplier;
> +	pll_params->ana_cp_prop = ana_cp_prop;
> +	pll_params->mpll_ana_v2i = mpll_ana_v2i;
> +	pll_params->ana_freq_vco = ana_freq_vco;
> +
> +	return 0;
> +}
> +
> +int intel_snps_phy_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_mpllb_state *pll_state)
> +{
> +	struct pll_output_params pll_params;
> +	u32 refclk = 100000000;
> +	u32 prescaler_divider = 1;
> +	u32 ref_range = 3;
> +	u32 ana_cp_int_gs = 64;
> +	u32 ana_cp_prop_gs = 124;
> +	int ret;
> +	/* x axis frequencies. One curve in each array per v2i point */
> +	const u64 dg2_curve_freq_hz[2][8] = {
> +		{2500000000, 3000000000, 3000000000, 3500000000, 3500000000, 4000000000, 4000000000, 5000000000},
> +		{4000000000, 4600000000, 4601000000, 5400000000, 5401000000, 6600000000, 6601000000, 8001000000}};
> +
> +	/* y axis heights multiplied with 1000000000 */
> +	const u64 dg2_curve_0[2][8] = {
> +		{34149871, 39803269, 36034544, 40601014, 35646940, 40016109, 35127987, 41889522},
> +		{70000000, 78770454, 70451838, 80427119, 70991400, 84230173, 72945921, 87064218}};
> +
> +	/* Multiplied with 100 */
> +	const u64 dg2_curve_1[2][8] = {
> +		{85177000000000, 79385227160000, 95672603580000, 88857207160000, 109379790900000, 103528193900000, 131941242400000, 117279000000000},
> +		{60255000000000, 55569000000000, 72036000000000, 69509000000000,  81785000000000, 731030000000000, 96591000000000, 69077000000000}};
> +
> +	/* Multiplied with 1000000000000 */
> +	const u64 dg2_curve_2[2][8] = {
> +		{2186930000, 2835287134, 2395395343, 2932270687, 2351887545, 2861031697, 2294149152, 3091730000},
> +		{4560000000, 5570000000, 4610000000, 5770000000, 4670000000, 6240000000, 4890000000, 6600000000}
> +	};

These can be static const instead of on the stack. Please put spaces
between { } and the numbers. Please put the final }; on a line of its
own. Put these as first variables in the function.

Just eyeballing, the constants look big enough to warrant ULL.

> +
> +	ret = _intel_phy_compute_hdmi_tmds_pll(pixel_clock, refclk, ref_range,
> +					       ana_cp_int_gs, ana_cp_prop_gs,
> +					       dg2_curve_freq_hz, dg2_curve_0,
> +					       dg2_curve_1, dg2_curve_2, prescaler_divider,
> +					       &pll_params);
> +
> +	if (ret)
> +		return ret;
> +
> +	pll_state->clock = pixel_clock;
> +	pll_state->ref_control =
> +		REG_FIELD_PREP(SNPS_PHY_REF_CONTROL_REF_RANGE, ref_range);
> +	pll_state->mpllb_cp =
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_INT, pll_params.ana_cp_int) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_PROP, pll_params.ana_cp_prop) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_INT_GS, ana_cp_int_gs) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_PROP_GS, ana_cp_prop_gs);
> +	pll_state->mpllb_div =
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_DIV5_CLK_EN, pll_params.mpll_div5_en) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_TX_CLK_DIV, pll_params.tx_clk_div) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_PMIX_EN, pll_params.pmix_en) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_V2I, pll_params.mpll_ana_v2i) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FREQ_VCO, pll_params.ana_freq_vco);
> +	pll_state->mpllb_div2 =
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_REF_CLK_DIV, prescaler_divider) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_MULTIPLIER, pll_params.multiplier) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_HDMI_DIV, pll_params.hdmi_div);
> +	pll_state->mpllb_fracn1 =
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_CGG_UPDATE_EN, 1) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_EN, pll_params.fracn_en) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_DEN, pll_params.fracn_den);
> +	pll_state->mpllb_fracn2 =
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_QUOT, pll_params.fracn_quot) |
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_REM, pll_params.fracn_rem);
> +	pll_state->mpllb_sscen =
> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_SSC_UP_SPREAD, pll_params.ssc_up_spread);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_pll_algorithm.h b/drivers/gpu/drm/i915/display/intel_pll_algorithm.h
> new file mode 100644
> index 000000000000..83a620fb3db2
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_pll_algorithm.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Synopsys, Inc., Intel Corporation
> + */
> +
> +#ifndef __INTEL_PLL_ALGORITHM_H__
> +#define __INTEL_PLL_ALGORITHM_H__
> +
> +#include <linux/types.h>
> +
> +struct drm_i915_private;
> +struct intel_atomic_state;
> +struct intel_crtc;
> +struct intel_crtc_state;
> +struct intel_encoder;
> +struct intel_mpllb_state;
> +enum phy;

What's the deal with the forward declarations? You need exactly one of
them in this file.

> +
> +struct pll_output_params {
> +	u32 ssc_up_spread;
> +	u32 mpll_div5_en;
> +	u32 hdmi_div;
> +	u32 ana_cp_int;
> +	u32 ana_cp_prop;
> +	u32 refclk_postscalar;
> +	u32 tx_clk_div;
> +	u32 fracn_quot;
> +	u32 fracn_rem;
> +	u32 fracn_den;
> +	u32 fracn_en;
> +	u32 pmix_en;
> +	u32 multiplier;
> +	int mpll_ana_v2i;
> +	int ana_freq_vco;
> +};

This is internal to the implementation and should be in the .c file.

> +
> +int intel_snps_phy_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_mpllb_state *pll_state);

Usually context params go first.

> +
> +#endif /* __INTEL_PLL_ALGORITHM_H__ */
kernel test robot June 26, 2024, 6:12 p.m. UTC | #2 
Hi Ankit,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes drm-tip/drm-tip drm-xe/drm-xe-next linus/master v6.10-rc5 next-20240625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ankit-Nautiyal/drm-i915-display-Add-support-for-SNPS-PHY-HDMI-PLL-algorithm-for-DG2/20240626-131209
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20240626050056.3996349-2-ankit.k.nautiyal%40intel.com
patch subject: [PATCH 1/5] drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2
config: i386-buildonly-randconfig-006-20240626 (https://download.01.org/0day-ci/archive/20240627/202406270107.cweaZpry-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270107.cweaZpry-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406270107.cweaZpry-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/gpu/drm/i915/display/intel_pll_algorithm.o: in function `get_ana_cp_int_prop.constprop.0':
>> intel_pll_algorithm.c:(.text+0x2b): undefined reference to `__divdi3'
>> ld: intel_pll_algorithm.c:(.text+0xc2): undefined reference to `__divdi3'
   ld: intel_pll_algorithm.c:(.text+0xef): undefined reference to `__divdi3'
   ld: intel_pll_algorithm.c:(.text+0x153): undefined reference to `__divdi3'
   ld: intel_pll_algorithm.c:(.text+0x180): undefined reference to `__divdi3'
   ld: drivers/gpu/drm/i915/display/intel_pll_algorithm.o:intel_pll_algorithm.c:(.text+0x235): more undefined references to `__divdi3' follow
   ld: drivers/gpu/drm/i915/display/intel_pll_algorithm.o: in function `get_ana_cp_int_prop.constprop.0':
>> intel_pll_algorithm.c:(.text+0x282): undefined reference to `__udivdi3'
>> ld: intel_pll_algorithm.c:(.text+0x2f2): undefined reference to `__udivdi3'
   ld: intel_pll_algorithm.c:(.text+0x31b): undefined reference to `__udivdi3'
   ld: intel_pll_algorithm.c:(.text+0x33e): undefined reference to `__udivdi3'
   ld: intel_pll_algorithm.c:(.text+0x36c): undefined reference to `__udivdi3'
   ld: drivers/gpu/drm/i915/display/intel_pll_algorithm.o:intel_pll_algorithm.c:(.text+0x384): more undefined references to `__udivdi3' follow
   ld: drivers/gpu/drm/i915/display/intel_pll_algorithm.o: in function `intel_snps_phy_compute_hdmi_tmds_pll':
>> intel_pll_algorithm.c:(.text+0x602): undefined reference to `__udivmoddi4'
   ld: intel_pll_algorithm.c:(.text+0x61e): undefined reference to `__udivdi3'
kernel test robot June 26, 2024, 6:46 p.m. UTC | #3 
Hi Ankit,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes drm-tip/drm-tip drm-xe/drm-xe-next linus/master v6.10-rc5 next-20240625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ankit-Nautiyal/drm-i915-display-Add-support-for-SNPS-PHY-HDMI-PLL-algorithm-for-DG2/20240626-131209
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20240626050056.3996349-2-ankit.k.nautiyal%40intel.com
patch subject: [PATCH 1/5] drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2
config: i386-randconfig-011-20240626 (https://download.01.org/0day-ci/archive/20240627/202406270253.GoXTAfRN-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270253.GoXTAfRN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406270253.GoXTAfRN-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_pll_algorithm.c:153:14: error: variable 'a' is used uninitialized whenever 'for' loop exits because its condition is false [-Werror,-Wsometimes-uninitialized]
     153 |         for (j = 0; j < 8; j += 2) {
         |                     ^~~~~
   drivers/gpu/drm/i915/display/intel_pll_algorithm.c:161:67: note: uninitialized use occurs here
     161 |         get_ana_cp_int_prop(vco_clk, refclk_postscalar, mpll_ana_v2i, c, a,
         |                                                                          ^
   drivers/gpu/drm/i915/display/intel_pll_algorithm.c:153:14: note: remove the condition if it is always true
     153 |         for (j = 0; j < 8; j += 2) {
         |                     ^~~~~
   drivers/gpu/drm/i915/display/intel_pll_algorithm.c:124:10: note: initialize the variable 'a' to silence this warning
     124 |         int c, a, j;
         |                 ^
         |                  = 0
>> drivers/gpu/drm/i915/display/intel_pll_algorithm.c:153:14: error: variable 'ana_freq_vco' is used uninitialized whenever 'for' loop exits because its condition is false [-Werror,-Wsometimes-uninitialized]
     153 |         for (j = 0; j < 8; j += 2) {
         |                     ^~~~~
   drivers/gpu/drm/i915/display/intel_pll_algorithm.c:179:29: note: uninitialized use occurs here
     179 |         pll_params->ana_freq_vco = ana_freq_vco;
         |                                    ^~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_pll_algorithm.c:153:14: note: remove the condition if it is always true
     153 |         for (j = 0; j < 8; j += 2) {
         |                     ^~~~~
   drivers/gpu/drm/i915/display/intel_pll_algorithm.c:123:18: note: initialize the variable 'ana_freq_vco' to silence this warning
     123 |         int ana_freq_vco;
         |                         ^
         |                          = 0
   2 errors generated.


vim +153 drivers/gpu/drm/i915/display/intel_pll_algorithm.c

    92	
    93	static int _intel_phy_compute_hdmi_tmds_pll(u64 pixel_clock, u32 refclk,
    94						    u32 ref_range,
    95						    u32 ana_cp_int_gs,
    96						    u32 ana_cp_prop_gs,
    97						    const u64 curve_freq_hz[2][8],
    98						    const u64 curve_0[2][8],
    99						    const u64 curve_1[2][8],
   100						    const u64 curve_2[2][8],
   101						    u32 prescaler_divider,
   102						    struct pll_output_params *pll_params)
   103	{
   104		/*datarate 10khz */
   105		u64 datarate = pixel_clock * 10000;
   106		u32 ssc_up_spread = 1;
   107		u32 mpll_div5_en = 1;
   108		u32 hdmi_div = 1;
   109		u32 ana_cp_int;
   110		u32 ana_cp_prop;
   111		u32 refclk_postscalar = refclk >> prescaler_divider;
   112		u32 tx_clk_div;
   113		u64 vco_clk;
   114		u32 vco_div_refclk_integer;
   115		u32 vco_div_refclk_fracn;
   116		u32 fracn_quot;
   117		u32 fracn_rem;
   118		u32 fracn_den;
   119		u32 fracn_en;
   120		u32 pmix_en;
   121		u32 multiplier;
   122		int mpll_ana_v2i;
   123		int ana_freq_vco;
   124		int c, a, j;
   125	
   126		if (pixel_clock < 25175 || pixel_clock > 600000)
   127			return -EINVAL;
   128	
   129		/* Select appropriate v2i point */
   130		if (datarate <= INTEL_SNPS_PHY_HDMI_9999MHZ) {
   131			mpll_ana_v2i = 2;
   132			tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_9999MHZ / datarate);
   133		} else {
   134			mpll_ana_v2i = 3;
   135			tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_16GHZ / datarate);
   136		}
   137		vco_clk = (datarate << tx_clk_div) >> 1;
   138	
   139		/* Highly accurate division, calculate fraction to 32 bits of precision */
   140		vco_div_refclk_integer = vco_clk / refclk_postscalar;
   141		vco_div_refclk_fracn = ((vco_clk % refclk_postscalar) << 32) / refclk_postscalar;
   142		fracn_quot = vco_div_refclk_fracn >> 16;
   143		fracn_rem = vco_div_refclk_fracn & 0xffff;
   144		fracn_rem = fracn_rem - (fracn_rem >> 15);
   145		fracn_den = 0xffff;
   146		fracn_en = (fracn_quot != 0 || fracn_rem != 0) ? 1 : 0;
   147		pmix_en = fracn_en;
   148		multiplier = (vco_div_refclk_integer - 16) * 2;
   149		/* Curve selection for ana_cp_* calculations. One curve hardcoded per v2i range */
   150		c = mpll_ana_v2i - 2;
   151	
   152		/* Find the right segment of the table */
 > 153		for (j = 0; j < 8; j += 2) {
   154			if (vco_clk <= curve_freq_hz[c][j + 1]) {
   155				a = j;
   156				ana_freq_vco = 3 - (a >> 1);
   157				break;
   158			}
   159		}
   160	
   161		get_ana_cp_int_prop(vco_clk, refclk_postscalar, mpll_ana_v2i, c, a,
   162				    curve_freq_hz, curve_0, curve_1, curve_2,
   163				    &ana_cp_int, &ana_cp_prop);
   164	
   165		pll_params->ssc_up_spread = ssc_up_spread;
   166		pll_params->mpll_div5_en = mpll_div5_en;
   167		pll_params->hdmi_div = hdmi_div;
   168		pll_params->ana_cp_int = ana_cp_int;
   169		pll_params->refclk_postscalar = refclk_postscalar;
   170		pll_params->tx_clk_div = tx_clk_div;
   171		pll_params->fracn_quot = fracn_quot;
   172		pll_params->fracn_rem = fracn_rem;
   173		pll_params->fracn_den = fracn_den;
   174		pll_params->fracn_en = fracn_en;
   175		pll_params->pmix_en = pmix_en;
   176		pll_params->multiplier = multiplier;
   177		pll_params->ana_cp_prop = ana_cp_prop;
   178		pll_params->mpll_ana_v2i = mpll_ana_v2i;
   179		pll_params->ana_freq_vco = ana_freq_vco;
   180	
   181		return 0;
   182	}
   183
Nautiyal, Ankit K June 27, 2024, 4:41 p.m. UTC | #4 
On 6/26/2024 3:34 PM, Jani Nikula wrote:
> On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Add helper _intel_phy_compute_hdmi_tmds_pll to calculate the necessary
>> parameters for configuring the HDMI PLL for SNPS MPLLB and C10 PHY.
>>
>> The pll parameters are computed for desired pixel clock, curve data
>> and other inputs used for interpolation and finally stored in the
>> pll_state. Bspec:54032
>>
>> Currently the helper is used to compute PLLs for DG2 SNPS PHY.
>> Support for computing Plls for C10 PHY is added in subsequent patches.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Okay, usually the high level review should come first, but there are so
> many annoying details here that they distract review. See inline.

Thanks Jani for the review and comments. This is indeed need more polishing.

I was more focused on getting the precision right to avoid floating 
point math, and overlooked some of the avoidable mistakes.

Also some of the names of variables were retained from the original 
algorithm to trace them back for review.

Will address the review comments and send new version. Please find my 
responses inline.

>
>> ---
>>   drivers/gpu/drm/i915/Makefile                 |   1 +
>>   .../drm/i915/display/intel_pll_algorithm.c    | 252 ++++++++++++++++++
>>   .../drm/i915/display/intel_pll_algorithm.h    |  39 +++
>>   3 files changed, 292 insertions(+)
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_pll_algorithm.c
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_pll_algorithm.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 4022e4499382..f1a293110bc6 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -334,6 +334,7 @@ i915-y += \
>>   	display/intel_lspcon.o \
>>   	display/intel_lvds.o \
>>   	display/intel_panel.o \
>> +	display/intel_pll_algorithm.o \
> We probably need to figure out a new name for this. That's too generic
> for what it is.
Hmm. Whether intel_snps_hdmi_pll.c works? I am open for any name that is 
more relevant.
>
>>   	display/intel_pps.o \
>>   	display/intel_qp_tables.o \
>>   	display/intel_sdvo.o \
>> diff --git a/drivers/gpu/drm/i915/display/intel_pll_algorithm.c b/drivers/gpu/drm/i915/display/intel_pll_algorithm.c
>> new file mode 100644
>> index 000000000000..d935715bd3ab
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_pll_algorithm.c
>> @@ -0,0 +1,252 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Synopsys, Inc., Intel Corporation
>> + */
>> +
>> +#include <linux/math.h>
> Blank line here.
>
>> +#include "i915_reg.h"
>> +#include "intel_ddi.h"
>> +#include "intel_ddi_buf_trans.h"
>> +#include "intel_de.h"
>> +#include "intel_display_types.h"
>> +#include "intel_snps_phy.h"
>> +#include "intel_snps_phy_regs.h"
>> +#include "intel_pll_algorithm.h"
> Do you really need all of the includes here?

I'll recheck this.


>
>> +
>> +#define INTEL_SNPS_PHY_HDMI_4999MHZ 4999999900ull
>> +#define INTEL_SNPS_PHY_HDMI_16GHZ 16000000000ull
>> +#define INTEL_SNPS_PHY_HDMI_9999MHZ (2 * INTEL_SNPS_PHY_HDMI_4999MHZ)
>> +
>> +#define CURVE0_MULTIPLIER 1000000000
>> +#define CURVE1_MULTIPLIER 100
>> +#define CURVE2_MULTIPLIER 1000000000000
>> +
>> +static int64_t interp(s64 x, s64 x1, s64 x2, s64 y1, s64 y2)
> Please use kernel types instead of C99 types.

Will fix this to s64.


>
>> +{
>> +	s64 dydx;
>> +
>> +	dydx = DIV_ROUND_UP((y2 - y1) * 100000, (x2 - x1));
>> +
>> +	return (y1 + DIV_ROUND_UP(dydx * (x - x1), 100000));
>> +}
>> +
>> +static void get_ana_cp_int_prop(u32 vco_clk,
>> +				u32 refclk_postscalar,
>> +				int mpll_ana_v2i,
>> +				int c, int a,
>> +				const u64 curve_freq_hz[2][8],
>> +				const u64 curve_0[2][8],
>> +				const u64 curve_1[2][8],
>> +				const u64 curve_2[2][8],
>> +				u32 *ana_cp_int,
>> +				u32 *ana_cp_prop)
>> +{
>> +	u64 vco_div_refclk_float;
>> +	u64 o_397ced90;
>> +	u64 o_20c634d6;
>> +	u64 o_20c634d4;
>> +	u64 o_72019306;
>> +	u64 o_6593e82b;
>> +	u64 o_5cefc329;
>> +	u64 o_49960328;
>> +	u64 o_544adb37;
>> +	u64 o_4ef74e66;
> That's pretty obfuscated. Result of some automated conversion? Can we
> have some more sensible names?

I just used the same name as it appear in the algorithm to make sure I 
am not missing anything.

I will rename these intermediate things to more sensible names.


>
>> +	u32 ana_cp_int_temp;
>> +	u64 temp1, temp2, temp3, temp4;
>> +
>> +	vco_div_refclk_float = vco_clk * (1000000000000 / refclk_postscalar);
>> +	o_397ced90 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
>> +			    curve_0[c][a], curve_0[c][a + 1]);
>> +
>> +	o_20c634d6 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
>> +			    curve_2[c][a], curve_2[c][a + 1]);
>> +
>> +	o_20c634d4 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
>> +			    curve_1[c][a], curve_1[c][a + 1]);
>> +
>> +	o_20c634d4 /= CURVE1_MULTIPLIER;
>> +
>> +	temp1 = o_20c634d6 * (4 - mpll_ana_v2i);
>> +
>> +	o_72019306 = temp1 / 16000;
>> +	o_6593e82b = temp1 / 160;
>> +
>> +	temp2 = ((112008301 * (vco_div_refclk_float / 100000)));
>> +	o_5cefc329 = CURVE2_MULTIPLIER * (temp2 / (o_397ced90 * (o_20c634d4 / CURVE0_MULTIPLIER)));
>> +
>> +	ana_cp_int_temp = min(DIV_ROUND_CLOSEST(o_5cefc329 / o_72019306, CURVE2_MULTIPLIER), 127);
>> +	ana_cp_int_temp = max(1, ana_cp_int_temp);
>> +
>> +	*ana_cp_int = ana_cp_int_temp;
>> +
>> +	o_49960328 = o_72019306 * ana_cp_int_temp;
>> +
>> +	temp3 = o_20c634d4 * (o_49960328 * o_397ced90 / CURVE0_MULTIPLIER);
>> +	o_544adb37 = int_sqrt(DIV_ROUND_UP(temp3, vco_div_refclk_float) * (1000000000000 / 55));
>> +
>> +	temp4 = DIV_ROUND_UP(vco_div_refclk_float, 1000000);
>> +	o_4ef74e66 = (1460281 * DIV_ROUND_UP(o_544adb37 * temp4, o_20c634d4));
>> +
>> +	*ana_cp_prop = max(1, min(DIV_ROUND_UP(o_4ef74e66, o_6593e82b), 127));
>> +}
>> +
>> +static int _intel_phy_compute_hdmi_tmds_pll(u64 pixel_clock, u32 refclk,
> What's with the underscore prefix? It's not a convention we have.

Will do away with the underscore.


>
>> +					    u32 ref_range,
>> +					    u32 ana_cp_int_gs,
>> +					    u32 ana_cp_prop_gs,
>> +					    const u64 curve_freq_hz[2][8],
>> +					    const u64 curve_0[2][8],
>> +					    const u64 curve_1[2][8],
>> +					    const u64 curve_2[2][8],
>> +					    u32 prescaler_divider,
>> +					    struct pll_output_params *pll_params)
>> +{
>> +	/*datarate 10khz */
> Please put space around /* and */.
>
>> +	u64 datarate = pixel_clock * 10000;
>> +	u32 ssc_up_spread = 1;
>> +	u32 mpll_div5_en = 1;
>> +	u32 hdmi_div = 1;
>> +	u32 ana_cp_int;
>> +	u32 ana_cp_prop;
>> +	u32 refclk_postscalar = refclk >> prescaler_divider;
>> +	u32 tx_clk_div;
>> +	u64 vco_clk;
>> +	u32 vco_div_refclk_integer;
>> +	u32 vco_div_refclk_fracn;
>> +	u32 fracn_quot;
>> +	u32 fracn_rem;
>> +	u32 fracn_den;
>> +	u32 fracn_en;
>> +	u32 pmix_en;
>> +	u32 multiplier;
>> +	int mpll_ana_v2i;
>> +	int ana_freq_vco;
>> +	int c, a, j;
>> +
>> +	if (pixel_clock < 25175 || pixel_clock > 600000)
>> +		return -EINVAL;
>> +
>> +	/* Select appropriate v2i point */
>> +	if (datarate <= INTEL_SNPS_PHY_HDMI_9999MHZ) {
>> +		mpll_ana_v2i = 2;
>> +		tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_9999MHZ / datarate);
>> +	} else {
>> +		mpll_ana_v2i = 3;
>> +		tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_16GHZ / datarate);
>> +	}
>> +	vco_clk = (datarate << tx_clk_div) >> 1;
>> +
>> +	/* Highly accurate division, calculate fraction to 32 bits of precision */
>> +	vco_div_refclk_integer = vco_clk / refclk_postscalar;
>> +	vco_div_refclk_fracn = ((vco_clk % refclk_postscalar) << 32) / refclk_postscalar;
>> +	fracn_quot = vco_div_refclk_fracn >> 16;
>> +	fracn_rem = vco_div_refclk_fracn & 0xffff;
>> +	fracn_rem = fracn_rem - (fracn_rem >> 15);
>> +	fracn_den = 0xffff;
>> +	fracn_en = (fracn_quot != 0 || fracn_rem != 0) ? 1 : 0;
>> +	pmix_en = fracn_en;
>> +	multiplier = (vco_div_refclk_integer - 16) * 2;
>> +	/* Curve selection for ana_cp_* calculations. One curve hardcoded per v2i range */
>> +	c = mpll_ana_v2i - 2;
>> +
>> +	/* Find the right segment of the table */
>> +	for (j = 0; j < 8; j += 2) {
> i is the usual loop variable.

Agreed.


>
>> +		if (vco_clk <= curve_freq_hz[c][j + 1]) {
>> +			a = j;
>> +			ana_freq_vco = 3 - (a >> 1);
>> +			break;
>> +		}
>> +	}
>> +
>> +	get_ana_cp_int_prop(vco_clk, refclk_postscalar, mpll_ana_v2i, c, a,
>> +			    curve_freq_hz, curve_0, curve_1, curve_2,
>> +			    &ana_cp_int, &ana_cp_prop);
>> +
>> +	pll_params->ssc_up_spread = ssc_up_spread;
>> +	pll_params->mpll_div5_en = mpll_div5_en;
>> +	pll_params->hdmi_div = hdmi_div;
>> +	pll_params->ana_cp_int = ana_cp_int;
>> +	pll_params->refclk_postscalar = refclk_postscalar;
>> +	pll_params->tx_clk_div = tx_clk_div;
>> +	pll_params->fracn_quot = fracn_quot;
>> +	pll_params->fracn_rem = fracn_rem;
>> +	pll_params->fracn_den = fracn_den;
>> +	pll_params->fracn_en = fracn_en;
>> +	pll_params->pmix_en = pmix_en;
>> +	pll_params->multiplier = multiplier;
>> +	pll_params->ana_cp_prop = ana_cp_prop;
>> +	pll_params->mpll_ana_v2i = mpll_ana_v2i;
>> +	pll_params->ana_freq_vco = ana_freq_vco;
>> +
>> +	return 0;
>> +}
>> +
>> +int intel_snps_phy_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_mpllb_state *pll_state)
>> +{
>> +	struct pll_output_params pll_params;
>> +	u32 refclk = 100000000;
>> +	u32 prescaler_divider = 1;
>> +	u32 ref_range = 3;
>> +	u32 ana_cp_int_gs = 64;
>> +	u32 ana_cp_prop_gs = 124;
>> +	int ret;
>> +	/* x axis frequencies. One curve in each array per v2i point */
>> +	const u64 dg2_curve_freq_hz[2][8] = {
>> +		{2500000000, 3000000000, 3000000000, 3500000000, 3500000000, 4000000000, 4000000000, 5000000000},
>> +		{4000000000, 4600000000, 4601000000, 5400000000, 5401000000, 6600000000, 6601000000, 8001000000}};
>> +
>> +	/* y axis heights multiplied with 1000000000 */
>> +	const u64 dg2_curve_0[2][8] = {
>> +		{34149871, 39803269, 36034544, 40601014, 35646940, 40016109, 35127987, 41889522},
>> +		{70000000, 78770454, 70451838, 80427119, 70991400, 84230173, 72945921, 87064218}};
>> +
>> +	/* Multiplied with 100 */
>> +	const u64 dg2_curve_1[2][8] = {
>> +		{85177000000000, 79385227160000, 95672603580000, 88857207160000, 109379790900000, 103528193900000, 131941242400000, 117279000000000},
>> +		{60255000000000, 55569000000000, 72036000000000, 69509000000000,  81785000000000, 731030000000000, 96591000000000, 69077000000000}};
>> +
>> +	/* Multiplied with 1000000000000 */
>> +	const u64 dg2_curve_2[2][8] = {
>> +		{2186930000, 2835287134, 2395395343, 2932270687, 2351887545, 2861031697, 2294149152, 3091730000},
>> +		{4560000000, 5570000000, 4610000000, 5770000000, 4670000000, 6240000000, 4890000000, 6600000000}
>> +	};
> These can be static const instead of on the stack. Please put spaces
> between { } and the numbers. Please put the final }; on a line of its
> own. Put these as first variables in the function.
>
> Just eyeballing, the constants look big enough to warrant ULL.

Alright, will do the suggested changes.


>
>> +
>> +	ret = _intel_phy_compute_hdmi_tmds_pll(pixel_clock, refclk, ref_range,
>> +					       ana_cp_int_gs, ana_cp_prop_gs,
>> +					       dg2_curve_freq_hz, dg2_curve_0,
>> +					       dg2_curve_1, dg2_curve_2, prescaler_divider,
>> +					       &pll_params);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	pll_state->clock = pixel_clock;
>> +	pll_state->ref_control =
>> +		REG_FIELD_PREP(SNPS_PHY_REF_CONTROL_REF_RANGE, ref_range);
>> +	pll_state->mpllb_cp =
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_INT, pll_params.ana_cp_int) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_PROP, pll_params.ana_cp_prop) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_INT_GS, ana_cp_int_gs) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_PROP_GS, ana_cp_prop_gs);
>> +	pll_state->mpllb_div =
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_DIV5_CLK_EN, pll_params.mpll_div5_en) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_TX_CLK_DIV, pll_params.tx_clk_div) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_PMIX_EN, pll_params.pmix_en) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_V2I, pll_params.mpll_ana_v2i) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FREQ_VCO, pll_params.ana_freq_vco);
>> +	pll_state->mpllb_div2 =
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_REF_CLK_DIV, prescaler_divider) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_MULTIPLIER, pll_params.multiplier) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_HDMI_DIV, pll_params.hdmi_div);
>> +	pll_state->mpllb_fracn1 =
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_CGG_UPDATE_EN, 1) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_EN, pll_params.fracn_en) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_DEN, pll_params.fracn_den);
>> +	pll_state->mpllb_fracn2 =
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_QUOT, pll_params.fracn_quot) |
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_REM, pll_params.fracn_rem);
>> +	pll_state->mpllb_sscen =
>> +		REG_FIELD_PREP(SNPS_PHY_MPLLB_SSC_UP_SPREAD, pll_params.ssc_up_spread);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_pll_algorithm.h b/drivers/gpu/drm/i915/display/intel_pll_algorithm.h
>> new file mode 100644
>> index 000000000000..83a620fb3db2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_pll_algorithm.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Synopsys, Inc., Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_PLL_ALGORITHM_H__
>> +#define __INTEL_PLL_ALGORITHM_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct drm_i915_private;
>> +struct intel_atomic_state;
>> +struct intel_crtc;
>> +struct intel_crtc_state;
>> +struct intel_encoder;
>> +struct intel_mpllb_state;
>> +enum phy;
> What's the deal with the forward declarations? You need exactly one of
> them in this file.

I'll re-check this.


>
>> +
>> +struct pll_output_params {
>> +	u32 ssc_up_spread;
>> +	u32 mpll_div5_en;
>> +	u32 hdmi_div;
>> +	u32 ana_cp_int;
>> +	u32 ana_cp_prop;
>> +	u32 refclk_postscalar;
>> +	u32 tx_clk_div;
>> +	u32 fracn_quot;
>> +	u32 fracn_rem;
>> +	u32 fracn_den;
>> +	u32 fracn_en;
>> +	u32 pmix_en;
>> +	u32 multiplier;
>> +	int mpll_ana_v2i;
>> +	int ana_freq_vco;
>> +};
> This is internal to the implementation and should be in the .c file.
Noted, will move to the .c file as suggested.
>
>> +
>> +int intel_snps_phy_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_mpllb_state *pll_state);
> Usually context params go first.

Alright will move pll_state first.


Thanks & Regards,

Ankit


>
>> +
>> +#endif /* __INTEL_PLL_ALGORITHM_H__ */
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 4022e4499382..f1a293110bc6 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -334,6 +334,7 @@  i915-y += \
 	display/intel_lspcon.o \
 	display/intel_lvds.o \
 	display/intel_panel.o \
+	display/intel_pll_algorithm.o \
 	display/intel_pps.o \
 	display/intel_qp_tables.o \
 	display/intel_sdvo.o \
diff --git a/drivers/gpu/drm/i915/display/intel_pll_algorithm.c b/drivers/gpu/drm/i915/display/intel_pll_algorithm.c
new file mode 100644
index 000000000000..d935715bd3ab
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_pll_algorithm.c
@@ -0,0 +1,252 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Synopsys, Inc., Intel Corporation
+ */
+
+#include <linux/math.h>
+#include "i915_reg.h"
+#include "intel_ddi.h"
+#include "intel_ddi_buf_trans.h"
+#include "intel_de.h"
+#include "intel_display_types.h"
+#include "intel_snps_phy.h"
+#include "intel_snps_phy_regs.h"
+#include "intel_pll_algorithm.h"
+
+#define INTEL_SNPS_PHY_HDMI_4999MHZ 4999999900ull
+#define INTEL_SNPS_PHY_HDMI_16GHZ 16000000000ull
+#define INTEL_SNPS_PHY_HDMI_9999MHZ (2 * INTEL_SNPS_PHY_HDMI_4999MHZ)
+
+#define CURVE0_MULTIPLIER 1000000000
+#define CURVE1_MULTIPLIER 100
+#define CURVE2_MULTIPLIER 1000000000000
+
+static int64_t interp(s64 x, s64 x1, s64 x2, s64 y1, s64 y2)
+{
+	s64 dydx;
+
+	dydx = DIV_ROUND_UP((y2 - y1) * 100000, (x2 - x1));
+
+	return (y1 + DIV_ROUND_UP(dydx * (x - x1), 100000));
+}
+
+static void get_ana_cp_int_prop(u32 vco_clk,
+				u32 refclk_postscalar,
+				int mpll_ana_v2i,
+				int c, int a,
+				const u64 curve_freq_hz[2][8],
+				const u64 curve_0[2][8],
+				const u64 curve_1[2][8],
+				const u64 curve_2[2][8],
+				u32 *ana_cp_int,
+				u32 *ana_cp_prop)
+{
+	u64 vco_div_refclk_float;
+	u64 o_397ced90;
+	u64 o_20c634d6;
+	u64 o_20c634d4;
+	u64 o_72019306;
+	u64 o_6593e82b;
+	u64 o_5cefc329;
+	u64 o_49960328;
+	u64 o_544adb37;
+	u64 o_4ef74e66;
+	u32 ana_cp_int_temp;
+	u64 temp1, temp2, temp3, temp4;
+
+	vco_div_refclk_float = vco_clk * (1000000000000 / refclk_postscalar);
+	o_397ced90 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
+			    curve_0[c][a], curve_0[c][a + 1]);
+
+	o_20c634d6 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
+			    curve_2[c][a], curve_2[c][a + 1]);
+
+	o_20c634d4 = interp(vco_clk, curve_freq_hz[c][a], curve_freq_hz[c][a + 1],
+			    curve_1[c][a], curve_1[c][a + 1]);
+
+	o_20c634d4 /= CURVE1_MULTIPLIER;
+
+	temp1 = o_20c634d6 * (4 - mpll_ana_v2i);
+
+	o_72019306 = temp1 / 16000;
+	o_6593e82b = temp1 / 160;
+
+	temp2 = ((112008301 * (vco_div_refclk_float / 100000)));
+	o_5cefc329 = CURVE2_MULTIPLIER * (temp2 / (o_397ced90 * (o_20c634d4 / CURVE0_MULTIPLIER)));
+
+	ana_cp_int_temp = min(DIV_ROUND_CLOSEST(o_5cefc329 / o_72019306, CURVE2_MULTIPLIER), 127);
+	ana_cp_int_temp = max(1, ana_cp_int_temp);
+
+	*ana_cp_int = ana_cp_int_temp;
+
+	o_49960328 = o_72019306 * ana_cp_int_temp;
+
+	temp3 = o_20c634d4 * (o_49960328 * o_397ced90 / CURVE0_MULTIPLIER);
+	o_544adb37 = int_sqrt(DIV_ROUND_UP(temp3, vco_div_refclk_float) * (1000000000000 / 55));
+
+	temp4 = DIV_ROUND_UP(vco_div_refclk_float, 1000000);
+	o_4ef74e66 = (1460281 * DIV_ROUND_UP(o_544adb37 * temp4, o_20c634d4));
+
+	*ana_cp_prop = max(1, min(DIV_ROUND_UP(o_4ef74e66, o_6593e82b), 127));
+}
+
+static int _intel_phy_compute_hdmi_tmds_pll(u64 pixel_clock, u32 refclk,
+					    u32 ref_range,
+					    u32 ana_cp_int_gs,
+					    u32 ana_cp_prop_gs,
+					    const u64 curve_freq_hz[2][8],
+					    const u64 curve_0[2][8],
+					    const u64 curve_1[2][8],
+					    const u64 curve_2[2][8],
+					    u32 prescaler_divider,
+					    struct pll_output_params *pll_params)
+{
+	/*datarate 10khz */
+	u64 datarate = pixel_clock * 10000;
+	u32 ssc_up_spread = 1;
+	u32 mpll_div5_en = 1;
+	u32 hdmi_div = 1;
+	u32 ana_cp_int;
+	u32 ana_cp_prop;
+	u32 refclk_postscalar = refclk >> prescaler_divider;
+	u32 tx_clk_div;
+	u64 vco_clk;
+	u32 vco_div_refclk_integer;
+	u32 vco_div_refclk_fracn;
+	u32 fracn_quot;
+	u32 fracn_rem;
+	u32 fracn_den;
+	u32 fracn_en;
+	u32 pmix_en;
+	u32 multiplier;
+	int mpll_ana_v2i;
+	int ana_freq_vco;
+	int c, a, j;
+
+	if (pixel_clock < 25175 || pixel_clock > 600000)
+		return -EINVAL;
+
+	/* Select appropriate v2i point */
+	if (datarate <= INTEL_SNPS_PHY_HDMI_9999MHZ) {
+		mpll_ana_v2i = 2;
+		tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_9999MHZ / datarate);
+	} else {
+		mpll_ana_v2i = 3;
+		tx_clk_div = ilog2(INTEL_SNPS_PHY_HDMI_16GHZ / datarate);
+	}
+	vco_clk = (datarate << tx_clk_div) >> 1;
+
+	/* Highly accurate division, calculate fraction to 32 bits of precision */
+	vco_div_refclk_integer = vco_clk / refclk_postscalar;
+	vco_div_refclk_fracn = ((vco_clk % refclk_postscalar) << 32) / refclk_postscalar;
+	fracn_quot = vco_div_refclk_fracn >> 16;
+	fracn_rem = vco_div_refclk_fracn & 0xffff;
+	fracn_rem = fracn_rem - (fracn_rem >> 15);
+	fracn_den = 0xffff;
+	fracn_en = (fracn_quot != 0 || fracn_rem != 0) ? 1 : 0;
+	pmix_en = fracn_en;
+	multiplier = (vco_div_refclk_integer - 16) * 2;
+	/* Curve selection for ana_cp_* calculations. One curve hardcoded per v2i range */
+	c = mpll_ana_v2i - 2;
+
+	/* Find the right segment of the table */
+	for (j = 0; j < 8; j += 2) {
+		if (vco_clk <= curve_freq_hz[c][j + 1]) {
+			a = j;
+			ana_freq_vco = 3 - (a >> 1);
+			break;
+		}
+	}
+
+	get_ana_cp_int_prop(vco_clk, refclk_postscalar, mpll_ana_v2i, c, a,
+			    curve_freq_hz, curve_0, curve_1, curve_2,
+			    &ana_cp_int, &ana_cp_prop);
+
+	pll_params->ssc_up_spread = ssc_up_spread;
+	pll_params->mpll_div5_en = mpll_div5_en;
+	pll_params->hdmi_div = hdmi_div;
+	pll_params->ana_cp_int = ana_cp_int;
+	pll_params->refclk_postscalar = refclk_postscalar;
+	pll_params->tx_clk_div = tx_clk_div;
+	pll_params->fracn_quot = fracn_quot;
+	pll_params->fracn_rem = fracn_rem;
+	pll_params->fracn_den = fracn_den;
+	pll_params->fracn_en = fracn_en;
+	pll_params->pmix_en = pmix_en;
+	pll_params->multiplier = multiplier;
+	pll_params->ana_cp_prop = ana_cp_prop;
+	pll_params->mpll_ana_v2i = mpll_ana_v2i;
+	pll_params->ana_freq_vco = ana_freq_vco;
+
+	return 0;
+}
+
+int intel_snps_phy_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_mpllb_state *pll_state)
+{
+	struct pll_output_params pll_params;
+	u32 refclk = 100000000;
+	u32 prescaler_divider = 1;
+	u32 ref_range = 3;
+	u32 ana_cp_int_gs = 64;
+	u32 ana_cp_prop_gs = 124;
+	int ret;
+	/* x axis frequencies. One curve in each array per v2i point */
+	const u64 dg2_curve_freq_hz[2][8] = {
+		{2500000000, 3000000000, 3000000000, 3500000000, 3500000000, 4000000000, 4000000000, 5000000000},
+		{4000000000, 4600000000, 4601000000, 5400000000, 5401000000, 6600000000, 6601000000, 8001000000}};
+
+	/* y axis heights multiplied with 1000000000 */
+	const u64 dg2_curve_0[2][8] = {
+		{34149871, 39803269, 36034544, 40601014, 35646940, 40016109, 35127987, 41889522},
+		{70000000, 78770454, 70451838, 80427119, 70991400, 84230173, 72945921, 87064218}};
+
+	/* Multiplied with 100 */
+	const u64 dg2_curve_1[2][8] = {
+		{85177000000000, 79385227160000, 95672603580000, 88857207160000, 109379790900000, 103528193900000, 131941242400000, 117279000000000},
+		{60255000000000, 55569000000000, 72036000000000, 69509000000000,  81785000000000, 731030000000000, 96591000000000, 69077000000000}};
+
+	/* Multiplied with 1000000000000 */
+	const u64 dg2_curve_2[2][8] = {
+		{2186930000, 2835287134, 2395395343, 2932270687, 2351887545, 2861031697, 2294149152, 3091730000},
+		{4560000000, 5570000000, 4610000000, 5770000000, 4670000000, 6240000000, 4890000000, 6600000000}
+	};
+
+	ret = _intel_phy_compute_hdmi_tmds_pll(pixel_clock, refclk, ref_range,
+					       ana_cp_int_gs, ana_cp_prop_gs,
+					       dg2_curve_freq_hz, dg2_curve_0,
+					       dg2_curve_1, dg2_curve_2, prescaler_divider,
+					       &pll_params);
+
+	if (ret)
+		return ret;
+
+	pll_state->clock = pixel_clock;
+	pll_state->ref_control =
+		REG_FIELD_PREP(SNPS_PHY_REF_CONTROL_REF_RANGE, ref_range);
+	pll_state->mpllb_cp =
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_INT, pll_params.ana_cp_int) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_PROP, pll_params.ana_cp_prop) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_INT_GS, ana_cp_int_gs) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_CP_PROP_GS, ana_cp_prop_gs);
+	pll_state->mpllb_div =
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_DIV5_CLK_EN, pll_params.mpll_div5_en) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_TX_CLK_DIV, pll_params.tx_clk_div) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_PMIX_EN, pll_params.pmix_en) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_V2I, pll_params.mpll_ana_v2i) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_FREQ_VCO, pll_params.ana_freq_vco);
+	pll_state->mpllb_div2 =
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_REF_CLK_DIV, prescaler_divider) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_MULTIPLIER, pll_params.multiplier) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_HDMI_DIV, pll_params.hdmi_div);
+	pll_state->mpllb_fracn1 =
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_CGG_UPDATE_EN, 1) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_EN, pll_params.fracn_en) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_DEN, pll_params.fracn_den);
+	pll_state->mpllb_fracn2 =
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_QUOT, pll_params.fracn_quot) |
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_FRACN_REM, pll_params.fracn_rem);
+	pll_state->mpllb_sscen =
+		REG_FIELD_PREP(SNPS_PHY_MPLLB_SSC_UP_SPREAD, pll_params.ssc_up_spread);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_pll_algorithm.h b/drivers/gpu/drm/i915/display/intel_pll_algorithm.h
new file mode 100644
index 000000000000..83a620fb3db2
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_pll_algorithm.h
@@ -0,0 +1,39 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Synopsys, Inc., Intel Corporation
+ */
+
+#ifndef __INTEL_PLL_ALGORITHM_H__
+#define __INTEL_PLL_ALGORITHM_H__
+
+#include <linux/types.h>
+
+struct drm_i915_private;
+struct intel_atomic_state;
+struct intel_crtc;
+struct intel_crtc_state;
+struct intel_encoder;
+struct intel_mpllb_state;
+enum phy;
+
+struct pll_output_params {
+	u32 ssc_up_spread;
+	u32 mpll_div5_en;
+	u32 hdmi_div;
+	u32 ana_cp_int;
+	u32 ana_cp_prop;
+	u32 refclk_postscalar;
+	u32 tx_clk_div;
+	u32 fracn_quot;
+	u32 fracn_rem;
+	u32 fracn_den;
+	u32 fracn_en;
+	u32 pmix_en;
+	u32 multiplier;
+	int mpll_ana_v2i;
+	int ana_freq_vco;
+};
+
+int intel_snps_phy_compute_hdmi_tmds_pll(u64 pixel_clock, struct intel_mpllb_state *pll_state);
+
+#endif /* __INTEL_PLL_ALGORITHM_H__ */