| Message ID | 20220831013359.1807905-2-jwerner@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-bindings: memory: Describing LPDDR topology | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 31/08/2022 04:33, Julius Werner wrote: > The bindings for different LPDDR versions mostly use the same kinds of > properties, so in order to reduce duplication when we're adding support > for more versions, this patch creates a new lpddr-props subschema that > can be referenced by the others to define these common parts. (This will > consider a few smaller I/O width and density numbers "legal" for LPDDR3 > that are usually not used there, but this should be harmless.) > > This also un-deprecates the manufacturer ID property for LPDDR3 (and > introduces it to LPDDR2), since it was found that having this > information available in a separate property can be useful in some > cases. > > Signed-off-by: Julius Werner <jwerner@chromium.org> > --- > .../ddr/jedec,lpddr-props.yaml | 60 +++++++++++++++++++ > .../memory-controllers/ddr/jedec,lpddr2.yaml | 40 ++----------- > .../memory-controllers/ddr/jedec,lpddr3.yaml | 39 ++---------- > 3 files changed, 68 insertions(+), 71 deletions(-) > create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml > new file mode 100644 > index 00000000000000..8b31c60ea2435b > --- /dev/null > +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml > @@ -0,0 +1,60 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr-props.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Common properties for LPDDR types > + > +description: > + Different LPDDR types generally use the same properties and only differ in the > + range of legal values for each. This file defines the common parts that can be > + reused for each type. > + > +maintainers: > + - Krzysztof Kozlowski <krzk@kernel.org> > + > +properties: > + manufacturer-id: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Manufacturer ID read from Mode Register 5. Are you sure that register numbers (here 5, 6-7-8 later) are the same between LPDDR 2-5? The description should match the broadest case and specific schema can narrow or correct it. > + minimum: 0 > + maximum: 255 > + > + revision-id: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: > + Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>). > + minItems: 2 No need for minItems. > + maxItems: 2 > + items: > + minimum: 0 > + maximum: 255 > + > + density: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Density in megabits of SDRAM chip. Decoded from Mode Register 8. > + enum: > + - 64 > + - 128 > + - 256 > + - 512 > + - 1024 > + - 2048 > + - 4096 > + - 8192 > + - 16384 > + - 32768 > + > + io-width: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + IO bus width in bits of SDRAM chip. Decoded from Mode Register 8. > + enum: > + - 32 > + - 16 > + - 8 While moving, order it from lowest to highest. > + > +additionalProperties: true > diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml > index 9d78f140609b6c..63c47235cb9896 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml > @@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# > > title: LPDDR2 SDRAM compliant to JEDEC JESD209-2 > > +allOf: > + - $ref: "jedec,lpddr-props.yaml#" > + This goes just before "properties:" > maintainers: > - Krzysztof Kozlowski <krzk@kernel.org> > > @@ -41,41 +44,6 @@ properties: > Property is deprecated, use revision-id instead. > deprecated: true > > - revision-id: > - $ref: /schemas/types.yaml#/definitions/uint32-array > - description: | > - Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>). > - minItems: 2 > - maxItems: 2 > - items: > - minimum: 0 > - maximum: 255 > - > - density: > - $ref: /schemas/types.yaml#/definitions/uint32 > - description: | > - Density in megabits of SDRAM chip. Obtained from device datasheet. > - enum: > - - 64 > - - 128 > - - 256 > - - 512 > - - 1024 > - - 2048 > - - 4096 > - - 8192 > - - 16384 > - - 32768 > - > - io-width: > - $ref: /schemas/types.yaml#/definitions/uint32 > - description: | > - IO bus width in bits of SDRAM chip. Obtained from device datasheet. > - enum: > - - 32 > - - 16 > - - 8 > - > tRRD-min-tck: > $ref: /schemas/types.yaml#/definitions/uint32 > maximum: 16 > @@ -168,7 +136,7 @@ required: > - density > - io-width > > -additionalProperties: false > +unevaluatedProperties: false > > examples: > - | > diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml > index 48908a19473c3f..5969166cdc9e0f 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml > @@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# > > title: LPDDR3 SDRAM compliant to JEDEC JESD209-3 > > +allOf: > + - $ref: "jedec,lpddr-props.yaml#" > + Also move below (before properties:) > maintainers: > - Krzysztof Kozlowski <krzk@kernel.org> > > @@ -20,40 +23,6 @@ properties: > const: 1 > deprecated: true > > - density: > - $ref: /schemas/types.yaml#/definitions/uint32 > - description: | > - Density in megabits of SDRAM chip. > - enum: > - - 4096 > - - 8192 > - - 16384 > - - 32768 This must stay (so density with enum, but no ref and no description). > - > - io-width: > - $ref: /schemas/types.yaml#/definitions/uint32 > - description: | > - IO bus width in bits of SDRAM chip. > - enum: > - - 32 > - - 16 The same. Best regards, Krzysztof
On 31/08/2022 04:33, Julius Werner wrote: > The bindings for different LPDDR versions mostly use the same kinds of > properties, so in order to reduce duplication when we're adding support > for more versions, this patch creates a new lpddr-props subschema that > can be referenced by the others to define these common parts. (This will > consider a few smaller I/O width and density numbers "legal" for LPDDR3 > that are usually not used there, but this should be harmless.) > > This also un-deprecates the manufacturer ID property for LPDDR3 (and > introduces it to LPDDR2), since it was found that having this > information available in a separate property can be useful in some > cases. Why do you need to un-deprecate them if you have this information in compatible? This was not exactly the previous consensus. My statement was ok for un-deprecating if you cannot derive them from compatible. Now you can. This should be the same as USB device schema. Best regards, Krzysztof
> > +properties: > > + manufacturer-id: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Manufacturer ID read from Mode Register 5. > > Are you sure that register numbers (here 5, 6-7-8 later) are the same > between LPDDR 2-5? The description should match the broadest case and > specific schema can narrow or correct it. Yes, the new LPDDR versions only ever seem to add new mode registers, but the meaning of 5, 6 and 7 have always stayed the same. (For 8, the bit fields have remained the same but the mapping of bit patterns to values has changed.) > > This also un-deprecates the manufacturer ID property for LPDDR3 (and > > introduces it to LPDDR2), since it was found that having this > > information available in a separate property can be useful in some > > cases. > > Why do you need to un-deprecate them if you have this information in > compatible? This was not exactly the previous consensus. My statement > was ok for un-deprecating if you cannot derive them from compatible. Now > you can. This should be the same as USB device schema. Okay. I think there is value in having these as separate properties, because it makes them much easier to read and use. If we don't have them we always need to do all this string parsing first, and if systems allow both kinds of compatible strings (auto-generated and static) they'll need to be able to distinguish and handle both of those when parsing... I think it would be much less friction if each datum of interest could directly be read out of a property. I think having a bit of redundancy is fine and common in device tree bindings (e.g. most properties for most devices are really implied by the compatible string because that same type of device is always built in the same way, but that doesn't mean it's not useful to list them separately for ease-of-access). But I can remove them if you disagree.
On 01/09/2022 03:09, Julius Werner wrote: >>> +properties: >>> + manufacturer-id: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Manufacturer ID read from Mode Register 5. >> >> Are you sure that register numbers (here 5, 6-7-8 later) are the same >> between LPDDR 2-5? The description should match the broadest case and >> specific schema can narrow or correct it. > > Yes, the new LPDDR versions only ever seem to add new mode registers, > but the meaning of 5, 6 and 7 have always stayed the same. (For 8, the > bit fields have remained the same but the mapping of bit patterns to > values has changed.) > >>> This also un-deprecates the manufacturer ID property for LPDDR3 (and >>> introduces it to LPDDR2), since it was found that having this >>> information available in a separate property can be useful in some >>> cases. >> >> Why do you need to un-deprecate them if you have this information in >> compatible? This was not exactly the previous consensus. My statement >> was ok for un-deprecating if you cannot derive them from compatible. Now >> you can. This should be the same as USB device schema. > > Okay. I think there is value in having these as separate properties, > because it makes them much easier to read and use. Storing same value in multiple places is duplication and maintenance effort. Does not make anything easier. > If we don't have > them we always need to do all this string parsing first, and if > systems allow both kinds of compatible strings (auto-generated and > static) they'll need to be able to distinguish and handle both of > those when parsing... I think it would be much less friction if each > datum of interest could directly be read out of a property. I think > having a bit of redundancy is fine and common in device tree bindings No, it's not common. > (e.g. most properties for most devices are really implied by the > compatible string because that same type of device is always built in > the same way, but that doesn't mean it's not useful to list them > separately for ease-of-access). But I can remove them if you disagree. Just like we do not have them for USB, I don't really see the reason to have them for memory. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml new file mode 100644 index 00000000000000..8b31c60ea2435b --- /dev/null +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml @@ -0,0 +1,60 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/memory-controllers/ddr/jedec,lpddr-props.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Common properties for LPDDR types + +description: + Different LPDDR types generally use the same properties and only differ in the + range of legal values for each. This file defines the common parts that can be + reused for each type. + +maintainers: + - Krzysztof Kozlowski <krzk@kernel.org> + +properties: + manufacturer-id: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Manufacturer ID read from Mode Register 5. + minimum: 0 + maximum: 255 + + revision-id: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>). + minItems: 2 + maxItems: 2 + items: + minimum: 0 + maximum: 255 + + density: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Density in megabits of SDRAM chip. Decoded from Mode Register 8. + enum: + - 64 + - 128 + - 256 + - 512 + - 1024 + - 2048 + - 4096 + - 8192 + - 16384 + - 32768 + + io-width: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + IO bus width in bits of SDRAM chip. Decoded from Mode Register 8. + enum: + - 32 + - 16 + - 8 + +additionalProperties: true diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml index 9d78f140609b6c..63c47235cb9896 100644 --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml @@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# title: LPDDR2 SDRAM compliant to JEDEC JESD209-2 +allOf: + - $ref: "jedec,lpddr-props.yaml#" + maintainers: - Krzysztof Kozlowski <krzk@kernel.org> @@ -41,41 +44,6 @@ properties: Property is deprecated, use revision-id instead. deprecated: true - revision-id: - $ref: /schemas/types.yaml#/definitions/uint32-array - description: | - Revision IDs read from Mode Register 6 and 7. One byte per uint32 cell (i.e. <MR6 MR7>). - minItems: 2 - maxItems: 2 - items: - minimum: 0 - maximum: 255 - - density: - $ref: /schemas/types.yaml#/definitions/uint32 - description: | - Density in megabits of SDRAM chip. Obtained from device datasheet. - enum: - - 64 - - 128 - - 256 - - 512 - - 1024 - - 2048 - - 4096 - - 8192 - - 16384 - - 32768 - - io-width: - $ref: /schemas/types.yaml#/definitions/uint32 - description: | - IO bus width in bits of SDRAM chip. Obtained from device datasheet. - enum: - - 32 - - 16 - - 8 - tRRD-min-tck: $ref: /schemas/types.yaml#/definitions/uint32 maximum: 16 @@ -168,7 +136,7 @@ required: - density - io-width -additionalProperties: false +unevaluatedProperties: false examples: - | diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml index 48908a19473c3f..5969166cdc9e0f 100644 --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr3.yaml @@ -6,6 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# title: LPDDR3 SDRAM compliant to JEDEC JESD209-3 +allOf: + - $ref: "jedec,lpddr-props.yaml#" + maintainers: - Krzysztof Kozlowski <krzk@kernel.org> @@ -20,40 +23,6 @@ properties: const: 1 deprecated: true - density: - $ref: /schemas/types.yaml#/definitions/uint32 - description: | - Density in megabits of SDRAM chip. - enum: - - 4096 - - 8192 - - 16384 - - 32768 - - io-width: - $ref: /schemas/types.yaml#/definitions/uint32 - description: | - IO bus width in bits of SDRAM chip. - enum: - - 32 - - 16 - - manufacturer-id: - $ref: /schemas/types.yaml#/definitions/uint32 - description: | - Manufacturer ID value read from Mode Register 5. The property is - deprecated, manufacturer should be derived from the compatible. - deprecated: true - - revision-id: - $ref: /schemas/types.yaml#/definitions/uint32-array - minItems: 2 - maxItems: 2 - items: - maximum: 255 - description: | - Revision value of SDRAM chip read from Mode Registers 6 and 7. - '#size-cells': const: 0 deprecated: true @@ -206,7 +175,7 @@ required: - density - io-width -additionalProperties: false +unevaluatedProperties: false examples: - |
The bindings for different LPDDR versions mostly use the same kinds of properties, so in order to reduce duplication when we're adding support for more versions, this patch creates a new lpddr-props subschema that can be referenced by the others to define these common parts. (This will consider a few smaller I/O width and density numbers "legal" for LPDDR3 that are usually not used there, but this should be harmless.) This also un-deprecates the manufacturer ID property for LPDDR3 (and introduces it to LPDDR2), since it was found that having this information available in a separate property can be useful in some cases. Signed-off-by: Julius Werner <jwerner@chromium.org> --- .../ddr/jedec,lpddr-props.yaml | 60 +++++++++++++++++++ .../memory-controllers/ddr/jedec,lpddr2.yaml | 40 ++----------- .../memory-controllers/ddr/jedec,lpddr3.yaml | 39 ++---------- 3 files changed, 68 insertions(+), 71 deletions(-) create mode 100644 Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml