On 03/09/25 10:31, Viresh Kumar wrote:

External email: Use caution opening links or attachments


+Sumit

On 02-09-25, 12:21, Aaron Kling wrote:

quoted

On Mon, Sep 1, 2025 at 12:53 AM Viresh Kumar [off-list ref] wrote:

quoted

On 31-08-25, 22:33, Aaron Kling via B4 Relay wrote:

quoted

+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 ?

I'm not very familiar with the opp system. If I read correctly,
dev_pm_opp_set_rate() will round to the closest rate while this code
will fail if the exact rate isn't found. This code is based on the
existing tegra194-cpufreq driver. And I'm unsure if this was done for
a reason.

Sumit, any explanation for this ?

dev_pm_opp_set_rate() is additionally checking clock. In Tegra194/234
the clock is handled by BPMP-FW and CPU nodes don't have clocks property.
So, the clk_round_rate() API causes NULL pointer. I see same in 
Tegra186/194 dtsi.
Tegra210 and previous SoCs have clocks property in CPU node.

Thank you,
Sumit Gupta

quoted

I have seen unexpected rates returned from clk_round_rate in
the development of this topic, so that could be related.

Right, but we should end up configuring a valid rate from the OPP
table.

quoted

quoted

quoted

+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.

I'd prefer not to deviate too much from the tegra194-cpufreq code this
is based on, so the drivers can be more easily kept in parity to each
other.

I am not sure if that is really important here. The kernel normally
contains code in this format:

if (err)
         return;

keep-working;

If you want both the drivers to have similar code, then maybe that
code should be moved to another file and used by both. But we
shouldn't keep them same when we feel that we can do better.

quoted

But I will look at making this a bit cleaner as per this and
the next comment.

Thanks.

quoted

quoted

quoted

+             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 ?

quoted

+                     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 ?

I meant bandwidth here.

quoted

To my knowledge, this does not change any clocks. The intent here is
to prime the interconnect data. In the pre-req series, there's a
change that sets all clocks to max frequency during probe. Then my use
case involves setting performance governor by default on some boots.
During testing, I noticed that the interconnect data provided by this
driver was all zeroes. Which led me to notice that set_bw is only
called when the target frequency changes. Which wasn't happening
because clocks were already set to max. If my understanding here is
wrong or there's a better way to handle this, I will fix it.

There are a lot of synchronization issues here because we are trying
to set clk and bw separately. I guess other variables, like regulator,
level, etc. (if used) will also be out of sync here.

I think the right way to fix this would be to call set-opp for the
device, so all the variables are configured properly.

--
viresh