From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from vps0.lunn.ch (vps0.lunn.ch [156.67.10.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 28E2B8C1E; Mon, 1 Jul 2024 17:00:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.67.10.101 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1719853243; cv=none; b=pZSsN7Q7qSngmB9BRSpDyDuYy212KtcfS42qCBIXkfgDjYmdf7ijg66ZiCyc2pKEQZf50/rOFDh4egdhxUuGXC4VmRLUVsbIOZEb8Cd2pkNlzxgfnr75sdOwcKOPJ19aV5iH8vMA0YplkdaSCmDJMWDdImAvm4oVs6jUBb6+tXE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1719853243; c=relaxed/simple; bh=MUiM9ao2RvE94F7ZTjLTyTt/XiwJiWbHlGOcXiJW+xw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=bjdfk/tAAB+hogOuFTLnHsSuy/lNMOQUPhzKyRIR5S/JiY5wda7vukzrOSB98lcX0EfAsp/B25kIwu5om1AIu0atz4C9M3aVlKJsES30T4pw9BA7WkuJqPVd2Hx4sJ8xrHKkNDdhApB24nn/alM3Z/hbi7ji61+I5cMWrfwL9QU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch; spf=pass smtp.mailfrom=lunn.ch; dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b=6RedJELs; arc=none smtp.client-ip=156.67.10.101 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lunn.ch Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="6RedJELs" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject: Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Content-Disposition:In-Reply-To:References; bh=2DldeZMtdvwKEVfE86XEEVj2HO43L/iuwsuOSidP0Fg=; b=6RedJELsuTvjekhk2pce8s+PiV XvdNCwWrC1BRHJUHX30Y01KUCEFx6YXjNLKzjXiI5SkJxX8LbzIj29qN9p74002anvPcjpNeZmOb5 Eei+JHZY8mNzpoM6bLmHmRxUdAYIPPE+jQ8fZ5NCXyo64mzTyaAe4KDCcjPtMx5GmkGA=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1sOKOD-001Z45-Jm; Mon, 01 Jul 2024 19:00:29 +0200 Date: Mon, 1 Jul 2024 19:00:29 +0200 From: Andrew Lunn To: Romain Gantois Cc: Heiner Kallweit , Russell King , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Thomas Petazzoni , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules Message-ID: References: <20240701-b4-dp83869-sfp-v1-0-a71d6d0ad5f8@bootlin.com> <20240701-b4-dp83869-sfp-v1-5-a71d6d0ad5f8@bootlin.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240701-b4-dp83869-sfp-v1-5-a71d6d0ad5f8@bootlin.com> > +static int dp83869_connect_phy(void *upstream, struct phy_device *phy) > +{ > + struct phy_device *phydev = upstream; > + struct dp83869_private *dp83869; > + > + dp83869 = phydev->priv; > + > + if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE) > + return 0; > + > + if (!phy->drv) { > + dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!\n"); more instances which could be phydev_{err|warn|info}(). > + return 0; > + } > + > + phy_support_asym_pause(phy); That is unusual. This is normally used by a MAC driver to indicate it supports asym pause. It is the MAC which implements pause, but the PHY negotiates it. So this tells phylib what to advertise to the link partner. Why would a PHY do this? What if the MAC does not support asym pause? > + linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces); > + phy->interface = PHY_INTERFACE_MODE_SGMII; > + phy->port = PORT_TP; > + > + phy->speed = SPEED_UNKNOWN; > + phy->duplex = DUPLEX_UNKNOWN; > + phy->pause = MLO_PAUSE_NONE; > + phy->interrupts = PHY_INTERRUPT_DISABLED; > + phy->irq = PHY_POLL; > + phy->phy_link_change = &dp83869_sfp_phy_change; > + phy->state = PHY_READY; I don't know of any other PHY which messes with the state machine like this. This needs some explanation. > + > + dp83869->mod_phy = phy; > + > + return 0; > +} > + > +static void dp83869_disconnect_phy(void *upstream) > +{ > + struct phy_device *phydev = upstream; > + struct dp83869_private *dp83869; > + > + dp83869 = phydev->priv; > + dp83869->mod_phy = NULL; > +} > + > +static int dp83869_module_start(void *upstream) > +{ > + struct phy_device *phydev = upstream; > + struct dp83869_private *dp83869; > + struct phy_device *mod_phy; > + int ret; > + > + dp83869 = phydev->priv; > + mod_phy = dp83869->mod_phy; > + if (!mod_phy) > + return 0; > + > + ret = phy_init_hw(mod_phy); > + if (ret) { > + dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: error %d", ret); > + return ret; > + } > + > + phy_start(mod_phy); Something else no other PHY driver does.... Andrew