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

[Ulf Hansson]

On Tue, 10 Aug 2021 at 01:56, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 09.08.2021 17:15, Ulf Hansson пишет:
> >> 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;
> +               }

Adding a new callback seems reasonable to support this.

> +
> +               default_pstate = ret;
> +
> +               if (power_on) {
> +                       ret = dev_pm_genpd_set_performance_state(dev, default_pstate);

However, this is more questionable to me.

First, I don't think we should care about whether this is "power_on"
or not. At this point, performance states are treated orthogonal to
idle states in genpd. We may decide to change that in some way, but
that deserves a different change.

Second, I don't think we should call
dev_pm_genpd_set_performance_state() from here. It's probably better
handled from the genpd callback itself, if/when needed.

That said, perhaps the new callback should just return a regular error
code and zero on success, rather than the current performance state.
See more below.

> +                       if (ret) {
> +                               dev_err(dev, "failed to set default performance state %u for PM domain %s: %d\n",
> +                                       default_pstate, pd->name, ret);
> +                               goto out;
> +                       }
> +               } else {
> +                       dev_gpd_data(dev)->rpm_pstate = default_pstate;

No, this isn't the right thing to do.

It looks like you are trying to use the ->rpm_pstate for
synchronization with runtime PM for consumer drivers. This is fragile
as it depends on the runtime PM deployment in the consumer driver. I
think you should look at ->rpm_pstate as a variable solely for
managing save/restore of the performance state for the device, during
runtime suspend/resume in genpd.

Synchronization of a vote for a performance state for a device, needs
to be managed by calling dev_pm_genpd_set_performance_state() - or by
calling an OPP function that calls it, like dev_pm_opp_set_rate(), for
example.

> +               }
> +       }
> +
>         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);

I don't see how this avoids tegra_pmc_genpd_default_perf_state() from
having to walk &genpd->child_links.

That's still an issue, right?

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

I am not saying you should change the clock rate. The current code
path that runs via devm_tegra_core_dev_init_opp_table() just calls
clk_get_rate and then dev_pm_opp_set_rate() with the current rate to
vote for the corresponding OPP level. Right?

Isn't this exactly what you want? No?

Kind regards
Uffe