Re: [PATCH 66/89 v9] drm/i915/skl: Implementation of SKL DPLL programming

Paulo Zanoni
2014-11-07T19:56:09Z

Hi

With this and the other patches you resent, you can add "Reviewed-by:
Paulo Zanoni <paulo.r.zanoni@intel.com>" to patches 58, 59, 60, 61, 62
and 66. And I guess this makes the "clocks" part of the series
complete and ready to merge.

That said, this patch has some weird comment syntax on the comment
that contains "Intermediate values are in HZ". I guess Daniel can add
the 2 missing white spaces when merging :)

The only 2 requested bikesheds that I really miss would be an inline
function to replace GET_CFG_CRx_REG() (from patch 61) and adding
explicit values to "enum skl_dpll" (requested in my review to patch
62). But if we decide to do this, it's probably better to just do this
in separate patches: let's not block this series anymore due to
bikeshedding :)

Thanks,
Paulo

2014-11-04 14:26 GMT-02:00 Damien Lespiau <damien.lespiau@intel.com>:
> From: Satheeshakrishna M <satheeshakrishna.m@intel.com>
>
> This patch implements SKL DPLL programming that includes:
> - DPLL allocation
> - wide range PLL calculation and programming
> - DP link rate programming
> - DDI to DPLL mapping
>
> v2: Incorporated following changes
> - Added vfunc for function required outside
> - Fixed multiple comments in WRPLL calculation
>
> v3: - Fix the DCO computation
> - Move the initialization up to not clobber the computed values
> - Use the correct macro for DP link rate programming.
> - Use wait_for() to wait for the PLL locked bit
>
> v4: Rebase on top of nigthly (Damien)
>
> v5: A few code cleanups in the WRPLL computation (Damien)
> - Use uint32_t when possible
> - Use abs_diff() in the WRPLL computation
> - Make the 64bits divisions use div64_u64()
> - Fix typo in dco_central_feq_deviation (freq)
> - Replace the chain of breaks with a goto
>
> v6: Port of the patch to work on top of the shared DPLLs (Damien)
> v7: Don't try to handle eDP in ddi_pll_select() (Damien)
> v8: Modified as per review comments from Paulo (Satheesh)
> v9: Rebase on top of Ander's clock computation staging work for atomic (Damien)
>
> Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com> (v3)
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 226 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 225 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index e8cdc1a..9e1f4b2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -954,6 +954,226 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> return true;
> }
>
> +struct skl_wrpll_params {
> + uint32_t dco_fraction;
> + uint32_t dco_integer;
> + uint32_t qdiv_ratio;
> + uint32_t qdiv_mode;
> + uint32_t kdiv;
> + uint32_t pdiv;
> + uint32_t central_freq;
> +};
> +
> +static void
> +skl_ddi_calculate_wrpll(int clock /* in Hz */,
> + struct skl_wrpll_params *wrpll_params)
> +{
> + uint64_t afe_clock = clock * 5; /* AFE Clock is 5x Pixel clock */
> + uint64_t dco_central_freq[3] = {8400000000, 9000000000, 9600000000};
> + uint32_t min_dco_deviation = 400;
> + uint32_t min_dco_index = 3;
> + uint32_t P0[4] = {1, 2, 3, 7};
> + uint32_t P2[4] = {1, 2, 3, 5};
> + bool found = false;
> + uint32_t candidate_p = 0;
> + uint32_t candidate_p0[3] = {0}, candidate_p1[3] = {0};
> + uint32_t candidate_p2[3] = {0};
> + uint32_t dco_central_freq_deviation[3];
> + uint32_t i, P1, k, dco_count;
> + bool retry_with_odd = false;
> + uint64_t dco_freq;
> +
> + /* Determine P0, P1 or P2 */
> + for (dco_count = 0; dco_count < 3; dco_count++) {
> + found = false;
> + candidate_p =
> + div64_u64(dco_central_freq[dco_count], afe_clock);
> + if (retry_with_odd == false)
> + candidate_p = (candidate_p % 2 == 0 ?
> + candidate_p : candidate_p + 1);
> +
> + for (P1 = 1; P1 < candidate_p; P1++) {
> + for (i = 0; i < 4; i++) {
> + if (!(P0[i] != 1 || P1 == 1))
> + continue;
> +
> + for (k = 0; k < 4; k++) {
> + if (P1 != 1 && P2[k] != 2)
> + continue;
> +
> + if (candidate_p == P0[i] * P1 * P2[k]) {
> + /* Found possible P0, P1, P2 */
> + found = true;
> + candidate_p0[dco_count] = P0[i];
> + candidate_p1[dco_count] = P1;
> + candidate_p2[dco_count] = P2[k];
> + goto found;
> + }
> +
> + }
> + }
> + }
> +
> +found:
> + if (found) {
> + dco_central_freq_deviation[dco_count] =
> + div64_u64(10000 *
> + abs_diff((candidate_p * afe_clock),
> + dco_central_freq[dco_count]),
> + dco_central_freq[dco_count]);
> +
> + if (dco_central_freq_deviation[dco_count] <
> + min_dco_deviation) {
> + min_dco_deviation =
> + dco_central_freq_deviation[dco_count];
> + min_dco_index = dco_count;
> + }
> + }
> +
> + if (min_dco_index > 2 && dco_count == 2) {
> + retry_with_odd = true;
> + dco_count = 0;
> + }
> + }
> +
> + if (min_dco_index > 2) {
> + WARN(1, "No valid values found for the given pixel clock\n");
> + } else {
> + wrpll_params->central_freq = dco_central_freq[min_dco_index];
> +
> + switch (dco_central_freq[min_dco_index]) {
> + case 9600000000:
> + wrpll_params->central_freq = 0;
> + break;
> + case 9000000000:
> + wrpll_params->central_freq = 1;
> + break;
> + case 8400000000:
> + wrpll_params->central_freq = 3;
> + }
> +
> + switch (candidate_p0[min_dco_index]) {
> + case 1:
> + wrpll_params->pdiv = 0;
> + break;
> + case 2:
> + wrpll_params->pdiv = 1;
> + break;
> + case 3:
> + wrpll_params->pdiv = 2;
> + break;
> + case 7:
> + wrpll_params->pdiv = 4;
> + break;
> + default:
> + WARN(1, "Incorrect PDiv\n");
> + }
> +
> + switch (candidate_p2[min_dco_index]) {
> + case 5:
> + wrpll_params->kdiv = 0;
> + break;
> + case 2:
> + wrpll_params->kdiv = 1;
> + break;
> + case 3:
> + wrpll_params->kdiv = 2;
> + break;
> + case 1:
> + wrpll_params->kdiv = 3;
> + break;
> + default:
> + WARN(1, "Incorrect KDiv\n");
> + }
> +
> + wrpll_params->qdiv_ratio = candidate_p1[min_dco_index];
> + wrpll_params->qdiv_mode =
> + (wrpll_params->qdiv_ratio == 1) ? 0 : 1;
> +
> + dco_freq = candidate_p0[min_dco_index] *
> + candidate_p1[min_dco_index] *
> + candidate_p2[min_dco_index] * afe_clock;
> +
> + /*
> + * Intermediate values are in Hz.
> + * Divide by MHz to match bsepc
> + */
> + wrpll_params->dco_integer = div_u64(dco_freq, (24 * MHz(1)));
> + wrpll_params->dco_fraction =
> + div_u64(((div_u64(dco_freq, 24) -
> + wrpll_params->dco_integer * MHz(1)) * 0x8000), MHz(1));
> +
> + }
> +}
> +
> +
> +static bool
> +skl_ddi_pll_select(struct intel_crtc *intel_crtc,
> + struct intel_encoder *intel_encoder,
> + int clock)
> +{
> + struct intel_shared_dpll *pll;
> + uint32_t ctrl1, cfgcr1, cfgcr2;
> +
> + /*
> + * See comment in intel_dpll_hw_state to understand why we always use 0
> + * as the DPLL id in this function.
> + */
> +
> + ctrl1 = DPLL_CTRL1_OVERRIDE(0);
> +
> + if (intel_encoder->type == INTEL_OUTPUT_HDMI) {
> + struct skl_wrpll_params wrpll_params = { 0, };
> +
> + ctrl1 |= DPLL_CTRL1_HDMI_MODE(0);
> +
> + skl_ddi_calculate_wrpll(clock * 1000, &wrpll_params);
> +
> + cfgcr1 = DPLL_CFGCR1_FREQ_ENABLE |
> + DPLL_CFGCR1_DCO_FRACTION(wrpll_params.dco_fraction) |
> + wrpll_params.dco_integer;
> +
> + cfgcr2 = DPLL_CFGCR2_QDIV_RATIO(wrpll_params.qdiv_ratio) |
> + DPLL_CFGCR2_QDIV_MODE(wrpll_params.qdiv_mode) |
> + DPLL_CFGCR2_KDIV(wrpll_params.kdiv) |
> + DPLL_CFGCR2_PDIV(wrpll_params.pdiv) |
> + wrpll_params.central_freq;
> + } else if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> + struct drm_encoder *encoder = &intel_encoder->base;
> + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> + switch (intel_dp->link_bw) {
> + case DP_LINK_BW_1_62:
> + ctrl1 |= DPLL_CRTL1_LINK_RATE(DPLL_CRTL1_LINK_RATE_810, 0);
> + break;
> + case DP_LINK_BW_2_7:
> + ctrl1 |= DPLL_CRTL1_LINK_RATE(DPLL_CRTL1_LINK_RATE_1350, 0);
> + break;
> + case DP_LINK_BW_5_4:
> + ctrl1 |= DPLL_CRTL1_LINK_RATE(DPLL_CRTL1_LINK_RATE_2700, 0);
> + break;
> + }
> +
> + cfgcr1 = cfgcr2 = 0;
> + } else /* eDP */
> + return true;
> +
> + intel_crtc->new_config->dpll_hw_state.ctrl1 = ctrl1;
> + intel_crtc->new_config->dpll_hw_state.cfgcr1 = cfgcr1;
> + intel_crtc->new_config->dpll_hw_state.cfgcr2 = cfgcr2;
> +
> + pll = intel_get_shared_dpll(intel_crtc);
> + if (pll == NULL) {
> + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> + pipe_name(intel_crtc->pipe));
> + return false;
> + }
> +
> + /* shared DPLL id 0 is DPLL 1 */
> + intel_crtc->new_config->ddi_pll_sel = pll->id + 1;
> +
> + return true;
> +}
>
> /*
> * Tries to find a *shared* PLL for the CRTC and store it in
> @@ -964,11 +1184,15 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> */
> bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
> {
> + struct drm_device *dev = intel_crtc->base.dev;
> struct intel_encoder *intel_encoder =
> intel_ddi_get_crtc_new_encoder(intel_crtc);
> int clock = intel_crtc->new_config->port_clock;
>
> - return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
> + if (IS_SKYLAKE(dev))
> + return skl_ddi_pll_select(intel_crtc, intel_encoder, clock);
> + else
> + return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
> }
>
> void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
> --
> 1.8.3.1
>

--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org