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

[Dave Gerlach]

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:
>>>> +    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