drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
This small series depends on Adam's series here [1].
The parameters have been determined using the script here [2] and validated by
trying the modes on a Kontron BL i.MX8MP with a HDMI USB grabber.
[1] https://patchwork.kernel.org/project/linux-phy/cover/20240904233100.114611-1-aford173@gmail.com/
[2] https://codeberg.org/fschrempf/samsung-hdmi-phy-pll-calculator/src/branch/main/pll.py
Frieder Schrempf (2):
phy: freescale: fsl-samsung-hdmi: Add references for calculating LUT
parameters to comment
phy: freescale: fsl-samsung-hdmi: Add PLL LUT entries for some
non-CEA-861 modes
drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
--
2.46.0
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:
---
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...
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.
--
Dominique
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.
Dominique Martinet wrote on Wed, Sep 11, 2024 at 09:23:31AM +0900:
> so if I find some time I'll whip some loop to check all other values...
There were more differences than I thought there'd be (see diff below)
they're all minor enough to probably not care, but your script finds
exact values for them so it might be worth updating the regs if we keep
the table...
(OTOH these seem to work, and "if it works don't touch it" -- I have no
further opinion now I've checked, curiosity is sated)
Might want to just check 154000000 though, reg6's 0x80 was not set so
SDC was disabled? and it'd get the integer fraction (153600000) instead
if I understand this correctly
---------
diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index a5ad51425ee3..72facc7bd045 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -73,7 +73,7 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 24024000,
.pll_div_regs = { 0xd1, 0x50, 0xf1, 0x99, 0x02, 0x80, 0x40 },
}, {
- .pixclk = 25175000,
+ .pixclk = 25177981,
.pll_div_regs = { 0xd1, 0x54, 0xfc, 0xcc, 0x91, 0x80, 0x40 },
}, {
.pixclk = 26750000,
@@ -82,16 +82,16 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 27027000,
.pll_div_regs = { 0xd1, 0x5a, 0xf2, 0xfd, 0x0c, 0x80, 0x40 },
}, {
- .pixclk = 29500000,
+ .pixclk = 29487500,
.pll_div_regs = { 0xd1, 0x62, 0xf4, 0x95, 0x08, 0x80, 0x40 },
}, {
- .pixclk = 30750000,
+ .pixclk = 30729661,
.pll_div_regs = { 0xd1, 0x66, 0xf4, 0x82, 0x01, 0x88, 0x45 },
}, {
- .pixclk = 30888000,
+ .pixclk = 30848949,
.pll_div_regs = { 0xd1, 0x66, 0xf4, 0x99, 0x18, 0x88, 0x45 },
}, {
- .pixclk = 33750000,
+ .pixclk = 33731250,
.pll_div_regs = { 0xd1, 0x70, 0xf4, 0x82, 0x01, 0x80, 0x40 },
}, {
.pixclk = 35000000,
@@ -106,13 +106,13 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 44500000,
.pll_div_regs = { 0xd1, 0x5c, 0x92, 0x98, 0x11, 0x84, 0x41 },
}, {
- .pixclk = 47000000,
+ .pixclk = 47005000,
.pll_div_regs = { 0xd1, 0x62, 0x94, 0x95, 0x82, 0x80, 0x40 },
}, {
- .pixclk = 47500000,
+ .pixclk = 47502000,
.pll_div_regs = { 0xd1, 0x63, 0x96, 0xa1, 0x82, 0x80, 0x40 },
}, {
- .pixclk = 50349650,
+ .pixclk = 50355924,
.pll_div_regs = { 0xd1, 0x54, 0x7c, 0xc3, 0x8f, 0x80, 0x40 },
}, {
.pixclk = 53250000,
@@ -124,16 +124,16 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 54054000,
.pll_div_regs = { 0xd1, 0x5a, 0x72, 0xfd, 0x0c, 0x80, 0x40 },
}, {
- .pixclk = 59000000,
+ .pixclk = 58975000,
.pll_div_regs = { 0xd1, 0x62, 0x74, 0x95, 0x08, 0x80, 0x40 },
}, {
- .pixclk = 59340659,
+ .pixclk = 59264777,
.pll_div_regs = { 0xd1, 0x62, 0x74, 0xdb, 0x52, 0x88, 0x47 },
}, {
- .pixclk = 61500000,
+ .pixclk = 61459322,
.pll_div_regs = { 0xd1, 0x66, 0x74, 0x82, 0x01, 0x88, 0x45 },
}, {
- .pixclk = 63500000,
+ .pixclk = 63437500,
.pll_div_regs = { 0xd1, 0x69, 0x74, 0x89, 0x08, 0x80, 0x40 },
}, {
.pixclk = 67500000,
@@ -145,25 +145,25 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 72072000,
.pll_div_regs = { 0xd1, 0x5a, 0x52, 0xfd, 0x0c, 0x80, 0x40 },
}, {
- .pixclk = 74176000,
+ .pixclk = 74175824,
.pll_div_regs = { 0xd1, 0x5d, 0x58, 0xdb, 0xA2, 0x88, 0x41 },
}, {
.pixclk = 74250000,
.pll_div_regs = { 0xd1, 0x5c, 0x52, 0x90, 0x0d, 0x84, 0x41 },
}, {
- .pixclk = 78500000,
+ .pixclk = 78487500,
.pll_div_regs = { 0xd1, 0x62, 0x54, 0x87, 0x01, 0x80, 0x40 },
}, {
- .pixclk = 82000000,
+ .pixclk = 81945763,
.pll_div_regs = { 0xd1, 0x66, 0x54, 0x82, 0x01, 0x88, 0x45 },
}, {
- .pixclk = 82500000,
+ .pixclk = 82486555,
.pll_div_regs = { 0xd1, 0x67, 0x54, 0x88, 0x01, 0x90, 0x49 },
}, {
- .pixclk = 89000000,
+ .pixclk = 89075000,
.pll_div_regs = { 0xd1, 0x70, 0x54, 0x84, 0x83, 0x80, 0x40 },
}, {
- .pixclk = 90000000,
+ .pixclk = 89950000,
.pll_div_regs = { 0xd1, 0x70, 0x54, 0x82, 0x01, 0x80, 0x40 },
}, {
.pixclk = 94000000,
@@ -178,7 +178,7 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 99000000,
.pll_div_regs = { 0xd1, 0x52, 0x32, 0x82, 0x01, 0x88, 0x47 },
}, {
- .pixclk = 100699300,
+ .pixclk = 100711847,
.pll_div_regs = { 0xd1, 0x54, 0x3c, 0xc3, 0x8f, 0x80, 0x40 },
}, {
.pixclk = 102500000,
@@ -196,16 +196,16 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 108108000,
.pll_div_regs = { 0xd1, 0x5a, 0x32, 0xfd, 0x0c, 0x80, 0x40 },
}, {
- .pixclk = 118000000,
+ .pixclk = 117950000,
.pll_div_regs = { 0xd1, 0x62, 0x34, 0x95, 0x08, 0x80, 0x40 },
}, {
- .pixclk = 123000000,
+ .pixclk = 122918644,
.pll_div_regs = { 0xd1, 0x66, 0x34, 0x82, 0x01, 0x88, 0x45 },
}, {
- .pixclk = 127000000,
+ .pixclk = 126875000,
.pll_div_regs = { 0xd1, 0x69, 0x34, 0x89, 0x08, 0x80, 0x40 },
}, {
- .pixclk = 135000000,
+ .pixclk = 134925000,
.pll_div_regs = { 0xd1, 0x70, 0x34, 0x82, 0x01, 0x80, 0x40 },
}, {
.pixclk = 135580000,
@@ -214,20 +214,20 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 137520000,
.pll_div_regs = { 0xd1, 0x72, 0x38, 0x99, 0x10, 0x85, 0x41 },
}, {
- .pixclk = 138750000,
+ .pixclk = 138658397,
.pll_div_regs = { 0xd1, 0x73, 0x35, 0x88, 0x05, 0x90, 0x4d },
}, {
- .pixclk = 140000000,
+ .pixclk = 140040000,
.pll_div_regs = { 0xd1, 0x75, 0x36, 0xa7, 0x90, 0x80, 0x40 },
}, {
- .pixclk = 148352000,
+ .pixclk = 148265128,
.pll_div_regs = { 0xd1, 0x7b, 0x35, 0xdb, 0x39, 0x90, 0x45 },
}, {
- .pixclk = 148500000,
+ .pixclk = 148396403,
.pll_div_regs = { 0xd1, 0x7b, 0x35, 0x84, 0x03, 0x90, 0x45 },
}, {
.pixclk = 154000000,
- .pll_div_regs = { 0xd1, 0x40, 0x18, 0x83, 0x01, 0x00, 0x40 },
+ .pll_div_regs = { 0xd1, 0x40, 0x18, 0x83, 0x01, 0x80, 0x40 },
}, {
.pixclk = 157000000,
.pll_div_regs = { 0xd1, 0x41, 0x11, 0xa7, 0x14, 0x80, 0x40 },
@@ -268,10 +268,10 @@ static const struct phy_config phy_pll_cfg[] = {
.pixclk = 254000000,
.pll_div_regs = { 0xd1, 0x69, 0x14, 0x89, 0x08, 0x80, 0x40 },
}, {
- .pixclk = 277500000,
+ .pixclk = 277316794,
.pll_div_regs = { 0xd1, 0x73, 0x15, 0x88, 0x05, 0x90, 0x4d },
}, {
- .pixclk = 297000000,
+ .pixclk = 296792806,
.pll_div_regs = { 0xd1, 0x7b, 0x15, 0x84, 0x03, 0x90, 0x45 },
},
};
-------------
--
Dominique
On 11.09.24 08:08, Dominique Martinet wrote:
> Dominique Martinet wrote on Wed, Sep 11, 2024 at 09:23:31AM +0900:
>> so if I find some time I'll whip some loop to check all other values...
>
> There were more differences than I thought there'd be (see diff below)
>
> they're all minor enough to probably not care, but your script finds
> exact values for them so it might be worth updating the regs if we keep
> the table...
> (OTOH these seem to work, and "if it works don't touch it" -- I have no
> further opinion now I've checked, curiosity is sated)
Yeah, that's interesting. A lot of those values seem to be a little off.
Or my calculation has some problem, but it's unlikely as everything else
seems to add up so nicely.
The deviations seem very minor, only in the per mill ballpark or even
lower. I wonder how they got these values initially.
It would still be nice to verify the clocks with some real measurements
on the hardware level before we change the existing values.
On a side note, there is drivers/gpu/drm/exynos/exynos_hdmi.c which is
basically for the same kind of PHY, probably an older version of the IP
core. It also has some LUTs for the PLL registers. But I didn't compare
them so far.
>
> Might want to just check 154000000 though, reg6's 0x80 was not set so
> SDC was disabled? and it'd get the integer fraction (153600000) instead
> if I understand this correctly
I think there are two switches. The fractional-N part can be enabled but
the sigma-delta-part can still be disabled. That's different than having
the fractional-N completely turned off.
My script doesn't cover this case. It assumes the SDC is always enabled
if the fractional-N is enabled.
The problem is that I don't know what part of the calculation is the SDC
value exactly. The reference manual is confusing and probably defective
in some way in regard to this. So I don't really know when to
enable/disable the SDC part.
For the 154 MHz this means even with SDC turned off the fractional-N
part will provide the 400 kHZ to be added to the integer part. At least
that's how I understand it. I might be wrong.
>
> ---------
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index a5ad51425ee3..72facc7bd045 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -73,7 +73,7 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 24024000,
> .pll_div_regs = { 0xd1, 0x50, 0xf1, 0x99, 0x02, 0x80, 0x40 },
> }, {
> - .pixclk = 25175000,
> + .pixclk = 25177981,
> .pll_div_regs = { 0xd1, 0x54, 0xfc, 0xcc, 0x91, 0x80, 0x40 },
> }, {
> .pixclk = 26750000,
> @@ -82,16 +82,16 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 27027000,
> .pll_div_regs = { 0xd1, 0x5a, 0xf2, 0xfd, 0x0c, 0x80, 0x40 },
> }, {
> - .pixclk = 29500000,
> + .pixclk = 29487500,
> .pll_div_regs = { 0xd1, 0x62, 0xf4, 0x95, 0x08, 0x80, 0x40 },
> }, {
> - .pixclk = 30750000,
> + .pixclk = 30729661,
> .pll_div_regs = { 0xd1, 0x66, 0xf4, 0x82, 0x01, 0x88, 0x45 },
> }, {
> - .pixclk = 30888000,
> + .pixclk = 30848949,
> .pll_div_regs = { 0xd1, 0x66, 0xf4, 0x99, 0x18, 0x88, 0x45 },
> }, {
> - .pixclk = 33750000,
> + .pixclk = 33731250,
> .pll_div_regs = { 0xd1, 0x70, 0xf4, 0x82, 0x01, 0x80, 0x40 },
> }, {
> .pixclk = 35000000,
> @@ -106,13 +106,13 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 44500000,
> .pll_div_regs = { 0xd1, 0x5c, 0x92, 0x98, 0x11, 0x84, 0x41 },
> }, {
> - .pixclk = 47000000,
> + .pixclk = 47005000,
> .pll_div_regs = { 0xd1, 0x62, 0x94, 0x95, 0x82, 0x80, 0x40 },
> }, {
> - .pixclk = 47500000,
> + .pixclk = 47502000,
> .pll_div_regs = { 0xd1, 0x63, 0x96, 0xa1, 0x82, 0x80, 0x40 },
> }, {
> - .pixclk = 50349650,
> + .pixclk = 50355924,
> .pll_div_regs = { 0xd1, 0x54, 0x7c, 0xc3, 0x8f, 0x80, 0x40 },
> }, {
> .pixclk = 53250000,
> @@ -124,16 +124,16 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 54054000,
> .pll_div_regs = { 0xd1, 0x5a, 0x72, 0xfd, 0x0c, 0x80, 0x40 },
> }, {
> - .pixclk = 59000000,
> + .pixclk = 58975000,
> .pll_div_regs = { 0xd1, 0x62, 0x74, 0x95, 0x08, 0x80, 0x40 },
> }, {
> - .pixclk = 59340659,
> + .pixclk = 59264777,
> .pll_div_regs = { 0xd1, 0x62, 0x74, 0xdb, 0x52, 0x88, 0x47 },
> }, {
> - .pixclk = 61500000,
> + .pixclk = 61459322,
> .pll_div_regs = { 0xd1, 0x66, 0x74, 0x82, 0x01, 0x88, 0x45 },
> }, {
> - .pixclk = 63500000,
> + .pixclk = 63437500,
> .pll_div_regs = { 0xd1, 0x69, 0x74, 0x89, 0x08, 0x80, 0x40 },
> }, {
> .pixclk = 67500000,
> @@ -145,25 +145,25 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 72072000,
> .pll_div_regs = { 0xd1, 0x5a, 0x52, 0xfd, 0x0c, 0x80, 0x40 },
> }, {
> - .pixclk = 74176000,
> + .pixclk = 74175824,
> .pll_div_regs = { 0xd1, 0x5d, 0x58, 0xdb, 0xA2, 0x88, 0x41 },
> }, {
> .pixclk = 74250000,
> .pll_div_regs = { 0xd1, 0x5c, 0x52, 0x90, 0x0d, 0x84, 0x41 },
> }, {
> - .pixclk = 78500000,
> + .pixclk = 78487500,
> .pll_div_regs = { 0xd1, 0x62, 0x54, 0x87, 0x01, 0x80, 0x40 },
> }, {
> - .pixclk = 82000000,
> + .pixclk = 81945763,
> .pll_div_regs = { 0xd1, 0x66, 0x54, 0x82, 0x01, 0x88, 0x45 },
> }, {
> - .pixclk = 82500000,
> + .pixclk = 82486555,
> .pll_div_regs = { 0xd1, 0x67, 0x54, 0x88, 0x01, 0x90, 0x49 },
> }, {
> - .pixclk = 89000000,
> + .pixclk = 89075000,
> .pll_div_regs = { 0xd1, 0x70, 0x54, 0x84, 0x83, 0x80, 0x40 },
> }, {
> - .pixclk = 90000000,
> + .pixclk = 89950000,
> .pll_div_regs = { 0xd1, 0x70, 0x54, 0x82, 0x01, 0x80, 0x40 },
> }, {
> .pixclk = 94000000,
> @@ -178,7 +178,7 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 99000000,
> .pll_div_regs = { 0xd1, 0x52, 0x32, 0x82, 0x01, 0x88, 0x47 },
> }, {
> - .pixclk = 100699300,
> + .pixclk = 100711847,
> .pll_div_regs = { 0xd1, 0x54, 0x3c, 0xc3, 0x8f, 0x80, 0x40 },
> }, {
> .pixclk = 102500000,
> @@ -196,16 +196,16 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 108108000,
> .pll_div_regs = { 0xd1, 0x5a, 0x32, 0xfd, 0x0c, 0x80, 0x40 },
> }, {
> - .pixclk = 118000000,
> + .pixclk = 117950000,
> .pll_div_regs = { 0xd1, 0x62, 0x34, 0x95, 0x08, 0x80, 0x40 },
> }, {
> - .pixclk = 123000000,
> + .pixclk = 122918644,
> .pll_div_regs = { 0xd1, 0x66, 0x34, 0x82, 0x01, 0x88, 0x45 },
> }, {
> - .pixclk = 127000000,
> + .pixclk = 126875000,
> .pll_div_regs = { 0xd1, 0x69, 0x34, 0x89, 0x08, 0x80, 0x40 },
> }, {
> - .pixclk = 135000000,
> + .pixclk = 134925000,
> .pll_div_regs = { 0xd1, 0x70, 0x34, 0x82, 0x01, 0x80, 0x40 },
> }, {
> .pixclk = 135580000,
> @@ -214,20 +214,20 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 137520000,
> .pll_div_regs = { 0xd1, 0x72, 0x38, 0x99, 0x10, 0x85, 0x41 },
> }, {
> - .pixclk = 138750000,
> + .pixclk = 138658397,
> .pll_div_regs = { 0xd1, 0x73, 0x35, 0x88, 0x05, 0x90, 0x4d },
> }, {
> - .pixclk = 140000000,
> + .pixclk = 140040000,
> .pll_div_regs = { 0xd1, 0x75, 0x36, 0xa7, 0x90, 0x80, 0x40 },
> }, {
> - .pixclk = 148352000,
> + .pixclk = 148265128,
> .pll_div_regs = { 0xd1, 0x7b, 0x35, 0xdb, 0x39, 0x90, 0x45 },
> }, {
> - .pixclk = 148500000,
> + .pixclk = 148396403,
> .pll_div_regs = { 0xd1, 0x7b, 0x35, 0x84, 0x03, 0x90, 0x45 },
> }, {
> .pixclk = 154000000,
> - .pll_div_regs = { 0xd1, 0x40, 0x18, 0x83, 0x01, 0x00, 0x40 },
> + .pll_div_regs = { 0xd1, 0x40, 0x18, 0x83, 0x01, 0x80, 0x40 },
> }, {
> .pixclk = 157000000,
> .pll_div_regs = { 0xd1, 0x41, 0x11, 0xa7, 0x14, 0x80, 0x40 },
> @@ -268,10 +268,10 @@ static const struct phy_config phy_pll_cfg[] = {
> .pixclk = 254000000,
> .pll_div_regs = { 0xd1, 0x69, 0x14, 0x89, 0x08, 0x80, 0x40 },
> }, {
> - .pixclk = 277500000,
> + .pixclk = 277316794,
> .pll_div_regs = { 0xd1, 0x73, 0x15, 0x88, 0x05, 0x90, 0x4d },
> }, {
> - .pixclk = 297000000,
> + .pixclk = 296792806,
> .pll_div_regs = { 0xd1, 0x7b, 0x15, 0x84, 0x03, 0x90, 0x45 },
> },
> };
> -------------
On Tue, Sep 10, 2024 at 7:23 PM Dominique Martinet
<dominique.martinet@atmark-techno.com> 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:
>
> ---
> 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...
>
>
>
>
> 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.
Is there a reason this couldn't be coded in C and used to expand my
integer calculator series? With that, we could drop the lookup table.
adam
> --
> Dominique
Adam Ford wrote on Tue, Sep 10, 2024 at 08:16:04PM -0500: > > 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. > > Is there a reason this couldn't be coded in C and used to expand my > integer calculator series? With that, we could drop the lookup table. Quoting a previous mail from Frieder: > I will clean things up a bit and then share what I have. I hope that this > allows anyone to calculate parameters for their non-standard displays if > required. > > If someone feels extra motivated they could try to calculate the fractional > parameters at runtime. However I'm not sure that this is feasible. The > numerical computation of a large number of parameters is quite heavy and it's > probably not easy to strip the algorithm down to something that can be run on > the target without too much overhead. Trying a random frequency with the algorithm he has implemented it easily takes 10 seconds to run on my imx8mp board, so even if we asssume C is 3-4 times faster I think the current algorithm is too slow for runtime and it makes more sense to extended the LUT to me (as long as the values can be & are checked at least once, which we now can) The current algorithm brute-forces its way through so there could be a better way of computing the fractional part of the divider, but I'm not sure it's worth the effort at this point; I guess it's a good intellectual challenge though so someone might do it in the future. -- Dominique
On 11.09.24 03:27, Dominique Martinet wrote: > Adam Ford wrote on Tue, Sep 10, 2024 at 08:16:04PM -0500: >>> 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. >> >> Is there a reason this couldn't be coded in C and used to expand my >> integer calculator series? With that, we could drop the lookup table. > > Quoting a previous mail from Frieder: >> I will clean things up a bit and then share what I have. I hope that this >> allows anyone to calculate parameters for their non-standard displays if >> required. >> >> If someone feels extra motivated they could try to calculate the fractional >> parameters at runtime. However I'm not sure that this is feasible. The >> numerical computation of a large number of parameters is quite heavy and it's >> probably not easy to strip the algorithm down to something that can be run on >> the target without too much overhead. > > Trying a random frequency with the algorithm he has implemented it > easily takes 10 seconds to run on my imx8mp board, so even if we asssume > C is 3-4 times faster I think the current algorithm is too slow for > runtime and it makes more sense to extended the LUT to me (as long as > the values can be & are checked at least once, which we now can) > > The current algorithm brute-forces its way through so there could be a > better way of computing the fractional part of the divider, but I'm not > sure it's worth the effort at this point; I guess it's a good > intellectual challenge though so someone might do it in the future. Second this. My algorithm is far from optimal and there might be more elegant solutions. If anyone feels like creating something that can be added to the driver that would be interesting and welcome for sure.
© 2016 - 2024 Red Hat, Inc.