Re: [PATCH v2 1/3] dt-bindings: reserved-memory: ramoops: Update the binding

[Mukesh Ojha]

Hi Krzysztof,

On 1/13/2023 5:34 PM, Krzysztof Kozlowski wrote:
> Subject: drop second/last, redundant "binding". The "dt-bindings" prefix
> is already stating that these are bindings.
>
> Your subject says nothing. Everything is "update".

I will fix this.

> On 13/01/2023 12:58, Mukesh Ojha wrote:
> > Update the ramoops region binding document with details
> > like region can also be reserved dynamically apart from
> > reserving it statically.
>  > So what exactly can be here reserved dynamically? And what does it mean
> 'dynamically'? By whom? How is this property of hardware (not OS)?
Normally, for ramoops, already known physical address range from DDR are 
mentioned in Device tree which gets reserved from reserved-memory kernel 
framework(example 1 in this file).

By being dynamic, I meant, with this we are letting the framework to get 
the size from a range of allowable address (<0x0 0x00000000 0xffffffff 
0xffffffff>).

Let me know, if you need more detail.

> > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> > ---
> > Change in v2:
> >    - Added this patch as per changes going to be done in patch 3/3
> >
> >   .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
> >   1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> > index 0391871..54e46e8 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> > +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> > @@ -10,7 +10,8 @@ description: |
> >     ramoops provides persistent RAM storage for oops and panics, so they can be
> >     recovered after a reboot. This is a child-node of "/reserved-memory", and
> >     is named "ramoops" after the backend, rather than "pstore" which is the
> > -  subsystem.
> > +  subsystem. This region can be reserved both statically or dynamically by
> > +  using appropriate property in device tree.
> >   
> >     Parts of this storage may be set aside for other persistent log buffers, such
> >     as kernel log messages, or for optional ECC error-correction data.  The total
> > @@ -112,7 +113,13 @@ unevaluatedProperties: false
> >   
> >   required:
> >     - compatible
> > -  - reg
> > +
> > +oneOf:
> > +  - required:
> > +      - reg
> > +
> > +  - required:
> > +      - size
>
> There is no such property. You cannot require it.

Forgot to mention this.

--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
@@ -37,6 +37,20 @@ properties:
   reg:
     description: region of memory that is preserved between reboots

+  size:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+    description: >
+      Length based on parent's \#size-cells. Size in bytes of memory to
+      reserve.
+
+  alloc-ranges:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: >
+      Address and Length pairs. Specifies regions of memory that are
+      acceptable to allocate from.
+
   ecc-size:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: enables ECC support and specifies ECC buffer size in 
bytes
@@ -119,6 +133,7 @@ oneOf:
       - reg

   - required:
+      - alloc-ranges
       - size
> >   
> >   anyOf:
> >     - required: [record-size]
> > @@ -142,3 +149,26 @@ examples:
> >               };
> >           };
> >       };
> > +
> > +  - |
> > +    / {
> > +        compatible = "foo";
> > +        model = "foo";
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        reserved-memory {
> > +            #address-cells = <2>;
> > +            #size-cells = <2>;
> > +            ranges;
> > +
> > +            ramoops: ramoops_region {
>
> Node names should be generic, no underscores in node names.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Thanks.

> Any reason in naming it differently then existing one? You have there
> example.

ramoops@bfdf0000 is not valid after dynamic allocation of memory.
Probably, will mention it as

ramoops_region: ramoops { ??

-Mukesh
> > +                compatible = "ramoops";
> > +                alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
> > +                size = <0x0 0x10000>;       /* 64kB */
> > +                console-size = <0x8000>;    /* 32kB */
> > +                record-size = <0x400>;      /*  1kB */
> > +                ecc-size = <16>;
> > +            };
> > +        };
> > +    };
>
> Best regards,
> Krzysztof