Message-ID: <Z-FycizAnGoxQLOj@shell.armlinux.org.uk>
Date: Mon, 24 Mar 2025 14:55:46 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 1/2] net: phy: Add support for new Aeonsemi PHYs

First, please include a cover message whenever you send a patch series.

On Sun, Mar 23, 2025 at 11:54:26PM +0100, Christian Marangi wrote:
> Add support for new Aeonsemi 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 0x7500
> before the firmware is loaded.

Hmm. That behaviour is really not nice for the kernel to deal with. C45
PHYs have multiple IDs (there's registers 2,3 and also 14,15, each is
per MMD). Do they all have the same value? Do any of them indicate any
kind of valid OUI ?

If there is no way to sanely detect this PHY, then I would suggest that
it is beyond the ability of the kernel, and at the very least, an
initial firmware version needs to be loaded by board boot firmware so
the PHY _can_ be properly identified.

Basically, it isn't the kernel's job to fix such broken hardware.

> +#define VEND1_LED_REG(_n)		(0x1800 + ((_n) * 0x10))
> +#define   VEND1_LED_REG_A_EVENT		GENMASK(15, 11)
> +#define     VEND1_LED_REG_A_EVENT_ON_10 FIELD_PREP_CONST(VEND1_LED_REG_A_EVENT, 0x0)
> +#define     VEND1_LED_REG_A_EVENT_ON_100 FIELD_PREP_CONST(VEND1_LED_REG_A_EVENT, 0x1)

I really don't like the pattern of "define constants using
FIELD_PREP*()". It seems to me it misses the entire point of the
bitfield macros, which is to prepare and extract bitfields.

When I see:

	swith (foo & BLAH_MASK) {
	case BLAH_OPTION_1:
		...

where BLAH_OPTION_1 is defined using FIELD_PREP*(), it just
makes me shudder.

	SWITCH (FIELD_GET(BLAH_MASK, foo)) {
	case BLAH_OPTION_1:
		...

where BLAH_OPTION_1 is defined as the numerical field value is much
more how the bitfield stuff is supposed to be used.

> +enum {
> +	MDIO_AN_C22 = 0xffe0,

I'd suggest defining this in a driver private namespace, rather than
using the MDIO_xxx which is used by linux/mdio.h

> +	/* Exit condition logic:
> +	 * - Wait for parity bit equal
> +	 * - Wait for status success, error OR ready
> +	 */
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
> +					FIELD_GET(AEON_IPC_STS_PARITY, val) ==
> +						curr_parity &&
> +					(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,
> +					10000, 2000000, false);

Use an inline function, and also please wrap a bit tighter, val seems to
wrap.

	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS,
					val, aeon_cmd_done(curr_parity, val),
					10000, 2000000, false);

> +	if (ret)
> +		return ret;
> +
> +	*ret_sts = val;
> +	if ((val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_SUCCESS)
> +		return -EFAULT;

EFAULT means "Bad address". Does not returning successful status mean
that there was a bad address? If not, please don't do this.

EFAULT is specifically used to return to userspace to tell it that it
passed the kernel a bad address.

> +
> +	return 0;
> +}
> +
> +static int aeon_ipc_send_msg(struct phy_device *phydev, u16 opcode,
> +			     u16 *data, unsigned int data_len, u16 *ret_sts)
> +{
> +	u32 cmd;
> +	int ret;
> +	int i;
> +
> +	if (data_len > AEON_IPC_DATA_MAX)
> +		return -EINVAL;
> +
> +	for (i = 0; i < data_len / sizeof(u16); i++)
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i),
> +			      data[i]);

What ensures that this won't overflow the number of registers?

> +
> +	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);
> +
> +	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);
> +	int ret;
> +	int i;
> +
> +	if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
> +		return -EINVAL;
> +
> +	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)
> +			return ret;
> +
> +		data[i] = ret;

Unsafe. AEON_IPC_STS_SIZE is 5 bits in size, which means this can write
indexes 0..31. You pass in a buffer of 8 u16's on the stack. What stops
the hardware engaging in stack smashing... nothing. Please code more
carefully.

> +	}
> +
> +	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;
> +
> +	/* 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);
> +	/* 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;
> +
> +	phydev->priv = devm_kzalloc(&phydev->mdio.dev,
> +				    sizeof(*priv), GFP_KERNEL);
> +	if (!phydev->priv)
> +		return -ENOMEM;
> +
> +	ret = aeon_firmware_load(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_sync_parity(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable PTP clk if not already Enabled */
> +	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;
> +
> +	return 0;
> +}
> +
> +static int as21xxx_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* AS21xxx supports 100M/1G/2.5G/5G/10G speed. */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> +			   phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> +			   phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> +			   phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> +			 phydev->supported);

Given all this, does genphy_read_abilities() actually read anything
useful from the PHY?

> +static struct phy_driver as21xxx_drivers[] = {
> +	{
> +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> +		.name		= "Aeonsemi AS21xxx",
> +		.probe		= as21xxx_probe,
> +		.get_features	= as21xxx_get_features,
> +		.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,
> +	},

What if firmware was already loaded? I think you implied that this
ID is only present when firmware hasn't been loaded.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists