On Thu, Nov 12, 2020 at 10:57:27PM +0300, Dmitry Osipenko wrote:

11.11.2020 14:38, Ulf Hansson пишет:

quoted

On Sun, 8 Nov 2020 at 13:19, Dmitry Osipenko [off-list ref] wrote:

quoted

05.11.2020 18:22, Dmitry Osipenko пишет:

quoted

05.11.2020 12:45, Ulf Hansson пишет:
...

quoted

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

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

===

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.

quoted

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.

These new power-domain handles would have to be added to devices that
potentially already have a power-domain handle, right? Isn't that going
to cause issues? I vaguely recall that we already have multiple power
domains for the XUSB controller and we have to jump through extra hoops
to make that work.

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.

Could we anchor the new power domain at the PMC for example? That would
allow us to avoid the "virtual" node. On the other hand, if we were to
use a regulator, we'd be adding a node for that, right? So isn't this
effectively going to be the same node if we use a power domain? Both
software constructs are using the same voltage regulator, so they should
be able to be described by the same device tree node, shouldn't they?

Thierry