+/* load data into the phy's memory */
+static int aquantia_load_memory(struct phy_device *phydev, u32 addr,
+				const u8 *data, size_t len)
+{
+	u16 crc = 0, up_crc;
+	size_t pos;
+
+	/* PHY expect addr in LE */
+	addr = cpu_to_le32(addr);
+
+	phy_write_mmd(phydev, MDIO_MMD_VEND1,
+		      VEND1_GLOBAL_MAILBOX_INTERFACE1,
+		      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
+	phy_write_mmd(phydev, MDIO_MMD_VEND1,
+		      VEND1_GLOBAL_MAILBOX_INTERFACE3,
+		      VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(addr));
+	phy_write_mmd(phydev, MDIO_MMD_VEND1,
+		      VEND1_GLOBAL_MAILBOX_INTERFACE4,
+		      VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr));
+
+	for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) {
+		u32 word = 0;
+
+		memcpy(&word, data + pos, min(sizeof(u32), len - pos));
+
+		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5,
+			      VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word));
+		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE6,
+			      VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(word));
+
+		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE1,
+			      VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE |
+			      VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE);
+
+		/* calculate CRC as we load data to the mailbox.
+		 * We convert word to big-endiang as PHY is BE and ailbox will
+		 * return a BE crc.

_m_ailbox.

And i would consistently uses CRC in comments.

+static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size)
+{
+	const struct aqr_fw_header *header;
+	u32 iram_offset = 0, iram_size = 0;
+	u32 dram_offset = 0, dram_size = 0;
+	char version[VERSION_STRING_SIZE];
+	u16 calculated_crc, read_crc;
+	u32 primary_offset = 0;
+	int ret;
+
+	/* extract saved crc at the end of the fw */
+	memcpy(&read_crc, data + size - 2, sizeof(read_crc));
+	/* crc is saved in big-endian as PHY is BE */
+	read_crc = be16_to_cpu(read_crc);
+	calculated_crc = crc_ccitt_false(0, data, size - 2);
+	if (read_crc != calculated_crc) {
+		phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n",
+			   read_crc, calculated_crc);
+		return -EINVAL;
+	}
+
+	/* Get the primary offset to extract DRAM and IRAM sections. */
+	memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16));

Please add some sanity checks. We should not fully trust the
firmware. Is PRIMARY_OFFSET_OFFSET + sizeof(u16) actually inside the
firmware blob?

+	primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset));
+
+	/* Find the DRAM and IRAM sections within the firmware file. */
+	header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET);

Is header actually inside the firmware blob?

+	memcpy(&iram_offset, &header->iram_offset, sizeof(u8) * 3);
+	memcpy(&iram_size, &header->iram_size, sizeof(u8) * 3);
+	memcpy(&dram_offset, &header->dram_offset, sizeof(u8) * 3);
+	memcpy(&dram_size, &header->dram_size, sizeof(u8) * 3);
+
+	/* offset are in LE and values needs to be converted to cpu endian */
+	iram_offset = le32_to_cpu(iram_offset);
+	iram_size = le32_to_cpu(iram_size);
+	dram_offset = le32_to_cpu(dram_offset);
+	dram_size = le32_to_cpu(dram_size);
+
+	/* Increment the offset with the primary offset. */
+	iram_offset += primary_offset;
+	dram_offset += primary_offset;
+
+	phydev_dbg(phydev, "primary %d IRAM offset=%d size=%d DRAM offset=%d size=%d\n",
+		   primary_offset, iram_offset, iram_size, dram_offset, dram_size);
+
+	strscpy(version, (char *)data + dram_offset + VERSION_STRING_OFFSET,
+		VERSION_STRING_SIZE);

Is version inside the blob....

+static int aqr_firmware_load_nvmem(struct phy_device *phydev)
+{
+	struct nvmem_cell *cell;
+	size_t size;
+	u8 *buf;
+	int ret;
+
+	cell = nvmem_cell_get(&phydev->mdio.dev, "firmware");

Does this need properties in device tree? Please update the binding.

+
+static int aqr_firmware_load_sysfs(struct phy_device *phydev)

_sysfs seems a bit odd here. Does request_firmware still use the user
mode helper? I _thought_ it just went direct to the filesystem?

+{
+	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);

Please update the device tree binding.

+static int aqr_firmware_load(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
+	if (ret > 0)
+		goto exit;

I assume this means a value of 0 indicates there is no firmware
running? Maybe a comment or a #define for 0?

	 Andrew