Message ID | 1573116902-7240-1-git-send-email-rajan.vaja@xilinx.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | clk: zynqmp: Add support for clock with CLK_DIVIDER_POWER_OF_TWO flag | expand |
On Thu, 07 Nov 2019 00:55:02 -0800, Rajan Vaja wrote: > From: Tejas Patel <tejas.patel@xilinx.com> > > Existing clock divider functions is not checking for > base of divider. So, if any clock divider is power of 2 > then clock rate calculation will be wrong. > > Add support to calculate divider value for the clocks > with CLK_DIVIDER_POWER_OF_TWO flag. > > Signed-off-by: Tejas Patel <tejas.patel@xilinx.com> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> > Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com> > --- > drivers/clk/zynqmp/divider.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > index d8f5b70d..ce63cf5 100644 > --- a/drivers/clk/zynqmp/divider.c > +++ b/drivers/clk/zynqmp/divider.c > @@ -2,7 +2,7 @@ > /* > * Zynq UltraScale+ MPSoC Divider support > * > - * Copyright (C) 2016-2018 Xilinx > + * Copyright (C) 2016-2019 Xilinx > * > * Adjustable divider clock implementation > */ > @@ -44,9 +44,26 @@ struct zynqmp_clk_divider { > }; > > static inline int zynqmp_divider_get_val(unsigned long parent_rate, > - unsigned long rate) > + unsigned long rate, u16 flags) > { > - return DIV_ROUND_CLOSEST(parent_rate, rate); > + int up, down; > + unsigned long up_rate, down_rate; > + > + if (flags & CLK_DIVIDER_POWER_OF_TWO) { > + up = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > + down = parent_rate / rate; Maybe use DIV_ROUND_DOWN_ULL()? > + > + up = __roundup_pow_of_two(up); > + down = __rounddown_pow_of_two(down); > + > + up_rate = DIV_ROUND_UP_ULL((u64)parent_rate, up); > + down_rate = DIV_ROUND_UP_ULL((u64)parent_rate, down); > + > + return (rate - up_rate) <= (down_rate - rate) ? up : down; The calculation looks correct. Maybe there could be a common helper with the _div_round_closest() function? > + > + } else { > + return DIV_ROUND_CLOSEST(parent_rate, rate); > + } > } > > /** > @@ -78,6 +95,9 @@ static unsigned long zynqmp_clk_divider_recalc_rate(struct clk_hw *hw, > else > value = div >> 16; > > + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > + value = 1 << value; Not sure, but I think a small helper similar to _get_div() would help with the readability. Just hide the difference between the normal and power of two divisors behind some helper functions. Michael > + > if (!value) { > WARN(!(divider->flags & CLK_DIVIDER_ALLOW_ZERO), > "%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n", > @@ -120,10 +140,13 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, > else > bestdiv = bestdiv >> 16; > > + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > + bestdiv = 1 << bestdiv; > + > return DIV_ROUND_UP_ULL((u64)*prate, bestdiv); > } > > - bestdiv = zynqmp_divider_get_val(*prate, rate); > + bestdiv = zynqmp_divider_get_val(*prate, rate, divider->flags); > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && divider->is_frac) > bestdiv = rate % *prate ? 1 : bestdiv; > @@ -151,7 +174,7 @@ static int zynqmp_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, > int ret; > const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops(); > > - value = zynqmp_divider_get_val(parent_rate, rate); > + value = zynqmp_divider_get_val(parent_rate, rate, divider->flags); > if (div_type == TYPE_DIV1) { > div = value & 0xFFFF; > div |= 0xffff << 16; > @@ -160,6 +183,9 @@ static int zynqmp_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, > div |= value << 16; > } > > + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > + div = __ffs(div); > + > ret = eemi_ops->clock_setdivider(clk_id, div); > > if (ret)
Hi Michael, Thanks for review. > -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: 07 November 2019 11:28 PM > To: Rajan Vaja <RAJANV@xilinx.com> > Cc: mturquette@baylibre.com; sboyd@kernel.org; Michal Simek > <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>; linux-clk@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Tejas Patel > <TEJASP@xilinx.com>; Radhey Shyam Pandey <radheys@xilinx.com>; > kernel@pengutronix.de > Subject: Re: [PATCH] clk: zynqmp: Add support for clock with > CLK_DIVIDER_POWER_OF_TWO flag > > EXTERNAL EMAIL > > On Thu, 07 Nov 2019 00:55:02 -0800, Rajan Vaja wrote: > > From: Tejas Patel <tejas.patel@xilinx.com> > > > > Existing clock divider functions is not checking for > > base of divider. So, if any clock divider is power of 2 > > then clock rate calculation will be wrong. > > > > Add support to calculate divider value for the clocks > > with CLK_DIVIDER_POWER_OF_TWO flag. > > > > Signed-off-by: Tejas Patel <tejas.patel@xilinx.com> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> > > Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com> > > --- > > drivers/clk/zynqmp/divider.c | 36 +++++++++++++++++++++++++++++++----- > > 1 file changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > > index d8f5b70d..ce63cf5 100644 > > --- a/drivers/clk/zynqmp/divider.c > > +++ b/drivers/clk/zynqmp/divider.c > > @@ -2,7 +2,7 @@ > > /* > > * Zynq UltraScale+ MPSoC Divider support > > * > > - * Copyright (C) 2016-2018 Xilinx > > + * Copyright (C) 2016-2019 Xilinx > > * > > * Adjustable divider clock implementation > > */ > > @@ -44,9 +44,26 @@ struct zynqmp_clk_divider { > > }; > > > > static inline int zynqmp_divider_get_val(unsigned long parent_rate, > > - unsigned long rate) > > + unsigned long rate, u16 flags) > > { > > - return DIV_ROUND_CLOSEST(parent_rate, rate); > > + int up, down; > > + unsigned long up_rate, down_rate; > > + > > + if (flags & CLK_DIVIDER_POWER_OF_TWO) { > > + up = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > > + down = parent_rate / rate; > > Maybe use DIV_ROUND_DOWN_ULL()? [Rajan] Ok. Will update in next version . > > > + > > + up = __roundup_pow_of_two(up); > > + down = __rounddown_pow_of_two(down); > > + > > + up_rate = DIV_ROUND_UP_ULL((u64)parent_rate, up); > > + down_rate = DIV_ROUND_UP_ULL((u64)parent_rate, down); > > + > > + return (rate - up_rate) <= (down_rate - rate) ? up : down; > > The calculation looks correct. Maybe there could be a common helper > with the _div_round_closest() function? [Rajan] _div_round_closest() is static function, and yes there is divider_round_rate_parent() which ultimately uses _div_round_closest(), but it requires divider width which is not exposed by firmware to driver. > > > + > > + } else { > > + return DIV_ROUND_CLOSEST(parent_rate, rate); > > + } > > } > > > > /** > > @@ -78,6 +95,9 @@ static unsigned long zynqmp_clk_divider_recalc_rate(struct > clk_hw *hw, > > else > > value = div >> 16; > > > > + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > > + value = 1 << value; > > Not sure, but I think a small helper similar to _get_div() would help > with the readability. Just hide the difference between the normal and > power of two divisors behind some helper functions. [Rajan] _git_dev() requires divider width which is not exposed by firmware to user. So _get_div() can't used here. Also, there is no similar helper available in linux. Correct me if I am missing something. Thanks, Rajan > > Michael > > > + > > if (!value) { > > WARN(!(divider->flags & CLK_DIVIDER_ALLOW_ZERO), > > "%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n", > > @@ -120,10 +140,13 @@ static long zynqmp_clk_divider_round_rate(struct > clk_hw *hw, > > else > > bestdiv = bestdiv >> 16; > > > > + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > > + bestdiv = 1 << bestdiv; > > + > > return DIV_ROUND_UP_ULL((u64)*prate, bestdiv); > > } > > > > - bestdiv = zynqmp_divider_get_val(*prate, rate); > > + bestdiv = zynqmp_divider_get_val(*prate, rate, divider->flags); > > > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && divider->is_frac) > > bestdiv = rate % *prate ? 1 : bestdiv; > > @@ -151,7 +174,7 @@ static int zynqmp_clk_divider_set_rate(struct clk_hw > *hw, unsigned long rate, > > int ret; > > const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops(); > > > > - value = zynqmp_divider_get_val(parent_rate, rate); > > + value = zynqmp_divider_get_val(parent_rate, rate, divider->flags); > > if (div_type == TYPE_DIV1) { > > div = value & 0xFFFF; > > div |= 0xffff << 16; > > @@ -160,6 +183,9 @@ static int zynqmp_clk_divider_set_rate(struct clk_hw > *hw, unsigned long rate, > > div |= value << 16; > > } > > > > + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > > + div = __ffs(div); > > + > > ret = eemi_ops->clock_setdivider(clk_id, div); > > > > if (ret)
diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c index d8f5b70d..ce63cf5 100644 --- a/drivers/clk/zynqmp/divider.c +++ b/drivers/clk/zynqmp/divider.c @@ -2,7 +2,7 @@ /* * Zynq UltraScale+ MPSoC Divider support * - * Copyright (C) 2016-2018 Xilinx + * Copyright (C) 2016-2019 Xilinx * * Adjustable divider clock implementation */ @@ -44,9 +44,26 @@ struct zynqmp_clk_divider { }; static inline int zynqmp_divider_get_val(unsigned long parent_rate, - unsigned long rate) + unsigned long rate, u16 flags) { - return DIV_ROUND_CLOSEST(parent_rate, rate); + int up, down; + unsigned long up_rate, down_rate; + + if (flags & CLK_DIVIDER_POWER_OF_TWO) { + up = DIV_ROUND_UP_ULL((u64)parent_rate, rate); + down = parent_rate / rate; + + up = __roundup_pow_of_two(up); + down = __rounddown_pow_of_two(down); + + up_rate = DIV_ROUND_UP_ULL((u64)parent_rate, up); + down_rate = DIV_ROUND_UP_ULL((u64)parent_rate, down); + + return (rate - up_rate) <= (down_rate - rate) ? up : down; + + } else { + return DIV_ROUND_CLOSEST(parent_rate, rate); + } } /** @@ -78,6 +95,9 @@ static unsigned long zynqmp_clk_divider_recalc_rate(struct clk_hw *hw, else value = div >> 16; + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) + value = 1 << value; + if (!value) { WARN(!(divider->flags & CLK_DIVIDER_ALLOW_ZERO), "%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n", @@ -120,10 +140,13 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, else bestdiv = bestdiv >> 16; + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) + bestdiv = 1 << bestdiv; + return DIV_ROUND_UP_ULL((u64)*prate, bestdiv); } - bestdiv = zynqmp_divider_get_val(*prate, rate); + bestdiv = zynqmp_divider_get_val(*prate, rate, divider->flags); if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && divider->is_frac) bestdiv = rate % *prate ? 1 : bestdiv; @@ -151,7 +174,7 @@ static int zynqmp_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, int ret; const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops(); - value = zynqmp_divider_get_val(parent_rate, rate); + value = zynqmp_divider_get_val(parent_rate, rate, divider->flags); if (div_type == TYPE_DIV1) { div = value & 0xFFFF; div |= 0xffff << 16; @@ -160,6 +183,9 @@ static int zynqmp_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, div |= value << 16; } + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) + div = __ffs(div); + ret = eemi_ops->clock_setdivider(clk_id, div); if (ret)