| Message ID | 1463402840-17062-3-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mike, Stephen, On Mon, May 16, 2016 at 02:47:02PM +0200, Maxime Ripard wrote: > In the current multiplier base clock implementation, if the > CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the > multiplier computed remains within the boundaries of our clock. > > This means that if the clock we want to reach is below the parent rate, > or if the multiplier is above the maximum that we can reach, we will end up > with a completely bogus one that the clock cannot achieve. > > Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Any comments? Thanks! Maxime
Quoting Maxime Ripard (2016-05-16 05:47:02) > In the current multiplier base clock implementation, if the > CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the > multiplier computed remains within the boundaries of our clock. > > This means that if the clock we want to reach is below the parent rate, > or if the multiplier is above the maximum that we can reach, we will end up > with a completely bogus one that the clock cannot achieve. > > Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Applied. Regards, Mike > --- > drivers/clk/clk-multiplier.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-multiplier.c b/drivers/clk/clk-multiplier.c > index 9e449c7b751c..dc037c957acd 100644 > --- a/drivers/clk/clk-multiplier.c > +++ b/drivers/clk/clk-multiplier.c > @@ -52,14 +52,28 @@ static unsigned long __bestmult(struct clk_hw *hw, unsigned long rate, > unsigned long *best_parent_rate, > u8 width, unsigned long flags) > { > + struct clk_multiplier *mult = to_clk_multiplier(hw); > unsigned long orig_parent_rate = *best_parent_rate; > unsigned long parent_rate, current_rate, best_rate = ~0; > unsigned int i, bestmult = 0; > + unsigned int maxmult = (1 << width) - 1; > + > + if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) { > + bestmult = rate / orig_parent_rate; > + > + /* Make sure we don't end up with a 0 multiplier */ > + if ((bestmult == 0) && > + !(mult->flags & CLK_MULTIPLIER_ZERO_BYPASS)) > + bestmult = 1; > > - if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) > - return rate / *best_parent_rate; > + /* Make sure we don't overflow the multiplier */ > + if (bestmult > maxmult) > + bestmult = maxmult; > + > + return bestmult; > + } > > - for (i = 1; i < ((1 << width) - 1); i++) { > + for (i = 1; i < maxmult; i++) { > if (rate == orig_parent_rate * i) { > /* > * This is the best case for us if we have a > -- > 2.8.2 >
On Mon, Jun 20, 2016 at 01:50:30PM -0700, Michael Turquette wrote: > Quoting Maxime Ripard (2016-05-16 05:47:02) > > In the current multiplier base clock implementation, if the > > CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the > > multiplier computed remains within the boundaries of our clock. > > > > This means that if the clock we want to reach is below the parent rate, > > or if the multiplier is above the maximum that we can reach, we will end up > > with a completely bogus one that the clock cannot achieve. > > > > Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > Applied. Thanks, but apparently you merged it in clk-next, while it should go in clk-fixes, shouldn't it? Maxime
diff --git a/drivers/clk/clk-multiplier.c b/drivers/clk/clk-multiplier.c index 9e449c7b751c..dc037c957acd 100644 --- a/drivers/clk/clk-multiplier.c +++ b/drivers/clk/clk-multiplier.c @@ -52,14 +52,28 @@ static unsigned long __bestmult(struct clk_hw *hw, unsigned long rate, unsigned long *best_parent_rate, u8 width, unsigned long flags) { + struct clk_multiplier *mult = to_clk_multiplier(hw); unsigned long orig_parent_rate = *best_parent_rate; unsigned long parent_rate, current_rate, best_rate = ~0; unsigned int i, bestmult = 0; + unsigned int maxmult = (1 << width) - 1; + + if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) { + bestmult = rate / orig_parent_rate; + + /* Make sure we don't end up with a 0 multiplier */ + if ((bestmult == 0) && + !(mult->flags & CLK_MULTIPLIER_ZERO_BYPASS)) + bestmult = 1; - if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) - return rate / *best_parent_rate; + /* Make sure we don't overflow the multiplier */ + if (bestmult > maxmult) + bestmult = maxmult; + + return bestmult; + } - for (i = 1; i < ((1 << width) - 1); i++) { + for (i = 1; i < maxmult; i++) { if (rate == orig_parent_rate * i) { /* * This is the best case for us if we have a
In the current multiplier base clock implementation, if the CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the multiplier computed remains within the boundaries of our clock. This means that if the clock we want to reach is below the parent rate, or if the multiplier is above the maximum that we can reach, we will end up with a completely bogus one that the clock cannot achieve. Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/clk/clk-multiplier.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)