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

[Mukesh Ojha]

Hi,

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".
>
> 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)?
>
> > 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.

I was thinking, since this size is part reserved-memory.yaml and
we have

allOf:
  - $ref: "reserved-memory.yaml"

Is your comment still applies?

-Mukesh
> >   
> >   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
>
> Any reason in naming it differently then existing one? You have there
> example.
>
> > +                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