On Sun, Feb 16, 2014 at 01:39:56AM +0000, Mark Brown wrote: > On Tue, Feb 11, 2014 at 02:17:53PM +0000, Lorenzo Pieralisi wrote: > > > +According to the Server Base System Architecture document (SBSA, [3]), the > > +power states an ARM CPU can be put into are identified by the following list: > > > +1 - Running > > +2 - Idle_standby > > +3 - Idle_retention > > +4 - Sleep > > +5 - Off > > > +ARM platforms implement the power states specified in the SBSA through power > > +management schemes that allow an OS PM implementation to put the processor in > > +different idle states (which include states 1 to 4 above; "off" state is not > > +an idle state since it does not have wake-up capabilities, hence it is not > > +considered in this document). > > This is explicitly talking about SBSA - is there any restriction with > regard to non-SBSA systems? I can't think of any right now and this > seems purely informational but it might be worth mentioning that > non-SBSA systems might potentially have other states if the intention is > to allow that. It is informational, for nomenclature reasons. I do not think we have a problem here, I will check if rewording is necessary. > > +- state node > > + > > + Description: must be child of either the cpu-idle-states node or > > + a state node. > > + > > + The state node name shall be "stateN", where N = {0, 1, ...} is > > + the node number; state nodes which are siblings within a single common > > + parent node must be given a unique and sequential N value, starting > > + from 0. > > This came up with the CPU bindings as well but I'm really not convinced > that making the naming of the nodes important is great - it's not normal > for DT and it makes the usual enumeration code not work. Can we not > just have a property for state number in the node instead and allow the > name to be anything? It seems we even have the index property > already... Name can be anything now, acked. > > + - compatible > > + Usage: Required > > + Value type: <stringlist> > > + Definition: Must be "arm,cpu-idle-state". > > Do we really need this given that the location in the tree is fixed - > what would we extend it with in future? I do not think it hurts either, honestly. Unless there is a strong reason to remove it I would leave it there. > > + - index > > + Usage: Required > > + Value type: <u32> > > + Definition: It represents an idle state index, starting from 2. > > + Index 0 represents the processor state "running" > > + and index 1 represents processor mode > > + "idle_standby", entered by executing a wfi > > + instruction (SBSA,[3]); indexes 0 and 1 are > > + standard ARM states that need not be described. > > ...but other numbers are valid. Now it is sequential {0,1,....} and I defined what it means in terms of power consumption (ordering). > > + - entry-method > > + Usage: Required > > + Value type: <stringlist> > > + Definition: Describes the method by which a CPU enters the > > + idle state. This property is required and must be > > + one of: > > + > > + - "arm,psci-cpu-suspend" > > + ARM PSCI firmware interface, CPU suspend > > + method[2]. > > + > > + - "[vendor],[method]" > > + An implementation dependent string with > > + format "vendor,method", where vendor is a string > > + denoting the name of the manufacturer and > > + method is a string specifying the mechanism > > + used to enter the idle state. > > Might be worth reversing these - arm,psci-cpu-suspend is just an example > of a (hopefully very widely used) vendor method. Given that everyone is > supposed to be using PSCI might it even be worth making it the default > and the property optional? I think it is ok as it is, honestly. It is certainly not through this property that psci will be mandated, it is just a way to describe an entry method and it seems fine to me. > > + - power-state > > + Usage: See definition. > > + Value type: <u32> > > + Definition: Depends on the entry-method property value. > > + If entry-method is "arm,psci-cpu-suspend": > > + # Property is required and represents > > + psci-power-state parameter. Please refer to > > + [2] for PSCI bindings definition. > > Should we just have the entry method nodes define their own properties > for this stuff? I guess this goes back to the comment I made on some > other binding that it might be cleaner to have an explicit PSCI binding > rather than put PSCI explicitly in indiidual bindings. I added to the PSCI bindings in this series. OK, I can add something like: "Refer to entry-method bindings for the property value definition". > > + - entry-latency > > + Usage: Required > > + Value type: <prop-encoded-array> > > + Definition: u32 value representing worst case latency > > + in microseconds required to enter the idle state. > > Why is this defined as an array? For possible extensions. > > + - cache-state-retained > > + Usage: See definition > > + Value type: <none> > > + Definition: if present cache memory is retained on power down, > > + otherwise it is lost. > > Might be better to define which caches? > > > + STATE1: state1 { > > + compatible = "arm,cpu-idle-state"; > > Even if we stick with the node name being meaningful it'd be nice to > replace the STATEN here with a meaningful state name to improve > legibility. Ok I will add this to v4. Thanks, Lorenzo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-arm-kernel