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

[PATCH v2 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.

v1 -> v2:
- change the approach in 1/3: create a new clock for nand inheriting
  the rate from the aemif clock (verified that nand still works on
  da850-lcdk)
- patch 3/3: also update the davinci_cpufreq driver - the only
  (indirect) user of da850_set_pll0rate()
- s/requested_rate/rate in 3/3

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     | 31 +++++++++++++++++++++++++------
 drivers/cpufreq/davinci-cpufreq.c |  2 +-
 drivers/mtd/nand/davinci_nand.c   |  2 +-
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.9.3

[PATCH v2 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().

Create a separate clock for nand, inheriting the rate of the aemif
clock and retrieve it in the davinci_nand module.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da850.c   | 7 ++++++-
 drivers/mtd/nand/davinci_nand.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index e770c97..1fcc986 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -367,6 +367,11 @@ static struct clk aemif_clk = {
 	.flags		= ALWAYS_ENABLED,
 };
 
+static struct clk nand_clk = {
+	.name		= "nand",
+	.parent		= &aemif_clk,
+};
+
 static struct clk usb11_clk = {
 	.name		= "usb11",
 	.parent		= &pll0_sysclk4,
@@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = {
 	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(NULL,		"nand",		&nand_clk),
 	CLK("ohci-da8xx",	"usb11",	&usb11_clk),
 	CLK("musb-da8xx",	"usb20",	&usb20_clk),
 	CLK("spi_davinci.0",	NULL,		&spi0_clk),
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 27fa8b8..5857d06 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -694,7 +694,7 @@ static int nand_davinci_probe(struct platform_device *pdev)
 
 	ret = -EINVAL;
 
-	info->clk = devm_clk_get(&pdev->dev, "aemif");
+	info->clk = devm_clk_get(&pdev->dev, "nand");
 	if (IS_ERR(info->clk)) {
 		ret = PTR_ERR(info->clk);
 		dev_dbg(&pdev->dev, "unable to get AEMIF clock, err %d\n", ret);
-- 
2.9.3

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

[Sekhar Nori]

On Friday 02 December 2016 09:08 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().
> 
> Create a separate clock for nand, inheriting the rate of the aemif
> clock and retrieve it in the davinci_nand module.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/mach-davinci/da850.c   | 7 ++++++-
>  drivers/mtd/nand/davinci_nand.c | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index e770c97..1fcc986 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -367,6 +367,11 @@ static struct clk aemif_clk = {
>  	.flags		= ALWAYS_ENABLED,
>  };
>  
> +static struct clk nand_clk = {
> +	.name		= "nand",
> +	.parent		= &aemif_clk,
> +};
> +
>  static struct clk usb11_clk = {
>  	.name		= "usb11",
>  	.parent		= &pll0_sysclk4,
> @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = {
>  	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(NULL,		"nand",		&nand_clk),

Why use a NULL device name here?

>  	CLK("ohci-da8xx",	"usb11",	&usb11_clk),
>  	CLK("musb-da8xx",	"usb20",	&usb20_clk),
>  	CLK("spi_davinci.0",	NULL,		&spi0_clk),
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 27fa8b8..5857d06 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -694,7 +694,7 @@ static int nand_davinci_probe(struct platform_device *pdev)
>  
>  	ret = -EINVAL;
>  
> -	info->clk = devm_clk_get(&pdev->dev, "aemif");
> +	info->clk = devm_clk_get(&pdev->dev, "nand");

This driver is used by keystone devices too. So just changing the
connection id will likely break them. Why do you need to change the
connection id?

Thanks,
Sekhar

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

[Bartosz Golaszewski]

2016-12-05 9:38 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Friday 02 December 2016 09:08 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().
>>
>> Create a separate clock for nand, inheriting the rate of the aemif
>> clock and retrieve it in the davinci_nand module.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  arch/arm/mach-davinci/da850.c   | 7 ++++++-
>>  drivers/mtd/nand/davinci_nand.c | 2 +-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>> index e770c97..1fcc986 100644
>> --- a/arch/arm/mach-davinci/da850.c
>> +++ b/arch/arm/mach-davinci/da850.c
>> @@ -367,6 +367,11 @@ static struct clk aemif_clk = {
>>       .flags          = ALWAYS_ENABLED,
>>  };
>>
>> +static struct clk nand_clk = {
>> +     .name           = "nand",
>> +     .parent         = &aemif_clk,
>> +};
>> +
>>  static struct clk usb11_clk = {
>>       .name           = "usb11",
>>       .parent         = &pll0_sysclk4,
>> @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = {
>>       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(NULL,               "nand",         &nand_clk),
>
> Why use a NULL device name here?
>
>>       CLK("ohci-da8xx",       "usb11",        &usb11_clk),
>>       CLK("musb-da8xx",       "usb20",        &usb20_clk),
>>       CLK("spi_davinci.0",    NULL,           &spi0_clk),
>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>> index 27fa8b8..5857d06 100644
>> --- a/drivers/mtd/nand/davinci_nand.c
>> +++ b/drivers/mtd/nand/davinci_nand.c
>> @@ -694,7 +694,7 @@ static int nand_davinci_probe(struct platform_device *pdev)
>>
>>       ret = -EINVAL;
>>
>> -     info->clk = devm_clk_get(&pdev->dev, "aemif");
>> +     info->clk = devm_clk_get(&pdev->dev, "nand");
>
> This driver is used by keystone devices too. So just changing the
> connection id will likely break them. Why do you need to change the
> connection id?
>
> Thanks,
> Sekhar

Thanks, I didn't know it.

I thought it would make the purpose of the clock more obvious. I'll
change it back to "aemif".

Best regards,
Bartosz Golaszewski

[PATCH v2 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 1fcc986..a55101c 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -548,7 +548,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 v2 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.

Also: update the davinci cpufreq driver. It's the only user of this
clock and currently it passes the cpufreq table index to
clk_set_rate(), which is confusing. Make it pass the requested clock
rate in Hz.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da850.c     | 22 ++++++++++++++++++----
 drivers/cpufreq/davinci-cpufreq.c |  2 +-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index a55101c..92e3303 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -1179,14 +1179,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 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 == rate) {
+			opp = (struct da850_opp *)freq->driver_data;
+			break;
+		}
+	}
+
+	if (opp == NULL)
+		return -EINVAL;
+
 	prediv = opp->prediv;
 	mult = opp->mult;
 	postdiv = opp->postdiv;
diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c
index b95a872..d54a27c 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -55,7 +55,7 @@ static int davinci_target(struct cpufreq_policy *policy, unsigned int idx)
 			return ret;
 	}
 
-	ret = clk_set_rate(armclk, idx);
+	ret = clk_set_rate(armclk, new_freq * 1000);
 	if (ret)
 		return ret;
 
-- 
2.9.3

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

[Viresh Kumar]

Good to see you again Bartosz :)

On 02-12-16, 16:38, 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.
> 
> Also: update the davinci cpufreq driver. It's the only user of this
> clock and currently it passes the cpufreq table index to
> clk_set_rate(), which is confusing. Make it pass the requested clock
> rate in Hz.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/mach-davinci/da850.c     | 22 ++++++++++++++++++----
>  drivers/cpufreq/davinci-cpufreq.c |  2 +-
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index a55101c..92e3303 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -1179,14 +1179,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 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 == rate) {
> +			opp = (struct da850_opp *)freq->driver_data;
> +			break;
> +		}
> +	}
> +
> +	if (opp == NULL)
> +		return -EINVAL;
> +
>  	prediv = opp->prediv;
>  	mult = opp->mult;
>  	postdiv = opp->postdiv;
> diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c
> index b95a872..d54a27c 100644
> --- a/drivers/cpufreq/davinci-cpufreq.c
> +++ b/drivers/cpufreq/davinci-cpufreq.c
> @@ -55,7 +55,7 @@ static int davinci_target(struct cpufreq_policy *policy, unsigned int idx)
>  			return ret;
>  	}
>  
> -	ret = clk_set_rate(armclk, idx);
> +	ret = clk_set_rate(armclk, new_freq * 1000);
>  	if (ret)
>  		return ret;

I haven't checked the internal of davinci or the way rate is changed now, but
from cpufreq's perspective, fee free to add my:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

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

[Sekhar Nori]

On Friday 02 December 2016 09:08 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.
> 
> Also: update the davinci cpufreq driver. It's the only user of this
> clock and currently it passes the cpufreq table index to
> clk_set_rate(), which is confusing. Make it pass the requested clock
> rate in Hz.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/mach-davinci/da850.c     | 22 ++++++++++++++++++----
>  drivers/cpufreq/davinci-cpufreq.c |  2 +-
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index a55101c..92e3303 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -1179,14 +1179,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 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;

A small optimization here. Instead of multiplying potentially every
frequency in the table by 1000, you could divide the incoming rate down
to KHz. This will also avoid the need for 'freq_rate'. Should have
noticed this earlier. Sorry about that.

Thanks,
Sekhar

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

[Bartosz Golaszewski]

2016-12-05 9:45 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Friday 02 December 2016 09:08 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.
>>
>> Also: update the davinci cpufreq driver. It's the only user of this
>> clock and currently it passes the cpufreq table index to
>> clk_set_rate(), which is confusing. Make it pass the requested clock
>> rate in Hz.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  arch/arm/mach-davinci/da850.c     | 22 ++++++++++++++++++----
>>  drivers/cpufreq/davinci-cpufreq.c |  2 +-
>>  2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>> index a55101c..92e3303 100644
>> --- a/arch/arm/mach-davinci/da850.c
>> +++ b/arch/arm/mach-davinci/da850.c
>> @@ -1179,14 +1179,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 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;
>
> A small optimization here. Instead of multiplying potentially every
> frequency in the table by 1000, you could divide the incoming rate down
> to KHz. This will also avoid the need for 'freq_rate'. Should have
> noticed this earlier. Sorry about that.
>
> Thanks,
> Sekhar

I thought about it, but figured the multiplication would be safer. I
will change it if you prefer this version.

Best regards,
Bartosz Golaszewski