[PATCH] net: phy: marvell10g: add firmware load support

[PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
bit is pulled up, the host must load firmware to the PHY after reset.
Add support for loading firmware.

Firmware files are available from Marvell under NDA.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 64c9f3bba2cd..9572426ba1c6 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -27,13 +27,28 @@
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
 #include <linux/sfp.h>
+#include <linux/firmware.h>
+#include <linux/delay.h>
 
 #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
 #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
 
+#define MV_FIRMWARE_HEADER_SIZE		32
+
 enum {
 	MV_PMA_BOOT		= 0xc050,
 	MV_PMA_BOOT_FATAL	= BIT(0),
+	MV_PMA_BOOT_PROGRESS_MASK = 0x0006,
+	MV_PMA_BOOT_WAITING	= 0x0002,
+	MV_PMA_BOOT_FW_LOADED	= BIT(6),
+
+	MV_PCS_FW_LOW_WORD	= 0xd0f0,
+	MV_PCS_FW_HIGH_WORD	= 0xd0f1,
+	MV_PCS_RAM_DATA		= 0xd0f2,
+	MV_PCS_RAM_CHECKSUM	= 0xd0f3,
+
+	MV_PMA_FW_REV1		= 0xc011,
+	MV_PMA_FW_REV2		= 0xc012,
 
 	MV_PCS_BASE_T		= 0x0000,
 	MV_PCS_BASE_R		= 0x1000,
@@ -223,6 +238,99 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
 	return 0;
 }
 
+static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
+		unsigned int size)
+{
+	unsigned int low_byte, high_byte;
+	u16 checksum = 0, ram_checksum;
+	unsigned int i = 0;
+
+	while (i < size) {
+		low_byte = data[i++];
+		high_byte = data[i++];
+		checksum += low_byte + high_byte;
+		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
+				(high_byte << 8) | low_byte);
+		cond_resched();
+	}
+
+	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
+	if (ram_checksum != checksum) {
+		dev_err(&phydev->mdio.dev, "firmware checksum failed");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void mv3310_report_firmware_rev(struct phy_device *phydev)
+{
+	int rev1, rev2;
+
+	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
+	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
+	if (rev1 < 0 || rev2 < 0)
+		return;
+
+	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
+			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
+			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
+}
+
+static int mv3310_load_firmware(struct phy_device *phydev)
+{
+	const struct firmware *fw_entry;
+	char *fw_file;
+	int ret;
+
+	switch (phydev->drv->phy_id) {
+	case MARVELL_PHY_ID_88X3310:
+		fw_file = "mrvl/x3310fw.hdr";
+		break;
+	case MARVELL_PHY_ID_88E2110:
+		fw_file = "mrvl/e21x0fw.hdr";
+		break;
+	default:
+		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
+				phydev->drv->name);
+		return -EINVAL;
+	}
+
+	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
+	if (ret < 0)
+		return ret;
+
+	/* Firmware size must be larger than header, and even */
+	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
+			(fw_entry->size % 2) != 0) {
+		dev_err(&phydev->mdio.dev, "firmware file invalid");
+		return -EINVAL;
+	}
+
+	/* Clear checksum register */
+	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
+
+	/* Set firmware load address */
+	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
+	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
+
+	ret = mv3310_write_firmware(phydev,
+			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
+			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
+	if (ret < 0)
+		return ret;
+
+	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
+			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
+
+	release_firmware(fw_entry);
+
+	msleep(100);
+	mv3310_report_firmware_rev(phydev);
+
+	return 0;
+}
+
 static const struct sfp_upstream_ops mv3310_sfp_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
@@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
 		return -ENODEV;
 	}
 
+	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
+		ret = mv3310_load_firmware(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Russell King - ARM Linux admin]

On Tue, Mar 31, 2020 at 08:47:38PM +0300, Baruch Siach wrote:
> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
> bit is pulled up, the host must load firmware to the PHY after reset.
> Add support for loading firmware.
> 
> Firmware files are available from Marvell under NDA.

As I understand it, the firmware for the different revisions of the
88x3310 are different, so I think the current derivation of filenames
is not correct.

Is this code theoretical, or has it been tested on such a system?  As
far as I'm aware, all the 3310 systems out there so far have been
strapped to boot the firmware from SPI.

> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 64c9f3bba2cd..9572426ba1c6 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -27,13 +27,28 @@
>  #include <linux/marvell_phy.h>
>  #include <linux/phy.h>
>  #include <linux/sfp.h>
> +#include <linux/firmware.h>
> +#include <linux/delay.h>
>  
>  #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>  
> +#define MV_FIRMWARE_HEADER_SIZE		32
> +
>  enum {
>  	MV_PMA_BOOT		= 0xc050,
>  	MV_PMA_BOOT_FATAL	= BIT(0),
> +	MV_PMA_BOOT_PROGRESS_MASK = 0x0006,
> +	MV_PMA_BOOT_WAITING	= 0x0002,
> +	MV_PMA_BOOT_FW_LOADED	= BIT(6),
> +
> +	MV_PCS_FW_LOW_WORD	= 0xd0f0,
> +	MV_PCS_FW_HIGH_WORD	= 0xd0f1,
> +	MV_PCS_RAM_DATA		= 0xd0f2,
> +	MV_PCS_RAM_CHECKSUM	= 0xd0f3,
> +
> +	MV_PMA_FW_REV1		= 0xc011,
> +	MV_PMA_FW_REV2		= 0xc012,
>  
>  	MV_PCS_BASE_T		= 0x0000,
>  	MV_PCS_BASE_R		= 0x1000,
> @@ -223,6 +238,99 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
>  	return 0;
>  }
>  
> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
> +		unsigned int size)
> +{
> +	unsigned int low_byte, high_byte;
> +	u16 checksum = 0, ram_checksum;
> +	unsigned int i = 0;
> +
> +	while (i < size) {
> +		low_byte = data[i++];
> +		high_byte = data[i++];
> +		checksum += low_byte + high_byte;
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
> +				(high_byte << 8) | low_byte);
> +		cond_resched();
> +	}
> +
> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +	if (ram_checksum != checksum) {
> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
> +{
> +	int rev1, rev2;
> +
> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
> +	if (rev1 < 0 || rev2 < 0)
> +		return;
> +
> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
> +}
> +
> +static int mv3310_load_firmware(struct phy_device *phydev)
> +{
> +	const struct firmware *fw_entry;
> +	char *fw_file;
> +	int ret;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MARVELL_PHY_ID_88X3310:
> +		fw_file = "mrvl/x3310fw.hdr";
> +		break;
> +	case MARVELL_PHY_ID_88E2110:
> +		fw_file = "mrvl/e21x0fw.hdr";
> +		break;
> +	default:
> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
> +				phydev->drv->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Firmware size must be larger than header, and even */
> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
> +			(fw_entry->size % 2) != 0) {
> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
> +		return -EINVAL;
> +	}
> +
> +	/* Clear checksum register */
> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +
> +	/* Set firmware load address */
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
> +
> +	ret = mv3310_write_firmware(phydev,
> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
> +
> +	release_firmware(fw_entry);
> +
> +	msleep(100);
> +	mv3310_report_firmware_rev(phydev);
> +
> +	return 0;
> +}
> +
>  static const struct sfp_upstream_ops mv3310_sfp_ops = {
>  	.attach = phy_sfp_attach,
>  	.detach = phy_sfp_detach,
> @@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
>  		return -ENODEV;
>  	}
>  
> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
> +		ret = mv3310_load_firmware(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> -- 
> 2.25.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Heiner Kallweit]

On 31.03.2020 19:47, Baruch Siach wrote:
> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
> bit is pulled up, the host must load firmware to the PHY after reset.
> Add support for loading firmware.
> 
> Firmware files are available from Marvell under NDA.
> 

Loading firmware files that are available under NDA only in GPL-licensed
code may be problematic. I'd expect firmware files to be available in
linux-firmware at least.
I'd be interested in how the other phylib maintainers see this.

Two more remarks inline.

Last but not least:
The patch should have been annotated "net-next", and net-next is closed currently.

> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 64c9f3bba2cd..9572426ba1c6 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -27,13 +27,28 @@
>  #include <linux/marvell_phy.h>
>  #include <linux/phy.h>
>  #include <linux/sfp.h>
> +#include <linux/firmware.h>
> +#include <linux/delay.h>
>  
>  #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>  
> +#define MV_FIRMWARE_HEADER_SIZE		32
> +
>  enum {
>  	MV_PMA_BOOT		= 0xc050,
>  	MV_PMA_BOOT_FATAL	= BIT(0),
> +	MV_PMA_BOOT_PROGRESS_MASK = 0x0006,
> +	MV_PMA_BOOT_WAITING	= 0x0002,
> +	MV_PMA_BOOT_FW_LOADED	= BIT(6),
> +
> +	MV_PCS_FW_LOW_WORD	= 0xd0f0,
> +	MV_PCS_FW_HIGH_WORD	= 0xd0f1,
> +	MV_PCS_RAM_DATA		= 0xd0f2,
> +	MV_PCS_RAM_CHECKSUM	= 0xd0f3,
> +
> +	MV_PMA_FW_REV1		= 0xc011,
> +	MV_PMA_FW_REV2		= 0xc012,
>  
>  	MV_PCS_BASE_T		= 0x0000,
>  	MV_PCS_BASE_R		= 0x1000,
> @@ -223,6 +238,99 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
>  	return 0;
>  }
>  
> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
> +		unsigned int size)
> +{
> +	unsigned int low_byte, high_byte;
> +	u16 checksum = 0, ram_checksum;
> +	unsigned int i = 0;
> +
> +	while (i < size) {
> +		low_byte = data[i++];
> +		high_byte = data[i++];
> +		checksum += low_byte + high_byte;
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
> +				(high_byte << 8) | low_byte);
> +		cond_resched();
> +	}
> +
> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +	if (ram_checksum != checksum) {
> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
> +{
> +	int rev1, rev2;
> +
> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
> +	if (rev1 < 0 || rev2 < 0)
> +		return;
> +
> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
> +}
> +
> +static int mv3310_load_firmware(struct phy_device *phydev)
> +{
> +	const struct firmware *fw_entry;
> +	char *fw_file;
> +	int ret;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MARVELL_PHY_ID_88X3310:
> +		fw_file = "mrvl/x3310fw.hdr";

Firmware files should be declared with MODULE_FIRMWARE().

> +		break;
> +	case MARVELL_PHY_ID_88E2110:
> +		fw_file = "mrvl/e21x0fw.hdr";
> +		break;
> +	default:
> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
> +				phydev->drv->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Firmware size must be larger than header, and even */
> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
> +			(fw_entry->size % 2) != 0) {
> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
> +		return -EINVAL;
> +	}
> +
> +	/* Clear checksum register */
> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +
> +	/* Set firmware load address */
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
> +
> +	ret = mv3310_write_firmware(phydev,
> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
> +
> +	release_firmware(fw_entry);
> +
> +	msleep(100);
> +	mv3310_report_firmware_rev(phydev);
> +
> +	return 0;
> +}
> +
>  static const struct sfp_upstream_ops mv3310_sfp_ops = {
>  	.attach = phy_sfp_attach,
>  	.detach = phy_sfp_detach,
> @@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
>  		return -ENODEV;
>  	}
>  
> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
> +		ret = mv3310_load_firmware(phydev);
> +		if (ret < 0)
> +			return ret;

You bail out from probe if a firmware file can't be loaded that is
available under NDA only. That doesn't seem to be too nice.

> +	}
> +
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> 

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Florian Fainelli]

On 3/31/2020 11:16 AM, Heiner Kallweit wrote:
> On 31.03.2020 19:47, Baruch Siach wrote:
>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>> bit is pulled up, the host must load firmware to the PHY after reset.
>> Add support for loading firmware.
>>
>> Firmware files are available from Marvell under NDA.
>>
> 
> Loading firmware files that are available under NDA only in GPL-licensed
> code may be problematic. I'd expect firmware files to be available in
> linux-firmware at least.
> I'd be interested in how the other phylib maintainers see this.

I second that, having the firmware available in linux-firmware is
necessary for this code to be accepted.
-- 
Florian

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Florian Fainelli]

On 3/31/2020 10:47 AM, Baruch Siach wrote:
> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
> bit is pulled up, the host must load firmware to the PHY after reset.
> Add support for loading firmware.
> 
> Firmware files are available from Marvell under NDA.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---

[snip]

> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
> +		unsigned int size)
> +{
> +	unsigned int low_byte, high_byte;
> +	u16 checksum = 0, ram_checksum;
> +	unsigned int i = 0;
> +
> +	while (i < size) {
> +		low_byte = data[i++];
> +		high_byte = data[i++];
> +		checksum += low_byte + high_byte;
> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
> +				(high_byte << 8) | low_byte);

If there is an error upon write, there is really no point in continuing
any further, so you should just break out of the loop and return an
error. Having to continue and then fail the checksum is going to take an
awful amount of time.

> +		cond_resched();
> +	}
> +
> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);

Likewise, you need to check for phy_read_mmd() returning < 0.

