Re: [PATCH v3 02/15] dt/bindings: Update binding for PM domain idle states

[Brendan Jackman <brendan.jackman@arm.com>]

Hi Lina,

Apologies, I sent this reply before and automatically included an "IMPORTANT
NOTICE" footer, please disregard that email, here's the same thing without the
footer.

On Thu, Aug 11, 2016 at 03:10:23PM -0600, Lina Iyer wrote:
> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote:
> >
> >
> >On 10/08/16 17:40, Lina Iyer wrote:
> >>Hi Sudeep,
> >>
> >>On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote:
> >>>Hi Lina,
> >>>
> >>>I have few concerns mainly due to the lack of description and not the
> >>>binding per say.
> >
> >[...]
> >
> >>It is pretty clear that CPUs cannot not define the domain idle states.
> >>Domains define their own idle states. Just as you mention above. CPU is
> >>just a single component in its domain. There may be other devices like
> >>PMUs, Coresights etc that also may have a say in the idle state the
> >>domain may be put in, when the devices are idle. As such, adding domain
> >>idle states to the CPU's idle state property is not appropriate.
> >>
> >
> >No I am not saying we need to add domain idle states to the CPU's idle
> >state property. I am saying we need to remove cpu-idle-states or ignore
> >it when PD is present. And get all the idle state information for PD.
> >
> >I am objecting the split we are creating across CPU and higher level
> >power domains. And this binding document is incomplete as it skips all
> >those details. We just need PD handle in CPU and no idle state
> >information there. Create PD hierarchy and have all idle state
> >information at one place.
> >
> Let me think about this a bit and see what I can come up with.
>
> >>Our kernel has runtime PM for devices and then there is CPUidle, both
> >>are diverging without one knowing about the other. We have to start
> >>unifying them inorder to have better holistic power management in the
> >>SoC. To that regard, we have to start imagining CPUs as just another
> >>device, albeit a special device. But for our purposes in determining
> >>domain idle state, it will just be a device attached to the domain.
> >>
> >
> >Absolutely agree on that. No arguments. I am asking to go a step ahead
> >to include even cpu/core level power domains not just cluster/higher
> >level domains.
> >
> >>>We need to have all the idle state information at one place and in this
> >>>case PD seems more appropriate instead of splitting them across.
> >>>
> >>That approach isn't correct. Where will we put the idle states of other
> >>devices that are also part of the domain? We are thinking about a model,
> >>where every device defines its own idle states and we define
> >>relationships between those idle states and their parents' idle states.
> >
> >Yes I understand. You confused me here. Won't that be one-to-one
> >relationship ? If not, how is that dealt in the current bindings ?
> >
> >>Ofcourse, devices don't have idle states today, but that is something we
> >>have been pondering over.
> >>
> >
> >Yes we these binding should be easily extensible, I don't see any issue.
> >
> >>>We can also keep the code clean and not break compatibility. Whenever
> >>>both PD and CPU contains idle-states, PD must take precedence.
> >>>
> >>Why?
> >>The CPU and PD states are orthogonal. While the PD state is dependent on
> >>the CPU state, the latter is not true. Devices determine their own
> >>states. Based on the individual device states, we then determine the
> >>state of the parent and bubble up on the hierarchy.
> >>
> >
> >I may be missing something. Now with your example in the binding, if
> >another device shares the cluster PD, can it have different idle states?
> >If so how does it map ?
> >
> >
> >In general whatever binding we come up must not just address OS
> >coordinated mode. Also I was thinking to have better coverage in
> >the description by having a bit more complex system like:
> >
> >cluster0
> >	CLUSTER_RET(Retention)
> >	CLUSTER_PG(Power Gate)
> >	core0
> >		CORE_RET
> >		CORE_PG
> >	core1
> >		CORE_RET
> >		CORE_PG
> >
> >cluster1
> >	CLUSTER_RET
> >	CLUSTER_PG
> >	core0
> >		CORE_RET
> >		CORE_PG
> >	core1
> >		CORE_RET
> >		CORE_PG
> >
> >Platform Co-ordinate supports the following states and we should
> >be able to determine that from the binding:
> >
> >CORE_RET
> >CORE_PG
> >CORE_RET + CLUSTER_RET
>
> The problem that we have to sove here is knowing that CORE_RET +
> CLUSTER_PG (hypothetically) an invalid combination. Kevin and
> I debated it in the earlier RFC and we dont have a good way to solve
> this generically for all devices.
>


This is interesting. I had been working on the assumption that a parent
power domain cannot enter any idle state until its children were all in
their deepest idle state. I now realise that it's easy to imagine
platforms where this isn't the case.

However, I don't understand how your current bindings solve this issue
and why using domain-power-states for all states (i.e. ignoring
cpu-idle-states and putting CPU idle states in the domain-idle-states of
a per-CPU power domain - I believe this is what Sudeep is suggesting)
makes it any more difficult.

Could you link to this previous discussion you mentioned? I'm having
trouble finding it (R.I.P Gmane).

> >CORE_PG + CLUSTER_RET
> >CORE_PG + CLUSTER_PG
> >

Cheers,
Brendan