On Fri, 2 Jul 2021 at 01:28, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> Implement attach_dev() callback of power domain drivers that initializes
> the domain's performance state. GENPD core will apply the performance
> state on the first runtime PM resume of the attached device.
>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
> drivers/soc/tegra/pmc.c | 147 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 147 insertions(+)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index f63dfb2ca3f9..ebafb818b08e 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -506,6 +506,151 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value,
> writel(value, pmc->scratch + offset);
> }
>
> +static const char * const tegra_emc_compats[] = {
> + "nvidia,tegra20-emc",
> + "nvidia,tegra30-emc",
> + NULL,
> +};
> +
> +/*
> + * This GENPD callback is used by both powergate and core domains.
> + *
> + * 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)
> +{
> + 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;
> + struct clk *clk;
> + u32 hw_version;
> + int ret;
> +
> + /*
> + * Tegra114+ SocS don't support OPP yet. But if they will get OPP
> + * support, then we want to skip OPP for older kernels to preserve
> + * compatibility of newer DTBs with older kernels.
> + */
> + if (!pmc->soc->supports_core_domain)
> + return 0;
> +
> + /*
> + * The EMC devices are a special case because we have a protection
> + * from non-EMC drivers getting clock handle before EMC driver is
> + * fully initialized. The goal of the protection is to prevent
> + * devfreq driver from getting failures if it will try to change
> + * EMC clock rate until clock is fully initialized. The EMC drivers
> + * will initialize the performance state by themselves.
> + */
> + if (of_device_compatible_match(dev->of_node, tegra_emc_compats))
> + return 0;
> +
> + clk = clk_get(dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&genpd->dev, "failed to get clk of %s: %pe\n",
> + dev_name(dev), clk);
> + return PTR_ERR(clk);
> + }
> +
> + rate = clk_get_rate(clk);
> + if (!rate) {
> + dev_err(&genpd->dev, "failed to get clk rate of %s\n",
> + dev_name(dev));
> + ret = -EINVAL;
> + goto put_clk;
> + }
> +
> + if (of_machine_is_compatible("nvidia,tegra20"))
> + hw_version = BIT(tegra_sku_info.soc_process_id);
> + else
> + hw_version = BIT(tegra_sku_info.soc_speedo_id);
> +
> + opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
> + if (IS_ERR(opp_table)) {
> + dev_err(&genpd->dev, "failed to set OPP supported HW for %s: %d\n",
> + dev_name(dev), ret);
> + ret = PTR_ERR(opp_table);
> + goto put_clk;
> + }
> +
> + ret = dev_pm_opp_of_add_table(dev);
> + if (ret) {
> + /* older DTBs that don't have OPPs will get -ENODEV here */
> + if (ret != -ENODEV)
> + dev_err(&genpd->dev, "failed to get OPP table of %s: %d\n",
> + dev_name(dev), ret);
> + else
> + ret = 0;
> +
> + goto put_supported_hw;
> + }
> +
> + /* find suitable OPP for the rate */
> + opp = dev_pm_opp_find_freq_ceil(dev, &rate);
> +
> + if (opp == ERR_PTR(-ERANGE))
> + opp = dev_pm_opp_find_freq_floor(dev, &rate);
> +
> + if (IS_ERR(opp)) {
> + dev_err(&genpd->dev, "failed to find OPP for %luHz of %s: %pe\n",
> + rate, dev_name(dev), opp);
> + ret = PTR_ERR(opp);
> + goto remove_dev_table;
> + }
> +
> + 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?
> +
> + 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?
[...]
Kind regards
Uffe
[1]
https://lkml.org/lkml/2021/7/20/99
