Re: [PATCH v3 09/10] v4l: Aptina-style sensor PLL support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Andy,

Thanks for the review.

On Saturday 03 March 2012 12:35:09 Andy Walls wrote:
> On Sat, 2012-03-03 at 16:28 +0100, Laurent Pinchart wrote:
> > Add a generic helper function to compute PLL parameters for PLL found in
> > several Aptina sensors.

[snip]

> > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > +			 const struct aptina_pll_limits *limits)
> > +{
> > +	unsigned int mf_min;
> > +	unsigned int mf_max;
> > +	unsigned int p1_min;
> > +	unsigned int p1_max;
> > +	unsigned int p1;
> > +	unsigned int div;
> > +
> > +	if (pll->ext_clock < limits->ext_clock_min ||
> > +	    pll->ext_clock > limits->ext_clock_max) {
> > +		dev_err(dev, "pll: invalid external clock frequency.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (pll->pix_clock > limits->pix_clock_max) {
> > +		dev_err(dev, "pll: invalid pixel clock frequency.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Compute the multiplier M and combined N*P1 divisor. */
> > +	div = gcd(pll->pix_clock, pll->ext_clock);
> > +	pll->m = pll->pix_clock / div;
> > +	div = pll->ext_clock / div;
> > +
> > +	/* We now have the smallest M and N*P1 values that will result in the
> > +	 * desired pixel clock frequency, but they might be out of the valid
> > +	 * range. Compute the factor by which we should multiply them given
> > +	 * the following constraints:
> > +	 *
> > +	 * - minimum/maximum multiplier
> > +	 * - minimum/maximum multiplier output clock frequency assuming the
> > +	 *   minimum/maximum N value
> > +	 * - minimum/maximum combined N*P1 divisor
> > +	 */
> > +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> > +	mf_min = max(mf_min, limits->out_clock_min /
> > +		     (pll->ext_clock / limits->n_min * pll->m));
> > +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> > +	mf_max = limits->m_max / pll->m;
> > +	mf_max = min(mf_max, limits->out_clock_max /
> > +		    (pll->ext_clock / limits->n_max * pll->m));
> > +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, 
div));
> > +
> > +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> > +	if (mf_min > mf_max) {
> > +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * We're looking for the highest acceptable P1 value
> 
> Why the *highest* acceptable post-divide (P1) value?

According to the Aptina datasheets, "it is desirable to keep (fEXTCLK / n) as 
large as possible within the limits".

> > for which a
> > +	 * multiplier factor MF exists that fulfills the following 
conditions:
> > +	 *
> > +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> > +	 *    even
> > +	 * 2. mf is in the [mf_min, mf_max] range computed above
> > +	 * 3. div * mf is a multiple of p1, in order to compute
> > +	 *	n = div * mf / p1
> > +	 *	m = pll->m * mf
> > +	 * 4. the internal clock frequency, given by ext_clock / n, is in the
> > +	 *    [int_clock_min, int_clock_max] range given by the limits
> > +	 * 5. the output clock frequency, given by ext_clock / n * m, is in 
the
> > +	 *    [out_clock_min, out_clock_max] range given by the limits
> > +	 *
> 
> So just to make your constrained optimzation problem even more complex:
> 
> I would imagine you would get faster PLL lock and less phase noise by
> having the VCO operate near its center frequency.
> 
> If you think that is a sensible constraint, then that translates to
> having the PLL output before post-divide (i.e. ext_clock / n * m), to be
> as close as possible to the center frequency of the VCO (i.e.
> (out_clock_max - out_clock_min) / 2 ).

Good point. But... *ouch* :-)

Do you think it's worth it ?

-- 
Regards,

Laurent Pinchart