> +	if (ram_checksum != checksum) {
> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
> +{
> +	int rev1, rev2;
> +
> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
> +	if (rev1 < 0 || rev2 < 0)
> +		return;
> +
> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
> +}
> +
> +static int mv3310_load_firmware(struct phy_device *phydev)
> +{
> +	const struct firmware *fw_entry;
> +	char *fw_file;
> +	int ret;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MARVELL_PHY_ID_88X3310:
> +		fw_file = "mrvl/x3310fw.hdr";
> +		break;
> +	case MARVELL_PHY_ID_88E2110:
> +		fw_file = "mrvl/e21x0fw.hdr";
> +		break;
> +	default:
> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
> +				phydev->drv->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Firmware size must be larger than header, and even */
> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
> +			(fw_entry->size % 2) != 0) {
> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
> +		return -EINVAL;
> +	}

You need to release the firmware file here. There is also possibly
another case that you are not covering here, which is that the firmware
on disk is newer than the firmware *already* loaded in the PHY, this
should presumably update the running firmware to the latest copy.

Without being able to publish the firmware in linux-firmware though, all
of this may be moot.

> +
> +	/* Clear checksum register */
> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +
> +	/* Set firmware load address */
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
> +
> +	ret = mv3310_write_firmware(phydev,
> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
> +	if (ret < 0)
> +		return ret;

And here too.

> +
> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
> +
> +	release_firmware(fw_entry);
-- 
Florian

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

Hi Russell,

On Tue, Mar 31 2020, Russell King - ARM Linux admin wrote:
> On Tue, Mar 31, 2020 at 08:47:38PM +0300, Baruch Siach wrote:
>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>> bit is pulled up, the host must load firmware to the PHY after reset.
>> Add support for loading firmware.
>>
>> Firmware files are available from Marvell under NDA.
>
> As I understand it, the firmware for the different revisions of the
> 88x3310 are different, so I think the current derivation of filenames
> is not correct.

I was not aware of that.

Shmuel, have you seen different firmware revisions from Marvell for 88x3310?

> Is this code theoretical, or has it been tested on such a system?  As
> far as I'm aware, all the 3310 systems out there so far have been
> strapped to boot the firmware from SPI.

I tested this code on a system with 88E2110. Shmuel tested similar code
with 88X3310. In both cases the hardware designer preferred run-time
load of PHY firmware.

baruch

>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 114 insertions(+)
>>
>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>> index 64c9f3bba2cd..9572426ba1c6 100644
>> --- a/drivers/net/phy/marvell10g.c
>> +++ b/drivers/net/phy/marvell10g.c
>> @@ -27,13 +27,28 @@
>>  #include <linux/marvell_phy.h>
>>  #include <linux/phy.h>
>>  #include <linux/sfp.h>
>> +#include <linux/firmware.h>
>> +#include <linux/delay.h>
>>
>>  #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
>>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>>
>> +#define MV_FIRMWARE_HEADER_SIZE		32
>> +
>>  enum {
>>  	MV_PMA_BOOT		= 0xc050,
>>  	MV_PMA_BOOT_FATAL	= BIT(0),
>> +	MV_PMA_BOOT_PROGRESS_MASK = 0x0006,
>> +	MV_PMA_BOOT_WAITING	= 0x0002,
>> +	MV_PMA_BOOT_FW_LOADED	= BIT(6),
>> +
>> +	MV_PCS_FW_LOW_WORD	= 0xd0f0,
>> +	MV_PCS_FW_HIGH_WORD	= 0xd0f1,
>> +	MV_PCS_RAM_DATA		= 0xd0f2,
>> +	MV_PCS_RAM_CHECKSUM	= 0xd0f3,
>> +
>> +	MV_PMA_FW_REV1		= 0xc011,
>> +	MV_PMA_FW_REV2		= 0xc012,
>>
>>  	MV_PCS_BASE_T		= 0x0000,
>>  	MV_PCS_BASE_R		= 0x1000,
>> @@ -223,6 +238,99 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
>>  	return 0;
>>  }
>>
>> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
>> +		unsigned int size)
>> +{
>> +	unsigned int low_byte, high_byte;
>> +	u16 checksum = 0, ram_checksum;
>> +	unsigned int i = 0;
>> +
>> +	while (i < size) {
>> +		low_byte = data[i++];
>> +		high_byte = data[i++];
>> +		checksum += low_byte + high_byte;
>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
>> +				(high_byte << 8) | low_byte);
>> +		cond_resched();
>> +	}
>> +
>> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
>> +	if (ram_checksum != checksum) {
>> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
>> +{
>> +	int rev1, rev2;
>> +
>> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
>> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
>> +	if (rev1 < 0 || rev2 < 0)
>> +		return;
>> +
>> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
>> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
>> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
>> +}
>> +
>> +static int mv3310_load_firmware(struct phy_device *phydev)
>> +{
>> +	const struct firmware *fw_entry;
>> +	char *fw_file;
>> +	int ret;
>> +
>> +	switch (phydev->drv->phy_id) {
>> +	case MARVELL_PHY_ID_88X3310:
>> +		fw_file = "mrvl/x3310fw.hdr";
>> +		break;
>> +	case MARVELL_PHY_ID_88E2110:
>> +		fw_file = "mrvl/e21x0fw.hdr";
>> +		break;
>> +	default:
>> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
>> +				phydev->drv->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Firmware size must be larger than header, and even */
>> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
>> +			(fw_entry->size % 2) != 0) {
>> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Clear checksum register */
>> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
>> +
>> +	/* Set firmware load address */
>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
>> +
>> +	ret = mv3310_write_firmware(phydev,
>> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
>> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
>> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
>> +
>> +	release_firmware(fw_entry);
>> +
>> +	msleep(100);
>> +	mv3310_report_firmware_rev(phydev);
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct sfp_upstream_ops mv3310_sfp_ops = {
>>  	.attach = phy_sfp_attach,
>>  	.detach = phy_sfp_detach,
>> @@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
>>  		return -ENODEV;
>>  	}
>>
>> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
>> +		ret = mv3310_load_firmware(phydev);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv)
>>  		return -ENOMEM;
>> --
>> 2.25.1
>>
>>


--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Shmuel H.]

Hi Baruch,

On 01/04/2020 08:01, Baruch Siach wrote:
> Hi Russell,
>
> On Tue, Mar 31 2020, Russell King - ARM Linux admin wrote:
>> On Tue, Mar 31, 2020 at 08:47:38PM +0300, Baruch Siach wrote:
>>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>>> bit is pulled up, the host must load firmware to the PHY after reset.
>>> Add support for loading firmware.
>>>
>>> Firmware files are available from Marvell under NDA.
>> As I understand it, the firmware for the different revisions of the
>> 88x3310 are different, so I think the current derivation of filenames
>> is not correct.
> I was not aware of that.
>
> Shmuel, have you seen different firmware revisions from Marvell for 88x3310?

There are many firmware revisions, but I didn't see any mention of one
being compatible with a specific HW revision on the changelog / datasheets.

>
>> Is this code theoretical, or has it been tested on such a system?  As
>> far as I'm aware, all the 3310 systems out there so far have been
>> strapped to boot the firmware from SPI.
> I tested this code on a system with 88E2110. Shmuel tested similar code
> with 88X3310. In both cases the hardware designer preferred run-time
> load of PHY firmware.
>
> baruch
>
>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>> ---
>>>  drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 114 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>>> index 64c9f3bba2cd..9572426ba1c6 100644
>>> --- a/drivers/net/phy/marvell10g.c
>>> +++ b/drivers/net/phy/marvell10g.c
>>> @@ -27,13 +27,28 @@
>>>  #include <linux/marvell_phy.h>
>>>  #include <linux/phy.h>
>>>  #include <linux/sfp.h>
>>> +#include <linux/firmware.h>
>>> +#include <linux/delay.h>
>>>
>>>  #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
>>>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>>>
>>> +#define MV_FIRMWARE_HEADER_SIZE		32
>>> +
>>>  enum {
>>>  	MV_PMA_BOOT		= 0xc050,
>>>  	MV_PMA_BOOT_FATAL	= BIT(0),
>>> +	MV_PMA_BOOT_PROGRESS_MASK = 0x0006,
>>> +	MV_PMA_BOOT_WAITING	= 0x0002,
>>> +	MV_PMA_BOOT_FW_LOADED	= BIT(6),
>>> +
>>> +	MV_PCS_FW_LOW_WORD	= 0xd0f0,
>>> +	MV_PCS_FW_HIGH_WORD	= 0xd0f1,
>>> +	MV_PCS_RAM_DATA		= 0xd0f2,
>>> +	MV_PCS_RAM_CHECKSUM	= 0xd0f3,
>>> +
>>> +	MV_PMA_FW_REV1		= 0xc011,
>>> +	MV_PMA_FW_REV2		= 0xc012,
>>>
>>>  	MV_PCS_BASE_T		= 0x0000,
>>>  	MV_PCS_BASE_R		= 0x1000,
>>> @@ -223,6 +238,99 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
>>>  	return 0;
>>>  }
>>>
>>> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
>>> +		unsigned int size)
>>> +{
>>> +	unsigned int low_byte, high_byte;
>>> +	u16 checksum = 0, ram_checksum;
>>> +	unsigned int i = 0;
>>> +
>>> +	while (i < size) {
>>> +		low_byte = data[i++];
>>> +		high_byte = data[i++];
>>> +		checksum += low_byte + high_byte;
>>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
>>> +				(high_byte << 8) | low_byte);
>>> +		cond_resched();
>>> +	}
>>> +
>>> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
>>> +	if (ram_checksum != checksum) {
>>> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
>>> +{
>>> +	int rev1, rev2;
>>> +
>>> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
>>> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
>>> +	if (rev1 < 0 || rev2 < 0)
>>> +		return;
>>> +
>>> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
>>> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
>>> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
>>> +}
>>> +
>>> +static int mv3310_load_firmware(struct phy_device *phydev)
>>> +{
>>> +	const struct firmware *fw_entry;
>>> +	char *fw_file;
>>> +	int ret;
>>> +
>>> +	switch (phydev->drv->phy_id) {
>>> +	case MARVELL_PHY_ID_88X3310:
>>> +		fw_file = "mrvl/x3310fw.hdr";
>>> +		break;
>>> +	case MARVELL_PHY_ID_88E2110:
>>> +		fw_file = "mrvl/e21x0fw.hdr";
>>> +		break;
>>> +	default:
>>> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
>>> +				phydev->drv->name);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	/* Firmware size must be larger than header, and even */
>>> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
>>> +			(fw_entry->size % 2) != 0) {
>>> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	/* Clear checksum register */
>>> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
>>> +
>>> +	/* Set firmware load address */
>>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
>>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
>>> +
>>> +	ret = mv3310_write_firmware(phydev,
>>> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
>>> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
>>> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
>>> +
>>> +	release_firmware(fw_entry);
>>> +
>>> +	msleep(100);
>>> +	mv3310_report_firmware_rev(phydev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct sfp_upstream_ops mv3310_sfp_ops = {
>>>  	.attach = phy_sfp_attach,
>>>  	.detach = phy_sfp_detach,
>>> @@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
>>>  		return -ENODEV;
>>>  	}
>>>
>>> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
>>> +		ret = mv3310_load_firmware(phydev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	}
>>> +
>>>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>>>  	if (!priv)
>>>  		return -ENOMEM;
>>> --
>>> 2.25.1
>>>
>>>
>
> --
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

-- 
- Shmuel Hazan

mailto:sh@tkos.co.il | tel:+972-523-746-435 | http://tkos.co.il

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Shmuel H.]

Hi Baruch,

On 01/04/2020 08:07, Shmuel H. wrote:
> Hi Baruch,
>
> On 01/04/2020 08:01, Baruch Siach wrote:
>> Hi Russell,
>>
>> On Tue, Mar 31 2020, Russell King - ARM Linux admin wrote:
>>> On Tue, Mar 31, 2020 at 08:47:38PM +0300, Baruch Siach wrote:
>>>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>>>> bit is pulled up, the host must load firmware to the PHY after reset.
>>>> Add support for loading firmware.
>>>>
>>>> Firmware files are available from Marvell under NDA.
>>> As I understand it, the firmware for the different revisions of the
>>> 88x3310 are different, so I think the current derivation of filenames
>>> is not correct.
>> I was not aware of that.
>>
>> Shmuel, have you seen different firmware revisions from Marvell for 88x3310?
> There are many firmware revisions, but I didn't see any mention of one
> being compatible with a specific HW revision on the changelog / datasheets.
Sorry,
Apparently, Marvell do provide multiple firmware versions for the
MVx3310 (REV A1, REV A0, latest).

>
>>> Is this code theoretical, or has it been tested on such a system?  As
>>> far as I'm aware, all the 3310 systems out there so far have been
>>> strapped to boot the firmware from SPI.
>> I tested this code on a system with 88E2110. Shmuel tested similar code
>> with 88X3310. In both cases the hardware designer preferred run-time
>> load of PHY firmware.
>>
>> baruch
>>
>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>> ---
>>>>  drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 114 insertions(+)
>>>>
>>>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>>>> index 64c9f3bba2cd..9572426ba1c6 100644
>>>> --- a/drivers/net/phy/marvell10g.c
>>>> +++ b/drivers/net/phy/marvell10g.c
>>>> @@ -27,13 +27,28 @@
>>>>  #include <linux/marvell_phy.h>
>>>>  #include <linux/phy.h>
>>>>  #include <linux/sfp.h>
>>>> +#include <linux/firmware.h>
>>>> +#include <linux/delay.h>
>>>>
>>>>  #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
>>>>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>>>>
>>>> +#define MV_FIRMWARE_HEADER_SIZE		32
>>>> +
>>>>  enum {
>>>>  	MV_PMA_BOOT		= 0xc050,
>>>>  	MV_PMA_BOOT_FATAL	= BIT(0),
>>>> +	MV_PMA_BOOT_PROGRESS_MASK = 0x0006,
>>>> +	MV_PMA_BOOT_WAITING	= 0x0002,
>>>> +	MV_PMA_BOOT_FW_LOADED	= BIT(6),
>>>> +
>>>> +	MV_PCS_FW_LOW_WORD	= 0xd0f0,
>>>> +	MV_PCS_FW_HIGH_WORD	= 0xd0f1,
>>>> +	MV_PCS_RAM_DATA		= 0xd0f2,
>>>> +	MV_PCS_RAM_CHECKSUM	= 0xd0f3,
>>>> +
>>>> +	MV_PMA_FW_REV1		= 0xc011,
>>>> +	MV_PMA_FW_REV2		= 0xc012,
>>>>
>>>>  	MV_PCS_BASE_T		= 0x0000,
>>>>  	MV_PCS_BASE_R		= 0x1000,
>>>> @@ -223,6 +238,99 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
>>>>  	return 0;
>>>>  }
>>>>
>>>> +static int mv3310_write_firmware(struct phy_device *phydev, const u8 *data,
>>>> +		unsigned int size)
>>>> +{
>>>> +	unsigned int low_byte, high_byte;
>>>> +	u16 checksum = 0, ram_checksum;
>>>> +	unsigned int i = 0;
>>>> +
>>>> +	while (i < size) {
>>>> +		low_byte = data[i++];
>>>> +		high_byte = data[i++];
>>>> +		checksum += low_byte + high_byte;
>>>> +		phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_DATA,
>>>> +				(high_byte << 8) | low_byte);
>>>> +		cond_resched();
>>>> +	}
>>>> +
>>>> +	ram_checksum = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
>>>> +	if (ram_checksum != checksum) {
>>>> +		dev_err(&phydev->mdio.dev, "firmware checksum failed");
>>>> +		return -EIO;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void mv3310_report_firmware_rev(struct phy_device *phydev)
>>>> +{
>>>> +	int rev1, rev2;
>>>> +
>>>> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV1);
>>>> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_REV2);
>>>> +	if (rev1 < 0 || rev2 < 0)
>>>> +		return;
>>>> +
>>>> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision %d.%d.%d.%d",
>>>> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
>>>> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff);
>>>> +}
>>>> +
>>>> +static int mv3310_load_firmware(struct phy_device *phydev)
>>>> +{
>>>> +	const struct firmware *fw_entry;
>>>> +	char *fw_file;
>>>> +	int ret;
>>>> +
>>>> +	switch (phydev->drv->phy_id) {
>>>> +	case MARVELL_PHY_ID_88X3310:
>>>> +		fw_file = "mrvl/x3310fw.hdr";
>>>> +		break;
>>>> +	case MARVELL_PHY_ID_88E2110:
>>>> +		fw_file = "mrvl/e21x0fw.hdr";
>>>> +		break;
>>>> +	default:
>>>> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s PHY",
>>>> +				phydev->drv->name);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	/* Firmware size must be larger than header, and even */
>>>> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
>>>> +			(fw_entry->size % 2) != 0) {
>>>> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	/* Clear checksum register */
>>>> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
>>>> +
>>>> +	/* Set firmware load address */
>>>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD, 0);
>>>> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD, 0x0010);
>>>> +
>>>> +	ret = mv3310_write_firmware(phydev,
>>>> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
>>>> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
>>>> +			MV_PMA_BOOT_FW_LOADED, MV_PMA_BOOT_FW_LOADED);
>>>> +
>>>> +	release_firmware(fw_entry);
>>>> +
>>>> +	msleep(100);
>>>> +	mv3310_report_firmware_rev(phydev);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static const struct sfp_upstream_ops mv3310_sfp_ops = {
>>>>  	.attach = phy_sfp_attach,
>>>>  	.detach = phy_sfp_detach,
>>>> @@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
>>>>  		return -ENODEV;
>>>>  	}
>>>>
>>>> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
>>>> +		ret = mv3310_load_firmware(phydev);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>>>>  	if (!priv)
>>>>  		return -ENOMEM;
>>>> --
>>>> 2.25.1
>>>>
>>>>
>> --
>>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
>> =}------------------------------------------------ooO--U--Ooo------------{=
>>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

-- 
- Shmuel Hazan

mailto:sh@tkos.co.il | tel:+972-523-746-435 | http://tkos.co.il

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

Hi Heiner,

On Tue, Mar 31 2020, Heiner Kallweit wrote:
> On 31.03.2020 19:47, Baruch Siach wrote:
>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>> bit is pulled up, the host must load firmware to the PHY after reset.
>> Add support for loading firmware.
>>
>> Firmware files are available from Marvell under NDA.
>>
>
> Loading firmware files that are available under NDA only in GPL-licensed
> code may be problematic. I'd expect firmware files to be available in
> linux-firmware at least.
> I'd be interested in how the other phylib maintainers see this.

The inside-secure crypto acceleration driver
(drivers/crypto/inside-secure/) original had only NDA firmware.

> Two more remarks inline.
>
> Last but not least:
> The patch should have been annotated "net-next", and net-next is closed currently.
>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

[...]

>> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) == MV_PMA_BOOT_WAITING) {
>> +		ret = mv3310_load_firmware(phydev);
>> +		if (ret < 0)
>> +			return ret;
>
> You bail out from probe if a firmware file can't be loaded that is
> available under NDA only. That doesn't seem to be too nice.

The code verifies that the PHY is in MV_PMA_BOOT_WAITING state. The PHY
is not usable in this state unless the firmware is loaded. This is just
like the MV_PMA_BOOT_FATAL error in the code above.

In the common case of firmware loaded from SPI flash, the code will not
try to load the firmware.

baruch

>
>> +	}
>> +
>>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv)
>>  		return -ENOMEM;
>>

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

