[PATCH 0/3] ARM: da850: fix pll0 rate setting

[PATCH 0/3] ARM: da850: fix pll0 rate setting

[Bartosz Golaszewski]

While trying to set the pll0 rate from the kernel I noticed there are
two issues with da850 clocks. The first patch fixes an infinite loop
in propagate_rate(). The third fixes an oops in da850_set_pll0rate().
The second patch is just a coding style fix, while we're at it.

Bartosz Golaszewski (3):
  ARM: da850: fix infinite loop in clk_set_rate()
  ARM: da850: coding style fix
  ARM: da850: fix da850_set_pll0rate()

 arch/arm/mach-davinci/da850.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
2.9.3

[PATCH 1/3] ARM: da850: fix infinite loop in clk_set_rate()

[Bartosz Golaszewski]

The aemif clock is added twice to the lookup table in da850.c. This
breaks the children list of pll0_sysclk3 as we're using the same list
links in struct clk. When calling clk_set_rate(), we get stuck in
propagate_rate().

Simply add the clock once, but specify both the con_id and dev_id in
the lookup entry.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da850.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index e770c97..1e11ce8 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -536,8 +536,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK("da8xx_lcdc.0",	"fck",		&lcdc_clk),
 	CLK("da830-mmc.0",	NULL,		&mmcsd0_clk),
 	CLK("da830-mmc.1",	NULL,		&mmcsd1_clk),
-	CLK("ti-aemif",		NULL,		&aemif_clk),
-	CLK(NULL,		"aemif",	&aemif_clk),
+	CLK("ti-aemif",		"aemif",	&aemif_clk),
 	CLK("ohci-da8xx",	"usb11",	&usb11_clk),
 	CLK("musb-da8xx",	"usb20",	&usb20_clk),
 	CLK("spi_davinci.0",	NULL,		&spi0_clk),
-- 
2.9.3

[PATCH 2/3] ARM: da850: coding style fix

[Bartosz Golaszewski]

Fix alignment of the clock lookup table entries.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da850.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 1e11ce8..855b720 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -542,7 +542,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK("spi_davinci.0",	NULL,		&spi0_clk),
 	CLK("spi_davinci.1",	NULL,		&spi1_clk),
 	CLK("vpif",		NULL,		&vpif_clk),
-	CLK("ahci_da850",		NULL,		&sata_clk),
+	CLK("ahci_da850",	NULL,		&sata_clk),
 	CLK("davinci-rproc.0",	NULL,		&dsp_clk),
 	CLK(NULL,		NULL,		&ehrpwm_clk),
 	CLK("ehrpwm.0",		"fck",		&ehrpwm0_clk),
-- 
2.9.3

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

[Bartosz Golaszewski]

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(-)

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;
-- 
2.9.3

Re: [PATCH 1/3] ARM: da850: fix infinite loop in clk_set_rate()

[Sekhar Nori]

Hi Bartosz,

On Thursday 01 December 2016 10:45 PM, Bartosz Golaszewski wrote:
> The aemif clock is added twice to the lookup table in da850.c. This
> breaks the children list of pll0_sysclk3 as we're using the same list
> links in struct clk. When calling clk_set_rate(), we get stuck in
> propagate_rate().
> 
> Simply add the clock once, but specify both the con_id and dev_id in
> the lookup entry.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The issue is real, but the fix is not going to be this simple, I am
afraid. This will break NAND on all da850 boards including LCDK.

The aemif clock is accessed in two ways. One by the
drivers/memory/ti-aemif.c, using ti-aemif as the device name and NULL
connection id. Second by drivers/mtd/nand/davinci_nand.c and
arch/arm/mach-davinci/aemif.c using davinci-nand as device id and with
"aemif" as the connection id.

We will need to match both. The only way to fix this without breaking
anything is to create two clocks for the two lookups above. Both cannot
be PSC clocks for the same PSC module as that would be racy. Instead
just create a new nand clock node which is a child of the aemif node and
inherits parent's clock rate.

Thanks,
Sekhar

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

[Sekhar Nori]

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

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

[Bartosz Golaszewski]

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