Re: [PATCH 3/8] cpufreq: tegra186: add OPP support and set bandwidth

[Sumit Gupta <sumitg@nvidia.com>]

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:
>> On Mon, Sep 1, 2025 at 12:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> On 31-08-25, 22:33, Aaron Kling via B4 Relay wrote:
>>>> +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


>> 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.
>
>>>> +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.
>
>> But I will look at making this a bit cleaner as per this and
>> the next comment.
> Thanks.
>
>>>> +             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 ?
> I meant bandwidth here.
>
>> 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