Message-ID: <074fc86c-6e6e-4dbc-857a-21e9abf965ac@fris.de>
Date: Wed, 11 Sep 2024 20:24:01 +0200
From: Frieder Schrempf <frieder@...s.de>
To: Dominique Martinet <dominique.martinet@...ark-techno.com>
Cc: Kishon Vijay Abraham I <kishon@...nel.org>, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org, Vinod Koul <vkoul@...nel.org>,
 Adam Ford <aford173@...il.com>, Lucas Stach <l.stach@...gutronix.de>,
 Marco Felsch <m.felsch@...gutronix.de>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH 0/2] Extending PLL LUT for i.MX8MP Samsung HDMI PHY

On 11.09.24 02:23, Dominique Martinet wrote:
> Frieder Schrempf wrote on Tue, Sep 10, 2024 at 08:14:51PM +0200:
>> [2] https://codeberg.org/fschrempf/samsung-hdmi-phy-pll-calculator/src/branch/main/pll.py
> 
> Great work! Thanks!
> 
> I was curious about the existing table entries, recomputing existing
> values doesn't yield the same values. For example, the first entry is
> {
>          .pixclk = 22250000,
>          .pll_div_regs = { 0xd1, 0x4b, 0xf1, 0x89, 0x88, 0x80, 0x40 },
> }
> but computing it yields
> {
>      .pixclk = 22250000,
>      .pll_div_regs = { 0xd1, 0x4a, 0xf0, 0xef, 0x10, 0x81, 0x40 },
> }
> 
> I assume there just are multiple ways to generate the same frequencies,
> which is fine in itself, but it'd be great to be able to "back-compute"
> the entries as a sanity check.
> 
> I've played a bit with your script and spent more time on it than I'd
> like to admit, but something like this seems to do the trick, plugging
> in the regs from the kernel:

Thanks for the feedback and the additional code. Yes, the script yields 
different results for the existing table entries.

For the reverse operation I used the spreadsheet (pll.ods, also in the 
repo). Actually I wrote that before the Python code and used it for 
verification.

> 
> ---
> pll = FractionalNPLL(freq_ref)
> 
> regs = [0xd1, 0x4b, 0xf1, 0x89, 0x88, 0x80, 0x40]
> # assume fractional
> if not regs[0] & 0xD0:
>      print("reg[0] missing 0xD0")
>      sys.exit(1)
> pll.freq_frac = True
> pll.params["p"] = regs[0] & 0x2f
> pll.params["m"] = regs[1]
> pll.params["s"] = (regs[2] >> 4) + 1
> pll.params["n2"] = ((regs[2] >> 3) & 0x1) + 1
> pll.params["n"] = (regs[2] & 0x7) + 4
> pll.params["lc"] = regs[3] & 0x7f
> if regs[4] & 0x80:
>      pll.params["lc"] = - pll.params["lc"]
> pll.params["k"] = regs[4] & 0x7f
> pll.params["lc_s"] = regs[5] & 0x7f
> pll.params["k_s"] = regs[6] & 0xbf
> 
> 
> f_vco = int(pll.params["m"] * pll.f_ref / pll.params["p"])
> if f_vco < 750000000 or f_vco > 3000000000:
>      print(f"f_vco {f_vco} out of range")
>      sys.exit(1)
> f_calc = f_vco / pll.params["s"] / 5
> pll.freq_int = round(f_calc)
> print(f_calc)
> sdc = pll.calc_sdc(pll.params)
> frac = pll.calc_f_frac(sdc, pll.params)
> print(frac)
> freq = pll.freq_int + frac
> print(freq)
> pll.print_reg_driver_data(freq)
> exit(0);
> ---
> yields this back (comments added manually)
> ---
> 22500000.0 (integer part)
> -250000.0 (fractional part)
> 22250000.0 (summed)
> 
> PHY Driver Table Entry:
> {
>      .pixclk = 22250000.0,
>      .pll_div_regs = { 0xd1, 0x4b, 0xf1, 0x89, 0x88, 0x81, 0x40 },
> }
> ---
> 
> so if I find some time I'll whip some loop to check all other values...

If you look at the second sheet in the pll.ods file, you can see that I 
already started to create a table for verifying the existing values. I 
gave up at some point when I saw my calculations were giving reasonable 
results and continued with writing the script. But it's easy to extend 
the first columns with the values from the driver and copy the other 
columns to provide the calculations.

> 
> That aside, I see no problem with this, just one meta-comment about
> adding a link to the script in an external repo: I see some other
> drivers have python scripts in their trees e.g.
> drivers/comedi/drivers/ni_routing/tools/*py
> drivers/gpu/drm/ci/xfails/update-xfails.py
> drivers/gpu/drm/msm/registers/gen_header.py
> 
> would it make sense to commit the script here instead of linking to a
> repo that might be lost in the future?
> 
> I'm not quite sure what policy the linux repo has here, so leaving that
> as an open question.

I was thinking about this as well and I really have no idea what is 
preferred by the maintainers. So I will just wait for some feedback here.

Powered by blists - more mailing lists