| Message ID | 20240904023310.163371-5-aford173@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: freescale: fsl-samsung-hdmi: Expand phy clock options | expand |
On 04.09.24 4:32 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> > --- > V5: No Change > V4: New to series > --- > drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 40 +++++++++++++++----- > 1 file changed, 31 insertions(+), 9 deletions(-) > > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > index 8f2c0082aa12..56b08e684179 100644 > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > @@ -550,7 +550,7 @@ 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; > + u32 int_div_clk, delta_int, delta_frac; > int i; > u16 m; > u8 p, s; > @@ -563,6 +563,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,15 +580,21 @@ 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; > + /* Calculate the differences and use the closest one */ > + delta_frac = (rate - phy_pll_cfg[i].pixclk); > + delta_int = (rate - int_div_clk); > + > + if (delta_int < delta_frac) > + return int_div_clk; > + else > + 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; > + u32 int_div_clk, delta_int, delta_frac; > int i; > u16 m; > u8 p, s; > @@ -602,19 +609,34 @@ static int phy_clk_set_rate(struct clk_hw *hw, > 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; ^ unneeded whitespace (comment belongs to patch 3/5) > + 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]; > } > > + /* Calculate the differences for each clock against the requested value */ > + delta_frac = (rate - phy_pll_cfg[i].pixclk); > + delta_int = (rate - int_div_clk); > + > + /* Use the value closest to the desired */ > + if (delta_int < delta_frac) > + phy->cur_cfg = &calculated_phy_pll_cfg; ^ unneeded whitespace Are you sure that this is correct? The calculated_phy_pll_cfg is only set above if there is an exact match for the integer divider. I don't think it contains the data you expect here, or am I missing something? > + else > + phy->cur_cfg = &phy_pll_cfg[i]; > +done: > return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); > } >
On 04.09.24 3:40 PM, Frieder Schrempf wrote: > On 04.09.24 4:32 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> >> --- >> V5: No Change >> V4: New to series >> --- >> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 40 +++++++++++++++----- >> 1 file changed, 31 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c >> index 8f2c0082aa12..56b08e684179 100644 >> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c >> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c >> @@ -550,7 +550,7 @@ 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; >> + u32 int_div_clk, delta_int, delta_frac; >> int i; >> u16 m; >> u8 p, s; >> @@ -563,6 +563,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,15 +580,21 @@ 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; >> + /* Calculate the differences and use the closest one */ >> + delta_frac = (rate - phy_pll_cfg[i].pixclk); >> + delta_int = (rate - int_div_clk); >> + >> + if (delta_int < delta_frac) >> + return int_div_clk; >> + else >> + return phy_pll_cfg[i].pixclk; I would also prefer to use a helper for calculating the closest rate. Something like: static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate, u32 int_div_clk, u32 frac_div_clk) { if ((rate - int_div_clk) < (rate - frac_div_clk)) return int_div_clk; return frac_div_clk; } This can be used above: return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, phy_pll_cfg[i].pixclk); And also below in phy_clk_set_rate(): 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]; >> } >> >> 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; >> + u32 int_div_clk, delta_int, delta_frac; >> int i; >> u16 m; >> u8 p, s; >> @@ -602,19 +609,34 @@ static int phy_clk_set_rate(struct clk_hw *hw, >> 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; > > ^ unneeded whitespace (comment belongs to > patch 3/5) > >> + 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]; >> } >> >> + /* Calculate the differences for each clock against the requested value */ >> + delta_frac = (rate - phy_pll_cfg[i].pixclk); >> + delta_int = (rate - int_div_clk); >> + >> + /* Use the value closest to the desired */ >> + if (delta_int < delta_frac) >> + phy->cur_cfg = &calculated_phy_pll_cfg; > > ^ unneeded whitespace > > Are you sure that this is correct? The calculated_phy_pll_cfg is only > set above if there is an exact match for the integer divider. I don't > think it contains the data you expect here, or am I missing something? > >> + else >> + phy->cur_cfg = &phy_pll_cfg[i]; >> +done: >> return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); >> } >>
On Wed, Sep 4, 2024 at 8:54 AM Frieder Schrempf <frieder.schrempf@kontron.de> wrote: > > On 04.09.24 3:40 PM, Frieder Schrempf wrote: > > On 04.09.24 4:32 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> > >> --- > >> V5: No Change > >> V4: New to series > >> --- > >> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 40 +++++++++++++++----- > >> 1 file changed, 31 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > >> index 8f2c0082aa12..56b08e684179 100644 > >> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > >> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > >> @@ -550,7 +550,7 @@ 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; > >> + u32 int_div_clk, delta_int, delta_frac; > >> int i; > >> u16 m; > >> u8 p, s; > >> @@ -563,6 +563,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,15 +580,21 @@ 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; > >> + /* Calculate the differences and use the closest one */ > >> + delta_frac = (rate - phy_pll_cfg[i].pixclk); > >> + delta_int = (rate - int_div_clk); > >> + > >> + if (delta_int < delta_frac) > >> + return int_div_clk; > >> + else > >> + return phy_pll_cfg[i].pixclk; > > I would also prefer to use a helper for calculating the closest rate. > Something like: > > static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate, > u32 int_div_clk, > u32 frac_div_clk) > { > if ((rate - int_div_clk) < (rate - frac_div_clk)) > return int_div_clk; > > return frac_div_clk; > } I like this idea. As Dominique noted, the int_div_clk might be greater than rate, so I'll add abs() here when I simplify this. > > This can be used above: > > return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, > phy_pll_cfg[i].pixclk); > > And also below in phy_clk_set_rate(): > > 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]; > I like this too. adam > > >> } > >> > >> 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; > >> + u32 int_div_clk, delta_int, delta_frac; > >> int i; > >> u16 m; > >> u8 p, s; > >> @@ -602,19 +609,34 @@ static int phy_clk_set_rate(struct clk_hw *hw, > >> 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; > > > > ^ unneeded whitespace (comment belongs to > > patch 3/5) > > > >> + 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]; > >> } > >> > >> + /* Calculate the differences for each clock against the requested value */ > >> + delta_frac = (rate - phy_pll_cfg[i].pixclk); > >> + delta_int = (rate - int_div_clk); > >> + > >> + /* Use the value closest to the desired */ > >> + if (delta_int < delta_frac) > >> + phy->cur_cfg = &calculated_phy_pll_cfg; > > > > ^ unneeded whitespace > > > > Are you sure that this is correct? The calculated_phy_pll_cfg is only > > set above if there is an exact match for the integer divider. I don't > > think it contains the data you expect here, or am I missing something? > > > >> + else > >> + phy->cur_cfg = &phy_pll_cfg[i]; > >> +done: > >> return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg); > >> } > >>
diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c index 8f2c0082aa12..56b08e684179 100644 --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c @@ -550,7 +550,7 @@ 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; + u32 int_div_clk, delta_int, delta_frac; int i; u16 m; u8 p, s; @@ -563,6 +563,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,15 +580,21 @@ 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; + /* Calculate the differences and use the closest one */ + delta_frac = (rate - phy_pll_cfg[i].pixclk); + delta_int = (rate - int_div_clk); + + if (delta_int < delta_frac) + return int_div_clk; + else + 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; + u32 int_div_clk, delta_int, delta_frac; int i; u16 m; u8 p, s; @@ -602,19 +609,34 @@ static int phy_clk_set_rate(struct clk_hw *hw, 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]; } + /* Calculate the differences for each clock against the requested value */ + delta_frac = (rate - phy_pll_cfg[i].pixclk); + delta_int = (rate - int_div_clk); + + /* Use the value closest to the desired */ + if (delta_int < delta_frac) + 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); }