| Message ID | 1477409199-52182-4-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 25/10/16 16:26, Lina Iyer wrote: > Update domain-idle-state binding to use "domain-idle-state" compatible > from Documentation/devicetree/bindings/arm/idle-states.txt. > > Cc: <devicetree@vger.kernel.org> > Cc: Rob Herring <robh@kernel.org> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt > index e165036..6fb53a3 100644 > --- a/Documentation/devicetree/bindings/power/power_domain.txt > +++ b/Documentation/devicetree/bindings/power/power_domain.txt > @@ -30,8 +30,9 @@ Optional properties: > available in the next section. > > - domain-idle-states : A phandle of an idle-state that shall be soaked into a > - generic domain power state. The idle state definitions are > - compatible with arm,idle-state specified in [1]. > + generic domain power state. The idle state definitions must be > + compatible with "domain-idle-state" I would reword the below a bit different so that it's flexible to be reused without "arm,idle-state". > as well as > + "arm,idle-state" as defined in [1]. 'Idle states that are "arm,idle-state" compatible are generally "domain-idle-state" compatible as well if it's a PM domain.' or something like that in line with what's in patch 2/4. That would give us the scope of reuse of "domain-idle-state" in device for future. Also it aligns with your patch 4/4. Otherwise, it looks good.
On Tue, Oct 25 2016 at 09:59 -0600, Sudeep Holla wrote: > > >On 25/10/16 16:26, Lina Iyer wrote: >>Update domain-idle-state binding to use "domain-idle-state" compatible >>from Documentation/devicetree/bindings/arm/idle-states.txt. >> >>Cc: <devicetree@vger.kernel.org> >>Cc: Rob Herring <robh@kernel.org> >>Suggested-by: Sudeep Holla <sudeep.holla@arm.com> >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>--- >> Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt >>index e165036..6fb53a3 100644 >>--- a/Documentation/devicetree/bindings/power/power_domain.txt >>+++ b/Documentation/devicetree/bindings/power/power_domain.txt >>@@ -30,8 +30,9 @@ Optional properties: >> available in the next section. >> >> - domain-idle-states : A phandle of an idle-state that shall be soaked into a >>- generic domain power state. The idle state definitions are >>- compatible with arm,idle-state specified in [1]. >>+ generic domain power state. The idle state definitions must be >>+ compatible with "domain-idle-state" > >I would reword the below a bit different so that it's flexible to be >reused without "arm,idle-state". > >>as well as >>+ "arm,idle-state" as defined in [1]. > >'Idle states that are "arm,idle-state" compatible are generally >"domain-idle-state" compatible as well if it's a PM domain.' > I believe we should have both compatible strings. Per [1], any CPU that follows the idle state compatible *must* have "arm,idle-state" as a compatible. Since we are re-using the same compatible, its only correct that we retain what is already spec'd up in [1] and in addition provide this new compatible. Thanks, Lina >or something like that in line with what's in patch 2/4. > >That would give us the scope of reuse of "domain-idle-state" in device >for future. Also it aligns with your patch 4/4. > >Otherwise, it looks good. > >-- >Regards, >Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/10/16 17:24, Lina Iyer wrote: > On Tue, Oct 25 2016 at 09:59 -0600, Sudeep Holla wrote: >> >> >> On 25/10/16 16:26, Lina Iyer wrote: >>> Update domain-idle-state binding to use "domain-idle-state" compatible >>> from Documentation/devicetree/bindings/arm/idle-states.txt. >>> >>> Cc: <devicetree@vger.kernel.org> >>> Cc: Rob Herring <robh@kernel.org> >>> Suggested-by: Sudeep Holla <sudeep.holla@arm.com> >>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>> --- >>> Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt >>> b/Documentation/devicetree/bindings/power/power_domain.txt >>> index e165036..6fb53a3 100644 >>> --- a/Documentation/devicetree/bindings/power/power_domain.txt >>> +++ b/Documentation/devicetree/bindings/power/power_domain.txt >>> @@ -30,8 +30,9 @@ Optional properties: >>> available in the next section. >>> >>> - domain-idle-states : A phandle of an idle-state that shall be >>> soaked into a >>> - generic domain power state. The idle state >>> definitions are >>> - compatible with arm,idle-state specified in [1]. >>> + generic domain power state. The idle state >>> definitions must be >>> + compatible with "domain-idle-state" >> >> I would reword the below a bit different so that it's flexible to be >> reused without "arm,idle-state". >> >>> as well as >>> + "arm,idle-state" as defined in [1]. >> >> 'Idle states that are "arm,idle-state" compatible are generally >> "domain-idle-state" compatible as well if it's a PM domain.' >> > I believe we should have both compatible strings. Per [1], any CPU that > follows the idle state compatible *must* have "arm,idle-state" as a > compatible. Yes that's implicit for a CPU device. But generic power domain bindings should not have that explicitly as it *can be* used for non CPU device. > Since we are re-using the same compatible, its only correct > that we retain what is already spec'd up in [1] and in addition provide > this new compatible. > Yes [1] applies for *CPUs only* while this applies for *any device* and *any power domain*, so I would drop *must have* "arm,idle-state" here to keep this generic based on my understanding on how compatibles work.
Lina Iyer <lina.iyer@linaro.org> writes: > Update domain-idle-state binding to use "domain-idle-state" compatible > from Documentation/devicetree/bindings/arm/idle-states.txt. > > Cc: <devicetree@vger.kernel.org> > Cc: Rob Herring <robh@kernel.org> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) With no current users for this, I don't see the point of adding a compatible now. IMO, this should wait and be added with the identified user we can discuss it then. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/10/16 21:49, Kevin Hilman wrote: > Lina Iyer <lina.iyer@linaro.org> writes: > >> Update domain-idle-state binding to use "domain-idle-state" compatible >> from Documentation/devicetree/bindings/arm/idle-states.txt. >> >> Cc: <devicetree@vger.kernel.org> >> Cc: Rob Herring <robh@kernel.org> >> Suggested-by: Sudeep Holla <sudeep.holla@arm.com> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >> --- >> Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) > > With no current users for this, I don't see the point of adding a > compatible now. > > IMO, this should wait and be added with the identified user we can > discuss it then. > No, IMO it needs to be used for the proposed SoC idle/genpd solution. I understand the nodes that are "arm,idle-state" compatible can be used for this new SoC hierarchical idle management, but it was never defined for that use originally. So this new feature must be advertised by the firmware with the presence of "domain-idle-state". Yes we might have other ways to detect that but I have already seen that broken on the reference platform, so we need alternate/DT way to specify that. Not all existing "arm,idle-state" compatible nodes will be capable of supporting this new SoC idle feature. It's just better and safer for a new feature getting added that relies on DT to have a new compatible.
On Wed, Oct 26, 2016 at 09:57:35AM +0100, Sudeep Holla wrote: > > > On 25/10/16 21:49, Kevin Hilman wrote: > > Lina Iyer <lina.iyer@linaro.org> writes: > > > > > Update domain-idle-state binding to use "domain-idle-state" compatible > > > from Documentation/devicetree/bindings/arm/idle-states.txt. > > > > > > Cc: <devicetree@vger.kernel.org> > > > Cc: Rob Herring <robh@kernel.org> > > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > --- > > > Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > With no current users for this, I don't see the point of adding a > > compatible now. > > > > IMO, this should wait and be added with the identified user we can > > discuss it then. > > > > No, IMO it needs to be used for the proposed SoC idle/genpd solution. > > I understand the nodes that are "arm,idle-state" compatible can be used > for this new SoC hierarchical idle management, but it was never defined > for that use originally. So this new feature must be advertised by the > firmware with the presence of "domain-idle-state". > > Yes we might have other ways to detect that but I have already seen that > broken on the reference platform, so we need alternate/DT way to specify > that. > > Not all existing "arm,idle-state" compatible nodes will be capable of > supporting this new SoC idle feature. It's just better and safer for a > new feature getting added that relies on DT to have a new compatible. Or perhaps you should describe something new rather than trying to graft in what's there. This combination of compatible strings looks a bit odd to me. Though, I've not really spent much time thinking about this. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt index e165036..6fb53a3 100644 --- a/Documentation/devicetree/bindings/power/power_domain.txt +++ b/Documentation/devicetree/bindings/power/power_domain.txt @@ -30,8 +30,9 @@ Optional properties: available in the next section. - domain-idle-states : A phandle of an idle-state that shall be soaked into a - generic domain power state. The idle state definitions are - compatible with arm,idle-state specified in [1]. + generic domain power state. The idle state definitions must be + compatible with "domain-idle-state" as well as + "arm,idle-state" as defined in [1]. The domain-idle-state property reflects the idle state of this PM domain and not the idle states of the devices or sub-domains in the PM domain. Devices and sub-domains have their own idle-states independent of the parent @@ -85,7 +86,7 @@ Example 3: }; DOMAIN_RET: state@0 { - compatible = "arm,idle-state"; + compatible = "domain-idle-state", "arm,idle-state"; reg = <0x0>; entry-latency-us = <1000>; exit-latency-us = <2000>; @@ -93,7 +94,7 @@ Example 3: }; DOMAIN_PWR_DN: state@1 { - compatible = "arm,idle-state"; + compatible = "domain-idle-state", "arm,idle-state"; reg = <0x1>; entry-latency-us = <5000>; exit-latency-us = <8000>;
Update domain-idle-state binding to use "domain-idle-state" compatible from Documentation/devicetree/bindings/arm/idle-states.txt. Cc: <devicetree@vger.kernel.org> Cc: Rob Herring <robh@kernel.org> Suggested-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- Documentation/devicetree/bindings/power/power_domain.txt | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)