Hi Florian,

On Tue, Mar 31 2020, Florian Fainelli wrote:
> On 3/31/2020 11:16 AM, Heiner Kallweit wrote:
>> On 31.03.2020 19:47, Baruch Siach wrote:
>>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>>> bit is pulled up, the host must load firmware to the PHY after reset.
>>> Add support for loading firmware.
>>>
>>> Firmware files are available from Marvell under NDA.
>> 
>> Loading firmware files that are available under NDA only in GPL-licensed
>> code may be problematic. I'd expect firmware files to be available in
>> linux-firmware at least.
>> I'd be interested in how the other phylib maintainers see this.
>
> I second that, having the firmware available in linux-firmware is
> necessary for this code to be accepted.

That's your decision to make. In any case I'll post an updated version
of this patch with fixes for reference to anyone who needs this
functionality to make these PHYs work.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

Hi Shmuel, Russell,

On Wed, Apr 01 2020, Shmuel H. wrote:
> On 01/04/2020 08:07, Shmuel H. wrote:
>> On 01/04/2020 08:01, Baruch Siach wrote:
>>> On Tue, Mar 31 2020, Russell King - ARM Linux admin wrote:
>>>> On Tue, Mar 31, 2020 at 08:47:38PM +0300, Baruch Siach wrote:
>>>>> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
>>>>> bit is pulled up, the host must load firmware to the PHY after reset.
>>>>> Add support for loading firmware.
>>>>>
>>>>> Firmware files are available from Marvell under NDA.
>>>> As I understand it, the firmware for the different revisions of the
>>>> 88x3310 are different, so I think the current derivation of filenames
>>>> is not correct.
>>> I was not aware of that.
>>>
>>> Shmuel, have you seen different firmware revisions from Marvell for 88x3310?
>> There are many firmware revisions, but I didn't see any mention of one
>> being compatible with a specific HW revision on the changelog / datasheets.
> Sorry,
> Apparently, Marvell do provide multiple firmware versions for the
> MVx3310 (REV A1, REV A0, latest).

