| Message ID | 20220822191957.28546-3-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | EDAC/synopsys: Add generic resources and Baikal-T1 support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On 22/08/2022 22:19, Serge Semin wrote: > Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > are individual IRQs for each ECC and DFI events.The dedicated scrubber Missing space before "The". > clock source is absent since it's fully synchronous to the core clock. You need allOf:if-then restricting this per variant. > In addition to that the DFI-DDR PHY CSRs can be accessed via a separate > registers space. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > .../memory-controllers/snps,dw-umctl2-ddrc.yaml | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml > index 8db92210cfe1..899a6c5f9806 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml > @@ -26,6 +26,7 @@ properties: > enum: > - snps,ddrc-3.80a > - xlnx,zynqmp-ddrc-2.40a > + - baikal,bt1-ddrc Messed order. Don't add stuff at the end, but in alphabetical order. > > interrupts: > description: > @@ -49,7 +50,14 @@ properties: > enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ] > > reg: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + reg-names: > + minItems: 1 > + items: > + - const: umctl2 > + - const: phy You need allOf:if-then restricting this per variant. Best regards, Krzysztof
On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: > On 22/08/2022 22:19, Serge Semin wrote: > > Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > > with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > > are individual IRQs for each ECC and DFI events.The dedicated scrubber > > Missing space before "The". Ok. Thanks. > > > clock source is absent since it's fully synchronous to the core clock. > > You need allOf:if-then restricting this per variant. I really don't like the allOf-if-if-etc pattern because it gets to be very bulky if all the vendor-specific and generic platform peculiarities are placed in there. I am more keen of having a generic DT-schema which would be then allOf-ed by the vendor-specific device bindings. What do you think I'd provide such design in this case too? But I'll need to move the compatible property definition to the "select" property. Like this: Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: +[...] +# Please create a separate DT-schema for your DW uMCTL2 DDR controller +# and make sure it's assigned with the vendor-specific compatible string. +select: + properties: + compatible: + oneOf: + - deprecated: true + description: Synopsys DW uMCTL2 DDR controller v3.80a + const: snps,ddrc-3.80a + - description: Synopsys DW uMCTL2 DDR controller + const: snps,dw-umctl2-ddrc + - description: Xilinx ZynqMP DDR controller v2.40a + const: xlnx,zynqmp-ddrc-2.40a + required: + - compatible + +properties: + compatible: true +[...] +required: + - compatible + - reg + - interrupts + +additionalProperties: true After that the "snps,dw-umctl2-ddrc.yaml" schema can be referenced in the allOf composition. Like this: Documentation/devicetree/bindings/memory-controllers/baikal,bt1-ddrc.yaml: +[...] +allOf: + - $ref: /schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml# +[...] At the same time the generic DT-schema will be used to evaluate the "snps,ddrc-3.80a", "snps,dw-umctl2-ddrc" and "xlnx,zynqmp-ddrc-2.40a" device nodes as before. What do you think about that? One big positive side of this that even though the generic schema can't define the IRQ/resets/clocks phandlers order because various platforms may have different external signals setup, the vendor-specific schema can and should. So I'll be able to describe the Baikal-T1 DDRC specific properties (clocks, clock-names, interrupts, interrupt-names, etc) in much more details including the reference signals order what you asked in the previous patch review. -Sergey > > > In addition to that the DFI-DDR PHY CSRs can be accessed via a separate > > registers space. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > .../memory-controllers/snps,dw-umctl2-ddrc.yaml | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml > > index 8db92210cfe1..899a6c5f9806 100644 > > --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml > > +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml > > @@ -26,6 +26,7 @@ properties: > > enum: > > - snps,ddrc-3.80a > > - xlnx,zynqmp-ddrc-2.40a > > + - baikal,bt1-ddrc > > Messed order. Don't add stuff at the end, but in alphabetical order. Ok. But could you please give me a reference with this requirement documented? I've submitted many DT-bindings patches with the compatible property updates and none of them received such comment from Rob. > > > > > interrupts: > > description: > > @@ -49,7 +50,14 @@ properties: > > enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ] > > > > reg: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 2 > > + > > + reg-names: > > + minItems: 1 > > + items: > > + - const: umctl2 > > + - const: phy > > You need allOf:if-then restricting this per variant. Please see my comment above regarding possible solution of this. -Sergey > > Best regards, > Krzysztof
On Mon, Aug 22, 2022 at 10:19:45PM +0300, Serge Semin wrote: > Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > are individual IRQs for each ECC and DFI events.The dedicated scrubber > clock source is absent since it's fully synchronous to the core clock. > In addition to that the DFI-DDR PHY CSRs can be accessed via a separate > registers space. Are you sure the phy and dfi irq shouldn't be a separate device? Rob
On 26/08/2022 11:54, Serge Semin wrote: > On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: >> On 22/08/2022 22:19, Serge Semin wrote: >>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a >>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There >>> are individual IRQs for each ECC and DFI events.The dedicated scrubber >> > >> Missing space before "The". > > Ok. Thanks. > >> >>> clock source is absent since it's fully synchronous to the core clock. >> > >> You need allOf:if-then restricting this per variant. > > I really don't like the allOf-if-if-etc pattern because it gets to be > very bulky if all the vendor-specific and generic platform > peculiarities are placed in there. I am more keen of having a > generic DT-schema which would be then allOf-ed by the vendor-specific > device bindings. What do you think I'd provide such design in this > case too? Sure, it would work. > > But I'll need to move the compatible property definition to the > "select" property. Like this: > > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: > +[...] > +# Please create a separate DT-schema for your DW uMCTL2 DDR controller > +# and make sure it's assigned with the vendor-specific compatible string. > +select: > + properties: > + compatible: > + oneOf: > + - deprecated: true > + description: Synopsys DW uMCTL2 DDR controller v3.80a > + const: snps,ddrc-3.80a > + - description: Synopsys DW uMCTL2 DDR controller > + const: snps,dw-umctl2-ddrc > + - description: Xilinx ZynqMP DDR controller v2.40a > + const: xlnx,zynqmp-ddrc-2.40a > + required: > + - compatible Not entirely. If you need select, then add it with compatibles, but all descriptions and deprecated are staying in properties. > + > +properties: > + compatible: true > +[...] > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: true > > After that the "snps,dw-umctl2-ddrc.yaml" schema can be referenced in the > allOf composition. Like this: > > Documentation/devicetree/bindings/memory-controllers/baikal,bt1-ddrc.yaml: > +[...] > +allOf: > + - $ref: /schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml# > +[...] > > At the same time the generic DT-schema will be used to evaluate the > "snps,ddrc-3.80a", "snps,dw-umctl2-ddrc" and "xlnx,zynqmp-ddrc-2.40a" > device nodes as before. What do you think about that? > > One big positive side of this that even though the generic schema > can't define the IRQ/resets/clocks phandlers order because various > platforms may have different external signals setup, the > vendor-specific schema can and should. So I'll be able to describe the > Baikal-T1 DDRC specific properties (clocks, clock-names, interrupts, > interrupt-names, etc) in much more details including the reference > signals order what you asked in the previous patch review. It's ok. You need then second schema for your device, because something must end with additional/unevaluatedProperties false. Best regards, Krzysztof
On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote: > On 26/08/2022 11:54, Serge Semin wrote: > > On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: > >> On 22/08/2022 22:19, Serge Semin wrote: > >>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > >>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > >>> are individual IRQs for each ECC and DFI events.The dedicated scrubber > >> > > > >> Missing space before "The". > > > > Ok. Thanks. > > > >> > >>> clock source is absent since it's fully synchronous to the core clock. > >> > > > >> You need allOf:if-then restricting this per variant. > > > > I really don't like the allOf-if-if-etc pattern because it gets to be > > very bulky if all the vendor-specific and generic platform > > peculiarities are placed in there. I am more keen of having a > > generic DT-schema which would be then allOf-ed by the vendor-specific > > device bindings. What do you think I'd provide such design in this > > case too? > > Sure, it would work. > > > > > But I'll need to move the compatible property definition to the > > "select" property. Like this: > > > > Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: > > +[...] > > +# Please create a separate DT-schema for your DW uMCTL2 DDR controller > > +# and make sure it's assigned with the vendor-specific compatible string. > > +select: > > + properties: > > + compatible: > > + oneOf: > > + - deprecated: true > > + description: Synopsys DW uMCTL2 DDR controller v3.80a > > + const: snps,ddrc-3.80a > > + - description: Synopsys DW uMCTL2 DDR controller > > + const: snps,dw-umctl2-ddrc > > + - description: Xilinx ZynqMP DDR controller v2.40a > > + const: xlnx,zynqmp-ddrc-2.40a > > + required: > > + - compatible > > Not entirely. If you need select, then add it with compatibles, but all > descriptions and deprecated are staying in properties. Ok. But note in such case the compatible string constraints will get to be opened for any non-common string. Like this: + properties: + compatible: + oneOf: + - const: snps,ddrc-3.80a + - {} It's required for the DT-schemas referencing the common one, otherwise they will fail DT-nodes evaluation due to the "compatible" property missing the vendor-specific string. > > > > + > > +properties: > > + compatible: true > > +[...] > > +required: > > + - compatible > > + - reg > > + - interrupts > > + > > +additionalProperties: true > > > > After that the "snps,dw-umctl2-ddrc.yaml" schema can be referenced in the > > allOf composition. Like this: > > > > Documentation/devicetree/bindings/memory-controllers/baikal,bt1-ddrc.yaml: > > +[...] > > +allOf: > > + - $ref: /schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml# > > +[...] > > > > At the same time the generic DT-schema will be used to evaluate the > > "snps,ddrc-3.80a", "snps,dw-umctl2-ddrc" and "xlnx,zynqmp-ddrc-2.40a" > > device nodes as before. What do you think about that? > > > > One big positive side of this that even though the generic schema > > can't define the IRQ/resets/clocks phandlers order because various > > platforms may have different external signals setup, the > > vendor-specific schema can and should. So I'll be able to describe the > > Baikal-T1 DDRC specific properties (clocks, clock-names, interrupts, > > interrupt-names, etc) in much more details including the reference > > signals order what you asked in the previous patch review. > > It's ok. You need then second schema for your device, because something > must end with additional/unevaluatedProperties false. Right. I'll add a separate DT-schema for my device. -Sergey > > Best regards, > Krzysztof
On 08/09/2022 11:46, Serge Semin wrote: > On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote: >> On 26/08/2022 11:54, Serge Semin wrote: >>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: >>>> On 22/08/2022 22:19, Serge Semin wrote: >>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a >>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There >>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber >>>> >>> >>>> Missing space before "The". >>> >>> Ok. Thanks. >>> >>>> >>>>> clock source is absent since it's fully synchronous to the core clock. >>>> >>> >>>> You need allOf:if-then restricting this per variant. >>> >>> I really don't like the allOf-if-if-etc pattern because it gets to be >>> very bulky if all the vendor-specific and generic platform >>> peculiarities are placed in there. I am more keen of having a >>> generic DT-schema which would be then allOf-ed by the vendor-specific >>> device bindings. What do you think I'd provide such design in this >>> case too? >> >> Sure, it would work. >> >>> >>> But I'll need to move the compatible property definition to the >>> "select" property. Like this: >>> >>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: >>> +[...] >>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller >>> +# and make sure it's assigned with the vendor-specific compatible string. >>> +select: >>> + properties: >>> + compatible: >>> + oneOf: >>> + - deprecated: true >>> + description: Synopsys DW uMCTL2 DDR controller v3.80a >>> + const: snps,ddrc-3.80a >>> + - description: Synopsys DW uMCTL2 DDR controller >>> + const: snps,dw-umctl2-ddrc >>> + - description: Xilinx ZynqMP DDR controller v2.40a >>> + const: xlnx,zynqmp-ddrc-2.40a >>> + required: >>> + - compatible >> > >> Not entirely. If you need select, then add it with compatibles, but all >> descriptions and deprecated are staying in properties. > > Ok. But note in such case the compatible string constraints will get > to be opened for any non-common string. Like this: > > + properties: > + compatible: > + oneOf: > + - const: snps,ddrc-3.80a > + - {} Not really. If you define here specific device compatibles in select, they must be here as well. > > It's required for the DT-schemas referencing the common one, otherwise > they will fail DT-nodes evaluation due to the "compatible" property > missing the vendor-specific string. o you probably mix here purposes. Either you define common schema or device specific one. If you define common, usually it does not enforce any compatibles. You do not need select, no need for compatibles either, although you can add above syntax if it is valid. If you write here specific device bindings, then compatibles should be listed. Judging from what you wrote it's neither this nor that... Best regards, Krzysztof
On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote: > On 08/09/2022 11:46, Serge Semin wrote: > > On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote: > >> On 26/08/2022 11:54, Serge Semin wrote: > >>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: > >>>> On 22/08/2022 22:19, Serge Semin wrote: > >>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > >>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > >>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber > >>>> > >>> > >>>> Missing space before "The". > >>> > >>> Ok. Thanks. > >>> > >>>> > >>>>> clock source is absent since it's fully synchronous to the core clock. > >>>> > >>> > >>>> You need allOf:if-then restricting this per variant. > >>> > >>> I really don't like the allOf-if-if-etc pattern because it gets to be > >>> very bulky if all the vendor-specific and generic platform > >>> peculiarities are placed in there. I am more keen of having a > >>> generic DT-schema which would be then allOf-ed by the vendor-specific > >>> device bindings. What do you think I'd provide such design in this > >>> case too? > >> > >> Sure, it would work. > >> > >>> > >>> But I'll need to move the compatible property definition to the > >>> "select" property. Like this: > >>> > >>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: > >>> +[...] > >>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller > >>> +# and make sure it's assigned with the vendor-specific compatible string. > >>> +select: > >>> + properties: > >>> + compatible: > >>> + oneOf: > >>> + - deprecated: true > >>> + description: Synopsys DW uMCTL2 DDR controller v3.80a > >>> + const: snps,ddrc-3.80a > >>> + - description: Synopsys DW uMCTL2 DDR controller > >>> + const: snps,dw-umctl2-ddrc > >>> + - description: Xilinx ZynqMP DDR controller v2.40a > >>> + const: xlnx,zynqmp-ddrc-2.40a > >>> + required: > >>> + - compatible > >> > > > >> Not entirely. If you need select, then add it with compatibles, but all > >> descriptions and deprecated are staying in properties. > > > > Ok. But note in such case the compatible string constraints will get > > to be opened for any non-common string. Like this: > > > > + properties: > > + compatible: > > + oneOf: > > + - const: snps,ddrc-3.80a > > + - {} > > Not really. If you define here specific device compatibles in select, > they must be here as well. > > > > > It's required for the DT-schemas referencing the common one, otherwise > > they will fail DT-nodes evaluation due to the "compatible" property > > missing the vendor-specific string. > > o you probably mix here purposes. Either you define common schema or > device specific one. If you define common, usually it does not enforce > any compatibles. You do not need select, no need for compatibles either, > although you can add above syntax if it is valid. If you write here > specific device bindings, then compatibles should be listed. Judging > from what you wrote it's neither this nor that... My idea was to provide both the common DT-schema and the vendor-specific ones. But the later one would refer to the common schema in the framework of the "allOf:" composition. Like this: snps,dw-umctl2-ddrc.yaml: + [...] + select: + properties: + compatible: + enum: + - snps,ddrc-3.80a + [...] + properties: + compatible: + oneOf: + - const: snps,ddrc-3.80a + - {} + interrupts: + [...] + interrupt-names: + [...] + additionalProperties: false + [...] baikal,bt1-ddrc.yaml: + [...] + allOf: + - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#" + [...] + unevaluateProperties: false + [...] Thus the common schema as before would provide the widest set of the properties and their constraints, while the vendor-specific one would be !obligated! to follow the common schema conventions, but provide a more specific set of the properties and constraints. A similar approach is implemented for instance in the DW USB3 DT-schemas, but with the generic compatible string fallback. In this case we don't need the fallback string, but in order for the common schema being applicable for both the common and vendor-specific DT-nodes the compatible property constraints need to be designed as is provided in the example above. Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into the two ones: snps,dw-umctl2-ddrc-common.yaml and snps,dw-umctl2-ddrc.yaml So the first schema would contain all the common properties definition and would be only used for being referenced in the particular device DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and vendor-specific DT-schemas would just have it "allOf:"ed. Personally I'd prefer the design pattern with the always-true compatible property constraint as in the example above since it seems easier to maintain than having the common and generic device DT-schemas. Note having a common DT-schema and a vendor-specific one referencing the common schema is very much useful practice for the devices based on the same IP-core. Vendor-device driver authors tend to create their own bindings for the same clocks/resets/phys/etc thus making the drivers much harder to maintain (for a bright example see what has been done for the DW PCIe RP/EP IP-core). The worst part of it is that ones the DT-bindings interface is implemented it can't be changed since the kernel needs to be backward compatible with the DT-sources compiled before. So the main goal of the common DT-schema is to fix the common DT interface and make sure the new vendor-specific device drivers would at least consider either to follow the IP-core DT convention or to widen it up if required. -Sergey > > Best regards, > Krzysztof
On 08/09/2022 17:08, Serge Semin wrote: > On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote: >> On 08/09/2022 11:46, Serge Semin wrote: >>> On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote: >>>> On 26/08/2022 11:54, Serge Semin wrote: >>>>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: >>>>>> On 22/08/2022 22:19, Serge Semin wrote: >>>>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a >>>>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There >>>>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber >>>>>> >>>>> >>>>>> Missing space before "The". >>>>> >>>>> Ok. Thanks. >>>>> >>>>>> >>>>>>> clock source is absent since it's fully synchronous to the core clock. >>>>>> >>>>> >>>>>> You need allOf:if-then restricting this per variant. >>>>> >>>>> I really don't like the allOf-if-if-etc pattern because it gets to be >>>>> very bulky if all the vendor-specific and generic platform >>>>> peculiarities are placed in there. I am more keen of having a >>>>> generic DT-schema which would be then allOf-ed by the vendor-specific >>>>> device bindings. What do you think I'd provide such design in this >>>>> case too? >>>> >>>> Sure, it would work. >>>> >>>>> >>>>> But I'll need to move the compatible property definition to the >>>>> "select" property. Like this: >>>>> >>>>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: >>>>> +[...] >>>>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller >>>>> +# and make sure it's assigned with the vendor-specific compatible string. >>>>> +select: >>>>> + properties: >>>>> + compatible: >>>>> + oneOf: >>>>> + - deprecated: true >>>>> + description: Synopsys DW uMCTL2 DDR controller v3.80a >>>>> + const: snps,ddrc-3.80a >>>>> + - description: Synopsys DW uMCTL2 DDR controller >>>>> + const: snps,dw-umctl2-ddrc >>>>> + - description: Xilinx ZynqMP DDR controller v2.40a >>>>> + const: xlnx,zynqmp-ddrc-2.40a >>>>> + required: >>>>> + - compatible >>>> >>> >>>> Not entirely. If you need select, then add it with compatibles, but all >>>> descriptions and deprecated are staying in properties. >>> >>> Ok. But note in such case the compatible string constraints will get >>> to be opened for any non-common string. Like this: >>> >>> + properties: >>> + compatible: >>> + oneOf: >>> + - const: snps,ddrc-3.80a >>> + - {} >> >> Not really. If you define here specific device compatibles in select, >> they must be here as well. >> >>> >>> It's required for the DT-schemas referencing the common one, otherwise >>> they will fail DT-nodes evaluation due to the "compatible" property >>> missing the vendor-specific string. >> > >> o you probably mix here purposes. Either you define common schema or >> device specific one. If you define common, usually it does not enforce >> any compatibles. You do not need select, no need for compatibles either, >> although you can add above syntax if it is valid. If you write here >> specific device bindings, then compatibles should be listed. Judging >> from what you wrote it's neither this nor that... > > My idea was to provide both the common DT-schema and the > vendor-specific ones. But the later one would refer to the common > schema in the framework of the "allOf:" composition. Like this: > > snps,dw-umctl2-ddrc.yaml: > + [...] > + select: > + properties: > + compatible: > + enum: > + - snps,ddrc-3.80a > + [...] > + properties: > + compatible: > + oneOf: > + - const: snps,ddrc-3.80a > + - {} This is not the approach snps,dwc3.yaml is doing. It is closer to snps,dw-pcie.yaml, but that one ends with additionalProperties:true so it is expected to be constrained by something else. You can choose either way, but what you wrote before looked different - basically loosing the compatibles documentation. > + interrupts: > + [...] > + interrupt-names: > + [...] > + additionalProperties: false > + [...] > > baikal,bt1-ddrc.yaml: > + [...] > + allOf: > + - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#" > + [...] > + unevaluateProperties: false > + [...] > > Thus the common schema as before would provide the widest set of the > properties and their constraints, while the vendor-specific one would > be !obligated! to follow the common schema conventions, but provide a > more specific set of the properties and constraints. A similar > approach is implemented for instance in the DW USB3 DT-schemas, but > with the generic compatible string fallback. In this case we don't > need the fallback string, but in order for the common schema being > applicable for both the common and vendor-specific DT-nodes the > compatible property constraints need to be designed as is provided in > the example above. Anyway, it's getting complicated so I am not sure about which option now we discuss. You cannot have deprecated entries in select and compatibles described there, without describing them in properties. > > Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into > the two ones: > snps,dw-umctl2-ddrc-common.yaml > and > snps,dw-umctl2-ddrc.yaml > So the first schema would contain all the common properties definition > and would be only used for being referenced in the particular device > DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and > vendor-specific DT-schemas would just have it "allOf:"ed. > > Personally I'd prefer the design pattern with the always-true > compatible property constraint as in the example above since it seems > easier to maintain than having the common and generic device > DT-schemas. > > Note having a common DT-schema and a vendor-specific one referencing > the common schema is very much useful practice for the devices based > on the same IP-core. Vendor-device driver authors tend to create their > own bindings for the same clocks/resets/phys/etc thus making the > drivers much harder to maintain (for a bright example see what has > been done for the DW PCIe RP/EP IP-core). The worst part of it is that > ones the DT-bindings interface is implemented it can't be changed > since the kernel needs to be backward compatible with the DT-sources > compiled before. So the main goal of the common DT-schema is to fix > the common DT interface and make sure the new vendor-specific device > drivers would at least consider either to follow the IP-core DT > convention or to widen it up if required. Best regards, Krzysztof
On Thu, Sep 08, 2022 at 05:27:30PM +0200, Krzysztof Kozlowski wrote: > On 08/09/2022 17:08, Serge Semin wrote: > > On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote: > >> On 08/09/2022 11:46, Serge Semin wrote: > >>> On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote: > >>>> On 26/08/2022 11:54, Serge Semin wrote: > >>>>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote: > >>>>>> On 22/08/2022 22:19, Serge Semin wrote: > >>>>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > >>>>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > >>>>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber > >>>>>> > >>>>> > >>>>>> Missing space before "The". > >>>>> > >>>>> Ok. Thanks. > >>>>> > >>>>>> > >>>>>>> clock source is absent since it's fully synchronous to the core clock. > >>>>>> > >>>>> > >>>>>> You need allOf:if-then restricting this per variant. > >>>>> > >>>>> I really don't like the allOf-if-if-etc pattern because it gets to be > >>>>> very bulky if all the vendor-specific and generic platform > >>>>> peculiarities are placed in there. I am more keen of having a > >>>>> generic DT-schema which would be then allOf-ed by the vendor-specific > >>>>> device bindings. What do you think I'd provide such design in this > >>>>> case too? > >>>> > >>>> Sure, it would work. > >>>> > >>>>> > >>>>> But I'll need to move the compatible property definition to the > >>>>> "select" property. Like this: > >>>>> > >>>>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml: > >>>>> +[...] > >>>>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller > >>>>> +# and make sure it's assigned with the vendor-specific compatible string. > >>>>> +select: > >>>>> + properties: > >>>>> + compatible: > >>>>> + oneOf: > >>>>> + - deprecated: true > >>>>> + description: Synopsys DW uMCTL2 DDR controller v3.80a > >>>>> + const: snps,ddrc-3.80a > >>>>> + - description: Synopsys DW uMCTL2 DDR controller > >>>>> + const: snps,dw-umctl2-ddrc > >>>>> + - description: Xilinx ZynqMP DDR controller v2.40a > >>>>> + const: xlnx,zynqmp-ddrc-2.40a > >>>>> + required: > >>>>> + - compatible > >>>> > >>> > >>>> Not entirely. If you need select, then add it with compatibles, but all > >>>> descriptions and deprecated are staying in properties. > >>> > >>> Ok. But note in such case the compatible string constraints will get > >>> to be opened for any non-common string. Like this: > >>> > >>> + properties: > >>> + compatible: > >>> + oneOf: > >>> + - const: snps,ddrc-3.80a > >>> + - {} > >> > >> Not really. If you define here specific device compatibles in select, > >> they must be here as well. > >> > >>> > >>> It's required for the DT-schemas referencing the common one, otherwise > >>> they will fail DT-nodes evaluation due to the "compatible" property > >>> missing the vendor-specific string. > >> > > > >> o you probably mix here purposes. Either you define common schema or > >> device specific one. If you define common, usually it does not enforce > >> any compatibles. You do not need select, no need for compatibles either, > >> although you can add above syntax if it is valid. If you write here > >> specific device bindings, then compatibles should be listed. Judging > >> from what you wrote it's neither this nor that... > > > > My idea was to provide both the common DT-schema and the > > vendor-specific ones. But the later one would refer to the common > > schema in the framework of the "allOf:" composition. Like this: > > > > snps,dw-umctl2-ddrc.yaml: > > + [...] > > + select: > > + properties: > > + compatible: > > + enum: > > + - snps,ddrc-3.80a > > + [...] > > + properties: > > + compatible: > > + oneOf: > > + - const: snps,ddrc-3.80a > > + - {} > > This is not the approach snps,dwc3.yaml is doing. It is closer to > snps,dw-pcie.yaml, but that one ends with additionalProperties:true so > it is expected to be constrained by something else. Right. I should have used the "additionalProperties: true" instead in the common DT-schema below. > > You can choose either way, but what you wrote before looked different - > basically loosing the compatibles documentation. Ok. I'll make sure the compatibles documentation won't be lost. > > > + interrupts: > > + [...] > > + interrupt-names: > > + [...] > > + additionalProperties: false * here should have been "true" of course. > > + [...] > > > > baikal,bt1-ddrc.yaml: > > + [...] > > + allOf: > > + - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#" > > + [...] > > + unevaluateProperties: false > > + [...] > > > > Thus the common schema as before would provide the widest set of the > > properties and their constraints, while the vendor-specific one would > > be !obligated! to follow the common schema conventions, but provide a > > more specific set of the properties and constraints. A similar > > approach is implemented for instance in the DW USB3 DT-schemas, but > > with the generic compatible string fallback. In this case we don't > > need the fallback string, but in order for the common schema being > > applicable for both the common and vendor-specific DT-nodes the > > compatible property constraints need to be designed as is provided in > > the example above. > > Anyway, it's getting complicated so I am not sure about which option now > we discuss. You cannot have deprecated entries in select and compatibles > described there, without describing them in properties. Ok. That's what I intended in the later approach. I'll resend the series with the updated bindings. We can continue the discussion after that in the new emails thread. -Sergey > > > > > Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into > > the two ones: > > snps,dw-umctl2-ddrc-common.yaml > > and > > snps,dw-umctl2-ddrc.yaml > > So the first schema would contain all the common properties definition > > and would be only used for being referenced in the particular device > > DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and > > vendor-specific DT-schemas would just have it "allOf:"ed. > > > > Personally I'd prefer the design pattern with the always-true > > compatible property constraint as in the example above since it seems > > easier to maintain than having the common and generic device > > DT-schemas. > > > > Note having a common DT-schema and a vendor-specific one referencing > > the common schema is very much useful practice for the devices based > > on the same IP-core. Vendor-device driver authors tend to create their > > own bindings for the same clocks/resets/phys/etc thus making the > > drivers much harder to maintain (for a bright example see what has > > been done for the DW PCIe RP/EP IP-core). The worst part of it is that > > ones the DT-bindings interface is implemented it can't be changed > > since the kernel needs to be backward compatible with the DT-sources > > compiled before. So the main goal of the common DT-schema is to fix > > the common DT interface and make sure the new vendor-specific device > > drivers would at least consider either to follow the IP-core DT > > convention or to widen it up if required. > > > Best regards, > Krzysztof
Hi Rob, Sorry for missing your message. It has kind of got lost among the Krzysztof' comments. On Tue, Aug 30, 2022 at 01:00:28PM -0500, Rob Herring wrote: > On Mon, Aug 22, 2022 at 10:19:45PM +0300, Serge Semin wrote: > > Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a > > with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There > > are individual IRQs for each ECC and DFI events.The dedicated scrubber > > clock source is absent since it's fully synchronous to the core clock. > > In addition to that the DFI-DDR PHY CSRs can be accessed via a separate > > registers space. > > Are you sure the phy and dfi irq shouldn't be a separate device? I am sure that the DFI IRQ is a part of the DW uMCTl2 DDR controller specification. The DFI interface has a special signal called "dfi_alert_n". It is supposed to be supplied to the DDR controller from the DDR PHY. The signal state indicates the CRC/Parity errors detected on the address/command sent to the PHY/SDRAM side. Aside with some other statuses the signal state is reflected in the DW uMCTL2 CRCPARSTAT register. The CSR state in its turn is then sent out via the corresponding output wire (dfi_err_int) up to the IRQ controller. So to speak there is no doubts the DFI errors IRQ is a part of the DW uMCTL2 DDRC IRQs interface. Regarding the PHY CSR space. You are right. It is a separate device indeed. I'll drop the PHY CSR region from here. -Sergey > > Rob
diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml index 8db92210cfe1..899a6c5f9806 100644 --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml @@ -26,6 +26,7 @@ properties: enum: - snps,ddrc-3.80a - xlnx,zynqmp-ddrc-2.40a + - baikal,bt1-ddrc interrupts: description: @@ -49,7 +50,14 @@ properties: enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ] reg: - maxItems: 1 + minItems: 1 + maxItems: 2 + + reg-names: + minItems: 1 + items: + - const: umctl2 + - const: phy clocks: description:
Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There are individual IRQs for each ECC and DFI events.The dedicated scrubber clock source is absent since it's fully synchronous to the core clock. In addition to that the DFI-DDR PHY CSRs can be accessed via a separate registers space. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- .../memory-controllers/snps,dw-umctl2-ddrc.yaml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)