Re: [PATCH 5/9] ARM: i.MX8MM: correct unrecognized fracpll frequency

From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Trent Piepho <trent.piepho@igorinstitute.com>,
	Barebox List <barebox@lists.infradead.org>
Subject: Re: [PATCH 5/9] ARM: i.MX8MM: correct unrecognized fracpll frequency
Date: Wed, 15 Sep 2021 12:39:55 +0200	[thread overview]
Message-ID: <2d727028-03bb-9b19-3c1d-1c430c793889@pengutronix.de> (raw)
In-Reply-To: <CAMHeXxOmYuM9+ucqiXEzSSJ7z_0xgRLSE4BYG7GVNCFT0PR3xg@mail.gmail.com>

Hi,

On 08.09.21 21:00, Trent Piepho wrote:
> On Sun, Sep 5, 2021 at 6:52 AM Ahmad Fatoum <a.fatoum@pengutronix.de> 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?

Thanks for the reviews by the way!

Cheers,
Ahmad

> 
>>
>> Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
>> [afa: ported from U-Boot 6ac4d4480663]
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/ddr/imx8m/ddrphy_utils.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ddr/imx8m/ddrphy_utils.c b/drivers/ddr/imx8m/ddrphy_utils.c
>> index 9a4e1a22ee5e..6836e7d4b351 100644
>> --- a/drivers/ddr/imx8m/ddrphy_utils.c
>> +++ b/drivers/ddr/imx8m/ddrphy_utils.c
>> @@ -198,7 +198,7 @@ static struct imx_int_pll_rate_table imx8mm_fracpll_tbl[] = {
>>         { .rate = 600000000U,  .r1 = MDIV(300) | PDIV(3) | SDIV(2), .r2 = 0 },
>>         { .rate = 594000000U,  .r1 = MDIV( 99) | PDIV(1) | SDIV(2), .r2 = 0 },
>>         { .rate = 400000000U,  .r1 = MDIV(300) | PDIV(9) | SDIV(1), .r2 = 0 },
>> -       { .rate = 266666667U,  .r1 = MDIV(400) | PDIV(9) | SDIV(2), .r2 = 0 },
>> +       { .rate = 266000000U,  .r1 = MDIV(400) | PDIV(9) | SDIV(2), .r2 = 0 },
>>         { .rate = 167000000U,  .r1 = MDIV(334) | PDIV(3) | SDIV(4), .r2 = 0 },
>>         { .rate = 100000000U,  .r1 = MDIV(300) | PDIV(9) | SDIV(3), .r2 = 0 },
>>  };
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox