Re: [PATCH RFC 2/2] Documentation: arm: define DT C-states bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, Dec 04, 2013 at 03:20:10PM +0000, Dave Martin wrote:
> On Mon, Dec 02, 2013 at 04:20:05PM +0000, Lorenzo Pieralisi wrote:
> > ARM based platforms implement a variety of power management schemes that
> > allow processors to enter at run-time low-power states, aka C-states
> > in ACPI jargon. The parameters defining these C-states vary on a per-platform
> > basis forcing the OS to hardcode the state parameters in platform
> > specific static tables whose size grows as the number of platforms supported
> > in the kernel increases and hampers device drivers standardization.
> > 
> > Therefore, this patch aims at standardizing C-state device tree bindings for
> > ARM platforms. Bindings define C-state parameters inclusive of entry methods
> > and state latencies, to allow operating systems to retrieve the
> > configuration entries from the device tree and initialize the related
> > power management drivers, paving the way for common code in the kernel
> > to deal with power states and removing the need for static data in current
> > and previous kernel versions.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > +
> 
> [...]
> 
> > +	- cpus
> 
> Because cpus is really a topology subtree, it might be good to have
> different names.
> 
> To avoid confusion with Mark's affinity properties, maybe a different
> word would be preferable instead of "affinity".
> 
> Maybe "topology" instead of "cpus", and "affected-topology" instead of
> "affinity"?
> 
> 
> cpufreq also has its concepts of "related" and "affected" cpus, which
> tries to describe something similar (in all honesty, I always struggle
> to remember which is which ... but if we were consistent with it, that
> might help).

Yes, I will try to come up with something clearer.

> > +		Usage: Optional
> > +		Value type: <phandle>
> > +		Definition: If defined, the phandle points to a node in the
> > +			    cpu-map[2] representing all CPUs on which C-state
> > +			    is valid. If not present or system is UP, the
> > +			    C-state has to be considered valid for all CPUs in
> > +			    the system.
> > +
> > +	- affinity
> > +		Usage: Optional
> > +		Value type: <phandle>
> > +		Definition: If defined, phandle points to a node in the
> > +			    cpu-map[2] that represents all CPUs that are
> > +			    affected (ie share) by the C-state and have to
> > +			    be coordinated on C-state entry/exit. If not
> > +			    present or system is UP, the C-state is local to
> > +			    a CPU and need no coordination (ie it is a CPU
> > +			    state, that does not require coordination with
> > +			    other CPUs). If present, the affinity property
> > +			    must contain a phandle to a cpu-map node that
> > +			    represents a subset, possibly inclusive of the
> > +			    CPUs described through the cpus property.
> 
> Can you elaborate on how cpus and affinity might be different?

I was referring to:

- cpus -> processor type (eg valid on A15 or A7, or different implementations
  of the same processor in the same chip)
- affinity -> power domain (a subset of the cpus that require coordination)

Nowadays the distinction does not make much sense (I hardly see a power
state valid on eg A15 clusters [cpus], where just a subset of its cpus need to
be coordinated [affinity] - might be if other levels of caches are added or
if you have multiple clusters of the same CPU type with different power
states capabilities).

I think this deserves more attention, and probably adding power domain
information can remove this mumbo jumbo, there is a scary level of
duplicated information in there.

> The statement about "having to be coordainted" also feels a bit vague,
> though I'm not sure how much we can usefully say here.
> 
> If we describe power domains more explicitly it might help with this,
> because that could bring some description of what needs to be
> coordinated.

Yes, see above.

> > +	- power-depth
> > +		Usage: Required
> > +		Value type: <u32>
> > +		Definition: Integer value, starting from 2 (value 0 meaning
> > +			    running and value 1 representing power depth of
> > +			    wfi (C1)), that defines the level of depth of a
> > +			    power state.
> > +			    The system denotes power states with different
> > +			    depths, an increasing value meaning less power
> > +			    consumption and might involve powering down more
> > +			    components.  Devices that are affected by
> > +			    C-states entry must define the maximum power
> > +			    depth supported in their respective device tree
> > +			    bindings so that OSPM can take decision on how
> > +			    to handle the device in question when the C-state
> > +			    is entered. All devices (per-CPU or external) with
> > +			    a power depth lower than the one defined in the
> > +			    C-state entry stop operating when the C-state
> > +			    is entered and action is required by OSPM to
> > +			    guarantee their logic and memory content is saved
> > +			    restored to guarantee proper functioning.
> 
> Any reason to use numbers instead of strings?
> 
> Strings make the DT more readable ... we would presumably only have to
> parse this information once, so it shouldn't be an overhead, unless there
> are hundreds of C-state nodes.

Yes, but it is supposed to be a unique identifier in the entire system.
Ok, we can create a list of strings denoting power depths, as long as
they are "standard" fine by me, but I think that a number would be
easier to use, even though honestly I think it is better to use power
domains and get rid of this property altogether.

Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux