Hi Morimoto-san,
On Thursday, 14 December 2017 04:10:27 EET Kuninori Morimoto wrote:
> Hi Laurent
>
> Thank you for your feedback
>
> >> + * NOTES
> >> + * N = (n + 1), M = (m + 1), P = 2
> >> + * 2000 < fvco < 4096Mhz
> >
> > Are you sure that the fvco constraint is really 2kHz, and not 2GHz ? 2kHz
> > - 4GHz would be a surprisingly large range.
>
> It is 2kHz. This is came from HW team, and indicated
> on HW design sheet (?)
OK, it's a surprising VCO, no issue with that :-)
> >> + * Basically M=1
> >
> > Why is this ?
>
> This is came from HW team, too.
> They are assuming M=1, basically.
> But yes confusable, let's remove it from comment.
> m is started from 0 (= M=1), no need to explain.
Sounds good to me.
> >> + for (m = 0; m < 4; m++) {
> >> + for (n = 119; n > 38; n--) {
> >> + unsigned long long fvco = input * 2 * (n + 1) / (m + 1);
> >
> > This code runs for Gen3 only, so unsigned long would be enough. The rest
> > of the function already relies on native support for 64-bit calculation.
> > If you wanted to run this on a 32-bit CPU, you would likely need to
> > do_div() for the division, and convert input to u64 to avoid integer
> > overflows, otherwise the calculation will be performed on 32-bit before a
> > final conversion to 64-bit.
>
> (snip)
>
> >> + if ((fvco < 2000) ||
> >> + (fvco > 4096000000ll))
> >
> > No need for the inner parentheses, and you can write both conditions on a
> > single line. Furthemore 4096 MHz will fit in a 32-bit number, so there's
> > no need for the ll.
>
> Yes, but compiled by 32bit too, right ?
> Without this "ll", 32bit compiler say
>
> warning: this decimal constant is unsigned only in ISO C90
That's right. How about 4096000000UL then, to force unsigned integer types ?
Or possibly even better, 4096 * 1000 * 1000UL to make it more readable ?
> # anyway, I will add this assumption (= used only by 64bit CPU)
> # on comment to avoid future confusion
>
> > I think you can then drop the output >= 4000000000 check inside the inner
> > fdpll loop, as the output frequency can't be higher than 4GHz if the VCO
> > frequency isn't.
>
> I think code has
>
> if (output >= 400000000)
>
> This is 400MHz, not 4GHz
You're right, my bad. Maybe I should write it 400 * 1000 * 1000 :-)
> >> for (fdpll = 1; fdpll < 32; fdpll++) {
> >> unsigned long output;
> >
> > The output frequency on the line below can be calculated with
> >
> > output = fvco / 2 / (fdpll + 1)
> >
> > to avoid the multiplication by (n + 1) and division by (m + 1).
>
> It is nice idea to avoid extra calculation.
> I will use this idea, and add extrate comment to avoid confusion
Thank you.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
