mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Trent Piepho <trent.piepho@igorinstitute.com>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Barebox List <barebox@lists.infradead.org>
Subject: Re: [PATCH 5/9] ARM: i.MX8MM: correct unrecognized fracpll frequency
Date: Thu, 23 Sep 2021 20:12:13 -0700
Message-ID: <CAMHeXxOvuTP=FzXek0X_YB1u41+VnLh9LZTg=djcQx2fPX91Mg@mail.gmail.com> (raw)
In-Reply-To: <2d727028-03bb-9b19-3c1d-1c430c793889@pengutronix.de>

On Wed, Sep 15, 2021 at 3:39 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> 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?

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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


  reply	other threads:[~2021-09-24  3:14 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-05 13:51 [PATCH 0/9] ARM: i.MX8M: add nano support Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 1/9] ARM: i.MX: add i.MX8MN (Nano) SoC support boilerplate Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 2/9] clk: imx: add i.MX8MN (Nano) support Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 3/9] ARM: i.MX8MN: adapt early clock support Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 4/9] ARM: i.MX: extend drivers for i.MX8MN (Nano) support Ahmad Fatoum
     [not found]   ` <CAMHeXxN_P3pVSAStL2Z=8La_ioGHNu2hw9qQyFYAJ1VNA8hiEQ@mail.gmail.com>
2021-09-08 19:01     ` Trent Piepho
2021-09-15 10:36       ` Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 5/9] ARM: i.MX8MM: correct unrecognized fracpll frequency Ahmad Fatoum
     [not found]   ` <CAMHeXxPV_5mzAH3gbpy4WrL16kSstkLhJvx-VUQqbmy9C1r6WQ@mail.gmail.com>
2021-09-08 19:00     ` Trent Piepho
2021-09-15 10:39       ` Ahmad Fatoum
2021-09-24  3:12         ` Trent Piepho [this message]
2021-09-05 13:51 ` [PATCH 6/9] ddr: imx8m: add i.MX8MN (Nano) support Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 7/9] ddr: imx8m: ddrphy_train: add DDR4 support Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 8/9] scripts: imx: add i.MX8MN support to imx-image Ahmad Fatoum
2021-09-05 13:51 ` [PATCH 9/9] ARM: i.MX8MN: add i.MX8MN-EVK support Ahmad Fatoum

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMHeXxOvuTP=FzXek0X_YB1u41+VnLh9LZTg=djcQx2fPX91Mg@mail.gmail.com' \
    --to=trent.piepho@igorinstitute.com \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

mail archive of the barebox mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lore.barebox.org/barebox/0 barebox/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 barebox barebox/ https://lore.barebox.org/barebox \
		barebox@lists.infradead.org
	public-inbox-index barebox

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git