2015-12-03 8:03 GMT-02:00 Ville Syrjälä <ville.syrj...@linux.intel.com>: > On Wed, Dec 02, 2015 at 11:35:00AM -0200, Paulo Zanoni wrote: >> 2015-12-01 11:08 GMT-02:00 <ville.syrj...@linux.intel.com>: >> > From: Ville Syrjälä <ville.syrj...@linux.intel.com> >> > >> > When we want to use SPLL for FDI we want SSC, which means we have to >> > disable clock bending for the PCH SSC reference (bend and spread are >> > mtutually exclusive). So let's turn off bending when we want spread. > ^^^^^^^^^ > Spotted this typo here just now. Will fix that one too. > >> > In case the BIOS enabled clock bending for some reason we'll just turn >> > it off and enable the spread mode instead. >> > >> > Not sure what happens if the BIOS is actually using the bend source for >> > HDMI at this time, but I suppose it should be no worse than what already >> > happens when we simply turn on the spread. >> > >> > We don't currently use the bend source for anything, and only use the >> > PCH SSC reference for the SPLL to drive FDI (always with spread). >> > >> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_reg.h | 2 ++ >> > drivers/gpu/drm/i915/intel_display.c | 65 >> > ++++++++++++++++++++++++++++++++++-- >> > 2 files changed, 65 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h >> > b/drivers/gpu/drm/i915/i915_reg.h >> > index 6d7ac192982d..fcc819f400a6 100644 >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > @@ -7320,6 +7320,7 @@ enum skl_disp_power_wells { >> > #define SBI_READY (0x0<<0) >> > >> > /* SBI offsets */ >> > +#define SBI_SSCDIVINTPHASE 0x0200 >> > #define SBI_SSCDIVINTPHASE6 0x0600 >> > #define SBI_SSCDIVINTPHASE_DIVSEL_MASK ((0x7f)<<1) >> > #define SBI_SSCDIVINTPHASE_DIVSEL(x) ((x)<<1) >> > @@ -7327,6 +7328,7 @@ enum skl_disp_power_wells { >> > #define SBI_SSCDIVINTPHASE_INCVAL(x) ((x)<<8) >> > #define SBI_SSCDIVINTPHASE_DIR(x) ((x)<<15) >> > #define SBI_SSCDIVINTPHASE_PROPAGATE (1<<0) >> > +#define SBI_SSCDITHPHASE 0x0204 >> > #define SBI_SSCCTL 0x020c >> > #define SBI_SSCCTL6 0x060C >> > #define SBI_SSCCTL_PATHALT (1<<3) >> > diff --git a/drivers/gpu/drm/i915/intel_display.c >> > b/drivers/gpu/drm/i915/intel_display.c >> > index d049b087e8e6..c429029e3bed 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -8551,6 +8551,65 @@ static void lpt_disable_clkout_dp(struct drm_device >> > *dev) >> > mutex_unlock(&dev_priv->sb_lock); >> > } >> > >> > +#define BEND_IDX(steps) ((50 + (steps)) / 5) >> > + >> > +static const uint16_t sscdivintphase[] = { >> > + [BEND_IDX( 50)] = 0x3B23, >> > + [BEND_IDX( 45)] = 0x3B23, >> > + [BEND_IDX( 40)] = 0x3C23, >> > + [BEND_IDX( 35)] = 0x3C23, >> > + [BEND_IDX( 30)] = 0x3D23, >> > + [BEND_IDX( 25)] = 0x3D23, >> > + [BEND_IDX( 20)] = 0x3E23, >> > + [BEND_IDX( 15)] = 0x3E23, >> > + [BEND_IDX( 10)] = 0x3F23, >> > + [BEND_IDX( 5)] = 0x3F23, >> > + [BEND_IDX( 0)] = 0x0025, >> > + [BEND_IDX( -5)] = 0x0025, >> > + [BEND_IDX(-10)] = 0x0125, >> > + [BEND_IDX(-15)] = 0x0125, >> > + [BEND_IDX(-20)] = 0x0225, >> > + [BEND_IDX(-25)] = 0x0225, >> > + [BEND_IDX(-30)] = 0x0325, >> > + [BEND_IDX(-35)] = 0x0325, >> > + [BEND_IDX(-40)] = 0x0425, >> > + [BEND_IDX(-45)] = 0x0425, >> > + [BEND_IDX(-50)] = 0x0525, >> > +}; >> > + >> > +/* >> > + * Bend CLKOUT_DP >> > + * steps -50 to 50 inclusive, in steps of 5 >> > + * < 0 slow down the clock, > 0 speed up the clock, 0 == no bend (135MHz) >> > + * change in clock period = -(steps / 10) * 5.787 ps >> > + */ >> > +static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv, int >> > steps) >> >> As far as I understood from your comments and the table, "int steps" >> should always be a multiple of 5, right? > > Yes, it's in decimal .1 fixed point, but only .0 and .5 values are allowed. > I could have made it binary .1 fixed point, but then comparing with the > spec might have been somewhat less obvious.
I actually liked your approach since it kinda matches the spec, so you can keep it. I was just asking for confirmation due to the "% 5" problem below. > >> >> > +{ >> > + uint32_t tmp; >> > + >> > + int idx = BEND_IDX(steps); >> >> So shouldn't we do the following? >> >> if (WARN_ON(steps % 5 != 0)) >> return; > > Yes, that's a good idea. > >> >> > + >> > + if (WARN_ON(idx >= ARRAY_SIZE(sscdivintphase))) >> > + return; >> > + >> > + mutex_lock(&dev_priv->sb_lock); >> > + >> > + if (steps % 5 != 0) >> >> Shouldn't this be "steps % 10 != 0"? Otherwise, we'll always assign 0x0. > > Doh! Thanks for catching that. Will fix. > >> >> Everything else looks correct. >> >> >> > + tmp = 0xAAAAAAAB; >> > + else >> > + tmp = 0x00000000; >> > + intel_sbi_write(dev_priv, SBI_SSCDITHPHASE, tmp, SBI_ICLK); >> > + >> > + tmp = intel_sbi_read(dev_priv, SBI_SSCDIVINTPHASE, SBI_ICLK); >> > + tmp &= 0xffff0000; >> > + tmp |= sscdivintphase[idx]; >> > + intel_sbi_write(dev_priv, SBI_SSCDIVINTPHASE, tmp, SBI_ICLK); >> > + >> > + mutex_unlock(&dev_priv->sb_lock); >> > +} >> > + >> > +#undef BEND_IDX >> > + >> > static void lpt_init_pch_refclk(struct drm_device *dev) >> > { >> > struct intel_encoder *encoder; >> > @@ -8566,10 +8625,12 @@ static void lpt_init_pch_refclk(struct drm_device >> > *dev) >> > } >> > } >> > >> > - if (has_vga) >> > + if (has_vga) { >> > + lpt_bend_clkout_dp(to_i915(dev), 0); >> > lpt_enable_clkout_dp(dev, true, true); >> > - else >> > + } else { >> > lpt_disable_clkout_dp(dev); >> > + } >> > } >> > >> > /* >> > -- >> > 2.4.10 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx