| Message ID | 20230912045157.177966-10-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK | expand |
Hi, Geert, On 14.09.2023 15:55, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) >> the computation formula for PLL rate is as follows: >> >> Fout = ((m + k/65536) * Fin) / (p * 2^s) >> >> and k has values in range [-32768, 32767]. Dividing k by 65536 with >> integer variables leads all the time to zero. Thus we may have slight >> differences b/w what has been set vs. what is displayed. Thus, >> get rid of this and decompose the formula before dividing k by 65536. >> >> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, >> struct pll_clk *pll_clk = to_pll(hw); >> struct rzg2l_cpg_priv *priv = pll_clk->priv; >> unsigned int val1, val2; >> - unsigned int mult = 1; >> - unsigned int div = 1; >> + unsigned int div; >> + u64 rate; >> + s16 kdiv; >> >> if (pll_clk->type != CLK_TYPE_SAM_PLL) >> return parent_rate; >> >> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); >> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); >> - mult = MDIV(val1) + KDIV(val1) / 65536; >> + kdiv = KDIV(val1); >> div = PDIV(val1) << SDIV(val2); >> >> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); >> + rate = (u64)MDIV(val1) * parent_rate; >> + rate += ((long long)parent_rate * kdiv) / 65536; > > As the division is a binary shift, you can use the mul_u64_u32_shr() helper, > and incorporate the sdiv shift at the same time: > > rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); > > You can save a multiplication by premultiplying mdiv by 65536: > > rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), > 16 + SDIV(val2)); Looking again at this: KDIV (aka DIV_K) could have negative values thus mul_u64_u32_shr() cannot be used here. > >> + >> + return DIV_ROUND_CLOSEST_ULL(rate, div); > > return DIV_ROUND_CLOSEST_ULL(rate, PDIV(val1)); > >> } >> >> static const struct clk_ops rzg2l_cpg_pll_ops = { > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Tue, Sep 26, 2023 at 1:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 14.09.2023 15:55, Geert Uytterhoeven wrote: > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) > >> the computation formula for PLL rate is as follows: > >> > >> Fout = ((m + k/65536) * Fin) / (p * 2^s) > >> > >> and k has values in range [-32768, 32767]. Dividing k by 65536 with > >> integer variables leads all the time to zero. Thus we may have slight > >> differences b/w what has been set vs. what is displayed. Thus, > >> get rid of this and decompose the formula before dividing k by 65536. > >> > >> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- a/drivers/clk/renesas/rzg2l-cpg.c > >> +++ b/drivers/clk/renesas/rzg2l-cpg.c > >> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, > >> struct pll_clk *pll_clk = to_pll(hw); > >> struct rzg2l_cpg_priv *priv = pll_clk->priv; > >> unsigned int val1, val2; > >> - unsigned int mult = 1; > >> - unsigned int div = 1; > >> + unsigned int div; > >> + u64 rate; > >> + s16 kdiv; > >> > >> if (pll_clk->type != CLK_TYPE_SAM_PLL) > >> return parent_rate; > >> > >> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); > >> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); > >> - mult = MDIV(val1) + KDIV(val1) / 65536; > >> + kdiv = KDIV(val1); > >> div = PDIV(val1) << SDIV(val2); > >> > >> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); > >> + rate = (u64)MDIV(val1) * parent_rate; > >> + rate += ((long long)parent_rate * kdiv) / 65536; > > > > As the division is a binary shift, you can use the mul_u64_u32_shr() helper, > > and incorporate the sdiv shift at the same time: > > > > rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); [1]^ > > > > You can save a multiplication by premultiplying mdiv by 65536: > > > > rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), > > 16 + SDIV(val2)); [2]^ > > Looking again at this: KDIV (aka DIV_K) could have negative values thus > mul_u64_u32_shr() cannot be used here. That means you can indeed not use [1]. But you can still use [2], as MDIV() must be in the range 64..533[3], so "(MDIV(val1) << 16)) + (s16)KDIV(val1)" is always positive. Note that you do need the cast to s16 (which I had missed before), or the intermediate variable kdiv of type s16 (like in your patch). [3] As the current PLL driver is read-only, there is no calculation or validation of the PLL parameters. > >> + > >> + return DIV_ROUND_CLOSEST_ULL(rate, div); > > > > return DIV_ROUND_CLOSEST_ULL(rate, PDIV(val1)); Gr{oetje,eeting}s, Geert
Hi Claudiu, On Tue, Sep 26, 2023 at 4:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Sep 26, 2023 at 1:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > > On 14.09.2023 15:55, Geert Uytterhoeven wrote: > > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >> > > >> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) > > >> the computation formula for PLL rate is as follows: > > >> > > >> Fout = ((m + k/65536) * Fin) / (p * 2^s) > > >> > > >> and k has values in range [-32768, 32767]. Dividing k by 65536 with > > >> integer variables leads all the time to zero. Thus we may have slight > > >> differences b/w what has been set vs. what is displayed. Thus, > > >> get rid of this and decompose the formula before dividing k by 65536. > > >> > > >> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > >> --- a/drivers/clk/renesas/rzg2l-cpg.c > > >> +++ b/drivers/clk/renesas/rzg2l-cpg.c > > >> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, > > >> struct pll_clk *pll_clk = to_pll(hw); > > >> struct rzg2l_cpg_priv *priv = pll_clk->priv; > > >> unsigned int val1, val2; > > >> - unsigned int mult = 1; > > >> - unsigned int div = 1; > > >> + unsigned int div; > > >> + u64 rate; > > >> + s16 kdiv; > > >> > > >> if (pll_clk->type != CLK_TYPE_SAM_PLL) > > >> return parent_rate; > > >> > > >> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); > > >> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); > > >> - mult = MDIV(val1) + KDIV(val1) / 65536; > > >> + kdiv = KDIV(val1); > > >> div = PDIV(val1) << SDIV(val2); > > >> > > >> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); > > >> + rate = (u64)MDIV(val1) * parent_rate; > > >> + rate += ((long long)parent_rate * kdiv) / 65536; > > > > > > As the division is a binary shift, you can use the mul_u64_u32_shr() helper, > > > and incorporate the sdiv shift at the same time: > > > > > > rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); > > [1]^ > > > > > > > You can save a multiplication by premultiplying mdiv by 65536: > > > > > > rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), > > > 16 + SDIV(val2)); > > [2]^ > > > > > Looking again at this: KDIV (aka DIV_K) could have negative values thus > > mul_u64_u32_shr() cannot be used here. > > That means you can indeed not use [1]. > > But you can still use [2], as MDIV() must be in the range 64..533[3], > so "(MDIV(val1) << 16)) + (s16)KDIV(val1)" is always positive. > Note that you do need the cast to s16 (which I had missed before), or > the intermediate variable kdiv of type s16 (like in your patch). Or include the cast to a signed type in the definition of KDIV(). Gr{oetje,eeting}s, Geert
Hi, Geert, On 27.09.2023 11:00, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 26, 2023 at 4:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Tue, Sep 26, 2023 at 1:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: >>> On 14.09.2023 15:55, Geert Uytterhoeven wrote: >>>> On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>> >>>>> According to hardware manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf) >>>>> the computation formula for PLL rate is as follows: >>>>> >>>>> Fout = ((m + k/65536) * Fin) / (p * 2^s) >>>>> >>>>> and k has values in range [-32768, 32767]. Dividing k by 65536 with >>>>> integer variables leads all the time to zero. Thus we may have slight >>>>> differences b/w what has been set vs. what is displayed. Thus, >>>>> get rid of this and decompose the formula before dividing k by 65536. >>>>> >>>>> Fixes: ef3c613ccd68a ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC") >>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> Thanks for your patch! >>>> >>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c >>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c >>>>> @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, >>>>> struct pll_clk *pll_clk = to_pll(hw); >>>>> struct rzg2l_cpg_priv *priv = pll_clk->priv; >>>>> unsigned int val1, val2; >>>>> - unsigned int mult = 1; >>>>> - unsigned int div = 1; >>>>> + unsigned int div; >>>>> + u64 rate; >>>>> + s16 kdiv; >>>>> >>>>> if (pll_clk->type != CLK_TYPE_SAM_PLL) >>>>> return parent_rate; >>>>> >>>>> val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); >>>>> val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); >>>>> - mult = MDIV(val1) + KDIV(val1) / 65536; >>>>> + kdiv = KDIV(val1); >>>>> div = PDIV(val1) << SDIV(val2); >>>>> >>>>> - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); >>>>> + rate = (u64)MDIV(val1) * parent_rate; >>>>> + rate += ((long long)parent_rate * kdiv) / 65536; >>>> >>>> As the division is a binary shift, you can use the mul_u64_u32_shr() helper, >>>> and incorporate the sdiv shift at the same time: >>>> >>>> rate += mul_u64_u32_shr(parent_rate, KDIV(val1), 16 + SDIV(val2)); >> >> [1]^ >> >>>> >>>> You can save a multiplication by premultiplying mdiv by 65536: >>>> >>>> rate = mul_u64_u32_shr(parent_rate, (MDIV(val1) << 16)) + KDIV(val1), >>>> 16 + SDIV(val2)); >> >> [2]^ >> >>> >>> Looking again at this: KDIV (aka DIV_K) could have negative values thus >>> mul_u64_u32_shr() cannot be used here. >> >> That means you can indeed not use [1]. You're right. Thanks for the input! >> >> But you can still use [2], as MDIV() must be in the range 64..533[3], >> so "(MDIV(val1) << 16)) + (s16)KDIV(val1)" is always positive. >> Note that you do need the cast to s16 (which I had missed before), or >> the intermediate variable kdiv of type s16 (like in your patch). > > Or include the cast to a signed type in the definition of KDIV(). > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c index d0d086d6dc51..b391c9548421 100644 --- a/drivers/clk/renesas/rzg2l-cpg.c +++ b/drivers/clk/renesas/rzg2l-cpg.c @@ -696,18 +696,22 @@ static unsigned long rzg2l_cpg_pll_clk_recalc_rate(struct clk_hw *hw, struct pll_clk *pll_clk = to_pll(hw); struct rzg2l_cpg_priv *priv = pll_clk->priv; unsigned int val1, val2; - unsigned int mult = 1; - unsigned int div = 1; + unsigned int div; + u64 rate; + s16 kdiv; if (pll_clk->type != CLK_TYPE_SAM_PLL) return parent_rate; val1 = readl(priv->base + GET_REG_SAMPLL_CLK1(pll_clk->conf)); val2 = readl(priv->base + GET_REG_SAMPLL_CLK2(pll_clk->conf)); - mult = MDIV(val1) + KDIV(val1) / 65536; + kdiv = KDIV(val1); div = PDIV(val1) << SDIV(val2); - return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult, div); + rate = (u64)MDIV(val1) * parent_rate; + rate += ((long long)parent_rate * kdiv) / 65536; + + return DIV_ROUND_CLOSEST_ULL(rate, div); } static const struct clk_ops rzg2l_cpg_pll_ops = {