[net-next,RFC,v2,2/3] net: phy: Add support for Aeonsemi AS21xxx PHYs

Commit Message

Christian Marangi March 26, 2025, 12:23 a.m. UTC

Add support for Aeonsemi AS21xxx 10G C45 PHYs. These PHYs intergate
an IPC to setup some configuration and require special handling to
sync with the parity bit. The parity bit is a way the IPC use to
follow correct order of command sent.

Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
AS21210PB1 that all register with the PHY ID 0x7500 0x7510
before the firmware is loaded.

They all support up to 5 LEDs with various HW mode supported.

While implementing it was found some strange coincidence with using the
same logic for implementing C22 in MMD regs in Broadcom PHYs.

For reference here the AS21xxx PHY name logic:

AS21x1xxB1
    ^ ^^
    | |J: Supports SyncE/PTP
    | |P: No SyncE/PTP support
    | 1: Supports 2nd Serdes
    | 2: Not 2nd Serdes support
    0: 10G, 5G, 2.5G
    5: 5G, 2.5G
    2: 2.5G

Signed-off-by: Christian Marangi <[e-mail]>
---
 MAINTAINERS               |   6 +
 drivers/net/phy/Kconfig   |  12 +
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/as21xxx.c | 973 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 992 insertions(+)
 create mode 100644 drivers/net/phy/as21xxx.c

Comments

Russell King (Oracle) March 26, 2025, 1:53 p.m. UTC | #1

