Re: [Intel-gfx] [PATCH 1/3] drm/i915: Disable M2 frac division for integer case

[Purushothaman, Vijay A]

On 3/3/2015 9:06 PM, Ville Syrjälä wrote:

On Tue, Mar 03, 2015 at 08:41:54PM +0530, Vijay Purushothaman wrote:

v2 : Handle M2 frac division for both M2 frac and int cases

v3 : Addressed Ville's review comments. Cleared the old bits for RMW

Signed-off-by: Vijay Purushothaman <vijay.a.purushotha...@linux.intel.com>
---
  drivers/gpu/drm/i915/i915_reg.h      |    1 +
  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++------
  2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 55143cb..8200e98 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1029,6 +1029,7 @@ enum skl_disp_power_wells {
  #define  DPIO_CHV_FIRST_MOD           (0 << 8)
  #define  DPIO_CHV_SECOND_MOD          (1 << 8)
  #define  DPIO_CHV_FEEDFWD_GAIN_SHIFT  0
+#define  DPIO_CHV_FEEDFWD_GAIN_MASK            (0xF << 0)
  #define CHV_PLL_DW3(ch) _PIPE(ch, _CHV_PLL_DW3_CH0, _CHV_PLL_DW3_CH1)

#define _CHV_PLL_DW6_CH0 0x8018

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 7298796..15904a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6131,6 +6131,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
        enum dpio_channel port = vlv_pipe_to_channel(pipe);
        u32 loopfilter, intcoeff;
        u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
+       u32 dpio_val;
        int refclk;

bestn = pipe_config->dpll.n;

@@ -6139,6 +6140,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
        bestm2 = pipe_config->dpll.m2 >> 22;
        bestp1 = pipe_config->dpll.p1;
        bestp2 = pipe_config->dpll.p2;
+       dpio_val = 0;

/*

         * Enable Refclk and SSC
@@ -6163,13 +6165,23 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
                        DPIO_CHV_M1_DIV_BY_2 |
                        1 << DPIO_CHV_N_DIV_SHIFT);

- /* M2 fraction division */

-       vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW2(port), bestm2_frac);
+       if (bestm2_frac) {
+               /* M2 fraction division */
+               vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW2(port), bestm2_frac);

- /* M2 fraction division enable */

-       vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port),
-                      DPIO_CHV_FRAC_DIV_EN |
-                      (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
+               /* M2 fraction division enable */
+               dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port));
+               dpio_val |= DPIO_CHV_FRAC_DIV_EN;
+               dpio_val &= ~DPIO_CHV_FEEDFWD_GAIN_MASK;
+               dpio_val |= (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT);
+               vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port), dpio_val);
+
+       } else {
+               /* M2 fraction division disable */
+               dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port));
+               dpio_val &= ~DPIO_CHV_FRAC_DIV_EN;
+               vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port), dpio_val);

You're not writing the feedfwd gain here. The register docs list it as
'Feedforwad gain for fractional mode/SSC mode PLL'. The SSC part there
makes me think these bits might mean something even if the fractional
divider is not used.

At least I don't see any harm in setting it even if the fractional
divider is not used. So with that in mind I'd probably write this as
something like:

val = read(PLL_DW3)
val &= ~(FRAC_DIV_EN | FEEDFWD_GAIN_MASK)
val |= 2 << FEEDFWD_GAIN_SHIFT;
if (bestm2_trac)
        val |= FRAC_DIV_EN;
write(PLL_DW3, val);

This should also make it less likely we will accidentally update
only one of the branches in the future when both need changing.

Makes sense. I will send v4 with this change and add your signed-off-by tag.

Thanks,
Vijay

+       }

/* Loop filter */

        refclk = i9xx_get_refclk(crtc, 0);
--
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx