04.08.2021 12:59, Ulf Hansson пишет:

On Mon, 2 Aug 2021 at 20:23, Dmitry Osipenko [off-list ref] wrote:

quoted

02.08.2021 17:48, Ulf Hansson пишет:
...

quoted

quoted

+       if (!list_empty(&genpd->child_links)) {
+               link = list_first_entry(&genpd->child_links, struct gpd_link,
+                                       child_node);
+               core_genpd = link->parent;
+       } else {
+               core_genpd = genpd;
+       }

This looks a bit odd to me. A genpd provider shouldn't need to walk
these links as these are considered internals to genpd. Normally this
needs lockings, etc.

Why exactly do you need this?

We have a chain of PMC domain -> core domain, both domains are created
and liked together by this PMC driver. Devices are attached to either
PMC domain or to core domain. PMC domain doesn't handle the performance
changes, performance requests go down to the core domain.

Did I get this right? The core domain is the parent to the PMC domain?

You got this right.

quoted

This is needed in order to translate the device's OPP into performance
state of the core domain, based on the domain to which device is attached.

So, the PMC domain doesn't have an OPP table associated with it, but
some of its attached devices may still have available OPPs, which
should be managed through the parent domain (core domain). Correct?

Yes, the OPPs are specified only for the core domain.

Is there a DT patch in the series that I can look at that shows how
this is encoded?

See patches which are adding domains and OPPs to DTs:

https://patchwork.ozlabs.org/project/linux-tegra/patch/20210701232728.23591-34-digetx@gmail.com/

https://patchwork.ozlabs.org/project/linux-tegra/patch/20210701232728.23591-35-digetx@gmail.com/

Hmm, I have the feeling that we should try to manage in some generic
way in genpd, rather than having to deal with it here.

Still it requires a platform-specific knowledge. It could be some new
genpd hook for the initialization. But I don't know what other platforms
may want to initialize, so it's not clear to me how to make it generic.

quoted

quoted

quoted

+
+       pd_opp_table = dev_pm_opp_get_opp_table(&core_genpd->dev);
+       if (IS_ERR(pd_opp_table)) {
+               dev_err(&genpd->dev, "failed to get OPP table of %s: %pe\n",
+                       dev_name(&core_genpd->dev), pd_opp_table);
+               ret = PTR_ERR(pd_opp_table);
+               goto put_dev_opp;
+       }
+
+       pd_opp = dev_pm_opp_xlate_required_opp(opp_table, pd_opp_table, opp);
+       if (IS_ERR(pd_opp)) {
+               dev_err(&genpd->dev,
+                       "failed to xlate required OPP for %luHz of %s: %pe\n",
+                       rate, dev_name(dev), pd_opp);
+               ret = PTR_ERR(pd_opp);
+               goto put_pd_opp_table;
+       }
+
+       /*
+        * The initialized state will be applied by GENPD core on the first
+        * RPM-resume of the device.  This means that drivers don't need to
+        * explicitly initialize performance state.
+        */
+       state = pm_genpd_opp_to_performance_state(&core_genpd->dev, pd_opp);
+       gpd_data->rpm_pstate = state;

Could the above be replaced with Rajendra's suggestion [1], which
changes genpd to internally during attach, to set a default
performance state when there is a "required-opp" specified in the
device  node?

It's not a "static" performance level here, but any level depending on
h/w state left from bootloader and etc. The performance level
corresponds to the voltage of the core domain, hence we need to
initialize the voltage vote before device is resumed.

Why not let the driver deal with this instead? It should be able to
probe its device, no matter what state the bootloader has put the
device into.

To me, it sounds like a call to dev_pm_genpd_set_performance_state()
(perhaps via dev_pm_opp_set_opp() or dev_pm_opp_set_rate()) from the
driver itself, should be sufficient?

We did that in a previous versions of this series where drivers were
calling devm_tegra_core_dev_init_opp_table() helper during the probe to
initialize performance state of the domain. Moving OPP state
initialization into central place made drivers cleaner by removing the
boilerplate code.

I can revert back to the previous variant, although this variant works
well too.

I understand that it means the domain may change the OPP during boot,
without respecting a vote for a device that has not been probed yet.
But is there a problem with this?

Domains themselves don't change OPP, there is no problem with that. The
point is to have cleaner code in the drivers.