On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> 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@xxxxxxxxx>
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__ */
--
Jani Nikula, Intel
