[PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller

Piotr Kubik posted 2 patches 9 months ago
There is a newer version of this series
[PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Piotr Kubik 9 months ago
From: Piotr Kubik <piotr.kubik@adtran.com>

Add the Si3474 I2C Power Sourcing Equipment controller device tree
bindings documentation.

Signed-off-by: Piotr Kubik <piotr.kubik@adtran.com>
---
 .../bindings/net/pse-pd/skyworks,si3474.yaml  | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml

diff --git a/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
new file mode 100644
index 000000000000..646924a3cfb0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pse-pd/skyworks,si3474.yaml
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pse-pd/skyworks,si3474.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Skyworks Si3474 Power Sourcing Equipment controller
+
+maintainers:
+  - Piotr Kubik <piotr.kubik@adtran.com>
+
+allOf:
+  - $ref: pse-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - skyworks,si3474
+
+  reg-names:
+    items:
+      - const: main
+      - const: slave
+
+  reg:
+    maxItems: 2
+
+  channels:
+    description: The Si3474 is a single-chip PoE PSE controller managing
+      8 physical power delivery channels. Internally, it's structured
+      into two logical "Quads".
+      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
+      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
+      This parameter describes the relationship between the logical and
+      the physical power channels.
+
+    type: object
+    additionalProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      '^channel@[0-7]$':
+        type: object
+        additionalProperties: false
+
+        properties:
+          reg:
+            maxItems: 1
+
+        required:
+          - reg
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+required:
+  - compatible
+  - reg
+  - pse-pis
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ethernet-pse@26 {
+        compatible = "skyworks,si3474";
+        reg-names = "main", "slave";
+        reg = <0x26>, <0x27>;
+
+        channels {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          phys0_0: channel@0 {
+            reg = <0>;
+          };
+          phys0_1: channel@1 {
+            reg = <1>;
+          };
+          phys0_2: channel@2 {
+            reg = <2>;
+          };
+          phys0_3: channel@3 {
+            reg = <3>;
+          };
+          phys0_4: channel@4 {
+            reg = <4>;
+          };
+          phys0_5: channel@5 {
+            reg = <5>;
+          };
+          phys0_6: channel@6 {
+            reg = <6>;
+          };
+          phys0_7: channel@7 {
+            reg = <7>;
+          };
+        };
+        pse-pis {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          pse_pi0: pse-pi@0 {
+            reg = <0>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_0>, <&phys0_1>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+          pse_pi1: pse-pi@1 {
+            reg = <1>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_2>, <&phys0_3>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+          pse_pi2: pse-pi@2 {
+            reg = <2>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_4>, <&phys0_5>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+          pse_pi3: pse-pi@3 {
+            reg = <3>;
+            #pse-cells = <0>;
+            pairset-names = "alternative-a", "alternative-b";
+            pairsets = <&phys0_6>, <&phys0_7>;
+            polarity-supported = "MDI-X", "S";
+            vpwr-supply = <&reg_pse>;
+          };
+        };
+      };
+    };
-- 
2.43.0

Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Krzysztof Kozlowski 9 months ago
On 13/05/2025 00:05, Piotr Kubik wrote:
> +
> +maintainers:
> +  - Piotr Kubik <piotr.kubik@adtran.com>
> +
> +allOf:
> +  - $ref: pse-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - skyworks,si3474
> +
> +  reg-names:
> +    items:
> +      - const: main
> +      - const: slave

s/slave/secondary/ (or whatever is there in recommended names in coding
style)

> +
> +  reg:

First reg, then reg-names. Please follow other bindings/examples.

> +    maxItems: 2
> +
> +  channels:
> +    description: The Si3474 is a single-chip PoE PSE controller managing
> +      8 physical power delivery channels. Internally, it's structured
> +      into two logical "Quads".
> +      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
> +      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
> +      This parameter describes the relationship between the logical and
> +      the physical power channels.

How exactly this maps here logical and physical channels? You just
listed channels one after another...

> +
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    patternProperties:
> +      '^channel@[0-7]$':
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          reg:
> +            maxItems: 1
> +
Best regards,
Krzysztof
Re: [EXTERNAL]Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Piotr Kubik 8 months, 4 weeks ago
On 5/13/25 10:24, Krzysztof Kozlowski wrote:
> On 13/05/2025 00:05, Piotr Kubik wrote:
>> +
>> +maintainers:
>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>> +
>> +allOf:
>> +  - $ref: pse-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - skyworks,si3474
>> +
>> +  reg-names:
>> +    items:
>> +      - const: main
>> +      - const: slave
> 
> s/slave/secondary/ (or whatever is there in recommended names in coding
> style)
> 

Well I was thinking about it and decided to use 'slave' for at least two reasons:
- si3474 datasheet calls the second part of IC (we configure it here) this way
- description of i2c_new_ancillary_device() calls this device explicitly slave multiple times

>> +
>> +  reg:
> 
> First reg, then reg-names. Please follow other bindings/examples.
> 
>> +    maxItems: 2
>> +
>> +  channels:
>> +    description: The Si3474 is a single-chip PoE PSE controller managing
>> +      8 physical power delivery channels. Internally, it's structured
>> +      into two logical "Quads".
>> +      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
>> +      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
>> +      This parameter describes the relationship between the logical and
>> +      the physical power channels.
> 
> How exactly this maps here logical and physical channels? You just
> listed channels one after another...

yes, here in this example it is 1 to 1 simple mapping, but in a real world,
depending on hw connections, there is a possibility that 
e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.

> 
>> +
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      '^channel@[0-7]$':
>> +        type: object
>> +        additionalProperties: false
>> +
>> +        properties:
>> +          reg:
>> +            maxItems: 1
>> +
> Best regards,
> Krzysztof

Thanks
/Piotr
Re: [EXTERNAL]Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Kory Maincent 8 months, 4 weeks ago
On Thu, 15 May 2025 15:20:40 +0000
Piotr Kubik <piotr.kubik@adtran.com> wrote:

> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
> > On 13/05/2025 00:05, Piotr Kubik wrote:  
> >> +
> >> +maintainers:
> >> +  - Piotr Kubik <piotr.kubik@adtran.com>
> >> +
> >> +allOf:
> >> +  - $ref: pse-controller.yaml#
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - skyworks,si3474
> >> +
> >> +  reg-names:
> >> +    items:
> >> +      - const: main
> >> +      - const: slave  
> > 
> > s/slave/secondary/ (or whatever is there in recommended names in coding
> > style)
> >   
> 
> Well I was thinking about it and decided to use 'slave' for at least two
> reasons:
> - si3474 datasheet calls the second part of IC (we configure it here) this way
> - description of i2c_new_ancillary_device() calls this device explicitly
> slave multiple times

It is better to avoid the usage of such word in new code. Secondary suits well
for replacement.

> >> +
> >> +  reg:  
> > 
> > First reg, then reg-names. Please follow other bindings/examples.
> >   
> >> +    maxItems: 2
> >> +
> >> +  channels:
> >> +    description: The Si3474 is a single-chip PoE PSE controller managing
> >> +      8 physical power delivery channels. Internally, it's structured
> >> +      into two logical "Quads".
> >> +      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
> >> +      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
> >> +      This parameter describes the relationship between the logical and
> >> +      the physical power channels.  
> > 
> > How exactly this maps here logical and physical channels? You just
> > listed channels one after another...  
> 
> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
> depending on hw connections, there is a possibility that 
> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.

But here you should describe the channels of the controller and the channel has
no link to the relationship between logical and physical power channels. This
relationship rather is described in the "pairsets" parameter of PSE PI.

Maybe something like that:

The Si3474 is a single-chip PoE PSE controller managing 8 physical power
delivery channels. Internally, it's structured into two logical "Quads".
Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
This parameter defines the 8 physical delivery channels on the controller that
can be referenced by PSE PIs through their "pairsets" property. The actual port
matrix mapping is created when PSE PIs reference these channels in their
pairsets. For 4-pair operation, two channels from the same group (0-3 or 4-7)
must be referenced by a single PSE PI.

Similarly the description I used on the tps23881 is also not correct. I have to
change it.
 
I didn't look into the datasheet, could we have parameters specific to a
quad? If that the case we maybe should have something like that:
          quad0: quad@0 {                                                 
            reg = <0>;                                                          
            #address-cells = <1>;                                               
            #size-cells = <0>;                                                                                       
                                                                                
            phys0: port@0 {                                                     
              reg = <0>;                                                        
            };                                                                  
                                                                                
            phys1: port@1 {                                                     
              reg = <1>;                                                        
            };                                                                  
                                                                                
            phys2: port@2 {                                                     
              reg = <2>;                                                        
            };                                                                  
                                                                                
            phys3: port@3 {                                                     
              reg = <3>;                                                        
            };                                                                  
          };                                                                    
                                                                                
          quad@1 {                                                           
            reg = <1>;                                                          
            #address-cells = <1>;                                               
            #size-cells = <0>;                                                  
                                                                                
            phys4: port@0 {                                                     
              reg = <0>;                                                        
            };                                                                  
                                                                                
            phys5: port@1 {                                                     
              reg = <1>;                                                        
            };                                                                  
                                                                                
            phys6: port@2 {                                                     
              reg = <2>;                                                        
            };                                                                  
                                                                                
            phys7: port@3 {                                                     
              reg = <3>;                                                        
            };                                                                  
          };                                                                    
        };

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Piotr Kubik 8 months, 3 weeks ago
On 5/17/25 00:35, Kory Maincent wrote:
> On Thu, 15 May 2025 15:20:40 +0000
> Piotr Kubik <piotr.kubik@adtran.com> wrote:
> 
>> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
>>> On 13/05/2025 00:05, Piotr Kubik wrote:  
>>>> +
>>>> +maintainers:
>>>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pse-controller.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - skyworks,si3474
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: main
>>>> +      - const: slave  
>>>
>>> s/slave/secondary/ (or whatever is there in recommended names in coding
>>> style)
>>>   
>>
>> Well I was thinking about it and decided to use 'slave' for at least two
>> reasons:
>> - si3474 datasheet calls the second part of IC (we configure it here) this way
>> - description of i2c_new_ancillary_device() calls this device explicitly
>> slave multiple times
> 
> It is better to avoid the usage of such word in new code. Secondary suits well
> for replacement.
> 
OK, I replaced it already in v3:
https://lore.kernel.org/netdev/ebe9a9f5-db9c-4b82-a5e9-3b108a0c6338@adtran.com

>>>> +
>>>> +  reg:  
>>>
>>> First reg, then reg-names. Please follow other bindings/examples.
>>>   
>>>> +    maxItems: 2
>>>> +
>>>> +  channels:
>>>> +    description: The Si3474 is a single-chip PoE PSE controller managing
>>>> +      8 physical power delivery channels. Internally, it's structured
>>>> +      into two logical "Quads".
>>>> +      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
>>>> +      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
>>>> +      This parameter describes the relationship between the logical and
>>>> +      the physical power channels.  
>>>
>>> How exactly this maps here logical and physical channels? You just
>>> listed channels one after another...  
>>
>> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
>> depending on hw connections, there is a possibility that 
>> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.
> 
> But here you should describe the channels of the controller and the channel has
> no link to the relationship between logical and physical power channels. This
> relationship rather is described in the "pairsets" parameter of PSE PI.
> 

Exactly, I got to the same conclusion, that it is the pse-pis node responsibility 
and removed that part in v3.
Commented it in v2:
https://lore.kernel.org/netdev/0b7afab0-0283-4a52-82bc-0d492f752034@adtran.com/

> Maybe something like that:
> 
> The Si3474 is a single-chip PoE PSE controller managing 8 physical power
> delivery channels. Internally, it's structured into two logical "Quads".
> Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
> Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
> This parameter defines the 8 physical delivery channels on the controller that
> can be referenced by PSE PIs through their "pairsets" property. The actual port
> matrix mapping is created when PSE PIs reference these channels in their
> pairsets. For 4-pair operation, two channels from the same group (0-3 or 4-7)
> must be referenced by a single PSE PI.
> 
> Similarly the description I used on the tps23881 is also not correct. I have to
> change it.

I like it. I will update.
>  
> I didn't look into the datasheet, could we have parameters specific to a
> quad? If that the case we maybe should have something like that:
>           quad0: quad@0 {                                                 
>             reg = <0>;                                                          
>             #address-cells = <1>;                                               
>             #size-cells = <0>;                                                                                       
>                                                                                 
>             phys0: port@0 {                                                     
>               reg = <0>;                                                        
>             };                                                                  
>                                                                                 
>             phys1: port@1 {                                                     
>               reg = <1>;                                                        
>             };                                                                  
>                                                                                 
>             phys2: port@2 {                                                     
>               reg = <2>;                                                        
>             };                                                                  
>                                                                                 
>             phys3: port@3 {                                                     
>               reg = <3>;                                                        
>             };                                                                  
>           };                                                                    
>                                                                                 
>           quad@1 {                                                           
>             reg = <1>;                                                          
>             #address-cells = <1>;                                               
>             #size-cells = <0>;                                                  
>                                                                                 
>             phys4: port@0 {                                                     
>               reg = <0>;                                                        
>             };                                                                  
>                                                                                 
>             phys5: port@1 {                                                     
>               reg = <1>;                                                        
>             };                                                                  
>                                                                                 
>             phys6: port@2 {                                                     
>               reg = <2>;                                                        
>             };                                                                  
>                                                                                 
>             phys7: port@3 {                                                     
>               reg = <3>;                                                        
>             };                                                                  
>           };                                                                    
>         };
> 
No, I don't see any quad specific parameters, except i2c address we define the level above. 
So I think this would be over-engineered.

> Regards,

Thanks
/Piotr
Re: [EXTERNAL]Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Krzysztof Kozlowski 8 months, 4 weeks ago
On 15/05/2025 17:20, Piotr Kubik wrote:
> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
>> On 13/05/2025 00:05, Piotr Kubik wrote:
>>> +
>>> +maintainers:
>>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>>> +
>>> +allOf:
>>> +  - $ref: pse-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - skyworks,si3474
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: main
>>> +      - const: slave
>>
>> s/slave/secondary/ (or whatever is there in recommended names in coding
>> style)
>>
> 
> Well I was thinking about it and decided to use 'slave' for at least two reasons:
> - si3474 datasheet calls the second part of IC (we configure it here) this way


This could be a reason, but specs are changing over time (see I2C, I3C)
to include different namings. If this annoys certain government sending
their executive directives, then even better.


> - description of i2c_new_ancillary_device() calls this device explicitly slave multiple times

Old driver code should not be an argument. If code changes, which it can
anytime, are you going to change binding? No, because such change in the
binding would not be allowed.

> 
>>> +
>>> +  reg:
>>
>> First reg, then reg-names. Please follow other bindings/examples.
>>
>>> +    maxItems: 2
>>> +
>>> +  channels:
>>> +    description: The Si3474 is a single-chip PoE PSE controller managing
>>> +      8 physical power delivery channels. Internally, it's structured
>>> +      into two logical "Quads".
>>> +      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
>>> +      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
>>> +      This parameter describes the relationship between the logical and
>>> +      the physical power channels.
>>
>> How exactly this maps here logical and physical channels? You just
>> listed channels one after another...
> 
> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
> depending on hw connections, there is a possibility that 
> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.
> 

Ack, I see that's actually common for pse-pd. It's fine.


Best regards,
Krzysztof
Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for Si3474 PSE controller
Posted by Piotr Kubik 8 months, 4 weeks ago
On 5/16/25 15:37, Krzysztof Kozlowski wrote:
> On 15/05/2025 17:20, Piotr Kubik wrote:
>> On 5/13/25 10:24, Krzysztof Kozlowski wrote:
>>> On 13/05/2025 00:05, Piotr Kubik wrote:
>>>> +
>>>> +maintainers:
>>>> +  - Piotr Kubik <piotr.kubik@adtran.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pse-controller.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - skyworks,si3474
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: main
>>>> +      - const: slave
>>>
>>> s/slave/secondary/ (or whatever is there in recommended names in coding
>>> style)
>>>
>>
>> Well I was thinking about it and decided to use 'slave' for at least two reasons:
>> - si3474 datasheet calls the second part of IC (we configure it here) this way
> 
> 
> This could be a reason, but specs are changing over time (see I2C, I3C)
> to include different namings. If this annoys certain government sending
> their executive directives, then even better.
> 

OK, I've read some discussions regarding this issue and decided to rename here and in si3474 code.

> 
>> - description of i2c_new_ancillary_device() calls this device explicitly slave multiple times
> 
> Old driver code should not be an argument. If code changes, which it can
> anytime, are you going to change binding? No, because such change in the
> binding would not be allowed.
> 



>>
>>>> +
>>>> +  reg:
>>>
>>> First reg, then reg-names. Please follow other bindings/examples.
>>>
>>>> +    maxItems: 2
>>>> +
>>>> +  channels:
>>>> +    description: The Si3474 is a single-chip PoE PSE controller managing
>>>> +      8 physical power delivery channels. Internally, it's structured
>>>> +      into two logical "Quads".
>>>> +      Quad 0 Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
>>>> +      Quad 1 Manages physical channels ('ports' in datasheet) 4, 5, 6, 7.
>>>> +      This parameter describes the relationship between the logical and
>>>> +      the physical power channels.
>>>
>>> How exactly this maps here logical and physical channels? You just
>>> listed channels one after another...
>>
>> yes, here in this example it is 1 to 1 simple mapping, but in a real world,
>> depending on hw connections, there is a possibility that 
>> e.g. "pse_pi0" will use "<&phys0_4>, <&phys0_5>" pairset for lan port 3.
>>
> 
> Ack, I see that's actually common for pse-pd. It's fine.
> 
Actually, after some consideration I agreed with you and decided to remove this part 
in v3 as this 'channels' node does not really  describe HW mapping, the pse-pis part does.
v3: https://lore.kernel.org/netdev/ebe9a9f5-db9c-4b82-a5e9-3b108a0c6338@adtran.com/

> 
> Best regards,
> Krzysztof

Thanks
/Piotr