Re: [PATCH v5 2/7] phy: Add driver for EyeQ5 Ethernet PHY wrapper

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Vinod,

On Tue Dec 23, 2025 at 4:53 PM CET, Vinod Koul wrote:
> On 15-12-25, 17:26, Théo Lebrun wrote:
>> EyeQ5 embeds a system-controller called OLB. It features many unrelated
>> registers, and some of those are registers used to configure the
>> integration of the RGMII/SGMII Cadence PHY used by MACB/GEM instances.
>> 
>> Wrap in a neat generic PHY provider, exposing two PHYs with standard
>> phy_init() / phy_set_mode() / phy_power_on() operations.
>> 
>> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  MAINTAINERS                 |   1 +
>>  drivers/phy/Kconfig         |  13 +++
>>  drivers/phy/Makefile        |   1 +
>>  drivers/phy/phy-eyeq5-eth.c | 249 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 264 insertions(+)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5b11839cba9d..2f67ec9fad57 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -17605,6 +17605,7 @@ F:	arch/mips/boot/dts/mobileye/
>>  F:	arch/mips/configs/eyeq5_defconfig
>>  F:	arch/mips/mobileye/board-epm5.its.S
>>  F:	drivers/clk/clk-eyeq.c
>> +F:	drivers/phy/phy-eyeq5-eth.c
>>  F:	drivers/pinctrl/pinctrl-eyeq5.c
>>  F:	drivers/reset/reset-eyeq.c
>>  F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 678dd0452f0a..1aa6eff12dbc 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -101,6 +101,19 @@ config PHY_NXP_PTN3222
>>  	  schemes. It supports all three USB 2.0 data rates: Low Speed, Full
>>  	  Speed and High Speed.
>>  
>> +config PHY_EYEQ5_ETH
>
> sorted please

I wouldn't mind, but entries are currently (v6.19-rc4) not sorted:

$ diff -U100 <(grep ^config drivers/phy/Kconfig) \
             <(grep ^config drivers/phy/Kconfig | sort)
--- /dev/fd/63 2026-01-05 15:55:53.891922890 +0100
+++ /dev/fd/62 2026-01-05 15:55:53.891922890 +0100
@@ -1,11 +1,11 @@
 config GENERIC_PHY
 config GENERIC_PHY_MIPI_DPHY
+config PHY_AIROHA_PCIE
+config PHY_CAN_TRANSCEIVER
+config PHY_EYEQ5_ETH
 config PHY_LPC18XX_USB_OTG
+config PHY_NXP_PTN3222
 config PHY_PISTACHIO_USB
 config PHY_SNPS_EUSB2
 config PHY_XGENE
 config USB_LGM_PHY
-config PHY_CAN_TRANSCEIVER
-config PHY_AIROHA_PCIE
-config PHY_NXP_PTN3222
-config PHY_EYEQ5_ETH

This is why I appended. In V2, I'll send a first patch to reorder
entries to then be able to add PHY_EYEQ5_ETH in the correct location.

>> +	tristate "Ethernet PHY Driver on EyeQ5"
>> +	depends on OF
>> +	depends on MACH_EYEQ5 || COMPILE_TEST
>> +	select AUXILIARY_BUS
>> +	select GENERIC_PHY
>> +	default MACH_EYEQ5
>
> hmmm why should it be default? Maybe add this is respective defconfig for
> platform instead..?

We have been doing this for other parts of OLB. If you are doing a
config for EyeQ5, most probably you want this enabled.

One example usecase this default field makes config easier: when you
migrate an eyeq* config to eyeq5 without resetting the full config by
applying eyeq5_defconfig. With default field you get this driver
enabled, otherwise you don't and Ethernet doesn't work.

I'd prefer keeping this default but we can drop it if you lean strongly
against it.

>> +	help
>> +	  Enable this to support the Ethernet PHY integrated on EyeQ5.
>> +	  It supports both RGMII and SGMII. Registers are located in a
>> +	  shared register region called OLB. If M is selected, the
>> +	  module will be called phy-eyeq5-eth.
>> +
>>  source "drivers/phy/allwinner/Kconfig"
>>  source "drivers/phy/amlogic/Kconfig"
>>  source "drivers/phy/broadcom/Kconfig"
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index bfb27fb5a494..8289497ece55 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -13,6 +13,7 @@ obj-$(CONFIG_PHY_SNPS_EUSB2)		+= phy-snps-eusb2.o
>>  obj-$(CONFIG_USB_LGM_PHY)		+= phy-lgm-usb.o
>>  obj-$(CONFIG_PHY_AIROHA_PCIE)		+= phy-airoha-pcie.o
>>  obj-$(CONFIG_PHY_NXP_PTN3222)		+= phy-nxp-ptn3222.o
>> +obj-$(CONFIG_PHY_EYEQ5_ETH)		+= phy-eyeq5-eth.o
>
> sorted please

Same as for Kconfig. Will do it in two steps: first sort, then add the
CONFIG_PHY_EYEQ5_ETH line.

$ diff -U100 <(grep ^obj-\\$ drivers/phy/Makefile) \
             <(grep ^obj-\\$ drivers/phy/Makefile | sort)
--- /dev/fd/63 2026-01-05 16:11:10.977537425 +0100
+++ /dev/fd/62 2026-01-05 16:11:10.978537396 +0100
@@ -1,11 +1,11 @@
-obj-$(CONFIG_GENERIC_PHY)              += phy-core.o
 obj-$(CONFIG_GENERIC_PHY_MIPI_DPHY)    += phy-core-mipi-dphy.o
+obj-$(CONFIG_GENERIC_PHY)              += phy-core.o
+obj-$(CONFIG_PHY_AIROHA_PCIE)          += phy-airoha-pcie.o
 obj-$(CONFIG_PHY_CAN_TRANSCEIVER)      += phy-can-transceiver.o
+obj-$(CONFIG_PHY_EYEQ5_ETH)            += phy-eyeq5-eth.o
 obj-$(CONFIG_PHY_LPC18XX_USB_OTG)      += phy-lpc18xx-usb-otg.o
-obj-$(CONFIG_PHY_XGENE)                += phy-xgene.o
+obj-$(CONFIG_PHY_NXP_PTN3222)          += phy-nxp-ptn3222.o
 obj-$(CONFIG_PHY_PISTACHIO_USB)        += phy-pistachio-usb.o
 obj-$(CONFIG_PHY_SNPS_EUSB2)           += phy-snps-eusb2.o
+obj-$(CONFIG_PHY_XGENE)                += phy-xgene.o
 obj-$(CONFIG_USB_LGM_PHY)              += phy-lgm-usb.o
-obj-$(CONFIG_PHY_AIROHA_PCIE)          += phy-airoha-pcie.o
-obj-$(CONFIG_PHY_NXP_PTN3222)          += phy-nxp-ptn3222.o
-obj-$(CONFIG_PHY_EYEQ5_ETH)            += phy-eyeq5-eth.o

[...]

>> +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);
>
> this is same patter in init as well...?

Yes! phy_ops::init() must reinit the HW to ensure its config fields
are well taken into account. We might inherit an already initialised
PHY from the bootloader.

We could in theory move those three ops (writel+writel+udelay) into a
helper function but I feel like it would decrease readability without
increasing code quality.

>> +
>> +	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);
>
> these are good for debug but not for upstream, please drop

Ah this is surprising! They helped me debug this driver and fix one bug
or two so I thought I'd leave them in. They get compiled out by
default. And there is no ftrace events equivalent which would make
those dev_dbg() moot.

⟩ git grep -F dev_dbg\( drivers/ | wc -l
25174
⟩ git grep -F dev_dbg\( drivers/phy/ | wc -l
260

[...]

>> +static int eq5_phy_probe(struct auxiliary_device *adev,
>> +			 const struct auxiliary_device_id *id)
>> +{
>> +	struct device *dev = &adev->dev;
>> +	struct phy_provider *provider;
>> +	struct eq5_phy_private *priv;
>> +	void __iomem *base;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = dev;
>> +	dev_set_drvdata(dev, priv);
>> +
>> +	base = (void __iomem *)dev_get_platdata(dev);
>
> no need to cast for void *

Yes! My initial goal was to prevent a sparse warning about the implicit
cast from `void *` as returned by dev_get_platdata() and
`void __iomem *base`.

But it does not matter in our case (and the correct solution for
explicit cast would have implied __force).

--

I'll wait for feedback on `default MACH_EYEQ5` and `dev_dbg()` before
sending the next revision.

Thanks for the review!

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