* Matthijs van Duin <matthijsvanduin@xxxxxxxxx> [160514 21:02]:
> On Fri, May 13, 2016 at 09:40:53AM -0700, Tony Lindgren wrote:
> > +#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.
> > + /* 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!
> > + * 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.
>
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
