[PATCH V6 0/5] phy: freescale: fsl-samsung-hdmi: Expand phy clock options

[PATCH V6 0/5] phy: freescale: fsl-samsung-hdmi: Expand phy clock options

[Adam Ford]

Currently, there is a look-up-table to describe all the clock options the HDMI PHY
can use.  Some of these entries in the LUT are using a fractional divider which does
not have a well documented algorithm for determinging values, but the the integer
divider can use an algorithm to calculate the integer divder values dynamically
beyond those listed in the LUT and also duplicates some of the entries.

The first two patches do not do anything functionally other than simplify
some of the register accesses and de-duplicates some of the register look-ups.

The third patch adds support for the integer divider and uses it whenever the
clock request is an exact match.  Otherwise, it will use the LUT as before.
The rouding is still based on the LUT if the integer clock isn't an exact match.

The forth patch updates thes set_rate and round_rate functions to use either
the fractional clock LUT or the the integer divder mechanism to determine
which ever clock rate might be closest match.

The last patch removes the integer divider entries from the LUT since by then
it'll be comparing both the integer divider calculator and the closest value
in the LUT.

In my testing with a AOC 4K monitor, I was able to add 4 entries in my modetest
table.  I do not have an HDMI analyzer, so I just used my monitor to determine
if this series worked.

V6: Fix some whitespace
    Fix issues with calculating the closest divider
    
V5: Updates patch 1 of 5 for whitespace and comment updates
      Patches 1 and 2 add RB tags
      No functional changes from V4
      
Adam Ford (5):
  phy: freescale: fsl-samsung-hdmi: Replace register defines with macro
  phy: freescale: fsl-samsung-hdmi: Simplify REG21_PMS_S_MASK lookup
  phy: freescale: fsl-samsung-hdmi: Support dynamic integer
  phy: freescale: fsl-samsung-hdmi: Use closest divider
  phy: freescale: fsl-samsung-hdmi: Remove unnecessary LUT entries

 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 570 ++++++++++---------
 1 file changed, 292 insertions(+), 278 deletions(-)

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V6 1/5] phy: freescale: fsl-samsung-hdmi: Replace register defines with macro

[Adam Ford]

There are 47 registers defined as PHY_REG_xx were xx goes from 00 to
47.  Simplify this by replacing them all with a macro which is passed
the register number to return the proper register offset.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V6:  No Change

V5:  Restore comment about REG33.
     Add spacing between the REG entries to easier readability
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 133 ++++++-------------
 1 file changed, 43 insertions(+), 90 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 9048cdc760c2..acea7008aefc 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -14,76 +14,29 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-#define PHY_REG_00		0x00
-#define PHY_REG_01		0x04
-#define PHY_REG_02		0x08
-#define PHY_REG_08		0x20
-#define PHY_REG_09		0x24
-#define PHY_REG_10		0x28
-#define PHY_REG_11		0x2c
-
-#define PHY_REG_12		0x30
-#define  REG12_CK_DIV_MASK	GENMASK(5, 4)
-
-#define PHY_REG_13		0x34
-#define  REG13_TG_CODE_LOW_MASK	GENMASK(7, 0)
-
-#define PHY_REG_14		0x38
-#define  REG14_TOL_MASK		GENMASK(7, 4)
-#define  REG14_RP_CODE_MASK	GENMASK(3, 1)
-#define  REG14_TG_CODE_HIGH_MASK	GENMASK(0, 0)
-
-#define PHY_REG_15		0x3c
-#define PHY_REG_16		0x40
-#define PHY_REG_17		0x44
-#define PHY_REG_18		0x48
-#define PHY_REG_19		0x4c
-#define PHY_REG_20		0x50
-
-#define PHY_REG_21		0x54
-#define  REG21_SEL_TX_CK_INV	BIT(7)
-#define  REG21_PMS_S_MASK	GENMASK(3, 0)
-
-#define PHY_REG_22		0x58
-#define PHY_REG_23		0x5c
-#define PHY_REG_24		0x60
-#define PHY_REG_25		0x64
-#define PHY_REG_26		0x68
-#define PHY_REG_27		0x6c
-#define PHY_REG_28		0x70
-#define PHY_REG_29		0x74
-#define PHY_REG_30		0x78
-#define PHY_REG_31		0x7c
-#define PHY_REG_32		0x80
+#define PHY_REG(reg)		(reg * 4)
 
+#define REG12_CK_DIV_MASK	GENMASK(5, 4)
+
+#define REG13_TG_CODE_LOW_MASK	GENMASK(7, 0)
+
+#define REG14_TOL_MASK		GENMASK(7, 4)
+#define REG14_RP_CODE_MASK	GENMASK(3, 1)
+#define REG14_TG_CODE_HIGH_MASK	GENMASK(0, 0)
+
+#define REG21_SEL_TX_CK_INV	BIT(7)
+#define REG21_PMS_S_MASK	GENMASK(3, 0)
 /*
  * REG33 does not match the ref manual. According to Sandor Yu from NXP,
  * "There is a doc issue on the i.MX8MP latest RM"
  * REG33 is being used per guidance from Sandor
  */
+#define REG33_MODE_SET_DONE	BIT(7)
+#define REG33_FIX_DA		BIT(1)
 
-#define PHY_REG_33		0x84
-#define  REG33_MODE_SET_DONE	BIT(7)
-#define  REG33_FIX_DA		BIT(1)
-
-#define PHY_REG_34		0x88
-#define  REG34_PHY_READY	BIT(7)
-#define  REG34_PLL_LOCK		BIT(6)
-#define  REG34_PHY_CLK_READY	BIT(5)
-
-#define PHY_REG_35		0x8c
-#define PHY_REG_36		0x90
-#define PHY_REG_37		0x94
-#define PHY_REG_38		0x98
-#define PHY_REG_39		0x9c
-#define PHY_REG_40		0xa0
-#define PHY_REG_41		0xa4
-#define PHY_REG_42		0xa8
-#define PHY_REG_43		0xac
-#define PHY_REG_44		0xb0
-#define PHY_REG_45		0xb4
-#define PHY_REG_46		0xb8
-#define PHY_REG_47		0xbc
+#define REG34_PHY_READY	BIT(7)
+#define REG34_PLL_LOCK		BIT(6)
+#define REG34_PHY_CLK_READY	BIT(5)
 
 #define PHY_PLL_DIV_REGS_NUM 6
 
@@ -369,29 +322,29 @@ struct reg_settings {
 };
 
 static const struct reg_settings common_phy_cfg[] = {
-	{ PHY_REG_00, 0x00 }, { PHY_REG_01, 0xd1 },
-	{ PHY_REG_08, 0x4f }, { PHY_REG_09, 0x30 },
-	{ PHY_REG_10, 0x33 }, { PHY_REG_11, 0x65 },
+	{ PHY_REG(0), 0x00 }, { PHY_REG(1), 0xd1 },
+	{ PHY_REG(8), 0x4f }, { PHY_REG(9), 0x30 },
+	{ PHY_REG(10), 0x33 }, { PHY_REG(11), 0x65 },
 	/* REG12 pixclk specific */
 	/* REG13 pixclk specific */
 	/* REG14 pixclk specific */
-	{ PHY_REG_15, 0x80 }, { PHY_REG_16, 0x6c },
-	{ PHY_REG_17, 0xf2 }, { PHY_REG_18, 0x67 },
-	{ PHY_REG_19, 0x00 }, { PHY_REG_20, 0x10 },
+	{ PHY_REG(15), 0x80 }, { PHY_REG(16), 0x6c },
+	{ PHY_REG(17), 0xf2 }, { PHY_REG(18), 0x67 },
+	{ PHY_REG(19), 0x00 }, { PHY_REG(20), 0x10 },
 	/* REG21 pixclk specific */
-	{ PHY_REG_22, 0x30 }, { PHY_REG_23, 0x32 },
-	{ PHY_REG_24, 0x60 }, { PHY_REG_25, 0x8f },
-	{ PHY_REG_26, 0x00 }, { PHY_REG_27, 0x00 },
-	{ PHY_REG_28, 0x08 }, { PHY_REG_29, 0x00 },
-	{ PHY_REG_30, 0x00 }, { PHY_REG_31, 0x00 },
-	{ PHY_REG_32, 0x00 }, { PHY_REG_33, 0x80 },
-	{ PHY_REG_34, 0x00 }, { PHY_REG_35, 0x00 },
-	{ PHY_REG_36, 0x00 }, { PHY_REG_37, 0x00 },
-	{ PHY_REG_38, 0x00 }, { PHY_REG_39, 0x00 },
-	{ PHY_REG_40, 0x00 }, { PHY_REG_41, 0xe0 },
-	{ PHY_REG_42, 0x83 }, { PHY_REG_43, 0x0f },
-	{ PHY_REG_44, 0x3E }, { PHY_REG_45, 0xf8 },
-	{ PHY_REG_46, 0x00 }, { PHY_REG_47, 0x00 }
+	{ PHY_REG(22), 0x30 }, { PHY_REG(23), 0x32 },
+	{ PHY_REG(24), 0x60 }, { PHY_REG(25), 0x8f },
+	{ PHY_REG(26), 0x00 }, { PHY_REG(27), 0x00 },
+	{ PHY_REG(28), 0x08 }, { PHY_REG(29), 0x00 },
+	{ PHY_REG(30), 0x00 }, { PHY_REG(31), 0x00 },
+	{ PHY_REG(32), 0x00 }, { PHY_REG(33), 0x80 },
+	{ PHY_REG(34), 0x00 }, { PHY_REG(35), 0x00 },
+	{ PHY_REG(36), 0x00 }, { PHY_REG(37), 0x00 },
+	{ PHY_REG(38), 0x00 }, { PHY_REG(39), 0x00 },
+	{ PHY_REG(40), 0x00 }, { PHY_REG(41), 0xe0 },
+	{ PHY_REG(42), 0x83 }, { PHY_REG(43), 0x0f },
+	{ PHY_REG(44), 0x3E }, { PHY_REG(45), 0xf8 },
+	{ PHY_REG(46), 0x00 }, { PHY_REG(47), 0x00 }
 };
 
 struct fsl_samsung_hdmi_phy {
@@ -442,7 +395,7 @@ fsl_samsung_hdmi_phy_configure_pixclk(struct fsl_samsung_hdmi_phy *phy,
 	}
 
 	writeb(REG21_SEL_TX_CK_INV | FIELD_PREP(REG21_PMS_S_MASK, div),
-	       phy->regs + PHY_REG_21);
+	       phy->regs + PHY_REG(21));
 }
 
 static void
@@ -469,7 +422,7 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
 		break;
 	}
 
-	writeb(FIELD_PREP(REG12_CK_DIV_MASK, ilog2(div)), phy->regs + PHY_REG_12);
+	writeb(FIELD_PREP(REG12_CK_DIV_MASK, ilog2(div)), phy->regs + PHY_REG(12));
 
 	/*
 	 * Calculation for the frequency lock detector target code (fld_tg_code)
@@ -489,11 +442,11 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
 
 	/* FLD_TOL and FLD_RP_CODE taken from downstream driver */
 	writeb(FIELD_PREP(REG13_TG_CODE_LOW_MASK, fld_tg_code),
-	       phy->regs + PHY_REG_13);
+	       phy->regs + PHY_REG(13));
 	writeb(FIELD_PREP(REG14_TOL_MASK, 2) |
 	       FIELD_PREP(REG14_RP_CODE_MASK, 2) |
 	       FIELD_PREP(REG14_TG_CODE_HIGH_MASK, fld_tg_code >> 8),
-	       phy->regs + PHY_REG_14);
+	       phy->regs + PHY_REG(14));
 }
 
 static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
