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

09.08.2021 17:15, Ulf Hansson пишет:

quoted

quoted

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.

Alright, what do you think about this:

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a934c679e6ce..5faed62075e9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c

@@ -2669,12 +2669,37 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
        dev->pm_domain->detach = genpd_dev_pm_detach;
        dev->pm_domain->sync = genpd_dev_pm_sync;

+       if (pd->default_performance_state) {
+               unsigned int default_pstate;
+
+               ret = pd->default_performance_state(pd, dev);
+               if (ret < 0) {
+                       dev_err(dev, "failed to get default performance state for PM domain %s: %d\n",
+                               pd->name, ret);
+                       goto out;
+               }

+               }
+       }
+
        if (power_on) {
                genpd_lock(pd);
                ret = genpd_power_on(pd, 0);
                genpd_unlock(pd);
        }

+out:
        if (ret)
                genpd_remove_device(pd, dev);

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 81d1f019fa0c..9efb55f52462 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c

@@ -518,15 +518,14 @@ static const char * const tegra_emc_compats[] = {
  * We retrieve clock rate of the attached device and initialize domain's
  * performance state in accordance to the clock rate.
  */
-static int tegra_pmc_pd_attach_dev(struct generic_pm_domain *genpd,
-                                  struct device *dev)
+static int tegra_pmc_genpd_default_perf_state(struct generic_pm_domain *genpd,
+                                             struct device *dev)
 {
-       struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
        struct opp_table *opp_table, *pd_opp_table;
        struct generic_pm_domain *core_genpd;
        struct dev_pm_opp *opp, *pd_opp;
-       unsigned long rate, state;
        struct gpd_link *link;
+       unsigned long rate;
        struct clk *clk;
        u32 hw_version;
        int ret;

@@ -633,8 +632,7 @@ static int tegra_pmc_pd_attach_dev(struct generic_pm_domain *genpd,
         * 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;
+       ret = pm_genpd_opp_to_performance_state(&core_genpd->dev, pd_opp);

        dev_pm_opp_put(pd_opp);

 put_pd_opp_table:

@@ -1383,7 +1381,7 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)

        pg->id = id;
        pg->genpd.name = np->name;
-       pg->genpd.attach_dev = tegra_pmc_pd_attach_dev;
+       pg->genpd.default_performance_state = tegra_pmc_genpd_default_perf_state;
        pg->genpd.power_off = tegra_genpd_power_off;
        pg->genpd.power_on = tegra_genpd_power_on;
        pg->pmc = pmc;

@@ -1500,7 +1498,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
                return -ENOMEM;

        genpd->name = np->name;
-       genpd->attach_dev = tegra_pmc_pd_attach_dev;
+       genpd->default_performance_state = tegra_pmc_genpd_default_perf_state;
        genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
        genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 21a0577305ef..cd4867817ca5 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h

@@ -143,6 +143,8 @@ struct generic_pm_domain {
                          struct device *dev);
        void (*detach_dev)(struct generic_pm_domain *domain,
                           struct device *dev);
+       int (*default_performance_state)(struct generic_pm_domain *domain,
+                                        struct device *dev);
        unsigned int flags;             /* Bit field of configs for genpd */
        struct genpd_power_state *states;
        void (*free_states)(struct genpd_power_state *states,

quoted

quoted

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?

For some devices clock rate is either preset by bootloader, or by clk driver, or by assigned-clocks in a device-tree. And then I don't see what's the difference in comparison to initialization for the current rate.

For some devices, like memory controller, we can't just change the clock rate because it's a complex procedure and some boards will use fixed rate, but the power vote still must be initialized.