On Wed, Sep 15, 2021 at 3:39 AM Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> wrote: > On 08.09.21 21:00, Trent Piepho wrote: > > On Sun, Sep 5, 2021 at 6:52 AM Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> wrote: > >> > >> Correct the value in imx8mm_fracpll_tbl to match the one expanded by > >> MHZ(266) macro, rounding it down to MHz range only. > > > > It's not really "correcting" the value, since the value is wrong. > > > > 24 MHz / 9 * 400 / 2^2 = 266666666.6 Hz > > > > Maybe it would be better to say, "make the value in imx8mm_facpll_tbl > > incorrect so it matches another incorrect value used elsewhere." > > > > In which case, one has to wonder why it would not be better to use the > > correct value everywhere? > > Fair point. I just ported the patch and called it a day. > Looking further into it, the rounding is strange as well. > 166.75 MHz is rounded to 167, but 266.6 is rounded to 267. > It would be cleaner to just use MHz values through out and don't use > Hz values at all and always round up. This makes the code diverge > from U-Boot, but I guess that's acceptable. Works for you? I think the original patch was sort of a "not my code base, don't care, minimum effort" kind of thing. I see the issue with diverging from u-boot, but also one hates to be stuck with bad code because of a low standard elsewhere. I looked through this entire module and there is no reason to use Hz, rounded or not, for these values. It is just a symbolic identifier for a supported DRAM speed. It makes more sense in this case to use an enum, start at zero, and have the value of the identifier match the location in the table. Then no more table search is needed. I re-wrote this to do that. I'll send a patch. It's 20% less code. But really it's even less code, since I can't help but to write comments, something the original authors were able to largely avoid. _______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
