netdev - Re: [PATCH 18/43] dt-bindings: net: Add DT bindings ep93xx eth

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>] [day] [month] [year] [list]

Date: Mon, 15 May 2023 16:42:57 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: Andrew Lunn <andrew@...n.ch>
Cc: Arnd Bergmann <arnd@...nel.org>, Linus Walleij <linusw@...nel.org>, 
 Alexander Sverdlin <alexander.sverdlin@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Hartley Sweeten
 <hsweeten@...ionengravers.com>, netdev@...r.kernel.org, 
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/43] dt-bindings: net: Add DT bindings ep93xx eth

Hello Andrew!

On Mon, 2023-04-24 at 15:39 +0200, Andrew Lunn wrote:
> > +  copy_addr:
> > +    type: boolean
> > +    description:
> > +      Flag indicating that the MAC address should be copied
> > +      from the IndAd registers (as programmed by the bootloader)
> 
> Looking at ep93xx_register_eth(), all callers are setting copy_addr
> to
> 1. So i don't think you need this.

Agreed. Dropped copy_addr entirely.

> 
> > +
> > +  phy_id:
> > +    description: MII phy_id to use
> 
> The eEP93xx Ethernet driver is a very old driver, so it is doing MDIO
> and PHY the old way. Ideally you should be using ep93xx_mdio_read()
> and ep93xx_mdio_write() to create an MDIO bus with
> of_mdiobus_regsiter, and then use a phy-handle to point to the PHY on
> the bus. It will then be the same as all other ethernet drivers using
> DT.

I've tinkered with the preferred way, however this involves turning on 

- CONFIG_PHYLIB
- CONFIG_MDIO_DEVICE

And maybe CONFIG_MICREL_PHY (at least for me, unless i can use some
common phy driver) which implies a kernel size increase - which is
undesirable for us.

Can we slip by with something like:

+       np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
+       if (!np) {
+               dev_err(&pdev->dev, "Please provide \"phy-handle\"\n");
+               return -ENODEV;
+       }
+
+       if (of_property_read_u32(np, "reg", &phy_id)) {
+               dev_err(&pdev->dev, "Failed to locate \"phy_id\"\n");
+               return -ENOENT;
+       }

And standard device tree bindings ?:

+    ethernet@...10000 {
+      compatible = "cirrus,ep9301-eth";
+      reg = <0x80010000 0x10000>;
+      interrupt-parent = <&vic1>;
+      interrupts = <7>;
+      phy-handle = <&phy0>;
+      mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        phy0: ethernet-phy@1 {
+          reg = <1>;
+          device_type = "ethernet-phy";
+        };
+      };
+    };

> 
>     Andrew