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

[Théo Lebrun]

Hello Vladimir,

On Tue Feb 10, 2026 at 8:35 PM CET, Vladimir Oltean wrote:
> 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?

Ack!

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

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

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

Some fields are non-resetting (BIT 30).
Will drop. I was trying to play it safe for no good reason.

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

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

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

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?

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?

Thanks for the extensive review Vladimir,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com