clk: tegra: EMC/MC clock fixes and improvements
Subsystems (3)
cover clk: tegra: EMC/MC clock fixes and improvements
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(-)
Signature · 1 line
2.21.0
PATCH 1/5 clk: tegra: emc: Don't enable EMC clock manually
The EMC clock marked as critical, hence it is already enabled at the registration time.
--- drivers/clk/tegra/clk-emc.c | 2 -- 1 file changed, 2 deletions(-)
drivers/clk/tegra/clk-emc.c +0 -2
@@ -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; };
Signature · 1 line
2.21.0
PATCH 2/5 clk: tegra: emc: Support multiple ram codes parsing
In reply to Dmitry OsipenkoThe 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.
--- drivers/clk/tegra/clk-emc.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)
drivers/clk/tegra/clk-emc.c +23 -14
@@ -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 )
Signature · 1 line
2.21.0
PATCH 3/5 clk: tegra: emc: Fix EMC max-rate clamping
In reply to Dmitry OsipenkoWhen 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.
--- drivers/clk/tegra/clk-emc.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
drivers/clk/tegra/clk-emc.c +10 -7
@@ -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) {
Signature · 1 line
2.21.0
PATCH 4/5 clk: tegra: emc: Replace BUG() with WARN_ONCE()
In reply to Dmitry OsipenkoThere is no justification for the BUG() in this code.
--- drivers/clk/tegra/clk-emc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/clk/tegra/clk-emc.c +4 -1
@@ -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 ; }
Signature · 1 line
2.21.0
PATCH 5/5 clk: tegra: divider: Mark Memory Controller clock as read-only
In reply to Dmitry OsipenkoThe 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.
--- drivers/clk/tegra/clk-divider.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clk/tegra/clk-divider.c +3 -2
@@ -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); }
Signature · 1 line
2.21.0
Re: [PATCH v1 5/5] clk: tegra: divider: Mark Memory Controller clock as read-only
12.04.2019 1:02, Dmitry Osipenko пишет:
Quoted 14 lines
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 <[email protected]>--- drivers/clk/tegra/clk-divider.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/clk/tegra/clk-divider.c +0
-0
drivers/clk/tegra/clk-divider.c struct clk *tegra_clk_register_mc(const char *name, const char *parent_name, · line 181
Quoted 9 lines
drivers/clk/tegra/clk-divider.c +3
-2@@ -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.