I checked the Marvell website again. The text on the links leading to
firmware files appear to hint at different hardware revisions. But the
firmware release notes don't mention any hardware revision dependency.

Russell, have you seen any indication that the latest firmware file does
not support all current PHY revisions?

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

RE: [PATCH] net: phy: marvell10g: add firmware load support

[Ioana Ciornei]

> Subject: [PATCH] net: phy: marvell10g: add firmware load support
> 
> When Marvell 88X3310 and 88E2110 hardware configuration SPI_CONFIG strap
> bit is pulled up, the host must load firmware to the PHY after reset.
> Add support for loading firmware.
> 
> Firmware files are available from Marvell under NDA.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/phy/marvell10g.c | 114 +++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index
> 64c9f3bba2cd..9572426ba1c6 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -27,13 +27,28 @@
>  #include <linux/marvell_phy.h>
>  #include <linux/phy.h>
>  #include <linux/sfp.h>
> +#include <linux/firmware.h>
> +#include <linux/delay.h>
> 

[snip]

> +
> +static void mv3310_report_firmware_rev(struct phy_device *phydev) {
> +	int rev1, rev2;
> +
> +	rev1 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> MV_PMA_FW_REV1);
> +	rev2 = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> MV_PMA_FW_REV2);
> +	if (rev1 < 0 || rev2 < 0)
> +		return;
> +
> +	dev_info(&phydev->mdio.dev, "Loaded firmware revision
> %d.%d.%d.%d",
> +			(rev1 & 0xff00) >> 8, rev1 & 0x00ff,
> +			(rev2 & 0xff00) >> 8, rev2 & 0x00ff); }
> +
> +static int mv3310_load_firmware(struct phy_device *phydev) {
> +	const struct firmware *fw_entry;
> +	char *fw_file;
> +	int ret;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MARVELL_PHY_ID_88X3310:
> +		fw_file = "mrvl/x3310fw.hdr";
> +		break;
> +	case MARVELL_PHY_ID_88E2110:
> +		fw_file = "mrvl/e21x0fw.hdr";
> +		break;

Couldn't the static selection of the firmware be replaced by a new generic 
property in the PHY's device node? This would help to just have a solution
in place that works even if the firmware version sees any upgrades or
new filenames are added.

Also, from a generic standpoint, having a 'firmware-name' property in the
device node would also be of help in a situation when the exact same PHY
is integrated twice on the same board but with different MII side link
modes thus different firmware images.
This is typically the case for Aquantia PHYs.

Ioana

> +	default:
> +		dev_warn(&phydev->mdio.dev, "unknown firmware file for %s
> PHY",
> +				phydev->drv->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Firmware size must be larger than header, and even */
> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
> +			(fw_entry->size % 2) != 0) {
> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
> +		return -EINVAL;
> +	}
> +
> +	/* Clear checksum register */
> +	phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_RAM_CHECKSUM);
> +
> +	/* Set firmware load address */
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_LOW_WORD,
> 0);
> +	phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_HIGH_WORD,
> 0x0010);
> +
> +	ret = mv3310_write_firmware(phydev,
> +			fw_entry->data + MV_FIRMWARE_HEADER_SIZE,
> +			fw_entry->size - MV_FIRMWARE_HEADER_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT,
> +			MV_PMA_BOOT_FW_LOADED,
> MV_PMA_BOOT_FW_LOADED);
> +
> +	release_firmware(fw_entry);
> +
> +	msleep(100);
> +	mv3310_report_firmware_rev(phydev);
> +
> +	return 0;
> +}
> +
>  static const struct sfp_upstream_ops mv3310_sfp_ops = {
>  	.attach = phy_sfp_attach,
>  	.detach = phy_sfp_detach,
> @@ -249,6 +357,12 @@ static int mv3310_probe(struct phy_device *phydev)
>  		return -ENODEV;
>  	}
> 
> +	if ((ret & MV_PMA_BOOT_PROGRESS_MASK) ==
> MV_PMA_BOOT_WAITING) {
> +		ret = mv3310_load_firmware(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> --
> 2.25.1

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Andrew Lunn]

Ioana

> This is typically the case for Aquantia PHYs.

The Aquantia is just odd. I would never use it as a generic example.
If i remember correctly, its 'firmware' is actually made up of
multiple parts, only part of which is actual firmware. It has
provisioning, which can be used to set register values. This
potentially invalidates the driver, which makes assumptions about
reset values of registers. And is contains board specific data, like
eye configuration.

As i understand it, Aquantia customises the firmware for the specific
PHY on each board design.

For a general purpose OS like Linux, this will have to change before
we support firmware upload. We need generic firmware, which is the
same everywhere, and then PHY specific blobs for things like the eye
configuration. This basic idea has been around a long time in the WiFi
world. The Atheros WiFi chipsets needed a board specific blod which
contains calibration data, path losses on the board, in order that the
transmit power could be tuned to prevent it sending too much power out
the aerial.

    Andrew

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Russell King - ARM Linux admin]

