| Message ID | 1480612516-18853-4-git-send-email-bgolaszewski@baylibre.com |
|---|---|
| State | New |
| Headers | show |
On Thursday 01 December 2016 10:45 PM, Bartosz Golaszewski wrote: > This function is broken - its second argument is an index to the freq > table, not the requested clock rate in Hz. It leads to an oops when > called from clk_set_rate() since this argument isn't bounds checked > either. > > Fix it by iterating over the array of supported frequencies and > selecting a one that matches or returning -EINVAL for unsupported > rates. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> When this function was written, it was written for speed. The only user of setting pll0 rate is drivers/cpufreq/davinci-cpufreq.c (not sure how you were trying to set pll0 rate). And that driver directly passes the table index to the set_rate() function. The idea was to optimize for speed in cpufreq driver and quickly index into the pll data instead of searching through it. But I agree, it is confusing and we are better off with maintainable code. So, no problem with the patch, but this needs to be done along with updates to cpufreq driver. > --- > arch/arm/mach-davinci/da850.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c > index 855b720..1c0f296 100644 > --- a/arch/arm/mach-davinci/da850.c > +++ b/arch/arm/mach-davinci/da850.c > @@ -1173,14 +1173,28 @@ static int da850_set_armrate(struct clk *clk, unsigned long index) > return clk_set_rate(pllclk, index); > } > > -static int da850_set_pll0rate(struct clk *clk, unsigned long index) > +static int da850_set_pll0rate(struct clk *clk, unsigned long requested_rate) Calling it just 'rate' is fine, IMO. The name of the function is enough to suggest that its the rate to be set to. Thanks, Sekhar
2016-12-02 12:20 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: > On Thursday 01 December 2016 10:45 PM, Bartosz Golaszewski wrote: >> This function is broken - its second argument is an index to the freq >> table, not the requested clock rate in Hz. It leads to an oops when >> called from clk_set_rate() since this argument isn't bounds checked >> either. >> >> Fix it by iterating over the array of supported frequencies and >> selecting a one that matches or returning -EINVAL for unsupported >> rates. >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > When this function was written, it was written for speed. The only user > of setting pll0 rate is drivers/cpufreq/davinci-cpufreq.c (not sure how > you were trying to set pll0 rate). And that driver directly passes the > table index to the set_rate() function. > Hi Sekhar, thanks for the hints. The origin of this series is the default pll0 frequency set by upstream u-boot which caused FIFO underflows in LCDC even with the pixel clock well below 37.5 MHz. I had already sent a patch to the u-boot mailing list, but thought I'd try setting the clock from within tilcdc code. This is when I stumbled upon this issue. I'll send a v2 of this series. Thanks, Bartosz Golaszewski
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c index 855b720..1c0f296 100644 --- a/arch/arm/mach-davinci/da850.c +++ b/arch/arm/mach-davinci/da850.c @@ -1173,14 +1173,28 @@ static int da850_set_armrate(struct clk *clk, unsigned long index) return clk_set_rate(pllclk, index); } -static int da850_set_pll0rate(struct clk *clk, unsigned long index) +static int da850_set_pll0rate(struct clk *clk, unsigned long requested_rate) { - unsigned int prediv, mult, postdiv; - struct da850_opp *opp; struct pll_data *pll = clk->pll_data; + struct cpufreq_frequency_table *freq; + unsigned int prediv, mult, postdiv; + struct da850_opp *opp = NULL; int ret; - opp = (struct da850_opp *) cpufreq_info.freq_table[index].driver_data; + for (freq = da850_freq_table; + freq->frequency != CPUFREQ_TABLE_END; freq++) { + /* requested_rate is in Hz, freq->frequency is in KHz */ + unsigned long freq_rate = freq->frequency * 1000; + + if (freq_rate == requested_rate) { + opp = (struct da850_opp *)freq->driver_data; + break; + } + } + + if (opp == NULL) + return -EINVAL; + prediv = opp->prediv; mult = opp->mult; postdiv = opp->postdiv;
This function is broken - its second argument is an index to the freq table, not the requested clock rate in Hz. It leads to an oops when called from clk_set_rate() since this argument isn't bounds checked either. Fix it by iterating over the array of supported frequencies and selecting a one that matches or returning -EINVAL for unsupported rates. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- arch/arm/mach-davinci/da850.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) -- 2.9.3