Re: [PATCH 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver

On 08-06-15, 20:29, Pi-Cheng Chen wrote:

Sorry for the delay, I have been quite busy recently.

+++ b/drivers/cpufreq/mt8173-cpufreq.c +static LIST_HEAD(cpu_dvfs_info_list);

+static inline struct mtk_cpu_dvfs_info *to_mtk_cpu_dvfs_info(

• 	struct list_head *list)
  

+{

• return list_entry(list, struct mtk_cpu_dvfs_info, node);

+}

+static inline void mtk_cpu_dvfs_info_add(struct mtk_cpu_dvfs_info *info) +{

• list_add(&info->node, &cpu_dvfs_info_list);

+}

No stupid wrappers please. Doesn't make anything better.

+static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_get(int cpu)

A very bad name to a routine with very specific functionality.

+{

• struct mtk_cpu_dvfs_info *info;
• struct list_head *list;
• list_for_each(list, &cpu_dvfs_info_list) {

• info = to_mtk_cpu_dvfs_info(list);
  

• if (cpumask_test_cpu(cpu, info->cpus))
  

• 	return info;
  

• }
• return NULL;

+}

+static void mtk_cpu_dvfs_info_release(void) +{

• struct list_head *list, *tmp;
• struct mtk_cpu_dvfs_info *info;
• list_for_each_safe(list, tmp, &cpu_dvfs_info_list) {

• info = to_mtk_cpu_dvfs_info(list);
  

• dev_pm_opp_free_cpufreq_table(info->cpu_dev,
  

• 			      &info->freq_table);
  

• if (!IS_ERR(info->proc_reg))
  

• 	regulator_put(info->proc_reg);
  

• if (!IS_ERR(info->sram_reg))
  

• 	regulator_put(info->sram_reg);
  

• if (!IS_ERR(info->cpu_clk))
  

• 	clk_put(info->cpu_clk);
  

• if (!IS_ERR(info->inter_clk))
  

• 	clk_put(info->inter_clk);
  

• of_free_opp_table(info->cpu_dev);
  

• list_del(list);
  

• kfree(info);
  

• }

+}

+#define MIN(a, b) ((a) < (b) ? (a) : (b)) +#define MAX(a, b) ((a) > (b) ? (a) : (b))

Look for these in kernel.h

+static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) +{

• if (info->need_voltage_trace)

• return mtk_cpufreq_voltage_trace(info, vproc);
  

• else

• return regulator_set_voltage(info->proc_reg, vproc,
  

• 			     vproc + VOLT_TOL);
  

+}

+static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,

• 		  unsigned int index)
  

