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

[Chanwoo Choi]

Hi Dmitry,

On Mon, Nov 2, 2020 at 12:23 AM Dmitry Osipenko <digetx at gmail.com> wrote:
>
> 01.11.2020 17:39, Chanwoo Choi пишет:
> > Hi Dmitry,
> >
> > On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <digetx at 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 at gmail.com>
> >> Tested-by: Nicolas Chauvet <kwizart at gmail.com>
> >> Signed-off-by: Dmitry Osipenko <digetx at 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.

OK. Thanks.

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

OK.

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

IMO, 'operating-points-v2' word was defined on OPP core. I think that
the external user
of OPP better to use the public helper function instead of using the
interval definition
or value of OPP core directly. Basically, I prefer the provided helper
function if there.
But, it is not critical and doesn't affect the operation. If you want
to keep, it is ok.

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

It is my missing point. If there, you're right.
Could you provide the code point to check the clock reference on the OPP core?

-- 
Best Regards,
Chanwoo Choi