Haven't reviewed it completely yet, but this is all I have done.
On 4 March 2015 at 14:19, pi-cheng.chen pi-cheng.chen@linaro.org wrote:
+static int mtk_cpufreq_notify(struct notifier_block *nb,
unsigned long action, void *data)+{
struct cpufreq_freqs *freqs = data;struct cpu_opp_table *opp_tbl = dvfs_info->opp_tbl;
There is only one dvfs info ? but there are two clusters, sorry got confused a bit..
int old_vproc, new_vproc, old_index, new_index;if (!cpumask_test_cpu(freqs->cpu, &dvfs_info->cpus))return NOTIFY_DONE;old_vproc = regulator_get_voltage(dvfs_info->proc_reg);old_index = cpu_opp_table_get_volt_index(old_vproc);new_index = cpu_opp_table_get_freq_index(freqs->new * 1000);new_vproc = opp_tbl[new_index].vproc;if (old_vproc == new_vproc)return 0;if ((action == CPUFREQ_PRECHANGE && old_vproc < new_vproc) ||(action == CPUFREQ_POSTCHANGE && old_vproc > new_vproc))mtk_cpufreq_voltage_trace(old_index, new_index);return NOTIFY_OK;+}
+static struct notifier_block mtk_cpufreq_nb = {
.notifier_call = mtk_cpufreq_notify,+};
+static int cpu_opp_table_init(struct device *dev) +{
struct device *cpu_dev = dvfs_info->cpu_dev;struct cpu_opp_table *opp_tbl;struct dev_pm_opp *opp;int ret, cnt, i;unsigned long rate, vproc, vsram;ret = of_init_opp_table(cpu_dev);if (ret) {dev_err(dev, "Failed to init mtk_opp_table: %d\n", ret);return ret;}rcu_read_lock();cnt = dev_pm_opp_get_opp_count(cpu_dev);if (cnt < 0) {dev_err(cpu_dev, "No OPP table is found: %d", cnt);ret = cnt;goto out_free_opp_tbl;}opp_tbl = devm_kcalloc(dev, (cnt + 1), sizeof(struct cpu_opp_table),GFP_ATOMIC);if (!opp_tbl) {ret = -ENOMEM;goto out_free_opp_tbl;}for (i = 0, rate = 0; i < cnt; i++, rate++) {opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);if (IS_ERR(opp)) {ret = PTR_ERR(opp);goto out_free_opp_tbl;}vproc = dev_pm_opp_get_voltage(opp);vproc = get_regulator_voltage_ceil(dvfs_info->proc_reg, vproc);vsram = vproc + VOLT_SHIFT_LOWER_LIMIT;vsram = get_regulator_voltage_ceil(dvfs_info->sram_reg, vsram);if (vproc < 0 || vsram < 0) {ret = -EINVAL;goto out_free_opp_tbl;}opp_tbl[i].freq = rate;opp_tbl[i].vproc = vproc;opp_tbl[i].vsram = vsram;}opp_tbl[i].freq = 0;opp_tbl[i].vproc = -1;opp_tbl[i].vsram = -1;dvfs_info->opp_tbl = opp_tbl;+out_free_opp_tbl:
rcu_read_unlock();of_free_opp_table(cpu_dev);return ret;+}
+static struct cpufreq_cpu_domain *get_cpu_domain(struct list_head *domain_list,
int cpu)+{
struct list_head *node;list_for_each(node, domain_list) {struct cpufreq_cpu_domain *domain;domain = container_of(node, struct cpufreq_cpu_domain, node);if (cpumask_test_cpu(cpu, &domain->cpus))return domain;}return NULL;+}
+static int mtk_cpufreq_probe(struct platform_device *pdev)
On a dual cluster big LITTLE (your system), how many times is probe getting called ? Once or twice, i.e. for each cluster ??
+{
struct clk *inter_clk;struct cpufreq_dt_platform_data *pd;struct platform_device *dev;unsigned long inter_freq;int cpu, ret;inter_clk = clk_get(&pdev->dev, NULL);
How is this supposed to work ? How will pdev->dev give intermediate clock ?
if (IS_ERR(inter_clk)) {if (PTR_ERR(inter_clk) == -EPROBE_DEFER) {dev_warn(&pdev->dev, "clock not ready. defer probeing.\n");return -EPROBE_DEFER;}dev_err(&pdev->dev, "Failed to get intermediate clock\n");return -ENODEV;}inter_freq = clk_get_rate(inter_clk);pd = devm_kzalloc(&pdev->dev, sizeof(*pd), GFP_KERNEL);if (!pd)return -ENOMEM;dvfs_info = devm_kzalloc(&pdev->dev, sizeof(*dvfs_info), GFP_KERNEL);if (!dvfs_info)return -ENOMEM;
Instead of two allocations, you could have made pd part of dvfs_info and allocated only once.
pd->independent_clocks = 1,
s/,/; ??
INIT_LIST_HEAD(&pd->domain_list);for_each_possible_cpu(cpu) {struct device *cpu_dev;struct cpufreq_cpu_domain *new_domain;struct regulator *proc_reg, *sram_reg;cpu_dev = get_cpu_device(cpu);
This should be done in the below if block only.
if (!dvfs_info->cpu_dev) {proc_reg = regulator_get_exclusive(cpu_dev, "proc");sram_reg = regulator_get_exclusive(cpu_dev, "sram");if (PTR_ERR(proc_reg) == -EPROBE_DEFER ||PTR_ERR(sram_reg) == -EPROBE_DEFER)return -EPROBE_DEFER;if (!IS_ERR_OR_NULL(proc_reg) &&!IS_ERR_OR_NULL(sram_reg)) {dvfs_info->cpu_dev = cpu_dev;dvfs_info->proc_reg = proc_reg;dvfs_info->sram_reg = sram_reg;cpumask_copy(&dvfs_info->cpus,&cpu_topology[cpu].core_sibling);}}if (get_cpu_domain(&pd->domain_list, cpu))continue;
This isn't required if you do below..
new_domain = devm_kzalloc(&pdev->dev, sizeof(*new_domain),GFP_KERNEL);if (!new_domain)return -ENOMEM;cpumask_copy(&new_domain->cpus,&cpu_topology[cpu].core_sibling);new_domain->intermediate_freq = inter_freq;list_add(&new_domain->node, &pd->domain_list);
Just issue a 'break' from here as you don't want to let this loop run again.
}if (IS_ERR_OR_NULL(dvfs_info->proc_reg) ||IS_ERR_OR_NULL(dvfs_info->sram_reg)) {dev_err(&pdev->dev, "Failed to get regulators\n");return -ENODEV;}
If you really need these, then don't allocate new_domain unless you find a CPU with these regulators..
ret = cpu_opp_table_init(&pdev->dev);if (ret) {dev_err(&pdev->dev, "Failed to setup cpu_opp_table: %d\n",ret);return ret;}ret = cpufreq_register_notifier(&mtk_cpufreq_nb,CPUFREQ_TRANSITION_NOTIFIER);if (ret) {dev_err(&pdev->dev, "Failed to register cpufreq notifier\n");return ret;}
Don't want to free OPP table here on error ?
dev = platform_device_register_data(NULL, "cpufreq-dt", -1, pd,sizeof(*pd));
So this routine is going to be called only once. Then how are you initializing stuff for both the clusters in the upper for loop ? It looked very very confusing.
if (IS_ERR(dev)) {dev_err(&pdev->dev,"Failed to register cpufreq-dt platform device\n");return PTR_ERR(dev);}return 0;+}
+static const struct of_device_id mtk_cpufreq_match[] = {
{.compatible = "mediatek,mtk-cpufreq",
Can't you use "mediatek,mt8173" here ?
},{}+}; +MODULE_DEVICE_TABLE(of, mtk_cpufreq_match);
+static struct platform_driver mtk_cpufreq_platdrv = {
.driver = {.name = "mtk-cpufreq",.of_match_table = mtk_cpufreq_match,},.probe = mtk_cpufreq_probe,+}; +module_platform_driver(mtk_cpufreq_platdrv);