linux-kernel - Re: [PATCH v6 3/8] phy: Add driver for EyeQ5 Ethernet PHY wrapper

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: <20260210193516.temrg46yozxma7xb@skbuf>
Date: Tue, 10 Feb 2026 21:35:16 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
	Grégory Clement <gregory.clement@...tlin.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	linux-mips@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
	linux-clk@...r.kernel.org,
	Benoît Monin <benoit.monin@...tlin.com>,
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Luca Ceresoli <luca.ceresoli@...tlin.com>
Subject: Re: [PATCH v6 3/8] phy: Add driver for EyeQ5 Ethernet PHY wrapper

Hi Theo,

On Tue, Jan 27, 2026 at 06:09:31PM +0100, Théo Lebrun wrote:
> +static int eq5_phy_init(struct phy *phy)
> +{
> +	struct eq5_phy_inst *inst = phy_get_drvdata(phy);
> +	struct eq5_phy_private *priv = inst->priv;
> +	struct device *dev = priv->dev;
> +	u32 reg;
> +
> +	dev_dbg(dev, "phy_init(inst=%td)\n", inst - priv->phys);

Nitpick: can you please remove the debugging prints and maybe add some
trace points to the PHY core if you feel strongly about having some
introspection?

> +
> +	writel(0, inst->gp);
> +	writel(0, inst->sgmii);
> +
> +	udelay(5);

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.

> +
> +	reg = readl(inst->gp) | EQ5_GP_TX_SWRST_DIS | EQ5_GP_TX_M_CLKE |

When you write 0 to inst->gp and then read it back, do you expect to
(a) get back 0 or
(b) are some fields non-resetting?

I see both as inconsistent, since if (a), you can remove the
readl(inst->gp) and expect the same result. And if (b), it also
shouldn't matter if you write zeroes a second time, if it was fine the
first time?

Shortly said, is readl(inst->gp) really needed?

> +	      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?

> +	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.

> +
> +	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/

> +	return 0;
> +}