On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:

On 22/08/2022 22:19, Serge Semin wrote:

quoted

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.

quoted

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

quoted

In addition to that the DFI-DDR PHY CSRs can be accessed via a separate
registers space.

Signed-off-by: Serge Semin <redacted>
---
 .../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.

quoted

 
   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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel