| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50be886b-553c-4af7-b40e-3ab4163dd3e3@adtran.com>
Date: Sat, 17 May 2025 09:44:23 +0000
From: Piotr Kubik <piotr.kubik@...ran.com>
To: Kory Maincent <kory.maincent@...tlin.com>
CC: Krzysztof Kozlowski <krzk@...nel.org>, Oleksij Rempel
<o.rempel@...gutronix.de>, Andrew Lunn <andrew+netdev@...n.ch>, "David S.
Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: Add bindings for
Si3474 PSE controller
On 5/17/25 00:35, Kory Maincent wrote:
> On Thu, 15 May 2025 15:20:40 +0000
> Piotr Kubik <piotr.kubik@...ran.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@...ran.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
Powered by blists - more mailing lists