Re: [PATCH net-next v7 4/4] net: phy: realtek: load firmware for RTL8261C_CG

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Jun 29, 2026 at 02:47:18PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> This patch adds support for loading firmware. Download some parameters
> for RTL8261C_CG.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
> ---
> Changes in v2:
>  - remove __pack, struct rtl8261x_fw_header and rtl8261x_fw_entry will not pad
>  - reverse xmas tree for some definition
>  - add explanation on rtl_phy_write_mmd_bits()
> 
> Changes in v3:
>  - add struct rtl8261x_priv
> 
> Changes in v4:
>  - add struct device *dev
> 
> Changes in v5:
>  - no changes
> 
> Changes in v6:
>  - replace rtl_phy_write_mmd_bits with phy_modify_mmd, keep mdio lock
>  - check msb and lsb at the beginning of rtl8261x_fw_execute_entry()
>  - add comments on rtl8261x_config_init()
> 
> Changes in v7:
>  - no changes
> ---
>  drivers/net/phy/realtek/realtek_main.c | 220 +++++++++++++++++++++++++
>  1 file changed, 220 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index ef3700894ebf..bf7bc19fb44c 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -8,7 +8,9 @@
>   * Copyright (c) 2004 Freescale Semiconductor, Inc.
>   */
>  #include <linux/bitops.h>
> +#include <linux/crc32.h>
>  #include <linux/ethtool_netlink.h>
> +#include <linux/firmware.h>
>  #include <linux/of.h>
>  #include <linux/phy.h>
>  #include <linux/pm_wakeirq.h>
> @@ -281,6 +283,42 @@
>  					 RTL8261X_INT_ALDPS_CHG | \
>  					 RTL8261X_INT_JABBER)
>  
> +#define FW_MAIN_MAGIC			0x52544C38
> +#define FW_SUB_MAGIC_8261C		0x32363143
> +#define RTL8261X_POLL_TIMEOUT_MS	100
> +
> +#define RTL8261C_CE_FW_NAME	"rtl_nic/rtl8261c.bin"
> +MODULE_FIRMWARE(RTL8261C_CE_FW_NAME);
> +
> +enum rtl8261x_fw_op {
> +	OP_WRITE = 0x00,	/* Write */
> +	OP_POLL  = 0x02,	/* Polling */
> +};
> +
> +struct rtl8261x_fw_header {
> +	__le32 main_magic;	/* Main magic number 0x52544C38 ("RTL8") */
> +	__le32 sub_magic;	/* Sub magic number */
> +	__le16 version_major;	/* Major version */
> +	__le16 version_minor;	/* Minor version */
> +	__le16 num_entries;	/* Number of entries */
> +	__le16 reserved;	/* Reserved */
> +	__le32 crc32;		/* CRC32 checksum */
> +};
> +
> +struct rtl8261x_fw_entry {
> +	__u8  type;		/* Operation type (OP_*) */
> +	__u8  dev;		/* MMD device */
> +	__le16 addr;		/* Register address */
> +	__u8  msb;		/* MSB bit position */
> +	__u8  lsb;		/* LSB bit position */
> +	__le16 value;		/* Value to write/compare */
> +	__le16 timeout_ms;	/* Poll timeout in milliseconds */
> +	__u8  poll_set;		/* Poll for set (1) or clear (0) */
> +	__u8  reserved;		/* Reserved */
> +};
> +
> +#define FW_HEADER_SIZE		sizeof(struct rtl8261x_fw_header)
> +#define FW_ENTRY_SIZE		sizeof(struct rtl8261x_fw_entry)
>  
>  /* RTL8211E and RTL8211F support up to three LEDs */
>  #define RTL8211x_LED_COUNT			3
> @@ -300,6 +338,11 @@ struct rtl821x_priv {
>  	u16 iner;
>  };
>  
> +struct rtl8261x_priv {
> +	const char *fw_name;
> +	bool fw_loaded;
> +};
> +
>  static int rtl821x_read_page(struct phy_device *phydev)
>  {
>  	return __phy_read(phydev, RTL821x_PAGE_SELECT);
> @@ -342,8 +385,16 @@ static int rtl821x_modify_ext_page(struct phy_device *phydev, u16 ext_page,
>  
>  static int rtl8261x_probe(struct phy_device *phydev)
>  {
> +	struct device *dev = &phydev->mdio.dev;
> +	struct rtl8261x_priv *priv;
>  	int sub_phy_id, ret;
>  
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phydev->priv = priv;
> +
>  	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, RTL8261X_EXT_ADDR_REG,
>  			    RTL_8261X_SUB_PHY_ID_ADDR);
>  	if (ret < 0)
> @@ -357,6 +408,7 @@ static int rtl8261x_probe(struct phy_device *phydev)
>  
>  	switch (sub_phy_id) {
>  	case RTL8261C_CE_MODEL:
> +		priv->fw_name = RTL8261C_CE_FW_NAME;
>  		phydev_info(phydev, "RTL8261C detected (sub_id 0x%02x)\n", sub_phy_id);
>  		break;
>  
> @@ -407,6 +459,153 @@ static int rtl8261x_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int rtl8261x_verify_firmware(struct phy_device *phydev, const struct firmware *fw)
> +{
> +	const struct rtl8261x_fw_header *hdr;
> +	u32 main_magic, sub_magic;
> +	u32 calc_crc, file_crc;
> +	size_t data_len;
> +	u16 num_entries;
> +
> +	if (fw->size < FW_HEADER_SIZE) {
> +		phydev_err(phydev, "Firmware too small: %zu bytes\n", fw->size);
> +		return -EINVAL;
> +	}
> +
> +	hdr = (const struct rtl8261x_fw_header *)fw->data;
> +
> +	main_magic = le32_to_cpu(hdr->main_magic);
> +	if (main_magic != FW_MAIN_MAGIC) {
> +		phydev_err(phydev, "Invalid firmware magic: 0x%08x\n", main_magic);
> +		return -EINVAL;
> +	}
> +
> +	sub_magic = le32_to_cpu(hdr->sub_magic);
> +	if (sub_magic != FW_SUB_MAGIC_8261C) {
> +		phydev_err(phydev, "Invalid sub magic: 0x%08x\n", sub_magic);
> +		return -EINVAL;
> +	}
> +
> +	num_entries = le16_to_cpu(hdr->num_entries);
> +	data_len = num_entries * FW_ENTRY_SIZE;
> +
> +	if (fw->size != sizeof(*hdr) + data_len) {
> +		phydev_err(phydev, "Firmware size mismatch\n");
> +		return -EINVAL;
> +	}
> +
> +	calc_crc = crc32(~0, fw->data + FW_HEADER_SIZE, data_len) ^ ~0;
> +	file_crc = le32_to_cpu(hdr->crc32);
> +
> +	if (calc_crc != file_crc) {
> +		phydev_err(phydev, "CRC32 mismatch: calculated=0x%08x file=0x%08x\n",
> +			   calc_crc, file_crc);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtl8261x_fw_execute_entry(struct phy_device *phydev,
> +				     const struct rtl8261x_fw_entry *entry)
> +{
> +	u16 addr, value, timeout_ms;
> +	u8 dev, msb, lsb, poll_set;
> +	u32 bits, expect_val;
> +	int ret = 0;
> +	int val;
> +
> +	dev = entry->dev;
> +	addr = le16_to_cpu(entry->addr);
> +	msb = entry->msb;
> +	lsb = entry->lsb;
> +	value = le16_to_cpu(entry->value);
> +	timeout_ms = le16_to_cpu(entry->timeout_ms);
> +	poll_set = entry->poll_set;
> +
> +	if (timeout_ms == 0)
> +		timeout_ms = RTL8261X_POLL_TIMEOUT_MS;
> +
> +	if (msb > 15 || lsb > msb) {
> +		phydev_err(phydev, "Invalid firmware bits: msb=%d, lsb=%d\n", msb, lsb);
> +		return -EINVAL;
> +	}
> +
> +	switch (entry->type) {
> +	case OP_WRITE:
> +		ret = phy_modify_mmd(phydev, dev, addr,
> +				     GENMASK(msb, lsb), (value << lsb) & GENMASK(msb, lsb));
> +		if (ret) {
> +			phydev_err(phydev, "WRITE failed: dev=%d addr=0x%04x\n", dev, addr);
> +			return ret;

Here you have a return on error.

> +		}
> +		break;
> +
> +	case OP_POLL: {
> +		bits = GENMASK(msb, lsb);
> +		expect_val = (value << lsb) & bits;
> +
> +		if (poll_set)
> +			ret = phy_read_mmd_poll_timeout(phydev, dev, addr, val,
> +							(val & bits) == expect_val,
> +							1000, timeout_ms * 1000, false);
> +		else
> +			ret = phy_read_mmd_poll_timeout(phydev, dev, addr, val,
> +							(val & bits) != expect_val,
> +							1000, timeout_ms * 1000, false);
> +		if (ret)
> +			phydev_err(phydev, "POLL timeout: dev=%d addr=0x%04x\n",
> +				   dev, addr);
> +		break;

Here you don't. It would be better if the code was consistent.

> +	}
> +	default:
> +		phydev_err(phydev, "Unknown firmware operation: %d\n", entry->type);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int rtl8261x_fw_load(struct phy_device *phydev)
> +{
> +	struct rtl8261x_priv *priv = phydev->priv;
> +	const struct rtl8261x_fw_entry *entry;
> +	const struct rtl8261x_fw_header *hdr;
> +	const struct firmware *fw;
> +	int ret, i;
> +
> +	if (!priv->fw_name)
> +		return 0;
> +
> +	ret = request_firmware(&fw, priv->fw_name, &phydev->mdio.dev);
> +	if (ret) {
> +		phydev_err(phydev, "Failed to load firmware %s: %d\n", priv->fw_name, ret);
> +		return ret;
> +	}
> +
> +	ret = rtl8261x_verify_firmware(phydev, fw);
> +	if (ret)
> +		goto release_fw;
> +
> +	hdr = (const struct rtl8261x_fw_header *)fw->data;
> +
> +	entry = (const struct rtl8261x_fw_entry *)(fw->data + FW_HEADER_SIZE);
> +	for (i = 0; i < le16_to_cpu(hdr->num_entries); i++, entry++) {
> +		ret = rtl8261x_fw_execute_entry(phydev, entry);
> +		if (ret) {
> +			phydev_err(phydev, "Entry %d failed: %d\n", i, ret);
> +			goto release_fw;
> +		}
> +	}
> +
> +	priv->fw_loaded = true;
> +
> +release_fw:
> +	release_firmware(fw);
> +	return ret;
> +}
> +
>  static int rtl8261x_config_intr(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -484,6 +683,26 @@ static int rtl8261x_config_aneg(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int rtl8261x_config_init(struct phy_device *phydev)
> +{
> +	struct rtl8261x_priv *priv = phydev->priv;
> +	int ret = 0;
> +
> +	/* The firmware parameters are preserved across IEEE soft resets and
> +	 * suspend/resume cycles. Reloading is only necessary after a power
> +	 * cycle or hard reset.
> +	 */
> +	if (priv->fw_name && !priv->fw_loaded) {
> +		ret = rtl8261x_fw_load(phydev);
> +		if (ret) {
> +			phydev_err(phydev, "Firmware loading failed: %d\n", ret);

It seems pretty verbose when things go wrong. Do you need an error
message at every level?

    Andrew

---
pw-bot: cr