[RFC PATCH net-next] net: phy: aquantia: add firmware load support

[RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Robert Marko]

Aquantia PHY-s require firmware to be loaded before they start operating.
It can be automatically loaded in case when there is a SPI-NOR connected
to Aquantia PHY-s or can be loaded from the host via MDIO.

This patch adds support for loading the firmware via MDIO as in most cases
there is no SPI-NOR being used to save on cost.
Firmware loading code itself is ported from mainline U-boot with cleanups.

The firmware has mixed values both in big and little endian.
PHY core itself is big-endian but it expects values to be in little-endian.
The firmware is little-endian but CRC-16 value for it is stored at the end
of firmware in big-endian.

It seems the PHY does the conversion internally from firmware that is
little-endian to the PHY that is big-endian on using the mailbox
but mailbox returns a big-endian CRC-16 to verify the written data
integrity.

Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/net/phy/Kconfig         |   1 +
 drivers/net/phy/aquantia_main.c | 276 ++++++++++++++++++++++++++++++++
 2 files changed, 277 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 107880d13d21..558fef3d9491 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -98,6 +98,7 @@ config ADIN1100_PHY
 
 config AQUANTIA_PHY
 	tristate "Aquantia PHYs"
+	select CRC_CCITT
 	help
 	  Currently supports the Aquantia AQ1202, AQ2104, AQR105, AQR405
 
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 334a6904ca5a..d343df3d0e61 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -12,6 +12,10 @@
 #include <linux/delay.h>
 #include <linux/bitfield.h>
 #include <linux/phy.h>
+#include <linux/of.h>
+#include <linux/firmware.h>
+#include <linux/crc-ccitt.h>
+#include <linux/nvmem-consumer.h>
 
 #include "aquantia.h"
 
@@ -92,10 +96,40 @@
 #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES		0xd31b
 
 /* Vendor specific 1, MDIO_MMD_VEND1 */
+#define VEND1_GLOBAL_SC				0x0
+#define VEND1_GLOBAL_SC_SOFT_RESET		BIT(15)
+#define VEND1_GLOBAL_SC_LOW_POWER		BIT(11)
+
 #define VEND1_GLOBAL_FW_ID			0x0020
 #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
 #define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
 
+#define VEND1_GLOBAL_MAILBOX_INTERFACE1			0x0200
+#define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE		BIT(15)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE		BIT(14)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET	BIT(12)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE1_BUSY		BIT(8)
+
+#define VEND1_GLOBAL_MAILBOX_INTERFACE2			0x0201
+#define VEND1_GLOBAL_MAILBOX_INTERFACE3			0x0202
+#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK	GENMASK(15, 0)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16))
+#define VEND1_GLOBAL_MAILBOX_INTERFACE4			0x0203
+#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK	GENMASK(15, 2)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x))
+
+#define VEND1_GLOBAL_MAILBOX_INTERFACE5			0x0204
+#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK	GENMASK(15, 0)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK, (u16)((x) >> 16))
+#define VEND1_GLOBAL_MAILBOX_INTERFACE6			0x0205
+#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK	GENMASK(15, 0)
+#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x)	FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x))
+
+#define VEND1_GLOBAL_CONTROL2			0xc001
+#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST	BIT(15)
+#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD	BIT(6)
+#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL	BIT(0)
+
 #define VEND1_GLOBAL_GEN_STAT2			0xc831
 #define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG	BIT(15)
 
@@ -152,6 +186,30 @@
 #define AQR107_OP_IN_PROG_SLEEP		1000
 #define AQR107_OP_IN_PROG_TIMEOUT	100000
 
+#define UP_RESET_SLEEP		100
+
+/* addresses of memory segments in the phy */
+#define DRAM_BASE_ADDR		0x3FFE0000
+#define IRAM_BASE_ADDR		0x40000000
+
+/* firmware image format constants */
+#define VERSION_STRING_SIZE		0x40
+#define VERSION_STRING_OFFSET		0x0200
+/* primary offset is written at an offset from the start of the fw blob */
+#define PRIMARY_OFFSET_OFFSET		0x8
+/* primary offset needs to be then added to a base offset */
+#define PRIMARY_OFFSET_SHIFT		12
+#define PRIMARY_OFFSET(x)		((x) << PRIMARY_OFFSET_SHIFT)
+#define HEADER_OFFSET			0x300
+
+struct aqr_fw_header {
+	u32 padding;
+	u8 iram_offset[3];
+	u8 iram_size[3];
+	u8 dram_offset[3];
+	u8 dram_size[3];
+} __packed;
+
 struct aqr107_hw_stat {
 	const char *name;
 	int reg;
@@ -677,6 +735,142 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
 	return 0;
 }
 
+/* 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.
+		 */
+		word = cpu_to_be32(word);
+		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
+	}
+
+	up_crc = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE2);
+	if (crc != up_crc) {
+		phydev_err(phydev, "CRC mismatch: calculated 0x%04x PHY 0x%04x\n",
+			   crc, up_crc);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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));
+	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);
+	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);
+	phydev_info(phydev, "loading firmware version '%s'\n", version);
+
+	/* stall the microcprocessor */
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
+		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD);
+
+	phydev_dbg(phydev, "loading DRAM 0x%08x from offset=%d size=%d\n",
+		   DRAM_BASE_ADDR, dram_offset, dram_size);
+	ret = aquantia_load_memory(phydev, DRAM_BASE_ADDR, data + dram_offset,
+				   dram_size);
+	if (ret)
+		return ret;
+
+	phydev_dbg(phydev, "loading IRAM 0x%08x from offset=%d size=%d\n",
+		   IRAM_BASE_ADDR, iram_offset, iram_size);
+	ret = aquantia_load_memory(phydev, IRAM_BASE_ADDR, data + iram_offset,
+				   iram_size);
+	if (ret)
+		return ret;
+
+	/* make sure soft reset and low power mode are clear */
+	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC,
+			   VEND1_GLOBAL_SC_SOFT_RESET | VEND1_GLOBAL_SC_LOW_POWER);
+
+	/* Release the microprocessor. UP_RESET must be held for 100 usec. */
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
+		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL |
+		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD |
+		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST);
+	usleep_range(UP_RESET_SLEEP, UP_RESET_SLEEP * 2);
+
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
+		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD);
+
+	return 0;
+}
+
 static int aqr107_get_rate_matching(struct phy_device *phydev,
 				    phy_interface_t iface)
 {
@@ -711,13 +905,95 @@ static int aqr107_resume(struct phy_device *phydev)
 	return aqr107_wait_processor_intensive_op(phydev);
 }
 
+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");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	buf = nvmem_cell_read(cell, &size);
+	if (IS_ERR(buf)) {
+		ret = PTR_ERR(buf);
+		goto exit;
+	}
+
+	ret = aqr_fw_boot(phydev, buf, size);
+	if (ret)
+		phydev_err(phydev, "firmware loading failed: %d\n", ret);
+
+exit:
+	nvmem_cell_put(cell);
+
+	return ret;
+}
+
+static int aqr_firmware_load_sysfs(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);
+		goto exit;
+	}
+
+	ret = aqr_fw_boot(phydev, fw->data, fw->size);
+	if (ret)
+		phydev_err(phydev, "firmware loading failed: %d\n", ret);
+
+exit:
+	release_firmware(fw);
+
+	return ret;
+}
+
+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;
+
+	ret = aqr_firmware_load_nvmem(phydev);
+	if (!ret)
+		goto exit;
+
+	ret = aqr_firmware_load_sysfs(phydev);
+	if (ret)
+		return ret;
+
+exit:
+	return 0;
+}
+
 static int aqr107_probe(struct phy_device *phydev)
 {
+	int ret;
+
 	phydev->priv = devm_kzalloc(&phydev->mdio.dev,
 				    sizeof(struct aqr107_priv), GFP_KERNEL);
 	if (!phydev->priv)
 		return -ENOMEM;
 
+	ret = aqr_firmware_load(phydev);
+	if (ret)
+		return ret;
+
 	return aqr_hwmon_probe(phydev);
 }
 