On Wed, Mar 26, 2025 at 01:23:58AM +0100, Christian Marangi wrote:
> +static int aeon_ipc_send_cmd(struct phy_device *phydev, u32 cmd,
> +			     u16 *ret_sts)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	bool curr_parity;
> +	int ret;
> +
> +	/* The IPC sync by using a single parity bit.
> +	 * Each CMD have alternately this bit set or clear
> +	 * to understand correct flow and packet order.
> +	 */
> +	curr_parity = priv->parity_status;
> +	if (priv->parity_status)
> +		cmd |= AEON_IPC_CMD_PARITY;
> +
> +	/* Always update parity for next packet */
> +	priv->parity_status = !priv->parity_status;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_CMD, cmd);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for packet to be processed */
> +	usleep_range(AEON_IPC_DELAY, AEON_IPC_DELAY + 5000);
> +
> +	/* With no ret_sts, ignore waiting for packet completion
> +	 * (ipc parity bit sync)
> +	 */
> +	if (!ret_sts)
> +		return 0;
> +
> +	ret = aeon_ipcs_wait_cmd(phydev, curr_parity);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*ret_sts = ret;
> +	if ((*ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_SUCCESS)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int aeon_ipc_send_msg(struct phy_device *phydev, u16 opcode,
> +			     u16 *data, unsigned int data_len, u16 *ret_sts)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	u32 cmd;
> +	int ret;
> +	int i;
> +
> +	/* IPC have a max of 8 register to transfer data,
> +	 * make sure we never exceed this.
> +	 */
> +	if (data_len > AEON_IPC_DATA_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->ipc_lock);
> +
> +	for (i = 0; i < data_len / sizeof(u16); i++)
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i),
> +			      data[i]);
> +
> +	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, data_len) |
> +	      FIELD_PREP(AEON_IPC_CMD_OPCODE, opcode);
> +	ret = aeon_ipc_send_cmd(phydev, cmd, ret_sts);
> +	if (ret)
> +		phydev_err(phydev, "failed to send ipc msg for %x: %d\n", opcode, ret);
> +
> +	mutex_unlock(&priv->ipc_lock);
> +
> +	return ret;
> +}
> +
> +static int aeon_ipc_rcv_msg(struct phy_device *phydev, u16 ret_sts,
> +			    u16 *data)
> +{
> +	unsigned int size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
> +	struct as21xxx_priv *priv = phydev->priv;
> +	int ret;
> +	int i;
> +
> +	if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
> +		return -EINVAL;
> +
> +	/* Prevent IPC from stack smashing the kernel */
> +	if (size > AEON_IPC_DATA_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->ipc_lock);
> +
> +	for (i = 0; i < DIV_ROUND_UP(size, sizeof(u16)); i++) {
> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i));
> +		if (ret < 0) {
> +			size = ret;
> +			goto out;
> +		}
> +
> +		data[i] = ret;
> +	}
> +
> +out:
> +	mutex_unlock(&priv->ipc_lock);
> +
> +	return size;
> +}
> +
> +/* Logic to sync parity bit with IPC.
> + * We send 2 NOP cmd with same partity and we wait for IPC
> + * to handle the packet only for the second one. This way
> + * we make sure we are sync for every next cmd.
> + */
> +static int aeon_ipc_sync_parity(struct phy_device *phydev)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	u16 ret_sts;
> +	u32 cmd;
> +	int ret;
> +
> +	mutex_lock(&priv->ipc_lock);
> +
> +	/* Send NOP with no parity */
> +	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, 0) |
> +	      FIELD_PREP(AEON_IPC_CMD_OPCODE, IPC_CMD_NOOP);
> +	aeon_ipc_send_cmd(phydev, cmd, NULL);
> +
> +	/* Reset packet parity */
> +	priv->parity_status = false;
> +
> +	/* Send second NOP with no parity */
> +	ret = aeon_ipc_send_cmd(phydev, cmd, &ret_sts);
> +
> +	mutex_unlock(&priv->ipc_lock);
> +
> +	/* We expect to return -EFAULT */
> +	if (ret != -EFAULT)
> +		return ret;
> +
> +	if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int aeon_ipc_get_fw_version(struct phy_device *phydev)
> +{
> +	u16 ret_data[8], data[1];
> +	u16 ret_sts;
> +	int ret;
> +
> +	data[0] = IPC_INFO_VERSION;
> +	ret = aeon_ipc_send_msg(phydev, IPC_CMD_INFO, data, sizeof(data),
> +				&ret_sts);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_rcv_msg(phydev, ret_sts, ret_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	phydev_info(phydev, "Firmware Version: %s\n", (char *)ret_data);
> +
> +	return 0;
> +}
> +
> +static int aeon_dpc_ra_enable(struct phy_device *phydev)
> +{
> +	u16 data[2];
> +	u16 ret_sts;
> +
> +	data[0] = IPC_CFG_PARAM_DIRECT;
> +	data[1] = IPC_CFG_PARAM_DIRECT_DPC_RA;
> +
> +	return aeon_ipc_send_msg(phydev, IPC_CMD_CFG_PARAM, data,
> +				 sizeof(data), &ret_sts);
> +}
> +
> +static int as21xxx_probe(struct phy_device *phydev)
> +{
> +	struct as21xxx_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev,
> +			    sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	phydev->priv = priv;
> +
> +	ret = devm_mutex_init(&phydev->mdio.dev,
> +			      &priv->ipc_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_firmware_load(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_sync_parity(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> +			       VEND1_PTP_CLK_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_dpc_ra_enable(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_get_fw_version(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->needs_reregister = true;
> +
> +	return 0;
> +}

This probe function allocates devres resources that wil lbe freed when
it returns through the unbinding in patch 1. This is a recipe for
confusion - struct as21xxx_priv must never be used from any of the
"real" driver.

I would suggest:

1. document that devres resources will not be preserved when
   phydev->needs_reregister is set true.

2. rename struct as21xxx_priv to struct as21xxx_fw_load_priv to make
   it clear that it's for firmware loading.

3. use a prefix that uniquely identifies those functions that can only
   be called with this structure populated.

4. set phydev->priv to NULL at the end of this probe function to ensure
   no one dereferences the free'd pointer in a "real" driver, which
   could lead to use-after-free errors.

In summary, I really don't like this approach - it feels too much of a
hack, _and_ introduces the potential for drivers that makes use of this
to get stuff really very wrong. In my opinion that's not a model that
we should add to the kernel.

I'll say again - why can't the PHY firmware be loaded by board firmware.
You've been silent on my feedback on this point. Given that you're
ignoring me... for this patch series...

Hard NAK.

until you start responding to my review comments.

Andrew Lunn March 26, 2025, 1:57 p.m. UTC | #2

> In summary, I really don't like this approach - it feels too much of a
> hack, _and_ introduces the potential for drivers that makes use of this
> to get stuff really very wrong. In my opinion that's not a model that
> we should add to the kernel.

I agree.

> 
> I'll say again - why can't the PHY firmware be loaded by board firmware.
> You've been silent on my feedback on this point. Given that you're
> ignoring me... for this patch series...

And i still think using the match op is something that should be
investigated, alongside the bootloader loading firmware.

	Andrew

Simon Horman March 26, 2025, 2 p.m. UTC | #3

On Wed, Mar 26, 2025 at 01:23:58AM +0100, Christian Marangi wrote:
> Add support for Aeonsemi AS21xxx 10G C45 PHYs. These PHYs intergate

nit: integrate

> an IPC to setup some configuration and require special handling to
> sync with the parity bit. The parity bit is a way the IPC use to
> follow correct order of command sent.
> 
> Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
> AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
> AS21210PB1 that all register with the PHY ID 0x7500 0x7510
> before the firmware is loaded.
> 
> They all support up to 5 LEDs with various HW mode supported.
> 
> While implementing it was found some strange coincidence with using the
> same logic for implementing C22 in MMD regs in Broadcom PHYs.
> 
> For reference here the AS21xxx PHY name logic:
> 
> AS21x1xxB1
>     ^ ^^
>     | |J: Supports SyncE/PTP
>     | |P: No SyncE/PTP support
>     | 1: Supports 2nd Serdes
>     | 2: Not 2nd Serdes support
>     0: 10G, 5G, 2.5G
>     5: 5G, 2.5G
>     2: 2.5G
> 
> Signed-off-by: Christian Marangi <[e-mail]>

...

> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c

...

Please consider limiting line lengths to 80 columns which is still preferred
for Networking code. There is an option to checkpatch for this.

> +
> +#define VEND1_FW_START_ADDR		0x100
> +
> +#define VEND1_GLB_REG_MDIO_INDIRECT_ADDRCMD 0x101
> +#define VEND1_GLB_REG_MDIO_INDIRECT_LOAD 0x102
> +
> +#define VEND1_GLB_REG_MDIO_INDIRECT_STATUS 0x103
> +
> +#define VEND1_PTP_CLK			0x142
> +#define   VEND1_PTP_CLK_EN		BIT(6)
> +
> +/* 5 LED at step of 0x20
> + * FE: Fast-Ethernet (10/100)
> + * GE: Gigabit-Ethernet (1000)
> + * NG: New-Generation (2500/5000/10000)
> + */
> +#define VEND1_LED_REG(_n)		(0x1800 + ((_n) * 0x10))
> +#define   VEND1_LED_REG_A_EVENT		GENMASK(15, 11)
> +#define VEND1_LED_CONF			0x1881
> +#define   VEND1_LED_CONFG_BLINK		GENMASK(7, 0)
> +
> +#define VEND1_SPEED_STATUS		0x4002
> +#define   VEND1_SPEED_MASK		GENMASK(7, 0)
> +#define   VEND1_SPEED_10000		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x3)
> +#define   VEND1_SPEED_5000		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x5)
> +#define   VEND1_SPEED_2500		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x9)
> +#define   VEND1_SPEED_1000		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x10)
> +#define   VEND1_SPEED_100		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x20)
> +#define   VEND1_SPEED_10		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x0)
> +
> +#define VEND1_IPC_CMD			0x5801
> +#define   AEON_IPC_CMD_PARITY		BIT(15)
> +#define   AEON_IPC_CMD_SIZE		GENMASK(10, 6)
> +#define   AEON_IPC_CMD_OPCODE		GENMASK(5, 0)
> +
> +#define IPC_CMD_NOOP			0x0  /* Do nothing */
> +#define IPC_CMD_INFO			0x1  /* Get Firmware Version */
> +#define IPC_CMD_SYS_CPU			0x2  /* SYS_CPU */
> +#define IPC_CMD_BULK_DATA		0xa  /* Pass bulk data in ipc registers. */
> +#define IPC_CMD_BULK_WRITE		0xc  /* Write bulk data to memory */
> +#define IPC_CMD_CFG_PARAM		0x1a /* Write config parameters to memory */
> +#define IPC_CMD_NG_TESTMODE		0x1b /* Set NG test mode and tone */
> +#define IPC_CMD_TEMP_MON		0x15 /* Temperature monitoring function */
> +#define IPC_CMD_SET_LED			0x23 /* Set led */
> +
> +#define VEND1_IPC_STS			0x5802
> +#define   AEON_IPC_STS_PARITY		BIT(15)
> +#define   AEON_IPC_STS_SIZE		GENMASK(14, 10)
> +#define   AEON_IPC_STS_OPCODE		GENMASK(9, 4)
> +#define   AEON_IPC_STS_STATUS		GENMASK(3, 0)
> +#define   AEON_IPC_STS_STATUS_RCVD	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x1)
> +#define   AEON_IPC_STS_STATUS_PROCESS	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x2)
> +#define   AEON_IPC_STS_STATUS_SUCCESS	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x4)
> +#define   AEON_IPC_STS_STATUS_ERROR	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x8)
> +#define   AEON_IPC_STS_STATUS_BUSY	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0xe)
> +#define   AEON_IPC_STS_STATUS_READY	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0xf)
> +
> +#define VEND1_IPC_DATA0			0x5808
> +#define VEND1_IPC_DATA1			0x5809
> +#define VEND1_IPC_DATA2			0x580a
> +#define VEND1_IPC_DATA3			0x580b
> +#define VEND1_IPC_DATA4			0x580c
> +#define VEND1_IPC_DATA5			0x580d
> +#define VEND1_IPC_DATA6			0x580e
> +#define VEND1_IPC_DATA7			0x580f
> +#define VEND1_IPC_DATA(_n)		(VEND1_IPC_DATA0 + (_n))
> +
> +/* Sub command of CMD_INFO */
> +#define IPC_INFO_VERSION		0x1
> +
> +/* Sub command of CMD_SYS_CPU */
> +#define IPC_SYS_CPU_REBOOT		0x3
> +#define IPC_SYS_CPU_IMAGE_OFST		0x4
> +#define IPC_SYS_CPU_IMAGE_CHECK		0x5
> +#define IPC_SYS_CPU_PHY_ENABLE		0x6
> +
> +/* Sub command of CMD_CFG_PARAM */
> +#define IPC_CFG_PARAM_DIRECT		0x4
> +
> +/* CFG DIRECT sub command */
> +#define IPC_CFG_PARAM_DIRECT_NG_PHYCTRL	0x1
> +#define IPC_CFG_PARAM_DIRECT_CU_AN	0x2
> +#define IPC_CFG_PARAM_DIRECT_SDS_PCS	0x3
> +#define IPC_CFG_PARAM_DIRECT_AUTO_EEE	0x4
> +#define IPC_CFG_PARAM_DIRECT_SDS_PMA	0x5
> +#define IPC_CFG_PARAM_DIRECT_DPC_RA	0x6
> +#define IPC_CFG_PARAM_DIRECT_DPC_PKT_CHK 0x7
> +#define IPC_CFG_PARAM_DIRECT_DPC_SDS_WAIT_ETH 0x8
> +#define IPC_CFG_PARAM_DIRECT_WDT	0x9
> +#define IPC_CFG_PARAM_DIRECT_SDS_RESTART_AN 0x10
> +#define IPC_CFG_PARAM_DIRECT_TEMP_MON	0x11
> +#define IPC_CFG_PARAM_DIRECT_WOL	0x12
> +
> +/* Sub command of CMD_TEMP_MON */
> +#define IPC_CMD_TEMP_MON_GET		0x4
> +
> +#define AS21XXX_MDIO_AN_C22		0xffe0
> +
> +#define PHY_ID_AS21XXX			0x75009410
> +/* AS21xxx ID Legend
> + * AS21x1xxB1
> + *     ^ ^^
> + *     | |J: Supports SyncE/PTP
> + *     | |P: No SyncE/PTP support
> + *     | 1: Supports 2nd Serdes
> + *     | 2: Not 2nd Serdes support
> + *     0: 10G, 5G, 2.5G
> + *     5: 5G, 2.5G
> + *     2: 2.5G
> + */
> +#define PHY_ID_AS21011JB1		0x75009402
> +#define PHY_ID_AS21011PB1		0x75009412
> +#define PHY_ID_AS21010JB1		0x75009422
> +#define PHY_ID_AS21010PB1		0x75009432
> +#define PHY_ID_AS21511JB1		0x75009442
> +#define PHY_ID_AS21511PB1		0x75009452
> +#define PHY_ID_AS21510JB1		0x75009462
> +#define PHY_ID_AS21510PB1		0x75009472
> +#define PHY_ID_AS21210JB1		0x75009482
> +#define PHY_ID_AS21210PB1		0x75009492
> +#define PHY_VENDOR_AEONSEMI		0x75009400
> +
> +#define AEON_MAX_LDES			5
> +#define AEON_IPC_DELAY			10000
> +#define AEON_IPC_TIMEOUT		(AEON_IPC_DELAY * 100)
> +#define AEON_IPC_DATA_MAX		(8 * sizeof(u16))
> +
> +#define AEON_BOOT_ADDR			0x1000
> +#define AEON_CPU_BOOT_ADDR		0x2000
> +#define AEON_CPU_CTRL_FW_LOAD		(BIT(4) | BIT(2) | BIT(1) | BIT(0))
> +#define AEON_CPU_CTRL_FW_START		BIT(0)
> +
> +enum as21xxx_led_event {
> +	VEND1_LED_REG_A_EVENT_ON_10 = 0x0,
> +	VEND1_LED_REG_A_EVENT_ON_100,
> +	VEND1_LED_REG_A_EVENT_ON_1000,
> +	VEND1_LED_REG_A_EVENT_ON_2500,
> +	VEND1_LED_REG_A_EVENT_ON_5000,
> +	VEND1_LED_REG_A_EVENT_ON_10000,
> +	VEND1_LED_REG_A_EVENT_ON_FE_GE,
> +	VEND1_LED_REG_A_EVENT_ON_NG,
> +	VEND1_LED_REG_A_EVENT_ON_FULL_DUPLEX,
> +	VEND1_LED_REG_A_EVENT_ON_COLLISION,
> +	VEND1_LED_REG_A_EVENT_BLINK_TX,
> +	VEND1_LED_REG_A_EVENT_BLINK_RX,
> +	VEND1_LED_REG_A_EVENT_BLINK_ACT,
> +	VEND1_LED_REG_A_EVENT_ON_LINK,
> +	VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_ACT,
> +	VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_RX,
> +	VEND1_LED_REG_A_EVENT_ON_FE_GE_BLINK_ACT,
> +	VEND1_LED_REG_A_EVENT_ON_NG_BLINK_ACT,
> +	VEND1_LED_REG_A_EVENT_ON_NG_BLINK_FE_GE,
> +	VEND1_LED_REG_A_EVENT_ON_FD_BLINK_COLLISION,
> +	VEND1_LED_REG_A_EVENT_ON,
> +	VEND1_LED_REG_A_EVENT_OFF,
> +};
> +
> +struct as21xxx_led_pattern_info {
> +	unsigned int pattern;
> +	u16 val;
> +};
> +
> +struct as21xxx_priv {
> +	bool parity_status;
> +	/* Protect concurrent IPC access */
> +	struct mutex ipc_lock;
> +};
> +
> +static struct as21xxx_led_pattern_info as21xxx_led_supported_pattern[] = {
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10),
> +		.val = VEND1_LED_REG_A_EVENT_ON_10
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_100),
> +		.val = VEND1_LED_REG_A_EVENT_ON_100
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_1000),
> +		.val = VEND1_LED_REG_A_EVENT_ON_1000
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_2500),
> +		.val = VEND1_LED_REG_A_EVENT_ON_2500
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_5000),
> +		.val = VEND1_LED_REG_A_EVENT_ON_5000
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10000),
> +		.val = VEND1_LED_REG_A_EVENT_ON_10000
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK),
> +		.val = VEND1_LED_REG_A_EVENT_ON_LINK
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
> +			   BIT(TRIGGER_NETDEV_LINK_100) |
> +			   BIT(TRIGGER_NETDEV_LINK_1000),
> +		.val = VEND1_LED_REG_A_EVENT_ON_FE_GE
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_2500) |
> +			   BIT(TRIGGER_NETDEV_LINK_5000) |
> +			   BIT(TRIGGER_NETDEV_LINK_10000),
> +		.val = VEND1_LED_REG_A_EVENT_ON_NG
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_FULL_DUPLEX),
> +		.val = VEND1_LED_REG_A_EVENT_ON_FULL_DUPLEX
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_TX),
> +		.val = VEND1_LED_REG_A_EVENT_BLINK_TX
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_RX),
> +		.val = VEND1_LED_REG_A_EVENT_BLINK_RX
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_TX) |
> +			   BIT(TRIGGER_NETDEV_RX),
> +		.val = VEND1_LED_REG_A_EVENT_BLINK_ACT
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
> +			   BIT(TRIGGER_NETDEV_LINK_100) |
> +			   BIT(TRIGGER_NETDEV_LINK_1000) |
> +			   BIT(TRIGGER_NETDEV_LINK_2500) |
> +			   BIT(TRIGGER_NETDEV_LINK_5000) |
> +			   BIT(TRIGGER_NETDEV_LINK_10000),
> +		.val = VEND1_LED_REG_A_EVENT_ON_LINK
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
> +			   BIT(TRIGGER_NETDEV_LINK_100) |
> +			   BIT(TRIGGER_NETDEV_LINK_1000) |
> +			   BIT(TRIGGER_NETDEV_LINK_2500) |
> +			   BIT(TRIGGER_NETDEV_LINK_5000) |
> +			   BIT(TRIGGER_NETDEV_LINK_10000) |
> +			   BIT(TRIGGER_NETDEV_TX) |
> +			   BIT(TRIGGER_NETDEV_RX),
> +		.val = VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_ACT
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
> +			   BIT(TRIGGER_NETDEV_LINK_100) |
> +			   BIT(TRIGGER_NETDEV_LINK_1000) |
> +			   BIT(TRIGGER_NETDEV_LINK_2500) |
> +			   BIT(TRIGGER_NETDEV_LINK_5000) |
> +			   BIT(TRIGGER_NETDEV_LINK_10000) |
> +			   BIT(TRIGGER_NETDEV_RX),
> +		.val = VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_RX
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
> +			   BIT(TRIGGER_NETDEV_LINK_100) |
> +			   BIT(TRIGGER_NETDEV_LINK_1000) |
> +			   BIT(TRIGGER_NETDEV_TX) |
> +			   BIT(TRIGGER_NETDEV_RX),
> +		.val = VEND1_LED_REG_A_EVENT_ON_FE_GE_BLINK_ACT
> +	},
> +	{
> +		.pattern = BIT(TRIGGER_NETDEV_LINK_2500) |
> +			   BIT(TRIGGER_NETDEV_LINK_5000) |
> +			   BIT(TRIGGER_NETDEV_LINK_10000) |
> +			   BIT(TRIGGER_NETDEV_TX) |
> +			   BIT(TRIGGER_NETDEV_RX),
> +		.val = VEND1_LED_REG_A_EVENT_ON_NG_BLINK_ACT
> +	}
> +};
> +
> +static int aeon_firmware_boot(struct phy_device *phydev, const u8 *data, size_t size)
> +{
> +	int i, ret;
> +	u16 val;
> +
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_CTRL,
> +			     VEND1_GLB_CPU_CTRL_MASK, AEON_CPU_CTRL_FW_LOAD);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_FW_START_ADDR,
> +			    AEON_BOOT_ADDR);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_MDIO_INDIRECT_ADDRCMD,
> +			     0x3ffc, 0xc000);
> +	if (ret)
> +		return ret;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_MDIO_INDIRECT_STATUS);
> +	if (val > 1) {
> +		phydev_err(phydev, "wrong origin mdio_indirect_status: %x\n", val);
> +		return -EINVAL;
> +	}
> +
> +	/* Firmware is always aligned to u16 */
> +	for (i = 0; i < size; i += 2) {
> +		val = data[i + 1] << 8 | data[i];
> +
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_MDIO_INDIRECT_LOAD, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_RESET_ADDR_LO_BASEADDR,
> +			    lower_16_bits(AEON_CPU_BOOT_ADDR));
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_RESET_ADDR_HI_BASEADDR,
> +			    upper_16_bits(AEON_CPU_BOOT_ADDR));
> +	if (ret)
> +		return ret;
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_CTRL,
> +			      VEND1_GLB_CPU_CTRL_MASK, AEON_CPU_CTRL_FW_START);
> +}
> +
> +static int aeon_firmware_load(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	const struct firmware *fw;
> +	const char *fw_name;
> +	int ret;
> +
> +	ret = of_property_read_string(dev->of_node, "firmware-name",
> +				      &fw_name);
> +	if (ret)
> +		return ret;
> +
> +	ret = request_firmware(&fw, fw_name, dev);
> +	if (ret) {
> +		phydev_err(phydev, "failed to find FW file %s (%d)\n",
> +			   fw_name, ret);
> +		return ret;
> +	}
> +
> +	ret = aeon_firmware_boot(phydev, fw->data, fw->size);
> +
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +static inline int aeon_ipcs_wait_cmd(struct phy_device *phydev, bool parity_status)
> +{
> +	u16 val;
> +
> +	/* Exit condition logic:
> +	 * - Wait for parity bit equal
> +	 * - Wait for status success, error OR ready
> +	 */
> +	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
> +					 FIELD_GET(AEON_IPC_STS_PARITY, val) == parity_status &&
> +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_RCVD &&
> +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_PROCESS &&
> +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_BUSY,
> +					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);
> +}
> +
> +static int aeon_ipc_send_cmd(struct phy_device *phydev, u32 cmd,
> +			     u16 *ret_sts)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	bool curr_parity;
> +	int ret;
> +
> +	/* The IPC sync by using a single parity bit.
> +	 * Each CMD have alternately this bit set or clear
> +	 * to understand correct flow and packet order.
> +	 */
> +	curr_parity = priv->parity_status;
> +	if (priv->parity_status)
> +		cmd |= AEON_IPC_CMD_PARITY;
> +
> +	/* Always update parity for next packet */
> +	priv->parity_status = !priv->parity_status;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_CMD, cmd);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for packet to be processed */
> +	usleep_range(AEON_IPC_DELAY, AEON_IPC_DELAY + 5000);
> +
> +	/* With no ret_sts, ignore waiting for packet completion
> +	 * (ipc parity bit sync)
> +	 */
> +	if (!ret_sts)
> +		return 0;
> +
> +	ret = aeon_ipcs_wait_cmd(phydev, curr_parity);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*ret_sts = ret;
> +	if ((*ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_SUCCESS)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int aeon_ipc_send_msg(struct phy_device *phydev, u16 opcode,
> +			     u16 *data, unsigned int data_len, u16 *ret_sts)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	u32 cmd;
> +	int ret;
> +	int i;
> +
> +	/* IPC have a max of 8 register to transfer data,
> +	 * make sure we never exceed this.
> +	 */
> +	if (data_len > AEON_IPC_DATA_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->ipc_lock);
> +
> +	for (i = 0; i < data_len / sizeof(u16); i++)
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i),
> +			      data[i]);
> +
> +	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, data_len) |
> +	      FIELD_PREP(AEON_IPC_CMD_OPCODE, opcode);
> +	ret = aeon_ipc_send_cmd(phydev, cmd, ret_sts);
> +	if (ret)
> +		phydev_err(phydev, "failed to send ipc msg for %x: %d\n", opcode, ret);
> +
> +	mutex_unlock(&priv->ipc_lock);
> +
> +	return ret;
> +}
> +
> +static int aeon_ipc_rcv_msg(struct phy_device *phydev, u16 ret_sts,
> +			    u16 *data)
> +{
> +	unsigned int size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
> +	struct as21xxx_priv *priv = phydev->priv;
> +	int ret;
> +	int i;
> +
> +	if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
> +		return -EINVAL;
> +
> +	/* Prevent IPC from stack smashing the kernel */
> +	if (size > AEON_IPC_DATA_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->ipc_lock);
> +
> +	for (i = 0; i < DIV_ROUND_UP(size, sizeof(u16)); i++) {
> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i));
> +		if (ret < 0) {
> +			size = ret;
> +			goto out;
> +		}
> +
> +		data[i] = ret;
> +	}
> +
> +out:
> +	mutex_unlock(&priv->ipc_lock);
> +
> +	return size;
> +}
> +
> +/* Logic to sync parity bit with IPC.
> + * We send 2 NOP cmd with same partity and we wait for IPC
> + * to handle the packet only for the second one. This way
> + * we make sure we are sync for every next cmd.
> + */
> +static int aeon_ipc_sync_parity(struct phy_device *phydev)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	u16 ret_sts;
> +	u32 cmd;
> +	int ret;
> +
> +	mutex_lock(&priv->ipc_lock);
> +
> +	/* Send NOP with no parity */
> +	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, 0) |
> +	      FIELD_PREP(AEON_IPC_CMD_OPCODE, IPC_CMD_NOOP);
> +	aeon_ipc_send_cmd(phydev, cmd, NULL);
> +
> +	/* Reset packet parity */
> +	priv->parity_status = false;
> +
> +	/* Send second NOP with no parity */
> +	ret = aeon_ipc_send_cmd(phydev, cmd, &ret_sts);
> +
> +	mutex_unlock(&priv->ipc_lock);
> +
> +	/* We expect to return -EFAULT */
> +	if (ret != -EFAULT)
> +		return ret;
> +
> +	if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int aeon_ipc_get_fw_version(struct phy_device *phydev)
> +{
> +	u16 ret_data[8], data[1];
> +	u16 ret_sts;
> +	int ret;
> +
> +	data[0] = IPC_INFO_VERSION;
> +	ret = aeon_ipc_send_msg(phydev, IPC_CMD_INFO, data, sizeof(data),
> +				&ret_sts);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_rcv_msg(phydev, ret_sts, ret_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	phydev_info(phydev, "Firmware Version: %s\n", (char *)ret_data);
> +
> +	return 0;
> +}
> +
> +static int aeon_dpc_ra_enable(struct phy_device *phydev)
> +{
> +	u16 data[2];
> +	u16 ret_sts;
> +
> +	data[0] = IPC_CFG_PARAM_DIRECT;
> +	data[1] = IPC_CFG_PARAM_DIRECT_DPC_RA;
> +
> +	return aeon_ipc_send_msg(phydev, IPC_CMD_CFG_PARAM, data,
> +				 sizeof(data), &ret_sts);
> +}
> +
> +static int as21xxx_probe(struct phy_device *phydev)
> +{
> +	struct as21xxx_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev,
> +			    sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	phydev->priv = priv;
> +
> +	ret = devm_mutex_init(&phydev->mdio.dev,
> +			      &priv->ipc_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_firmware_load(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_sync_parity(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> +			       VEND1_PTP_CLK_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_dpc_ra_enable(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_get_fw_version(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->needs_reregister = true;
> +
> +	return 0;
> +}
> +
> +static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
> +{
> +	int status;
> +
> +	/* Normal C22 BMCR report inconsistent data, use
> +	 * the mapped C22 in C45 to have more consistent link info.
> +	 */
> +	*bmcr = phy_read_mmd(phydev, MDIO_MMD_AN,
> +			     AS21XXX_MDIO_AN_C22 + MII_BMCR);
> +	if (*bmcr < 0)
> +		return *bmcr;
> +
> +	/* Autoneg is being started, therefore disregard current
> +	 * link status and report link as down.
> +	 */
> +	if (*bmcr & BMCR_ANRESTART) {
> +		phydev->link = 0;
> +		return 0;
> +	}
> +
> +	status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +	if (status < 0)
> +		return status;
> +
> +	phydev->link = !!(status & MDIO_STAT1_LSTATUS);
> +
> +	return 0;
> +}
> +
> +static int as21xxx_read_c22_lpa(struct phy_device *phydev)
> +{
> +	int lpagb;
> +
> +	/* MII_STAT1000 are only filled in the mapped C22
> +	 * in C45, use that to fill lpagb values and check.
> +	 */
> +	lpagb = phy_read_mmd(phydev, MDIO_MMD_AN,
> +			     AS21XXX_MDIO_AN_C22 + MII_STAT1000);
> +	if (lpagb < 0)
> +		return lpagb;
> +
> +	if (lpagb & LPA_1000MSFAIL) {
> +		int adv = phy_read_mmd(phydev, MDIO_MMD_AN,
> +				       AS21XXX_MDIO_AN_C22 + MII_CTRL1000);
> +
> +		if (adv < 0)
> +			return adv;
> +
> +		if (adv & CTL1000_ENABLE_MASTER)
> +			phydev_err(phydev, "Master/Slave resolution failed, maybe conflicting manual settings?\n");
> +		else
> +			phydev_err(phydev, "Master/Slave resolution failed\n");
> +		return -ENOLINK;
> +	}
> +
> +	mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
> +					lpagb);
> +
> +	return 0;
> +}
> +
> +static int as21xxx_read_status(struct phy_device *phydev)
> +{
> +	int bmcr, old_link = phydev->link;
> +	int ret;
> +
> +	ret = as21xxx_read_link(phydev, &bmcr);
> +	if (ret)
> +		return ret;
> +
> +	/* why bother the PHY if nothing can have changed */
> +	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
> +		return 0;
> +
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE) {
> +		ret = genphy_c45_read_lpa(phydev);
> +		if (ret)
> +			return ret;
> +
> +		ret = as21xxx_read_c22_lpa(phydev);
> +		if (ret)
> +			return ret;
> +
> +		phy_resolve_aneg_linkmode(phydev);
> +	} else {
> +		int speed;
> +
> +		linkmode_zero(phydev->lp_advertising);
> +
> +		speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> +				     VEND1_SPEED_STATUS);
> +		if (speed < 0)
> +			return speed;
> +
> +		switch (speed & VEND1_SPEED_STATUS) {
> +		case VEND1_SPEED_10000:
> +			phydev->speed = SPEED_10000;
> +			phydev->duplex = DUPLEX_FULL;
> +			break;
> +		case VEND1_SPEED_5000:
> +			phydev->speed = SPEED_5000;
> +			phydev->duplex = DUPLEX_FULL;
> +			break;
> +		case VEND1_SPEED_2500:
> +			phydev->speed = SPEED_2500;
> +			phydev->duplex = DUPLEX_FULL;
> +			break;
> +		case VEND1_SPEED_1000:
> +			phydev->speed = SPEED_1000;
> +			if (bmcr & BMCR_FULLDPLX)
> +				phydev->duplex = DUPLEX_FULL;
> +			else
> +				phydev->duplex = DUPLEX_HALF;
> +			break;
> +		case VEND1_SPEED_100:
> +			phydev->speed = SPEED_100;
> +			phydev->duplex = DUPLEX_FULL;
> +			break;
> +		case VEND1_SPEED_10:
> +			phydev->speed = SPEED_10;
> +			phydev->duplex = DUPLEX_FULL;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int as21xxx_led_brightness_set(struct phy_device *phydev,
> +				      u8 index, enum led_brightness value)
> +{
> +	u16 val = VEND1_LED_REG_A_EVENT_OFF;
> +
> +	if (index > AEON_MAX_LDES)
> +		return -EINVAL;
> +
> +	if (value)
> +		val = VEND1_LED_REG_A_EVENT_ON;
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> +			      VEND1_LED_REG(index),
> +			      VEND1_LED_REG_A_EVENT,
> +			      FIELD_PREP(VEND1_LED_REG_A_EVENT, val));
> +}
> +
> +static int as21xxx_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +				       unsigned long rules)
> +{
> +	int i;
> +
> +	if (index > AEON_MAX_LDES)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(as21xxx_led_supported_pattern); i++)
> +		if (rules == as21xxx_led_supported_pattern[i].pattern)
> +			return 0;
> +
> +	return -EOPNOTSUPP;
> +}
> +

> +static int as21xxx_led_hw_control_get(struct phy_device *phydev, u8 index,
> +				      unsigned long *rules)
> +{
> +	u16 val;
> +	int i;
> +
> +	if (index > AEON_MAX_LDES)
> +		return -EINVAL;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_LED_REG(index));
> +	if (val < 0)

val is unsigned so this condition is never met.

Flagged by Smatch.

> +		return val;
> +
> +	val = FIELD_GET(VEND1_LED_REG_A_EVENT, val);
> +	for (i = 0; i < ARRAY_SIZE(as21xxx_led_supported_pattern); i++)
> +		if (val == as21xxx_led_supported_pattern[i].val) {
> +			*rules = as21xxx_led_supported_pattern[i].pattern;
> +			return 0;
> +		}
> +
> +	/* Should be impossible */
> +	return -EINVAL;
> +}

...

> +static int as21xxx_led_polarity_set(struct phy_device *phydev, int index,
> +				    unsigned long modes)
> +{
> +	bool led_active_low;
> +	u16 mask, val = 0;
> +	u32 mode;
> +
> +	if (index > AEON_MAX_LDES)
> +		return -EINVAL;
> +
> +	for_each_set_bit(mode, &modes, __PHY_LED_MODES_NUM) {
> +		switch (mode) {
> +		case PHY_LED_ACTIVE_LOW:
> +			led_active_low = true;
> +			break;
> +		case PHY_LED_ACTIVE_HIGH: /* default mode */
> +			led_active_low = false;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	mask = VEND1_GLB_CPU_CTRL_LED_POLARITY(index);
> +	if (led_active_low)

Perhaps it cannot happen, but if the loop above iterates zero times
then led_active_low will be uninitialised here.

Also flagged by Smatch.

> +		val = VEND1_GLB_CPU_CTRL_LED_POLARITY(index);
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> +			      VEND1_GLB_REG_CPU_CTRL,
> +			      mask, val);
> +}

...

Please note that net-next is currently closed for the merge-window.
So please wait for it to re-open before posting patches for it.

RFCs are welcome any time.

Simon Horman March 26, 2025, 2:05 p.m. UTC | #4

On Wed, Mar 26, 2025 at 02:00:15PM +0000, Simon Horman wrote:

...

> Please note that net-next is currently closed for the merge-window.
> So please wait for it to re-open before posting patches for it.
> 
> RFCs are welcome any time.

I'm very sorry that I missed that this is an RFC,
so my comment above is just noise.

Christian Marangi March 26, 2025, 2:43 p.m. UTC | #5

On Wed, Mar 26, 2025 at 01:53:39PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 26, 2025 at 01:23:58AM +0100, Christian Marangi wrote:
> > +static int aeon_ipc_send_cmd(struct phy_device *phydev, u32 cmd,
> > +			     u16 *ret_sts)
> > +{
> > +	struct as21xxx_priv *priv = phydev->priv;
> > +	bool curr_parity;
> > +	int ret;
> > +
> > +	/* The IPC sync by using a single parity bit.
> > +	 * Each CMD have alternately this bit set or clear
> > +	 * to understand correct flow and packet order.
> > +	 */
> > +	curr_parity = priv->parity_status;
> > +	if (priv->parity_status)
> > +		cmd |= AEON_IPC_CMD_PARITY;
> > +
> > +	/* Always update parity for next packet */
> > +	priv->parity_status = !priv->parity_status;
> > +
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_CMD, cmd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Wait for packet to be processed */
> > +	usleep_range(AEON_IPC_DELAY, AEON_IPC_DELAY + 5000);
> > +
> > +	/* With no ret_sts, ignore waiting for packet completion
> > +	 * (ipc parity bit sync)
> > +	 */
> > +	if (!ret_sts)
> > +		return 0;
> > +
> > +	ret = aeon_ipcs_wait_cmd(phydev, curr_parity);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*ret_sts = ret;
> > +	if ((*ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_SUCCESS)
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int aeon_ipc_send_msg(struct phy_device *phydev, u16 opcode,
> > +			     u16 *data, unsigned int data_len, u16 *ret_sts)
> > +{
> > +	struct as21xxx_priv *priv = phydev->priv;
> > +	u32 cmd;
> > +	int ret;
> > +	int i;
> > +
> > +	/* IPC have a max of 8 register to transfer data,
> > +	 * make sure we never exceed this.
> > +	 */
> > +	if (data_len > AEON_IPC_DATA_MAX)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&priv->ipc_lock);
> > +
> > +	for (i = 0; i < data_len / sizeof(u16); i++)
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i),
> > +			      data[i]);
> > +
> > +	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, data_len) |
> > +	      FIELD_PREP(AEON_IPC_CMD_OPCODE, opcode);
> > +	ret = aeon_ipc_send_cmd(phydev, cmd, ret_sts);
> > +	if (ret)
> > +		phydev_err(phydev, "failed to send ipc msg for %x: %d\n", opcode, ret);
> > +
> > +	mutex_unlock(&priv->ipc_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int aeon_ipc_rcv_msg(struct phy_device *phydev, u16 ret_sts,
> > +			    u16 *data)
> > +{
> > +	unsigned int size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
> > +	struct as21xxx_priv *priv = phydev->priv;
> > +	int ret;
> > +	int i;
> > +
> > +	if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
> > +		return -EINVAL;
> > +
> > +	/* Prevent IPC from stack smashing the kernel */
> > +	if (size > AEON_IPC_DATA_MAX)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&priv->ipc_lock);
> > +
> > +	for (i = 0; i < DIV_ROUND_UP(size, sizeof(u16)); i++) {
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i));
> > +		if (ret < 0) {
> > +			size = ret;
> > +			goto out;
> > +		}
> > +
> > +		data[i] = ret;
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&priv->ipc_lock);
> > +
> > +	return size;
> > +}
> > +
> > +/* Logic to sync parity bit with IPC.
> > + * We send 2 NOP cmd with same partity and we wait for IPC
> > + * to handle the packet only for the second one. This way
> > + * we make sure we are sync for every next cmd.
> > + */
> > +static int aeon_ipc_sync_parity(struct phy_device *phydev)
> > +{
> > +	struct as21xxx_priv *priv = phydev->priv;
> > +	u16 ret_sts;
> > +	u32 cmd;
> > +	int ret;
> > +
> > +	mutex_lock(&priv->ipc_lock);
> > +
> > +	/* Send NOP with no parity */
> > +	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, 0) |
> > +	      FIELD_PREP(AEON_IPC_CMD_OPCODE, IPC_CMD_NOOP);
> > +	aeon_ipc_send_cmd(phydev, cmd, NULL);
> > +
> > +	/* Reset packet parity */
> > +	priv->parity_status = false;
> > +
> > +	/* Send second NOP with no parity */
> > +	ret = aeon_ipc_send_cmd(phydev, cmd, &ret_sts);
> > +
> > +	mutex_unlock(&priv->ipc_lock);
> > +
> > +	/* We expect to return -EFAULT */
> > +	if (ret != -EFAULT)
> > +		return ret;
> > +
> > +	if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int aeon_ipc_get_fw_version(struct phy_device *phydev)
> > +{
> > +	u16 ret_data[8], data[1];
> > +	u16 ret_sts;
> > +	int ret;
> > +
> > +	data[0] = IPC_INFO_VERSION;
> > +	ret = aeon_ipc_send_msg(phydev, IPC_CMD_INFO, data, sizeof(data),
> > +				&ret_sts);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aeon_ipc_rcv_msg(phydev, ret_sts, ret_data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	phydev_info(phydev, "Firmware Version: %s\n", (char *)ret_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int aeon_dpc_ra_enable(struct phy_device *phydev)
> > +{
> > +	u16 data[2];
> > +	u16 ret_sts;
> > +
> > +	data[0] = IPC_CFG_PARAM_DIRECT;
> > +	data[1] = IPC_CFG_PARAM_DIRECT_DPC_RA;
> > +
> > +	return aeon_ipc_send_msg(phydev, IPC_CMD_CFG_PARAM, data,
> > +				 sizeof(data), &ret_sts);
> > +}
> > +
> > +static int as21xxx_probe(struct phy_device *phydev)
> > +{
> > +	struct as21xxx_priv *priv;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(&phydev->mdio.dev,
> > +			    sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +	phydev->priv = priv;
> > +
> > +	ret = devm_mutex_init(&phydev->mdio.dev,
> > +			      &priv->ipc_lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aeon_firmware_load(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aeon_ipc_sync_parity(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> > +			       VEND1_PTP_CLK_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aeon_dpc_ra_enable(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aeon_ipc_get_fw_version(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	phydev->needs_reregister = true;
> > +
> > +	return 0;
> > +}
> 
> This probe function allocates devres resources that wil lbe freed when
> it returns through the unbinding in patch 1. This is a recipe for
> confusion - struct as21xxx_priv must never be used from any of the
> "real" driver.
> 
> I would suggest:
> 
> 1. document that devres resources will not be preserved when
>    phydev->needs_reregister is set true.
> 
> 2. rename struct as21xxx_priv to struct as21xxx_fw_load_priv to make
>    it clear that it's for firmware loading.
> 
> 3. use a prefix that uniquely identifies those functions that can only
>    be called with this structure populated.
> 
> 4. set phydev->priv to NULL at the end of this probe function to ensure
>    no one dereferences the free'd pointer in a "real" driver, which
>    could lead to use-after-free errors.
> 
> In summary, I really don't like this approach - it feels too much of a
> hack, _and_ introduces the potential for drivers that makes use of this
> to get stuff really very wrong. In my opinion that's not a model that
> we should add to the kernel.
> 
> I'll say again - why can't the PHY firmware be loaded by board firmware.
> You've been silent on my feedback on this point. Given that you're
> ignoring me... for this patch series...
> 
> Hard NAK.
> 
> until you start responding to my review comments.
>

No I wasn't ignoring you, the description in v1 for this was very
generic and confusing so the idea was to post a real implementation so
we could discuss on the code in practice... My comments were done before
checking how phy_registration worked so they were only ideas (the
implementation changed a lot from what was my idea) Sorry if I gave this
impression while I was answering only to Andrew...

The problem of PHY firmware loaded by board firmware is that we
introduce lots of assumption on the table. Also doesn't that goes
against the idea that the kernel should not assume stuff set by the
bootloader (if they can be reset and are not part of the core system?)

From what I'm seeing on devices that have this, SPI is never mounted and
bootloader doesn't provide support for this and the thing is loaded only
by the OS in a crap way.

Also the PHY doesn't keep the FW with an hardware reset and permit the
kernel to load an updated (probably fixed) firmware is only beneficial.
(there is plan to upstream firmware to linux-firmware)

I agree that the approach of declaring a "generic" PHY entry and "abuse"
the probe function is an HACK, but I also feel using match_phy_device
doesn't solve the problem.

Correct me if I'm wrong but match_phy_device will only give true or
false, it won't solve the problem of the PHY changing after the FW is
loaded.

This current approach permit to provide to the user the exact new OPs of
the PHY detected after FW is loaded.

Is it really that bad to introduce the idea that a PHY family require
some initial tuneup before it can correctly identified?

Christian Marangi March 26, 2025, 2:47 p.m. UTC | #6

On Wed, Mar 26, 2025 at 02:57:09PM +0100, Andrew Lunn wrote:
> > In summary, I really don't like this approach - it feels too much of a
> > hack, _and_ introduces the potential for drivers that makes use of this
> > to get stuff really very wrong. In my opinion that's not a model that
> > we should add to the kernel.
> 
> I agree.
> 
> > 
> > I'll say again - why can't the PHY firmware be loaded by board firmware.
> > You've been silent on my feedback on this point. Given that you're
> > ignoring me... for this patch series...
> 
> And i still think using the match op is something that should be
> investigated, alongside the bootloader loading firmware.
>

The main problem (as said in the other answer) is that I feel match op
won't solve the problem and doesn't seems the good place to do complex
operation like OF and load firmware.

But now that I think about it your idea was to define a match_phy_device
for each PHY ID... The generic PHY ID is detected and firmware is loaded
and the PHY ID is checked again?

That way any other call of match_phy_device for other PHY of the same
family will found the FW loaded and check the ID?

That might work but sound more like a trick than a solid implementation.

Andrew Lunn March 26, 2025, 2:56 p.m. UTC | #7

> +#define PHY_ID_AS21XXX			0x75009410
> +/* AS21xxx ID Legend
> + * AS21x1xxB1
> + *     ^ ^^
> + *     | |J: Supports SyncE/PTP
> + *     | |P: No SyncE/PTP support
> + *     | 1: Supports 2nd Serdes
> + *     | 2: Not 2nd Serdes support
> + *     0: 10G, 5G, 2.5G
> + *     5: 5G, 2.5G
> + *     2: 2.5G
> + */
> +#define PHY_ID_AS21011JB1		0x75009402
> +#define PHY_ID_AS21011PB1		0x75009412
> +#define PHY_ID_AS21010JB1		0x75009422
> +#define PHY_ID_AS21010PB1		0x75009432
> +#define PHY_ID_AS21511JB1		0x75009442
> +#define PHY_ID_AS21511PB1		0x75009452
> +#define PHY_ID_AS21510JB1		0x75009462
> +#define PHY_ID_AS21510PB1		0x75009472
> +#define PHY_ID_AS21210JB1		0x75009482
> +#define PHY_ID_AS21210PB1		0x75009492
> +#define PHY_VENDOR_AEONSEMI		0x75009400

O.K. This helps.

> +static struct phy_driver as21xxx_drivers[] = {
> +	{
> +		/* PHY expose in C45 as 0x7500 0x9410
> +		 * before firmware is loaded.
> +		 */
> +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> +		.name		= "Aeonsemi AS21xxx",
> +		.probe		= as21xxx_probe,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
> +		.name		= "Aeonsemi AS21011JB1",
> +		.read_status	= as21xxx_read_status,
> +		.led_brightness_set = as21xxx_led_brightness_set,
> +		.led_hw_is_supported = as21xxx_led_hw_is_supported,
> +		.led_hw_control_set = as21xxx_led_hw_control_set,
> +		.led_hw_control_get = as21xxx_led_hw_control_get,
> +		.led_polarity_set = as21xxx_led_polarity_set,
> +	},

It is guaranteed by the current code that these entries are tried in
the order listed here. If that was to change, other drivers would
break.

So what you can do is have the first entry for PHY_ID_AS21XXX with
as21xxx_probe, have the probe download the firmware and then return
-ENODEV. PHY_ID_AS21XXX tells us there is no firmware, so this is what
we need to do. The -ENODEV then tells the core that this driver entry
does not match the hardware, try the next.

After the firmware download, the phylib core will still have the wrong
ID values. So you cannot use PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1).
But what you can do is have a .match_phy_device function. It will get
called, and it can read the real ID from the device, and perform a
match. If it does not match return -ENODEV, and the core will try the
next entry.

You either need N match_phy_device functions, one per ID value, or you
can make use of the .driver_data in phy_driver, and place the matching
data there.

In the end you should have the correct phy_driver structure for the
device. The core will still have the wrong ID values, which you should
document with a comment. But that mostly only effects
/sys/class/bus/mdio-bus/...

	Andrew

Christian Marangi March 26, 2025, 3:09 p.m. UTC | #8

On Wed, Mar 26, 2025 at 03:56:15PM +0100, Andrew Lunn wrote:
> > +#define PHY_ID_AS21XXX			0x75009410
> > +/* AS21xxx ID Legend
> > + * AS21x1xxB1
> > + *     ^ ^^
> > + *     | |J: Supports SyncE/PTP
> > + *     | |P: No SyncE/PTP support
> > + *     | 1: Supports 2nd Serdes
> > + *     | 2: Not 2nd Serdes support
> > + *     0: 10G, 5G, 2.5G
> > + *     5: 5G, 2.5G
> > + *     2: 2.5G
> > + */
> > +#define PHY_ID_AS21011JB1		0x75009402
> > +#define PHY_ID_AS21011PB1		0x75009412
> > +#define PHY_ID_AS21010JB1		0x75009422
> > +#define PHY_ID_AS21010PB1		0x75009432
> > +#define PHY_ID_AS21511JB1		0x75009442
> > +#define PHY_ID_AS21511PB1		0x75009452
> > +#define PHY_ID_AS21510JB1		0x75009462
> > +#define PHY_ID_AS21510PB1		0x75009472
> > +#define PHY_ID_AS21210JB1		0x75009482
> > +#define PHY_ID_AS21210PB1		0x75009492
> > +#define PHY_VENDOR_AEONSEMI		0x75009400
> 
> O.K. This helps.
> 
> > +static struct phy_driver as21xxx_drivers[] = {
> > +	{
> > +		/* PHY expose in C45 as 0x7500 0x9410
> > +		 * before firmware is loaded.
> > +		 */
> > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> > +		.name		= "Aeonsemi AS21xxx",
> > +		.probe		= as21xxx_probe,
> > +	},
> > +	{
> > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
> > +		.name		= "Aeonsemi AS21011JB1",
> > +		.read_status	= as21xxx_read_status,
> > +		.led_brightness_set = as21xxx_led_brightness_set,
> > +		.led_hw_is_supported = as21xxx_led_hw_is_supported,
> > +		.led_hw_control_set = as21xxx_led_hw_control_set,
> > +		.led_hw_control_get = as21xxx_led_hw_control_get,
> > +		.led_polarity_set = as21xxx_led_polarity_set,
> > +	},
> 
> It is guaranteed by the current code that these entries are tried in
> the order listed here. If that was to change, other drivers would
> break.
> 
> So what you can do is have the first entry for PHY_ID_AS21XXX with
> as21xxx_probe, have the probe download the firmware and then return
> -ENODEV. PHY_ID_AS21XXX tells us there is no firmware, so this is what
> we need to do. The -ENODEV then tells the core that this driver entry
> does not match the hardware, try the next.
> 
> After the firmware download, the phylib core will still have the wrong
> ID values. So you cannot use PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1).
> But what you can do is have a .match_phy_device function. It will get
> called, and it can read the real ID from the device, and perform a
> match. If it does not match return -ENODEV, and the core will try the
> next entry.
> 
> You either need N match_phy_device functions, one per ID value, or you
> can make use of the .driver_data in phy_driver, and place the matching
> data there.
> 
> In the end you should have the correct phy_driver structure for the
> device. The core will still have the wrong ID values, which you should
> document with a comment. But that mostly only effects
> /sys/class/bus/mdio-bus/...
>

Thanks a lot for the hint on how to use .match_phy_device for this,
wasn't aware of the enforced PHY order.

I will investigate this but may I ask who creates the sysfs entry and
at what stage? After phy_register?

Cause if that is the case can't this be solved by making the PHY
function rescan the ID? Checking patch 1 of this series, it won't be
driver_release/attach but just a flag to read the PHY again and update
phy_device.

From what I observed, updating the phy_id entry in phy_device (or
c45_ids) is harmless. A rescan totally deletes the problem of having to
use the match functions.

Russell King (Oracle) March 26, 2025, 6:02 p.m. UTC | #9

On Wed, Mar 26, 2025 at 03:43:47PM +0100, Christian Marangi wrote:
> On Wed, Mar 26, 2025 at 01:53:39PM +0000, Russell King (Oracle) wrote:
> > This probe function allocates devres resources that wil lbe freed when
> > it returns through the unbinding in patch 1. This is a recipe for
> > confusion - struct as21xxx_priv must never be used from any of the
> > "real" driver.
> > 
> > I would suggest:
> > 
> > 1. document that devres resources will not be preserved when
> >    phydev->needs_reregister is set true.
> > 
> > 2. rename struct as21xxx_priv to struct as21xxx_fw_load_priv to make
> >    it clear that it's for firmware loading.
> > 
> > 3. use a prefix that uniquely identifies those functions that can only
> >    be called with this structure populated.
> > 
> > 4. set phydev->priv to NULL at the end of this probe function to ensure
> >    no one dereferences the free'd pointer in a "real" driver, which
> >    could lead to use-after-free errors.
> > 
> > In summary, I really don't like this approach - it feels too much of a
> > hack, _and_ introduces the potential for drivers that makes use of this
> > to get stuff really very wrong. In my opinion that's not a model that
> > we should add to the kernel.
> > 
> > I'll say again - why can't the PHY firmware be loaded by board firmware.
> > You've been silent on my feedback on this point. Given that you're
> > ignoring me... for this patch series...
> > 
> > Hard NAK.
> > 
> > until you start responding to my review comments.
> >
> 
> No I wasn't ignoring you, the description in v1 for this was very
> generic and confusing so the idea was to post a real implementation so
> we could discuss on the code in practice... My comments were done before
> checking how phy_registration worked so they were only ideas (the
> implementation changed a lot from what was my idea) Sorry if I gave this
> impression while I was answering only to Andrew...
> 
> The problem of PHY firmware loaded by board firmware is that we
> introduce lots of assumption on the table. Also doesn't that goes
> against the idea that the kernel should not assume stuff set by the
> bootloader (if they can be reset and are not part of the core system?)
> 
> From what I'm seeing on devices that have this, SPI is never mounted and
> bootloader doesn't provide support for this and the thing is loaded only
> by the OS in a crap way.
> 
> Also the PHY doesn't keep the FW with an hardware reset and permit the
> kernel to load an updated (probably fixed) firmware is only beneficial.
> (there is plan to upstream firmware to linux-firmware)
> 
> I agree that the approach of declaring a "generic" PHY entry and "abuse"
> the probe function is an HACK, but I also feel using match_phy_device
> doesn't solve the problem.
> 
> Correct me if I'm wrong but match_phy_device will only give true or
> false, it won't solve the problem of the PHY changing after the FW is
> loaded.
> 
> This current approach permit to provide to the user the exact new OPs of
> the PHY detected after FW is loaded.
> 
> Is it really that bad to introduce the idea that a PHY family require
> some initial tuneup before it can correctly identified?

It's fragile, really fragile.

phy_device_register() can complete before the module loads, and thus
setting the flag has no effect.

Try building the PHY driver as a module with the code that registers
the PHY built-in...

Russell King (Oracle) March 26, 2025, 6:08 p.m. UTC | #10

On Wed, Mar 26, 2025 at 03:56:15PM +0100, Andrew Lunn wrote:
> After the firmware download, the phylib core will still have the wrong
> ID values. So you cannot use PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1).
> But what you can do is have a .match_phy_device function. It will get
> called, and it can read the real ID from the device, and perform a
> match. If it does not match return -ENODEV, and the core will try the
> next entry.

Before it returns -ENODEV, it could re-read the ID values and fill
them into struct phy_device. This would allow phylib's matching to
work.

> You either need N match_phy_device functions, one per ID value, or you
> can make use of the .driver_data in phy_driver, and place the matching
> data there.

An alternative would be to change the match_phy_device() method to
pass the phy_driver, which would allow a single match_phy_device
function to match the new hardware ID values against the PHY IDs in
the phy_driver without needing to modify the IDs in phy_device.

Christian Marangi March 26, 2025, 6:18 p.m. UTC | #11

On Wed, Mar 26, 2025 at 06:08:09PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 26, 2025 at 03:56:15PM +0100, Andrew Lunn wrote:
> > After the firmware download, the phylib core will still have the wrong
> > ID values. So you cannot use PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1).
> > But what you can do is have a .match_phy_device function. It will get
> > called, and it can read the real ID from the device, and perform a
> > match. If it does not match return -ENODEV, and the core will try the
> > next entry.
> 
> Before it returns -ENODEV, it could re-read the ID values and fill
> them into struct phy_device. This would allow phylib's matching to
> work.
>

Is it ok for PHY driver to change values in phy_device ""externally"" to
phy_device.c ? Maybe you still have to read the other response but a
bool with needs_rescan to handle this internally? 

> > You either need N match_phy_device functions, one per ID value, or you
> > can make use of the .driver_data in phy_driver, and place the matching
> > data there.
> 
> An alternative would be to change the match_phy_device() method to
> pass the phy_driver, which would allow a single match_phy_device
> function to match the new hardware ID values against the PHY IDs in
> the phy_driver without needing to modify the IDs in phy_device.
> 

I also considered extending the function with additional stuff but then
I considered that would mean rework each PHY driver and destroy PHY
driver downstream, not something we should care but still quite a big
task. If the -ENODEV path is not OK, I feel an additional OP is better
than tweaking match_phy_device.

Russell King (Oracle) March 26, 2025, 6:32 p.m. UTC | #12

On Wed, Mar 26, 2025 at 07:18:12PM +0100, Christian Marangi wrote:
> On Wed, Mar 26, 2025 at 06:08:09PM +0000, Russell King (Oracle) wrote:
> > An alternative would be to change the match_phy_device() method to
> > pass the phy_driver, which would allow a single match_phy_device
> > function to match the new hardware ID values against the PHY IDs in
> > the phy_driver without needing to modify the IDs in phy_device.
> > 
> 
> I also considered extending the function with additional stuff but then
> I considered that would mean rework each PHY driver and destroy PHY
> driver downstream, not something we should care but still quite a big
> task. If the -ENODEV path is not OK, I feel an additional OP is better
> than tweaking match_phy_device.

For those in the kernel, 8 files, 26 initialisers.

drivers/net/phy/nxp-c45-tja11xx.c can probably be simplified with
this idea, reducing it down to a pair of functions.

drivers/net/phy/bcm87xx.c can probably be reduced to one match function.

So, doing so would appear to bring benefits to other PHY drivers.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 873aa2cce4d7..9a2df6d221bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -645,6 +645,12 @@  F:	drivers/iio/accel/adxl380.h
 F:	drivers/iio/accel/adxl380_i2c.c
 F:	drivers/iio/accel/adxl380_spi.c
 
+AEONSEMI PHY DRIVER
+M:	Christian Marangi <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/net/phy/as21xxx.c
+
 AF8133J THREE-AXIS MAGNETOMETER DRIVER
 M:	Ondřej Jirman <[e-mail]>
 S:	Maintained
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 41c15a2c2037..0c745bbae6c0 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -119,6 +119,18 @@  config AMCC_QT2025_PHY
 	help
 	  Adds support for the Applied Micro Circuits Corporation QT2025 PHY.
 
+config AS21XXX_PHY
+	tristate "Aeonsemi AS21xxx PHYs"
+	help
+	  Currently supports the Aeonsemi AS21xxx PHY.
+
+	  These are C45 PHYs 10G that require all a generic firmware.
+
+	  Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
+	  AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
+	  AS21210PB1 that all register with the PHY ID 0x7500 0x7500
+	  before the firmware is loaded.
+
 source "drivers/net/phy/aquantia/Kconfig"
 
 config AX88796B_PHY
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index c8dac6e92278..14156bf11267 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -39,6 +39,7 @@  obj-$(CONFIG_AIR_EN8811H_PHY)   += air_en8811h.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
 obj-$(CONFIG_AMCC_QT2025_PHY)	+= qt2025.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia/
+obj-$(CONFIG_AS21XXX_PHY)	+= as21xxx.o
 ifdef CONFIG_AX88796B_RUST_PHY
   obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
 else
diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
new file mode 100644
index 000000000000..e3663bd4433b
--- /dev/null
+++ b/drivers/net/phy/as21xxx.c
@@ -0,0 +1,973 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Aeonsemi AS21XXxX PHY Driver
+ *
+ * Author: Christian Marangi <[e-mail]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+
+#define VEND1_GLB_REG_CPU_RESET_ADDR_LO_BASEADDR 0x3
+#define VEND1_GLB_REG_CPU_RESET_ADDR_HI_BASEADDR 0x4
+
+#define VEND1_GLB_REG_CPU_CTRL		0xe
+#define   VEND1_GLB_CPU_CTRL_MASK	GENMASK(4, 0)
+#define   VEND1_GLB_CPU_CTRL_LED_POLARITY_MASK GENMASK(12, 8)
+#define   VEND1_GLB_CPU_CTRL_LED_POLARITY(_n) FIELD_PREP(VEND1_GLB_CPU_CTRL_LED_POLARITY_MASK, \
+							 BIT(_n))
+
+#define VEND1_FW_START_ADDR		0x100
+
+#define VEND1_GLB_REG_MDIO_INDIRECT_ADDRCMD 0x101
+#define VEND1_GLB_REG_MDIO_INDIRECT_LOAD 0x102
+
+#define VEND1_GLB_REG_MDIO_INDIRECT_STATUS 0x103
+
+#define VEND1_PTP_CLK			0x142
+#define   VEND1_PTP_CLK_EN		BIT(6)
+
+/* 5 LED at step of 0x20
+ * FE: Fast-Ethernet (10/100)
+ * GE: Gigabit-Ethernet (1000)
+ * NG: New-Generation (2500/5000/10000)
+ */
+#define VEND1_LED_REG(_n)		(0x1800 + ((_n) * 0x10))
+#define   VEND1_LED_REG_A_EVENT		GENMASK(15, 11)
+#define VEND1_LED_CONF			0x1881
+#define   VEND1_LED_CONFG_BLINK		GENMASK(7, 0)
+
+#define VEND1_SPEED_STATUS		0x4002
+#define   VEND1_SPEED_MASK		GENMASK(7, 0)
+#define   VEND1_SPEED_10000		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x3)
+#define   VEND1_SPEED_5000		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x5)
+#define   VEND1_SPEED_2500		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x9)
+#define   VEND1_SPEED_1000		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x10)
+#define   VEND1_SPEED_100		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x20)
+#define   VEND1_SPEED_10		FIELD_PREP_CONST(VEND1_SPEED_MASK, 0x0)
+
+#define VEND1_IPC_CMD			0x5801
+#define   AEON_IPC_CMD_PARITY		BIT(15)
+#define   AEON_IPC_CMD_SIZE		GENMASK(10, 6)
+#define   AEON_IPC_CMD_OPCODE		GENMASK(5, 0)
+
+#define IPC_CMD_NOOP			0x0  /* Do nothing */
+#define IPC_CMD_INFO			0x1  /* Get Firmware Version */
+#define IPC_CMD_SYS_CPU			0x2  /* SYS_CPU */
+#define IPC_CMD_BULK_DATA		0xa  /* Pass bulk data in ipc registers. */
+#define IPC_CMD_BULK_WRITE		0xc  /* Write bulk data to memory */
+#define IPC_CMD_CFG_PARAM		0x1a /* Write config parameters to memory */
+#define IPC_CMD_NG_TESTMODE		0x1b /* Set NG test mode and tone */
+#define IPC_CMD_TEMP_MON		0x15 /* Temperature monitoring function */
+#define IPC_CMD_SET_LED			0x23 /* Set led */
+
+#define VEND1_IPC_STS			0x5802
+#define   AEON_IPC_STS_PARITY		BIT(15)
+#define   AEON_IPC_STS_SIZE		GENMASK(14, 10)
+#define   AEON_IPC_STS_OPCODE		GENMASK(9, 4)
+#define   AEON_IPC_STS_STATUS		GENMASK(3, 0)
+#define   AEON_IPC_STS_STATUS_RCVD	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x1)
+#define   AEON_IPC_STS_STATUS_PROCESS	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x2)
+#define   AEON_IPC_STS_STATUS_SUCCESS	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x4)
+#define   AEON_IPC_STS_STATUS_ERROR	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0x8)
+#define   AEON_IPC_STS_STATUS_BUSY	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0xe)
+#define   AEON_IPC_STS_STATUS_READY	FIELD_PREP_CONST(AEON_IPC_STS_STATUS, 0xf)
+
+#define VEND1_IPC_DATA0			0x5808
+#define VEND1_IPC_DATA1			0x5809
+#define VEND1_IPC_DATA2			0x580a
+#define VEND1_IPC_DATA3			0x580b
+#define VEND1_IPC_DATA4			0x580c
+#define VEND1_IPC_DATA5			0x580d
+#define VEND1_IPC_DATA6			0x580e
+#define VEND1_IPC_DATA7			0x580f
+#define VEND1_IPC_DATA(_n)		(VEND1_IPC_DATA0 + (_n))
+
+/* Sub command of CMD_INFO */
+#define IPC_INFO_VERSION		0x1
+
+/* Sub command of CMD_SYS_CPU */
+#define IPC_SYS_CPU_REBOOT		0x3
+#define IPC_SYS_CPU_IMAGE_OFST		0x4
+#define IPC_SYS_CPU_IMAGE_CHECK		0x5
+#define IPC_SYS_CPU_PHY_ENABLE		0x6
+
+/* Sub command of CMD_CFG_PARAM */
+#define IPC_CFG_PARAM_DIRECT		0x4
+
+/* CFG DIRECT sub command */
+#define IPC_CFG_PARAM_DIRECT_NG_PHYCTRL	0x1
+#define IPC_CFG_PARAM_DIRECT_CU_AN	0x2
+#define IPC_CFG_PARAM_DIRECT_SDS_PCS	0x3
+#define IPC_CFG_PARAM_DIRECT_AUTO_EEE	0x4
+#define IPC_CFG_PARAM_DIRECT_SDS_PMA	0x5
+#define IPC_CFG_PARAM_DIRECT_DPC_RA	0x6
+#define IPC_CFG_PARAM_DIRECT_DPC_PKT_CHK 0x7
+#define IPC_CFG_PARAM_DIRECT_DPC_SDS_WAIT_ETH 0x8
+#define IPC_CFG_PARAM_DIRECT_WDT	0x9
+#define IPC_CFG_PARAM_DIRECT_SDS_RESTART_AN 0x10
+#define IPC_CFG_PARAM_DIRECT_TEMP_MON	0x11
+#define IPC_CFG_PARAM_DIRECT_WOL	0x12
+
+/* Sub command of CMD_TEMP_MON */
+#define IPC_CMD_TEMP_MON_GET		0x4
+
+#define AS21XXX_MDIO_AN_C22		0xffe0
+
+#define PHY_ID_AS21XXX			0x75009410
+/* AS21xxx ID Legend
+ * AS21x1xxB1
+ *     ^ ^^
+ *     | |J: Supports SyncE/PTP
+ *     | |P: No SyncE/PTP support
+ *     | 1: Supports 2nd Serdes
+ *     | 2: Not 2nd Serdes support
+ *     0: 10G, 5G, 2.5G
+ *     5: 5G, 2.5G
+ *     2: 2.5G
+ */
+#define PHY_ID_AS21011JB1		0x75009402
+#define PHY_ID_AS21011PB1		0x75009412
+#define PHY_ID_AS21010JB1		0x75009422
+#define PHY_ID_AS21010PB1		0x75009432
+#define PHY_ID_AS21511JB1		0x75009442
+#define PHY_ID_AS21511PB1		0x75009452
+#define PHY_ID_AS21510JB1		0x75009462
+#define PHY_ID_AS21510PB1		0x75009472
+#define PHY_ID_AS21210JB1		0x75009482
+#define PHY_ID_AS21210PB1		0x75009492
+#define PHY_VENDOR_AEONSEMI		0x75009400
+
+#define AEON_MAX_LDES			5
+#define AEON_IPC_DELAY			10000
+#define AEON_IPC_TIMEOUT		(AEON_IPC_DELAY * 100)
+#define AEON_IPC_DATA_MAX		(8 * sizeof(u16))
+
+#define AEON_BOOT_ADDR			0x1000
+#define AEON_CPU_BOOT_ADDR		0x2000
+#define AEON_CPU_CTRL_FW_LOAD		(BIT(4) | BIT(2) | BIT(1) | BIT(0))
+#define AEON_CPU_CTRL_FW_START		BIT(0)
+
+enum as21xxx_led_event {
+	VEND1_LED_REG_A_EVENT_ON_10 = 0x0,
+	VEND1_LED_REG_A_EVENT_ON_100,
+	VEND1_LED_REG_A_EVENT_ON_1000,
+	VEND1_LED_REG_A_EVENT_ON_2500,
+	VEND1_LED_REG_A_EVENT_ON_5000,
+	VEND1_LED_REG_A_EVENT_ON_10000,
+	VEND1_LED_REG_A_EVENT_ON_FE_GE,
+	VEND1_LED_REG_A_EVENT_ON_NG,
+	VEND1_LED_REG_A_EVENT_ON_FULL_DUPLEX,
+	VEND1_LED_REG_A_EVENT_ON_COLLISION,
+	VEND1_LED_REG_A_EVENT_BLINK_TX,
+	VEND1_LED_REG_A_EVENT_BLINK_RX,
+	VEND1_LED_REG_A_EVENT_BLINK_ACT,
+	VEND1_LED_REG_A_EVENT_ON_LINK,
+	VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_ACT,
+	VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_RX,
+	VEND1_LED_REG_A_EVENT_ON_FE_GE_BLINK_ACT,
+	VEND1_LED_REG_A_EVENT_ON_NG_BLINK_ACT,
+	VEND1_LED_REG_A_EVENT_ON_NG_BLINK_FE_GE,
+	VEND1_LED_REG_A_EVENT_ON_FD_BLINK_COLLISION,
+	VEND1_LED_REG_A_EVENT_ON,
+	VEND1_LED_REG_A_EVENT_OFF,
+};
+
+struct as21xxx_led_pattern_info {
+	unsigned int pattern;
+	u16 val;
+};
+
+struct as21xxx_priv {
+	bool parity_status;
+	/* Protect concurrent IPC access */
+	struct mutex ipc_lock;
+};
+
+static struct as21xxx_led_pattern_info as21xxx_led_supported_pattern[] = {
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10),
+		.val = VEND1_LED_REG_A_EVENT_ON_10
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_100),
+		.val = VEND1_LED_REG_A_EVENT_ON_100
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_1000),
+		.val = VEND1_LED_REG_A_EVENT_ON_1000
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_2500),
+		.val = VEND1_LED_REG_A_EVENT_ON_2500
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_5000),
+		.val = VEND1_LED_REG_A_EVENT_ON_5000
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10000),
+		.val = VEND1_LED_REG_A_EVENT_ON_10000
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK),
+		.val = VEND1_LED_REG_A_EVENT_ON_LINK
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
+			   BIT(TRIGGER_NETDEV_LINK_100) |
+			   BIT(TRIGGER_NETDEV_LINK_1000),
+		.val = VEND1_LED_REG_A_EVENT_ON_FE_GE
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_2500) |
+			   BIT(TRIGGER_NETDEV_LINK_5000) |
+			   BIT(TRIGGER_NETDEV_LINK_10000),
+		.val = VEND1_LED_REG_A_EVENT_ON_NG
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_FULL_DUPLEX),
+		.val = VEND1_LED_REG_A_EVENT_ON_FULL_DUPLEX
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_TX),
+		.val = VEND1_LED_REG_A_EVENT_BLINK_TX
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_RX),
+		.val = VEND1_LED_REG_A_EVENT_BLINK_RX
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_TX) |
+			   BIT(TRIGGER_NETDEV_RX),
+		.val = VEND1_LED_REG_A_EVENT_BLINK_ACT
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
+			   BIT(TRIGGER_NETDEV_LINK_100) |
+			   BIT(TRIGGER_NETDEV_LINK_1000) |
+			   BIT(TRIGGER_NETDEV_LINK_2500) |
+			   BIT(TRIGGER_NETDEV_LINK_5000) |
+			   BIT(TRIGGER_NETDEV_LINK_10000),
+		.val = VEND1_LED_REG_A_EVENT_ON_LINK
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
+			   BIT(TRIGGER_NETDEV_LINK_100) |
+			   BIT(TRIGGER_NETDEV_LINK_1000) |
+			   BIT(TRIGGER_NETDEV_LINK_2500) |
+			   BIT(TRIGGER_NETDEV_LINK_5000) |
+			   BIT(TRIGGER_NETDEV_LINK_10000) |
+			   BIT(TRIGGER_NETDEV_TX) |
+			   BIT(TRIGGER_NETDEV_RX),
+		.val = VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_ACT
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
+			   BIT(TRIGGER_NETDEV_LINK_100) |
+			   BIT(TRIGGER_NETDEV_LINK_1000) |
+			   BIT(TRIGGER_NETDEV_LINK_2500) |
+			   BIT(TRIGGER_NETDEV_LINK_5000) |
+			   BIT(TRIGGER_NETDEV_LINK_10000) |
+			   BIT(TRIGGER_NETDEV_RX),
+		.val = VEND1_LED_REG_A_EVENT_ON_LINK_BLINK_RX
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_10) |
+			   BIT(TRIGGER_NETDEV_LINK_100) |
+			   BIT(TRIGGER_NETDEV_LINK_1000) |
+			   BIT(TRIGGER_NETDEV_TX) |
+			   BIT(TRIGGER_NETDEV_RX),
+		.val = VEND1_LED_REG_A_EVENT_ON_FE_GE_BLINK_ACT
+	},
+	{
+		.pattern = BIT(TRIGGER_NETDEV_LINK_2500) |
+			   BIT(TRIGGER_NETDEV_LINK_5000) |
+			   BIT(TRIGGER_NETDEV_LINK_10000) |
+			   BIT(TRIGGER_NETDEV_TX) |
+			   BIT(TRIGGER_NETDEV_RX),
+		.val = VEND1_LED_REG_A_EVENT_ON_NG_BLINK_ACT
+	}
+};
+
+static int aeon_firmware_boot(struct phy_device *phydev, const u8 *data, size_t size)
+{
+	int i, ret;
+	u16 val;
+
+	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_CTRL,
+			     VEND1_GLB_CPU_CTRL_MASK, AEON_CPU_CTRL_FW_LOAD);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_FW_START_ADDR,
+			    AEON_BOOT_ADDR);
+	if (ret)
+		return ret;
+
+	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_MDIO_INDIRECT_ADDRCMD,
+			     0x3ffc, 0xc000);
+	if (ret)
+		return ret;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_MDIO_INDIRECT_STATUS);
+	if (val > 1) {
+		phydev_err(phydev, "wrong origin mdio_indirect_status: %x\n", val);
+		return -EINVAL;
+	}
+
+	/* Firmware is always aligned to u16 */
+	for (i = 0; i < size; i += 2) {
+		val = data[i + 1] << 8 | data[i];
+
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_MDIO_INDIRECT_LOAD, val);
+		if (ret)
+			return ret;
+	}
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_RESET_ADDR_LO_BASEADDR,
+			    lower_16_bits(AEON_CPU_BOOT_ADDR));
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_RESET_ADDR_HI_BASEADDR,
+			    upper_16_bits(AEON_CPU_BOOT_ADDR));
+	if (ret)
+		return ret;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLB_REG_CPU_CTRL,
+			      VEND1_GLB_CPU_CTRL_MASK, AEON_CPU_CTRL_FW_START);
+}
+
+static int aeon_firmware_load(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	const struct firmware *fw;
+	const char *fw_name;
+	int ret;
+
+	ret = of_property_read_string(dev->of_node, "firmware-name",
+				      &fw_name);
+	if (ret)
+		return ret;
+
+	ret = request_firmware(&fw, fw_name, dev);
+	if (ret) {
+		phydev_err(phydev, "failed to find FW file %s (%d)\n",
+			   fw_name, ret);
+		return ret;
+	}
+
+	ret = aeon_firmware_boot(phydev, fw->data, fw->size);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+static inline int aeon_ipcs_wait_cmd(struct phy_device *phydev, bool parity_status)
+{
+	u16 val;
+
+	/* Exit condition logic:
+	 * - Wait for parity bit equal
+	 * - Wait for status success, error OR ready
+	 */
+	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
+					 FIELD_GET(AEON_IPC_STS_PARITY, val) == parity_status &&
+					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_RCVD &&
+					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_PROCESS &&
+					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_BUSY,
+					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);
+}
+
+static int aeon_ipc_send_cmd(struct phy_device *phydev, u32 cmd,
+			     u16 *ret_sts)
+{
+	struct as21xxx_priv *priv = phydev->priv;
+	bool curr_parity;
+	int ret;
+
+	/* The IPC sync by using a single parity bit.
+	 * Each CMD have alternately this bit set or clear
+	 * to understand correct flow and packet order.
+	 */
+	curr_parity = priv->parity_status;
+	if (priv->parity_status)
+		cmd |= AEON_IPC_CMD_PARITY;
+
+	/* Always update parity for next packet */
+	priv->parity_status = !priv->parity_status;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_CMD, cmd);
+	if (ret)
+		return ret;
+
+	/* Wait for packet to be processed */
+	usleep_range(AEON_IPC_DELAY, AEON_IPC_DELAY + 5000);
+
+	/* With no ret_sts, ignore waiting for packet completion
+	 * (ipc parity bit sync)
+	 */
+	if (!ret_sts)
+		return 0;
+
+	ret = aeon_ipcs_wait_cmd(phydev, curr_parity);
+	if (ret)
+		return ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS);
+	if (ret < 0)
+		return ret;
+
+	*ret_sts = ret;
+	if ((*ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_SUCCESS)
+		return -EFAULT;
+
+	return 0;
+}
+
+static int aeon_ipc_send_msg(struct phy_device *phydev, u16 opcode,
+			     u16 *data, unsigned int data_len, u16 *ret_sts)
+{
+	struct as21xxx_priv *priv = phydev->priv;
+	u32 cmd;
+	int ret;
+	int i;
+
+	/* IPC have a max of 8 register to transfer data,
+	 * make sure we never exceed this.
+	 */
+	if (data_len > AEON_IPC_DATA_MAX)
+		return -EINVAL;
+
+	mutex_lock(&priv->ipc_lock);
+
+	for (i = 0; i < data_len / sizeof(u16); i++)
+		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i),
+			      data[i]);
+
+	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, data_len) |
+	      FIELD_PREP(AEON_IPC_CMD_OPCODE, opcode);
+	ret = aeon_ipc_send_cmd(phydev, cmd, ret_sts);
+	if (ret)
+		phydev_err(phydev, "failed to send ipc msg for %x: %d\n", opcode, ret);
+
+	mutex_unlock(&priv->ipc_lock);
+
+	return ret;
+}
+
+static int aeon_ipc_rcv_msg(struct phy_device *phydev, u16 ret_sts,
+			    u16 *data)
+{
+	unsigned int size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
+	struct as21xxx_priv *priv = phydev->priv;
+	int ret;
+	int i;
+
+	if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
+		return -EINVAL;
+
+	/* Prevent IPC from stack smashing the kernel */
+	if (size > AEON_IPC_DATA_MAX)
+		return -EINVAL;
+
+	mutex_lock(&priv->ipc_lock);
+
+	for (i = 0; i < DIV_ROUND_UP(size, sizeof(u16)); i++) {
+		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i));
+		if (ret < 0) {
+			size = ret;
+			goto out;
+		}
+
+		data[i] = ret;
+	}
+
+out:
+	mutex_unlock(&priv->ipc_lock);
+
+	return size;
+}
+
+/* Logic to sync parity bit with IPC.
+ * We send 2 NOP cmd with same partity and we wait for IPC
+ * to handle the packet only for the second one. This way
+ * we make sure we are sync for every next cmd.
+ */
+static int aeon_ipc_sync_parity(struct phy_device *phydev)
+{
+	struct as21xxx_priv *priv = phydev->priv;
+	u16 ret_sts;
+	u32 cmd;
+	int ret;
+
+	mutex_lock(&priv->ipc_lock);
+
+	/* Send NOP with no parity */
+	cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, 0) |
+	      FIELD_PREP(AEON_IPC_CMD_OPCODE, IPC_CMD_NOOP);
+	aeon_ipc_send_cmd(phydev, cmd, NULL);
+
+	/* Reset packet parity */
+	priv->parity_status = false;
+
+	/* Send second NOP with no parity */
+	ret = aeon_ipc_send_cmd(phydev, cmd, &ret_sts);
+
+	mutex_unlock(&priv->ipc_lock);
+
+	/* We expect to return -EFAULT */
+	if (ret != -EFAULT)
+		return ret;
+
+	if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int aeon_ipc_get_fw_version(struct phy_device *phydev)
+{
+	u16 ret_data[8], data[1];
+	u16 ret_sts;
+	int ret;
+
+	data[0] = IPC_INFO_VERSION;
+	ret = aeon_ipc_send_msg(phydev, IPC_CMD_INFO, data, sizeof(data),
+				&ret_sts);
+	if (ret)
+		return ret;
+
+	ret = aeon_ipc_rcv_msg(phydev, ret_sts, ret_data);
+	if (ret < 0)
+		return ret;
+
+	phydev_info(phydev, "Firmware Version: %s\n", (char *)ret_data);
+
+	return 0;
+}
+
+static int aeon_dpc_ra_enable(struct phy_device *phydev)
+{
+	u16 data[2];
+	u16 ret_sts;
+
+	data[0] = IPC_CFG_PARAM_DIRECT;
+	data[1] = IPC_CFG_PARAM_DIRECT_DPC_RA;
+
+	return aeon_ipc_send_msg(phydev, IPC_CMD_CFG_PARAM, data,
+				 sizeof(data), &ret_sts);
+}
+
+static int as21xxx_probe(struct phy_device *phydev)
+{
+	struct as21xxx_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(&phydev->mdio.dev,
+			    sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	phydev->priv = priv;
+
+	ret = devm_mutex_init(&phydev->mdio.dev,
+			      &priv->ipc_lock);
+	if (ret)
+		return ret;
+
+	ret = aeon_firmware_load(phydev);
+	if (ret)
+		return ret;
+
+	ret = aeon_ipc_sync_parity(phydev);
+	if (ret)
+		return ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
+			       VEND1_PTP_CLK_EN);
+	if (ret)
+		return ret;
+
+	ret = aeon_dpc_ra_enable(phydev);
+	if (ret)
+		return ret;
+
+	ret = aeon_ipc_get_fw_version(phydev);
+	if (ret)
+		return ret;
+
+	phydev->needs_reregister = true;
+
+	return 0;
+}
+
+static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
+{
+	int status;
+
+	/* Normal C22 BMCR report inconsistent data, use
+	 * the mapped C22 in C45 to have more consistent link info.
+	 */
+	*bmcr = phy_read_mmd(phydev, MDIO_MMD_AN,
+			     AS21XXX_MDIO_AN_C22 + MII_BMCR);
+	if (*bmcr < 0)
+		return *bmcr;
+
+	/* Autoneg is being started, therefore disregard current
+	 * link status and report link as down.
+	 */
+	if (*bmcr & BMCR_ANRESTART) {
+		phydev->link = 0;
+		return 0;
+	}
+
+	status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+	if (status < 0)
+		return status;
+
+	phydev->link = !!(status & MDIO_STAT1_LSTATUS);
+
+	return 0;
+}
+
+static int as21xxx_read_c22_lpa(struct phy_device *phydev)
+{
+	int lpagb;
+
+	/* MII_STAT1000 are only filled in the mapped C22
+	 * in C45, use that to fill lpagb values and check.
+	 */
+	lpagb = phy_read_mmd(phydev, MDIO_MMD_AN,
+			     AS21XXX_MDIO_AN_C22 + MII_STAT1000);
+	if (lpagb < 0)
+		return lpagb;
+
+	if (lpagb & LPA_1000MSFAIL) {
+		int adv = phy_read_mmd(phydev, MDIO_MMD_AN,
+				       AS21XXX_MDIO_AN_C22 + MII_CTRL1000);
+
+		if (adv < 0)
+			return adv;
+
+		if (adv & CTL1000_ENABLE_MASTER)
+			phydev_err(phydev, "Master/Slave resolution failed, maybe conflicting manual settings?\n");
+		else
+			phydev_err(phydev, "Master/Slave resolution failed\n");
+		return -ENOLINK;
+	}
+
+	mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
+					lpagb);
+
+	return 0;
+}
+
+static int as21xxx_read_status(struct phy_device *phydev)
+{
+	int bmcr, old_link = phydev->link;
+	int ret;
+
+	ret = as21xxx_read_link(phydev, &bmcr);
+	if (ret)
+		return ret;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		ret = genphy_c45_read_lpa(phydev);
+		if (ret)
+			return ret;
+
+		ret = as21xxx_read_c22_lpa(phydev);
+		if (ret)
+			return ret;
+
+		phy_resolve_aneg_linkmode(phydev);
+	} else {
+		int speed;
+
+		linkmode_zero(phydev->lp_advertising);
+
+		speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
+				     VEND1_SPEED_STATUS);
+		if (speed < 0)
+			return speed;
+
+		switch (speed & VEND1_SPEED_STATUS) {
+		case VEND1_SPEED_10000:
+			phydev->speed = SPEED_10000;
+			phydev->duplex = DUPLEX_FULL;
+			break;
+		case VEND1_SPEED_5000:
+			phydev->speed = SPEED_5000;
+			phydev->duplex = DUPLEX_FULL;
+			break;
+		case VEND1_SPEED_2500:
+			phydev->speed = SPEED_2500;
+			phydev->duplex = DUPLEX_FULL;
+			break;
+		case VEND1_SPEED_1000:
+			phydev->speed = SPEED_1000;
+			if (bmcr & BMCR_FULLDPLX)
+				phydev->duplex = DUPLEX_FULL;
+			else
+				phydev->duplex = DUPLEX_HALF;
+			break;
+		case VEND1_SPEED_100:
+			phydev->speed = SPEED_100;
+			phydev->duplex = DUPLEX_FULL;
+			break;
+		case VEND1_SPEED_10:
+			phydev->speed = SPEED_10;
+			phydev->duplex = DUPLEX_FULL;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int as21xxx_led_brightness_set(struct phy_device *phydev,
+				      u8 index, enum led_brightness value)
+{
+	u16 val = VEND1_LED_REG_A_EVENT_OFF;
+
+	if (index > AEON_MAX_LDES)
+		return -EINVAL;
+
+	if (value)
+		val = VEND1_LED_REG_A_EVENT_ON;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1,
+			      VEND1_LED_REG(index),
+			      VEND1_LED_REG_A_EVENT,
+			      FIELD_PREP(VEND1_LED_REG_A_EVENT, val));
+}
+
+static int as21xxx_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	int i;
+
+	if (index > AEON_MAX_LDES)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(as21xxx_led_supported_pattern); i++)
+		if (rules == as21xxx_led_supported_pattern[i].pattern)
+			return 0;
+
+	return -EOPNOTSUPP;
+}
+
+static int as21xxx_led_hw_control_get(struct phy_device *phydev, u8 index,
+				      unsigned long *rules)
+{
+	u16 val;
+	int i;
+
+	if (index > AEON_MAX_LDES)
+		return -EINVAL;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_LED_REG(index));
+	if (val < 0)
+		return val;
+
+	val = FIELD_GET(VEND1_LED_REG_A_EVENT, val);
+	for (i = 0; i < ARRAY_SIZE(as21xxx_led_supported_pattern); i++)
+		if (val == as21xxx_led_supported_pattern[i].val) {
+			*rules = as21xxx_led_supported_pattern[i].pattern;
+			return 0;
+		}
+
+	/* Should be impossible */
+	return -EINVAL;
+}
+
+static int as21xxx_led_hw_control_set(struct phy_device *phydev, u8 index,
+				      unsigned long rules)
+{
+	u16 val = 0;
+	int i;
+
+	if (index > AEON_MAX_LDES)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(as21xxx_led_supported_pattern); i++)
+		if (rules == as21xxx_led_supported_pattern[i].pattern) {
+			val = as21xxx_led_supported_pattern[i].val;
+			break;
+		}
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1,
+			      VEND1_LED_REG(index),
+			      VEND1_LED_REG_A_EVENT,
+			      FIELD_PREP(VEND1_LED_REG_A_EVENT, val));
+}
+
+static int as21xxx_led_polarity_set(struct phy_device *phydev, int index,
+				    unsigned long modes)
+{
+	bool led_active_low;
+	u16 mask, val = 0;
+	u32 mode;
+
+	if (index > AEON_MAX_LDES)
+		return -EINVAL;
+
+	for_each_set_bit(mode, &modes, __PHY_LED_MODES_NUM) {
+		switch (mode) {
+		case PHY_LED_ACTIVE_LOW:
+			led_active_low = true;
+			break;
+		case PHY_LED_ACTIVE_HIGH: /* default mode */
+			led_active_low = false;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	mask = VEND1_GLB_CPU_CTRL_LED_POLARITY(index);
+	if (led_active_low)
+		val = VEND1_GLB_CPU_CTRL_LED_POLARITY(index);
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND1,
+			      VEND1_GLB_REG_CPU_CTRL,
+			      mask, val);
+}
+
+static struct phy_driver as21xxx_drivers[] = {
+	{
+		/* PHY expose in C45 as 0x7500 0x9410
+		 * before firmware is loaded.
+		 */
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
+		.name		= "Aeonsemi AS21xxx",
+		.probe		= as21xxx_probe,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
+		.name		= "Aeonsemi AS21011JB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21011PB1),
+		.name		= "Aeonsemi AS21011PB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21010PB1),
+		.name		= "Aeonsemi AS21010PB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21010JB1),
+		.name		= "Aeonsemi AS21010JB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21210PB1),
+		.name		= "Aeonsemi AS21210PB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21510JB1),
+		.name		= "Aeonsemi AS21510JB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21510PB1),
+		.name		= "Aeonsemi AS21510PB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21511JB1),
+		.name		= "Aeonsemi AS21511JB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21210JB1),
+		.name		= "Aeonsemi AS21210JB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_AS21511PB1),
+		.name		= "Aeonsemi AS21511PB1",
+		.read_status	= as21xxx_read_status,
+		.led_brightness_set = as21xxx_led_brightness_set,
+		.led_hw_is_supported = as21xxx_led_hw_is_supported,
+		.led_hw_control_set = as21xxx_led_hw_control_set,
+		.led_hw_control_get = as21xxx_led_hw_control_get,
+		.led_polarity_set = as21xxx_led_polarity_set,
+	},
+};
+module_phy_driver(as21xxx_drivers);
+
+static struct mdio_device_id __maybe_unused as21xxx_tbl[] = {
+	{ PHY_ID_MATCH_VENDOR(PHY_VENDOR_AEONSEMI) },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, as21xxx_tbl);
+
+MODULE_DESCRIPTION("Aeonsemi AS21xxx PHY driver");
+MODULE_AUTHOR("Christian Marangi <[e-mail]>");
+MODULE_LICENSE("GPL");