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

[PATCH v4 0/4] 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

v3 -> v4:
- 1/4: add a comment explaining why we're leaving a NULL dev_id
- 2/4: emac_clk is added twice to the clock lookup table too - fix it
  and verify that davinci_mmdio gets a functional clock
- 4/4: s/opp == NULL/!opp/

Bartosz Golaszewski (4):
  ARM: da850: fix infinite loop in clk_set_rate()
  ARM: da850: don't add the emac clock to the clock lookup table twice
  ARM: da850: coding style fix
  ARM: da850: fix da850_set_pll0rate()

 arch/arm/mach-davinci/da850.c     | 51 +++++++++++++++++++++++++++++++++------
 drivers/cpufreq/davinci-cpufreq.c |  2 +-
 2 files changed, 45 insertions(+), 8 deletions(-)

-- 
2.9.3

[PATCH v4 1/4] 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 | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index e770c97..e9d019c 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -367,6 +367,16 @@ static struct clk aemif_clk = {
 	.flags		= ALWAYS_ENABLED,
 };
 
+/*
+ * In order to avoid adding the aemif_clk to the clock lookup table twice (and
+ * screwing up the linked list in the process) create a separate clock for
+ * nand inheriting the rate from aemif_clk.
+ */
+static struct clk aemif_nand_clk = {
+	.name		= "nand",
+	.parent		= &aemif_clk,
+};
+
 static struct clk usb11_clk = {
 	.name		= "usb11",
 	.parent		= &pll0_sysclk4,
@@ -537,7 +547,15 @@ 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),
+	/*
+	 * The only user of this clock is davinci_nand and it get's it through
+	 * con_id. The nand node itself is created from within the aemif
+	 * driver to guarantee that it's probed after the aemif timing
+	 * parameters are configured. of_dev_auxdata is not accessible from
+	 * the aemif driver and can't be passed to of_platform_populate(). For
+	 * that reason we're leaving the dev_id here as NULL.
+	 */
+	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

[PATCH v4 2/4] ARM: da850: don't add the emac clock to the clock lookup table twice

[Bartosz Golaszewski]

Similarly to the aemif clock - this screws up the linked list of clock
children. Create a separate clock for mdio inheriting the rate from
emac_clk.

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 e9d019c..6b1fbac 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -319,6 +319,11 @@ static struct clk emac_clk = {
 	.gpsc		= 1,
 };
 
+static struct clk mdio_clk = {
+	.name		= "mdio",
+	.parent		= &emac_clk,
+};
+
 static struct clk mcasp_clk = {
 	.name		= "mcasp",
 	.parent		= &async3_clk,
@@ -539,7 +544,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK(NULL,		"arm",		&arm_clk),
 	CLK(NULL,		"rmii",		&rmii_clk),
 	CLK("davinci_emac.1",	NULL,		&emac_clk),
-	CLK("davinci_mdio.0",	"fck",		&emac_clk),
+	CLK("davinci_mdio.0",	"fck",		&mdio_clk),
 	CLK("davinci-mcasp.0",	NULL,		&mcasp_clk),
 	CLK("davinci-mcbsp.0",	NULL,		&mcbsp0_clk),
 	CLK("davinci-mcbsp.1",	NULL,		&mcbsp1_clk),
-- 
2.9.3

[PATCH v4 3/4] 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 6b1fbac..5f8ffaa 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -566,7 +566,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 v4 4/4] 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     | 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 5f8ffaa..b410598 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -1197,14 +1197,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;
+	rate /= 1000;
+
+	for (freq = da850_freq_table;
+	     freq->frequency != CPUFREQ_TABLE_END; freq++) {
+		/* rate is in Hz, freq->frequency is in KHz */
+		if (freq->frequency == rate) {
+			opp = (struct da850_opp *)freq->driver_data;
+			break;
+		}
+	}
+
+	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;
 
-- 
2.9.3

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

[Sekhar Nori]

On Wednesday 07 December 2016 08:52 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>

Applied to fixes branch. The prefix for mach-davinci patches is "ARM:
davinci: ...". I fixed it up when applying. Please take care for future.

Thanks,
Sekhar

Re: [PATCH v4 2/4] ARM: da850: don't add the emac clock to the clock lookup table twice

[Sekhar Nori]

On Wednesday 07 December 2016 08:52 PM, Bartosz Golaszewski wrote:
> Similarly to the aemif clock - this screws up the linked list of clock
> children. Create a separate clock for mdio inheriting the rate from
> emac_clk.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Applied with change in headline (see comment on 1/4). Also added a 
comment explaining why mdio clk is needed.

> ---
>  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 e9d019c..6b1fbac 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -319,6 +319,11 @@ static struct clk emac_clk = {
>  	.gpsc		= 1,
>  };
>  

/*
 * In order to avoid adding the emac_clk to the clock lookup table twice (and
 * screwing up the linked list in the process) create a separate clock for
 * mdio inheriting the rate from emac_clk.
 */

> +static struct clk mdio_clk = {
> +	.name		= "mdio",
> +	.parent		= &emac_clk,
> +};

Thanks,
Sekhar

Re: [PATCH v4 3/4] ARM: da850: coding style fix

[Sekhar Nori]

On Wednesday 07 December 2016 08:52 PM, Bartosz Golaszewski wrote:
> Fix alignment of the clock lookup table entries.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Applied to v4.11/fixes-non-critical

Thanks,
Sekhar

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

[Sekhar Nori]

On Wednesday 07 December 2016 08:52 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>

Applied to v4.11/fixes-non-critical

Thanks,
Sekhar