-- 
2.41.0

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Andrew Lunn]

> +/* 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

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Christian Marangi]

On Mon, Oct 02, 2023 at 10:18:05PM +0200, Andrew Lunn wrote:
> > +/* 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.
>

This is problematic... Since this is a plain standard PHY and we don't
have a compatible (as it's matched with the PHY id) we don't have DT to
add this... Sooo how to add this? Should we update the generic-phy dt?

Should we create a dummy dt and add a compatible adding
ethernet-phy.ID... just for this properties?

This is why we were a bit confused about adding a DT commit to this.

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

-- 
	Ansuel

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Andrew Lunn]

> This is problematic... Since this is a plain standard PHY and we don't
> have a compatible (as it's matched with the PHY id) we don't have DT to
> add this... Sooo how to add this? Should we update the generic-phy dt?
> 
> Should we create a dummy dt and add a compatible adding
> ethernet-phy.ID... just for this properties?
> 
> This is why we were a bit confused about adding a DT commit to this.

Just do what other PHYs do. ti,dp83869.yaml, motorcomm,yt8xxx.yaml,
nxp,tja11xx.yaml, etc.

	Andrew

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[kernel test robot]

Hi Robert,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Robert-Marko/net-phy-aquantia-add-firmware-load-support/20230930-184113
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230930104008.234831-1-robimarko%40gmail.com
patch subject: [RFC PATCH net-next] net: phy: aquantia: add firmware load support
config: i386-randconfig-063-20231003 (https://download.01.org/0day-ci/archive/20231003/202310031545.L584NIxt-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/202310031545.L584NIxt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310031545.L584NIxt-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/aquantia_main.c:746:14: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [usertype] addr @@     got restricted __le32 [usertype] @@
   drivers/net/phy/aquantia_main.c:746:14: sparse:     expected unsigned int [usertype] addr
   drivers/net/phy/aquantia_main.c:746:14: sparse:     got restricted __le32 [usertype]
>> drivers/net/phy/aquantia_main.c:776:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] word @@     got restricted __be32 [usertype] @@
   drivers/net/phy/aquantia_main.c:776:22: sparse:     expected unsigned int [addressable] [usertype] word
   drivers/net/phy/aquantia_main.c:776:22: sparse:     got restricted __be32 [usertype]
>> drivers/net/phy/aquantia_main.c:803:20: sparse: sparse: cast to restricted __be16
>> drivers/net/phy/aquantia_main.c:813:26: sparse: sparse: cast to restricted __le32
   drivers/net/phy/aquantia_main.c:823:23: sparse: sparse: cast to restricted __le32
   drivers/net/phy/aquantia_main.c:824:21: sparse: sparse: cast to restricted __le32
   drivers/net/phy/aquantia_main.c:825:23: sparse: sparse: cast to restricted __le32
   drivers/net/phy/aquantia_main.c:826:21: sparse: sparse: cast to restricted __le32

vim +746 drivers/net/phy/aquantia_main.c

   737	
   738	/* load data into the phy's memory */
   739	static int aquantia_load_memory(struct phy_device *phydev, u32 addr,
   740					const u8 *data, size_t len)
   741	{
   742		u16 crc = 0, up_crc;
   743		size_t pos;
   744	
   745		/* PHY expect addr in LE */
 > 746		addr = cpu_to_le32(addr);
   747	
   748		phy_write_mmd(phydev, MDIO_MMD_VEND1,
   749			      VEND1_GLOBAL_MAILBOX_INTERFACE1,
   750			      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
   751		phy_write_mmd(phydev, MDIO_MMD_VEND1,
   752			      VEND1_GLOBAL_MAILBOX_INTERFACE3,
   753			      VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(addr));
   754		phy_write_mmd(phydev, MDIO_MMD_VEND1,
   755			      VEND1_GLOBAL_MAILBOX_INTERFACE4,
   756			      VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr));
   757	
   758		for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) {
   759			u32 word = 0;
   760	
   761			memcpy(&word, data + pos, min(sizeof(u32), len - pos));
   762	
   763			phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5,
   764				      VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word));
   765			phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE6,
   766				      VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(word));
   767	
   768			phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE1,
   769				      VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE |
   770				      VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE);
   771	
   772			/* calculate CRC as we load data to the mailbox.
   773			 * We convert word to big-endiang as PHY is BE and ailbox will
   774			 * return a BE crc.
   775			 */
 > 776			word = cpu_to_be32(word);
   777			crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
   778		}
   779	
   780		up_crc = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE2);
   781		if (crc != up_crc) {
   782			phydev_err(phydev, "CRC mismatch: calculated 0x%04x PHY 0x%04x\n",
   783				   crc, up_crc);
   784			return -EINVAL;
   785		}
   786	
   787		return 0;
   788	}
   789	
   790	static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size)
   791	{
   792		const struct aqr_fw_header *header;
   793		u32 iram_offset = 0, iram_size = 0;
   794		u32 dram_offset = 0, dram_size = 0;
   795		char version[VERSION_STRING_SIZE];
   796		u16 calculated_crc, read_crc;
   797		u32 primary_offset = 0;
   798		int ret;
   799	
   800		/* extract saved crc at the end of the fw */
   801		memcpy(&read_crc, data + size - 2, sizeof(read_crc));
   802		/* crc is saved in big-endian as PHY is BE */
 > 803		read_crc = be16_to_cpu(read_crc);
   804		calculated_crc = crc_ccitt_false(0, data, size - 2);
   805		if (read_crc != calculated_crc) {
   806			phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n",
   807				   read_crc, calculated_crc);
   808			return -EINVAL;
   809		}
   810	
   811		/* Get the primary offset to extract DRAM and IRAM sections. */
   812		memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16));
 > 813		primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset));
   814	
   815		/* Find the DRAM and IRAM sections within the firmware file. */
   816		header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET);
   817		memcpy(&iram_offset, &header->iram_offset, sizeof(u8) * 3);
   818		memcpy(&iram_size, &header->iram_size, sizeof(u8) * 3);
   819		memcpy(&dram_offset, &header->dram_offset, sizeof(u8) * 3);
   820		memcpy(&dram_size, &header->dram_size, sizeof(u8) * 3);
   821	
   822		/* offset are in LE and values needs to be converted to cpu endian */
   823		iram_offset = le32_to_cpu(iram_offset);
   824		iram_size = le32_to_cpu(iram_size);
   825		dram_offset = le32_to_cpu(dram_offset);
   826		dram_size = le32_to_cpu(dram_size);
   827	
   828		/* Increment the offset with the primary offset. */
   829		iram_offset += primary_offset;
   830		dram_offset += primary_offset;
   831	
   832		phydev_dbg(phydev, "primary %d IRAM offset=%d size=%d DRAM offset=%d size=%d\n",
   833			   primary_offset, iram_offset, iram_size, dram_offset, dram_size);
   834	
   835		strscpy(version, (char *)data + dram_offset + VERSION_STRING_OFFSET,
   836			VERSION_STRING_SIZE);
   837		phydev_info(phydev, "loading firmware version '%s'\n", version);
   838	
   839		/* stall the microcprocessor */
   840		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
   841			      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD);
   842	
   843		phydev_dbg(phydev, "loading DRAM 0x%08x from offset=%d size=%d\n",
   844			   DRAM_BASE_ADDR, dram_offset, dram_size);
   845		ret = aquantia_load_memory(phydev, DRAM_BASE_ADDR, data + dram_offset,
   846					   dram_size);
   847		if (ret)
   848			return ret;
   849	
   850		phydev_dbg(phydev, "loading IRAM 0x%08x from offset=%d size=%d\n",
   851			   IRAM_BASE_ADDR, iram_offset, iram_size);
   852		ret = aquantia_load_memory(phydev, IRAM_BASE_ADDR, data + iram_offset,
   853					   iram_size);
   854		if (ret)
   855			return ret;
   856	
   857		/* make sure soft reset and low power mode are clear */
   858		phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC,
   859				   VEND1_GLOBAL_SC_SOFT_RESET | VEND1_GLOBAL_SC_LOW_POWER);
   860	
   861		/* Release the microprocessor. UP_RESET must be held for 100 usec. */
   862		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
   863			      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL |
   864			      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD |
   865			      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST);
   866		usleep_range(UP_RESET_SLEEP, UP_RESET_SLEEP * 2);
   867	
   868		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
   869			      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD);
   870	
   871		return 0;
   872	}
   873	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Christian Marangi]

