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

[PATCH v3 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

v2 -> v3:
- 1/3: keep the "aemif" connector id for the nand clock
- 3/3: instead of multiplying freq->frequency, divide rate by 1000
- retested both davinci_nand and clk_set_rate() for pll0

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     | 29 +++++++++++++++++++++++------
 drivers/cpufreq/davinci-cpufreq.c |  2 +-
 2 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.9.3

[PATCH v3 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 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index e770c97..c008e5e 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 aemif_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,		"aemif",	&aemif_nand_clk),
 	CLK("ohci-da8xx",	"usb11",	&usb11_clk),
 	CLK("musb-da8xx",	"usb20",	&usb20_clk),
 	CLK("spi_davinci.0",	NULL,		&spi0_clk),
-- 
2.9.3

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

[Sekhar Nori]

On Monday 05 December 2016 03:39 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 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index e770c97..c008e5e 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 aemif_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,		"aemif",	&aemif_nand_clk),

Why use a NULL device name here? Same question was asked on v2
submission. Also, can you please make sure you are testing this in both
DT mode (da850-lcdk) and non-DT boot (da850-evm).

Thanks,
Sekhar

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

[Bartosz Golaszewski]

2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 05 December 2016 03:39 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 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>> index e770c97..c008e5e 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 aemif_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,               "aemif",        &aemif_nand_clk),
>
> Why use a NULL device name here? Same question was asked on v2

Eek, sorry, I missed that.

> submission. Also, can you please make sure you are testing this in both
> DT mode (da850-lcdk) and non-DT boot (da850-evm).

Will do.

Thanks,
Bartosz

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

[Sekhar Nori]

On Monday 05 December 2016 04:02 PM, Bartosz Golaszewski wrote:
> 2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Monday 05 December 2016 03:39 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 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>>> index e770c97..c008e5e 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 aemif_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,               "aemif",        &aemif_nand_clk),
>>
>> Why use a NULL device name here? Same question was asked on v2
> 
> Eek, sorry, I missed that.

For next version, can you also add a comment on top of 'struct clk
aemif_nand_clk' explaining why its needed?

Thanks,
Sekhar

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

[Bartosz Golaszewski]

2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 05 December 2016 03:39 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 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>> index e770c97..c008e5e 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 aemif_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,               "aemif",        &aemif_nand_clk),
>
> Why use a NULL device name here?

Hi Sekhar,

there's an issue with this bit. I added an of_dev_auxdata entry to
da8xx-dt.c for the nand node, but it didn't work (the nand driver
could not get the clock). When I dug deeper, it turned out, the nand
node is created from aemif_probe() instead of from
da850_init_machine() and the lookup table is not passed as argument to
of_platform_populate().

There are two solutions: one is using "620000000.nand" as dev_id in
the clock lookup table, but that's ugly. The second is leaving dev_id
as NULL - I verified that the nand driver works correctly having only
the connector id. Please let me know which one you prefer or if you
have other ideas.

Best regards,
Bartosz Golaszewski

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

[Sekhar Nori]

On Tuesday 06 December 2016 05:28 PM, Bartosz Golaszewski wrote:
> 2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Monday 05 December 2016 03:39 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 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>>> index e770c97..c008e5e 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 aemif_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,               "aemif",        &aemif_nand_clk),
>>
>> Why use a NULL device name here?
> 
> Hi Sekhar,
> 
> there's an issue with this bit. I added an of_dev_auxdata entry to
> da8xx-dt.c for the nand node, but it didn't work (the nand driver
> could not get the clock). When I dug deeper, it turned out, the nand
> node is created from aemif_probe() instead of from
> da850_init_machine() and the lookup table is not passed as argument to
> of_platform_populate().
> 
> There are two solutions: one is using "620000000.nand" as dev_id in
> the clock lookup table, but that's ugly. The second is leaving dev_id
> as NULL - I verified that the nand driver works correctly having only
> the connector id. Please let me know which one you prefer or if you
> have other ideas.

Alright, I will take a look at whats going on here. This series will
have to wait for v4.11 anyway.

Thanks,
Sekhar

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

[David Lechner]

On 12/05/2016 04:09 AM, 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().

&emac_clk is used twice in this list as well. Shouldn't we fix it too? I 
would expect that it causes the same problem.

>
> 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 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index e770c97..c008e5e 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 aemif_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,		"aemif",	&aemif_nand_clk),
>  	CLK("ohci-da8xx",	"usb11",	&usb11_clk),
>  	CLK("musb-da8xx",	"usb20",	&usb20_clk),
>  	CLK("spi_davinci.0",	NULL,		&spi0_clk),
>

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

[Sekhar Nori]

On Wednesday 07 December 2016 07:24 AM, David Lechner wrote:
> On 12/05/2016 04:09 AM, 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().
> 
> &emac_clk is used twice in this list as well. Shouldn't we fix it too? I
> would expect that it causes the same problem.

Yes, indeed.

Thanks,
Sekhar

[PATCH v3 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 c008e5e..006ec56 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 v3 3/3] ARM: da850: fix da850_set_pll0rate()

[Bartosz Golaszewski]

This function is confusing - its second argument is an index to the
freq table, not the requested clock rate in Hz, but it's used as the
set_rate callback for the pll0 clock. It leads to an oops when the
caller doesn't know the internals and passes the rate in Hz as
argument instead of the cpufreq index 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>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm/mach-davinci/da850.c     | 20 ++++++++++++++++----
 drivers/cpufreq/davinci-cpufreq.c |  2 +-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 006ec56..9837541 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -1179,14 +1179,26 @@ 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++) {
+		/* rate is in Hz, freq->frequency is in KHz */
+		if (freq->frequency == rate / 1000) {
+			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 v3 3/3] ARM: da850: fix da850_set_pll0rate()

[Sekhar Nori]

On Monday 05 December 2016 03:39 PM, Bartosz Golaszewski wrote:
> This function is confusing - its second argument is an index to the
> freq table, not the requested clock rate in Hz, but it's used as the
> set_rate callback for the pll0 clock. It leads to an oops when the
> caller doesn't know the internals and passes the rate in Hz as
> argument instead of the cpufreq index 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>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  arch/arm/mach-davinci/da850.c     | 20 ++++++++++++++++----
>  drivers/cpufreq/davinci-cpufreq.c |  2 +-
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 006ec56..9837541 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -1179,14 +1179,26 @@ 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++) {
> +		/* rate is in Hz, freq->frequency is in KHz */
> +		if (freq->frequency == rate / 1000) {

Or
	rate = rate / 1000;

before the loop begins? The idea behind my comment was mostly to reduce
the amount of calculation in the loop.

Thanks,
Sekhar

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

[David Lechner]

On 12/05/2016 04:09 AM, Bartosz Golaszewski wrote:
> This function is confusing - its second argument is an index to the
> freq table, not the requested clock rate in Hz, but it's used as the
> set_rate callback for the pll0 clock. It leads to an oops when the
> caller doesn't know the internals and passes the rate in Hz as
> argument instead of the cpufreq index 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>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  arch/arm/mach-davinci/da850.c     | 20 ++++++++++++++++----
>  drivers/cpufreq/davinci-cpufreq.c |  2 +-
>  2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 006ec56..9837541 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -1179,14 +1179,26 @@ 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++) {
> +		/* rate is in Hz, freq->frequency is in KHz */
> +		if (freq->frequency == rate / 1000) {
> +			opp = (struct da850_opp *)freq->driver_data;
> +			break;
> +		}
> +	}
> +
> +	if (opp == NULL)

Would be simpler to write:

	if (!opp)

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