| Message ID | 1673611126-13803-1-git-send-email-quic_mojha@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/3] dt-bindings: reserved-memory: ramoops: Update the binding | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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@quicinc.com> > --- > 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. > > 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
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@quicinc.com> >> --- >> 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 >
Hi, On 1/13/2023 5:28 PM, Mukesh Ojha wrote: > The reserved memory region for ramoops is assumed to be at a fixed > and known location when read from the devicetree. This is not desirable > in environments where it is preferred the region to be dynamically > allocated at runtime. So, update the document while adding the > support in the driver. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > Change in v2: > - Added this patch as per changes going to be done in patch 3/3 > > Documentation/admin-guide/ramoops.rst | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst > index e9f8514..88884b2 100644 > --- a/Documentation/admin-guide/ramoops.rst > +++ b/Documentation/admin-guide/ramoops.rst > @@ -16,8 +16,9 @@ survive after a restart. > Ramoops concepts > ---------------- > > -Ramoops uses a predefined memory area to store the dump. The start and size > -and type of the memory area are set using three variables: > +Ramoops uses both predefined and dynamically memory area to store the dump. > +The start and size and type of the memory area are set using three > +variables: > > * ``mem_address`` for the start > * ``mem_size`` for the size. The memory size will be rounded down to a > @@ -70,7 +71,8 @@ Setting the ramoops parameters can be done in several different manners: > > B. Use Device Tree bindings, as described in > ``Documentation/devicetree/bindings/reserved-memory/ramoops.yaml``. > - For example:: > + > + Example of statically reserved ramoops region:: > > reserved-memory { > #address-cells = <2>; > @@ -85,6 +87,23 @@ Setting the ramoops parameters can be done in several different manners: > }; > }; > > + Example of dynamically reserved ramoops region:: > + > + reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + ramoops@8f000000 { Will fix it as ramoops_region : ramoops ? > + compatible = "ramoops"; > + alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>; > + size = <0 0x100000>; > + record-size = <0x4000>; > + console-size = <0x4000>; > + }; > + }; > + > + > C. Use a platform device and set the platform data. The parameters can then > be set through that platform data. An example of doing that is: > -Mukesh
On Fri, 13 Jan 2023 17:28:44 +0530, Mukesh Ojha wrote: > Update the ramoops region binding document with details > like region can also be reserved dynamically apart from > reserving it statically. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > 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(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/reserved-memory/ramoops.example.dts:17.13-40: Warning (reg_format): /reserved-memory/ramoops@bfdf0000:reg: property has invalid length (8 bytes) (#address-cells == 2, #size-cells == 2) Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format' /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/reserved-memory/ramoops.example.dtb: reserved-memory: ramoops@bfdf0000:reg:0: [3219062784, 65536] is too short From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/reg.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1673611126-13803-1-git-send-email-quic_mojha@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
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@quicinc.com> >> --- >> 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 >
On 27/01/2023 13:29, Mukesh Ojha wrote: >>> +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? Ah, you are right. It's ok. Best regards, Krzysztof
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 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 { + compatible = "ramoops"; + alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>; + size = <0x0 0x10000>; /* 64kB */ + console-size = <0x8000>; /* 32kB */ + record-size = <0x400>; /* 1kB */ + ecc-size = <16>; + }; + }; + };
Update the ramoops region binding document with details like region can also be reserved dynamically apart from reserving it statically. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- 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(-)