On Mon, Oct 02, 2023 at 11:07:25PM +0200, Andrew Lunn wrote:
> > This is problematic... Since this is a plain standard PHY and we don't
> > have a compatible (as it's matched with the PHY id) we don't have DT to
> > add this... Sooo how to add this? Should we update the generic-phy dt?
> > 
> > Should we create a dummy dt and add a compatible adding
> > ethernet-phy.ID... just for this properties?
> > 
> > This is why we were a bit confused about adding a DT commit to this.
> 
> Just do what other PHYs do. ti,dp83869.yaml, motorcomm,yt8xxx.yaml,
> nxp,tja11xx.yaml, etc.
> 

Thanks I prepared a DT hoping it will be good in v2.

-- 
	Ansuel

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Simon Horman]

On Sat, Sep 30, 2023 at 12:39:44PM +0200, Robert Marko wrote:
> Aquantia PHY-s require firmware to be loaded before they start operating.
> It can be automatically loaded in case when there is a SPI-NOR connected
> to Aquantia PHY-s or can be loaded from the host via MDIO.
> 
> This patch adds support for loading the firmware via MDIO as in most cases
> there is no SPI-NOR being used to save on cost.
> Firmware loading code itself is ported from mainline U-boot with cleanups.
> 
> The firmware has mixed values both in big and little endian.
> PHY core itself is big-endian but it expects values to be in little-endian.
> The firmware is little-endian but CRC-16 value for it is stored at the end
> of firmware in big-endian.
> 
> It seems the PHY does the conversion internally from firmware that is
> little-endian to the PHY that is big-endian on using the mailbox
> but mailbox returns a big-endian CRC-16 to verify the written data
> integrity.
> 
> Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Robert Marko <robimarko@gmail.com>