+{

• struct cpufreq_frequency_table *freq_table = policy->freq_table;
• struct clk *cpu_clk = policy->clk;
• struct clk *armpll = clk_get_parent(cpu_clk);
• struct mtk_cpu_dvfs_info *info = policy->driver_data;
• struct device *cpu_dev = info->cpu_dev;
• struct dev_pm_opp *opp;
• long freq_hz, old_freq_hz;
• int vproc, old_vproc, inter_vproc, target_vproc, ret;
• inter_vproc = info->intermediate_voltage;
• old_freq_hz = clk_get_rate(cpu_clk);
• old_vproc = regulator_get_voltage(info->proc_reg);
• freq_hz = freq_table[index].frequency * 1000;

A blank line here.

• rcu_read_lock();
• opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
• if (IS_ERR(opp)) {

• rcu_read_unlock();
  

• pr_err("cpu%d: failed to find OPP for %ld\n",
  

•        policy->cpu, freq_hz);
  

• return PTR_ERR(opp);
  

• }
• vproc = dev_pm_opp_get_voltage(opp);
• rcu_read_unlock();
• /*

• * If the new voltage or the intermediate voltage is higher than the
  

• * current voltage, scale up voltage first.
  

• */
  

• target_vproc = (inter_vproc > vproc) ? inter_vproc : vproc;
• if (old_vproc < target_vproc) {

• ret = mtk_cpufreq_set_voltage(info, target_vproc);
  

• if (ret) {
  

• 	pr_err("cpu%d: failed to scale up voltage!\n",
  

• 	       policy->cpu);
  

• 	mtk_cpufreq_set_voltage(info, old_vproc);
  

• 	return ret;
  

• }
  

• }
• /* Reparent the CPU clock to intermediate clock. */
• ret = clk_set_parent(cpu_clk, info->inter_clk);
• if (ret) {

• pr_err("cpu%d: failed to re-parent cpu clock!\n",
  

•        policy->cpu);
  

• mtk_cpufreq_set_voltage(info, old_vproc);
  

• WARN_ON(1);
  

• return ret;
  

• }
• /* Set the original PLL to target rate. */
• ret = clk_set_rate(armpll, freq_hz);
• if (ret) {

• pr_err("cpu%d: failed to scale cpu clock rate!\n",
  

•        policy->cpu);
  

• clk_set_parent(cpu_clk, armpll);
  

• mtk_cpufreq_set_voltage(info, old_vproc);
  

• return ret;
  

• }
• /* Set parent of CPU clock back to the original PLL. */
• ret = clk_set_parent(cpu_clk, armpll);
• if (ret) {

• pr_err("cpu%d: failed to re-parent cpu clock!\n",
  

•        policy->cpu);
  

• mtk_cpufreq_set_voltage(info, inter_vproc);
  

• WARN_ON(1);
  

• return ret;
  

• }
• /*

• * If the new voltage is lower than the intermediate voltage or the
  

• * original voltage, scale down to the new voltage.
  

• */
  

• if (vproc < inter_vproc || vproc < old_vproc) {

• ret = mtk_cpufreq_set_voltage(info, vproc);
  

• if (ret) {
  

• 	pr_err("cpu%d: failed to scale down voltage!\n",
  

• 	       policy->cpu);
  

• 	clk_set_parent(cpu_clk, info->inter_clk);
  

• 	clk_set_rate(armpll, old_freq_hz);
  

• 	clk_set_parent(cpu_clk, armpll);
  

• 	return ret;
  

• }
  

• }
• return 0;

+}

+static int mtk_cpufreq_init(struct cpufreq_policy *policy) +{

• struct mtk_cpu_dvfs_info *info;
• int ret;
• info = mtk_cpu_dvfs_info_get(policy->cpu);
• if (!info) {

• pr_err("%s: mtk cpu dvfs info for cpu%d is not initialized\n",
  

•        __func__, policy->cpu);
  

• return -ENODEV;
  

• }
• ret = cpufreq_table_validate_and_show(policy, info->freq_table);
• if (ret) {

• pr_err("%s: invalid frequency table: %d\n", __func__, ret);
  

• return ret;
  

• }
• cpumask_copy(policy->cpus, info->cpus);
• policy->driver_data = info;
• policy->clk = info->cpu_clk;
• return 0;

+}

+static struct cpufreq_driver mt8173_cpufreq_driver = {

• .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
• .verify = cpufreq_generic_frequency_table_verify,
• .target_index = mtk_cpufreq_set_target,
• .get = cpufreq_generic_get,
• .init = mtk_cpufreq_init,
• .name = "mtk-cpufreq",
• .attr = cpufreq_generic_attr,

+};

+static int mtk_cpu_dvfs_info_init(int cpu) +{

• struct device *cpu_dev;
• struct regulator *proc_reg = ERR_PTR(-ENODEV);
• struct regulator *sram_reg = ERR_PTR(-ENODEV);
• struct clk *cpu_clk = ERR_PTR(-ENODEV);
• struct clk *inter_clk = ERR_PTR(-ENODEV);
• struct mtk_cpu_dvfs_info *info;
• struct cpufreq_frequency_table *freq_table;
• struct dev_pm_opp *opp;
• unsigned long rate;
• int ret;
• cpu_dev = get_cpu_device(cpu);
• if (!cpu_dev) {

• pr_err("failed to get cpu%d device\n", cpu);
  

• return -ENODEV;
  

• }
• ret = of_init_opp_table(cpu_dev);
• if (ret) {

• pr_warn("no OPP table for cpu%d\n", cpu);
  

• return ret;
  

• }
• cpu_clk = clk_get(cpu_dev, "cpu");
• if (IS_ERR(cpu_clk)) {

• if (PTR_ERR(cpu_clk) == -EPROBE_DEFER)
  

• 	pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu);
  

• else
  

• 	pr_err("failed to get cpu clk for cpu%d\n", cpu);
  

• ret = PTR_ERR(cpu_clk);
  

• goto out_free_opp_table;
  

• }
• inter_clk = clk_get(cpu_dev, "intermediate");
• if (IS_ERR(inter_clk)) {

• if (PTR_ERR(inter_clk) == -EPROBE_DEFER)
  

• 	pr_warn("intermediate clk for cpu%d not ready, retry.\n",
  

• 		cpu);
  

• else
  

• 	pr_err("failed to get intermediate clk for cpu%d\n",
  

• 	       cpu);
  

• ret = PTR_ERR(cpu_clk);
  

• goto out_free_resources;
  

• }
• proc_reg = regulator_get_exclusive(cpu_dev, "proc");
• if (IS_ERR(proc_reg)) {

• if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
  

• 	pr_warn("proc regulator for cpu%d not ready, retry.\n",
  

• 		cpu);
  

• else
  

• 	pr_err("failed to get proc regulator for cpu%d\n",
  

• 	       cpu);
  

• ret = PTR_ERR(proc_reg);
  

• goto out_free_resources;
  

• }
• /* Both presence and absence of sram regulator are valid cases. */
• sram_reg = regulator_get_exclusive(cpu_dev, "sram");
• info = kzalloc(sizeof(*info), GFP_KERNEL);
• if (!info) {

• ret = -ENOMEM;
  

• goto out_free_resources;
  

• }
• ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
• if (ret) {

• pr_err("failed to init cpufreq table for cpu%d: %d\n",
  

•        cpu, ret);
  

• goto out_free_mtk_cpu_dvfs_info;
  

• }
• if (!alloc_cpumask_var(&info->cpus, GFP_KERNEL))

Why getting into such trouble? What about just saving policy's pointer in info and use it for freq_table, cpus mask, etc ?

• goto out_free_cpufreq_table;
  

+}

+static int mt8173_cpufreq_probe(struct platform_device *pdev) +{

• int cpu, ret;
• for_each_possible_cpu(cpu) {

• /*
  

•  * If the struct mtk_cpu_dvfs_info for the cpu power domain
  

•  * is already initialized, skip this CPU.
  

•  */
  

• if (!mtk_cpu_dvfs_info_get(cpu)) {
  

• 	ret = mtk_cpu_dvfs_info_init(cpu);
  

• 	if (ret) {
  

• 		if (ret != -EPROBE_DEFER)
  

• 			pr_err("%s probe fail\n", __func__);
  

• 		mtk_cpu_dvfs_info_release();
  

• 		return ret;
  

• 	}
  

• }
  

• }
• ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
• if (ret) {

• pr_err("failed to register mtk cpufreq driver\n");
  

• mtk_cpu_dvfs_info_release();
  

• }
• return ret;

+}

+static struct platform_driver mt8173_cpufreq_platdrv = {

• .driver = {

• .name	= "mt8173-cpufreq",
  

• },
• .probe = mt8173_cpufreq_probe,

+}; +module_platform_driver(mt8173_cpufreq_platdrv);

+static int mt8173_cpufreq_driver_init(void) +{

• struct platform_device *pdev;
• if (!of_machine_is_compatible("mediatek,mt8173"))

• return -ENODEV;
  

• pdev = platform_device_register_simple("mt8173-cpufreq", -1, NULL, 0);
• if (IS_ERR(pdev)) {

• pr_err("failed to register mtk-cpufreq platform device\n");
  

• return PTR_ERR(pdev);
  

• }
• return 0;

+} +module_init(mt8173_cpufreq_driver_init);

Can this driver be built as module? Why this module* crap all over the place?