On Tue, 21 Jan 2014 13:36:56 +0200
Ville Syrjälä <ville.syrj...@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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to