Re: [PATCH 1/2] drm/i915: clock readout support for DDI v2

Jesse Barnes
2014-01-21T20:38:27Z

On Tue, 21 Jan 2014 13:36:56 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > +static int intel_ddi_calc_wrpll_link(u32 wrpll)
> > +{
> > + int n, p, r;
> > +
> > + r = wrpll & WRPLL_DIVIDER_REF_MASK;
> > + p = (wrpll & WRPLL_DIVIDER_POST_MASK) >> WRPLL_DIVIDER_POST_SHIFT;
> > + n = (wrpll & WRPLL_DIVIDER_FB_MASK) >> WRPLL_DIVIDER_FB_SHIFT;
> > +
> > + return (LC_FREQ * n) / (p * r);
>
> This is assuming the WRPLL will use the LCPLL as reference. Ideally we
> should read out the ref clock settings too.

I don't think I see that in this config, but I've added code to look
for that. Not sure if I got the ref freq right either; I think it's
135MHz in the PCH case...

> > + case PORT_CLK_SEL_SPLL:
> > + link_clock = 135000;
>
> SPLL could also output 810 MHz.

And even 2700 MHz. Fixed.

> > + break;
> > + default:
> > + WARN(1, "bad port clock sel\n");
> > + return;
> > + }
>
> Could do the port_clock = link_clock * 2; here, and then pass port clock
> to intel_dotclock_calculate() and avoid having to multiply crtc_clock by
> 2 afterwards.

Yeah that tidies things up nicely. Fixed.

> As a side note, I must say it's a bit annoying that the DDI PLL code is
> different to the rest of our PLL code. Hz vs. kHz etc. Makes it a bit
> harder to figure out what it's doing.

Yeah that could be converted, mixing them up definitely isn't ideal.

> > +
> > + if (pipe_config->has_pch_encoder)
> > + pipe_config->adjusted_mode.crtc_clock =
> > + intel_dotclock_calculate(link_clock,
> > + &pipe_config->fdi_m_n);
> > + else
>
> else if (has_dp_encoder)
>
> > + pipe_config->adjusted_mode.crtc_clock =
> > + intel_dotclock_calculate(link_clock,
> > + &pipe_config->dp_m_n);
>
> else
> .crtc_clock = link_clock; // or port_clock if you take my suggestion above

Fixed.

> > + intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder,
> > + &pipe_config->dp_m_n);
>
> Not needed. We already do intel_dp_get_m_n() in intel_ddi_get_config().

Ah right, just above. Fixed.

Thanks a lot for the review.

--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org