Use the introduced functionality to read firmware files and flash their
contents into the device's internal flash memory to implement the devlink
flash update callback.

Sample output on EDS2 development board:
 # devlink -j dev info i2c/1-0070 | jq '.[][]["versions"]["running"]'
 {
   "fw": "6026"
 }
 # devlink dev flash i2c/1-0070 file firmware_fw2.hex
 [utility] Prepare flash mode
 [utility] Downloading image 100%
 [utility] Flash mode enabled
 [firmware1-part1] Downloading image 100%
 [firmware1-part1] Flashing image
 [firmware1-part2] Downloading image 100%
 [firmware1-part2] Flashing image
 [firmware1] Flashing done
 [firmware2] Downloading image 100%
 [firmware2] Flashing image 100%
 [firmware2] Flashing done
 [utility] Leaving flash mode
 Flashing done
 # devlink -j dev info i2c/1-0070 | jq '.[][]["versions"]["running"]'
 {
   "fw": "7006"
 }

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v3:
* Fixed return value documentation for zl3073x_flash_update()
---
 Documentation/networking/devlink/zl3073x.rst | 14 +++++
 drivers/dpll/zl3073x/devlink.c               | 65 ++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst
index 4b6cfaf386433..fc5a8dc272a77 100644
--- a/Documentation/networking/devlink/zl3073x.rst
+++ b/Documentation/networking/devlink/zl3073x.rst
@@ -49,3 +49,17 @@ The ``zl3073x`` driver reports the following versions
       - running
       - 1.3.0.1
       - Device configuration version customized by OEM
+
+Flash Update
+============
+
+The ``zl3073x`` driver implements support for flash update using the
+``devlink-flash`` interface. It supports updating the device flash using a
+combined flash image ("bundle") that contains multiple components (firmware
+parts and configurations).
+
+During the flash procedure, the standard firmware interface is not available,
+so the driver unregisters all DPLLs and associated pins, and re-registers them
+once the flash procedure is complete.
+
+The driver does not support any overwrite mask flags.
diff --git a/drivers/dpll/zl3073x/devlink.c b/drivers/dpll/zl3073x/devlink.c
index d0f6d9cd4a68e..96fca97446bc7 100644
--- a/drivers/dpll/zl3073x/devlink.c
+++ b/drivers/dpll/zl3073x/devlink.c
@@ -9,6 +9,8 @@
 #include "core.h"
 #include "devlink.h"
 #include "dpll.h"
+#include "flash.h"
+#include "fw.h"
 #include "regs.h"
 
 /**
@@ -141,11 +143,74 @@ void zl3073x_devlink_flash_notify(struct zl3073x_dev *zldev, const char *msg,
 					   total);
 }
 
+/**
+ * zl3073x_flash_update - Devlink flash update callback
+ * @devlink: devlink structure pointer
+ * @params: flashing parameters pointer
+ * @extack: netlink extack pointer to report errors
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_devlink_flash_update(struct devlink *devlink,
+			     struct devlink_flash_update_params *params,
+			     struct netlink_ext_ack *extack)
+{
+	struct zl3073x_dev *zldev = devlink_priv(devlink);
+	struct zl3073x_fw_component *util;
+	struct zl3073x_fw *zlfw;
+	int rc = 0;
+
+	/* Load firmware */
+	zlfw = zl3073x_fw_load(zldev, params->fw->data, params->fw->size,
+			       extack);
+	if (IS_ERR(zlfw))
+		return PTR_ERR(zlfw);
+
+	util = zlfw->component[ZL_FW_COMPONENT_UTIL];
+	if (!util) {
+		zl3073x_devlink_flash_notify(zldev,
+					     "Utility is missing in firmware",
+					     NULL, 0, 0);
+		rc = -EOPNOTSUPP;
+		goto error;
+	}
+
+	/* Stop normal operation during flash */
+	zl3073x_dev_stop(zldev);
+
+	/* Enter flashing mode */
+	rc = zl3073x_flash_mode_enter(zldev, util->data, util->size, extack);
+	if (!rc) {
+		/* Flash the firmware */
+		rc = zl3073x_fw_flash(zldev, zlfw, extack);
+
+		/* Leave flashing mode */
+		zl3073x_flash_mode_leave(zldev, extack);
+	}
+
+	/* Restart normal operation */
+	rc = zl3073x_dev_start(zldev, true);
+	if (rc)
+		dev_warn(zldev->dev, "Failed to re-start normal operation\n");
+
+error:
+	/* Free flash context */
+	zl3073x_fw_free(zlfw);
+
+	zl3073x_devlink_flash_notify(zldev,
+				     rc ? "Flashing failed" : "Flashing done",
+				     NULL, 0, 0);
+
+	return rc;
+}
+
 static const struct devlink_ops zl3073x_devlink_ops = {
 	.info_get = zl3073x_devlink_info_get,
 	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
 	.reload_down = zl3073x_devlink_reload_down,
 	.reload_up = zl3073x_devlink_reload_up,
+	.flash_update = zl3073x_devlink_flash_update,
 };
 
 static void
-- 
2.49.1

On Wed, 13 Aug 2025 19:44:08 +0200 Ivan Vecera wrote:
> +	struct zl3073x_dev *zldev = devlink_priv(devlink);
> +	struct zl3073x_fw_component *util;
> +	struct zl3073x_fw *zlfw;
> +	int rc = 0;
> +
> +	/* Load firmware */

Please drop the comments which more or less repeat the name 
of the function called.

> +	zlfw = zl3073x_fw_load(zldev, params->fw->data, params->fw->size,
> +			       extack);
> +	if (IS_ERR(zlfw))
> +		return PTR_ERR(zlfw);
> +
> +	util = zlfw->component[ZL_FW_COMPONENT_UTIL];
> +	if (!util) {
> +		zl3073x_devlink_flash_notify(zldev,
> +					     "Utility is missing in firmware",
> +					     NULL, 0, 0);
> +		rc = -EOPNOTSUPP;

I'd think -EINVAL would be more appropriate.
If you want to be fancy maybe ENOEXEC ?

> +		goto error;
> +	}
> +
> +	/* Stop normal operation during flash */
> +	zl3073x_dev_stop(zldev);
> +
> +	/* Enter flashing mode */
> +	rc = zl3073x_flash_mode_enter(zldev, util->data, util->size, extack);
> +	if (!rc) {
> +		/* Flash the firmware */
> +		rc = zl3073x_fw_flash(zldev, zlfw, extack);

this error code seems to be completely ignored, no?

> +		/* Leave flashing mode */
> +		zl3073x_flash_mode_leave(zldev, extack);
> +	}
> +
> +	/* Restart normal operation */
> +	rc = zl3073x_dev_start(zldev, true);
> +	if (rc)
> +		dev_warn(zldev->dev, "Failed to re-start normal operation\n");

And also we can't really cleanly handle the failure case.

This is why I was speculating about implementing the down/up portion
in the devlink core. Add a flag that the driver requires reload_down
to be called before the flashing operation, and reload_up after.
This way not only core handles some of the error handling, but also
it can mark the device as reload_failed if things go sideways, which 
is a nicer way to surface this sort of permanent error state.

Not feeling strongly about it, but I think it'd be cleaner, so bringing
it up in case my previous comment from a while back wasn't clear.

> +error:
> +	/* Free flash context */
> +	zl3073x_fw_free(zlfw);
> +
> +	zl3073x_devlink_flash_notify(zldev,
> +				     rc ? "Flashing failed" : "Flashing done",
> +				     NULL, 0, 0);

Hi Jakub,

On 19. 08. 25 4:29 dop., Jakub Kicinski wrote:
> On Wed, 13 Aug 2025 19:44:08 +0200 Ivan Vecera wrote:
>> +	struct zl3073x_dev *zldev = devlink_priv(devlink);
>> +	struct zl3073x_fw_component *util;
>> +	struct zl3073x_fw *zlfw;
>> +	int rc = 0;
>> +
>> +	/* Load firmware */
> 
> Please drop the comments which more or less repeat the name
> of the function called.

Will do.

>> +	zlfw = zl3073x_fw_load(zldev, params->fw->data, params->fw->size,
>> +			       extack);
>> +	if (IS_ERR(zlfw))
>> +		return PTR_ERR(zlfw);
>> +
>> +	util = zlfw->component[ZL_FW_COMPONENT_UTIL];
>> +	if (!util) {
>> +		zl3073x_devlink_flash_notify(zldev,
>> +					     "Utility is missing in firmware",
>> +					     NULL, 0, 0);
>> +		rc = -EOPNOTSUPP;
> 
> I'd think -EINVAL would be more appropriate.
> If you want to be fancy maybe ENOEXEC ?

OK, will use -ENOEXEC.

>> +		goto error;
>> +	}
>> +
>> +	/* Stop normal operation during flash */
>> +	zl3073x_dev_stop(zldev);
>> +
>> +	/* Enter flashing mode */
>> +	rc = zl3073x_flash_mode_enter(zldev, util->data, util->size, extack);
>> +	if (!rc) {
>> +		/* Flash the firmware */
>> +		rc = zl3073x_fw_flash(zldev, zlfw, extack);
> 
> this error code seems to be completely ignored, no?

Yep, you are right, this should be propagated to the caller.

>> +		/* Leave flashing mode */
>> +		zl3073x_flash_mode_leave(zldev, extack);
>> +	}
>> +
>> +	/* Restart normal operation */
>> +	rc = zl3073x_dev_start(zldev, true);
>> +	if (rc)
>> +		dev_warn(zldev->dev, "Failed to re-start normal operation\n");
> 
> And also we can't really cleanly handle the failure case.
> 
> This is why I was speculating about implementing the down/up portion
> in the devlink core. Add a flag that the driver requires reload_down
> to be called before the flashing operation, and reload_up after.
> This way not only core handles some of the error handling, but also
> it can mark the device as reload_failed if things go sideways, which
> is a nicer way to surface this sort of permanent error state.

This makes sense... The question is if this should reuse existing
.reload_down and .reload_up callbacks let's say with new devlink action
DEVLINK_RELOAD_ACTION_FW_UPDATE or rather introduce new callbacks
.flash_update_down/_up() to avoid confusions.

Thanks,
Ivan

On Fri, 29 Aug 2025 16:49:22 +0200 Ivan Vecera wrote:
> >> +		/* Leave flashing mode */
> >> +		zl3073x_flash_mode_leave(zldev, extack);
> >> +	}
> >> +
> >> +	/* Restart normal operation */
> >> +	rc = zl3073x_dev_start(zldev, true);
> >> +	if (rc)
> >> +		dev_warn(zldev->dev, "Failed to re-start normal operation\n");  
> > 
> > And also we can't really cleanly handle the failure case.
> > 
> > This is why I was speculating about implementing the down/up portion
> > in the devlink core. Add a flag that the driver requires reload_down
> > to be called before the flashing operation, and reload_up after.
> > This way not only core handles some of the error handling, but also
> > it can mark the device as reload_failed if things go sideways, which
> > is a nicer way to surface this sort of permanent error state.  
> 
> This makes sense... The question is if this should reuse existing
> .reload_down and .reload_up callbacks let's say with new devlink action
> DEVLINK_RELOAD_ACTION_FW_UPDATE or rather introduce new callbacks
> .flash_update_down/_up() to avoid confusions.

Whatever makes sense for your driver, for now. I'm assuming both ops
are the same, otherwise you wouldn't be asking? It should be trivial
for someone add the extra ops later, and just hook them both up to the
same functions in existing drivers.

Hi Kuba and Jiri,

On 30. 08. 25 1:56 dop., Jakub Kicinski wrote:
> On Fri, 29 Aug 2025 16:49:22 +0200 Ivan Vecera wrote:
>>>> +		/* Leave flashing mode */
>>>> +		zl3073x_flash_mode_leave(zldev, extack);
>>>> +	}
>>>> +
>>>> +	/* Restart normal operation */
>>>> +	rc = zl3073x_dev_start(zldev, true);
>>>> +	if (rc)
>>>> +		dev_warn(zldev->dev, "Failed to re-start normal operation\n");
>>>
>>> And also we can't really cleanly handle the failure case.
>>>
>>> This is why I was speculating about implementing the down/up portion
>>> in the devlink core. Add a flag that the driver requires reload_down
>>> to be called before the flashing operation, and reload_up after.
>>> This way not only core handles some of the error handling, but also
>>> it can mark the device as reload_failed if things go sideways, which
>>> is a nicer way to surface this sort of permanent error state.
>>
>> This makes sense... The question is if this should reuse existing
>> .reload_down and .reload_up callbacks let's say with new devlink action
>> DEVLINK_RELOAD_ACTION_FW_UPDATE or rather introduce new callbacks
>> .flash_update_down/_up() to avoid confusions.
> 
> Whatever makes sense for your driver, for now. I'm assuming both ops
> are the same, otherwise you wouldn't be asking? It should be trivial
> for someone add the extra ops later, and just hook them both up to the
> same functions in existing drivers.

Things are a little bit complicated after further investigation...

Some internal flashing backround first:
The zl3073x HW needs an external program called "flash utility"
to access an internal flash inside the chip. This utility provides
flash API over I2C/SPI bus that is different from the FW API provided
by the normal firmware. So to access the flash memory the driver has
to stop the device's CPU, to load the utility into chip RAM and resume
the CPU to execute the utility. At this point normal FW API is not
accessible so the driver has to stop the normal operation (unregister
DPLL devices, pins etc.). Then it updates flash using flash API and
after flash operations it has to reset device's CPU to restart newly
flashed firmware. Finally when normal FW is available it resumes
the normal operation (re-register DPLL devices etc.).

Current steps in this patch:
1. Load given FW file and verify that utility is present
2. Stop normal operations
3. Stop CPU, download utility to device, resume CPU
4. Flash components from the FW file
5. Unconditionally reset device's CPU to load normal FW
6. Resume normal operations

I found 4 possible options how to handle:

1. Introduce DEVLINK_RELOAD_ACTION_FW_UPDATE devlink action and reuse
    .reload_down/up() callbacks and call them prior and after
    .flash_update callback.

At first glance, it looks the most elegant... The zl3073x driver stops
during .reload_down() normal operation, then in .flash_update will
switch the device to flash mode, performs flash update and finally
during .reload_up() will resume normal operation.

Issues:
- a problematic case, when the given firmware file does not contain
   utility... During .reload_down() this cannot be checked as the
   firmware is not available during .reload_down() callback.
- DEVLINK_RELOAD_ACTION_FW_UPDATE has to be placed in devlink UAPI
   and but this reload action should be handled as internal one as
   there should not be possible to initiate it from the userspace

   e.g. devlink dev reload DEV action fw_update

2. Add new .flash_down/up() or .flash_prepare/done() optional callbacks
    that are called prior and after .flash_update if they are provided by
    a driver.

This looks also good and very similar to previous option. Could resolve
the 1st issue as we can pass 'devlink_flash_update_params' to both
new callbacks, so the driver can parse firmware file during
.flash_down and check for utility presence.

Issues:
- the firmware has to be parsed twice - during .flash_down() and
   .flash_update()
   This could be resolved by extending devlink_flash_update_params
   structure by 'void *priv' field that could be used by a driver
   during flash operation.
   It could be also useful to add flash_failed flag similar to
   reload_failed that would record a status reported by .flash_up()
   callback.

3. Keep my original approach but without restarting normal operation
    (re-register DPLL devices and pins). User has to use explicitly
    devlink reload fw_activate to restart normal operation.

This could be reasonable but introduces some kind of asymmetry because
the driver stops normal operation during .flash_update() and left
the device in intermediate state (devlink interface is working but
DPLL devices and pins are not registered). Only upon explicit user
request (fw_activate) would it restore normal mode (re-registration).

4. Keep my original approach, fix the ignored error code reported by
    Jakub and pass "re-start normal operation failure" via devlink
    notification.

 From my POV better than previous one as the driver will do its best to
resume device state prior the flashing. Only corner case where the
firmware is unresponsive after reset will cause that normal operation
won't be resumed -> could be handled by health reporting?

Thanks for opinions and advises.

Ivan

On Mon, 1 Sep 2025 18:34:14 +0200 Ivan Vecera wrote:
> 4. Keep my original approach, fix the ignored error code reported by
>     Jakub and pass "re-start normal operation failure" via devlink
>     notification.

4 is fine, we can revisit when another such device appears (tho I
believe this is an increasingly common way to implement FW update).