From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f42.google.com (mail-ed1-f42.google.com [209.85.208.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 47053284880 for ; Wed, 25 Feb 2026 14:03:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772028188; cv=none; b=ruZcRiRMTvoq7qG3hr347vebdYmOpfMPLoosLgKWaHQI67K4qDCcooM/DZBusQ99+aG7V7WQgCbHkWeLJPi0xfSwYLFi34HNmI2hwjwkoDzrUTzD+JFb+3mvmsK/gjhVHEgi91KBQ0qVjmGkzbnJlCZ0E1OZafd4i240jo5PHMs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772028188; c=relaxed/simple; bh=Wbxkai96KV++LHuFBYgeBqSwbkDv6oZ7clagX+nNyW0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=m5bymEcnMfeijvi/ojpx9vN/bpRujWiRMdfFqlHZpGKMOikUrK4uKt9c5dtPoWu8KQRcj66auDRDYiqkOKzivm8sieQqJObS5tox02bI0jE/FlOemgP5CVdARKNb6KpkU/8ekNU6UU306t8vgEtxFAOYaiVSXDiHIk1ncuLTNLk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=cehSiZtT; arc=none smtp.client-ip=209.85.208.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="cehSiZtT" Received: by mail-ed1-f42.google.com with SMTP id 4fb4d7f45d1cf-65b9db5e4c1so787903a12.2 for ; Wed, 25 Feb 2026 06:03:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1772028185; x=1772632985; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=/X7TGnbDY8JHbzvNZLr4KocFzXXLBLgEWZOADNbBVWE=; b=cehSiZtTELhq9w7365ouxBfC+5t+oBMOkuMaCDTUF/eO7Vz7ykZGSaEhJi2DwYBpam rbhxaMuHO9jtEpxp95sk/sRiFAtTAcYxsV56Ue1su0kedWNsF3KdyKkuEKF+BGAWkgTK uRDjFh6ujNKJQ5CTtv2gYVZ+TuOLCEK+lIM5KvWbNjFbWpJfsaAQWcl+m3FUPIo80m+r WdiRoOu9htDehEAGVKejtEDNh9uMCIoNi+UvlibTiY2EBk517VqQWZwtuJT+CIVaP1rh ASVH69t7gjF/y0loTo2WtL56hnFiMqKyk/gMwhzQySG1DUVG8Cj8E6xmA5UuTkdq5dtC 43pw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772028185; x=1772632985; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=/X7TGnbDY8JHbzvNZLr4KocFzXXLBLgEWZOADNbBVWE=; b=otND7rdH2NKVpsMQttOMHtU+UaghQWrLStXZXsIl6wDMBWRS89qHwBnGU9iRarwl03 R+aMFM58Vrlm/jl1q/yS5euQ2q05MjPfdSUzejIG6rNGB+XswxNhubxiu/ogNTDmT/Yj A8TMYS65CberEor80UYKf13J/8QL2OrlbFbS19Yb02M5wlsm6jFFf+UcEKFFho/mKMky OvWB0JHPs/RwX1Bj8Aq4MtU6TR4FOk+Dk7OEElrJZdCK1L2bFCK1R0533fBjkbS4t78I xk8uoUikCjkBrNvQD9bTYK0N2MLTsNAsIKzUR7sJojd90FDqqYsA9whUwtqteXzUMVY+ Rj+Q== X-Forwarded-Encrypted: i=1; AJvYcCXXMKZ87NaA2MTngEkBFCGRfiDdc0w5bHMqVRdJ5g2Ql/YgMpMzs7Xn0oEbFN7KwWSmBdOluiB6DZrz@vger.kernel.org X-Gm-Message-State: AOJu0YzjNnfv20U6/mTHhHVQKaaeNZZZZ6LBUOHAfcsTeU+i9zuKXgjk Dxe/mtO1Dq+Eh5elBk91Rbt8AH5TFmpFLoBwhtJNFTq5EJpr3Evip9q3xmuzo3Y3 X-Gm-Gg: ATEYQzyKNP0yPUOSWIhLjNqvt22ypEMERNoWX7Hv7ccsmRXVNamjrucS45zQNoG0kpG HpXM+s5FzrRs1JfdlZ0LQFshg/TkbldLbhsKLnwtHd7xby0mcsCGjLtNNJxvVbGkpwlLShrrcUy uSBhj/MCx8hFVIfRUDUzeavfuPBg0I8hK9zASfRF+sL0V1Xd7fQIxr8OkhpkLHjPoSa9Wqa1FF7 TaYqwBeHgY2DuG6K9OYp3K9NeHnIdZyVnRi+2L8Jv9Ibt97tBwie3KBTugv/9C/HJlwANmiKX6F +9Wu5eYIKw1khL1tCHohbmmAHQkaHoRIXf9lzVlOxhj1KOFDcHVO41vEa+UU6GnxTkL8PhcfBF4 YAhWI0o0heTe6KYo/LE5FV8S6yqE3DGvKMTfgPFeMTeXNzvTz+UmcoSpr0csKLIzRfYJwpoBMHR ofLpt7kGKx5ix+9A== X-Received: by 2002:a05:600c:6218:b0:483:702f:4639 with SMTP id 5b1f17b1804b1-483a95eabd7mr166023945e9.2.1772022593550; Wed, 25 Feb 2026 04:29:53 -0800 (PST) Received: from skbuf ([2a02:2f04:d608:3a00:6e08:ea2c:b2bc:dea]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-43970bf9feasm33655969f8f.6.2026.02.25.04.29.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Feb 2026 04:29:52 -0800 (PST) Date: Wed, 25 Feb 2026 14:29:49 +0200 From: Vladimir Oltean To: =?utf-8?B?VGjDqW8=?= Lebrun Cc: Vladimir Kondratiev , =?utf-8?Q?Gr=C3=A9gory?= Clement , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Vinod Koul , Kishon Vijay Abraham I , Michael Turquette , Stephen Boyd , Philipp Zabel , Thomas Bogendoerfer , Neil Armstrong , linux-mips@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org, linux-clk@vger.kernel.org, =?utf-8?Q?Beno=C3=AEt?= Monin , Tawfik Bayouk , Thomas Petazzoni , Luca Ceresoli Subject: Re: [PATCH v6 3/8] phy: Add driver for EyeQ5 Ethernet PHY wrapper Message-ID: <20260225122949.pt55t3eefr5nawmu@skbuf> References: <20260127-macb-phy-v6-0-cdd840588188@bootlin.com> <20260127-macb-phy-v6-3-cdd840588188@bootlin.com> <20260210193516.temrg46yozxma7xb@skbuf> Precedence: bulk X-Mailing-List: linux-mips@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Tue, Feb 24, 2026 at 06:20:21PM +0100, Théo Lebrun wrote: > > Could you please add a macro or comment hinting at the origin of the > > magic number 5 here? You could also place these 3 lines in a common > > helper, also called from eq5_phy_exit(), to avoid minor code > > duplication. > > ACK, something named `eq5_phy_reinit()`. > > I don't have precise explanation for the 5µs value; I only know it is > time to let the PHY settle before further register config writes. > Is this enough? > > udelay(5); /* settling time */ If there's a single occurrence and there's a comment, it's fine. > >> + EQ5_GP_SYS_SWRST_DIS | EQ5_GP_SYS_M_CLKE | > >> + FIELD_PREP(EQ5_GP_RGMII_DRV, 0x9); > > > > Quick sanity check on your proposal to use #phy-cells = <1>. This is not > > a request to change anything. > > > > What if you need to customize the RGMII drive strength (or some other > > setting, maybe SGMII polarity if that is available) per lane, for a > > particular board? How would you do that if each PHY does not have its > > own OF node? > > I have no knowledge of what that 0x9 stands for, I didn't see the point > exposing it to devicetree. We could plan for the future and add a cell > or create subnodes, but here I kept it simple stupid. Is it OK? If you don't know that you need to customize anything, it's fine the way it is. > >> + writel(reg, inst->gp); > >> + > >> + return 0; > >> +} > >> + > >> +static int eq5_phy_exit(struct phy *phy) > >> +{ > >> + struct eq5_phy_inst *inst = phy_get_drvdata(phy); > >> + struct eq5_phy_private *priv = inst->priv; > >> + struct device *dev = priv->dev; > >> + > >> + dev_dbg(dev, "phy_exit(inst=%td)\n", inst - priv->phys); > >> + > >> + writel(0, inst->gp); > >> + writel(0, inst->sgmii); > >> + udelay(5); > >> + > >> + return 0; > >> +} > >> + > >> +static int eq5_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode) > >> +{ > >> + struct eq5_phy_inst *inst = phy_get_drvdata(phy); > >> + struct eq5_phy_private *priv = inst->priv; > >> + struct device *dev = priv->dev; > >> + > >> + dev_dbg(dev, "phy_set_mode(inst=%td, mode=%d, submode=%d)\n", > >> + inst - priv->phys, mode, submode); > >> + > >> + if (mode != PHY_MODE_ETHERNET) > >> + return -EOPNOTSUPP; > >> + > >> + if (!phy_interface_mode_is_rgmii(submode) && > >> + submode != PHY_INTERFACE_MODE_SGMII) > >> + return -EOPNOTSUPP; > > > > Both PHYs are equal in capabilities, and support both RGMII and SGMII, > > correct? I see the driver is implemented as if they were, but it doesn't > > hurt to ask. > > Datasheet indicates 0 can do SGMII/RGMII and 1 can do only RGMII. > Did you imply that the driver code should reject SGMII on PHY 1 > if it ever gets asked for? I didn't imply anything, as I didn't know the facts. But now that I do, yes, I'm explicitly requesting you to reject the submodes that PHY 1 doesn't support. I also notice that you haven't implemented support for phy_validate(). Please do so, even if your PHY consumer does not call it (it should, to detect which modes and submodes are supported). > >> + > >> + inst->phy_interface = submode; > > > > Short story: don't rely on the phy_set_mode_ext() -> phy_power_on() order. > > Implement the driver so that it works the other way around too. > > > > Long story: > > https://lore.kernel.org/netdev/aXzFH09AeIRawCwU@shell.armlinux.org.uk/ > > I wouldn't mind, but what should phy_power_on() do if no submode has > been provided through phy_set_mode_ext() yet? Guess one? Fail? Assume a default initial submode, and power on using the rules of that submode. In your case you don't even have to assume, you can read EQ5_GP_SGMII_MODE to figure out what the submode is at probe time. > Also our PHY will need to be reset to change its mode if we do > power_on() followed by set_mode(), which in practice is never something > we want. Maybe there is a flag to indicate that we require a submode to > power on? Such flag doesn't exist, nor do I think it is desirable. It would unnecessarily complicate consumer drivers, which would have to support two code paths if they were to follow the "generic" PHY API model. Feel free to reset the PHY if requested to change the submode while it is powered on. For example, lynx_28g_set_mode() does that.