netdev - Re: [PATCH 0/3] staging: octeon: Convert to use phylink

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <b70d9361-c689-4837-bc9d-8e800cda380c@lunn.ch>
Date:   Thu, 13 Apr 2023 17:45:13 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ladislav Michl <oss-lists@...ops.cz>
Cc:     linux-staging@...ts.linux.dev, netdev@...r.kernel.org,
        linux-mips@...r.kernel.org,
        Chris Packham <chris.packham@...iedtelesis.co.nz>
Subject: Re: [PATCH 0/3] staging: octeon: Convert to use phylink

Hi Ladislav

For phylink questions, it is a good idea to Cc: the phylink
Maintainer. And for general PHY problems, Cc: the phy Maintainers.

On Thu, Apr 13, 2023 at 04:11:07PM +0200, Ladislav Michl wrote:
> The purpose of this patches is to provide support for SFP cage to
> Octeon ethernet driver. This is tested with following DT snippet:
> 
> 	smi0: mdio@...0000001800 {
> 		compatible = "cavium,octeon-3860-mdio";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		reg = <0x11800 0x00001800 0x0 0x40>;
> 
> 		/* QSGMII PHY */
> 		phy0: ethernet-phy@0 {
> 			compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";

Please don't use a compatible for the specific PHY. In fact,
compatibles are only used for things which are not PHYs, like Ethernet
switches. phylib reads the ID registers of the PHY and uses them to
load the correct PHY driver.

Also, C22 is the default, so you don't need that either.

> 			reg = <0>;
> 			interrupt-parent = <&gpio>;
> 			interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
> 			marvell,reg-init =
> 			  <0xff 24 0 0x2800>, <0xff 23 0 0x2001>, /* errata 3.1.1 - PHY Initialization #1 */
> 			  <0 29 0 3>, <0 30 0 2>, <0 29 0 0>,	  /* errata 3.1.2 - PHY Initialization #2 */

Please add C code to deal with these erratas in the marvell PHY
driver.

> 			  <4 26 0 0x2>, 			  /* prekrizeni RX a TX QSGMII sbernice */
> 			  <4 0 0x1000 0x1000>, 			  /* Q_ANEG workaround: P4R0B12 = 1 */
> 			  <3 16 0 0x1117>;			  /* nastavení LED: G=link+act, Y=1Gbit */
> 		};

Comments are normally in English. The last one seems to be setting the
LED. This is tolerated, but not ideal. It is not clear to me what the
other two do.

> 	pip: pip@...00a0000000 {
> 		compatible = "cavium,octeon-3860-pip";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		reg = <0x11800 0xa0000000 0x0 0x2000>;
> 
> 		/* Interface 0 goes to SFP */
> 		interface@0 {
> 			compatible = "cavium,octeon-3860-pip-interface";
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			reg = <0>; /* interface */
> 
> 			ethernet@0 {
> 				compatible = "cavium,octeon-3860-pip-port";
> 				reg = <0>; /* Port */
> 				local-mac-address = [ 00 00 00 00 00 00 ];
> 				managed = "in-band-status";
> 				phy-connection-type = "1000base-x";
> 				sfp = <&sfp>;
> 			};
> 		};

> 		/* Interface 1 goes to eth1-eth4 and is QSGMII */
> 		interface@1 {
> 			compatible = "cavium,octeon-3860-pip-interface";
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			reg = <1>; /* interface */
> 
> 			ethernet@0 {
> 				compatible = "cavium,octeon-3860-pip-port";
> 				reg = <0>; /* Port */
> 				local-mac-address = [ 00 00 00 00 00 00 ];
> 				phy-handle = <&phy0>;

If this is a QSGMII link, don't you need phy-mode property?
 
> However testing revealed some glitches:
> 1. driver previously returned -EPROBE_DEFER when no phy was attached.
> Phylink stack does not seem to do so, which end up with:
> 
> Marvell PHY driver as a module:
> octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode
> octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Generic PHY] (irq=POLL)
> octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
> octeon_ethernet 11800a0000000.pip eth2: PHY [8001180000001800:01] driver [Generic PHY] (irq=POLL)
> octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode
> octeon_ethernet 11800a0000000.pip eth0: switched to inband/sgmii link mode
> octeon_ethernet 11800a0000000.pip eth0: PHY [i2c:sfp:16] driver [Marvell 88E1111] (irq=POLL)
> octeon_ethernet 11800a0000000.pip eth3: PHY [8001180000001800:02] driver [Marvell 88E1340S] (irq=25)
> octeon_ethernet 11800a0000000.pip eth3: configuring for phy/sgmii link mode
> octeon_ethernet 11800a0000000.pip eth4: PHY [8001180000001800:03] driver [Marvell 88E1340S] (irq=25)
> octeon_ethernet 11800a0000000.pip eth4: configuring for phy/sgmii link mode
> octeon_ethernet 11800a0000000.pip eth1: Link is Up - 100Mbps/Full - flow control off
> 
> Marvell PHY driver built-in:
> octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode
> octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Marvell 88E1340S] (irq=25)
> octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
> Error: Driver 'Marvell 88E1101' is already registered, aborting...
> libphy: Marvell 88E1101: Error -16 in registering driver
> Error: Driver 'Marvell 88E1101' is already registered, aborting...
> libphy: Marvell 88E1101: Error -16 in registering driver

This is very odd. But it could be a side effect of the
compatible. Please try with it removed.

> 2. It is not possible to call phylink_create from ndo_init callcack as
> it evetually calls sfp_bus_add_upstream which calls rtnl_lock().
> As this lock is already taken, it just deadlocks. Is this an unsupported
> scenario?

You normally call phylink_create() in _probe().

    Andrew