@@ -503,7 +456,7 @@ static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
 	u8 val;
 
 	/* HDMI PHY init */
-	writeb(REG33_FIX_DA, phy->regs + PHY_REG_33);
+	writeb(REG33_FIX_DA, phy->regs + PHY_REG(33));
 
 	/* common PHY registers */
 	for (i = 0; i < ARRAY_SIZE(common_phy_cfg); i++)
@@ -511,14 +464,14 @@ static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
 
 	/* set individual PLL registers PHY_REG2 ... PHY_REG7 */
 	for (i = 0; i < PHY_PLL_DIV_REGS_NUM; i++)
-		writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG_02 + i * 4);
+		writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG(2) + i * 4);
 
 	fsl_samsung_hdmi_phy_configure_pixclk(phy, cfg);
 	fsl_samsung_hdmi_phy_configure_pll_lock_det(phy, cfg);
 
-	writeb(REG33_FIX_DA | REG33_MODE_SET_DONE, phy->regs + PHY_REG_33);
+	writeb(REG33_FIX_DA | REG33_MODE_SET_DONE, phy->regs + PHY_REG(33));
 
-	ret = readb_poll_timeout(phy->regs + PHY_REG_34, val,
+	ret = readb_poll_timeout(phy->regs + PHY_REG(34), val,
 				 val & REG34_PLL_LOCK, 50, 20000);
 	if (ret)
 		dev_err(phy->dev, "PLL failed to lock\n");
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V6 2/5] phy: freescale: fsl-samsung-hdmi: Simplify REG21_PMS_S_MASK lookup

[Adam Ford]

The value of 'S' is writen to two places, PHY_REG3[7:4] and
PHY_REG21[3:0].  There is a lookup table which contains
the value of PHY_REG3.  Rather than using a switch statement
based on the pixel clock to search for the value of 'S' again,
just shift the contents of PHY_REG3[7:4] >> 4 and place the value
in PHY_REG21[3:0].  Doing this can eliminate an entire function.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V6:  No Change
V5:  No Change
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 39 ++------------------
 1 file changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index acea7008aefc..4f6874226f9a 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -364,40 +364,6 @@ to_fsl_samsung_hdmi_phy(struct clk_hw *hw)
 	return container_of(hw, struct fsl_samsung_hdmi_phy, hw);
 }
 
-static void
-fsl_samsung_hdmi_phy_configure_pixclk(struct fsl_samsung_hdmi_phy *phy,
-				      const struct phy_config *cfg)
-{
-	u8 div = 0x1;
-
-	switch (cfg->pixclk) {
-	case  22250000 ...  33750000:
-		div = 0xf;
-		break;
-	case  35000000 ...  40000000:
-		div = 0xb;
-		break;
-	case  43200000 ...  47500000:
-		div = 0x9;
-		break;
-	case  50349650 ...  63500000:
-		div = 0x7;
-		break;
-	case  67500000 ...  90000000:
-		div = 0x5;
-		break;
-	case  94000000 ... 148500000:
-		div = 0x3;
-		break;
-	case 154000000 ... 297000000:
-		div = 0x1;
-		break;
-	}
-
-	writeb(REG21_SEL_TX_CK_INV | FIELD_PREP(REG21_PMS_S_MASK, div),
-	       phy->regs + PHY_REG(21));
-}
-
 static void
 fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
 					    const struct phy_config *cfg)
@@ -466,7 +432,10 @@ static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
 	for (i = 0; i < PHY_PLL_DIV_REGS_NUM; i++)
 		writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG(2) + i * 4);
 
-	fsl_samsung_hdmi_phy_configure_pixclk(phy, cfg);
+	/* High nibble of pll_div_regs[1] contains S which also gets written to REG21 */
+	writeb(REG21_SEL_TX_CK_INV | FIELD_PREP(REG21_PMS_S_MASK,
+	       cfg->pll_div_regs[1] >> 4), phy->regs + PHY_REG(21));
+
 	fsl_samsung_hdmi_phy_configure_pll_lock_det(phy, cfg);
 
 	writeb(REG33_FIX_DA | REG33_MODE_SET_DONE, phy->regs + PHY_REG(33));
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V6 3/5] phy: freescale: fsl-samsung-hdmi: Support dynamic integer

[Adam Ford]

There is currently a look-up table for a variety of resolutions.
Since the phy has the ability to dynamically calculate the values
necessary to use the intger divider which should allow more
resolutions without having to update the look-up-table.

If the lookup table cannot find an exact match, fall back to the
dynamic calculator of the integer divider.

Previously, the value of P was hard-coded to 1, this required an
update to the phy_pll_cfg table to add in the extra value into the
table, so if the value of P is calculated to be something else
by the PMS calculator, the calculated_phy_pll_cfg structure
can be used instead without having to keep track of which method
was used.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V6:  Fix comment typos and remove an unnecesary extra calculation
     by using the cached value.
V5:  No Change
V4:  No Change
V3:  Change size of pll_div_regs to include PHY_REG01 (P)
     Create calculated_phy_pll_cfg to containe the values
     Eliminate the PMS calculation from fsl_samsung_hdmi_phy_configure
     Make the LUT primary and fall back to integer calculator in
     phy_clk_round_rate.
     Check the range right away to ensure it's reaonsable rather than
     trying to find a clock only to learn it's outside the range.
     Overall added notes and comments where stuff may not be intuitive.

V2:  Update phy_clk_round_rate and phy_clk_set_rate to both support
     the integer clock PMS calculator.
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 341 +++++++++++++------
 1 file changed, 235 insertions(+), 106 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 4f6874226f9a..4b13e386e5ba 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -16,6 +16,8 @@
 
 #define PHY_REG(reg)		(reg * 4)
 
+#define REG01_PMS_P_MASK	GENMASK(3, 0)
+#define REG03_PMS_S_MASK	GENMASK(7, 4)
 #define REG12_CK_DIV_MASK	GENMASK(5, 4)
 
 #define REG13_TG_CODE_LOW_MASK	GENMASK(7, 0)
@@ -38,281 +40,296 @@
 #define REG34_PLL_LOCK		BIT(6)
 #define REG34_PHY_CLK_READY	BIT(5)
 
-#define PHY_PLL_DIV_REGS_NUM 6
+#ifndef MHZ
+#define MHZ	(1000UL * 1000UL)
+#endif
+
+#define PHY_PLL_DIV_REGS_NUM 7
 
 struct phy_config {
 	u32	pixclk;
 	u8	pll_div_regs[PHY_PLL_DIV_REGS_NUM];
 };
 
