[PATCH v1 0/5] clk: tegra: EMC/MC clock fixes and improvements

[PATCH v1 0/5] clk: tegra: EMC/MC clock fixes and improvements

[Dmitry Osipenko]

Hello, I was helping with fixing EMC clock scaling on T124 Nyan Big and
in process found some weak points in the code. Primarily the ram code
parsing didn't work if device-tree defines memory timings for multiple
ram codes and after fixing that I spotted few other things that could be
improved.

Dmitry Osipenko (5):
  clk: tegra: emc: Don't enable EMC clock manually
  clk: tegra: emc: Support multiple ram codes parsing
  clk: tegra: emc: Fix EMC max-rate clamping
  clk: tegra: emc: Replace BUG() with WARN_ONCE()
  clk: tegra: divider: Mark Memory Controller clock as read-only

 drivers/clk/tegra/clk-divider.c |  5 +--
 drivers/clk/tegra/clk-emc.c     | 57 ++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.21.0

[PATCH v1 1/5] clk: tegra: emc: Don't enable EMC clock manually

[Dmitry Osipenko]

The EMC clock marked as critical, hence it is already enabled at the
registration time.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-emc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
index 0621a3a82ea6..23416982e7c7 100644
--- a/drivers/clk/tegra/clk-emc.c
+++ b/drivers/clk/tegra/clk-emc.c
@@ -532,7 +532,5 @@ struct clk *tegra_clk_register_emc(void __iomem *base, struct device_node *np,
 	/* Allow debugging tools to see the EMC clock */
 	clk_register_clkdev(clk, "emc", "tegra-clk-debug");
 
-	clk_prepare_enable(clk);
-
 	return clk;
 };
-- 
2.21.0

[PATCH v1 2/5] clk: tegra: emc: Support multiple ram codes parsing

[Dmitry Osipenko]

The timings parser doesn't append timings, but instead it parses only
the first timing and hence doesn't store all of the timings when
device-tree has timings for multiple ram codes. In a result EMC scaling
doesn't work if timings are missing.

Tested-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-emc.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
index 23416982e7c7..28068584ff6e 100644
--- a/drivers/clk/tegra/clk-emc.c
+++ b/drivers/clk/tegra/clk-emc.c
@@ -121,18 +121,23 @@ static int emc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 	struct tegra_clk_emc *tegra;
 	u8 ram_code = tegra_read_ram_code();
 	struct emc_timing *timing = NULL;
-	int i;
+	int i, k;
 
 	tegra = container_of(hw, struct tegra_clk_emc, hw);
 
