Re: [PATCH v6 51/52] PM / devfreq: tegra30: Support interconnect and OPPs from device-tree

[Dmitry Osipenko <digetx@gmail.com>]

01.11.2020 17:39, Chanwoo Choi пишет:
> Hi Dmitry,
> 
> On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> This patch moves ACTMON driver away from generating OPP table by itself,
>> transitioning it to use the table which comes from device-tree. This
>> change breaks compatibility with older device-trees in order to bring
>> support for the interconnect framework to the driver. This is a mandatory
>> change which needs to be done in order to implement interconnect-based
>> memory DVFS. Users of legacy device-trees will get a message telling that
>> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
>> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
>>
>> Tested-by: Peter Geis <pgwipeout@gmail.com>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
...
>>  static int tegra_devfreq_get_dev_status(struct device *dev,
>> @@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>         stat->private_data = tegra;
>>
>>         /* The below are to be used by the other governors */
>> -       stat->current_frequency = cur_freq;
>> +       stat->current_frequency = cur_freq * KHZ;
> 
> I can't find any change related to the frequency unit on this patch.
> Do you fix the previous bug of the frequency unit?

Previously, OPP entries that were generated by the driver used KHz
units. Now, OPP entries are coming from a device-tree and they have Hz
units. This driver operates with KHz internally, hence we need to
convert the units now.

>>
>>         actmon_dev = &tegra->devices[MCALL];
>>
>> @@ -705,7 +693,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
>>                 target_freq = max(target_freq, dev->target_freq);
>>         }
>>
>> -       *freq = target_freq;
>> +       *freq = target_freq * KHZ;
> 
> ditto.
> 
>>
>>         return 0;
>>  }
>> @@ -773,13 +761,22 @@ static struct devfreq_governor tegra_devfreq_governor = {
>>
>>  static int tegra_devfreq_probe(struct platform_device *pdev)
>>  {
>> +       u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
>>         struct tegra_devfreq_device *dev;
>>         struct tegra_devfreq *tegra;
>> +       struct opp_table *opp_table;
>>         struct devfreq *devfreq;
>>         unsigned int i;
>>         long rate;
>>         int err;
>>
>> +       /* legacy device-trees don't have OPP table and must be updated */
>> +       if (!device_property_present(&pdev->dev, "operating-points-v2")) {
>> +               dev_err(&pdev->dev, "OPP table not found, cannot continue\n");
>> +               dev_err(&pdev->dev, "please update your device tree\n");
>> +               return -ENODEV;
>> +       }
> 
> As you mentioned, it breaks the old dtb. I have no objection to improving
> the driver. Instead, you need confirmation from the Devicetree maintainer.

Older DTBs will continue to work, but devfreq driver won't. We already
did this for other Tegra drivers before (CPUFREQ driver for example) and
Rob Herring confirmed that there is no problem here.

> And,
> I recommend that you use dev_pm_opp_of_get_opp_desc_node(&pdev->dev)
> to check whether a device contains opp-table or not.

I'm not sure what are the benefits, this will make code less
expressive/readable and we will need to add extra of_node_put(), which
device_property_present() handles for us.

Could you please give the rationale?

>> +
>>         tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>>         if (!tegra)
>>                 return -ENOMEM;
>> @@ -821,11 +818,29 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>                 return err;
>>         }
>>
>> +       tegra->opp_table = dev_pm_opp_get_opp_table(&pdev->dev);
>> +       if (IS_ERR(tegra->opp_table))
>> +               return dev_err_probe(&pdev->dev, PTR_ERR(tegra->opp_table),
>> +                                    "Failed to prepare OPP table\n");
> 
> As I knew, each device can contain the opp_table on devicetree.
> It means that opp_table has not depended to another device driver.
> Did you see this exception case with EPROBE_DEFER error?

OPP core will try to grab the clock reference for the table and it may
cause EPROBE_DEFER. Although, it shouldn't happen here because we have
devm_clk_get() before the get_opp_table(), hence seems the deferred
probe indeed shouldn't happen in this case.