Re: [PATCH 1/3] cpufreq: mediatek: add support for CPU speedbin

[AngeloGioacchino Del Regno]

On 9/23/26 09:23, Roman Vivchar via B4 Relay wrote:
> From: Roman Vivchar <rva333@xxxxxxxxxxxxxx>
>
> MediaTek SoCs are often released under the same marketing name, but with
> different suffix for the speedbin. For example, mt6572m has 1 GHz limit,
> mt6572a - 1.2 GHz, mt6572w - 1.4 GHz.
>
> Add code to support per-cluster 'opp-supported-hw' property. The DT is
> expected to pass a CPU speedbin using standard 'nvmem-cells' property
> under the CPU node.
>
> Signed-off-by: Roman Vivchar <rva333@xxxxxxxxxxxxxx>
> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 50 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 052ca7cd2f4f..f15135b2898b 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -10,6 +10,7 @@
>   #include <linux/cpumask.h>
>   #include <linux/minmax.h>
>   #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
>   #include <linux/of.h>
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
> @@ -57,6 +58,7 @@ struct mtk_cpu_dvfs_info {
>   	const struct mtk_cpufreq_platform_data *soc_data;
>   	int vtrack_max;
>   	bool ccifreq_bound;
> +	int opp_token;
>   };
>   
>   static struct platform_device *cpufreq_pdev;
> @@ -381,6 +383,38 @@ static struct device *of_get_cci(struct device *cpu_dev)
>   	return &pdev->dev;
>   }
>   
> +static int mtk_cpu_parse_speedbin(struct mtk_cpu_dvfs_info *info, int cpu)
> +{
> +	struct device *cpu_dev = get_cpu_device(cpu);
> +	struct dev_pm_opp_config config = {};
> +	u32 val, opp_hw_ver;
> +	int ret;
> +
> +	ret = nvmem_cell_read_variable_le_u32(cpu_dev, "speed_grade", &val);

Let's keep consistency with cell names: for example, on the GPU side, the node
in devicetree is called "gpu-speedbin@1234", and the panfrost node has

nvmem-cells = <&gpu_speedbin>;
nvmem-cell-names = "speed-bin";

Check MT8186 and MT8188 devicetrees ;-)

> +	if (ret) {
> +		if (ret == -ENOENT)

if (ret != -ENOENT && ret != -EOPNOTSUPP)

> +			/* speedbin is optional */
> +			return 0;
> +		return dev_err_probe(cpu_dev, ret, "cpu%d: failed to read speedbin\n", cpu);
> +	}
> +
> +	/* Convert the raw value to a bitmask */
> +	if (val >= 32)
> +		return dev_err_probe(cpu_dev, -EINVAL,
> +				     "cpu%d: invalid speedbin value %u\n", cpu, val);
> +	opp_hw_ver = BIT(val);

If you look at nvmem/mtk-efuse.c there is a mtk_efuse_gpu_speedbin_pp() function
that is doing (almost) *exactly* what you're doing here.

Check if the same constraints as GPU speed binning applies to the CPU: if they do,
just allow "cpu-speedbin" as node name in the fixup function, otherwise add a new
post processing function for the CPU and act accordingly.

> +
> +	config.supported_hw = &opp_hw_ver;
> +	config.supported_hw_count = 1;
> +
> +	info->opp_token = dev_pm_opp_set_config(cpu_dev, &config);

It's way easier if you use

devm_pm_opp_set_supported_hw(dev, &opp_hw_ver, 1);

Cheers,
Angelo

> +	if (info->opp_token < 0)
> +		return dev_err_probe(cpu_dev, info->opp_token,
> +				     "cpu%d: failed to set OPP config\n", cpu);
> +
> +	return 0;
> +}
> +
>   static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   {
>   	struct device *cpu_dev;
> @@ -450,18 +484,22 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   		}
>   	}
>   
> +	ret = mtk_cpu_parse_speedbin(info, cpu);
> +	if (ret)
> +		goto out_disable_sram_reg;
> +
>   	/* Get OPP-sharing information from "operating-points-v2" bindings */
>   	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &info->cpus);
>   	if (ret) {
>   		dev_err_probe(cpu_dev, ret,
>   			"cpu%d: failed to get OPP-sharing information\n", cpu);
> -		goto out_disable_sram_reg;
> +		goto out_free_speedbin;
>   	}
>   
>   	ret = dev_pm_opp_of_cpumask_add_table(&info->cpus);
>   	if (ret) {
>   		dev_err_probe(cpu_dev, ret, "cpu%d: no OPP table\n", cpu);
> -		goto out_disable_sram_reg;
> +		goto out_free_speedbin;
>   	}
>   
>   	ret = clk_prepare_enable(info->cpu_clk);
> @@ -533,6 +571,10 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   out_free_opp_table:
>   	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
>   
> +out_free_speedbin:
> +	if (info->opp_token > 0)
> +		dev_pm_opp_clear_config(info->opp_token);
> +
>   out_disable_sram_reg:
>   	if (info->sram_reg)
>   		regulator_disable(info->sram_reg);
> @@ -573,6 +615,10 @@ static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info)
>   	clk_disable_unprepare(info->inter_clk);
>   	clk_put(info->inter_clk);
>   	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> +
> +	if (info->opp_token > 0)
> +		dev_pm_opp_clear_config(info->opp_token);
> +
>   	dev_pm_opp_unregister_notifier(info->cpu_dev, &info->opp_nb);
>   	if (info->soc_data->ccifreq_supported)
>   		put_device(info->cci_dev);