diff mbox series

[net-next,1/4] net: phy: marvell10g: Support firmware loading on 88X3310

Message ID 20231214201442.660447-2-tobias@waldekranz.com
State New
Headers show
Series net: phy: marvell10g: Firmware loading and LED support for 88X3310 | expand

Commit Message

Tobias Waldekranz Dec. 14, 2023, 8:14 p.m. UTC
When probing, if a device is waiting for firmware to be loaded into
its RAM, ask userspace for the binary and load it over XMDIO.

We have no choice but to bail out of the probe if firmware is not
available, as the device does not have any built-in image on which to
fall back.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/phy/marvell10g.c | 149 +++++++++++++++++++++++++++++++++++
 1 file changed, 149 insertions(+)

Comments

Andrew Lunn Dec. 15, 2023, 2:30 p.m. UTC | #1
On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> When probing, if a device is waiting for firmware to be loaded into
> its RAM, ask userspace for the binary and load it over XMDIO.

Does a device without firmware have valid ID registers? Is the driver
going to probe via bus enumeration, or is it necessary to use a
compatible with ID values?

> +	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {

This validates that the firmware is big enough to hold the header...

> +		memcpy(&hdr, sect, sizeof(hdr));
> +		hdr.data.size = cpu_to_le32(hdr.data.size);
> +		hdr.data.addr = cpu_to_le32(hdr.data.addr);
> +		hdr.data.csum = cpu_to_le16(hdr.data.csum);
> +		hdr.next_hdr = cpu_to_le32(hdr.next_hdr);

I'm surprised sparse is not complaining about this. You have the same
source and destination, and sparse probably wants the destination to
be marked as little endian.

> +		hdr.csum = cpu_to_le16(hdr.csum);
> +
> +		for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
> +			csum += sect[i];
> +
> +		if ((u16)~csum != hdr.csum) {
> +			dev_err(&phydev->mdio.dev, "Corrupt section header\n");
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));

What i don't see is any validation that the firmware left at sect +
sizeof(hdr) big enough to contain hdr.data.size bytes.

	    Andrew
Andrew Lunn Dec. 15, 2023, 2:33 p.m. UTC | #2
> +	for (i = 0, csum = 0; i < hdr->data.size; i++)
> +		csum += data[i];

> +	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
> +		memcpy(&hdr, sect, sizeof(hdr));
> +		hdr.data.size = cpu_to_le32(hdr.data.size);

hdr.data.size is little endian. Doing a for loop using a little endian
test seems wrong. Should this actually be le32_to_cpu()?

       Andrew
Russell King (Oracle) Dec. 15, 2023, 2:34 p.m. UTC | #3
On Fri, Dec 15, 2023 at 03:30:09PM +0100, Andrew Lunn wrote:
> On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> > When probing, if a device is waiting for firmware to be loaded into
> > its RAM, ask userspace for the binary and load it over XMDIO.
> 
> Does a device without firmware have valid ID registers?

Yes it does - remember one of the ZII dev boards had a 3310 without
firmware, and that can be successfully probed by the driver, which
is key to my userspace tooling being able to access the PHY (since
userspace can only access devices on MDIO buses that are bound to
a netdev.)
Russell King (Oracle) Dec. 15, 2023, 3:52 p.m. UTC | #4
On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> +	MV_PMA_BOOT_PRGS_MASK	= 0x0006,
> +	MV_PMA_BOOT_PRGS_INIT	= 0x0000,
> +	MV_PMA_BOOT_PRGS_WAIT	= 0x0002,
> +	MV_PMA_BOOT_PRGS_CSUM	= 0x0004,
> +	MV_PMA_BOOT_PRGS_JRAM	= 0x0006,

You only seem to use PRGS_WAIT, the rest seem unused.

> +struct mv3310_fw_hdr {
> +	struct {
> +		u32 size;
> +		u32 addr;
> +		u16 csum;
> +	} __packed data;

It's probably better to get rid of this embedded struct and just place
the members in the parent struct (although csum woul dneed to be
renamed).

> +
> +	u8 flags;
> +#define MV3310_FW_HDR_DATA_ONLY BIT(6)
> +
> +	u8 port_skip;
> +	u32 next_hdr;
> +	u16 csum;
> +
> +	u8 pad[14];
> +} __packed;
> +
> +static int mv3310_load_fw_sect(struct phy_device *phydev,
> +			       const struct mv3310_fw_hdr *hdr, const u8 *data)
> +{
> +	int err = 0;
> +	size_t i;
> +	u16 csum;
> +
> +	dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n",
> +		hdr->data.size,
> +		(hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable",
> +		hdr->data.addr);
> +
> +	for (i = 0, csum = 0; i < hdr->data.size; i++)
> +		csum += data[i];
> +
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Corrupt section data\n");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +
> +	/* Any existing checksum is cleared by a read */
> +	__phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW,  hdr->data.addr & 0xffff);
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16);
> +
> +	for (i = 0; i < hdr->data.size; i += 2) {
> +		__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA,
> +				(data[i + 1] << 8) | data[i]);
> +	}
> +
> +	csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Download failed\n");
> +		err = -EIO;
> +		goto unlock;
> +	}
> +
> +	if (hdr->flags & MV3310_FW_HDR_DATA_ONLY)
> +		goto unlock;
> +
> +	__phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED);
> +	mdelay(200);
> +	if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) {
> +		dev_err(&phydev->mdio.dev, "Application did not startup\n");
> +		err = -ENODEV;
> +	}

I'm confused why this is done here - after each section in the firmware
file, rather than having loaded all sections in the firmware file and
only then starting the application. Surely if there's multiple sections
that we're going to load, we want to load _all_ sections before starting
the application?
kernel test robot Dec. 16, 2023, 2:35 p.m. UTC | #5
Hi Tobias,

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/Tobias-Waldekranz/net-phy-marvell10g-Support-firmware-loading-on-88X3310/20231215-041703
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231214201442.660447-2-tobias%40waldekranz.com
patch subject: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310
config: x86_64-randconfig-123-20231216 (https://download.01.org/0day-ci/archive/20231216/202312162238.aJCgm39Y-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312162238.aJCgm39Y-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/202312162238.aJCgm39Y-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/marvell10g.c:620:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] size @@     got restricted __le32 [usertype] @@
   drivers/net/phy/marvell10g.c:620:31: sparse:     expected unsigned int [addressable] [usertype] size
   drivers/net/phy/marvell10g.c:620:31: sparse:     got restricted __le32 [usertype]
>> drivers/net/phy/marvell10g.c:621:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] addr @@     got restricted __le32 [usertype] @@
   drivers/net/phy/marvell10g.c:621:31: sparse:     expected unsigned int [addressable] [usertype] addr
   drivers/net/phy/marvell10g.c:621:31: sparse:     got restricted __le32 [usertype]
>> drivers/net/phy/marvell10g.c:622:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short [addressable] [usertype] csum @@     got restricted __le16 [usertype] @@
   drivers/net/phy/marvell10g.c:622:31: sparse:     expected unsigned short [addressable] [usertype] csum
   drivers/net/phy/marvell10g.c:622:31: sparse:     got restricted __le16 [usertype]
>> drivers/net/phy/marvell10g.c:623:30: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] next_hdr @@     got restricted __le32 [usertype] @@
   drivers/net/phy/marvell10g.c:623:30: sparse:     expected unsigned int [addressable] [usertype] next_hdr
   drivers/net/phy/marvell10g.c:623:30: sparse:     got restricted __le32 [usertype]
   drivers/net/phy/marvell10g.c:624:26: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short [addressable] [usertype] csum @@     got restricted __le16 [usertype] @@
   drivers/net/phy/marvell10g.c:624:26: sparse:     expected unsigned short [addressable] [usertype] csum
   drivers/net/phy/marvell10g.c:624:26: sparse:     got restricted __le16 [usertype]

vim +620 drivers/net/phy/marvell10g.c

   595	
   596	static int mv3310_load_fw(struct phy_device *phydev)
   597	{
   598		const struct mv3310_chip *chip = to_mv3310_chip(phydev);
   599		const struct firmware *fw;
   600		struct mv3310_fw_hdr hdr;
   601		const u8 *sect;
   602		size_t i;
   603		u16 csum;
   604		int err;
   605	
   606		if (!chip->firmware_path)
   607			return -EOPNOTSUPP;
   608	
   609		err = request_firmware(&fw, chip->firmware_path, &phydev->mdio.dev);
   610		if (err)
   611			return err;
   612	
   613		if (fw->size & 1) {
   614			err = -EINVAL;
   615			goto release;
   616		}
   617	
   618		for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
   619			memcpy(&hdr, sect, sizeof(hdr));
 > 620			hdr.data.size = cpu_to_le32(hdr.data.size);
 > 621			hdr.data.addr = cpu_to_le32(hdr.data.addr);
 > 622			hdr.data.csum = cpu_to_le16(hdr.data.csum);
 > 623			hdr.next_hdr = cpu_to_le32(hdr.next_hdr);
   624			hdr.csum = cpu_to_le16(hdr.csum);
   625	
   626			for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
   627				csum += sect[i];
   628	
   629			if ((u16)~csum != hdr.csum) {
   630				dev_err(&phydev->mdio.dev, "Corrupt section header\n");
   631				err = -EINVAL;
   632				break;
   633			}
   634	
   635			err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));
   636			if (err)
   637				break;
   638	
   639			if (!hdr.next_hdr)
   640				break;
   641	
   642			sect = fw->data + hdr.next_hdr;
   643		}
   644	
   645	release:
   646		release_firmware(fw);
   647		return err;
   648	}
   649
Tobias Waldekranz Dec. 18, 2023, 5:11 p.m. UTC | #6
On fre, dec 15, 2023 at 15:30, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
>> When probing, if a device is waiting for firmware to be loaded into
>> its RAM, ask userspace for the binary and load it over XMDIO.
>
> Does a device without firmware have valid ID registers? Is the driver
> going to probe via bus enumeration, or is it necessary to use a
> compatible with ID values?
>
>> +	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
>
> This validates that the firmware is big enough to hold the header...
>
>> +		memcpy(&hdr, sect, sizeof(hdr));
>> +		hdr.data.size = cpu_to_le32(hdr.data.size);
>> +		hdr.data.addr = cpu_to_le32(hdr.data.addr);
>> +		hdr.data.csum = cpu_to_le16(hdr.data.csum);
>> +		hdr.next_hdr = cpu_to_le32(hdr.next_hdr);
>
> I'm surprised sparse is not complaining about this. You have the same
> source and destination, and sparse probably wants the destination to
> be marked as little endian.
>
>> +		hdr.csum = cpu_to_le16(hdr.csum);
>> +
>> +		for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
>> +			csum += sect[i];
>> +
>> +		if ((u16)~csum != hdr.csum) {
>> +			dev_err(&phydev->mdio.dev, "Corrupt section header\n");
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));
>
> What i don't see is any validation that the firmware left at sect +
> sizeof(hdr) big enough to contain hdr.data.size bytes.
>

Thanks Andrew and Russel, for the review!

You both make valid points, I'll try to be less clever about this whole
section in v2.
Tobias Waldekranz Dec. 19, 2023, 10:15 a.m. UTC | #7
On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> On Thu, 14 Dec 2023 21:14:39 +0100
> Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
>> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
>
> And do you have permission to publish this firmware into linux-firmware?

No, I do not.

> Because when we tried this with Marvell, their lawyer guy said we can't
> do that...

I don't even have good enough access to ask the question, much less get
rejected by Marvell :) I just used that path so that it would line up
with linux-firmware if Marvell was to publish it in the future.

Should MODULE_FIRMWARE be avoided for things that are not in
linux-firmware?
Marek Behún Dec. 19, 2023, 10:49 a.m. UTC | #8
On Tue, 19 Dec 2023 11:15:41 +0100
Tobias Waldekranz <tobias@waldekranz.com> wrote:

> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> > On Thu, 14 Dec 2023 21:14:39 +0100
> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
> >  
> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");  
> >
> > And do you have permission to publish this firmware into linux-firmware?  
> 
> No, I do not.
> 
> > Because when we tried this with Marvell, their lawyer guy said we can't
> > do that...  
> 
> I don't even have good enough access to ask the question, much less get
> rejected by Marvell :) I just used that path so that it would line up
> with linux-firmware if Marvell was to publish it in the future.

Yeah, it was pretty stupid in my opinion. The lawyer guy's reasoning
was that to download the firmware from Marvell's Customer Portal you
have to agree with Terms & Conditions, so it can't be distributed to
people who did not agree to Terms & Conditions. We told him that anyone
can get access to the firmware without agreeing anyway, since they can
just read the SPI NOR module connected to the PHY if we burn the
firmware in manufacture...

> Should MODULE_FIRMWARE be avoided for things that are not in
> linux-firmware?

