* Matthijs van Duin [off-list ref] [160514 21:02]:

On Fri, May 13, 2016 at 09:40:53AM -0700, Tony Lindgren wrote:

quoted

+#define TI_ADPLL_DIV_N_MAX		GENMASK(7, 0)
+#define TI_ADPLL_MULT_M_MAX		(GENMASK(11, 0) + 1)

These are PLL-type dependent, also the +1 is wrong since M isn't
off-by-one like N and N2 are.  (consistency? who needs that anyway?)

Here's what my own header says:

			//		PLLS	PLLLJ
/*10*/	u16 prediv;	// aka N	0..127	0..255	(off by one)
/*12*/	u16 postdiv;	// aka M2	1..31	1..127  (but see note below)
/*14*/	u16 mult;	// aka M	2..2047	2..4095
/*16*/	u16 bypassdiv;	// aka N2	0..15	0..15	(off by one)

the "note below" referred to being:

// Using the fractional multiplier increases jitter (presumably more for PLLS
// than for PLLLJ) and imposes constraints on the multiplier:
//	PLLLJ:  mult < 4094
//	PLLS:	mult < 2046 && mult >= 20
// Other docs say mult > 100 is required for PLLS for max 2.5% period jitter.

OK thanks for that info.

quoted

+	/* Ratio for integer multiplier M and pre-divider N */
+	rational_best_approximation(rate, dcorate, TI_ADPLL_MULT_M_MAX,
+				    TI_ADPLL_DIV_N_MAX, &m, &n);

I'm seeing all sorts of problems here...

"dcorate" is a rather misleading name since I would expect that to
refer to the rate of the dco, obviously, while in fact it's the input
clock adjusted to account for an implicit factor of 2 (or 8 if you
enable M4XEN, the utility of which I do not see).

It makes no sense to use rational_best_approximation on the integer
part and then calculate the fractional part separately.  Not only is the
calculation wrong, it's needlessly complicated.  You could just have
passed TI_ADPLL_MULT_M_MAX << 18 to rational_best_approximation and then
split m into integer and fractional part.

Hmm I guess I was trying to only change the integer part separately
if possible but the code is not even doing that.. I guess there's no
advantage to that and we have the the post divider to play with. I'll
update the patch for what you're suggesting.

The biggest problem however is that the best rational approximation does
not guarantee the refclk and dcoclk are within valid range.  This is
unlikely to be a problem for PLLS, but PLLLJ has quite narrow ranges:
0.5-2.5 MHz for refclk and 0.5-2.0 GHz for dcoclk.

Yeah that's a concern :) Some of this can be corrected by manually
changing the multiplier and divider values, maybe not all cases
though.

Seems strange if we already don't have a decent piece of code to use
here.. Anybody got code stashed away for the PLLS and PLLLJ set_rate
handling?

I don't really have much time right now to spend on this, I suggest
checking previous threads on the 814x PLLs since I'm pretty sure the
complications/constraints for setting them have been discussed.

OK sure, thanks a lot for your comments though!

quoted

+ *                                            Maybe we can
+ * make the SD_DIV_TARGET_MHZ configurable also?

What use would it have?  All docs I've ever seen say sddiv must be set
to ceil(dcoclk / 250_MHz) and none of the docs contain any background
information whatsoever on how this thing works, so there's no informed
way to choose an alternate value.

quoted

+	if ((sd >= TI814X_ADPLLJ_MIN_SD_DIV) &&
+	    (sd <= TI814X_ADPLLJ_MAX_SD_DIV)) {

always true due to the limited range permitted for dcoclk.

OK in that case I'll update the comments to reflect that :)

Regards,

Tony