* [RFC V3 1/3] phy: freescale: fsl-samsung-hdmi: Replace register defines with macro @ 2024-08-30 3:24 Adam Ford 2024-08-30 3:24 ` [RFC V3 2/3] phy: freescale: fsl-samsung-hdmi: Simplify REG21_PMS_S_MASK lookup Adam Ford 2024-08-30 3:24 ` [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer Adam Ford 0 siblings, 2 replies; 9+ messages in thread From: Adam Ford @ 2024-08-30 3:24 UTC (permalink / raw) To: linux-phy Cc: dominique.martinet, linux-imx, festevam, frieder.schrempf, aford, Adam Ford, Vinod Koul, Kishon Vijay Abraham I, Lucas Stach, Uwe Kleine-König, Marco Felsch, linux-kernel 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> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c index 9048cdc760c2..bc5d3625ece6 100644 --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c @@ -14,76 +14,20 @@ #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 - -/* - * 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 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 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) +#define REG33_MODE_SET_DONE BIT(7) +#define REG33_FIX_DA BIT(1) +#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 +313,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 +386,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 +413,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 +433,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 +447,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 +455,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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC V3 2/3] phy: freescale: fsl-samsung-hdmi: Simplify REG21_PMS_S_MASK lookup 2024-08-30 3:24 [RFC V3 1/3] phy: freescale: fsl-samsung-hdmi: Replace register defines with macro Adam Ford @ 2024-08-30 3:24 ` Adam Ford 2024-08-30 3:24 ` [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer Adam Ford 1 sibling, 0 replies; 9+ messages in thread From: Adam Ford @ 2024-08-30 3:24 UTC (permalink / raw) To: linux-phy Cc: dominique.martinet, linux-imx, festevam, frieder.schrempf, aford, Adam Ford, Vinod Koul, Kishon Vijay Abraham I, Lucas Stach, Marco Felsch, Uwe Kleine-König, linux-kernel 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> --- V3: New to series diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c index bc5d3625ece6..a700a300dc6f 100644 --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c @@ -355,40 +355,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) @@ -457,7 +423,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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-08-30 3:24 [RFC V3 1/3] phy: freescale: fsl-samsung-hdmi: Replace register defines with macro Adam Ford 2024-08-30 3:24 ` [RFC V3 2/3] phy: freescale: fsl-samsung-hdmi: Simplify REG21_PMS_S_MASK lookup Adam Ford @ 2024-08-30 3:24 ` Adam Ford 2024-08-30 5:56 ` Dominique Martinet 1 sibling, 1 reply; 9+ messages in thread From: Adam Ford @ 2024-08-30 3:24 UTC (permalink / raw) To: linux-phy Cc: dominique.martinet, linux-imx, festevam, frieder.schrempf, aford, Adam Ford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel 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> --- 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. diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c index a700a300dc6f..3fab40cde40d 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) #define REG14_TOL_MASK GENMASK(7, 4) @@ -29,281 +31,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 only, + * 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 }, }, }; @@ -313,7 +330,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 */ @@ -406,6 +424,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' rannge 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 look-up 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 _m x + * the reference frequency of 24MHz divided + * by the prescaler, _p + */ + tmp = (u64)_m * 24 * MHZ; + do_div(tmp, _p); + if (tmp < 750 * MHZ || + tmp > 3000 * MHZ) + continue; + + tmp = (u64)_m * 24 * MHZ; + do_div(tmp, _p * _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) { @@ -419,13 +507,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); @@ -453,29 +541,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 look-up 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-08-30 3:24 ` [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer Adam Ford @ 2024-08-30 5:56 ` Dominique Martinet 2024-08-30 15:04 ` Adam Ford 2024-09-02 21:20 ` Adam Ford 0 siblings, 2 replies; 9+ messages in thread From: Dominique Martinet @ 2024-08-30 5:56 UTC (permalink / raw) To: Adam Ford Cc: linux-phy, linux-imx, festevam, frieder.schrempf, aford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel, Makoto Sato Adam Ford wrote on Thu, Aug 29, 2024 at 10:24:27PM -0500: > 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. Thank you! I've updated to v3 and we must have missed something fiddling with v1 but our 31.5MHz-only screen turns on with this! Unfortunately among the other odd devices we support, there's one whose native resolution only supports 83.5MHz, and that doesn't come out right with the integer divider (that function returns 83.2MHz, which is 0.4%off) If we force the round/set rate functions to prefer the calculated value and allow that in imx8mp_hdmi_check_clk_rate (dw_hdmi-imx.c) then it also works, so I don't think we actually need to affine the model... Coming back to what Lucas replied to my initial mail HDMI would also a rate mismatch of ±0.5%, so the integer calculator works for all the frequencies we've currently added manually if we fix the check to allow that as well: 32000000: found 32000000 (100.0% match): p 1 / m 80 / s 12 51200000: found 51200000 (100.0% match): p 1 / m 64 / s 6 65000000: found 64800000 (99.6% match): p 1 / m 54 / s 4 71000000: found 70800000 (99.7% match): p 1 / m 59 / s 4 83500000: found 83200000 (99.6% match): p 1 / m 104 / s 6 (only actually tested 51.2 and 83.5 here, we don't have all the hardware available; I'll try to play with normal monitors that support more modes once the patch gets further finalized) So, as far as I'm concerned I'd be happy to move forward with that and will backport this to our tree/remove our kludged values, would "just" need to properly pick the closest value if no exact match instead of always falling back to the table (or just remove the table altogether if we can test a few monitors?) A couple of style nitpicks below > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > index a700a300dc6f..3fab40cde40d 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) > #define REG14_TOL_MASK GENMASK(7, 4) > @@ -29,281 +31,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 only, > + * 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 = { I'd change cur_cfg from pointer to the struct itself like this (partial patch that probably won't even apply on your branch:) ---- diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c index 9048cdc760c2..d7124604819c 100644 --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c @@ -402,7 +402,7 @@ struct fsl_samsung_hdmi_phy { /* clk provider */ struct clk_hw hw; - const struct phy_config *cur_cfg; + struct phy_config cur_cfg; }; static inline struct fsl_samsung_hdmi_phy * @@ -562,9 +562,9 @@ static int phy_clk_set_rate(struct clk_hw *hw, if (i < 0) return -EINVAL; - phy->cur_cfg = &phy_pll_cfg[i]; + phy->cur_cfg = phy_pll_cfg[i]; - return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); + return fsl_samsung_hdmi_phy_configure(phy, &phy->cur_cfg); } static const struct clk_ops phy_clk_ops = { ---- Then you can just set it directly for calculated values. But conceptually it's the same, just one less indirection. > @@ -406,6 +424,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' rannge from 1 to 11 */ typo: range > + 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 look-up 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 _m x > + * the reference frequency of 24MHz divided > + * by the prescaler, _p > + */ > + tmp = (u64)_m * 24 * MHZ; > + do_div(tmp, _p); > + if (tmp < 750 * MHZ || > + tmp > 3000 * MHZ) > + continue; > + > + tmp = (u64)_m * 24 * MHZ; > + do_div(tmp, _p * _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) > { > @@ -419,13 +507,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); > > @@ -453,29 +541,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 look-up 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; I'd move that comment and both multiplication and division inside fsl_samsung_hdmi_phy_find_pms, since it's a property of the computation (not having the comment made me ask last time, with the comment it's fine -- thanks for adding these comments, very helpful.) -- Dominique -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-08-30 5:56 ` Dominique Martinet @ 2024-08-30 15:04 ` Adam Ford 2024-09-01 21:46 ` Dominique Martinet 2024-09-02 21:20 ` Adam Ford 1 sibling, 1 reply; 9+ messages in thread From: Adam Ford @ 2024-08-30 15:04 UTC (permalink / raw) To: Dominique Martinet Cc: linux-phy, linux-imx, festevam, frieder.schrempf, aford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel, Makoto Sato On Fri, Aug 30, 2024 at 12:56 AM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Adam Ford wrote on Thu, Aug 29, 2024 at 10:24:27PM -0500: > > 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. > > Thank you! > > I've updated to v3 and we must have missed something fiddling with v1 > but our 31.5MHz-only screen turns on with this! > > Unfortunately among the other odd devices we support, there's one > whose native resolution only supports 83.5MHz, and that doesn't come out > right with the integer divider (that function returns 83.2MHz, which is > 0.4%off) > If we force the round/set rate functions to prefer the calculated value > and allow that in imx8mp_hdmi_check_clk_rate (dw_hdmi-imx.c) then it > also works, so I don't think we actually need to affine the model... > Coming back to what Lucas replied to my initial mail HDMI would also a > rate mismatch of ±0.5%, so the integer calculator works for all the > frequencies we've currently added manually if we fix the check to allow > that as well: > 32000000: found 32000000 (100.0% match): p 1 / m 80 / s 12 > 51200000: found 51200000 (100.0% match): p 1 / m 64 / s 6 > 65000000: found 64800000 (99.6% match): p 1 / m 54 / s 4 > 71000000: found 70800000 (99.7% match): p 1 / m 59 / s 4 > 83500000: found 83200000 (99.6% match): p 1 / m 104 / s 6 > > (only actually tested 51.2 and 83.5 here, we don't have all the hardware > available; I'll try to play with normal monitors that support more modes > once the patch gets further finalized) > > > So, as far as I'm concerned I'd be happy to move forward with that and > will backport this to our tree/remove our kludged values, would "just" > need to properly pick the closest value if no exact match instead of > always falling back to the table (or just remove the table altogether if > we can test a few monitors?) I took that step on my local build machine to remove the integer divider stuff from the LUT since it can be found automatically. I haven't committed them yet, but I'll likely add it to the series once submit it without the RFC. I am also likely to update the rounding function to look at the delta between the closest LUT clock value and the closest integer clock value and use whichever is closest to the desired value. > > A couple of style nitpicks below > > > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > > index a700a300dc6f..3fab40cde40d 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) > > #define REG14_TOL_MASK GENMASK(7, 4) > > @@ -29,281 +31,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 only, > > + * 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 = { > > I'd change cur_cfg from pointer to the struct itself like this (partial > patch that probably won't even apply on your branch:) > ---- > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > index 9048cdc760c2..d7124604819c 100644 > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > @@ -402,7 +402,7 @@ struct fsl_samsung_hdmi_phy { > > /* clk provider */ > struct clk_hw hw; > - const struct phy_config *cur_cfg; > + struct phy_config cur_cfg; > }; > > static inline struct fsl_samsung_hdmi_phy * > @@ -562,9 +562,9 @@ static int phy_clk_set_rate(struct clk_hw *hw, > if (i < 0) > return -EINVAL; > > - phy->cur_cfg = &phy_pll_cfg[i]; > + phy->cur_cfg = phy_pll_cfg[i]; > > - return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); > + return fsl_samsung_hdmi_phy_configure(phy, &phy->cur_cfg); > } > > static const struct clk_ops phy_clk_ops = { > ---- > > Then you can just set it directly for calculated values. > But conceptually it's the same, just one less indirection. I was thinking about that, but I wasn't sure the impact of un-const-ing would have on the LUT. Using the extra indirection minimizes the impact of changes to the LUT and how its handled by the compiler. adam > > > @@ -406,6 +424,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' rannge from 1 to 11 */ > > typo: range > > > + 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 look-up 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 _m x > > + * the reference frequency of 24MHz divided > > + * by the prescaler, _p > > + */ > > + tmp = (u64)_m * 24 * MHZ; > > + do_div(tmp, _p); > > + if (tmp < 750 * MHZ || > > + tmp > 3000 * MHZ) > > + continue; > > + > > + tmp = (u64)_m * 24 * MHZ; > > + do_div(tmp, _p * _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) > > { > > @@ -419,13 +507,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); > > > > @@ -453,29 +541,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 look-up 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; > > I'd move that comment and both multiplication and division inside > fsl_samsung_hdmi_phy_find_pms, since it's a property of the computation > (not having the comment made me ask last time, with the comment it's > fine -- thanks for adding these comments, very helpful.) > > -- > Dominique > > -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-08-30 15:04 ` Adam Ford @ 2024-09-01 21:46 ` Dominique Martinet 0 siblings, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2024-09-01 21:46 UTC (permalink / raw) To: Adam Ford Cc: linux-phy, linux-imx, festevam, frieder.schrempf, aford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel, Makoto Sato Adam Ford wrote on Fri, Aug 30, 2024 at 10:04:41AM -0500: > > So, as far as I'm concerned I'd be happy to move forward with that and > > will backport this to our tree/remove our kludged values, would "just" > > need to properly pick the closest value if no exact match instead of > > always falling back to the table (or just remove the table altogether if > > we can test a few monitors?) > > I took that step on my local build machine to remove the integer > divider stuff from the LUT since it can be found automatically. I > haven't committed them yet, but I'll likely add it to the series once > submit it without the RFC. > > I am also likely to update the rounding function to look at the delta > between the closest LUT clock value and the closest integer clock > value and use whichever is closest to the desired value. Sounds good, thanks! > > I'd change cur_cfg from pointer to the struct itself like this (partial > > patch that probably won't even apply on your branch:) > > ---- > > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > > index 9048cdc760c2..d7124604819c 100644 > > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > > @@ -402,7 +402,7 @@ struct fsl_samsung_hdmi_phy { > > > > /* clk provider */ > > struct clk_hw hw; > > - const struct phy_config *cur_cfg; > > + struct phy_config cur_cfg; > > }; > > > > static inline struct fsl_samsung_hdmi_phy * > > @@ -562,9 +562,9 @@ static int phy_clk_set_rate(struct clk_hw *hw, > > if (i < 0) > > return -EINVAL; > > > > - phy->cur_cfg = &phy_pll_cfg[i]; > > + phy->cur_cfg = phy_pll_cfg[i]; > > > > - return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); > > + return fsl_samsung_hdmi_phy_configure(phy, &phy->cur_cfg); > > } > > > > static const struct clk_ops phy_clk_ops = { > > ---- > > > > Then you can just set it directly for calculated values. > > But conceptually it's the same, just one less indirection. > > I was thinking about that, but I wasn't sure the impact of > un-const-ing would have on the LUT. > > Using the extra indirection minimizes the impact of changes to the LUT > and how its handled by the compiler. The LUT has to stay const, you don't want something modifying it by mistake; my suggestion makes cur_cfg a copy which does not need to be const and the assignment (`phy->cur_cfg = phy_pll_cfg[i];`) is equivalent to a memcpy -- the struct is small enough that the cost of copying it if the value didn't match is negligible in my opinion. With that said, I'm not saying you must do this, this just avoids using a static value which could be problematic if there are multiple parallel uses of the driver, go ahead with whatever you prefer for v4. (note my approach doesn't fix the same problem if the driver is used twice in parallel.. is it possible that e.g. one kernel thread calls phy_clk_set_rate() while another thread checks something about the hardware and calls phy_clk_round_rate()? If so neither approach is correct and we can't easily skip recomputing, and it'd be better to just have a phy_config struct on the stack instead -- on my imx8mp it takes 4.7us without -O, 0.6us with -O2 to compute; I *think* we can affort to recompute every time...) -- Dominique -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-08-30 5:56 ` Dominique Martinet 2024-08-30 15:04 ` Adam Ford @ 2024-09-02 21:20 ` Adam Ford 2024-09-03 0:47 ` Dominique Martinet 1 sibling, 1 reply; 9+ messages in thread From: Adam Ford @ 2024-09-02 21:20 UTC (permalink / raw) To: Dominique Martinet Cc: linux-phy, linux-imx, festevam, frieder.schrempf, aford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel, Makoto Sato On Fri, Aug 30, 2024 at 12:56 AM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Adam Ford wrote on Thu, Aug 29, 2024 at 10:24:27PM -0500: > > 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. > > Thank you! > > I've updated to v3 and we must have missed something fiddling with v1 > but our 31.5MHz-only screen turns on with this! > > Unfortunately among the other odd devices we support, there's one > whose native resolution only supports 83.5MHz, and that doesn't come out > right with the integer divider (that function returns 83.2MHz, which is > 0.4%off) > If we force the round/set rate functions to prefer the calculated value > and allow that in imx8mp_hdmi_check_clk_rate (dw_hdmi-imx.c) then it > also works, so I don't think we actually need to affine the model... > Coming back to what Lucas replied to my initial mail HDMI would also a > rate mismatch of ±0.5%, so the integer calculator works for all the > frequencies we've currently added manually if we fix the check to allow > that as well: > 32000000: found 32000000 (100.0% match): p 1 / m 80 / s 12 > 51200000: found 51200000 (100.0% match): p 1 / m 64 / s 6 > 65000000: found 64800000 (99.6% match): p 1 / m 54 / s 4 > 71000000: found 70800000 (99.7% match): p 1 / m 59 / s 4 > 83500000: found 83200000 (99.6% match): p 1 / m 104 / s 6 > > (only actually tested 51.2 and 83.5 here, we don't have all the hardware > available; I'll try to play with normal monitors that support more modes > once the patch gets further finalized) > > > So, as far as I'm concerned I'd be happy to move forward with that and > will backport this to our tree/remove our kludged values, would "just" > need to properly pick the closest value if no exact match instead of > always falling back to the table (or just remove the table altogether if > we can test a few monitors?) > > A couple of style nitpicks below > > > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > > index a700a300dc6f..3fab40cde40d 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) > > #define REG14_TOL_MASK GENMASK(7, 4) > > @@ -29,281 +31,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 only, > > + * 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 = { > > I'd change cur_cfg from pointer to the struct itself like this (partial > patch that probably won't even apply on your branch:) > ---- > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > index 9048cdc760c2..d7124604819c 100644 > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > @@ -402,7 +402,7 @@ struct fsl_samsung_hdmi_phy { > > /* clk provider */ > struct clk_hw hw; > - const struct phy_config *cur_cfg; > + struct phy_config cur_cfg; Wouldn't converting this from a pointer require me to do a memcpy later? It seems like that's more work than just pointing it to an address. > }; > > static inline struct fsl_samsung_hdmi_phy * > @@ -562,9 +562,9 @@ static int phy_clk_set_rate(struct clk_hw *hw, > if (i < 0) > return -EINVAL; > > - phy->cur_cfg = &phy_pll_cfg[i]; > + phy->cur_cfg = phy_pll_cfg[i]; I think this is would have to be a memcpy instead of just an equal statement since phy->cur_cfg would no longer be a pointer. > > - return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); > + return fsl_samsung_hdmi_phy_configure(phy, &phy->cur_cfg); > } > > static const struct clk_ops phy_clk_ops = { > ---- > > Then you can just set it directly for calculated values. > But conceptually it's the same, just one less indirection. > > > @@ -406,6 +424,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' rannge from 1 to 11 */ > > typo: range Thanks. I can fix. > > > + 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 look-up 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 _m x > > + * the reference frequency of 24MHz divided > > + * by the prescaler, _p > > + */ > > + tmp = (u64)_m * 24 * MHZ; > > + do_div(tmp, _p); > > + if (tmp < 750 * MHZ || > > + tmp > 3000 * MHZ) > > + continue; > > + > > + tmp = (u64)_m * 24 * MHZ; > > + do_div(tmp, _p * _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) > > { > > @@ -419,13 +507,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); > > > > @@ -453,29 +541,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 look-up 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; > > I'd move that comment and both multiplication and division inside > fsl_samsung_hdmi_phy_find_pms, since it's a property of the computation > (not having the comment made me ask last time, with the comment it's > fine -- thanks for adding these comments, very helpful.) > > -- > Dominique > > -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-09-02 21:20 ` Adam Ford @ 2024-09-03 0:47 ` Dominique Martinet 2024-09-03 1:20 ` Adam Ford 0 siblings, 1 reply; 9+ messages in thread From: Dominique Martinet @ 2024-09-03 0:47 UTC (permalink / raw) To: Adam Ford Cc: linux-phy, linux-imx, festevam, frieder.schrempf, aford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel, Makoto Sato Adam Ford wrote on Mon, Sep 02, 2024 at 04:20:11PM -0500: > > - const struct phy_config *cur_cfg; > > + struct phy_config cur_cfg; > > Wouldn't converting this from a pointer require me to do a memcpy > later? It seems like that's more work than just pointing it to an > address. > > > - phy->cur_cfg = &phy_pll_cfg[i]; > > + phy->cur_cfg = phy_pll_cfg[i]; > > I think this is would have to be a memcpy instead of just an equal > statement since phy->cur_cfg would no longer be a pointer. C allows copying structs like this, it's fine to write it as just an equal. It's not 100% equivalent, iiuc simple assignment is undefined behaviour if the elements aren't aligned but memcpy will work even in that case, but for us this is not a proble mand the generated code should be identical... Also note I'm only suggesting that because the struct is tiny (1*u32+7*u8 is less than two u64), but this code isn't meant to run very often anyway so we should prioritize readability -- if you think it's harder to understand than an extra pointer somewhere I have no strong opinion; as said in the previous mail if parallel uses are possible it'd be better kept on the stack anyway... -- Dominique -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer 2024-09-03 0:47 ` Dominique Martinet @ 2024-09-03 1:20 ` Adam Ford 0 siblings, 0 replies; 9+ messages in thread From: Adam Ford @ 2024-09-03 1:20 UTC (permalink / raw) To: Dominique Martinet Cc: linux-phy, linux-imx, festevam, frieder.schrempf, aford, Vinod Koul, Kishon Vijay Abraham I, Marco Felsch, Uwe Kleine-König, Lucas Stach, linux-kernel, Makoto Sato On Mon, Sep 2, 2024 at 7:47 PM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Adam Ford wrote on Mon, Sep 02, 2024 at 04:20:11PM -0500: > > > - const struct phy_config *cur_cfg; > > > + struct phy_config cur_cfg; > > > > Wouldn't converting this from a pointer require me to do a memcpy > > later? It seems like that's more work than just pointing it to an > > address. > > > > > - phy->cur_cfg = &phy_pll_cfg[i]; > > > + phy->cur_cfg = phy_pll_cfg[i]; > > > > I think this is would have to be a memcpy instead of just an equal > > statement since phy->cur_cfg would no longer be a pointer. > > C allows copying structs like this, it's fine to write it as just an > equal. > It's not 100% equivalent, iiuc simple assignment is undefined behaviour > if the elements aren't aligned but memcpy will work even in that case, > but for us this is not a proble mand the generated code should be > identical... Also note I'm only suggesting that because the struct is > tiny (1*u32+7*u8 is less than two u64), but this code isn't meant to run > very often anyway so we should prioritize readability -- if you think > it's harder to understand than an extra pointer somewhere I have no > strong opinion; as said in the previous mail if parallel uses are > possible it'd be better kept on the stack anyway... Dominique, Unless someone pushes back, I think keeping it a pointer is more consistent with what we did before, and it's more in my style of coding. I have two more patches in the series. * One updates the round and set functions to compare the closest values between the fractional divider and the integer divider and uses the option with the smallest difference between the achievable value and the desired rate if the actual value is not achievable. * The second removes the integer divider values from the LUT since having it there just takes up extra space and makes the LUT search longer. I'll send it as V4 with a cover letter describing everything I have done and the rationale behind it without the RFC designation. For what it's worth, adding the integer calculator added 4 entries to my modetest list, and changing the hdmi-tx to support a clock rante of +/- 5% added another 11 possible entries. I have not tested them all yet. I likely won't do a patch for the HDMI-TX, but if you want to send a patch increasing the tolerance, I'll likely test it as I have time. adam > > -- > Dominique > > -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-09-03 1:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-30 3:24 [RFC V3 1/3] phy: freescale: fsl-samsung-hdmi: Replace register defines with macro Adam Ford 2024-08-30 3:24 ` [RFC V3 2/3] phy: freescale: fsl-samsung-hdmi: Simplify REG21_PMS_S_MASK lookup Adam Ford 2024-08-30 3:24 ` [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer Adam Ford 2024-08-30 5:56 ` Dominique Martinet 2024-08-30 15:04 ` Adam Ford 2024-09-01 21:46 ` Dominique Martinet 2024-09-02 21:20 ` Adam Ford 2024-09-03 0:47 ` Dominique Martinet 2024-09-03 1:20 ` Adam Ford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).