On Wed, Apr 01, 2020 at 03:03:21PM +0200, Andrew Lunn wrote:
> For a general purpose OS like Linux, this will have to change before
> we support firmware upload. We need generic firmware, which is the
> same everywhere, and then PHY specific blobs for things like the eye
> configuration. This basic idea has been around a long time in the WiFi
> world. The Atheros WiFi chipsets needed a board specific blod which
> contains calibration data, path losses on the board, in order that the
> transmit power could be tuned to prevent it sending too much power out
> the aerial.

The reality of that approach is that people scratch around on the
Internet trying to find the board specific data file, and end up
using anything they can get their hands on just to have something
that works - irrespective of whether or not it is the correct one.

It's a total usability failure to have board specific blobs.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

RE: [PATCH] net: phy: marvell10g: add firmware load support

[Ioana Ciornei]

> Subject: Re: [PATCH] net: phy: marvell10g: add firmware load support
> 
> Ioana
> 
> > This is typically the case for Aquantia PHYs.
> 
> The Aquantia is just odd. I would never use it as a generic example.
> If i remember correctly, its 'firmware' is actually made up of multiple parts, only
> part of which is actual firmware. It has provisioning, which can be used to set
> register values. This potentially invalidates the driver, which makes assumptions
> about reset values of registers. And is contains board specific data, like eye
> configuration.
> 
> As i understand it, Aquantia customises the firmware for the specific PHY on
> each board design.
> 
> For a general purpose OS like Linux, this will have to change before we support
> firmware upload. We need generic firmware, which is the same everywhere, and
> then PHY specific blobs for things like the eye configuration. This basic idea has
> been around a long time in the WiFi world. The Atheros WiFi chipsets needed a
> board specific blod which contains calibration data, path losses on the board, in
> order that the transmit power could be tuned to prevent it sending too much
> power out the aerial.
> 
>     Andrew