-	for (i = 0; i < tegra->num_timings; i++) {
+	for (k = 0; k < tegra->num_timings; k++) {
+		if (tegra->timings[k].ram_code == ram_code)
+			break;
+	}
+
+	for (i = k; i < tegra->num_timings; i++) {
 		if (tegra->timings[i].ram_code != ram_code)
-			continue;
+			break;
 
 		timing = tegra->timings + i;
 
 		if (timing->rate > req->max_rate) {
-			i = max(i, 1);
+			i = max(i, k + 1);
 			req->rate = tegra->timings[i - 1].rate;
 			return 0;
 		}
@@ -282,7 +287,7 @@ static struct emc_timing *get_backup_timing(struct tegra_clk_emc *tegra,
 	for (i = timing_index+1; i < tegra->num_timings; i++) {
 		timing = tegra->timings + i;
 		if (timing->ram_code != ram_code)
-			continue;
+			break;
 
 		if (emc_parent_clk_sources[timing->parent_index] !=
 		    emc_parent_clk_sources[
@@ -293,7 +298,7 @@ static struct emc_timing *get_backup_timing(struct tegra_clk_emc *tegra,
 	for (i = timing_index-1; i >= 0; --i) {
 		timing = tegra->timings + i;
 		if (timing->ram_code != ram_code)
-			continue;
+			break;
 
 		if (emc_parent_clk_sources[timing->parent_index] !=
 		    emc_parent_clk_sources[
@@ -433,19 +438,23 @@ static int load_timings_from_dt(struct tegra_clk_emc *tegra,
 				struct device_node *node,
 				u32 ram_code)
 {
+	struct emc_timing *timings_ptr;
 	struct device_node *child;
 	int child_count = of_get_child_count(node);
 	int i = 0, err;
+	size_t size;
+
+	size = (tegra->num_timings + child_count) * sizeof(struct emc_timing);
 
-	tegra->timings = kcalloc(child_count, sizeof(struct emc_timing),
-				 GFP_KERNEL);
+	tegra->timings = krealloc(tegra->timings, size, GFP_KERNEL);
 	if (!tegra->timings)
 		return -ENOMEM;
 
-	tegra->num_timings = child_count;
+	timings_ptr = tegra->timings + tegra->num_timings;
+	tegra->num_timings += child_count;
 
 	for_each_child_of_node(node, child) {
-		struct emc_timing *timing = tegra->timings + (i++);
+		struct emc_timing *timing = timings_ptr + (i++);
 
 		err = load_one_timing_from_dt(tegra, timing, child);
 		if (err) {
@@ -456,7 +465,7 @@ static int load_timings_from_dt(struct tegra_clk_emc *tegra,
 		timing->ram_code = ram_code;
 	}
 
-	sort(tegra->timings, tegra->num_timings, sizeof(struct emc_timing),
+	sort(timings_ptr, child_count, sizeof(struct emc_timing),
 	     cmp_timings, NULL);
 
 	return 0;
@@ -499,10 +508,10 @@ struct clk *tegra_clk_register_emc(void __iomem *base, struct device_node *np,
 		 * fuses until the apbmisc driver is loaded.
 		 */
 		err = load_timings_from_dt(tegra, node, node_ram_code);
-		of_node_put(node);
-		if (err)
+		if (err) {
+			of_node_put(node);
 			return ERR_PTR(err);
-		break;
+		}
 	}
 
 	if (tegra->num_timings == 0)
-- 
2.21.0

[PATCH v1 3/5] clk: tegra: emc: Fix EMC max-rate clamping

[Dmitry Osipenko]

When a clk user requests rate that is higher than the maximum possible,
the rate shall be clamped to the maximum and not to the current value.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-emc.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
index 28068584ff6e..9a0179235939 100644
--- a/drivers/clk/tegra/clk-emc.c
+++ b/drivers/clk/tegra/clk-emc.c
@@ -121,7 +121,7 @@ static int emc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 	struct tegra_clk_emc *tegra;
 	u8 ram_code = tegra_read_ram_code();
 	struct emc_timing *timing = NULL;
-	int i, k;
+	int i, k, t;
 
 	tegra = container_of(hw, struct tegra_clk_emc, hw);
 
@@ -130,12 +130,17 @@ static int emc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 			break;
 	}
 
-	for (i = k; i < tegra->num_timings; i++) {
-		if (tegra->timings[i].ram_code != ram_code)
+	for (t = k; t < tegra->num_timings; t++) {
+		if (tegra->timings[t].ram_code != ram_code)
 			break;
+	}
 
+	for (i = k; i < t; i++) {
 		timing = tegra->timings + i;
 
+		if (timing->rate < req->rate && i != t - 1)
+			continue;
+
 		if (timing->rate > req->max_rate) {
 			i = max(i, k + 1);
 			req->rate = tegra->timings[i - 1].rate;
@@ -145,10 +150,8 @@ static int emc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 		if (timing->rate < req->min_rate)
 			continue;
 
-		if (timing->rate >= req->rate) {
-			req->rate = timing->rate;
-			return 0;
-		}
+		req->rate = timing->rate;
+		return 0;
 	}
 
 	if (timing) {
-- 
2.21.0

[PATCH v1 4/5] clk: tegra: emc: Replace BUG() with WARN_ONCE()

[Dmitry Osipenko]

There is no justification for the BUG() in this code.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-emc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
index 9a0179235939..93ecb538e59b 100644
--- a/drivers/clk/tegra/clk-emc.c
+++ b/drivers/clk/tegra/clk-emc.c
@@ -222,7 +222,10 @@ static int emc_set_timing(struct tegra_clk_emc *tegra,
 
 	if (emc_get_parent(&tegra->hw) == timing->parent_index &&
 	    clk_get_rate(timing->parent) != timing->parent_rate) {
-		BUG();
+		WARN_ONCE(1, "parent %s rate mismatch %lu %lu\n",
+			  __clk_get_name(timing->parent),
+			  clk_get_rate(timing->parent),
+			  timing->parent_rate);
 		return -EINVAL;
 	}
 
-- 
2.21.0

[PATCH v1 5/5] clk: tegra: divider: Mark Memory Controller clock as read-only

[Dmitry Osipenko]

The Memory Controller clock rate can't be simply changed and nothing in
kernel need to change the rate, hence let's make the clock read-only.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-divider.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
index 205fe8ff63f0..f891bbb0d06d 100644
--- a/drivers/clk/tegra/clk-divider.c
+++ b/drivers/clk/tegra/clk-divider.c
@@ -175,6 +175,7 @@ struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
 				  void __iomem *reg, spinlock_t *lock)
 {
 	return clk_register_divider_table(NULL, name, parent_name,
-					  CLK_IS_CRITICAL, reg, 16, 1, 0,
-					  mc_div_table, lock);
+					  CLK_IS_CRITICAL |
+					  CLK_DIVIDER_READ_ONLY,
+					  reg, 16, 1, 0, mc_div_table, lock);
 }
-- 
2.21.0

Re: [PATCH v1 5/5] clk: tegra: divider: Mark Memory Controller clock as read-only

[Dmitry Osipenko]

12.04.2019 1:02, Dmitry Osipenko пишет:
> The Memory Controller clock rate can't be simply changed and nothing in
> kernel need to change the rate, hence let's make the clock read-only.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clk/tegra/clk-divider.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 205fe8ff63f0..f891bbb0d06d 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -175,6 +175,7 @@ struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
>  				  void __iomem *reg, spinlock_t *lock)
>  {
>  	return clk_register_divider_table(NULL, name, parent_name,
> -					  CLK_IS_CRITICAL, reg, 16, 1, 0,
> -					  mc_div_table, lock);
> +					  CLK_IS_CRITICAL |
> +					  CLK_DIVIDER_READ_ONLY,
> +					  reg, 16, 1, 0, mc_div_table, lock);
>  }
> 

Turned out there is a bug here, the read-only flag is the divider's flag and hence it shall be:

 {
 	return clk_register_divider_table(NULL, name, parent_name,
-					  CLK_IS_CRITICAL |
-					  CLK_DIVIDER_READ_ONLY,
-					  reg, 16, 1, 0, mc_div_table, lock);
+					  CLK_IS_CRITICAL,
+					  reg, 16, 1, CLK_DIVIDER_READ_ONLY,
+					  mc_div_table, lock);
 }

I'll fix it up in v2.