Re: [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp

[Paulo Zanoni <przanoni@gmail.com>]

2013/7/18 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Jul 12, 2013 at 02:19:39PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Now it implements 3 different sequences from BSpec and also has
>> support for ULT.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++-----------
>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index dc3d6a7..be6164f 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4863,6 +4863,8 @@
>>  #define   SBI_SSCAUXDIV_FINALDIV2SEL(x)              ((x)<<4)
>>  #define  SBI_DBUFF0                          0x2a00
>>  #define   SBI_DBUFF0_ENABLE                  (1<<0)
>> +#define  SBI_GEN0                            0x1f00
>> +#define   SBI_GEN0_ENABLE                    (1<<0)
>
> bikeshed: I wouldn't haved bothered defining SBI_GEN0_ENABLE

Our docs have changed considerably since I originally wrote this
patch. They have joined both registers and a single definition makes
sense now. I also just noticed the docs say 0 is Enable and 1 is
Disable, even though the documentation for the Sequences suggests that
1 enables stuff and 0 disables stuff :(


>
>>
>>  /* LPT PIXCLK_GATE */
>>  #define PIXCLK_GATE                  0xC6020
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f4c5263..5f3b636 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5258,12 +5258,20 @@ static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
>>       intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
>>  }
>>
>> -/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
>> -static void lpt_enable_clkout_dp(struct drm_device *dev)
>> +/* Implements 3 different sequences from BSpec chapter "Display iCLK
>> + * Programming" based on the parameters passed:
>> + * - Sequence to enable CLKOUT_DP
>> + * - Sequence to enable CLKOUT_DP without spread
>> + * - Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O
>> + */
>> +static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
>> +                              bool with_fdi)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       uint32_t tmp;
>>
>> +     WARN(with_fdi && !with_spread, "FDI requires downspread\n");
>> +
>
> WARN_ON(with_fdi && IS_ULT(dev))?

Yeah, but I noticed I'm using IS_ULT checks where I should be checking
for LPT-LP.


> Should we proceed after the WARN, or break out early?

Either way we'll end up with possibly broken PCH clocks.... I'll try
to adjust the arguments to something sane and print the warn.


>
>>       mutex_lock(&dev_priv->dpio_lock);
>>
>>       tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
>> @@ -5273,17 +5281,26 @@ static void lpt_enable_clkout_dp(struct drm_device *dev)
>>
>>       udelay(24);
>>
>> -     tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
>> -     tmp &= ~SBI_SSCCTL_PATHALT;
>> -     intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
>> +     if (with_spread) {
>> +             tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
>> +             tmp &= ~SBI_SSCCTL_PATHALT;
>> +             intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
>>
>> -     lpt_reset_fdi_mphy(dev_priv);
>> -     lpt_program_fdi_mphy(dev_priv);
>> +             if (with_fdi) {
>> +                     lpt_reset_fdi_mphy(dev_priv);
>> +                     lpt_program_fdi_mphy(dev_priv);
>> +             }
>> +     }
>>
>> -     /* ULT uses SBI_GEN0, but ULT doesn't have VGA, so we don't care. */
>> -     tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
>> -     tmp |= SBI_DBUFF0_ENABLE;
>> -     intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
>> +     if (IS_ULT(dev)) {
>> +             tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
>> +             tmp |= SBI_GEN0_ENABLE;
>> +             intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
>> +     } else {
>> +             tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
>> +             tmp |= SBI_DBUFF0_ENABLE;
>> +             intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
>> +     }
>>
>>       mutex_unlock(&dev_priv->dpio_lock);
>>  }
>> @@ -5305,7 +5322,7 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
>>       if (!has_vga)
>>               return;
>>
>> -     lpt_enable_clkout_dp(dev);
>> +     lpt_enable_clkout_dp(dev, true, true);
>>  }
>>
>>  /*
>> --
>> 1.8.1.2
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni