On 24-10-22, 13:39, Hector Martin wrote:
> diff --git a/drivers/cpufreq/apple-soc-cpufreq.c b/drivers/cpufreq/apple-soc-cpufreq.c
> +struct apple_soc_cpufreq_info {
> + u64 max_pstate;
> + u64 cur_pstate_mask;
> + u64 cur_pstate_shift;
> +};
> +
> +struct apple_cpu_priv {
> + struct device *cpu_dev;
> + void __iomem *reg_base;
> + const struct apple_soc_cpufreq_info *info;
> +};
> +
> +static struct cpufreq_driver apple_soc_cpufreq_driver;
> +
> +const struct apple_soc_cpufreq_info soc_t8103_info = {
static ? For other instances too.
> + .max_pstate = 15,
> + .cur_pstate_mask = APPLE_DVFS_STATUS_CUR_PS_T8103,
> + .cur_pstate_shift = APPLE_DVFS_STATUS_CUR_PS_SHIFT_T8103,
> +};
> +
> +const struct apple_soc_cpufreq_info soc_t8112_info = {
> + .max_pstate = 31,
> + .cur_pstate_mask = APPLE_DVFS_STATUS_CUR_PS_T8112,
> + .cur_pstate_shift = APPLE_DVFS_STATUS_CUR_PS_SHIFT_T8112,
> +};
> +
> +const struct apple_soc_cpufreq_info soc_default_info = {
> + .max_pstate = 15,
> + .cur_pstate_mask = 0, /* fallback */
> +};
> +
> +static const struct of_device_id apple_soc_cpufreq_of_match[] = {
> + {
> + .compatible = "apple,t8103-cluster-cpufreq",
> + .data = &soc_t8103_info,
> + },
> + {
Isn't the preferred way for this is "}, {" instead ?
I couldn't find this in Coding Guidelines, but somehow remember that
to be the preferred format.
> + .compatible = "apple,t8112-cluster-cpufreq",
> + .data = &soc_t8112_info,
> + },
> + {
> + .compatible = "apple,cluster-cpufreq",
> + .data = &soc_default_info,
> + },
> + {}
> +};
> +
> +static unsigned int apple_soc_cpufreq_get_rate(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
> + struct apple_cpu_priv *priv = policy->driver_data;
> + unsigned int pstate;
> + unsigned int i;
Merge these two ?
> +
> + if (priv->info->cur_pstate_mask) {
> + u64 reg = readq_relaxed(priv->reg_base + APPLE_DVFS_STATUS);
> +
> + pstate = (reg & priv->info->cur_pstate_mask) >> priv->info->cur_pstate_shift;
> + } else {
> + /*
> + * For the fallback case we might not know the layout of DVFS_STATUS,
> + * so just use the command register value (which ignores boost limitations).
> + */
> + u64 reg = readq_relaxed(priv->reg_base + APPLE_DVFS_CMD);
> +
> + pstate = FIELD_GET(APPLE_DVFS_CMD_PS1, reg);
> + }
> +
> + for (i = 0; policy->freq_table[i].frequency != CPUFREQ_TABLE_END; i++)
You may want to use, cpufreq_for_each_valid_entry(), or some other
generic iterator here.
> + if (policy->freq_table[i].driver_data == pstate)
> + return policy->freq_table[i].frequency;
> +
> + dev_err(priv->cpu_dev, "could not find frequency for pstate %d\n",
> + pstate);
> + return 0;
> +}
> +
> +static int apple_soc_cpufreq_set_target(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct apple_cpu_priv *priv = policy->driver_data;
> + unsigned int pstate = policy->freq_table[index].driver_data;
> + u64 reg;
> +
> + /* Fallback for newer SoCs */
> + if (index > priv->info->max_pstate)
> + index = priv->info->max_pstate;
> +
> + if (readq_poll_timeout_atomic(priv->reg_base + APPLE_DVFS_CMD, reg,
> + !(reg & APPLE_DVFS_CMD_BUSY), 2,
> + APPLE_DVFS_TRANSITION_TIMEOUT)) {
> + return -EIO;
> + }
> +
> + reg &= ~(APPLE_DVFS_CMD_PS1 | APPLE_DVFS_CMD_PS2);
> + reg |= FIELD_PREP(APPLE_DVFS_CMD_PS1, pstate);
> + reg |= FIELD_PREP(APPLE_DVFS_CMD_PS2, pstate);
> + reg |= APPLE_DVFS_CMD_SET;
> +
> + writeq_relaxed(reg, priv->reg_base + APPLE_DVFS_CMD);
> +
> + return 0;
> +}
> +
> +static unsigned int apple_soc_cpufreq_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + if (apple_soc_cpufreq_set_target(policy, policy->cached_resolved_idx) < 0)
> + return 0;
> +
> + return policy->freq_table[policy->cached_resolved_idx].frequency;
> +}
> +
> +static int apple_soc_cpufreq_find_cluster(struct cpufreq_policy *policy,
> + void __iomem **reg_base,
> + const struct apple_soc_cpufreq_info **info)
> +{
> + struct of_phandle_args args;
> + const struct of_device_id *match;
> + int ret = 0;
> +
> + ret = of_perf_domain_get_sharing_cpumask(policy->cpu, "performance-domains",
> + "#performance-domain-cells",
> + policy->cpus, &args);
> + if (ret < 0)
> + return ret;
> +
> + match = of_match_node(apple_soc_cpufreq_of_match, args.np);
> + of_node_put(args.np);
> + if (!match)
> + return -ENODEV;
> +
> + *info = match->data;
> +
> + *reg_base = of_iomap(args.np, 0);
> + if (IS_ERR(*reg_base))
> + return PTR_ERR(*reg_base);
> +
> + return 0;
> +}
> +
> +static struct freq_attr *apple_soc_cpufreq_hw_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> + NULL,
> +};
> +
> +static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)
> +{
> + int ret, i;
> + unsigned int transition_latency;
> + void __iomem *reg_base;
> + struct device *cpu_dev;
> + struct apple_cpu_priv *priv;
> + const struct apple_soc_cpufreq_info *info;
> + struct cpufreq_frequency_table *freq_table;
> +
> + cpu_dev = get_cpu_device(policy->cpu);
> + if (!cpu_dev) {
> + pr_err("failed to get cpu%d device\n", policy->cpu);
> + return -ENODEV;
> + }
> +
> + ret = dev_pm_opp_of_add_table(cpu_dev);
> + if (ret < 0) {
> + dev_err(cpu_dev, "%s: failed to add OPP table: %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = apple_soc_cpufreq_find_cluster(policy, ®_base, &info);
> + if (ret) {
> + dev_err(cpu_dev, "%s: failed to get cluster info: %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
Why do you need this ? The OPP core should be able to find this
information by itself in your case AFAIU. The OPP core will refer
"operating-points-v2 = <&pcluster_opp>" and find that the cores are
related.
> + if (ret) {
> + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret);
> + goto out_iounmap;
> + }
> +
> + ret = dev_pm_opp_get_opp_count(cpu_dev);
> + if (ret <= 0) {
> + dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
Why would this happen in your case ?
> + ret = -EPROBE_DEFER;
> + goto out_free_opp;
> + }
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + ret = -ENOMEM;
> + goto out_free_opp;
> + }
> +
> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
> + if (ret) {
> + dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
> + goto out_free_priv;
> + }
> +
> + /* Get OPP levels (p-state indexes) and stash them in driver_data */
> + for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
> + unsigned long rate = freq_table[i].frequency * 1000;
> + struct dev_pm_opp *opp = dev_pm_opp_find_freq_floor(cpu_dev, &rate);
Shouldn't you use dev_pm_opp_find_freq_exact() here ?
--
viresh
