Re: [PATCH v3 7/7] PM / devfreq: Add devfreq driver for Exynos5420
From: Tomasz Figa <hidden>
Date: 2014-07-16 11:55:57
Also in:
linux-pm
Hi Abhilash, Please see my comments inline. On 15.07.2014 20:36, Abhilash Kesavan wrote:
From: "Arjun.K.V" <redacted> Exynos5420 bus device devfreq driver monitors PPMU counters and adjusts INT domain operating frequencies and voltages. Support for 5420 has been added to the extant 5250 support. Signed-off-by: Arjun.K.V <redacted> Signed-off-by: Andrew Bresticker <redacted> Signed-off-by: Doug Anderson <dianders@chromium.org> Signed-off-by: Abhilash Kesavan <redacted> --- drivers/devfreq/Kconfig | 10 +- drivers/devfreq/exynos/exynos5_bus.c | 598 ++++++++++++++++++++++++++++------ 2 files changed, 508 insertions(+), 100 deletions(-)
[snip]
-#define MAX_SAFEVOLT 1100000 /* 1.10V */ -/* Assume that the bus is saturated if the utilization is 25% */ -#define INT_BUS_SATURATION_RATIO 25 +/* Assume that we need to bump up the level if the utilization is 10% */ +#define INT_BUS_SATURATION_RATIO 10
There is nothing about this change in commit message and changing the ratio from 25% to 10% seems to be rather significant. Please give the rationale for this change.
+#define INT_VOLT_STEP_UV 12500
+
+struct exynos5_busfreq_drv_data {
+ int busf_type;
+};
+
+enum exynos5_busf_type {
+ TYPE_BUSF_EXYNOS5250,
+ TYPE_BUSF_EXYNOS5420,
+};Using this kind of enums is discouraged. Rather than that, just create a structure that describes the differences between variants and use this as type.
quoted hunk ↗ jump to hunk
enum int_level_idx { LV_0,@@ -41,12 +46,31 @@ enum int_level_idx { _LV_END };
[snip]
+
+static struct int_bus_opp_table *exynos5_int_opp_table;
+static struct int_bus_opp_table exynos5250_int_opp_table[] = {
{LV_0, 266000, 1025000},
{LV_1, 200000, 1025000},
{LV_2, 160000, 1025000},[snip]
+static struct int_clk_info exynos5420_aclk_300_jpeg[] = {
+ /* Level, Freq, Parent_Pll */
+ {LV_0, 300000, D_PLL},
+ {LV_1, 300000, D_PLL},
+ {LV_2, 200000, D_PLL},
+ {LV_3, 150000, D_PLL},
+ {LV_4, 75000, D_PLL},
+};These should be parsed from DT.
+
+#define EXYNOS5420_INT_PM_CLK(NAME, CLK, PCLK, CLK_INFO) \
+static struct int_comp_clks int_pm_clks_##NAME = { \
+ .mux_clk_name = CLK, \
+ .div_clk_name = PCLK, \
+ .clk_info = CLK_INFO, \
+}
+
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys, "aclk200_fsys",
+ "aclk200_fsys_d", exynos5420_aclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(pclk_200_fsys, "pclk200_fsys",
+ "pclk200_fsys_d", exynos5420_pclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(aclk_100_noc, "aclk100_noc",
+ "aclk100_noc_d", exynos5420_aclk_100_noc);
+EXYNOS5420_INT_PM_CLK(aclk_400_wcore, "aclk400_wcore",
+ "aclk400_wcore_d", exynos5420_aclk_400_wcore);
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys2, "aclk200_fsys2",
+ "aclk200_fsys2_d", exynos5420_aclk_200_fsys2);
+EXYNOS5420_INT_PM_CLK(aclk_400_mscl, "aclk400_mscl",
+ "aclk400_mscl_d", exynos5420_aclk_400_mscl);
+EXYNOS5420_INT_PM_CLK(aclk_166, "aclk166",
+ "aclk166_d", exynos5420_aclk_166);
+EXYNOS5420_INT_PM_CLK(aclk_266, "aclk266",
+ "aclk266_d", exynos5420_aclk_266);
+EXYNOS5420_INT_PM_CLK(aclk_66, "aclk66",
+ "aclk66_d", exynos5420_aclk_66);
+EXYNOS5420_INT_PM_CLK(aclk_300_disp1, "aclk300_disp1",
+ "aclk300_disp1_d", exynos5420_aclk_300_disp1);
+EXYNOS5420_INT_PM_CLK(aclk_300_jpeg, "aclk300_jpeg",
+ "aclk300_jpeg_d", exynos5420_aclk_300_jpeg);
+EXYNOS5420_INT_PM_CLK(aclk_400_disp1, "aclk400_disp1",
+ "aclk400_disp1_d", exynos5420_aclk_400_disp1);List of the clocks should be parsed from DT as well, without hardcoding data for every SoC in the driver.
quoted hunk ↗ jump to hunk
+ +static struct int_comp_clks *exynos5420_int_pm_clks[] = { + &int_pm_clks_aclk_200_fsys, + &int_pm_clks_pclk_200_fsys, + &int_pm_clks_aclk_100_noc, + &int_pm_clks_aclk_400_wcore, + &int_pm_clks_aclk_200_fsys2, + &int_pm_clks_aclk_400_mscl, + &int_pm_clks_aclk_166, + &int_pm_clks_aclk_266, + &int_pm_clks_aclk_66, + &int_pm_clks_aclk_300_disp1, + &int_pm_clks_aclk_300_jpeg, + &int_pm_clks_aclk_400_disp1, + NULL, +}; + +static int exynos5250_int_set_rate(struct busfreq_data_int *data, unsigned long rate) { int index, i; - for (index = 0; index < ARRAY_SIZE(exynos5_int_opp_table); index++) { - if (exynos5_int_opp_table[index].clk == rate) + for (index = 0; index < ARRAY_SIZE(exynos5250_int_opp_table); index++) { + if (exynos5250_int_opp_table[index].clk == rate) break; }@@ -161,8 +370,8 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data, return -EINVAL; /* Change the system clock divider values */ - for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) { - struct int_clk *clk_info = &exynos5_int_clks[i]; + for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) { + struct int_simple_clk *clk_info = &exynos5250_int_clks[i]; int ret; ret = clk_set_rate(clk_info->clk,@@ -177,10 +386,111 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data, return 0; } +static struct clk *exynos5420_find_pll(struct busfreq_data_int *data, + enum int_bus_pll target_pll) +{ + struct clk *target_src_clk = NULL; + + switch (target_pll) { + case C_PLL: + target_src_clk = data->mout_cpll; + break; + case M_PLL: + target_src_clk = data->mout_mpll; + break; + case D_PLL: + target_src_clk = data->mout_dpll; + break; + case I_PLL: + target_src_clk = data->mout_ipll; + break; + default: + break; + }
What about storing the clocks in an array? Then all you would need to do could be as simple as accessing data->plls[target_pll].
+
+ return target_src_clk;
+}
+
+static int exynos5420_int_set_rate(struct busfreq_data_int *data,
+ unsigned long target_freq, unsigned long pre_freq)
+{
+ unsigned int i;
+ unsigned long tar_rate;
+ int target_idx = -EINVAL;
+ int pre_idx = -EINVAL;
+ struct int_comp_clks *int_clk;
+ struct clk *new_src_pll;
+ struct clk *old_src_pll;
+ unsigned long old_src_rate, new_src_rate;
+ unsigned long rate1, rate2, rate3, rate4;
+
+ /* Find the levels for target and previous frequencies */
+ for (i = 0; i < _LV_END; i++) {
+ if (exynos5420_int_opp_table[i].clk == target_freq)
+ target_idx = exynos5420_int_opp_table[i].idx;
+ if (exynos5420_int_opp_table[i].clk == pre_freq)
+ pre_idx = exynos5420_int_opp_table[i].idx;
+ }
+
+ list_for_each_entry(int_clk, &data->list, node) {
+ tar_rate = int_clk->clk_info[target_idx].target_freq * 1000;
+
+ old_src_pll = clk_get_parent(int_clk->mux_clk);
+ new_src_pll = exynos5420_find_pll(data,
+ int_clk->clk_info[target_idx].src_pll);
+
+ if (old_src_pll == new_src_pll) {
+ /* No need to change pll */
+ clk_set_rate(int_clk->div_clk, tar_rate);
+ pr_debug("%s: %s now %lu (%lu)\n", __func__,
+ int_clk->mux_clk_name,
+ clk_get_rate(int_clk->div_clk), tar_rate);
+ continue;
+ }
+
+ old_src_rate = clk_get_rate(old_src_pll);
+ new_src_rate = clk_get_rate(new_src_pll);
+ rate1 = clk_get_rate(int_clk->div_clk);
+
+ /*
+ * If we're switching to a faster PLL we should bump up the
+ * divider before switching.
+ */
+ if (new_src_rate > old_src_rate) {
+ int new_div;
+ unsigned long tmp_rate;
+
+ new_div = DIV_ROUND_UP(new_src_rate, tar_rate);
+ tmp_rate = DIV_ROUND_UP(old_src_rate, new_div);
+ clk_set_rate(int_clk->div_clk, tmp_rate);
+ }
+ rate2 = clk_get_rate(int_clk->div_clk);
+
+ /* We can safely change the mux now */
+ clk_set_parent(int_clk->mux_clk, new_src_pll);
+ rate3 = clk_get_rate(int_clk->div_clk);
+
+ /*
+ * Give us a proper divider; technically not needed in the case
+ * where we pre-calculated the divider above (the new_src_rate >
+ * old_src_rate case), but let's be formal about it.
+ */
+ clk_set_rate(int_clk->div_clk, tar_rate);
+ rate4 = clk_get_rate(int_clk->div_clk);
+
+ pr_debug("%s: %s => PLL %d; %lu=>%lu=>%lu=>%lu (%lu)\n",
+ __func__, int_clk->mux_clk_name,
+ int_clk->clk_info[target_idx].src_pll,
+ rate1, rate2, rate3, rate4, tar_rate);
+ }
+ return 0;
+}The above function looks like it could be made much more generic and reused for Exynos5250 as well.
quoted hunk ↗ jump to hunk
+ static int exynos5_int_setvolt(struct busfreq_data_int *data, unsigned long volt) { - return regulator_set_voltage(data->vdd_int, volt, MAX_SAFEVOLT); + return regulator_set_voltage(data->vdd_int, volt, + volt + INT_VOLT_STEP_UV); } static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,@@ -218,18 +528,15 @@ static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq, if (data->disabled) goto out; - if (freq > exynos5_int_opp_table[0].clk) - pm_qos_update_request(&data->int_req, freq * 16 / 1000); - else - pm_qos_update_request(&data->int_req, -1); - if (old_freq < freq) err = exynos5_int_setvolt(data, volt); if (err) goto out; - err = exynos5_int_set_rate(data, freq); - + if (data->type == TYPE_BUSF_EXYNOS5250) + err = exynos5250_int_set_rate(data, freq); + else + err = exynos5420_int_set_rate(data, freq, old_freq); if (err) goto out;@@ -267,16 +574,20 @@ static int exynos5_int_get_dev_status(struct device *dev, } static struct devfreq_dev_profile exynos5_devfreq_int_profile = { - .initial_freq = 160000, - .polling_ms = 100, + .polling_ms = 10,
Another change not mentioned in commit message.
quoted hunk ↗ jump to hunk
.target = exynos5_busfreq_int_target, .get_dev_status = exynos5_int_get_dev_status, }; -static int exynos5250_init_int_tables(struct busfreq_data_int *data) +static int exynos5_init_int_tables(struct busfreq_data_int *data) { int i, err = 0; + if (data->type == TYPE_BUSF_EXYNOS5250) + exynos5_int_opp_table = exynos5250_int_opp_table; + else + exynos5_int_opp_table = exynos5420_int_opp_table; + for (i = LV_0; i < _LV_END; i++) { err = dev_pm_opp_add(data->dev, exynos5_int_opp_table[i].clk, exynos5_int_opp_table[i].volt);@@ -297,6 +608,7 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this, struct dev_pm_opp *opp; unsigned long maxfreq = ULONG_MAX; unsigned long freq; + unsigned long old_freq; unsigned long volt; int err = 0;@@ -322,8 +634,14 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this, if (err) goto unlock; - err = exynos5_int_set_rate(data, freq); + old_freq = data->curr_freq; + if (data->type == TYPE_BUSF_EXYNOS5250) + err = exynos5250_int_set_rate(data, freq); + else if (data->type == TYPE_BUSF_EXYNOS5420) + err = exynos5420_int_set_rate(data, freq, old_freq); + else + err = -EINVAL; if (err) goto unlock;@@ -345,16 +663,38 @@ unlock: return NOTIFY_DONE; } +static const struct of_device_id exynos5_busfreq_dt_match[]; + +static inline int exynos5_busfreq_get_driver_data(struct platform_device *pdev) +{ +#ifdef CONFIG_OF
Exynos is DT-only, so there is no need to handle non-DT cases.
quoted hunk ↗ jump to hunk
+ struct exynos5_busfreq_drv_data *data; + + if (pdev->dev.of_node) { + const struct of_device_id *match; + + match = of_match_node(exynos5_busfreq_dt_match, + pdev->dev.of_node); + data = (struct exynos5_busfreq_drv_data *) match->data; + return data->busf_type; + } +#endif + return platform_get_device_id(pdev)->driver_data; +} + static int exynos5_busfreq_int_probe(struct platform_device *pdev) { struct busfreq_data_int *data; struct busfreq_ppmu_data *ppmu_data; + struct device_node *np = pdev->dev.of_node; struct dev_pm_opp *opp; struct device *dev = &pdev->dev; - struct device_node *np; unsigned long initial_freq; unsigned long initial_volt; + struct clk *mux_clk, *div_clk; + struct int_comp_clks *int_pm_clk; int err = 0; + int nr_clk; int i; data = devm_kzalloc(&pdev->dev, sizeof(struct busfreq_data_int),@@ -364,22 +704,27 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev) return -ENOMEM; } + INIT_LIST_HEAD(&data->list); + data->type = exynos5_busfreq_get_driver_data(pdev); + ppmu_data = &data->ppmu_data; - ppmu_data->ppmu_end = PPMU_END; + if (data->type == TYPE_BUSF_EXYNOS5250) { + ppmu_data->ppmu_end = PPMU_END_5250; + } else if (data->type == TYPE_BUSF_EXYNOS5420) { + ppmu_data->ppmu_end = PPMU_END_5420; + } else { + dev_err(dev, "Cannot determine the device id %d\n", data->type); + return -EINVAL; + } + ppmu_data->ppmu = devm_kzalloc(dev, - sizeof(struct exynos_ppmu) * PPMU_END, - GFP_KERNEL); + sizeof(struct exynos_ppmu) * (ppmu_data->ppmu_end), + GFP_KERNEL); if (!ppmu_data->ppmu) { dev_err(dev, "Failed to allocate memory for exynos_ppmu\n"); return -ENOMEM; } - np = of_find_compatible_node(NULL, NULL, "samsung,exynos5250-ppmu"); - if (np == NULL) { - pr_err("Unable to find PPMU node\n"); - return -ENOENT; - }
This was actually closer to the right solution than what this series does. Actually there was similar attempt already, but for Exynos4 and I even suggested how this could be handled properly. Please see [1] for the whole discussion. [1] https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg27491.html
quoted hunk ↗ jump to hunk
- for (i = 0; i < ppmu_data->ppmu_end; i++) { /* map PPMU memory region */ ppmu_data->ppmu[i].hw_base = of_iomap(np, i);@@ -388,13 +733,17 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev) return -ENOMEM; } } + data->pm_notifier.notifier_call = exynos5_busfreq_int_pm_notifier_event; data->dev = dev; mutex_init(&data->lock); - err = exynos5250_init_int_tables(data); - if (err) + err = exynos5_init_int_tables(data); + if (err) { + dev_err(dev, "Cannot initialize busfreq table %d\n", + data->type); return err; + } data->vdd_int = devm_regulator_get(dev, "vdd_int"); if (IS_ERR(data->vdd_int)) {@@ -402,18 +751,70 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev) return PTR_ERR(data->vdd_int); } - for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) { - struct int_clk *clk_info = &exynos5_int_clks[i]; + if (data->type == TYPE_BUSF_EXYNOS5250) { + for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) { + struct int_simple_clk *clk_info = + &exynos5250_int_clks[i]; + + clk_info->clk = devm_clk_get(dev, clk_info->clk_name); + if (IS_ERR(clk_info->clk)) { + dev_err(dev, "Failed to get clock %s\n", + clk_info->clk_name); + return PTR_ERR(clk_info->clk); + } + } + } else { + data->mout_ipll = devm_clk_get(dev, "mout_ipll"); + if (IS_ERR(data->mout_ipll)) { + dev_err(dev, "Cannot get clock \"mout_ipll\"\n"); + return PTR_ERR(data->mout_ipll); + } - clk_info->clk = devm_clk_get(dev, clk_info->clk_name); - if (IS_ERR(clk_info->clk)) { - dev_err(dev, "Failed to get clock %s\n", - clk_info->clk_name); - return PTR_ERR(clk_info->clk); + data->mout_mpll = devm_clk_get(dev, "mout_mpll"); + if (IS_ERR(data->mout_mpll)) { + dev_err(dev, "Cannot get clock \"mout_mpll\"\n"); + return PTR_ERR(data->mout_mpll); + } + + data->mout_dpll = devm_clk_get(dev, "mout_dpll"); + if (IS_ERR(data->mout_dpll)) { + dev_err(dev, "Cannot get clock \"mout_dpll\"\n"); + return PTR_ERR(data->mout_dpll); + } + + data->mout_cpll = devm_clk_get(dev, "mout_cpll"); + if (IS_ERR(data->mout_cpll)) { + dev_err(dev, "Cannot get clock \"mout_cpll\"\n"); + return PTR_ERR(data->mout_cpll); + } + + for (nr_clk = 0; exynos5420_int_pm_clks[nr_clk] != NULL; + nr_clk++) { + int_pm_clk = exynos5420_int_pm_clks[nr_clk]; + mux_clk = devm_clk_get(dev, int_pm_clk->mux_clk_name); + if (IS_ERR(mux_clk)) { + dev_err(dev, "Cannot get mux clock: %s\n", + int_pm_clk->mux_clk_name); + return PTR_ERR(mux_clk); + } + div_clk = devm_clk_get(dev, int_pm_clk->div_clk_name); + if (IS_ERR(div_clk)) { + dev_err(dev, "Cannot get div clock: %s\n", + int_pm_clk->div_clk_name); + return PTR_ERR(div_clk); + } + int_pm_clk->mux_clk = mux_clk; + int_pm_clk->div_clk = div_clk; + list_add_tail(&int_pm_clk->node, &data->list);
All those could be probably handled with an array and a for loop. In general, this patch apparently contains many separate changes and not only is hard to review but also hard to debug potential problems - git bisect has commit granularity. Please refactor the driver step by step first and then add support for new SoC after it has all the needed features. Best regards, Tomasz