diff mbox

[3/3] ARM: da850: fix da850_set_pll0rate()

Message ID 1480612516-18853-4-git-send-email-bgolaszewski@baylibre.com
State New
Headers show

Commit Message

Bartosz Golaszewski Dec. 1, 2016, 5:15 p.m. UTC
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

Comments

Sekhar Nori Dec. 2, 2016, 11:20 a.m. UTC | #1
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
Bartosz Golaszewski Dec. 2, 2016, 1:09 p.m. UTC | #2
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 mbox

Patch

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;