...

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

...

> @@ -677,6 +735,142 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/* 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);

Hi Christian and Robert,


The type of addr us u32, but here it is assigned a __le32 value.

As flagged by Sparse.

> +
> +	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.
> +		 */
> +		word = cpu_to_be32(word);
> +		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
> +	}
> +
> +	up_crc = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE2);
> +	if (crc != up_crc) {
> +		phydev_err(phydev, "CRC mismatch: calculated 0x%04x PHY 0x%04x\n",
> +			   crc, up_crc);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +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);

The type of read_crc is u16.
But be16_to_cpu expects a __be16 argument.

As flagged by Sparse.


> +	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));
> +	primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset));

Similarly here.

> +
> +	/* Find the DRAM and IRAM sections within the firmware file. */
> +	header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET);
> +	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);

And here (x4).

> +
> +	/* 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);
> +	phydev_info(phydev, "loading firmware version '%s'\n", version);
> +
> +	/* stall the microcprocessor */
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
> +		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD);
> +
> +	phydev_dbg(phydev, "loading DRAM 0x%08x from offset=%d size=%d\n",
> +		   DRAM_BASE_ADDR, dram_offset, dram_size);
> +	ret = aquantia_load_memory(phydev, DRAM_BASE_ADDR, data + dram_offset,
> +				   dram_size);
> +	if (ret)
> +		return ret;
> +
> +	phydev_dbg(phydev, "loading IRAM 0x%08x from offset=%d size=%d\n",
> +		   IRAM_BASE_ADDR, iram_offset, iram_size);
> +	ret = aquantia_load_memory(phydev, IRAM_BASE_ADDR, data + iram_offset,
> +				   iram_size);
> +	if (ret)
> +		return ret;
> +
> +	/* make sure soft reset and low power mode are clear */
> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC,
> +			   VEND1_GLOBAL_SC_SOFT_RESET | VEND1_GLOBAL_SC_LOW_POWER);
> +
> +	/* Release the microprocessor. UP_RESET must be held for 100 usec. */
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
> +		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL |
> +		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD |
> +		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST);
> +	usleep_range(UP_RESET_SLEEP, UP_RESET_SLEEP * 2);
> +
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2,
> +		      VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD);
> +
> +	return 0;
> +}

