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

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

Changelog:

v2: Corrected "Mark Memory Controller clock as read-only" patch that
    had the "read-only divider" flag being set in a wrong place. Note
    it also turned out that this patch is needed for a proper EMC freq
    scaling on Tegra30 because some of memory timings may have
    configuration where MC clock configuration should be changed
    simultaneously with the EMC change and currently CLK framework
    reconfigure the divider back to /2 mode after EMC clock changes
    since it's the parent clock for MC. Although it's not really critical
    since system remains fully functional if MC clock configuration
    doesn't match the memory arbitration configuration.

Dmitry Osipenko (5):
  clk: tegra: emc: Don't enable EMC clock manually
  clk: tegra: emc: Support multiple RAM codes
  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 |  3 +-
 drivers/clk/tegra/clk-emc.c     | 57 ++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 23 deletions(-)

-- 
2.21.0

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

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

[Stephen Boyd]

Quoting Dmitry Osipenko (2019-04-14 12:23:17)
> The EMC clock marked as critical, hence it is already enabled at the
> registration time.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

Applied to clk-next

[PATCH v2 2/5] clk: tegra: emc: Support multiple RAM codes

[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

Re: [PATCH v2 2/5] clk: tegra: emc: Support multiple RAM codes

[Stephen Boyd]

Quoting Dmitry Osipenko (2019-04-14 12:23:18)
> 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>
> ---

Applied to clk-next

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

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

[Stephen Boyd]

Quoting Dmitry Osipenko (2019-04-14 12:23:19)
> 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>
> ---

Applied to clk-next

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

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

[Stephen Boyd]

Quoting Dmitry Osipenko (2019-04-14 12:23:20)
> There is no justification for the BUG() in this code.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

Applied to clk-next

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

[Dmitry Osipenko]

The Memory Controller (MC) clock rate can't be simply changed and nothing
in kernel need to change the rate, hence let's make the clock read-only.
This id also needed for the EMC driver because timing configuration may
require the MC clock diver to be disabled, that is handled by the EMC
clock / EMC driver integration and CLK framework shall not touch the
MC divider configuration on the EMC clock rate change.

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

diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
index 205fe8ff63f0..2a1822a22740 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,
+					  CLK_IS_CRITICAL,
+					  reg, 16, 1, CLK_DIVIDER_READ_ONLY,
 					  mc_div_table, lock);
 }
-- 
2.21.0

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

[Stephen Boyd]

Quoting Dmitry Osipenko (2019-04-14 12:23:21)
> The Memory Controller (MC) clock rate can't be simply changed and nothing
> in kernel need to change the rate, hence let's make the clock read-only.
> This id also needed for the EMC driver because timing configuration may
> require the MC clock diver to be disabled, that is handled by the EMC
> clock / EMC driver integration and CLK framework shall not touch the
> MC divider configuration on the EMC clock rate change.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

Applied to clk-next