lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250901055322.eorgaa3sycydjrrj@vireshk-i7>
Date: Mon, 1 Sep 2025 11:23:22 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: webgeek1234@...il.com
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-tegra@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 3/8] cpufreq: tegra186: add OPP support and set bandwidth

On 31-08-25, 22:33, Aaron Kling via B4 Relay wrote:
> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
> index bd94beebc4cc2fe6870e13ca55343cedb9729e99..f0abb44e2ed00a301161565e4c4f62cfed4a5814 100644
> --- a/drivers/cpufreq/tegra186-cpufreq.c
> +++ b/drivers/cpufreq/tegra186-cpufreq.c
> @@ -18,6 +18,7 @@
>  #define EDVD_CORE_VOLT_FREQ_F_SHIFT	0
>  #define EDVD_CORE_VOLT_FREQ_F_MASK	0xffff
>  #define EDVD_CORE_VOLT_FREQ_V_SHIFT	16
> +#define KHZ				1000

Can reuse:

include/linux/units.h:#define HZ_PER_KHZ                1000UL

> +static int tegra_cpufreq_set_bw(struct cpufreq_policy *policy, unsigned long freq_khz)
> +{
> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
> +	struct dev_pm_opp *opp;
> +	struct device *dev;
> +	int ret;
> +
> +	dev = get_cpu_device(policy->cpu);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	opp = dev_pm_opp_find_freq_exact(dev, freq_khz * KHZ, true);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +
> +	ret = dev_pm_opp_set_opp(dev, opp);

Won't it be easier to use dev_pm_opp_set_rate() instead ?

> +	if (ret)
> +		data->icc_dram_bw_scaling = false;
> +
> +	dev_pm_opp_put(opp);

The OPP core supports scope based cleanup helpers now, maybe use them
to remove all these put calls.

> +	return ret;
> +}
> +
> +static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
> +					    struct cpufreq_frequency_table *bpmp_lut,
> +					    struct cpufreq_frequency_table **opp_table)
> +{
> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
> +	struct cpufreq_frequency_table *freq_table = NULL;
> +	struct cpufreq_frequency_table *pos;
> +	struct device *cpu_dev;
> +	struct dev_pm_opp *opp;
> +	unsigned long rate;
> +	int ret, max_opps;
> +	int j = 0;
> +
> +	cpu_dev = get_cpu_device(policy->cpu);
> +	if (!cpu_dev) {
> +		pr_err("%s: failed to get cpu%d device\n", __func__, policy->cpu);
> +		return -ENODEV;
> +	}
> +
> +	/* Initialize OPP table mentioned in operating-points-v2 property in DT */
> +	ret = dev_pm_opp_of_add_table_indexed(cpu_dev, 0);
> +	if (!ret) {

If you handle the error case here, then the below can move out of the
if/else block.

> +		max_opps = dev_pm_opp_get_opp_count(cpu_dev);
> +		if (max_opps <= 0) {
> +			dev_err(cpu_dev, "Failed to add OPPs\n");
> +			return max_opps;
> +		}
> +
> +		/* Disable all opps and cross-validate against LUT later */
> +		for (rate = 0; ; rate++) {

Maybe using while(1) would be more readable ?

> +			opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
> +			if (IS_ERR(opp))
> +				break;
> +
> +			dev_pm_opp_put(opp);
> +			dev_pm_opp_disable(cpu_dev, rate);
> +		}
> +	} else {
> +		dev_err(cpu_dev, "Invalid or empty opp table in device tree\n");
> +		data->icc_dram_bw_scaling = false;
> +		return ret;
> +	}
> +
> +	freq_table = kcalloc((max_opps + 1), sizeof(*freq_table), GFP_KERNEL);
> +	if (!freq_table)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Cross check the frequencies from BPMP-FW LUT against the OPP's present in DT.
> +	 * Enable only those DT OPP's which are present in LUT also.
> +	 */
> +	cpufreq_for_each_valid_entry(pos, bpmp_lut) {
> +		opp = dev_pm_opp_find_freq_exact(cpu_dev, pos->frequency * KHZ, false);
> +		if (IS_ERR(opp))
> +			continue;
> +
> +		dev_pm_opp_put(opp);
> +
> +		ret = dev_pm_opp_enable(cpu_dev, pos->frequency * KHZ);
> +		if (ret < 0)
> +			return ret;
> +
> +		freq_table[j].driver_data = pos->driver_data;
> +		freq_table[j].frequency = pos->frequency;
> +		j++;
> +	}
> +
> +	freq_table[j].driver_data = pos->driver_data;
> +	freq_table[j].frequency = CPUFREQ_TABLE_END;
> +
> +	*opp_table = &freq_table[0];
> +
> +	dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
> +
> +	tegra_cpufreq_set_bw(policy, freq_table[j - 1].frequency);

Maybe a comment on why exactly you are changing the freq here ?

> +
> +	return ret;
> +}
> +
>  static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
>  	unsigned int cluster = data->cpus[policy->cpu].bpmp_cluster_id;
> +	struct cpufreq_frequency_table *freq_table;
> +	struct cpufreq_frequency_table *bpmp_lut;
>  	u32 cpu;
> +	int ret;
>  
> -	policy->freq_table = data->clusters[cluster].table;
>  	policy->cpuinfo.transition_latency = 300 * 1000;
>  	policy->driver_data = NULL;
>  
> @@ -85,6 +191,20 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
>  			cpumask_set_cpu(cpu, policy->cpus);
>  	}
>  
> +	bpmp_lut = data->clusters[cluster].bpmp_lut;
> +
> +	if (data->icc_dram_bw_scaling) {
> +		ret = tegra_cpufreq_init_cpufreq_table(policy, bpmp_lut, &freq_table);
> +		if (!ret) {
> +			policy->freq_table = freq_table;
> +			return 0;
> +		}
> +	}
> +
> +	data->icc_dram_bw_scaling = false;
> +	policy->freq_table = bpmp_lut;
> +	pr_info("OPP tables missing from DT, EMC frequency scaling disabled\n");
> +
>  	return 0;
>  }
>  
> @@ -102,6 +222,10 @@ static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
>  		writel(edvd_val, data->regs + edvd_offset);
>  	}
>  
> +	if (data->icc_dram_bw_scaling)
> +		tegra_cpufreq_set_bw(policy, tbl->frequency);
> +
> +
>  	return 0;
>  }
>  
> @@ -136,7 +260,7 @@ static struct cpufreq_driver tegra186_cpufreq_driver = {
>  	.init = tegra186_cpufreq_init,
>  };
>  
> -static struct cpufreq_frequency_table *init_vhint_table(
> +static struct cpufreq_frequency_table *tegra_cpufreq_bpmp_read_lut(
>  	struct platform_device *pdev, struct tegra_bpmp *bpmp,
>  	struct tegra186_cpufreq_cluster *cluster, unsigned int cluster_id,
>  	int *num_rates)
> @@ -231,6 +355,7 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
>  {
>  	struct tegra186_cpufreq_data *data;
>  	struct tegra_bpmp *bpmp;
> +	struct device *cpu_dev;
>  	unsigned int i = 0, err, edvd_offset;
>  	int num_rates = 0;
>  	u32 edvd_val, cpu;
> @@ -256,9 +381,9 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
>  	for (i = 0; i < TEGRA186_NUM_CLUSTERS; i++) {
>  		struct tegra186_cpufreq_cluster *cluster = &data->clusters[i];
>  
> -		cluster->table = init_vhint_table(pdev, bpmp, cluster, i, &num_rates);
> -		if (IS_ERR(cluster->table)) {
> -			err = PTR_ERR(cluster->table);
> +		cluster->bpmp_lut = tegra_cpufreq_bpmp_read_lut(pdev, bpmp, cluster, i, &num_rates);
> +		if (IS_ERR(cluster->bpmp_lut)) {
> +			err = PTR_ERR(cluster->bpmp_lut);
>  			goto put_bpmp;
>  		} else if (!num_rates) {
>  			err = -EINVAL;
> @@ -267,7 +392,7 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
>  
>  		for (cpu = 0; cpu < ARRAY_SIZE(tegra186_cpus); cpu++) {
>  			if (data->cpus[cpu].bpmp_cluster_id == i) {
> -				edvd_val = cluster->table[num_rates - 1].driver_data;
> +				edvd_val = cluster->bpmp_lut[num_rates - 1].driver_data;
>  				edvd_offset = data->cpus[cpu].edvd_offset;
>  				writel(edvd_val, data->regs + edvd_offset);
>  			}
> @@ -276,6 +401,19 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
>  
>  	tegra186_cpufreq_driver.driver_data = data;
>  
> +	/* Check for optional OPPv2 and interconnect paths on CPU0 to enable ICC scaling */
> +	cpu_dev = get_cpu_device(0);
> +	if (!cpu_dev) {
> +		err = -EPROBE_DEFER;
> +		goto put_bpmp;
> +	}
> +
> +	if (dev_pm_opp_of_get_opp_desc_node(cpu_dev)) {
> +		err = dev_pm_opp_of_find_icc_paths(cpu_dev, NULL);
> +		if (!err)
> +			data->icc_dram_bw_scaling = true;
> +	}
> +
>  	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
>  
>  put_bpmp:
> 
> -- 
> 2.50.1
> 

-- 
viresh

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