Re: [PATCH v7 02/37] soc/tegra: pmc: Implement attach_dev() of power domain drivers

On Wed, 4 Aug 2021 at 23:17, Dmitry Osipenko [off-list ref] wrote:

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

quoted

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

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.

quoted

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/

Thanks, this looks sane to me!

quoted

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.

Right. We may need some more thinking around this, if needed at all (see below).

quoted

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 am not against doing this in a central place, like $subject patch
suggests. As a matter of fact, it makes perfect sense to me.

However, what I am concerned about, is that you require to use genpd
internal data structures to do it. I think we should try to avoid
that.

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

I looked at that code and in that path we end up calling
dev_pm_opp_set_rate(), after it has initialized the opp table for the
device.

Rather than doing the OF parsing above to find out the current state
for the device, why can't you just call dev_pm_opp_set_rate() to
initialize a proper vote instead?

quoted

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.

Alright, this makes sense to me as well.

Kind regards
Uffe