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

[Javen <javen_xu@realsil.com.cn>]

>
>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?
>

I will remove the lower-level error messages and keep a single error message in rtl8261x_fw_load().

Thanks,
Javen

>    Andrew
>
>---
>pw-bot: cr