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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx