| Message ID | 20220421203659.27853-2-d-gerlach@ti.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | firmware: ti_sci: Introduce system suspend support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 4/21/22 3:36 PM, Dave Gerlach wrote: > Add documentation for the ti,ctx-memory-region property which is a > phandle to a reserved-memory carveout to be used by the ti_sci driver > storage of low power mode memory context. This is optional for normal > system operation but required to enabled suspend-to-mem usage of Deep > Sleep state. > > Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > --- > .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > index 34f5f877d444..ec88aa88a2a0 100644 > --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > @@ -61,6 +61,15 @@ properties: > mboxes: > minItems: 2 > > + ti,ctx-memory-region: > + description: > + Phandle to the reserved memory node to be associated with the > + ti-sci device, to be used for saving low power context. The > + reserved memory node should be a carveout node, and should > + be defined as per the bindings in > + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml > + $ref: /schemas/types.yaml#/definitions/string > + Why does this have to be yet another reserved carveout region, should be dynamically allocated. Andrew > ti,system-reboot-controller: > description: Determines If system reboot can be triggered by SoC reboot > type: boolean
Hi, On 4/22/22 14:02, Andrew Davis wrote: > On 4/21/22 3:36 PM, Dave Gerlach wrote: >> Add documentation for the ti,ctx-memory-region property which is a >> phandle to a reserved-memory carveout to be used by the ti_sci driver >> storage of low power mode memory context. This is optional for normal >> system operation but required to enabled suspend-to-mem usage of Deep >> Sleep state. >> >> Signed-off-by: Dave Gerlach <d-gerlach@ti.com> >> --- >> .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml >> index 34f5f877d444..ec88aa88a2a0 100644 >> --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml >> +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml >> @@ -61,6 +61,15 @@ properties: >> mboxes: >> minItems: 2 >> >> + ti,ctx-memory-region: >> + description: >> + Phandle to the reserved memory node to be associated with the >> + ti-sci device, to be used for saving low power context. The >> + reserved memory node should be a carveout node, and should >> + be defined as per the bindings in >> + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml >> + $ref: /schemas/types.yaml#/definitions/string >> + > > > Why does this have to be yet another reserved carveout region, > should be dynamically allocated. > This must be a fixed address in order to support other low power modes which have not yet been introduced. Regards, Dave > Andrew > > >> ti,system-reboot-controller: >> description: Determines If system reboot can be triggered by SoC reboot >> type: boolean
On 14:10-20220422, Dave Gerlach wrote: > Hi, > > On 4/22/22 14:02, Andrew Davis wrote: > > On 4/21/22 3:36 PM, Dave Gerlach wrote: > >> Add documentation for the ti,ctx-memory-region property which is a > >> phandle to a reserved-memory carveout to be used by the ti_sci driver > >> storage of low power mode memory context. This is optional for normal > >> system operation but required to enabled suspend-to-mem usage of Deep > >> Sleep state. > >> > >> Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > >> --- > >> .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > >> index 34f5f877d444..ec88aa88a2a0 100644 > >> --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > >> +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > >> @@ -61,6 +61,15 @@ properties: > >> mboxes: > >> minItems: 2 > >> > >> + ti,ctx-memory-region: > >> + description: > >> + Phandle to the reserved memory node to be associated with the > >> + ti-sci device, to be used for saving low power context. The > >> + reserved memory node should be a carveout node, and should > >> + be defined as per the bindings in > >> + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml > >> + $ref: /schemas/types.yaml#/definitions/string > >> + > > > > > > Why does this have to be yet another reserved carveout region, > > should be dynamically allocated. > > > > This must be a fixed address in order to support other low power modes > which have not yet been introduced. Please elaborate the need - Many of our devices, esp the AM62 class ones are memory constrained devices - LPM states are controlled entry states, why should we loose a chunk of DDR in operational state while waiting for the suspend or idle state to be invoked? OR, is the argument is as follows: - need a guarenteed memory for me to enter low power and not be dependent on availability on attempt. - Latency overhead of allocation during a "hot path" such as cpu idle, this is completely unacceptable? or something of that form.. please elaborate?
Hi, On 4/23/22 08:36, Nishanth Menon wrote: > On 14:10-20220422, Dave Gerlach wrote: >> Hi, >> >> On 4/22/22 14:02, Andrew Davis wrote: >>> On 4/21/22 3:36 PM, Dave Gerlach wrote: >>>> Add documentation for the ti,ctx-memory-region property which is a >>>> phandle to a reserved-memory carveout to be used by the ti_sci driver >>>> storage of low power mode memory context. This is optional for normal >>>> system operation but required to enabled suspend-to-mem usage of Deep >>>> Sleep state. >>>> >>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com> >>>> --- >>>> .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml >>>> index 34f5f877d444..ec88aa88a2a0 100644 >>>> --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml >>>> +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml >>>> @@ -61,6 +61,15 @@ properties: >>>> mboxes: >>>> minItems: 2 >>>> >>>> + ti,ctx-memory-region: >>>> + description: >>>> + Phandle to the reserved memory node to be associated with the >>>> + ti-sci device, to be used for saving low power context. The >>>> + reserved memory node should be a carveout node, and should >>>> + be defined as per the bindings in >>>> + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml >>>> + $ref: /schemas/types.yaml#/definitions/string >>>> + >>> >>> >>> Why does this have to be yet another reserved carveout region, >>> should be dynamically allocated. >>> >> >> This must be a fixed address in order to support other low power modes >> which have not yet been introduced. > > Please elaborate the need - Many of our devices, esp the AM62 class ones > are memory constrained devices - LPM states are controlled entry states, why > should we loose a chunk of DDR in operational state while waiting for > the suspend or idle state to be invoked? > OR, is the argument is as follows: > - need a guarenteed memory for me to enter low power and not be > dependent on availability on attempt. > - Latency overhead of allocation during a "hot path" such as cpu idle, > this is completely unacceptable? > > or something of that form.. please elaborate? Yes, in the case of some future low power modes, it is possible for the SoC to completely lose context. For the mode being introduced here, I agree that we could dynamically allocate this memory and then share that address around, but keeping it fixed of course works here, and *also* works for the complete context loss case, as a different mechanism would be restoring this context and can easily get the fixed address straight from the DT. Otherwise we would have two completely divergent paths because there is no way to share some dynamic address across the transition. To me it makes sense to keep it the same for all modes when possible. Regards, Dave
On 15:24-20220425, Dave Gerlach wrote: > Hi, > > On 4/23/22 08:36, Nishanth Menon wrote: > > On 14:10-20220422, Dave Gerlach wrote: > >> Hi, > >> > >> On 4/22/22 14:02, Andrew Davis wrote: > >>> On 4/21/22 3:36 PM, Dave Gerlach wrote: > >>>> Add documentation for the ti,ctx-memory-region property which is a > >>>> phandle to a reserved-memory carveout to be used by the ti_sci driver > >>>> storage of low power mode memory context. This is optional for normal > >>>> system operation but required to enabled suspend-to-mem usage of Deep > >>>> Sleep state. > >>>> > >>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > >>>> --- > >>>> .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > >>>> index 34f5f877d444..ec88aa88a2a0 100644 > >>>> --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > >>>> +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > >>>> @@ -61,6 +61,15 @@ properties: > >>>> mboxes: > >>>> minItems: 2 > >>>> > >>>> + ti,ctx-memory-region: Just memory-region? > >>>> + description: > >>>> + Phandle to the reserved memory node to be associated with the > >>>> + ti-sci device, to be used for saving low power context. The > >>>> + reserved memory node should be a carveout node, and should > >>>> + be defined as per the bindings in > >>>> + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml > >>>> + $ref: /schemas/types.yaml#/definitions/string $ref: /schemas/types.yaml#/definitions/phandle ? maxItems: 1 > >>>> + > >>> > >>> > >>> Why does this have to be yet another reserved carveout region, > >>> should be dynamically allocated. > >>> > >> > >> This must be a fixed address in order to support other low power modes > >> which have not yet been introduced. > > > > Please elaborate the need - Many of our devices, esp the AM62 class ones > > are memory constrained devices - LPM states are controlled entry states, why > > should we loose a chunk of DDR in operational state while waiting for > > the suspend or idle state to be invoked? > > OR, is the argument is as follows: > > - need a guarenteed memory for me to enter low power and not be > > dependent on availability on attempt. > > - Latency overhead of allocation during a "hot path" such as cpu idle, > > this is completely unacceptable? > > > > or something of that form.. please elaborate? > > Yes, in the case of some future low power modes, it is possible for the > SoC to completely lose context. For the mode being introduced here, I > agree that we could dynamically allocate this memory and then share that > address around, but keeping it fixed of course works here, and *also* > works for the complete context loss case, as a different mechanism would > be restoring this context and can easily get the fixed address straight > from the DT. Otherwise we would have two completely divergent paths > because there is no way to share some dynamic address across the transition. > > To me it makes sense to keep it the same for all modes when possible. a) Clearly this does'nt apply to all SoCs supporting ti,sci. Can we make it more stringent? b) This is a hardware description of a memory region that is carvedout for context information and maybe used as part of various LPM modes where restoration needs to occur prior to OS startup (and dynamic handshake can occur with the entity that manages power controls). I think this should be clearly articulated in commit message and description to help understand the rationale.
On Thu, Apr 21, 2022 at 03:36:54PM -0500, Dave Gerlach wrote: > Add documentation for the ti,ctx-memory-region property which is a > phandle to a reserved-memory carveout to be used by the ti_sci driver > storage of low power mode memory context. This is optional for normal > system operation but required to enabled suspend-to-mem usage of Deep > Sleep state. > > Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > --- > .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > index 34f5f877d444..ec88aa88a2a0 100644 > --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml > @@ -61,6 +61,15 @@ properties: > mboxes: > minItems: 2 > > + ti,ctx-memory-region: What's wrong with 'memory-region'? > + description: > + Phandle to the reserved memory node to be associated with the > + ti-sci device, to be used for saving low power context. The > + reserved memory node should be a carveout node, and should > + be defined as per the bindings in > + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml > + $ref: /schemas/types.yaml#/definitions/string > + > ti,system-reboot-controller: > description: Determines If system reboot can be triggered by SoC reboot > type: boolean > -- > 2.35.0 > >
diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml index 34f5f877d444..ec88aa88a2a0 100644 --- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml +++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml @@ -61,6 +61,15 @@ properties: mboxes: minItems: 2 + ti,ctx-memory-region: + description: + Phandle to the reserved memory node to be associated with the + ti-sci device, to be used for saving low power context. The + reserved memory node should be a carveout node, and should + be defined as per the bindings in + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml + $ref: /schemas/types.yaml#/definitions/string + ti,system-reboot-controller: description: Determines If system reboot can be triggered by SoC reboot type: boolean
Add documentation for the ti,ctx-memory-region property which is a phandle to a reserved-memory carveout to be used by the ti_sci driver storage of low power mode memory context. This is optional for normal system operation but required to enabled suspend-to-mem usage of Deep Sleep state. Signed-off-by: Dave Gerlach <d-gerlach@ti.com> --- .../devicetree/bindings/arm/keystone/ti,sci.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)