...

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Jakub Kicinski]

On Sat, 30 Sep 2023 12:39:44 +0200 Robert Marko wrote:
> +	ret = of_property_read_string(dev->of_node, "firmware-name",
> +				      &fw_name);

Perhaps a well established weirdness of the embedded world but why read
the fw name from OF?! You can identify what PHY it is and decide the
file name based on that. And also put that fw name in MODULE_FIRMWARE()
so that initramfs can be built with appropriate file in place :S

> +	ret = request_firmware(&fw, fw_name, dev);

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Andrew Lunn]

On Wed, Oct 04, 2023 at 04:28:31PM -0700, Jakub Kicinski wrote:
> On Sat, 30 Sep 2023 12:39:44 +0200 Robert Marko wrote:
> > +	ret = of_property_read_string(dev->of_node, "firmware-name",
> > +				      &fw_name);
> 
> Perhaps a well established weirdness of the embedded world but why read
> the fw name from OF?! You can identify what PHY it is and decide the
> file name based on that. And also put that fw name in MODULE_FIRMWARE()
> so that initramfs can be built with appropriate file in place :S

The Aquantia PHY and its `firmware` is just weird. It is more than
just firmware, it also contains what i think they call provisioning.
That is basically the reset defaults for registers. And not everything
is documented, and i think parts of that provision contains SERDES eye
configuration. So i think you end up with a custom firmware per board?
And you can never trust the firmware in one device will do the same
thing as a different firmware in another device, because the reset
defaults are a bit fuzzy. The PHY driver is somewhat built on sand,
since you cannot really trust any register to have any specific reset
value.

So i can understand putting the board specific firmware name in DT,
and that the firmware will never be in linux-firmware because it would
not scale, and there never being one firmware usable for all
boards. And this odd way of doing things means the usual mechanisms
for getting the firmware in initramfs does not work.

I suppose the question is, do we want to say this is all too ugly for
Linux, solve it in the bootloader, or spend the extra $0.50 for a
flash chip and put the firmware in at the factory. As a kernel
developer i would want the boot loader to solve this, so i can TFTP
boot the kernel. I would also like having a rescue mechanism for when
i brick Linux on the box and need to boot a Debian install image to
recover it.

    Andrew

Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

[Jakub Kicinski]

On Thu, 5 Oct 2023 04:43:51 +0200 Andrew Lunn wrote:
> The Aquantia PHY and its `firmware` is just weird. It is more than
> just firmware, it also contains what i think they call provisioning.
> That is basically the reset defaults for registers. And not everything
> is documented, and i think parts of that provision contains SERDES eye
> configuration. So i think you end up with a custom firmware per board?

Ah, that makes sense, thanks for explaining.