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

[Vladimir Oltean]

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

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.