I am just trying to understand the message, are we throwing our hands in the air
and wait for the vendor to change its policy?

If we don't act on this, it doesn't mean that it's not a problem... it is, but it's the bootloader's.

Ioana

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

Hi Florian,

On Tue, Mar 31 2020, Florian Fainelli wrote:
> On 3/31/2020 10:47 AM, Baruch Siach wrote:

[snip]

>> +	ret = request_firmware(&fw_entry, fw_file, &phydev->mdio.dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Firmware size must be larger than header, and even */
>> +	if (fw_entry->size <= MV_FIRMWARE_HEADER_SIZE ||
>> +			(fw_entry->size % 2) != 0) {
>> +		dev_err(&phydev->mdio.dev, "firmware file invalid");
>> +		return -EINVAL;
>> +	}
>
> You need to release the firmware file here.

Will fix.

> There is also possibly another case that you are not covering here,
> which is that the firmware on disk is newer than the firmware
> *already* loaded in the PHY, this should presumably update the running
> firmware to the latest copy.

Firmware is only loaded when the PHY boot state is MV_PMA_BOOT_WAITING
(see below). The code does not attempt to update existing PHY firmware.

> Without being able to publish the firmware in linux-firmware though, all
> of this may be moot.

I can't do much about that, unfortunately.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Andrew Lunn]

> I can't do much about that, unfortunately.

What did Marvell say when you asked them to release the firmware to
firmware-linux?

	Andrew

Re: [PATCH] net: phy: marvell10g: add firmware load support

[Baruch Siach]

Hi Andrew,

On Wed, Apr 01 2020, Andrew Lunn wrote:
>> I can't do much about that, unfortunately.
>
> What did Marvell say when you asked them to release the firmware to
> firmware-linux?

I didn't ask Marvell. Past experience is not very encouraging in this
regard.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -