Re: [PATCH 05/10] drm/i915: Disable CLKOUT_DP bending on LPT/WPT as needed

[Paulo Zanoni <przanoni@gmail.com>]

2015-12-03 8:03 GMT-02:00 Ville Syrjälä <ville.syrjala@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.syrjala@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@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.syrjala@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