Re: [PATCH 1/6] dt-bindings: ti,sci: Add ti,ctx-memory-region property

[Nishanth Menon]

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@xxxxxx>
> >>>> ---
> >>>>   .../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.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D