[Intel-gfx] [PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Paulo Zanoni]

Hi

2012/12/8 Damien Lespiau <damien.lespiau at intel.com>:
> On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> We need this code to init the PCH SSC refclk and the FDI registers.
>> The BIOS does this too and that's why VGA worked before this patch,
>> until you tried to suspend the machine...
>>
>> This patch implements the "Sequence to enable CLKOUT_DP for FDI usage
>> and configure PCH FDI/IO" from our documentation.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
>> +       if (IS_HASWELL(dev) && (dev->pci_device & 0xFF00) == 0x0C00)
>> +               is_sdv = true;
>
> Maybe have a IS_SDV() or IS_HSW_SDV() macro like we have a IS_ULT()
> one? The check should be on the PCH revision but hopefully there's a
> 1:1 correlation?

We should just remove this code in the future, maybe even now... I did
not care too much about creating a macro since this is going to be
killed.

>
>> +               if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
>> +                                      FDI_MPHY_IOSFSB_RESET_STATUS, 100))
>> +                       DRM_ERROR("FDI mPHY resert assert timeout\n");
>
> s/resert/reset/
>
>> +
>> +               tmp = I915_READ(SOUTH_CHICKEN2);
>> +               tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
>> +               I915_WRITE(SOUTH_CHICKEN2, tmp);
>> +
>> +               if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
>> +                                       FDI_MPHY_IOSFSB_RESET_STATUS) == 0,
>> +                                      100))
>> +                       DRM_ERROR("FDI mPHY reset de-assert timeout\n");
>> +       }
>
> When either of those two waits error out, we carry on the sequence. We
> should probably fail and disable the VGA output (one of those error
> paths that never get to be tested, yey! I don't know how well we
> support connectors disappearing)?

We have this "pattern" in many places: the doc tells us "wait for this
bit or wait at least XXXms", then we wait using wait_for and print
DRM_ERROR in case we timeout. In theory, even if we hit the timeout,
we should be fine since we've already waited for the amount of time
the spec tells us to wait... I'm not really sure what's the best thing
to do here, but it really seems we're probably never hit the "fail"
path.

>
>> +       tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
>> +       tmp |= SBI_DBUFF0_ENABLE;
>> +       intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
>
> Th ULT path is missing there.

The ULT will never reach this point, there's not VGA.

>
> I double checked the programming, looks good. I'll follow up with an
> amended patch for the 2 first bikesheds, leave the error path alone,
> and add the ULT path as a separate patch.
>
> --
> Damien



-- 
Paulo Zanoni