Re: [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2013/7/18 Ben Widawsky <ben@xxxxxxxxxxxx>:
> On Thu, Jul 18, 2013 at 04:26:42PM -0700, Ben Widawsky wrote:
>> On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> >
>> > For now there are no callers, but these functions are going to be
>> > needed for the code that allows Package C8+. Other future features may
>> > also require this code.
>> >
>>
>> The thing that's missing from the patches is any sort of assertions
>> about things being on before the disable sequence. Is this something we
>> don't need to address?
>>
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++
>> >  drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>> >  3 files changed, 105 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index be6164f..8e5a5ec 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -4930,7 +4930,14 @@
>> >  #define  LCPLL_CLK_FREQ_450                (0<<26)
>> >  #define  LCPLL_CD_CLOCK_DISABLE            (1<<25)
>> >  #define  LCPLL_CD2X_CLOCK_DISABLE  (1<<23)
>> > +#define  LCPLL_POWER_DOWN_ALLOW            (1<<22)
>> >  #define  LCPLL_CD_SOURCE_FCLK              (1<<21)
>> > +#define  LCPLL_CD_SOURCE_FCLK_DONE (1<<19)
>>
>> Hmm... the doc I am looking at says
>
> Oops. The doc I was looking at had some different names for things, was
> what I wanted to say.

I looked at the LCPLL_CTL register definition. Bit 22 is called
"Display Power Down Allow" and value 1 means "Allow". Bit 19 is called
"CD Source Fclk" and value 1 means "Done".

>
>>
>> > +
>> > +#define D_COMP                             (MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
>> > +#define  D_COMP_RCOMP_IN_PROGRESS  (1<<9)
>> > +#define  D_COMP_COMP_FORCE         (1<<8)
>> > +#define  D_COMP_COMP_DISABLE               (1<<0)
>> >
>> >  /* Pipe WM_LINETIME - watermark line time */
>> >  #define PIPE_WM_LINETIME_A         0x45270
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 059c9a8..ffb08bf 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>> >     return true;
>> >  }
>> >
>> > +/*
>> > + * This function implements pieces of two sequences from BSpec:
>> > + * - Sequence for display software to disable LCPLL
>> > + * - Sequence for display software to allow package C8+
>> > + * The steps implemented here are just the steps that actually touch the LCPLL
>> > + * register. Callers should take care of disabling all the display engine
>> > + * functions, doing the mode unset, fixing interrupts, etc.
>> > + */
>> > +void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>> > +                  bool switch_to_fclk, bool allow_power_down)
>> > +{
>> > +   uint32_t val;
>> > +
>> > +   val = I915_READ(LCPLL_CTL);
>> > +
>> > +   if (switch_to_fclk) {
>> > +           val |= LCPLL_CD_SOURCE_FCLK;
>> > +           I915_WRITE(LCPLL_CTL, val);
>> > +           POSTING_READ(LCPLL_CTL);
>> > +
>> > +           udelay(1);
>> > +
>> > +           val = I915_READ(LCPLL_CTL);
>> > +           if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
>> > +                   DRM_ERROR("Switching to FCLK failed\n");
>>
>> wait_for_us(..., 1)?
>>
>> > +   }
>> > +
>> > +   val |= LCPLL_PLL_DISABLE;
>> > +   I915_WRITE(LCPLL_CTL, val);
>> > +   POSTING_READ(LCPLL_CTL);
>> > +
>> > +   if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
>> > +           DRM_ERROR("LCPLL still locked\n");
>> > +
>> > +   val = I915_READ(D_COMP);
>> > +   val |= D_COMP_COMP_DISABLE;
>> > +   I915_WRITE(D_COMP, val);
>> > +   POSTING_READ(D_COMP);
>> > +
>> > +   udelay(2);
>>
>> ndelay(100)?
>>
>> > +
>> > +   val = I915_READ(D_COMP);
>> > +   if (val & D_COMP_RCOMP_IN_PROGRESS)
>> > +           DRM_ERROR("D_COMP RCOMP still in progress\n");
>>
>> wait_for(..., 1)?
>>
>> > +
>> > +   if (allow_power_down) {
>> > +           val = I915_READ(LCPLL_CTL);
>> > +           val |= LCPLL_POWER_DOWN_ALLOW;
>> > +           I915_WRITE(LCPLL_CTL, val);
>> > +           POSTING_READ(LCPLL_CTL);
>> > +   }
>> > +}
>> > +
>> > +/*
>> > + * Fully restores LCPLL, disallowing power down and switching back to LCPLL
>> > + * source.
>> > + */
>> > +void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>> > +{
>> > +   uint32_t val;
>> > +
>> > +   val = I915_READ(LCPLL_CTL);
>> > +
>>
>> I think we could potentially exit early here if the PLL is already
>> locked, and we're on CDclk. And indeed, I've already seen this case
>> occur, but I'm not sure I will ever see that case again.
>>
>> > +   if (val & LCPLL_POWER_DOWN_ALLOW) {
>> > +           val &= ~LCPLL_POWER_DOWN_ALLOW;
>> > +           I915_WRITE(LCPLL_CTL, val);
>> > +   }
>> > +
>> > +   val = I915_READ(D_COMP);
>> > +   val |= D_COMP_COMP_FORCE;
>> > +   val &= ~D_COMP_COMP_DISABLE;
>> > +   I915_WRITE(D_COMP, val);
>> > +
>>
>> I think you need a posting read here. I am not sure we're allowed to
>> read LCPLL_CTL until we know the write has landed.
>>
>>
>> > +   val = I915_READ(LCPLL_CTL);
>> > +   val &= ~LCPLL_PLL_DISABLE;
>> > +   I915_WRITE(LCPLL_CTL, val);
>> > +   POSTING_READ(LCPLL_CTL);
>>         ^ unnecessary POSTING_READ - but meh
>> > +
>> > +   if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
>> > +           DRM_ERROR("LCPLL not locked yet\n");
>> > +
>> > +   if (val & LCPLL_CD_SOURCE_FCLK) {
>> > +           val = I915_READ(LCPLL_CTL);
>> > +           val &= ~LCPLL_CD_SOURCE_FCLK;
>> > +           I915_WRITE(LCPLL_CTL, val);
>> > +           POSTING_READ(LCPLL_CTL);
>> > +
>> > +           udelay(1);
>> > +
>> > +           val = I915_READ(LCPLL_CTL);
>> > +           if (val & LCPLL_CD_SOURCE_FCLK_DONE)
>> > +                   DRM_ERROR("Switching back to LCPLL failed\n");
>> > +   }
>> > +}
>> > +
>> >  static void haswell_modeset_global_resources(struct drm_device *dev)
>> >  {
>> >     bool enable = false;
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 5dfc1a0..15989d1 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -832,5 +832,8 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>> >  extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>> >                                              enum transcoder pch_transcoder,
>> >                                              bool enable);
>> > +extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>> > +                         bool switch_to_fclk, bool allow_power_down);
>> > +extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
>> >
>> >  #endif /* __INTEL_DRV_H__ */
>>
>> I'm a bit torn as to whether or not it makes sense to extract the pure
>> LCPLL disable from hsw_disable_lcpll. Did you think about this, could
>> you explain the reason you decided against it? (I'm a bit partial since
>> that was the way I had written it).
>>
>> Does it every make sense to switch to fclk and not allow_power_down?
>>
>> --
>> Ben Widawsky, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux