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

[Ioana Ciornei <ioana.ciornei@nxp.com>]

> 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