Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Dmitry Osipenko <digetx@gmail.com>]

11.11.2020 14:38, Ulf Hansson пишет:
> On Sun, 8 Nov 2020 at 13:19, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 05.11.2020 18:22, Dmitry Osipenko пишет:
>>> 05.11.2020 12:45, Ulf Hansson пишет:
>>> ...
>>>> I need some more time to review this, but just a quick check found a
>>>> few potential issues...
>>>
>>> Thank you for starting the review! I'm pretty sure it will take a couple
>>> revisions until all the questions will be resolved :)
>>>
>>>> The "core-supply", that you specify as a regulator for each
>>>> controller's device node, is not the way we describe power domains.
>>>> Instead, it seems like you should register a power-domain provider
>>>> (with the help of genpd) and implement the ->set_performance_state()
>>>> callback for it. Each device node should then be hooked up to this
>>>> power-domain, rather than to a "core-supply". For DT bindings, please
>>>> have a look at Documentation/devicetree/bindings/power/power-domain.yaml
>>>> and Documentation/devicetree/bindings/power/power_domain.txt.
>>>>
>>>> In regards to the "sync state" problem (preventing to change
>>>> performance states until all consumers have been attached), this can
>>>> then be managed by the genpd provider driver instead.
>>>
>>> I'll need to take a closer look at GENPD, thank you for the suggestion.
>>>
>>> Sounds like a software GENPD driver which manages clocks and voltages
>>> could be a good idea, but it also could be an unnecessary
>>> over-engineering. Let's see..
>>>
>>
>> Hello Ulf and all,
>>
>> I took a detailed look at the GENPD and tried to implement it. Here is
>> what was found:
>>
>> 1. GENPD framework doesn't aggregate performance requests from the
>> attached devices. This means that if deviceA requests performance state
>> 10 and then deviceB requests state 3, then framework will set domain's
>> state to 3 instead of 10.
>>
>> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L376
> 
> As Viresh also stated, genpd does aggregate the votes. It even
> performs aggregation hierarchy (a genpd is allowed to have parent(s)
> to model a topology).

Yes, I already found and fixed the bug which confused me previously and
it's working well now.

>> 2. GENPD framework has a sync() callback in the genpd.domain structure,
>> but this callback isn't allowed to be used by the GENPD implementation.
>> The GENPD framework always overrides that callback for its own needs.
>> Hence GENPD doesn't allow to solve the bootstrapping
>> state-synchronization problem in a nice way.
>>
>> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L2606
> 
> That ->sync() callback isn't the callback you are looking for, it's a
> PM domain specific callback - and has other purposes.
> 
> To solve the problem you refer to, your genpd provider driver (a
> platform driver) should assign its ->sync_state() callback. The
> ->sync_state() callback will be invoked, when all consumer devices
> have been attached (and probed) to their corresponding provider.
> 
> You may have a look at drivers/cpuidle/cpuidle-psci-domain.c, to see
> an example of how this works. If there is anything unclear, just tell
> me and I will try to help.

Indeed, thank you for the clarification. This variant works well.

>> 3. Tegra doesn't have a dedicated hardware power-controller for the core
>> domain, instead there is only an external voltage regulator. Hence we
>> will need to create a phony device-tree node for the virtual power
>> domain, which is probably a wrong thing to do.
> 
> No, this is absolutely the correct thing to do.
> 
> This isn't a virtual power domain, it's a real power domain. You only
> happen to model the control of it as a regulator, as it fits nicely
> with that for *this* SoC. Don't get me wrong, that's fine as long as
> the supply is specified only in the power-domain provider node.
> 
> On another SoC, you might have a different FW interface for the power
> domain provider that doesn't fit well with the regulator. When that
> happens, all you need to do is to implement a new power domain
> provider and potentially re-define the power domain topology. More
> importantly, you don't need to re-invent yet another slew of device
> specific bindings - for each SoC.
> 
>>
>> ===
>>
>> Perhaps it should be possible to create some hacks to work around
>> bullets 2 and 3 in order to achieve what we need for DVFS on Tegra, but
>> bullet 1 isn't solvable without changing how the GENPD core works.
>>
>> Altogether, the GENPD in its current form is a wrong abstraction for a
>> system-wide DVFS in a case where multiple devices share power domain and
>> this domain is a voltage regulator. The regulator framework is the
>> correct abstraction in this case for today.
> 
> Well, I admit it's a bit complex. But it solves the problem in a
> nicely abstracted way that should work for everybody, at least in my
> opinion.

The OPP framework supports both voltage regulator and power domain,
hiding the implementation details from drivers. This means that OPP API
usage will be the same regardless of what approach (regulator or power
domain) is used for a particular SoC.

> Although, let's not exclude that there are pieces missing in genpd or
> the opp layer, as this DVFS feature is rather new - but then we should
> just extend/fix it.

Will be nice to have a per-device GENPD performance stats.

Thierry, could you please let me know what do you think about replacing
regulator with the power domain? Do you think it's a worthwhile change?

The difference in comparison to using voltage regulator directly is
minimal, basically the core-supply phandle is replaced is replaced with
a power-domain phandle in a device tree.

The only thing which makes me feel a bit uncomfortable is that there is
no real hardware node for the power domain node in a device-tree.