[Intel-gfx] [PATCH 01/47] drm/i915: rewrite the LCPLL code

[Lespiau, Damien]

On Tue, Oct 2, 2012 at 9:51 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> +       temp = lcpll_val & LCPLL_CLK_FREQ_MASK;
> +       if ((clk_val == 449 && (temp != LCPLL_CLK_FREQ_450)) ||
> +           (clk_val == 539 && (temp != LCPLL_CLK_FREQ_540))) {
> +               DRM_ERROR("LCPLL and CDCLK frequencies don't match\n");
> +               lcpll_needs_change = true;
> +
> +               lcpll_val &= ~LCPLL_CLK_FREQ_MASK;
> +               if (clk_val == 449)
> +                       lcpll_val |= LCPLL_CLK_FREQ_450;
> +               else
> +                       lcpll_val |= LCPLL_CLK_FREQ_540;
> +       }

This fixup code looks weird to me, have you encountered cases where it
makes it work or is it "just in case"? it's "weird" to me for the
following reasons:
  - It tries to adapt the LCPLL freq depending on the value programmed
in CDCLOCK, but CDCLOCK derives from LCPLL, should not fixup code  try
to do the opposite then (if we want to try to do that at all)?
  - the 540MHz clock is only available is FUSE_STRAP bit 24 is not
set, I guess we'd want to test that

> +       if (lcpll_val & LCPLL_PLL_DISABLE) {
> +               DRM_ERROR("LCPLL is disabled\n");
> +               lcpll_needs_change = true;
> +               lcpll_val &= ~LCPLL_PLL_DISABLE;
> +       }
> +
> +       if (lcpll_needs_change)
> +               I915_WRITE(LCPLL_CTL, lcpll_val);

Are we sure that enabling LCPLL has ever done something? if the driver
would have to enable LCPLL, you'd have to ensure that CDCLOCK is
enabled as well for instance. My gut feeling is that this BIOS domain.

I think I'd just try to drop the initial hunk instead of rewriting it,
but what do I know :p

-- 
Damien