netdev - Re: [PATCH net-next v3 2/8] dt-bindings: phy: add the "fsl,lynx-28g" compatible

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 10 Mar 2022 22:19:45 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kishon@...com" <kishon@...com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        Leo Li <leoyang.li@....com>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Hongxing Zhu <hongxing.zhu@....com>
Subject: Re: [PATCH net-next v3 2/8] dt-bindings: phy: add the "fsl,lynx-28g"
 compatible

On 10/03/2022 18:32, Ioana Ciornei wrote:
> On Thu, Mar 10, 2022 at 05:47:31PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2022 15:51, Ioana Ciornei wrote:
>>> Describe the "fsl,lynx-28g" compatible used by the Lynx 28G SerDes PHY
>>> driver on Layerscape based SoCs.
>>
>> The message is a bit misleading, because it suggests you add only
>> compatible to existing bindings. Instead please look at the git log how
>> people usually describe it in subject and message.
> 
> Sure, I can change the title and commit message.
> 
>>> +patternProperties:
>>> +  '^phy@[0-9a-f]$':
>>> +    type: object
>>> +    properties:
>>> +      reg:
>>> +        description:
>>> +          Number of the SerDes lane.
>>> +        minimum: 0
>>> +        maximum: 7
>>> +
>>> +      "#phy-cells":
>>> +        const: 0
>>
>> Why do you need all these children? You just enumerated them, without
>> statuses, resources or any properties. This should be rather just index
>> of lynx-28g phy.
> 
> I am just describing each lane of the SerDes block so that each ethernet
> dts node references it directly.

Instead, phy user should reference phy device node and phy ID. Just like
we do for other providers (everything with #xxxxx-cells).

> Since I am new to the generic PHY infrastructure I was using the COMPHY
> for the Marvell MVEBU SoCs (phy-mvebu-comphy.txt) as a loose example.

I don't know it but it might not be the best example... Just because we
have already some solution it does not mean it is good. :)

> Each lane there is described as a different child node as well. The only
> difference from the COMPHY is that Lynx 28G does not need #phy-cells =
> <1> to reference the input port, we just use '#phy-cells = <0>' on each
> lane.
> 
> What is wrong with this approach? Or better, is there an easier way to
> do this?

Because the nodes look artificial. It looks like you have nodes only
differentiate by index. As I said before - there are no other properties
in these nodes.

Imagine now a clock provider with 500 clocks like this...

The easier approach, especially since you have a shared registers, is to
use phy-cells = 1, without artificial nodes just to pass the index.

It would be entirely different if you actually had any properties in the
children. IOW, if these were actually some blocks with their own
characteristics and programming model.

Best regards,
Krzysztof