+/*
+ * The calculated_phy_pll_cfg only handles integer divider for PMS,
+ * meaning the last four entries will be fixed, but the first three will
+ * be calculated by the PMS calculator.
+ */
+static struct phy_config calculated_phy_pll_cfg = {
+	.pixclk = 0,
+	.pll_div_regs = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0x00 },
+};
+
+/* The lookup table contains values for which the fractional divder is used */
 static const struct phy_config phy_pll_cfg[] = {
 	{
 		.pixclk = 22250000,
-		.pll_div_regs = { 0x4b, 0xf1, 0x89, 0x88, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x4b, 0xf1, 0x89, 0x88, 0x80, 0x40 },
 	}, {
 		.pixclk = 23750000,
-		.pll_div_regs = { 0x50, 0xf1, 0x86, 0x85, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x50, 0xf1, 0x86, 0x85, 0x80, 0x40 },
 	}, {
 		.pixclk = 24000000,
-		.pll_div_regs = { 0x50, 0xf0, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x50, 0xf0, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 24024000,
-		.pll_div_regs = { 0x50, 0xf1, 0x99, 0x02, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x50, 0xf1, 0x99, 0x02, 0x80, 0x40 },
 	}, {
 		.pixclk = 25175000,
-		.pll_div_regs = { 0x54, 0xfc, 0xcc, 0x91, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x54, 0xfc, 0xcc, 0x91, 0x80, 0x40 },
 	}, {
 		.pixclk = 25200000,
-		.pll_div_regs = { 0x54, 0xf0, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x54, 0xf0, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 26750000,
-		.pll_div_regs = { 0x5a, 0xf2, 0x89, 0x88, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0xf2, 0x89, 0x88, 0x80, 0x40 },
 	}, {
 		.pixclk = 27000000,
-		.pll_div_regs = { 0x5a, 0xf0, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0xf0, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 27027000,
-		.pll_div_regs = { 0x5a, 0xf2, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0xf2, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 29500000,
-		.pll_div_regs = { 0x62, 0xf4, 0x95, 0x08, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x62, 0xf4, 0x95, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 30750000,
-		.pll_div_regs = { 0x66, 0xf4, 0x82, 0x01, 0x88, 0x45 },
+		.pll_div_regs = { 0xd1, 0x66, 0xf4, 0x82, 0x01, 0x88, 0x45 },
 	}, {
 		.pixclk = 30888000,
-		.pll_div_regs = { 0x66, 0xf4, 0x99, 0x18, 0x88, 0x45 },
+		.pll_div_regs = { 0xd1, 0x66, 0xf4, 0x99, 0x18, 0x88, 0x45 },
 	}, {
 		.pixclk = 33750000,
-		.pll_div_regs = { 0x70, 0xf4, 0x82, 0x01, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x70, 0xf4, 0x82, 0x01, 0x80, 0x40 },
 	}, {
 		.pixclk = 35000000,
-		.pll_div_regs = { 0x58, 0xb8, 0x8b, 0x88, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x58, 0xb8, 0x8b, 0x88, 0x80, 0x40 },
 	}, {
 		.pixclk = 36000000,
-		.pll_div_regs = { 0x5a, 0xb0, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0xb0, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 36036000,
-		.pll_div_regs = { 0x5a, 0xb2, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0xb2, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 40000000,
-		.pll_div_regs = { 0x64, 0xb0, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x64, 0xb0, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 43200000,
-		.pll_div_regs = { 0x5a, 0x90, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x90, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 43243200,
-		.pll_div_regs = { 0x5a, 0x92, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x92, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 44500000,
-		.pll_div_regs = { 0x5c, 0x92, 0x98, 0x11, 0x84, 0x41 },
+		.pll_div_regs = { 0xd1, 0x5c, 0x92, 0x98, 0x11, 0x84, 0x41 },
 	}, {
 		.pixclk = 47000000,
-		.pll_div_regs = { 0x62, 0x94, 0x95, 0x82, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x62, 0x94, 0x95, 0x82, 0x80, 0x40 },
 	}, {
 		.pixclk = 47500000,
-		.pll_div_regs = { 0x63, 0x96, 0xa1, 0x82, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x63, 0x96, 0xa1, 0x82, 0x80, 0x40 },
 	}, {
 		.pixclk = 50349650,
-		.pll_div_regs = { 0x54, 0x7c, 0xc3, 0x8f, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x54, 0x7c, 0xc3, 0x8f, 0x80, 0x40 },
 	}, {
 		.pixclk = 50400000,
-		.pll_div_regs = { 0x54, 0x70, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x54, 0x70, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 53250000,
-		.pll_div_regs = { 0x58, 0x72, 0x84, 0x03, 0x82, 0x41 },
+		.pll_div_regs = { 0xd1, 0x58, 0x72, 0x84, 0x03, 0x82, 0x41 },
 	}, {
 		.pixclk = 53500000,
-		.pll_div_regs = { 0x5a, 0x72, 0x89, 0x88, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x72, 0x89, 0x88, 0x80, 0x40 },
 	}, {
 		.pixclk = 54000000,
-		.pll_div_regs = { 0x5a, 0x70, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x70, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 54054000,
-		.pll_div_regs = { 0x5a, 0x72, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x72, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 59000000,
-		.pll_div_regs = { 0x62, 0x74, 0x95, 0x08, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x62, 0x74, 0x95, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 59340659,
-		.pll_div_regs = { 0x62, 0x74, 0xdb, 0x52, 0x88, 0x47 },
+		.pll_div_regs = { 0xd1, 0x62, 0x74, 0xdb, 0x52, 0x88, 0x47 },
 	}, {
 		.pixclk = 59400000,
-		.pll_div_regs = { 0x63, 0x70, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x63, 0x70, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 61500000,
-		.pll_div_regs = { 0x66, 0x74, 0x82, 0x01, 0x88, 0x45 },
+		.pll_div_regs = { 0xd1, 0x66, 0x74, 0x82, 0x01, 0x88, 0x45 },
 	}, {
 		.pixclk = 63500000,
-		.pll_div_regs = { 0x69, 0x74, 0x89, 0x08, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x69, 0x74, 0x89, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 67500000,
-		.pll_div_regs = { 0x54, 0x52, 0x87, 0x03, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x54, 0x52, 0x87, 0x03, 0x80, 0x40 },
 	}, {
 		.pixclk = 70000000,
-		.pll_div_regs = { 0x58, 0x58, 0x8b, 0x88, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x58, 0x58, 0x8b, 0x88, 0x80, 0x40 },
 	}, {
 		.pixclk = 72000000,
-		.pll_div_regs = { 0x5a, 0x50, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x50, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 72072000,
-		.pll_div_regs = { 0x5a, 0x52, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x52, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 74176000,
-		.pll_div_regs = { 0x5d, 0x58, 0xdb, 0xA2, 0x88, 0x41 },
+		.pll_div_regs = { 0xd1, 0x5d, 0x58, 0xdb, 0xA2, 0x88, 0x41 },
 	}, {
 		.pixclk = 74250000,
-		.pll_div_regs = { 0x5c, 0x52, 0x90, 0x0d, 0x84, 0x41 },
+		.pll_div_regs = { 0xd1, 0x5c, 0x52, 0x90, 0x0d, 0x84, 0x41 },
 	}, {
 		.pixclk = 78500000,
-		.pll_div_regs = { 0x62, 0x54, 0x87, 0x01, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x62, 0x54, 0x87, 0x01, 0x80, 0x40 },
 	}, {
 		.pixclk = 80000000,
-		.pll_div_regs = { 0x64, 0x50, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x64, 0x50, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 82000000,
-		.pll_div_regs = { 0x66, 0x54, 0x82, 0x01, 0x88, 0x45 },
+		.pll_div_regs = { 0xd1, 0x66, 0x54, 0x82, 0x01, 0x88, 0x45 },
 	}, {
 		.pixclk = 82500000,
-		.pll_div_regs = { 0x67, 0x54, 0x88, 0x01, 0x90, 0x49 },
+		.pll_div_regs = { 0xd1, 0x67, 0x54, 0x88, 0x01, 0x90, 0x49 },
 	}, {
 		.pixclk = 89000000,
-		.pll_div_regs = { 0x70, 0x54, 0x84, 0x83, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x70, 0x54, 0x84, 0x83, 0x80, 0x40 },
 	}, {
 		.pixclk = 90000000,
-		.pll_div_regs = { 0x70, 0x54, 0x82, 0x01, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x70, 0x54, 0x82, 0x01, 0x80, 0x40 },
 	}, {
 		.pixclk = 94000000,
-		.pll_div_regs = { 0x4e, 0x32, 0xa7, 0x10, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x4e, 0x32, 0xa7, 0x10, 0x80, 0x40 },
 	}, {
 		.pixclk = 95000000,
-		.pll_div_regs = { 0x50, 0x31, 0x86, 0x85, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x50, 0x31, 0x86, 0x85, 0x80, 0x40 },
 	}, {
 		.pixclk = 98901099,
-		.pll_div_regs = { 0x52, 0x3a, 0xdb, 0x4c, 0x88, 0x47 },
+		.pll_div_regs = { 0xd1, 0x52, 0x3a, 0xdb, 0x4c, 0x88, 0x47 },
 	}, {
 		.pixclk = 99000000,
-		.pll_div_regs = { 0x52, 0x32, 0x82, 0x01, 0x88, 0x47 },
+		.pll_div_regs = { 0xd1, 0x52, 0x32, 0x82, 0x01, 0x88, 0x47 },
 	}, {
 		.pixclk = 100699300,
-		.pll_div_regs = { 0x54, 0x3c, 0xc3, 0x8f, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x54, 0x3c, 0xc3, 0x8f, 0x80, 0x40 },
 	}, {
 		.pixclk = 100800000,
-		.pll_div_regs = { 0x54, 0x30, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x54, 0x30, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 102500000,
-		.pll_div_regs = { 0x55, 0x32, 0x8c, 0x05, 0x90, 0x4b },
+		.pll_div_regs = { 0xd1, 0x55, 0x32, 0x8c, 0x05, 0x90, 0x4b },
 	}, {
 		.pixclk = 104750000,
-		.pll_div_regs = { 0x57, 0x32, 0x98, 0x07, 0x90, 0x49 },
+		.pll_div_regs = { 0xd1, 0x57, 0x32, 0x98, 0x07, 0x90, 0x49 },
 	}, {
 		.pixclk = 106500000,
-		.pll_div_regs = { 0x58, 0x32, 0x84, 0x03, 0x82, 0x41 },
+		.pll_div_regs = { 0xd1, 0x58, 0x32, 0x84, 0x03, 0x82, 0x41 },
 	}, {
 		.pixclk = 107000000,
-		.pll_div_regs = { 0x5a, 0x32, 0x89, 0x88, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x32, 0x89, 0x88, 0x80, 0x40 },
 	}, {
 		.pixclk = 108000000,
-		.pll_div_regs = { 0x5a, 0x30, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x30, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 108108000,
-		.pll_div_regs = { 0x5a, 0x32, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x32, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 118000000,
-		.pll_div_regs = { 0x62, 0x34, 0x95, 0x08, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x62, 0x34, 0x95, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 118800000,
-		.pll_div_regs = { 0x63, 0x30, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x63, 0x30, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 123000000,
-		.pll_div_regs = { 0x66, 0x34, 0x82, 0x01, 0x88, 0x45 },
+		.pll_div_regs = { 0xd1, 0x66, 0x34, 0x82, 0x01, 0x88, 0x45 },
 	}, {
 		.pixclk = 127000000,
-		.pll_div_regs = { 0x69, 0x34, 0x89, 0x08, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x69, 0x34, 0x89, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 135000000,
-		.pll_div_regs = { 0x70, 0x34, 0x82, 0x01, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x70, 0x34, 0x82, 0x01, 0x80, 0x40 },
 	}, {
 		.pixclk = 135580000,
-		.pll_div_regs = { 0x71, 0x39, 0xe9, 0x82, 0x9c, 0x5b },
+		.pll_div_regs = { 0xd1, 0x71, 0x39, 0xe9, 0x82, 0x9c, 0x5b },
 	}, {
 		.pixclk = 137520000,
-		.pll_div_regs = { 0x72, 0x38, 0x99, 0x10, 0x85, 0x41 },
+		.pll_div_regs = { 0xd1, 0x72, 0x38, 0x99, 0x10, 0x85, 0x41 },
 	}, {
 		.pixclk = 138750000,
-		.pll_div_regs = { 0x73, 0x35, 0x88, 0x05, 0x90, 0x4d },
+		.pll_div_regs = { 0xd1, 0x73, 0x35, 0x88, 0x05, 0x90, 0x4d },
 	}, {
 		.pixclk = 140000000,
-		.pll_div_regs = { 0x75, 0x36, 0xa7, 0x90, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x75, 0x36, 0xa7, 0x90, 0x80, 0x40 },
 	}, {
 		.pixclk = 144000000,
-		.pll_div_regs = { 0x78, 0x30, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x78, 0x30, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 148352000,
-		.pll_div_regs = { 0x7b, 0x35, 0xdb, 0x39, 0x90, 0x45 },
+		.pll_div_regs = { 0xd1, 0x7b, 0x35, 0xdb, 0x39, 0x90, 0x45 },
 	}, {
 		.pixclk = 148500000,
-		.pll_div_regs = { 0x7b, 0x35, 0x84, 0x03, 0x90, 0x45 },
+		.pll_div_regs = { 0xd1, 0x7b, 0x35, 0x84, 0x03, 0x90, 0x45 },
 	}, {
 		.pixclk = 154000000,
-		.pll_div_regs = { 0x40, 0x18, 0x83, 0x01, 0x00, 0x40 },
+		.pll_div_regs = { 0xd1, 0x40, 0x18, 0x83, 0x01, 0x00, 0x40 },
 	}, {
 		.pixclk = 157000000,
-		.pll_div_regs = { 0x41, 0x11, 0xa7, 0x14, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x41, 0x11, 0xa7, 0x14, 0x80, 0x40 },
 	}, {
 		.pixclk = 160000000,
-		.pll_div_regs = { 0x42, 0x12, 0xa1, 0x20, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x42, 0x12, 0xa1, 0x20, 0x80, 0x40 },
 	}, {
 		.pixclk = 162000000,
-		.pll_div_regs = { 0x43, 0x18, 0x8b, 0x08, 0x96, 0x55 },
+		.pll_div_regs = { 0xd1, 0x43, 0x18, 0x8b, 0x08, 0x96, 0x55 },
 	}, {
 		.pixclk = 164000000,
-		.pll_div_regs = { 0x45, 0x11, 0x83, 0x82, 0x90, 0x4b },
+		.pll_div_regs = { 0xd1, 0x45, 0x11, 0x83, 0x82, 0x90, 0x4b },
 	}, {
 		.pixclk = 165000000,
-		.pll_div_regs = { 0x45, 0x11, 0x84, 0x81, 0x90, 0x4b },
+		.pll_div_regs = { 0xd1, 0x45, 0x11, 0x84, 0x81, 0x90, 0x4b },
 	}, {
 		.pixclk = 180000000,
-		.pll_div_regs = { 0x4b, 0x10, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x4b, 0x10, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 185625000,
-		.pll_div_regs = { 0x4e, 0x12, 0x9a, 0x95, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x4e, 0x12, 0x9a, 0x95, 0x80, 0x40 },
 	}, {
 		.pixclk = 188000000,
-		.pll_div_regs = { 0x4e, 0x12, 0xa7, 0x10, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x4e, 0x12, 0xa7, 0x10, 0x80, 0x40 },
 	}, {
 		.pixclk = 198000000,
-		.pll_div_regs = { 0x52, 0x12, 0x82, 0x01, 0x88, 0x47 },
+		.pll_div_regs = { 0xd1, 0x52, 0x12, 0x82, 0x01, 0x88, 0x47 },
 	}, {
 		.pixclk = 205000000,
-		.pll_div_regs = { 0x55, 0x12, 0x8c, 0x05, 0x90, 0x4b },
+		.pll_div_regs = { 0xd1, 0x55, 0x12, 0x8c, 0x05, 0x90, 0x4b },
 	}, {
 		.pixclk = 209500000,
-		.pll_div_regs = { 0x57, 0x12, 0x98, 0x07, 0x90, 0x49 },
+		.pll_div_regs = { 0xd1, 0x57, 0x12, 0x98, 0x07, 0x90, 0x49 },
 	}, {
 		.pixclk = 213000000,
-		.pll_div_regs = { 0x58, 0x12, 0x84, 0x03, 0x82, 0x41 },
+		.pll_div_regs = { 0xd1, 0x58, 0x12, 0x84, 0x03, 0x82, 0x41 },
 	}, {
 		.pixclk = 216000000,
-		.pll_div_regs = { 0x5a, 0x10, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x10, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 216216000,
-		.pll_div_regs = { 0x5a, 0x12, 0xfd, 0x0c, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x5a, 0x12, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 237600000,
-		.pll_div_regs = { 0x63, 0x10, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x63, 0x10, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 254000000,
-		.pll_div_regs = { 0x69, 0x14, 0x89, 0x08, 0x80, 0x40 },
+		.pll_div_regs = { 0xd1, 0x69, 0x14, 0x89, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 277500000,
-		.pll_div_regs = { 0x73, 0x15, 0x88, 0x05, 0x90, 0x4d },
+		.pll_div_regs = { 0xd1, 0x73, 0x15, 0x88, 0x05, 0x90, 0x4d },
 	}, {
 		.pixclk = 288000000,
-		.pll_div_regs = { 0x78, 0x10, 0x00, 0x00, 0x80, 0x00 },
+		.pll_div_regs = { 0xd1, 0x78, 0x10, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 297000000,
-		.pll_div_regs = { 0x7b, 0x15, 0x84, 0x03, 0x90, 0x45 },
+		.pll_div_regs = { 0xd1, 0x7b, 0x15, 0x84, 0x03, 0x90, 0x45 },
 	},
 };
 
@@ -322,7 +339,8 @@ struct reg_settings {
 };
 
 static const struct reg_settings common_phy_cfg[] = {
-	{ PHY_REG(0), 0x00 }, { PHY_REG(1), 0xd1 },
+	{ PHY_REG(0), 0x00 },
+	/* PHY_REG(1-7) pix clk specific */
 	{ PHY_REG(8), 0x4f }, { PHY_REG(9), 0x30 },
 	{ PHY_REG(10), 0x33 }, { PHY_REG(11), 0x65 },
 	/* REG12 pixclk specific */
@@ -415,6 +433,76 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
 	       phy->regs + PHY_REG(14));
 }
 
+static unsigned long fsl_samsung_hdmi_phy_find_pms(unsigned long fout, u8 *p, u16 *m, u8 *s)
+{
+	unsigned long best_freq = 0;
+	u32 min_delta = 0xffffffff;
+	u8 _p, best_p;
+	u16 _m, best_m;
+	u8 _s, best_s;
+
+	/* The ref manual states the values of 'P' range from 1 to 11 */
+	for (_p = 1; _p <= 11; ++_p) {
+		for (_s = 1; _s <= 16; ++_s) {
+			u64 tmp;
+			u32 delta;
+
+			/* s must be one or even */
+			if (_s > 1 && (_s & 0x01) == 1)
+				_s++;
+
+			/* _s cannot be 14 per the TRM */
+			if (_s == 14)
+				continue;
+
+			/*
+			 * TODO: Ref Manual doesn't state the range of _m
+			 * so this should be further refined if possible.
+			 * This range was set based on the original values
+			 * in the lookup table
+			 */
+			tmp = (u64)fout * (_p * _s);
+			do_div(tmp, 24 * MHZ);
+			_m = tmp;
+			if (_m < 0x30 || _m > 0x7b)
+				continue;
+
+			/*
+			 * Rev 2 of the Ref Manual states the
+			 * VCO can range between 750MHz and
+			 * 3GHz.  The VCO is assumed to be
+			 * is assumed to be (M * f_ref) / P,
+			 * where f_ref is 24MHz.
+			 */
+			tmp = (u64)_m * 24 * MHZ;
+			do_div(tmp, _p);
+			if (tmp < 750 * MHZ ||
+			    tmp > 3000 * MHZ)
+				continue;
+
+			/* Final frequency after post-divider */
+			do_div(tmp, _s);
+
+			delta = abs(fout - tmp);
+			if (delta < min_delta) {
+				best_p = _p;
+				best_s = _s;
+				best_m = _m;
+				min_delta = delta;
+				best_freq = tmp;
+			}
+		}
+	}
+
+	if (best_freq) {
+		*p = best_p;
+		*m = best_m;
+		*s = best_s;
+	}
+
+	return best_freq;
+}
+
 static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
 					  const struct phy_config *cfg)
 {
@@ -428,13 +516,13 @@ static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
 	for (i = 0; i < ARRAY_SIZE(common_phy_cfg); i++)
 		writeb(common_phy_cfg[i].val, phy->regs + common_phy_cfg[i].reg);
 
-	/* set individual PLL registers PHY_REG2 ... PHY_REG7 */
+	/* set individual PLL registers PHY_REG1 ... PHY_REG7 */
 	for (i = 0; i < PHY_PLL_DIV_REGS_NUM; i++)
-		writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG(2) + i * 4);
+		writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG(1) + i * 4);
 
-	/* High nibble of pll_div_regs[1] contains S which also gets written to REG21 */
+	/* High nibble of PHY_REG3 and low nibble of PHY_REG21 both contain 'S' */
 	writeb(REG21_SEL_TX_CK_INV | FIELD_PREP(REG21_PMS_S_MASK,
-	       cfg->pll_div_regs[1] >> 4), phy->regs + PHY_REG(21));
+	       cfg->pll_div_regs[2] >> 4), phy->regs + PHY_REG(21));
 
 	fsl_samsung_hdmi_phy_configure_pll_lock_det(phy, cfg);
 
@@ -462,29 +550,70 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
 static long phy_clk_round_rate(struct clk_hw *hw,
 			       unsigned long rate, unsigned long *parent_rate)
 {
+	u32 int_div_clk;
 	int i;
+	u16 m;
+	u8 p, s;
+
+	/* If the clock is out of range return error instead of searching */
+	if (rate > 297000000 || rate < 22250000)
+		return -EINVAL;
 
+	/* Check the lookup table */
 	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
 		if (phy_pll_cfg[i].pixclk <= rate)
-			return phy_pll_cfg[i].pixclk;
+			break;
+	/* If the rate is an exact match, return it now */
+	if (rate == phy_pll_cfg[i].pixclk)
+		return phy_pll_cfg[i].pixclk;
+
+	/*
+	 * The math on the lookup table shows the PMS math yields a
+	 * frequency 5 x pixclk.
+	 * When we check the integer divider against the desired rate,
+	 * multiply the rate x 5 and then divide the outcome by 5.
+	 */
+	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
 
-	return -EINVAL;
+	/* If the rate is an exact match, return it now */
+	if (int_div_clk == rate)
+		return int_div_clk;
+
+	/* Fall back to the closest value in the LUT */
+	return phy_pll_cfg[i].pixclk;
 }
 
 static int phy_clk_set_rate(struct clk_hw *hw,
 			    unsigned long rate, unsigned long parent_rate)
 {
 	struct fsl_samsung_hdmi_phy *phy = to_fsl_samsung_hdmi_phy(hw);
+	u32 int_div_clk;
 	int i;
-
-	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
-		if (phy_pll_cfg[i].pixclk <= rate)
-			break;
-
-	if (i < 0)
-		return -EINVAL;
-
-	phy->cur_cfg = &phy_pll_cfg[i];
+	u16 m;
+	u8 p, s;
+
+	/* If the integer divider works, just use it */
+	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
+	if (int_div_clk == rate) {
+		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
+		calculated_phy_pll_cfg.pixclk = int_div_clk;
+		calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
+		calculated_phy_pll_cfg.pll_div_regs[1] = m;
+		calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
+		/* pll_div_regs 3-6 are fixed and pre-defined already */
+		phy->cur_cfg  = &calculated_phy_pll_cfg;
+	} else {
+		/* Otherwise, search the LUT */
+		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
+		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
+			if (phy_pll_cfg[i].pixclk <= rate)
+				break;
+
+		if (i < 0)
+			return -EINVAL;
+
+		phy->cur_cfg = &phy_pll_cfg[i];
+	}
 
 	return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
 }
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Adam Ford]

Currently, if the clock values cannot be set to the exact rate,
the round_rate and set_rate functions use the closest value found in
the look-up-table.  In preparation of removing values from the LUT
that can be calculated evenly with the integer calculator, it's
necessary to ensure to check both the look-up-table and the integer
divider clock values to get the closest values to the requested
value.  It does this by measuring the difference between the
requested clock value and the closest value in both integer divider
calucator and the fractional clock look-up-table.

Which ever has the smallest difference between them is returned as
the cloesest rate.

Signed-off-by: Adam Ford <aford173@gmail.com>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
V6:  Simplify the calculation of the closest rate and fix
     a situation where the integer divider values may not be properly
     setup before they are used.
     Fixup some comments
V5:  No Change
V4:  New to series
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 46 ++++++++++++++------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 4b13e386e5ba..9a21dbbf1a82 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -547,6 +547,16 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
 	return phy->cur_cfg->pixclk;
 }
 
+static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
+						 u32 int_div_clk, u32 frac_div_clk)
+{
+	/* The int_div_clk may be greater than rate, so cast it and use ABS */
+	if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))
+		return int_div_clk;
+
+	return frac_div_clk;
+}
+
 static long phy_clk_round_rate(struct clk_hw *hw,
 			       unsigned long rate, unsigned long *parent_rate)
 {
@@ -563,6 +573,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
 	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
 		if (phy_pll_cfg[i].pixclk <= rate)
 			break;
+
 	/* If the rate is an exact match, return it now */
 	if (rate == phy_pll_cfg[i].pixclk)
 		return phy_pll_cfg[i].pixclk;
@@ -579,8 +590,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
 	if (int_div_clk == rate)
 		return int_div_clk;
 
-	/* Fall back to the closest value in the LUT */
-	return phy_pll_cfg[i].pixclk;
+	return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, phy_pll_cfg[i].pixclk);
 }
 
 static int phy_clk_set_rate(struct clk_hw *hw,
@@ -594,27 +604,37 @@ static int phy_clk_set_rate(struct clk_hw *hw,
 
 	/* If the integer divider works, just use it */
 	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
+	calculated_phy_pll_cfg.pixclk = int_div_clk;
+	calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
+	calculated_phy_pll_cfg.pll_div_regs[1] = m;
+	calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
+	phy->cur_cfg = &calculated_phy_pll_cfg;
 	if (int_div_clk == rate) {
 		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
-		calculated_phy_pll_cfg.pixclk = int_div_clk;
-		calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
-		calculated_phy_pll_cfg.pll_div_regs[1] = m;
-		calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
-		/* pll_div_regs 3-6 are fixed and pre-defined already */
-		phy->cur_cfg  = &calculated_phy_pll_cfg;
+		goto done;
 	} else {
 		/* Otherwise, search the LUT */
-		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
-		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
-			if (phy_pll_cfg[i].pixclk <= rate)
+		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
+			if (phy_pll_cfg[i].pixclk == rate) {
+				dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
+				phy->cur_cfg = &phy_pll_cfg[i];
+				goto done;
+			}
+
+			if (phy_pll_cfg[i].pixclk < rate)
 				break;
+		}
 
 		if (i < 0)
 			return -EINVAL;
-
-		phy->cur_cfg = &phy_pll_cfg[i];
 	}
 
+	if (fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
+						  phy_pll_cfg[i].pixclk) == int_div_clk)
+		phy->cur_cfg = &calculated_phy_pll_cfg;
+	else
+		phy->cur_cfg = &phy_pll_cfg[i];
+done:
 	return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
 }
 
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH V6 5/5] phy: freescale: fsl-samsung-hdmi: Remove unnecessary LUT entries

[Adam Ford]

The lookup table contains entries which use the integer divider
instead of just the fractional divider. Since the set and round
functions check both the integer divider values and the LUT values,
it's no longer necessary to keep the integer divider values in the
lookup table, as can be dynamically calcuated.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V6: Fix typos in commit message
V5: No Change
V4:  New to series
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 83 +++-----------------
 1 file changed, 13 insertions(+), 70 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 9a21dbbf1a82..1203143bad7a 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -69,25 +69,16 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 23750000,
 		.pll_div_regs = { 0xd1, 0x50, 0xf1, 0x86, 0x85, 0x80, 0x40 },
-	}, {
-		.pixclk = 24000000,
-		.pll_div_regs = { 0xd1, 0x50, 0xf0, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 24024000,
 		.pll_div_regs = { 0xd1, 0x50, 0xf1, 0x99, 0x02, 0x80, 0x40 },
 	}, {
 		.pixclk = 25175000,
 		.pll_div_regs = { 0xd1, 0x54, 0xfc, 0xcc, 0x91, 0x80, 0x40 },
-	}, {
-		.pixclk = 25200000,
-		.pll_div_regs = { 0xd1, 0x54, 0xf0, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 26750000,
 		.pll_div_regs = { 0xd1, 0x5a, 0xf2, 0x89, 0x88, 0x80, 0x40 },
-	}, {
-		.pixclk = 27000000,
-		.pll_div_regs = { 0xd1, 0x5a, 0xf0, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 27027000,
 		.pll_div_regs = { 0xd1, 0x5a, 0xf2, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
@@ -105,18 +96,9 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 35000000,
 		.pll_div_regs = { 0xd1, 0x58, 0xb8, 0x8b, 0x88, 0x80, 0x40 },
-	}, {
-		.pixclk = 36000000,
-		.pll_div_regs = { 0xd1, 0x5a, 0xb0, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 36036000,
 		.pll_div_regs = { 0xd1, 0x5a, 0xb2, 0xfd, 0x0c, 0x80, 0x40 },
-	}, {
-		.pixclk = 40000000,
-		.pll_div_regs = { 0xd1, 0x64, 0xb0, 0x00, 0x00, 0x80, 0x00 },
-	}, {
-		.pixclk = 43200000,
-		.pll_div_regs = { 0xd1, 0x5a, 0x90, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 43243200,
 		.pll_div_regs = { 0xd1, 0x5a, 0x92, 0xfd, 0x0c, 0x80, 0x40 },
@@ -132,19 +114,13 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 50349650,
 		.pll_div_regs = { 0xd1, 0x54, 0x7c, 0xc3, 0x8f, 0x80, 0x40 },
-	}, {
-		.pixclk = 50400000,
-		.pll_div_regs = { 0xd1, 0x54, 0x70, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 53250000,
 		.pll_div_regs = { 0xd1, 0x58, 0x72, 0x84, 0x03, 0x82, 0x41 },
 	}, {
 		.pixclk = 53500000,
 		.pll_div_regs = { 0xd1, 0x5a, 0x72, 0x89, 0x88, 0x80, 0x40 },
-	}, {
-		.pixclk = 54000000,
-		.pll_div_regs = { 0xd1, 0x5a, 0x70, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 54054000,
 		.pll_div_regs = { 0xd1, 0x5a, 0x72, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
@@ -153,10 +129,7 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 59340659,
 		.pll_div_regs = { 0xd1, 0x62, 0x74, 0xdb, 0x52, 0x88, 0x47 },
-	}, {
-		.pixclk = 59400000,
-		.pll_div_regs = { 0xd1, 0x63, 0x70, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 61500000,
 		.pll_div_regs = { 0xd1, 0x66, 0x74, 0x82, 0x01, 0x88, 0x45 },
 	}, {
@@ -168,10 +141,7 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 70000000,
 		.pll_div_regs = { 0xd1, 0x58, 0x58, 0x8b, 0x88, 0x80, 0x40 },
-	}, {
-		.pixclk = 72000000,
-		.pll_div_regs = { 0xd1, 0x5a, 0x50, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 72072000,
 		.pll_div_regs = { 0xd1, 0x5a, 0x52, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
@@ -183,10 +153,7 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 78500000,
 		.pll_div_regs = { 0xd1, 0x62, 0x54, 0x87, 0x01, 0x80, 0x40 },
-	}, {
-		.pixclk = 80000000,
-		.pll_div_regs = { 0xd1, 0x64, 0x50, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 82000000,
 		.pll_div_regs = { 0xd1, 0x66, 0x54, 0x82, 0x01, 0x88, 0x45 },
 	}, {
@@ -213,10 +180,7 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 100699300,
 		.pll_div_regs = { 0xd1, 0x54, 0x3c, 0xc3, 0x8f, 0x80, 0x40 },
-	}, {
-		.pixclk = 100800000,
-		.pll_div_regs = { 0xd1, 0x54, 0x30, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 102500000,
 		.pll_div_regs = { 0xd1, 0x55, 0x32, 0x8c, 0x05, 0x90, 0x4b },
 	}, {
@@ -228,19 +192,13 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 107000000,
 		.pll_div_regs = { 0xd1, 0x5a, 0x32, 0x89, 0x88, 0x80, 0x40 },
-	}, {
-		.pixclk = 108000000,
-		.pll_div_regs = { 0xd1, 0x5a, 0x30, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 108108000,
 		.pll_div_regs = { 0xd1, 0x5a, 0x32, 0xfd, 0x0c, 0x80, 0x40 },
 	}, {
 		.pixclk = 118000000,
 		.pll_div_regs = { 0xd1, 0x62, 0x34, 0x95, 0x08, 0x80, 0x40 },
-	}, {
-		.pixclk = 118800000,
-		.pll_div_regs = { 0xd1, 0x63, 0x30, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 123000000,
 		.pll_div_regs = { 0xd1, 0x66, 0x34, 0x82, 0x01, 0x88, 0x45 },
 	}, {
@@ -261,10 +219,7 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 140000000,
 		.pll_div_regs = { 0xd1, 0x75, 0x36, 0xa7, 0x90, 0x80, 0x40 },
-	}, {
-		.pixclk = 144000000,
-		.pll_div_regs = { 0xd1, 0x78, 0x30, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 148352000,
 		.pll_div_regs = { 0xd1, 0x7b, 0x35, 0xdb, 0x39, 0x90, 0x45 },
 	}, {
@@ -288,9 +243,6 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 165000000,
 		.pll_div_regs = { 0xd1, 0x45, 0x11, 0x84, 0x81, 0x90, 0x4b },
-	}, {
-		.pixclk = 180000000,
-		.pll_div_regs = { 0xd1, 0x4b, 0x10, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 185625000,
 		.pll_div_regs = { 0xd1, 0x4e, 0x12, 0x9a, 0x95, 0x80, 0x40 },
@@ -309,25 +261,16 @@ static const struct phy_config phy_pll_cfg[] = {
 	}, {
 		.pixclk = 213000000,
 		.pll_div_regs = { 0xd1, 0x58, 0x12, 0x84, 0x03, 0x82, 0x41 },
-	}, {
-		.pixclk = 216000000,
-		.pll_div_regs = { 0xd1, 0x5a, 0x10, 0x00, 0x00, 0x80, 0x00 },
 	}, {
 		.pixclk = 216216000,
 		.pll_div_regs = { 0xd1, 0x5a, 0x12, 0xfd, 0x0c, 0x80, 0x40 },
-	}, {
-		.pixclk = 237600000,
-		.pll_div_regs = { 0xd1, 0x63, 0x10, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 254000000,
 		.pll_div_regs = { 0xd1, 0x69, 0x14, 0x89, 0x08, 0x80, 0x40 },
 	}, {
 		.pixclk = 277500000,
 		.pll_div_regs = { 0xd1, 0x73, 0x15, 0x88, 0x05, 0x90, 0x4d },
-	}, {
-		.pixclk = 288000000,
-		.pll_div_regs = { 0xd1, 0x78, 0x10, 0x00, 0x00, 0x80, 0x00 },
-	}, {
+	},  {
 		.pixclk = 297000000,
 		.pll_div_regs = { 0xd1, 0x7b, 0x15, 0x84, 0x03, 0x90, 0x45 },
 	},
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Frieder Schrempf]

On 05.09.24 1:30 AM, Adam Ford wrote:
> Currently, if the clock values cannot be set to the exact rate,
> the round_rate and set_rate functions use the closest value found in
> the look-up-table.  In preparation of removing values from the LUT
> that can be calculated evenly with the integer calculator, it's
> necessary to ensure to check both the look-up-table and the integer
> divider clock values to get the closest values to the requested
> value.  It does this by measuring the difference between the
> requested clock value and the closest value in both integer divider
> calucator and the fractional clock look-up-table.
> 
> Which ever has the smallest difference between them is returned as
> the cloesest rate.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 0/5] phy: freescale: fsl-samsung-hdmi: Expand phy clock options

[Frieder Schrempf]

On 05.09.24 1:30 AM, Adam Ford wrote:
> Currently, there is a look-up-table to describe all the clock options the HDMI PHY
> can use.  Some of these entries in the LUT are using a fractional divider which does
> not have a well documented algorithm for determinging values, but the the integer
> divider can use an algorithm to calculate the integer divder values dynamically
> beyond those listed in the LUT and also duplicates some of the entries.
> 
> The first two patches do not do anything functionally other than simplify
> some of the register accesses and de-duplicates some of the register look-ups.
> 
> The third patch adds support for the integer divider and uses it whenever the
> clock request is an exact match.  Otherwise, it will use the LUT as before.
> The rouding is still based on the LUT if the integer clock isn't an exact match.
> 
> The forth patch updates thes set_rate and round_rate functions to use either
> the fractional clock LUT or the the integer divder mechanism to determine
> which ever clock rate might be closest match.
> 
> The last patch removes the integer divider entries from the LUT since by then
> it'll be comparing both the integer divider calculator and the closest value
> in the LUT.
> 
> In my testing with a AOC 4K monitor, I was able to add 4 entries in my modetest
> table.  I do not have an HDMI analyzer, so I just used my monitor to determine
> if this series worked.

So I tested this series and it works fine. With Dominique's patch to
allow for 0.5% deviation for the clock, all the 24 modes of my monitor
and 30 out of 42 modes of my HDMI grabber are working now.

I still have some issues with LCDIF underrun errors on modeswitch with
v6.11-rc6 but these are unrelated to this series.

Thanks Adam and Dominique for the great work!

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 0/5] phy: freescale: fsl-samsung-hdmi: Expand phy clock options

[Adam Ford]

On Thu, Sep 5, 2024 at 2:49 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 05.09.24 1:30 AM, Adam Ford wrote:
> > Currently, there is a look-up-table to describe all the clock options the HDMI PHY
> > can use.  Some of these entries in the LUT are using a fractional divider which does
> > not have a well documented algorithm for determinging values, but the the integer
> > divider can use an algorithm to calculate the integer divder values dynamically
> > beyond those listed in the LUT and also duplicates some of the entries.
> >
> > The first two patches do not do anything functionally other than simplify
> > some of the register accesses and de-duplicates some of the register look-ups.
> >
> > The third patch adds support for the integer divider and uses it whenever the
> > clock request is an exact match.  Otherwise, it will use the LUT as before.
> > The rouding is still based on the LUT if the integer clock isn't an exact match.
> >
> > The forth patch updates thes set_rate and round_rate functions to use either
> > the fractional clock LUT or the the integer divder mechanism to determine
> > which ever clock rate might be closest match.
> >
> > The last patch removes the integer divider entries from the LUT since by then
> > it'll be comparing both the integer divider calculator and the closest value
> > in the LUT.
> >
> > In my testing with a AOC 4K monitor, I was able to add 4 entries in my modetest
> > table.  I do not have an HDMI analyzer, so I just used my monitor to determine
> > if this series worked.
>
> So I tested this series and it works fine. With Dominique's patch to
> allow for 0.5% deviation for the clock, all the 24 modes of my monitor
> and 30 out of 42 modes of my HDMI grabber are working now.
>
> I still have some issues with LCDIF underrun errors on modeswitch with
> v6.11-rc6 but these are unrelated to this series.

I was comparing the LCDIF driver from the NXP downstream and the
mainline, and I noticed the panic threshold values are different in
the NXP downstream for the LCDIF3 which generates the video for the
HDMI.

The downstream threshold states the default low value is 1/3 and the
default high value is 2/3
It appears the mainline matches these values [1].
However, there is an option to override the defaults and change the
values to 1/2 and 3/4.  I don't really understand what these values
do, but it might be worth some investigation to see if playing with
these values helps or not.  The notes in both versions state the panic
isn't designed to trigger an interrupt, but to get the Noc and QoS
modules to handle it.

Either way, it's unrelated to this patch, but my monitor doesn't
always sync values from the LUT, but sometimes it does, but I can't
tell if we have an underflow or not.

adam

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/mxsfb/lcdif_kms.c?h=next-20240905#n350

>
> Thanks Adam and Dominique for the great work!

I enjoy puzzles.  Reading through the tables and looking for patterns
was fun.  I did spend some time trying to understand the fractional
divider stuff, but I didn't make much progress.

adam

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 0/5] phy: freescale: fsl-samsung-hdmi: Expand phy clock options

[Frieder Schrempf]

On 05.09.24 2:35 PM, Adam Ford wrote:
> On Thu, Sep 5, 2024 at 2:49 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> On 05.09.24 1:30 AM, Adam Ford wrote:
>>> Currently, there is a look-up-table to describe all the clock options the HDMI PHY
>>> can use.  Some of these entries in the LUT are using a fractional divider which does
>>> not have a well documented algorithm for determinging values, but the the integer
>>> divider can use an algorithm to calculate the integer divder values dynamically
>>> beyond those listed in the LUT and also duplicates some of the entries.
>>>
>>> The first two patches do not do anything functionally other than simplify
>>> some of the register accesses and de-duplicates some of the register look-ups.
>>>
>>> The third patch adds support for the integer divider and uses it whenever the
>>> clock request is an exact match.  Otherwise, it will use the LUT as before.
>>> The rouding is still based on the LUT if the integer clock isn't an exact match.
>>>
>>> The forth patch updates thes set_rate and round_rate functions to use either
>>> the fractional clock LUT or the the integer divder mechanism to determine
>>> which ever clock rate might be closest match.
>>>
>>> The last patch removes the integer divider entries from the LUT since by then
>>> it'll be comparing both the integer divider calculator and the closest value
>>> in the LUT.
>>>
>>> In my testing with a AOC 4K monitor, I was able to add 4 entries in my modetest
>>> table.  I do not have an HDMI analyzer, so I just used my monitor to determine
>>> if this series worked.
>>
>> So I tested this series and it works fine. With Dominique's patch to
>> allow for 0.5% deviation for the clock, all the 24 modes of my monitor
>> and 30 out of 42 modes of my HDMI grabber are working now.
>>
>> I still have some issues with LCDIF underrun errors on modeswitch with
>> v6.11-rc6 but these are unrelated to this series.
> 
> I was comparing the LCDIF driver from the NXP downstream and the
> mainline, and I noticed the panic threshold values are different in
> the NXP downstream for the LCDIF3 which generates the video for the
> HDMI.
> 
> The downstream threshold states the default low value is 1/3 and the
> default high value is 2/3
> It appears the mainline matches these values [1].
> However, there is an option to override the defaults and change the
> values to 1/2 and 3/4.  I don't really understand what these values
> do, but it might be worth some investigation to see if playing with
> these values helps or not.  The notes in both versions state the panic
> isn't designed to trigger an interrupt, but to get the Noc and QoS
> modules to handle it.

I will try that. Although I doubt that it is related to the thresholds.
This should be relevant in cases where bus priorities play some role
(high load, conflicts with other peripherals, etc.).

In my case the target is basically idle and other peripherals are not
used. Also it happens only when switching modes or turning on/off.

> 
> Either way, it's unrelated to this patch, but my monitor doesn't
> always sync values from the LUT, but sometimes it does, but I can't
> tell if we have an underflow or not.

You can check by reading register 0x32fc6024 and see if the second bit
(UNDERRUN) is set.

> 
> adam
> 
> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/mxsfb/lcdif_kms.c?h=next-20240905#n350
> 
>>
>> Thanks Adam and Dominique for the great work!
> 
> I enjoy puzzles.  Reading through the tables and looking for patterns
> was fun.  I did spend some time trying to understand the fractional
> divider stuff, but I didn't make much progress.

I tried myself and I made some further progress today. But I'm not quite
there, yet. The goal would be to be able to calculate some more table
entries. Let's see.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Dominique Martinet]

(sorry I meant to send this yesterday but I'm being forced to adjust my
mail pipeline with work and gmail and it didn't go out -- trying
again. Sorry if it actually did go through. Hopefully I didn't misfire
anything else yesterday...)

Adam Ford wrote on Wed, Sep 04, 2024 at 06:30:32PM -0500:
> Currently, if the clock values cannot be set to the exact rate,
> the round_rate and set_rate functions use the closest value found in
> the look-up-table.  In preparation of removing values from the LUT
> that can be calculated evenly with the integer calculator, it's
> necessary to ensure to check both the look-up-table and the integer
> divider clock values to get the closest values to the requested
> value.  It does this by measuring the difference between the
> requested clock value and the closest value in both integer divider
> calucator and the fractional clock look-up-table.
> 
> Which ever has the smallest difference between them is returned as
> the cloesest rate.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>

b4 (or whatever you're using) probably picked that up from the patch I
included in my reply to this patch, this sob should go away.



> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 4b13e386e5ba..9a21dbbf1a82 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -547,6 +547,16 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
>  	return phy->cur_cfg->pixclk;
>  }
>  
> +static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
> +						 u32 int_div_clk, u32 frac_div_clk)
> +{
> +	/* The int_div_clk may be greater than rate, so cast it and use ABS */
> +	if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))

I still think `rate - frac_div_clk` might not always hold in the future
(because there is no intrinsic reason we'd pick the smaller end in case
of inexact match and a future improvement might change this to the
closest value as well), so I'll argue again for having both use abs(),
but at least there's only one place to update if that changes in the
future now so hopefully whoever does this will notice...

> +		return int_div_clk;
> +
> +	return frac_div_clk;
> +}
> +
>  static long phy_clk_round_rate(struct clk_hw *hw,
>  			       unsigned long rate, unsigned long *parent_rate)
>  {
> @@ -563,6 +573,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>  	for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
>  		if (phy_pll_cfg[i].pixclk <= rate)
>  			break;
> +

(unrelated)

>  	/* If the rate is an exact match, return it now */
>  	if (rate == phy_pll_cfg[i].pixclk)
>  		return phy_pll_cfg[i].pixclk;
> @@ -579,8 +590,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>  	if (int_div_clk == rate)
>  		return int_div_clk;
>  
> -	/* Fall back to the closest value in the LUT */
> -	return phy_pll_cfg[i].pixclk;
> +	return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, phy_pll_cfg[i].pixclk);
>  }
>  
>  static int phy_clk_set_rate(struct clk_hw *hw,
> @@ -594,27 +604,37 @@ static int phy_clk_set_rate(struct clk_hw *hw,
>  
>  	/* If the integer divider works, just use it */

I found this comment a bit confusing given the current flow as of this
patch. Might make more sense immediately before the if?


>  	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
> +	calculated_phy_pll_cfg.pixclk = int_div_clk;
> +	calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
> +	calculated_phy_pll_cfg.pll_div_regs[1] = m;
> +	calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
> +	phy->cur_cfg = &calculated_phy_pll_cfg;
>  	if (int_div_clk == rate) {
>  		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
> -		calculated_phy_pll_cfg.pixclk = int_div_clk;
> -		calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
> -		calculated_phy_pll_cfg.pll_div_regs[1] = m;
> -		calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
> -		/* pll_div_regs 3-6 are fixed and pre-defined already */

nitpick: might want to keep the above comment?

> -		phy->cur_cfg  = &calculated_phy_pll_cfg;
> +		goto done;
>  	} else {
>  		/* Otherwise, search the LUT */
> -		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
> -		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> -			if (phy_pll_cfg[i].pixclk <= rate)
> +		for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
> +			if (phy_pll_cfg[i].pixclk == rate) {
> +				dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");

nitpick: might make sense to print what was picked in case of inexact
match as well, but these are dbg warning so probably fine either way.


overall I find the flow of this function hard to read; it's a bit ugly
flow-wise but jumping in the clock comparison 'if' might help trim this?
(and if we're going out of our way to factor out the diff, maybe the lut
lookup could be as well)

But I'm probably just being overcritical here, it's fine as is if you
pefer your version, just writing down this as an illustration of what I
meant with the above sentence as I'm not sure I was clear -- I'll be
just as happy to consider this series done so we can do more interesting
things :P

{
    u32 int_div_clk, frac_div_clk;
    int i;
    u16 m;
    u8 p, s;
    
    // (I haven't given up on that *5 to move inside this function...)
    int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
    if (int_div_clk == rate)
        goto use_int_clk;

    frac_div_clk = fsl_samsung_hdmi_phy_find_lut(rate, &i);
    // (not convinced that check actually brings much, but it's not like
    // it hurts either)
    if (frac_div_clk == rate)
        goto use_frac_clk;
    
    if (fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
                                              frac_div_clk) == int_div_clk) {
use_int_clk:
        dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
        calculated_phy_pll_cfg.pixclk = int_div_clk;
        calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
        calculated_phy_pll_cfg.pll_div_regs[1] = m;
        calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
        /* pll_div_regs 3-6 are fixed and pre-defined already */
        phy->cur_cfg  = &calculated_phy_pll_cfg;
    } else {
use_frac_clk:
        dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
        phy->cur_cfg = &phy_pll_cfg[i];
    }
    return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
}

-- 
Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Adam Ford]

On Thu, Sep 5, 2024 at 7:26 PM Dominique Martinet
<dominique.martinet@atmark-techno.com> wrote:
>
>
> (sorry I meant to send this yesterday but I'm being forced to adjust my
> mail pipeline with work and gmail and it didn't go out -- trying
> again. Sorry if it actually did go through. Hopefully I didn't misfire
> anything else yesterday...)
>
> Adam Ford wrote on Wed, Sep 04, 2024 at 06:30:32PM -0500:
> > Currently, if the clock values cannot be set to the exact rate,
> > the round_rate and set_rate functions use the closest value found in
> > the look-up-table.  In preparation of removing values from the LUT
> > that can be calculated evenly with the integer calculator, it's
> > necessary to ensure to check both the look-up-table and the integer
> > divider clock values to get the closest values to the requested
> > value.  It does this by measuring the difference between the
> > requested clock value and the closest value in both integer divider
> > calucator and the fractional clock look-up-table.
> >
> > Which ever has the smallest difference between them is returned as
> > the cloesest rate.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
>
> b4 (or whatever you're using) probably picked that up from the patch I
> included in my reply to this patch, this sob should go away.
>
For each iteration, I grabbed the patches from patchwork which
contained any s-o-b messages, if present.  I didn't add anything
manually.
>
>
> > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > index 4b13e386e5ba..9a21dbbf1a82 100644
> > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -547,6 +547,16 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
> >       return phy->cur_cfg->pixclk;
> >  }
> >
> > +static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
> > +                                              u32 int_div_clk, u32 frac_div_clk)
> > +{
> > +     /* The int_div_clk may be greater than rate, so cast it and use ABS */
> > +     if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))
>
> I still think `rate - frac_div_clk` might not always hold in the future
> (because there is no intrinsic reason we'd pick the smaller end in case
> of inexact match and a future improvement might change this to the
> closest value as well), so I'll argue again for having both use abs(),
> but at least there's only one place to update if that changes in the
> future now so hopefully whoever does this will notice...

I can add the ABS on the fractional divider.  I left it out on purpose
since the LUT table always return a value equal or less, so the extra
ABS seemed like busy work.  However, I can see the argument for being
consistent.

>
> > +             return int_div_clk;
> > +
> > +     return frac_div_clk;
> > +}
> > +
> >  static long phy_clk_round_rate(struct clk_hw *hw,
> >                              unsigned long rate, unsigned long *parent_rate)
> >  {
> > @@ -563,6 +573,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
> >       for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> >               if (phy_pll_cfg[i].pixclk <= rate)
> >                       break;
> > +
>
> (unrelated)

I don't understand what you're asking here.

>
> >       /* If the rate is an exact match, return it now */
> >       if (rate == phy_pll_cfg[i].pixclk)
> >               return phy_pll_cfg[i].pixclk;
> > @@ -579,8 +590,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
> >       if (int_div_clk == rate)
> >               return int_div_clk;
> >
> > -     /* Fall back to the closest value in the LUT */
> > -     return phy_pll_cfg[i].pixclk;
> > +     return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, phy_pll_cfg[i].pixclk);
> >  }
> >
> >  static int phy_clk_set_rate(struct clk_hw *hw,
> > @@ -594,27 +604,37 @@ static int phy_clk_set_rate(struct clk_hw *hw,
> >
> >       /* If the integer divider works, just use it */
>
> I found this comment a bit confusing given the current flow as of this
> patch. Might make more sense immediately before the if?
>

This code evolved with each iteration, but I didn't necessarily
reorganize the comments.  I can rearrange them.
>
> >       int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
> > +     calculated_phy_pll_cfg.pixclk = int_div_clk;
> > +     calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
> > +     calculated_phy_pll_cfg.pll_div_regs[1] = m;
> > +     calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
> > +     phy->cur_cfg = &calculated_phy_pll_cfg;
> >       if (int_div_clk == rate) {
> >               dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
> > -             calculated_phy_pll_cfg.pixclk = int_div_clk;
> > -             calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
> > -             calculated_phy_pll_cfg.pll_div_regs[1] = m;
> > -             calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
> > -             /* pll_div_regs 3-6 are fixed and pre-defined already */
>
> nitpick: might want to keep the above comment?

ok.
>
> > -             phy->cur_cfg  = &calculated_phy_pll_cfg;
> > +             goto done;
> >       } else {
> >               /* Otherwise, search the LUT */
> > -             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
> > -             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> > -                     if (phy_pll_cfg[i].pixclk <= rate)
> > +             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
> > +                     if (phy_pll_cfg[i].pixclk == rate) {
> > +                             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>
> nitpick: might make sense to print what was picked in case of inexact
> match as well, but these are dbg warning so probably fine either way.

I can add the actual values returned.

>
>
> overall I find the flow of this function hard to read; it's a bit ugly
> flow-wise but jumping in the clock comparison 'if' might help trim this?
> (and if we're going out of our way to factor out the diff, maybe the lut
> lookup could be as well)
>
> But I'm probably just being overcritical here, it's fine as is if you
> pefer your version, just writing down this as an illustration of what I
> meant with the above sentence as I'm not sure I was clear -- I'll be
> just as happy to consider this series done so we can do more interesting
> things :P

Now I am a bit more confused, because above I got the impression you
were withdrawing your s-o-b, but now it sounds like you want to move
it forward.

It sounded like Frieder was making some progress on understanding a
little more about the fractional divider.

>
> {
>     u32 int_div_clk, frac_div_clk;
>     int i;
>     u16 m;
>     u8 p, s;
>
>     // (I haven't given up on that *5 to move inside this function...)

I wanted to keep the PMS calculator returning the real clock value
since the calculations are based on equation in the ref manual, Fpll =
Fref * M / (P*S)
This way, the calling function can determine if it needs to be
multiplied by 5.  I haven't fully determined how the fractional
calculator determines what frequency it wants for a target frequency,
and using the values for P, M and S from the fractional divider
doesn't seem to always yield 5x like they did for the table entries
using the integer divider.

I am hoping someone from NXP can elaborate, or give us some clues on
how to get better fractional divider values.

>     int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
>     if (int_div_clk == rate)
>         goto use_int_clk;
>
>     frac_div_clk = fsl_samsung_hdmi_phy_find_lut(rate, &i);
>     // (not convinced that check actually brings much, but it's not like
>     // it hurts either)
>     if (frac_div_clk == rate)
>         goto use_frac_clk;
>
>     if (fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
>                                               frac_div_clk) == int_div_clk) {
> use_int_clk:
>         dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
>         calculated_phy_pll_cfg.pixclk = int_div_clk;
>         calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
>         calculated_phy_pll_cfg.pll_div_regs[1] = m;
>         calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
>         /* pll_div_regs 3-6 are fixed and pre-defined already */
>         phy->cur_cfg  = &calculated_phy_pll_cfg;
>     } else {
> use_frac_clk:
>         dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>         phy->cur_cfg = &phy_pll_cfg[i];
>     }
>     return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> }
>
> --
> Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Dominique Martinet]

Adam Ford wrote on Thu, Sep 05, 2024 at 07:57:36PM -0500:
> > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> >
> > b4 (or whatever you're using) probably picked that up from the patch I
> > included in my reply to this patch, this sob should go away.
> >
> For each iteration, I grabbed the patches from patchwork which
> contained any s-o-b messages, if present.  I didn't add anything
> manually.

Yes, I'm just saying the tool got confused - I replied to an earlier
iteration of this patch with a quote of the 0.5% tolerance patch I
properly sent afterwards here:
https://lore.kernel.org/all/ZtaawD3vb8gmnVmO@atmark-techno.com/

And it picked the sob from the patch, that I hadn't intended to be added
as a review -- from my understanding a sob is something you'd add if you
pick the patch through your tree (e.g. I add sob for patches I apply
through the 9p tree I maintain), but not something to give on reviews.
I'll reply to individual patches with reviewed-by/tested-by once this
thread has settled down; I don't have more comments than what I sent
just now, so I think this is almost ready.


> > > +static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
> > > +                                              u32 int_div_clk, u32 frac_div_clk)
> > > +{
> > > +     /* The int_div_clk may be greater than rate, so cast it and use ABS */
> > > +     if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))
> >
> > I still think `rate - frac_div_clk` might not always hold in the future
> > (because there is no intrinsic reason we'd pick the smaller end in case
> > of inexact match and a future improvement might change this to the
> > closest value as well), so I'll argue again for having both use abs(),
> > but at least there's only one place to update if that changes in the
> > future now so hopefully whoever does this will notice...
> 
> I can add the ABS on the fractional divider.  I left it out on purpose
> since the LUT table always return a value equal or less, so the extra
> ABS seemed like busy work.  However, I can see the argument for being
> consistent.

Yeah, I agree it's not needed right now; I won't insist on this, just
saying I prefer consistency/future-proofing here.

> > > +             return int_div_clk;
> > > +
> > > +     return frac_div_clk;
> > > +}
> > > +
> > >  static long phy_clk_round_rate(struct clk_hw *hw,
> > >                              unsigned long rate, unsigned long *parent_rate)
> > >  {
> > > @@ -563,6 +573,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
> > >       for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> > >               if (phy_pll_cfg[i].pixclk <= rate)
> > >                       break;
> > > +
> >
> > (unrelated)
> 
> I don't understand what you're asking here.

This chunk is just adding a blank line that's not related to the rest of
the patch; it's fine, just pointed it out if it wasn't intended.
(these are usually leftovers from tests or something like that)

> > > -             phy->cur_cfg  = &calculated_phy_pll_cfg;
> > > +             goto done;
> > >       } else {
> > >               /* Otherwise, search the LUT */
> > > -             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
> > > -             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> > > -                     if (phy_pll_cfg[i].pixclk <= rate)
> > > +             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
> > > +                     if (phy_pll_cfg[i].pixclk == rate) {
> > > +                             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
> >
> > nitpick: might make sense to print what was picked in case of inexact
> > match as well, but these are dbg warning so probably fine either way.
> 
> I can add the actual values returned.

Note my argument here wasn't about having the values on this line, it's
that the inexact case (when comparing with
fsl_samsung_hdmi_phy_get_closest_rate) doesn't print either line

(having the values can't hurt for dbg though, I think it could be useful
if you rework this)

> > overall I find the flow of this function hard to read; it's a bit ugly
> > flow-wise but jumping in the clock comparison 'if' might help trim this?
> > (and if we're going out of our way to factor out the diff, maybe the lut
> > lookup could be as well)
> >
> > But I'm probably just being overcritical here, it's fine as is if you
> > pefer your version, just writing down this as an illustration of what I
> > meant with the above sentence as I'm not sure I was clear -- I'll be
> > just as happy to consider this series done so we can do more interesting
> > things :P
> 
> Now I am a bit more confused, because above I got the impression you
> were withdrawing your s-o-b, but now it sounds like you want to move
> it forward.

I don't have any strong opinion here: I generally dislike nitpicking
about form, and value the progress you've done more than fixing the flow
of this function (e.g. this function already is good/progress from what
we have right now, we don't need perfect)

withdrawing my s-o-b doesn't meant I disagree with the patch, I just
hadn't willingly given it. If you don't mind reworking the patch a bit
I'll retest the new version, otherwise I'm fine approving this as is.

> It sounded like Frieder was making some progress on understanding a
> little more about the fractional divider.

That's awesome!
I think we can/want to get this merged first, and we can improve
with fractional divider calculations in a later iteration to get rid of
the LUT altogether; it'll be easier for other users of the chip to go a
step at a time as well if they notice a breakage.

> >     // (I haven't given up on that *5 to move inside this function...)
> 
> I wanted to keep the PMS calculator returning the real clock value
> since the calculations are based on equation in the ref manual, Fpll =
> Fref * M / (P*S)
> This way, the calling function can determine if it needs to be
> multiplied by 5.  I haven't fully determined how the fractional
> calculator determines what frequency it wants for a target frequency,
> and using the values for P, M and S from the fractional divider
> doesn't seem to always yield 5x like they did for the table entries
> using the integer divider.

The way I see it is that 5x is an artifact of the PMS calculation:
the caller doesn't want '5x rate', it wants 'rate', and actually setting
the p/m/s values provided by the function gets us 'rate', so it feels
like that's not the caller should have to worry about.. When we add
fractional divider support then it'll be the calculator's job to
determine if/when it needs that 5x.

But if I understand what you're saying, fsl_samsung_hdmi_phy_find_pms()
is not a pms specific to the NXP chip, but something more general to any
integer divisor that is better kept more generic if it weren't for the
few references to our hardware (e.g. limits on m)?

If so then I guess I can understand your position, I wouldn't go as far
as saying it warrants another level of indirection so I guess it's fine
as is.

(In a similar line of thought, had I implemented this I would have had
the function not return generic p/m/s but return the pll registers
directly -- it took me a moment to check why we were setting regs[2] to
`s-1` and confirming we had s>1...)



Anyway, once again I don't feel like I am in any position to be preachy
about all of this, so I'm fine with this patch as is - please take of it
what you agree with/want to rework and we can leave the rest here as far
as I'm concerned.
-- 
Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Frieder Schrempf]

On 06.09.24 02:57, Adam Ford wrote:
> On Thu, Sep 5, 2024 at 7:26 PM Dominique Martinet
> <dominique.martinet@atmark-techno.com> wrote:
>>
>>
>> (sorry I meant to send this yesterday but I'm being forced to adjust my
>> mail pipeline with work and gmail and it didn't go out -- trying
>> again. Sorry if it actually did go through. Hopefully I didn't misfire
>> anything else yesterday...)
>>
>> Adam Ford wrote on Wed, Sep 04, 2024 at 06:30:32PM -0500:
>>> Currently, if the clock values cannot be set to the exact rate,
>>> the round_rate and set_rate functions use the closest value found in
>>> the look-up-table.  In preparation of removing values from the LUT
>>> that can be calculated evenly with the integer calculator, it's
>>> necessary to ensure to check both the look-up-table and the integer
>>> divider clock values to get the closest values to the requested
>>> value.  It does this by measuring the difference between the
>>> requested clock value and the closest value in both integer divider
>>> calucator and the fractional clock look-up-table.
>>>
>>> Which ever has the smallest difference between them is returned as
>>> the cloesest rate.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
>>
>> b4 (or whatever you're using) probably picked that up from the patch I
>> included in my reply to this patch, this sob should go away.
>>
> For each iteration, I grabbed the patches from patchwork which
> contained any s-o-b messages, if present.  I didn't add anything
> manually.
>>
>>
>>> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>>> index 4b13e386e5ba..9a21dbbf1a82 100644
>>> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>>> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>>> @@ -547,6 +547,16 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
>>>        return phy->cur_cfg->pixclk;
>>>   }
>>>
>>> +static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
>>> +                                              u32 int_div_clk, u32 frac_div_clk)
>>> +{
>>> +     /* The int_div_clk may be greater than rate, so cast it and use ABS */
>>> +     if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))
>>
>> I still think `rate - frac_div_clk` might not always hold in the future
>> (because there is no intrinsic reason we'd pick the smaller end in case
>> of inexact match and a future improvement might change this to the
>> closest value as well), so I'll argue again for having both use abs(),
>> but at least there's only one place to update if that changes in the
>> future now so hopefully whoever does this will notice...
> 
> I can add the ABS on the fractional divider.  I left it out on purpose
> since the LUT table always return a value equal or less, so the extra
> ABS seemed like busy work.  However, I can see the argument for being
> consistent.
> 
>>
>>> +             return int_div_clk;
>>> +
>>> +     return frac_div_clk;
>>> +}
>>> +
>>>   static long phy_clk_round_rate(struct clk_hw *hw,
>>>                               unsigned long rate, unsigned long *parent_rate)
>>>   {
>>> @@ -563,6 +573,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>>>        for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
>>>                if (phy_pll_cfg[i].pixclk <= rate)
>>>                        break;
>>> +
>>
>> (unrelated)
> 
> I don't understand what you're asking here.
> 
>>
>>>        /* If the rate is an exact match, return it now */
>>>        if (rate == phy_pll_cfg[i].pixclk)
>>>                return phy_pll_cfg[i].pixclk;
>>> @@ -579,8 +590,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
>>>        if (int_div_clk == rate)
>>>                return int_div_clk;
>>>
>>> -     /* Fall back to the closest value in the LUT */
>>> -     return phy_pll_cfg[i].pixclk;
>>> +     return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, phy_pll_cfg[i].pixclk);
>>>   }
>>>
>>>   static int phy_clk_set_rate(struct clk_hw *hw,
>>> @@ -594,27 +604,37 @@ static int phy_clk_set_rate(struct clk_hw *hw,
>>>
>>>        /* If the integer divider works, just use it */
>>
>> I found this comment a bit confusing given the current flow as of this
>> patch. Might make more sense immediately before the if?
>>
> 
> This code evolved with each iteration, but I didn't necessarily
> reorganize the comments.  I can rearrange them.
>>
>>>        int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
>>> +     calculated_phy_pll_cfg.pixclk = int_div_clk;
>>> +     calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
>>> +     calculated_phy_pll_cfg.pll_div_regs[1] = m;
>>> +     calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
>>> +     phy->cur_cfg = &calculated_phy_pll_cfg;
>>>        if (int_div_clk == rate) {
>>>                dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
>>> -             calculated_phy_pll_cfg.pixclk = int_div_clk;
>>> -             calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
>>> -             calculated_phy_pll_cfg.pll_div_regs[1] = m;
>>> -             calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
>>> -             /* pll_div_regs 3-6 are fixed and pre-defined already */
>>
>> nitpick: might want to keep the above comment?
> 
> ok.
>>
>>> -             phy->cur_cfg  = &calculated_phy_pll_cfg;
>>> +             goto done;
>>>        } else {
>>>                /* Otherwise, search the LUT */
>>> -             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>>> -             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
>>> -                     if (phy_pll_cfg[i].pixclk <= rate)
>>> +             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
>>> +                     if (phy_pll_cfg[i].pixclk == rate) {
>>> +                             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>>
>> nitpick: might make sense to print what was picked in case of inexact
>> match as well, but these are dbg warning so probably fine either way.
> 
> I can add the actual values returned.
> 
>>
>>
>> overall I find the flow of this function hard to read; it's a bit ugly
>> flow-wise but jumping in the clock comparison 'if' might help trim this?
>> (and if we're going out of our way to factor out the diff, maybe the lut
>> lookup could be as well)
>>
>> But I'm probably just being overcritical here, it's fine as is if you
>> pefer your version, just writing down this as an illustration of what I
>> meant with the above sentence as I'm not sure I was clear -- I'll be
>> just as happy to consider this series done so we can do more interesting
>> things :P
> 
> Now I am a bit more confused, because above I got the impression you
> were withdrawing your s-o-b, but now it sounds like you want to move
> it forward.
> 
> It sounded like Frieder was making some progress on understanding a
> little more about the fractional divider.

I think I managed to get behind the calculation of the fractional-n 
divider parameters. I came up with a spreadsheet to calculate the output 
frequency from existing register values and I have a crude Python script 
that can be used to search for parameters for a given pixel clock.

I tested this with three different non-CEA-861 pixel clock values 
(supported by my HDMI USB grabber) for which the integer PLL yielded 
deviations >0.5%. With the new LUT entries those modes work now.

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.

> 
>>
>> {
>>      u32 int_div_clk, frac_div_clk;
>>      int i;
>>      u16 m;
>>      u8 p, s;
>>
>>      // (I haven't given up on that *5 to move inside this function...)
> 
> I wanted to keep the PMS calculator returning the real clock value
> since the calculations are based on equation in the ref manual, Fpll =
> Fref * M / (P*S)
> This way, the calling function can determine if it needs to be
> multiplied by 5.  I haven't fully determined how the fractional
> calculator determines what frequency it wants for a target frequency,
> and using the values for P, M and S from the fractional divider
> doesn't seem to always yield 5x like they did for the table entries
> using the integer divider.

For what I found out the factor of 5 always applies. For the integer 
part and also for the fractional part.

> 
> I am hoping someone from NXP can elaborate, or give us some clues on
> how to get better fractional divider values.


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Dominique Martinet]

Frieder Schrempf wrote on Fri, Sep 06, 2024 at 10:28:59PM +0200:
> I think I managed to get behind the calculation of the fractional-n divider
> parameters. I came up with a spreadsheet to calculate the output frequency
> from existing register values and I have a crude Python script that can be
> used to search for parameters for a given pixel clock.
> 
> I tested this with three different non-CEA-861 pixel clock values (supported
> by my HDMI USB grabber) for which the integer PLL yielded deviations >0.5%.
> With the new LUT entries those modes work now.
> 
> 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.

I think keeping the LUT is perfectly fine if we know where the values
come from - perhaps having your python program in a comment above the
LUT so anyone can check the values match?

My main problem with the LUT is just that -- there's no way of
checking. If the values come from somewhere sensible and can be verified
I think it makes sense to keep it to fill the holes where the integer
divider isn't enough.

> > This way, the calling function can determine if it needs to be
> > multiplied by 5.  I haven't fully determined how the fractional
> > calculator determines what frequency it wants for a target frequency,
> > and using the values for P, M and S from the fractional divider
> > doesn't seem to always yield 5x like they did for the table entries
> > using the integer divider.
> 
> For what I found out the factor of 5 always applies. For the integer part
> and also for the fractional part.

In that case I'm definitely in favor of moving it inside the function as
part of the calculation

Thank you both!
-- 
Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Adam Ford]

On Fri, Sep 6, 2024 at 11:50 PM Dominique Martinet
<dominique.martinet@atmark-techno.com> wrote:
>
> Frieder Schrempf wrote on Fri, Sep 06, 2024 at 10:28:59PM +0200:
> > I think I managed to get behind the calculation of the fractional-n divider
> > parameters. I came up with a spreadsheet to calculate the output frequency
> > from existing register values and I have a crude Python script that can be
> > used to search for parameters for a given pixel clock.
> >
> > I tested this with three different non-CEA-861 pixel clock values (supported
> > by my HDMI USB grabber) for which the integer PLL yielded deviations >0.5%.
> > With the new LUT entries those modes work now.
> >
> > 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.
>
> I think keeping the LUT is perfectly fine if we know where the values
> come from - perhaps having your python program in a comment above the
> LUT so anyone can check the values match?
>
> My main problem with the LUT is just that -- there's no way of
> checking. If the values come from somewhere sensible and can be verified
> I think it makes sense to keep it to fill the holes where the integer
> divider isn't enough.
>
> > > This way, the calling function can determine if it needs to be
> > > multiplied by 5.  I haven't fully determined how the fractional
> > > calculator determines what frequency it wants for a target frequency,
> > > and using the values for P, M and S from the fractional divider
> > > doesn't seem to always yield 5x like they did for the table entries
> > > using the integer divider.
> >
> > For what I found out the factor of 5 always applies. For the integer part
> > and also for the fractional part.
>
> In that case I'm definitely in favor of moving it inside the function as
> part of the calculation

I'll do a V7 with the 5x factor moved into the integer calculator.
I'll also try to rearrange the flow a bit as you requested.  Depending
on how much changes, I may strip off any s-o-b notes.
I was traveling over the weekend and I'm likely traveling the next
weekends, so I will try to get to it this week before I go, but I
can't promise anything quickly.

adam

>
> Thank you both!
> --
> Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy