diff mbox series

[RFC,net-next,06/19] pds_core: add FW update feature to devlink

Message ID 20221118225656.48309-7-snelson@pensando.io (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series pds core and vdpa drivers | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count fail Series longer than 15 patches (and no cover letter)
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: pabeni@redhat.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Shannon Nelson Nov. 18, 2022, 10:56 p.m. UTC
Add in the support for doing firmware updates, and for selecting
the next firmware image to boot on, and tie them into the
devlink flash and parameter handling.  The FW flash is the same
as in the ionic driver.  However, this device has the ability
to report what is in the firmware slots on the device and
allows you to select the slot to use on the next device boot.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 .../net/ethernet/pensando/pds_core/Makefile   |   3 +-
 drivers/net/ethernet/pensando/pds_core/core.h |   8 +
 .../net/ethernet/pensando/pds_core/devlink.c  | 103 ++++++++++
 drivers/net/ethernet/pensando/pds_core/fw.c   | 192 ++++++++++++++++++
 4 files changed, 305 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/pensando/pds_core/fw.c

Comments

Jakub Kicinski Nov. 28, 2022, 6:27 p.m. UTC | #1
On Fri, 18 Nov 2022 14:56:43 -0800 Shannon Nelson wrote:
> Add in the support for doing firmware updates, and for selecting
> the next firmware image to boot on, and tie them into the
> devlink flash and parameter handling.  The FW flash is the same
> as in the ionic driver.  However, this device has the ability
> to report what is in the firmware slots on the device and
> allows you to select the slot to use on the next device boot.

This is hardly vendor specific. Intel does a similar thing, IIUC.
Please work on a common interface.
Shannon Nelson Nov. 28, 2022, 10:25 p.m. UTC | #2
On 11/28/22 10:27 AM, Jakub Kicinski wrote:
> On Fri, 18 Nov 2022 14:56:43 -0800 Shannon Nelson wrote:
>> Add in the support for doing firmware updates, and for selecting
>> the next firmware image to boot on, and tie them into the
>> devlink flash and parameter handling.  The FW flash is the same
>> as in the ionic driver.  However, this device has the ability
>> to report what is in the firmware slots on the device and
>> allows you to select the slot to use on the next device boot.
> 
> This is hardly vendor specific. Intel does a similar thing, IIUC.
> Please work on a common interface.

I don't think Intel selects which FW image to boot, but it looks like 
mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between 
DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items to 
the enum devlink_param_fw_load_policy_value and use this API?  For example:

	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,

I could then modify the devlink dev info printed to refer to fw_slot_0, 
fw.slot_1, and fw.slot_2 instead of our vendor specific names.

Cheers,
sln
Jakub Kicinski Nov. 28, 2022, 11:33 p.m. UTC | #3
On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> I don't think Intel selects which FW image to boot, but it looks like 
> mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between 
> DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items to 
> the enum devlink_param_fw_load_policy_value and use this API?  For example:
> 
> 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,

Not the worst idea, although I presume normal FW flashing should switch
between slots to activate the new image by default? Which means the
action of fw flashing would alter the policy set by the user. A little
awkward from an API purist standpoint.

I'd just expose the active "bank" via netlink directly.

> I could then modify the devlink dev info printed to refer to fw_slot_0, 
> fw.slot_1, and fw.slot_2 instead of our vendor specific names.

Jake, didn't you have a similar capability in ice?

Knowing my memory I may have acquiesced to something in another driver
already. That said - I think it's cleaner if we just list the stored
versions per bank, no?
Shannon Nelson Nov. 28, 2022, 11:45 p.m. UTC | #4
On 11/28/22 3:33 PM, Jakub Kicinski wrote:
> On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
>> I don't think Intel selects which FW image to boot, but it looks like
>> mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
>> DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items to
>> the enum devlink_param_fw_load_policy_value and use this API?  For example:
>>
>>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
>>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
>>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
>>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
> 
> Not the worst idea, although I presume normal FW flashing should switch
> between slots to activate the new image by default? Which means the
> action of fw flashing would alter the policy set by the user. A little
> awkward from an API purist standpoint.

Yes, the action of flashing will set the new bank/slot to use on the 
next boot.  However, we have the ability to select from multiple valid 
images and we want to pass this flexibility to the user rather than 
force them to go through a whole flash sequence just to get to the other 
bank.

> 
> I'd just expose the active "bank" via netlink directly.
> 
>> I could then modify the devlink dev info printed to refer to fw_slot_0,
>> fw.slot_1, and fw.slot_2 instead of our vendor specific names.
> 
> Jake, didn't you have a similar capability in ice?
> 
> Knowing my memory I may have acquiesced to something in another driver
> already. That said - I think it's cleaner if we just list the stored
> versions per bank, no?

We are listing the stored images in the devlink dev info output, just 
want to let the user choose which of those valid images to use next.

Cheers,
sln
Jacob Keller Nov. 29, 2022, 12:13 a.m. UTC | #5
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, November 28, 2022 3:33 PM
> To: Shannon Nelson <shnelson@amd.com>
> Cc: Shannon Nelson <snelson@pensando.io>; netdev@vger.kernel.org;
> davem@davemloft.net; mst@redhat.com; jasowang@redhat.com;
> virtualization@lists.linux-foundation.org; drivers@pensando.io; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
> devlink
> 
> On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> > I don't think Intel selects which FW image to boot, but it looks like
> > mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
> > DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items to
> > the enum devlink_param_fw_load_policy_value and use this API?  For example:
> >
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
> 
> Not the worst idea, although I presume normal FW flashing should switch
> between slots to activate the new image by default? Which means the
> action of fw flashing would alter the policy set by the user. A little
> awkward from an API purist standpoint.
> 
> I'd just expose the active "bank" via netlink directly.
> 
> > I could then modify the devlink dev info printed to refer to fw_slot_0,
> > fw.slot_1, and fw.slot_2 instead of our vendor specific names.
> 
> Jake, didn't you have a similar capability in ice?
> 

We have two banks of flash, the active bank, and an inactive bank used for updates. We can determine the active bank from the Shadow RAM contents which are generated as the EMP firmware boots up.

> Knowing my memory I may have acquiesced to something in another driver
> already. That said - I think it's cleaner if we just list the stored
> versions per bank, no?

I think it would make sense to store them per bank and make the bank number some index instead of something separate as like this DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_<X> where each <X> makes a separate parameter.

Currently devlink info reports "stored" and "active", which aligns with our current use of the active vs inactive flash bank. We could be explicit and indicate which bank it is, though its a bit tricky since most of the firmware interface deals with it in terms of "active" and "inactive" rather than the absolute position of "bank 0 or bank 1".

Especially if another device has more than 2 banks I think its a good extension to devlink info, and we could probably get away with something like a new info attribute that specifies which bank index it is, and an attribute which indicates whether its active or not.

Thanks,
Jake
Jacob Keller Nov. 29, 2022, 12:18 a.m. UTC | #6
> -----Original Message-----
> From: Shannon Nelson <shnelson@amd.com>
> Sent: Monday, November 28, 2022 3:46 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Shannon Nelson <snelson@pensando.io>; netdev@vger.kernel.org;
> davem@davemloft.net; mst@redhat.com; jasowang@redhat.com;
> virtualization@lists.linux-foundation.org; drivers@pensando.io; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
> devlink
> 
> On 11/28/22 3:33 PM, Jakub Kicinski wrote:
> > On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> >> I don't think Intel selects which FW image to boot, but it looks like
> >> mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
> >> DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items to
> >> the enum devlink_param_fw_load_policy_value and use this API?  For
> example:
> >>
> >>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> >>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> >>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> >>        DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
> >
> > Not the worst idea, although I presume normal FW flashing should switch
> > between slots to activate the new image by default? Which means the
> > action of fw flashing would alter the policy set by the user. A little
> > awkward from an API purist standpoint.

This could potentially be handled by having DELVINK_PARAM_FW_LOAD_POLICY_FLASH be the automatic "select best version", and if a user has set a manual value then don't allow flashing until a reboot or the value is set back to POLICY_FLASH?

> 
> Yes, the action of flashing will set the new bank/slot to use on the
> next boot.  However, we have the ability to select from multiple valid
> images and we want to pass this flexibility to the user rather than
> force them to go through a whole flash sequence just to get to the other
> bank.
> 
> >
> > I'd just expose the active "bank" via netlink directly.
> >
> >> I could then modify the devlink dev info printed to refer to fw_slot_0,
> >> fw.slot_1, and fw.slot_2 instead of our vendor specific names.
> >
> > Jake, didn't you have a similar capability in ice?
> >
> > Knowing my memory I may have acquiesced to something in another driver
> > already. That said - I think it's cleaner if we just list the stored
> > versions per bank, no?
> 
> We are listing the stored images in the devlink dev info output, just
> want to let the user choose which of those valid images to use next.
> 
> Cheers,
> sln

Technically I think we could do something similar in ice to switch between the banks, at least as long as there is a valid image in the bank. The big trick is that I am not sure we can verify ahead of time whether we have a valid image and if you happen to boot into an invalid or blank image. There is some recovery firmware that should activate in that case, but I think our current driver doesn't implement enough of a recovery mode to actually handle this case to allow user to switch back.

Still, I think the ability to select the bank is valuable, and finding the right way to expose it is good.

Thanks,
Jake
diff mbox series

Patch

diff --git a/drivers/net/ethernet/pensando/pds_core/Makefile b/drivers/net/ethernet/pensando/pds_core/Makefile
index c7a722f7d9b8..06bd3da8c38b 100644
--- a/drivers/net/ethernet/pensando/pds_core/Makefile
+++ b/drivers/net/ethernet/pensando/pds_core/Makefile
@@ -7,6 +7,7 @@  pds_core-y := main.o \
 	      devlink.o \
 	      dev.o \
 	      adminq.o \
-	      core.o
+	      core.o \
+	      fw.o
 
 pds_core-$(CONFIG_DEBUG_FS) += debugfs.o
diff --git a/drivers/net/ethernet/pensando/pds_core/core.h b/drivers/net/ethernet/pensando/pds_core/core.h
index 87b221aa7b44..687e1debd079 100644
--- a/drivers/net/ethernet/pensando/pds_core/core.h
+++ b/drivers/net/ethernet/pensando/pds_core/core.h
@@ -123,6 +123,11 @@  struct pdsc_qcq {
 	struct dentry *dentry;
 };
 
+enum pdsc_devlink_param_id {
+	PDSC_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	PDSC_DEVLINK_PARAM_ID_FW_BOOT,
+};
+
 /* No state flags set means we are in a steady running state */
 enum pdsc_state_flags {
 	PDSC_S_FW_DEAD,		    /* fw stopped, waiting for startup or recovery */
@@ -287,4 +292,7 @@  void pdsc_process_adminq(struct pdsc_qcq *qcq);
 void pdsc_work_thread(struct work_struct *work);
 irqreturn_t pdsc_adminq_isr(int irq, void *data);
 
+int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
+			 struct netlink_ext_ack *extack);
+
 #endif /* _PDSC_H_ */
diff --git a/drivers/net/ethernet/pensando/pds_core/devlink.c b/drivers/net/ethernet/pensando/pds_core/devlink.c
index 42cf17229ace..0568e8b7391c 100644
--- a/drivers/net/ethernet/pensando/pds_core/devlink.c
+++ b/drivers/net/ethernet/pensando/pds_core/devlink.c
@@ -8,6 +8,100 @@ 
 
 #include "core.h"
 
+static char *slot_labels[] = { "fw.gold", "fw.mainfwa", "fw.mainfwb" };
+
+static int pdsc_dl_fw_boot_get(struct devlink *dl, u32 id,
+			       struct devlink_param_gset_ctx *ctx)
+{
+	struct pdsc *pdsc = devlink_priv(dl);
+	union pds_core_dev_cmd cmd = {
+		.fw_control.opcode = PDS_CORE_CMD_FW_CONTROL,
+		.fw_control.oper = PDS_CORE_FW_GET_BOOT,
+	};
+	union pds_core_dev_comp comp;
+	int err;
+
+	err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
+	if (err) {
+		if (err == -EIO) {
+			snprintf(ctx->val.vstr, sizeof(ctx->val.vstr), "(unknown)");
+			return 0;
+		} else {
+			return err;
+		}
+	}
+
+	if (comp.fw_control.slot >= ARRAY_SIZE(slot_labels))
+		snprintf(ctx->val.vstr, sizeof(ctx->val.vstr),
+			 "fw.slot%02d", comp.fw_control.slot);
+	else
+		snprintf(ctx->val.vstr, sizeof(ctx->val.vstr),
+			 "%s", slot_labels[comp.fw_control.slot]);
+
+	return 0;
+}
+
+static int pdsc_dl_fw_boot_set(struct devlink *dl, u32 id,
+			       struct devlink_param_gset_ctx *ctx)
+{
+	struct pdsc *pdsc = devlink_priv(dl);
+	union pds_core_dev_cmd cmd = {
+		.fw_control.opcode = PDS_CORE_CMD_FW_CONTROL,
+		.fw_control.oper = PDS_CORE_FW_SET_BOOT,
+	};
+	union pds_core_dev_comp comp;
+	enum pds_core_fw_slot slot;
+	int timeout;
+
+	for (slot = 0; slot < ARRAY_SIZE(slot_labels); slot++)
+		if (!strcmp(ctx->val.vstr, slot_labels[slot]))
+			break;
+
+	if (slot >= ARRAY_SIZE(slot_labels))
+		return -EINVAL;
+
+	cmd.fw_control.slot = slot;
+
+	/* This is known to be a longer running command, so be sure
+	 * to use a larger timeout on the command than usual
+	 */
+#define PDSC_SET_BOOT_TIMEOUT	10
+	timeout = max_t(int, PDSC_SET_BOOT_TIMEOUT, pdsc->devcmd_timeout);
+	return pdsc_devcmd(pdsc, &cmd, &comp, timeout);
+}
+
+static int pdsc_dl_fw_boot_validate(struct devlink *dl, u32 id,
+				    union devlink_param_value val,
+				    struct netlink_ext_ack *extack)
+{
+	enum pds_core_fw_slot slot;
+
+	for (slot = 0; slot < ARRAY_SIZE(slot_labels); slot++)
+		if (!strcmp(val.vstr, slot_labels[slot]))
+			return 0;
+
+	return -EINVAL;
+}
+
+static const struct devlink_param pdsc_dl_params[] = {
+	DEVLINK_PARAM_DRIVER(PDSC_DEVLINK_PARAM_ID_FW_BOOT,
+			     "boot_fw",
+			     DEVLINK_PARAM_TYPE_STRING,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     pdsc_dl_fw_boot_get,
+			     pdsc_dl_fw_boot_set,
+			     pdsc_dl_fw_boot_validate),
+};
+
+static int pdsc_dl_flash_update(struct devlink *dl,
+				struct devlink_flash_update_params *params,
+				struct netlink_ext_ack *extack)
+{
+	struct pdsc *pdsc = devlink_priv(dl);
+
+	return pdsc_firmware_update(pdsc, params->fw, extack);
+}
+
 static int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 			    struct netlink_ext_ack *extack)
 {
@@ -71,6 +165,7 @@  static int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 
 static const struct devlink_ops pdsc_dl_ops = {
 	.info_get	= pdsc_dl_info_get,
+	.flash_update	= pdsc_dl_flash_update,
 };
 
 struct pdsc *pdsc_dl_alloc(struct device *dev)
@@ -94,6 +189,12 @@  void pdsc_dl_free(struct pdsc *pdsc)
 int pdsc_dl_register(struct pdsc *pdsc)
 {
 	struct devlink *dl = priv_to_devlink(pdsc);
+	int err;
+
+	err = devlink_params_register(dl, pdsc_dl_params,
+				      ARRAY_SIZE(pdsc_dl_params));
+	if (err)
+		return err;
 
 	devlink_register(dl);
 
@@ -105,4 +206,6 @@  void pdsc_dl_unregister(struct pdsc *pdsc)
 	struct devlink *dl = priv_to_devlink(pdsc);
 
 	devlink_unregister(dl);
+	devlink_params_unregister(dl, pdsc_dl_params,
+				  ARRAY_SIZE(pdsc_dl_params));
 }
diff --git a/drivers/net/ethernet/pensando/pds_core/fw.c b/drivers/net/ethernet/pensando/pds_core/fw.c
new file mode 100644
index 000000000000..3c64deef5549
--- /dev/null
+++ b/drivers/net/ethernet/pensando/pds_core/fw.c
@@ -0,0 +1,192 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2022 Pensando Systems, Inc */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/firmware.h>
+
+#include "core.h"
+
+/* The worst case wait for the install activity is about 25 minutes when
+ * installing a new CPLD, which is very seldom.  Normal is about 30-35
+ * seconds.  Since the driver can't tell if a CPLD update will happen we
+ * set the timeout for the ugly case.
+ */
+#define PDSC_FW_INSTALL_TIMEOUT	(25 * 60)
+#define PDSC_FW_SELECT_TIMEOUT	30
+
+/* Number of periodic log updates during fw file download */
+#define PDSC_FW_INTERVAL_FRACTION	32
+
+static int pdsc_devcmd_firmware_download(struct pdsc *pdsc, u64 addr,
+					 u32 offset, u32 length)
+{
+	union pds_core_dev_cmd cmd = {
+		.fw_download.opcode = PDS_CORE_CMD_FW_DOWNLOAD,
+		.fw_download.offset = cpu_to_le32(offset),
+		.fw_download.addr = cpu_to_le64(addr),
+		.fw_download.length = cpu_to_le32(length),
+	};
+	union pds_core_dev_comp comp;
+
+	return pdsc_devcmd_locked(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
+}
+
+static int pdsc_devcmd_firmware_install(struct pdsc *pdsc)
+{
+	union pds_core_dev_cmd cmd = {
+		.fw_control.opcode = PDS_CORE_CMD_FW_CONTROL,
+		.fw_control.oper = PDS_CORE_FW_INSTALL_ASYNC
+	};
+	union pds_core_dev_comp comp;
+	int err;
+
+	err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
+	if (err < 0)
+		return err;
+
+	return comp.fw_control.slot;
+}
+
+static int pdsc_devcmd_firmware_activate(struct pdsc *pdsc,
+					 enum pds_core_fw_slot slot)
+{
+	union pds_core_dev_cmd cmd = {
+		.fw_control.opcode = PDS_CORE_CMD_FW_CONTROL,
+		.fw_control.oper = PDS_CORE_FW_ACTIVATE_ASYNC,
+		.fw_control.slot = slot
+	};
+	union pds_core_dev_comp comp;
+
+	return pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
+}
+
+static int pdsc_fw_status_long_wait(struct pdsc *pdsc,
+				    const char *label,
+				    unsigned long timeout,
+				    u8 fw_cmd,
+				    struct netlink_ext_ack *extack)
+{
+	union pds_core_dev_cmd cmd = {
+		.fw_control.opcode = PDS_CORE_CMD_FW_CONTROL,
+		.fw_control.oper = fw_cmd,
+	};
+	union pds_core_dev_comp comp;
+	unsigned long start_time;
+	unsigned long end_time;
+	int err;
+
+	/* Ping on the status of the long running async install
+	 * command.  We get EAGAIN while the command is still
+	 * running, else we get the final command status.
+	 */
+	start_time = jiffies;
+	end_time = start_time + (timeout * HZ);
+	do {
+		err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
+		msleep(20);
+	} while (time_before(jiffies, end_time) &&
+		 (err == -EAGAIN || err == -ETIMEDOUT));
+
+	if (err == -EAGAIN || err == -ETIMEDOUT) {
+		NL_SET_ERR_MSG_MOD(extack, "Firmware wait timed out");
+		dev_err(pdsc->dev, "DEV_CMD firmware wait %s timed out\n", label);
+	} else if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Firmware wait failed");
+	}
+
+	return err;
+}
+
+int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
+			 struct netlink_ext_ack *extack)
+{
+	u32 buf_sz, copy_sz, offset;
+	struct devlink *dl;
+	int next_interval;
+	u64 data_addr;
+	int err = 0;
+	u8 fw_slot;
+
+	dev_info(pdsc->dev, "Installing firmware\n");
+
+	dl = priv_to_devlink(pdsc);
+	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
+
+	buf_sz = sizeof(pdsc->cmd_regs->data);
+
+	dev_dbg(pdsc->dev,
+		"downloading firmware - size %d part_sz %d nparts %lu\n",
+		(int)fw->size, buf_sz, DIV_ROUND_UP(fw->size, buf_sz));
+
+	offset = 0;
+	next_interval = 0;
+	data_addr = offsetof(struct pds_core_dev_cmd_regs, data);
+	while (offset < fw->size) {
+		if (offset >= next_interval) {
+			devlink_flash_update_status_notify(dl, "Downloading", NULL,
+							   offset, fw->size);
+			next_interval = offset + (fw->size / PDSC_FW_INTERVAL_FRACTION);
+		}
+
+		copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
+		mutex_lock(&pdsc->devcmd_lock);
+		memcpy_toio(&pdsc->cmd_regs->data, fw->data + offset, copy_sz);
+		err = pdsc_devcmd_firmware_download(pdsc, data_addr, offset, copy_sz);
+		mutex_unlock(&pdsc->devcmd_lock);
+		if (err) {
+			dev_err(pdsc->dev,
+				"download failed offset 0x%x addr 0x%llx len 0x%x: %pe\n",
+				offset, data_addr, copy_sz, ERR_PTR(err));
+			NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
+			goto err_out;
+		}
+		offset += copy_sz;
+	}
+	devlink_flash_update_status_notify(dl, "Downloading", NULL,
+					   fw->size, fw->size);
+
+	devlink_flash_update_timeout_notify(dl, "Installing", NULL,
+					    PDSC_FW_INSTALL_TIMEOUT);
+
+	fw_slot = pdsc_devcmd_firmware_install(pdsc);
+	if (fw_slot < 0) {
+		err = fw_slot;
+		dev_err(pdsc->dev, "install failed: %pe\n", ERR_PTR(err));
+		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware install");
+		goto err_out;
+	}
+
+	err = pdsc_fw_status_long_wait(pdsc, "Installing",
+				       PDSC_FW_INSTALL_TIMEOUT,
+				       PDS_CORE_FW_INSTALL_STATUS,
+				       extack);
+	if (err)
+		goto err_out;
+
+	devlink_flash_update_timeout_notify(dl, "Selecting", NULL,
+					    PDSC_FW_SELECT_TIMEOUT);
+
+	err = pdsc_devcmd_firmware_activate(pdsc, fw_slot);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware select");
+		goto err_out;
+	}
+
+	err = pdsc_fw_status_long_wait(pdsc, "Selecting",
+				       PDSC_FW_SELECT_TIMEOUT,
+				       PDS_CORE_FW_ACTIVATE_STATUS,
+				       extack);
+	if (err)
+		goto err_out;
+
+	dev_info(pdsc->dev, "Firmware update completed, slot %d\n", fw_slot);
+
+err_out:
+	if (err)
+		devlink_flash_update_status_notify(dl, "Flash failed", NULL, 0, 0);
+	else
+		devlink_flash_update_status_notify(dl, "Flash done", NULL, 0, 0);
+	return err;
+}