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

[Kishon Vijay Abraham I]

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@xxxxxx>
>> ---
>>  .../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