On 6/26/2024 3:34 PM, Jani Nikula wrote:
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.
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.
Hmm. Whether intel_snps_hdmi_pll.c works? I am open for any name that is more relevant.--- 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?
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__ */