I don't know.
Tobias Waldekranz Dec. 19, 2023, 1:15 p.m. UTC | #9
On tis, dec 19, 2023 at 11:49, Marek Behún <kabel@kernel.org> wrote:
> On Tue, 19 Dec 2023 11:15:41 +0100
> Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
>> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
>> > On Thu, 14 Dec 2023 21:14:39 +0100
>> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
>> >  
>> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");  
>> >
>> > And do you have permission to publish this firmware into linux-firmware?  
>> 
>> No, I do not.
>> 
>> > Because when we tried this with Marvell, their lawyer guy said we can't
>> > do that...  
>> 
>> I don't even have good enough access to ask the question, much less get
>> rejected by Marvell :) I just used that path so that it would line up
>> with linux-firmware if Marvell was to publish it in the future.
>
> Yeah, it was pretty stupid in my opinion. The lawyer guy's reasoning
> was that to download the firmware from Marvell's Customer Portal you
> have to agree with Terms & Conditions, so it can't be distributed to
> people who did not agree to Terms & Conditions. We told him that anyone
> can get access to the firmware without agreeing anyway, since they can
> just read the SPI NOR module connected to the PHY if we burn the
> firmware in manufacture...

Yeah, they are needlessly secretive in lots of ways - much to their own
detriment, IMO. They also protect their functional specs as if you could
just run them through `pdf2rtl`, email the output to TSMC, and have your
own 7nm ASIC in the mail the following week.
Tobias Waldekranz Jan. 2, 2024, 1:09 p.m. UTC | #10
On tis, jan 02, 2024 at 10:12, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote:
>> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
>> > On Thu, 14 Dec 2023 21:14:39 +0100
>> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
>> >
>> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
>> >
>> > And do you have permission to publish this firmware into linux-firmware?
>> 
>> No, I do not.
>> 
>> > Because when we tried this with Marvell, their lawyer guy said we can't
>> > do that...
>> 
>> I don't even have good enough access to ask the question, much less get
>> rejected by Marvell :) I just used that path so that it would line up
>> with linux-firmware if Marvell was to publish it in the future.
>> 
>> Should MODULE_FIRMWARE be avoided for things that are not in
>> linux-firmware?
>
> Without the firmware being published, what use is having this code in
> mainline kernels?

Personally, I primarily want this merged so that future contributions to
the driver are easier to develop, since I'll be able test them on top of
a clean net-next base.

More broadly, I suppose it will help others who are looking to support
similar boards to run the latest kernel, without the need to juggle
external patches which are likely to break as the driver evolves.

Having a single, canonical, implementation of firmware loading, instead
of multiple slightly-broken-in-different-ways ones floating around, also
seems like a net positive.
diff mbox series

Patch

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ad43e280930c..83233b30d7b0 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -25,6 +25,7 @@ 
 #include <linux/bitfield.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
@@ -50,6 +51,13 @@  enum {
 	MV_PMA_21X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH	= 0x6,
 	MV_PMA_BOOT		= 0xc050,
 	MV_PMA_BOOT_FATAL	= BIT(0),
+	MV_PMA_BOOT_PRGS_MASK	= 0x0006,
+	MV_PMA_BOOT_PRGS_INIT	= 0x0000,
+	MV_PMA_BOOT_PRGS_WAIT	= 0x0002,
+	MV_PMA_BOOT_PRGS_CSUM	= 0x0004,
+	MV_PMA_BOOT_PRGS_JRAM	= 0x0006,
+	MV_PMA_BOOT_APP_STARTED	= BIT(4),
+	MV_PMA_BOOT_APP_LOADED	= BIT(6),
 
 	MV_PCS_BASE_T		= 0x0000,
 	MV_PCS_BASE_R		= 0x1000,
@@ -96,6 +104,12 @@  enum {
 	MV_PCS_PORT_INFO_NPORTS_MASK	= 0x0380,
 	MV_PCS_PORT_INFO_NPORTS_SHIFT	= 7,
 
+	/* Firmware downloading */
+	MV_PCS_FW_ADDR_LOW	= 0xd0f0,
+	MV_PCS_FW_ADDR_HIGH	= 0xd0f1,
+	MV_PCS_FW_DATA		= 0xd0f2,
+	MV_PCS_FW_CSUM		= 0xd0f3,
+
 	/* SerDes reinitialization 88E21X0 */
 	MV_AN_21X0_SERDES_CTRL2	= 0x800f,
 	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
@@ -156,6 +170,7 @@  struct mv3310_chip {
 
 	const struct mv3310_mactype *mactypes;
 	size_t n_mactypes;
+	const char *firmware_path;
 
 #ifdef CONFIG_HWMON
 	int (*hwmon_read_temp_reg)(struct phy_device *phydev);
@@ -506,6 +521,132 @@  static const struct sfp_upstream_ops mv3310_sfp_ops = {
 	.module_insert = mv3310_sfp_insert,
 };
 
+struct mv3310_fw_hdr {
+	struct {
+		u32 size;
+		u32 addr;
+		u16 csum;
+	} __packed data;
+
+	u8 flags;
+#define MV3310_FW_HDR_DATA_ONLY BIT(6)
+
+	u8 port_skip;
+	u32 next_hdr;
+	u16 csum;
+
+	u8 pad[14];
+} __packed;
+
+static int mv3310_load_fw_sect(struct phy_device *phydev,
+			       const struct mv3310_fw_hdr *hdr, const u8 *data)
+{
+	int err = 0;
+	size_t i;
+	u16 csum;
+
+	dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n",
+		hdr->data.size,
+		(hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable",
+		hdr->data.addr);
+
+	for (i = 0, csum = 0; i < hdr->data.size; i++)
+		csum += data[i];
+
+	if ((u16)~csum != hdr->data.csum) {
+		dev_err(&phydev->mdio.dev, "Corrupt section data\n");
+		return -EINVAL;
+	}
+
+	phy_lock_mdio_bus(phydev);
+
+	/* Any existing checksum is cleared by a read */
+	__phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
+
+	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW,  hdr->data.addr & 0xffff);
+	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16);
+
+	for (i = 0; i < hdr->data.size; i += 2) {
+		__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA,
+				(data[i + 1] << 8) | data[i]);
+	}
+
+	csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
+	if ((u16)~csum != hdr->data.csum) {
+		dev_err(&phydev->mdio.dev, "Download failed\n");
+		err = -EIO;
+		goto unlock;
+	}
+
+	if (hdr->flags & MV3310_FW_HDR_DATA_ONLY)
+		goto unlock;
+
+	__phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED);
+	mdelay(200);
+	if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) {
+		dev_err(&phydev->mdio.dev, "Application did not startup\n");
+		err = -ENODEV;
+	}
+
+unlock:
+	phy_unlock_mdio_bus(phydev);
+	return err;
+}
+
+static int mv3310_load_fw(struct phy_device *phydev)
+{
+	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
+	const struct firmware *fw;
+	struct mv3310_fw_hdr hdr;
+	const u8 *sect;
+	size_t i;
+	u16 csum;
+	int err;
+
+	if (!chip->firmware_path)
+		return -EOPNOTSUPP;
+
+	err = request_firmware(&fw, chip->firmware_path, &phydev->mdio.dev);
+	if (err)
+		return err;
+
+	if (fw->size & 1) {
+		err = -EINVAL;
+		goto release;
+	}
+
+	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
+		memcpy(&hdr, sect, sizeof(hdr));
+		hdr.data.size = cpu_to_le32(hdr.data.size);
+		hdr.data.addr = cpu_to_le32(hdr.data.addr);
+		hdr.data.csum = cpu_to_le16(hdr.data.csum);
+		hdr.next_hdr = cpu_to_le32(hdr.next_hdr);
+		hdr.csum = cpu_to_le16(hdr.csum);
+
+		for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
+			csum += sect[i];
+
+		if ((u16)~csum != hdr.csum) {
+			dev_err(&phydev->mdio.dev, "Corrupt section header\n");
+			err = -EINVAL;
+			break;
+		}
+
+		err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));
+		if (err)
+			break;
+
+		if (!hdr.next_hdr)
+			break;
+
+		sect = fw->data + hdr.next_hdr;
+	}
+
+release:
+	release_firmware(fw);
+	return err;
+}
+
 static int mv3310_probe(struct phy_device *phydev)
 {
 	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
@@ -527,6 +668,12 @@  static int mv3310_probe(struct phy_device *phydev)
 		return -ENODEV;
 	}
 
+	if ((ret & MV_PMA_BOOT_PRGS_MASK) == MV_PMA_BOOT_PRGS_WAIT) {
+		ret = mv3310_load_fw(phydev);
+		if (ret)
+			return ret;
+	}
+
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -1219,6 +1366,7 @@  static const struct mv3310_chip mv3310_type = {
 
 	.mactypes = mv3310_mactypes,
 	.n_mactypes = ARRAY_SIZE(mv3310_mactypes),
+	.firmware_path = "mrvl/x3310fw.hdr",
 
 #ifdef CONFIG_HWMON
 	.hwmon_read_temp_reg = mv3310_hwmon_read_temp_reg,
@@ -1489,4 +1637,5 @@  static struct mdio_device_id __maybe_unused mv3310_tbl[] = {
 };
 MODULE_DEVICE_TABLE(mdio, mv3310_tbl);
 MODULE_DESCRIPTION("Marvell Alaska X/M multi-gigabit Ethernet PHY driver");
+MODULE_FIRMWARE("mrvl/x3310fw.hdr");
 MODULE_LICENSE("GPL");