2013/7/18 Ben Widawsky <ben@xxxxxxxxxxxx>:
> 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?
I'll merge this patch with the next one when sending, so we'll have
our assertions.
>
>> 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
>
>> +
>> +#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)?
Done.
>
>> + }
>> +
>> + 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)?
You reviewed v1. In v2 this wait_for was already there. But I re-added
the ndelay(100).
>
>> +
>> + 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.
Makes sense. Done.
>
>> + 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.
Done.
>
>
>> + val = I915_READ(LCPLL_CTL);
>> + val &= ~LCPLL_PLL_DISABLE;
>> + I915_WRITE(LCPLL_CTL, val);
>> + POSTING_READ(LCPLL_CTL);
> ^ unnecessary POSTING_READ - but meh
Yeah, we have the wait_for which does the job. Done.
>> +
>> + 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?
For Package C8 we want to switch to fclk and allow power down, but
BSpec chapter "Display Sequences for LCPLL disabling" defines a
sequence that doesn't switch to fclk nor allow power down. On that
case, the graphics card will be completely disabled. I don't really
know if we'll need it, but at least we have the code for it :)
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
