Re: [PATCH v4 2/3] dt-bindings: pinctrl: Add aspeed,ast2700-soc0-pinctrl

[Billy Tsai]

...

> What remains unclear is how best to model the secondary and tertiary
> service processor control blocks (SSP/TSP) and the PCI control region.
> The SSP and TSP controls consume reserved memory regions used to
> configure the processors' address spaces, while the PCI control region
> contains the VGA scratch registers used to communicate host display
> ownership.

> The p2a and smp-ram features are supported on earlier generations
> but are not present on AST2700, and should be disabled in the binding
> for the AST2700 as you suggested.

Hi Krzysztof,

Following up on my previous email, I've reworked the AST2700 SCU0
binding to better reflect the hardware capabilities and address the
issues discussed.

In particular:
- Restrict p2a-control and smp-memram for AST2700, as these features
  are not present.
- Add SSP/TSP address mapping support using memory-region and
  memory-region-names.
- Rework the example so it reflects the AST2700 SCU0 layout and validates
  against the binding schema.

I wanted to share the current binding update for feedback.

Below is the current SCU0 binding diff:

diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
index a87f31fce019..86e560876cf2 100644
--- a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
+++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
@@ -46,6 +46,9 @@ properties:
   '#reset-cells':
     const: 1
 
+  memory-region: true
+  memory-region-names: true
+
 patternProperties:
   '^p2a-control@[0-9a-f]+$':
     description: >
@@ -87,6 +90,7 @@ patternProperties:
             - aspeed,ast2400-pinctrl
             - aspeed,ast2500-pinctrl
             - aspeed,ast2600-pinctrl
+            - aspeed,ast2700-soc0-pinctrl
 
     required:
       - compatible
@@ -156,6 +160,42 @@ required:
   - '#clock-cells'
   - '#reset-cells'
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            anyOf:
+              - const: aspeed,ast2700-scu0
+              - const: aspeed,ast2700-scu1
+    then:
+      patternProperties:
+        '^p2a-control@[0-9a-f]+$': false
+        '^smp-memram@[0-9a-f]+$': false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2700-scu0
+    then:
+      properties:
+        memory-region:
+          items:
+            - description: Region mapped through the first SSP address window.
+            - description: Region mapped through the second SSP address window.
+            - description: Region mapped through the TSP address window.
+
+        memory-region-names:
+          items:
+            - const: ssp-0
+            - const: ssp-1
+            - const: tsp
+    else:
+      properties:
+        memory-region: false
+        memory-region-names: false
+
 additionalProperties: false
 
 examples:
@@ -180,4 +220,81 @@ examples:
             reg = <0x7c 0x4>, <0x150 0x8>;
         };
     };
+
+  - |
+    / {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        reserved-memory {
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges;
+
+            ssp_region_0: memory@90000000 {
+                reg = <0x4 0x00000000 0x0 0x01000000>;
+                no-map;
+            };
+
+            ssp_region_1: memory@91000000 {
+                reg = <0x4 0x01000000 0x0 0x01000000>;
+                no-map;
+            };
+
+            tsp_region: memory@92000000 {
+                reg = <0x4 0x02000000 0x0 0x01000000>;
+                no-map;
+            };
+        };
+
+        bus {
+            #address-cells = <2>;
+            #size-cells = <2>;
+
+            syscon@12c02000 {
+                compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd";
+                reg = <0 0x12c02000 0 0x1000>;
+                ranges = <0x0 0x0 0x12c02000 0x1000>;
+                #address-cells = <1>;
+                #size-cells = <1>;
+                #clock-cells = <1>;
+                #reset-cells = <1>;
+
+                memory-region = <&ssp_region_0>, <&ssp_region_1>,
+                                <&tsp_region>;
+                memory-region-names = "ssp-0", "ssp-1", "tsp";
+
+                silicon-id@0 {
+                    compatible = "aspeed,ast2700-silicon-id", "aspeed,silicon-id";
+                    reg = <0x0 0x4>;
+                };
+
+                interrupt-controller@1b0 {
+                    compatible = "aspeed,ast2700-scu-ic0";
+                    reg = <0x1b0 0x4>;
+                    #interrupt-cells = <1>;
+                    interrupts-extended = <&intc0 97>;
+                    interrupt-controller;
+                };
+
+                interrupt-controller@1e0 {
+                    compatible = "aspeed,ast2700-scu-ic1";
+                    reg = <0x1e0 0x4>;
+                    #interrupt-cells = <1>;
+                    interrupts-extended = <&intc0 98>;
+                    interrupt-controller;
+                };
+
+                pinctrl@400 {
+                    compatible = "aspeed,ast2700-soc0-pinctrl";
+                    reg = <0x400 0x318>;
+                    emmc-state {
+                        function = "EMMC";
+                        groups = "EMMCG1";
+                    };
+                };
+            };
+        };
+    };
+
 ...

One additional question regarding the example: Since the AST2700 SCU0
node is not significantly different from previous generations and the
example mostly uses standard properties, do you think keeping this
additional example is useful, or would you prefer to keep the binding
minimal?

Thanks
Billy Tsai