Re: [PATCH v3 4/5] cpufreq: apple-soc: Add new driver to control Apple SoC CPU P-states

[Hector Martin]

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