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

[Ulf Hansson]

On Wed, 4 Aug 2021 at 23:17, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 04.08.2021 12:59, Ulf Hansson пишет:
> > On Mon, 2 Aug 2021 at 20:23, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
> >>
> >> 02.08.2021 17:48, Ulf Hansson пишет:
> >> ...
> >>>> +       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.
>
> >>
> >> 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@xxxxxxxxx/
>
> https://patchwork.ozlabs.org/project/linux-tegra/patch/20210701232728.23591-35-digetx@xxxxxxxxx/

Thanks, this looks sane to me!

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

>
> >>>> +
> >>>> +       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?

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