diff mbox series

[v2,1/7] dt-bindings: mfd: ti,j721e-system-controller.yaml: Document "syscon"

Message ID 20201109170409.4498-2-kishon@ti.com
State New
Headers show
Series J7200: Add PCIe DT nodes to Enable PCIe | expand

Commit Message

Kishon Vijay Abraham I Nov. 9, 2020, 5:04 p.m. UTC 
Add binding documentation for "syscon" which should be a subnode of
the system controller (scm-conf).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

---
 .../mfd/ti,j721e-system-controller.yaml       | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

-- 
2.17.1

Comments

Rob Herring Nov. 11, 2020, 9:28 p.m. UTC | #1 
On Mon, Nov 09, 2020 at 10:34:03PM +0530, Kishon Vijay Abraham I wrote:
> Add binding documentation for "syscon" which should be a subnode of

> the system controller (scm-conf).

> 

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---

>  .../mfd/ti,j721e-system-controller.yaml       | 40 +++++++++++++++++++

>  1 file changed, 40 insertions(+)

> 

> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

> index 19fcf59fd2fe..0b115b707ab2 100644

> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

> @@ -50,6 +50,38 @@ patternProperties:

>        specified in

>        Documentation/devicetree/bindings/mux/reg-mux.txt

>  

> +  "^syscon@[0-9a-f]+$":

> +    type: object

> +    description: |


Don't need '|' if there's no formatting.

> +      This is the system controller configuration required to configure PCIe

> +      mode, lane width and speed.

> +

> +    properties:

> +      compatible:

> +        items:

> +          - enum:

> +              - ti,j721e-system-controller

> +          - const: syscon

> +          - const: simple-mfd


Humm, then what are this node's sub-nodes? And the same compatible as 
the parent?

> +

> +      reg:

> +        maxItems: 1

> +

> +      "#address-cells":

> +        const: 1

> +

> +      "#size-cells":

> +        const: 1

> +

> +      ranges: true

> +

> +    required:

> +      - compatible

> +      - reg

> +      - "#address-cells"

> +      - "#size-cells"

> +      - ranges

> +

>  required:

>    - compatible

>    - reg

> @@ -72,5 +104,13 @@ examples:

>              compatible = "mmio-mux";

>              reg = <0x00004080 0x50>;

>          };

> +

> +        pcie1_ctrl: syscon@4074 {

> +            compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";

> +            reg = <0x00004074 0x4>;

> +            #address-cells = <1>;

> +            #size-cells = <1>;

> +            ranges = <0x4074 0x4074 0x4>;


Must be packing a bunch of functions into 4 byte region!

> +        };

>      };

>  ...

> -- 

> 2.17.1

>
Kishon Vijay Abraham I Nov. 12, 2020, 5:25 a.m. UTC | #2 
Hi Rob,

On 12/11/20 2:58 am, Rob Herring wrote:
> On Mon, Nov 09, 2020 at 10:34:03PM +0530, Kishon Vijay Abraham I wrote:

>> Add binding documentation for "syscon" which should be a subnode of

>> the system controller (scm-conf).

>>

>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>> ---

>>  .../mfd/ti,j721e-system-controller.yaml       | 40 +++++++++++++++++++

>>  1 file changed, 40 insertions(+)

>>

>> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

>> index 19fcf59fd2fe..0b115b707ab2 100644

>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

>> @@ -50,6 +50,38 @@ patternProperties:

>>        specified in

>>        Documentation/devicetree/bindings/mux/reg-mux.txt

>>  

>> +  "^syscon@[0-9a-f]+$":

>> +    type: object

>> +    description: |

> 

> Don't need '|' if there's no formatting.


Okay, will fix this.
> 

>> +      This is the system controller configuration required to configure PCIe

>> +      mode, lane width and speed.

>> +

>> +    properties:

>> +      compatible:

>> +        items:

>> +          - enum:

>> +              - ti,j721e-system-controller

>> +          - const: syscon

>> +          - const: simple-mfd

> 

> Humm, then what are this node's sub-nodes? And the same compatible as 

> the parent?

> 


This node doesn't have sub-nodes.

So one is the parent syscon node which has the entire system control
region and then sub-nodes for each of the modules. In this case the PCIe
in system control has only one 4 byte register that has to be configured.

Both the parent node and sub-node are syscon, so given the same
compatible for both.
>> +

