On 02/11/2022 15.18, Viresh Kumar wrote:
> On 24-10-22, 13:39, Hector Martin wrote:
>> +const struct apple_soc_cpufreq_info soc_t8103_info = {
>
> static ? For other instances too.
Ack, fixed.
>> +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.
I did an informal search and the two-line form seems to be more common,
though both are in widespread use. I can change it if you want, though
it seems kind of a wash.
>> +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 ?
Done.
>
>> +
>> + 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.
Done.
>> + 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.
We have multiple clusters sharing an OPP table (e.g. the M1 Ultra has 2
e-cluster and 4 p-clusters, and duplicating OPP tables seems very
silly), so this is necessary to tell it about the subset of cores
sharing a table that are actually one domain.
>
>> + 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 ?
Good question. This came from scpi-cpufreq.c. It sounds like it doesn't
make any sense here; the error path should just error out, not defer.
I'll change it to that.
>> + 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 ?
Actually, it would seem the correct thing to do is
dev_pm_opp_find_freq_ceil, or otherwise use _floor and add 999.
dev_pm_opp_init_cpufreq_table() truncates down to kHz, so the real
frequency will always be between `rate` and `rate + 999` here. This
makes it work with frequencies that aren't a multiple of 1 kHz (we don't
have any of those but it seems broken not to support it).
- Hector
