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

[Théo Lebrun]

Hello Théo,

On Tue Feb 24, 2026 at 6:20 PM CET, Théo Lebrun wrote:
> 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?

To give something concrete, here is what it could look like.
 - We refuse phy_power_on() if we haven't been given a mode (we probe
   with `inst->phy_interface == PHY_INTERFACE_MODE_NA`).
 - We finish phy_set_mode() by restarting if `phy->power_count`.

I am not convinced. I would prefer to refuse phy_set_mode() with EBUSY
if `phy->power_count == 0`.



static void eq5_phy_reinit(struct phy *phy)
{
	struct eq5_phy_inst *inst = phy_get_drvdata(phy);

	writel(0, inst->gp);
	writel(0, inst->sgmii);
	udelay(5); /* settling time */
}

static int eq5_phy_init(struct phy *phy)
{
	struct eq5_phy_inst *inst = phy_get_drvdata(phy);
	u32 reg;

	eq5_phy_reinit(phy);

	reg = EQ5_GP_TX_SWRST_DIS | EQ5_GP_TX_M_CLKE |
	      EQ5_GP_SYS_SWRST_DIS | EQ5_GP_SYS_M_CLKE |
	      FIELD_PREP(EQ5_GP_RGMII_DRV, 0x9);
	writel(reg, inst->gp);

	return 0;
}

static int eq5_phy_exit(struct phy *phy)
{
	eq5_phy_reinit(phy);
	return 0;
}

static int eq5_phy_power_on(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;

	if (inst->phy_interface == PHY_INTERFACE_MODE_NA)
		return -ENOMEDIUM;

	if (inst->phy_interface == PHY_INTERFACE_MODE_SGMII) {
		writel(readl(inst->gp) | EQ5_GP_SGMII_MODE, inst->gp);

		reg = EQ5_SGMII_PWR_EN | EQ5_SGMII_RST_DIS | EQ5_SGMII_PLL_EN;
		writel(reg, inst->sgmii);

		if (readl_poll_timeout(inst->sgmii, reg,
				       reg & EQ5_SGMII_PLL_ACK, 1, 100)) {
			dev_err(dev, "PLL timeout\n");
			return -ETIMEDOUT;
		}

		reg = readl(inst->sgmii);
		reg |= EQ5_SGMII_PWR_STATE | EQ5_SGMII_SIG_DET_SW;
		writel(reg, inst->sgmii);
	} else {
		writel(readl(inst->gp) & ~EQ5_GP_SGMII_MODE, inst->gp);
		writel(0, inst->sgmii);
	}

	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);

	if (mode != PHY_MODE_ETHERNET)
		return -EOPNOTSUPP;

	if (!phy_interface_mode_is_rgmii(submode) &&
	    submode != PHY_INTERFACE_MODE_SGMII)
		return -EOPNOTSUPP;

	if (submode != inst->phy_interface) {
		inst->phy_interface = submode;

		if (phy->power_count) {
			eq5_phy_reinit(phy);
			return eq5_phy_power_on(phy);
		}
	}

	return 0;
}

static int eq5_phy_power_off(struct phy *phy)
{
	struct eq5_phy_inst *inst = phy_get_drvdata(phy);

	writel(readl(inst->gp) & ~EQ5_GP_SGMII_MODE, inst->gp);
	writel(0, inst->sgmii);

	return 0;
}

Thanks,

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