>> +      reg:

>> +        maxItems: 1

>> +

>> +      "#address-cells":

>> +        const: 1

>> +

>> +      "#size-cells":

>> +        const: 1

>> +

>> +      ranges: true

>> +

>> +    required:

>> +      - compatible

>> +      - reg

>> +      - "#address-cells"

>> +      - "#size-cells"

>> +      - ranges

>> +

>>  required:

>>    - compatible

>>    - reg

>> @@ -72,5 +104,13 @@ examples:

>>              compatible = "mmio-mux";

>>              reg = <0x00004080 0x50>;

>>          };

>> +

>> +        pcie1_ctrl: syscon@4074 {

>> +            compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";

>> +            reg = <0x00004074 0x4>;

>> +            #address-cells = <1>;

>> +            #size-cells = <1>;

>> +            ranges = <0x4074 0x4074 0x4>;

> 

> Must be packing a bunch of functions into 4 byte region!


For the PCIe case, it only has a 4 byte register to be configured. The
other option would be to get phandle to the parent syscon node and then
hard-code the offset in the driver.

Thank You,
Kishon
Rob Herring Nov. 12, 2020, 4:46 p.m. UTC | #3 
On Wed, Nov 11, 2020 at 11:25 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>

> Hi Rob,

>

> On 12/11/20 2:58 am, Rob Herring wrote:

> > On Mon, Nov 09, 2020 at 10:34:03PM +0530, Kishon Vijay Abraham I wrote:

> >> Add binding documentation for "syscon" which should be a subnode of

> >> the system controller (scm-conf).

> >>

> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> >> ---

> >>  .../mfd/ti,j721e-system-controller.yaml       | 40 +++++++++++++++++++

> >>  1 file changed, 40 insertions(+)

> >>

> >> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

> >> index 19fcf59fd2fe..0b115b707ab2 100644

> >> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

> >> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml

> >> @@ -50,6 +50,38 @@ patternProperties:

> >>        specified in

> >>        Documentation/devicetree/bindings/mux/reg-mux.txt

> >>

> >> +  "^syscon@[0-9a-f]+$":

> >> +    type: object

> >> +    description: |

> >

> > Don't need '|' if there's no formatting.

>

> Okay, will fix this.

> >

> >> +      This is the system controller configuration required to configure PCIe

> >> +      mode, lane width and speed.

> >> +

> >> +    properties:

> >> +      compatible:

> >> +        items:

> >> +          - enum:

> >> +              - ti,j721e-system-controller

> >> +          - const: syscon

> >> +          - const: simple-mfd

> >

> > Humm, then what are this node's sub-nodes? And the same compatible as

> > the parent?

> >

>

> This node doesn't have sub-nodes.

>

> So one is the parent syscon node which has the entire system control

> region and then sub-nodes for each of the modules. In this case the PCIe

> in system control has only one 4 byte register that has to be configured.

>

> Both the parent node and sub-node are syscon, so given the same

> compatible for both.


'syscon' is just a hint. It doesn't define what any h/w is. IMO, we
never should have added it.

A compatible defines what the programming interface is for the node.
This one should only ever appear more than once if you have multiple
instances of the same block. So different registers, different
compatible. What you have here is just completely broken.

I don't think you even need a child node here. Just have PCIe node
point to the parent with an offset arg.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
index 19fcf59fd2fe..0b115b707ab2 100644
--- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
@@ -50,6 +50,38 @@  patternProperties:
       specified in
       Documentation/devicetree/bindings/mux/reg-mux.txt
 
+  "^syscon@[0-9a-f]+$":
+    type: object
+    description: |
+      This is the system controller configuration required to configure PCIe
+      mode, lane width and speed.
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - ti,j721e-system-controller
+          - const: syscon
+          - const: simple-mfd
+
+      reg:
+        maxItems: 1
+
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 1
+
+      ranges: true
+
+    required:
+      - compatible
+      - reg
+      - "#address-cells"
+      - "#size-cells"
+      - ranges
+
 required:
   - compatible
   - reg
@@ -72,5 +104,13 @@  examples:
             compatible = "mmio-mux";
             reg = <0x00004080 0x50>;
         };
+
+        pcie1_ctrl: syscon@4074 {
+            compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";
+            reg = <0x00004074 0x4>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges = <0x4074 0x4074 0x4>;
+        };
     };
 ...