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/
> 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.
>>>> +
>>>> + 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 can revert back to the previous variant, although this variant works
well too.
> 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.
