[PATCH net-next v2 0/5] dpll: zl3073x: Add support for devlink flash

[PATCH net-next v2 0/5] dpll: zl3073x: Add support for devlink flash

[Ivan Vecera]

Add functionality for accessing device hardware registers, loading
firmware bundles, and accessing the device's internal flash memory,
and use it to implement the devlink flash functionality.

Patch breakdown:
Patch1: helpers to access hardware registers
Patch2: low level functions to access flash memory
Patch3: support to load firmware bundles
Patch4: refactoring device initialization and helper functions
        for stopping and resuming device normal operation
Patch5: devlink .flash_update callback implementation

Changes:
v2:
* fixed several warnings found by patchwork bot
* added includes into new .c files
* fixed typos
* fixed uninitialized variable

Ivan Vecera (5):
  dpll: zl3073x: Add functions to access hardware registers
  dpll: zl3073x: Add low-level flash functions
  dpll: zl3073x: Add firmware loading functionality
  dpll: zl3073x: Refactor DPLL initialization
  dpll: zl3073x: Implement devlink flash callback

 Documentation/networking/devlink/zl3073x.rst |  14 +
 drivers/dpll/zl3073x/Makefile                |   2 +-
 drivers/dpll/zl3073x/core.c                  | 362 +++++++---
 drivers/dpll/zl3073x/core.h                  |  33 +
 drivers/dpll/zl3073x/devlink.c               |  92 ++-
 drivers/dpll/zl3073x/devlink.h               |   3 +
 drivers/dpll/zl3073x/flash.c                 | 684 +++++++++++++++++++
 drivers/dpll/zl3073x/flash.h                 |  29 +
 drivers/dpll/zl3073x/fw.c                    | 498 ++++++++++++++
 drivers/dpll/zl3073x/fw.h                    |  52 ++
 drivers/dpll/zl3073x/regs.h                  |  51 ++
 11 files changed, 1729 insertions(+), 91 deletions(-)
 create mode 100644 drivers/dpll/zl3073x/flash.c
 create mode 100644 drivers/dpll/zl3073x/flash.h
 create mode 100644 drivers/dpll/zl3073x/fw.c
 create mode 100644 drivers/dpll/zl3073x/fw.h

-- 
2.49.1

[PATCH net-next v2 1/5] dpll: zl3073x: Add functions to access hardware registers

[Ivan Vecera]

Besides the device host registers that are directly accessible, there
are also hardware registers that can be accessed indirectly via specific
host registers.

Add register definitions for accessing hardware registers and provide
helper functions for working with them. Additionally, extend the number
of pages in the regmap configuration to 256, as the host registers used
for accessing hardware registers are located on page 255.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/core.c | 155 +++++++++++++++++++++++++++++++++++-
 drivers/dpll/zl3073x/core.h |  30 +++++++
 drivers/dpll/zl3073x/regs.h |  12 +++
 3 files changed, 193 insertions(+), 4 deletions(-)

diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index 7ebcfc5ec1f09..86c26edc90462 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -95,9 +95,9 @@ EXPORT_SYMBOL_NS_GPL(zl30735_chip_info, "ZL3073X");
 
 #define ZL_RANGE_OFFSET		0x80
 #define ZL_PAGE_SIZE		0x80
-#define ZL_NUM_PAGES		15
+#define ZL_NUM_PAGES		256
 #define ZL_PAGE_SEL		0x7F
-#define ZL_PAGE_SEL_MASK	GENMASK(3, 0)
+#define ZL_PAGE_SEL_MASK	GENMASK(7, 0)
 #define ZL_NUM_REGS		(ZL_NUM_PAGES * ZL_PAGE_SIZE)
 
 /* Regmap range configuration */
@@ -174,9 +174,10 @@ static bool
 zl3073x_check_reg(struct zl3073x_dev *zldev, unsigned int reg, size_t size)
 {
 	/* Check that multiop lock is held when accessing registers
-	 * from page 10 and above.
+	 * from page 10 and above except the page 255 that does not
+	 * need this protection.
 	 */
-	if (ZL_REG_PAGE(reg) >= 10)
+	if (ZL_REG_PAGE(reg) >= 10 && ZL_REG_PAGE(reg) < 255)
 		lockdep_assert_held(&zldev->multiop_lock);
 
 	/* Check the index is in valid range for indexed register */
@@ -446,6 +447,152 @@ int zl3073x_mb_op(struct zl3073x_dev *zldev, unsigned int op_reg, u8 op_val,
 	return zl3073x_poll_zero_u8(zldev, op_reg, op_val);
 }
 
+/**
+ * zl3073x_do_hwreg_op - Perform HW register read/write operation
+ * @zldev: zl3073x device pointer
+ * @op: operation to perform
+ *
+ * Performs requested operation and waits for its completion.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_do_hwreg_op(struct zl3073x_dev *zldev, u8 op)
+{
+	int rc;
+
+	/* Set requested operation and set pending bit */
+	rc = zl3073x_write_u8(zldev, ZL_REG_HWREG_OP, op | ZL_HWREG_OP_PENDING);
+	if (rc)
+		return rc;
+
+	/* Poll for completion - pending bit cleared */
+	return zl3073x_poll_zero_u8(zldev, ZL_REG_HWREG_OP,
+				    ZL_HWREG_OP_PENDING);
+}
+
+/**
+ * zl3073x_read_hwreg - Read HW register
+ * @zldev: zl3073x device pointer
+ * @addr: HW register address
+ * @value: Value of the HW register
+ *
+ * Reads HW register value and stores it into @value.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_read_hwreg(struct zl3073x_dev *zldev, u32 addr, u32 *value)
+{
+	int rc;
+
+	/* Set address to read data from */
+	rc = zl3073x_write_u32(zldev, ZL_REG_HWREG_ADDR, addr);
+	if (rc)
+		return rc;
+
+	/* Perform the read operation */
+	rc = zl3073x_do_hwreg_op(zldev, ZL_HWREG_OP_READ);
+	if (rc)
+		return rc;
+
+	/* Read the received data */
+	return zl3073x_read_u32(zldev, ZL_REG_HWREG_READ_DATA, value);
+}
+
+/**
+ * zl3073x_write_hwreg - Write value to HW register
+ * @zldev: zl3073x device pointer
+ * @addr: HW registers address
+ * @value: Value to be written to HW register
+ *
+ * Stores the requested value into HW register.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_write_hwreg(struct zl3073x_dev *zldev, u32 addr, u32 value)
+{
+	int rc;
+
+	/* Set address to write data to */
+	rc = zl3073x_write_u32(zldev, ZL_REG_HWREG_ADDR, addr);
+	if (rc)
+		return rc;
+
+	/* Set data to be written */
+	rc = zl3073x_write_u32(zldev, ZL_REG_HWREG_WRITE_DATA, value);
+	if (rc)
+		return rc;
+
+	/* Perform the write operation */
+	return zl3073x_do_hwreg_op(zldev, ZL_HWREG_OP_WRITE);
+}
+
+/**
+ * zl3073x_update_hwreg - Update certain bits in HW register
+ * @zldev: zl3073x device pointer
+ * @addr: HW register address
+ * @value: Value to be written into HW register
+ * @mask: Bitmask indicating bits to be updated
+ *
+ * Reads given HW register, updates requested bits specified by value and
+ * mask and writes result back to HW register.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_update_hwreg(struct zl3073x_dev *zldev, u32 addr, u32 value,
+			 u32 mask)
+{
+	u32 tmp;
+	int rc;
+
+	rc = zl3073x_read_hwreg(zldev, addr, &tmp);
+	if (rc)
+		return rc;
+
+	tmp &= ~mask;
+	tmp |= value & mask;
+
+	return zl3073x_write_hwreg(zldev, addr, tmp);
+}
+
+/**
+ * zl3073x_write_hwreg_seq - Write HW registers sequence
+ * @zldev: pointer to device structure
+ * @seq: pointer to first sequence item
+ * @num_items: number of items in sequence
+ *
+ * Writes given HW registers sequence.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_write_hwreg_seq(struct zl3073x_dev *zldev,
+			    const struct zl3073x_hwreg_seq_item *seq,
+			    size_t num_items)
+{
+	int i, rc = 0;
+
+	for (i = 0; i < num_items; i++) {
+		dev_dbg(zldev->dev, "Write 0x%0x [0x%0x] to 0x%0x",
+			seq[i].value, seq[i].mask, seq[i].addr);
+
+		if (seq[i].mask == U32_MAX)
+			/* Write value directly */
+			rc = zl3073x_write_hwreg(zldev, seq[i].addr,
+						 seq[i].value);
+		else
+			/* Update only bits specified by the mask */
+			rc = zl3073x_update_hwreg(zldev, seq[i].addr,
+						  seq[i].value, seq[i].mask);
+		if (rc)
+			return rc;
+
+		if (seq->wait)
+			msleep(seq->wait);
+	}
+
+	return rc;
+}
+
 /**
  * zl3073x_ref_state_fetch - get input reference state
  * @zldev: pointer to zl3073x_dev structure
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index 71af2c8001109..16e750d77e1dd 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -3,6 +3,7 @@
 #ifndef _ZL3073X_CORE_H
 #define _ZL3073X_CORE_H
 
+#include <linux/bitfield.h>
 #include <linux/kthread.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -115,6 +116,28 @@ int zl3073x_dev_probe(struct zl3073x_dev *zldev,
  * Registers operations
  **********************/
 
+/**
+ * struct zl3073x_hwreg_seq_item - HW register write sequence item
+ * @addr: HW register to be written
+ * @value: value to be written to HW register
+ * @mask: bitmask indicating bits to be updated
+ * @wait: number of ms to wait after register write
+ */
+struct zl3073x_hwreg_seq_item {
+	u32	addr;
+	u32	value;
+	u32	mask;
+	u32	wait;
+};
+
+#define HWREG_SEQ_ITEM(_addr, _value, _mask, _wait)	\
+{							\
+	.addr	= _addr,				\
+	.value	= FIELD_PREP_CONST(_mask, _value),	\
+	.mask	= _mask,				\
+	.wait	= _wait,				\
+}
+
 int zl3073x_mb_op(struct zl3073x_dev *zldev, unsigned int op_reg, u8 op_val,
 		  unsigned int mask_reg, u16 mask_val);
 int zl3073x_poll_zero_u8(struct zl3073x_dev *zldev, unsigned int reg, u8 mask);
@@ -126,6 +149,13 @@ int zl3073x_write_u8(struct zl3073x_dev *zldev, unsigned int reg, u8 val);
 int zl3073x_write_u16(struct zl3073x_dev *zldev, unsigned int reg, u16 val);
 int zl3073x_write_u32(struct zl3073x_dev *zldev, unsigned int reg, u32 val);
 int zl3073x_write_u48(struct zl3073x_dev *zldev, unsigned int reg, u64 val);
+int zl3073x_read_hwreg(struct zl3073x_dev *zldev, u32 addr, u32 *value);
+int zl3073x_write_hwreg(struct zl3073x_dev *zldev, u32 addr, u32 value);
+int zl3073x_update_hwreg(struct zl3073x_dev *zldev, u32 addr, u32 value,
+			 u32 mask);
+int zl3073x_write_hwreg_seq(struct zl3073x_dev *zldev,
+			    const struct zl3073x_hwreg_seq_item *seq,
+			    size_t num_items);
 
 /*****************
  * Misc operations
diff --git a/drivers/dpll/zl3073x/regs.h b/drivers/dpll/zl3073x/regs.h
index 614e33128a5c9..80922987add34 100644
--- a/drivers/dpll/zl3073x/regs.h
+++ b/drivers/dpll/zl3073x/regs.h
@@ -260,4 +260,16 @@
 #define ZL_REG_OUTPUT_ESYNC_WIDTH		ZL_REG(14, 0x18, 4)
 #define ZL_REG_OUTPUT_PHASE_COMP		ZL_REG(14, 0x20, 4)
 
+/*
+ * Register Page 255 - HW registers access
+ */
+#define ZL_REG_HWREG_OP				ZL_REG(0xff, 0x00, 1)
+#define ZL_HWREG_OP_WRITE			0x28
+#define ZL_HWREG_OP_READ			0x29
+#define ZL_HWREG_OP_PENDING			BIT(1)
+
+#define ZL_REG_HWREG_ADDR			ZL_REG(0xff, 0x04, 4)
+#define ZL_REG_HWREG_WRITE_DATA			ZL_REG(0xff, 0x08, 4)
+#define ZL_REG_HWREG_READ_DATA			ZL_REG(0xff, 0x0c, 4)
+
 #endif /* _ZL3073X_REGS_H */
-- 
2.49.1

[PATCH net-next v2 2/5] dpll: zl3073x: Add low-level flash functions

[Ivan Vecera]

To implement the devlink device flash functionality, the driver needs
to access both the device memory and the internal flash memory. The flash
memory is accessed using a device-specific program (called the flash
utility). This flash utility must be downloaded by the driver into
the device memory and then executed by the device CPU. Once running,
the flash utility provides a flash API to access the flash memory itself.

During this operation, the normal functionality provided by the standard
firmware is not available. Therefore, the driver must ensure that DPLL
callbacks and monitoring functions are not executed during the flash
operation.

Add all necessary functions for downloading the utility to device memory,
entering and exiting flash mode, and performing flash operations.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v2:
* extended 'comp_str' to 32 chars to avoid warnings related to snprintf
* added additional includes
---
 drivers/dpll/zl3073x/Makefile  |   2 +-
 drivers/dpll/zl3073x/devlink.c |   9 +
 drivers/dpll/zl3073x/devlink.h |   3 +
 drivers/dpll/zl3073x/flash.c   | 684 +++++++++++++++++++++++++++++++++
 drivers/dpll/zl3073x/flash.h   |  29 ++
 drivers/dpll/zl3073x/regs.h    |  39 ++
 6 files changed, 765 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dpll/zl3073x/flash.c
 create mode 100644 drivers/dpll/zl3073x/flash.h

diff --git a/drivers/dpll/zl3073x/Makefile b/drivers/dpll/zl3073x/Makefile
index c3e2f02f319dc..9894513f67dd3 100644
--- a/drivers/dpll/zl3073x/Makefile
+++ b/drivers/dpll/zl3073x/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_ZL3073X)		+= zl3073x.o
-zl3073x-objs			:= core.o devlink.o dpll.o prop.o
+zl3073x-objs			:= core.o devlink.o dpll.o flash.o prop.o
 
 obj-$(CONFIG_ZL3073X_I2C)	+= zl3073x_i2c.o
 zl3073x_i2c-objs		:= i2c.o
diff --git a/drivers/dpll/zl3073x/devlink.c b/drivers/dpll/zl3073x/devlink.c
index 7e7fe726ee37a..f3ca973a4d416 100644
--- a/drivers/dpll/zl3073x/devlink.c
+++ b/drivers/dpll/zl3073x/devlink.c
@@ -138,6 +138,15 @@ zl3073x_devlink_reload_up(struct devlink *devlink,
 	return 0;
 }
 
+void zl3073x_devlink_flash_notify(struct zl3073x_dev *zldev, const char *msg,
+				  const char *component, u32 done, u32 total)
+{
+	struct devlink *devlink = priv_to_devlink(zldev);
+
+	devlink_flash_update_status_notify(devlink, msg, component, done,
+					   total);
+}
+
 static const struct devlink_ops zl3073x_devlink_ops = {
 	.info_get = zl3073x_devlink_info_get,
 	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
diff --git a/drivers/dpll/zl3073x/devlink.h b/drivers/dpll/zl3073x/devlink.h
index 037720db204fc..63dfd6fa1cd60 100644
--- a/drivers/dpll/zl3073x/devlink.h
+++ b/drivers/dpll/zl3073x/devlink.h
@@ -9,4 +9,7 @@ struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev);
 
 int zl3073x_devlink_register(struct zl3073x_dev *zldev);
 
+void zl3073x_devlink_flash_notify(struct zl3073x_dev *zldev, const char *msg,
+				  const char *component, u32 done, u32 total);
+
 #endif /* _ZL3073X_DEVLINK_H */
diff --git a/drivers/dpll/zl3073x/flash.c b/drivers/dpll/zl3073x/flash.c
new file mode 100644
index 0000000000000..df27e892e8c29
--- /dev/null
+++ b/drivers/dpll/zl3073x/flash.c
@@ -0,0 +1,684 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/errno.h>
+#include <linux/jiffies.h>
+#include <linux/minmax.h>
+#include <linux/netlink.h>
+#include <linux/sched/signal.h>
+#include <linux/sprintf.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <net/devlink.h>
+
+#include "core.h"
+#include "devlink.h"
+#include "flash.h"
+
+#define ZL_FLASH_ERR_PFX "FW update failed: "
+#define ZL_FLASH_ERR_MSG(_zldev, _extack, _msg, ...)			\
+	do {								\
+		dev_err((_zldev)->dev, ZL_FLASH_ERR_PFX _msg "\n",	\
+			## __VA_ARGS__);				\
+		NL_SET_ERR_MSG_FMT_MOD((_extack), ZL_FLASH_ERR_PFX _msg,\
+				       ## __VA_ARGS__);			\
+	} while (0)
+
+/**
+ * zl3073x_flash_download_block - Download image block to device memory
+ * @zldev: zl3073x device structure
+ * @image: image to be downloaded
+ * @start: start position (in 32-bit words)
+ * @size: size to download (in 32-bit words)
+ * @extack: netlink extack pointer to report errors
+ *
+ * Returns 0 in case of success or negative value otherwise.
+ */
+static int
+zl3073x_flash_download(struct zl3073x_dev *zldev, const char *component,
+		       u32 addr, const void *data, size_t size,
+		       struct netlink_ext_ack *extack)
+{
+#define CHECK_DELAY	5000 /* Check for interrupt each 5 seconds */
+	unsigned long timeout;
+	const void *ptr, *end;
+	int rc = 0;
+
+	dev_dbg(zldev->dev, "Downloading %zu bytes to device memory at 0x%0x\n",
+		size, addr);
+
+	timeout = jiffies + msecs_to_jiffies(CHECK_DELAY);
+
+	for (ptr = data, end = data + size; ptr < end; ptr += 4, addr += 4) {
+		/* Write current word to HW memory */
+		rc = zl3073x_write_hwreg(zldev, addr, *(const u32 *)ptr);
+		if (rc) {
+			ZL_FLASH_ERR_MSG(zldev, extack,
+					 "failed to write to memory at 0x%0x",
+					 addr);
+			return rc;
+		}
+
+		/* Check for pending interrupt each 5 seconds */
+		if (time_after(jiffies, timeout)) {
+			if (signal_pending(current)) {
+				ZL_FLASH_ERR_MSG(zldev, extack,
+						 "Flashing interrupted");
+				return -EINTR;
+			}
+
+			timeout = jiffies + msecs_to_jiffies(CHECK_DELAY);
+		}
+
+		/* Report status each 1 kB block */
+		if ((ptr - data) % 1024 == 0)
+			zl3073x_devlink_flash_notify(zldev, "Downloading image",
+						     component, ptr - data,
+						     size);
+	}
+
+	zl3073x_devlink_flash_notify(zldev, "Downloading image", component,
+				     ptr - data, size);
+
+	dev_dbg(zldev->dev, "%zu bytes downloaded to device memory\n", size);
+
+	return rc;
+}
+
+/**
+ * zl3073x_flash_error_check - Check for flash utility errors
+ * @zldev: zl3073x device structure
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function checks for errors detected by the flash utility and
+ * reports them if any were found.
+ *
+ * Return: 0 on success, -EIO when errors are detected
+ */
+static int
+zl3073x_flash_error_check(struct zl3073x_dev *zldev,
+			  struct netlink_ext_ack *extack)
+{
+	u32 count, cause;
+	int rc;
+
+	rc = zl3073x_read_u32(zldev, ZL_REG_ERROR_COUNT, &count);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_u32(zldev, ZL_REG_ERROR_CAUSE, &cause);
+	if (rc)
+		return rc;
+
+	/* Return if no error occurred */
+	if (!count)
+		return 0;
+
+	/* Report errors */
+	ZL_FLASH_ERR_MSG(zldev, extack,
+			 "utility error occurred: count=%u cause=0x%x", count,
+			 cause);
+
+	return -EIO;
+}
+
+/**
+ * zl3073x_flash_wait_ready - Check or wait for utility to be ready to flash
+ * @zldev: zl3073x device structure
+ * @timeout_ms: timeout for the waiting
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_flash_wait_ready(struct zl3073x_dev *zldev, unsigned int timeout_ms)
+{
+#define ZL_FLASH_POLL_DELAY_MS	100
+	unsigned long timeout;
+	int rc, i;
+
+	dev_dbg(zldev->dev, "Waiting for flashing to be ready\n");
+
+	timeout = jiffies + msecs_to_jiffies(timeout_ms);
+
+	for (i = 0, timeout = jiffies + msecs_to_jiffies(timeout_ms);
+	     time_before(jiffies, timeout);
+	     i++) {
+		u8 value;
+
+		/* Check for interrupt each 1s */
+		if (i > 9) {
+			if (signal_pending(current))
+				return -EINTR;
+			i = 0;
+		}
+
+		/* Read write_flash register value */
+		rc = zl3073x_read_u8(zldev, ZL_REG_WRITE_FLASH, &value);
+		if (rc)
+			return rc;
+
+		value = FIELD_GET(ZL_WRITE_FLASH_OP, value);
+
+		/* Check if the current operation was done */
+		if (value == ZL_WRITE_FLASH_OP_DONE)
+			return 0; /* Operation was successfully done */
+
+		msleep(ZL_FLASH_POLL_DELAY_MS);
+	}
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * zl3073x_flash_cmd_wait - Perform flash operation and wait for finish
+ * @zldev: zl3073x device structure
+ * @operation: operation to perform
+ * @extack: netlink extack pointer to report errors
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_flash_cmd_wait(struct zl3073x_dev *zldev, u32 operation,
+		       struct netlink_ext_ack *extack)
+{
+#define FLASH_PHASE1_TIMEOUT_MS 60000	/* up to 1 minute */
+#define FLASH_PHASE2_TIMEOUT_MS 120000	/* up to 2 minutes */
+	u8 value;
+	int rc;
+
+	dev_dbg(zldev->dev, "Sending flash command: 0x%x\n", operation);
+
+	/* Wait for access */
+	rc = zl3073x_flash_wait_ready(zldev, FLASH_PHASE1_TIMEOUT_MS);
+	if (rc)
+		return rc;
+
+	/* Issue the requested operation */
+	rc = zl3073x_read_u8(zldev, ZL_REG_WRITE_FLASH, &value);
+	if (rc)
+		return rc;
+
+	value &= ~ZL_WRITE_FLASH_OP;
+	value |= FIELD_PREP(ZL_WRITE_FLASH_OP, operation);
+
+	rc = zl3073x_write_u8(zldev, ZL_REG_WRITE_FLASH, value);
+	if (rc)
+		return rc;
+
+	/* Wait for command completion */
+	rc = zl3073x_flash_wait_ready(zldev, FLASH_PHASE2_TIMEOUT_MS);
+	if (rc)
+		return rc;
+
+	/* Check for utility errors */
+	return zl3073x_flash_error_check(zldev, extack);
+}
+
+/**
+ * zl3073x_flash_get_sector_size - Get flash sector size
+ * @zldev: zl3073x device structure
+ * @sector_size: sector size returned by the function
+ *
+ * The function reads the flash sector size detected by flash utility and
+ * stores it into @sector_size.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_flash_get_sector_size(struct zl3073x_dev *zldev, size_t *sector_size)
+{
+	u8 flash_info;
+	int rc;
+
+	rc = zl3073x_read_u8(zldev, ZL_REG_FLASH_INFO, &flash_info);
+	if (rc)
+		return rc;
+
+	switch (FIELD_GET(ZL_FLASH_INFO_SECTOR_SIZE, flash_info)) {
+	case ZL_FLASH_INFO_SECTOR_4K:
+		*sector_size = 0x1000;
+		break;
+	case ZL_FLASH_INFO_SECTOR_64K:
+		*sector_size = 0x10000;
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	return rc;
+}
+
+/**
+ * zl3073x_flash_sectors - Flash sectors
+ * @zldev: zl3073x device structure
+ * @component: component name
+ * @page: destination flash page
+ * @addr: device memory address to load data
+ * @data: pointer to data to be flashed
+ * @size: size of data
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function flashes given @data with size of @size to the internal flash
+ * memory block starting from page @page. The function uses sector flash
+ * method and has to take into account the flash sector size reported by
+ * flashing utility. Input data are spliced into blocks according this
+ * sector size and each block is flashed separately.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_flash_sectors(struct zl3073x_dev *zldev, const char *component,
+			  u32 page, u32 addr, const void *data, size_t size,
+			  struct netlink_ext_ack *extack)
+{
+#define ZL_FLASH_MAX_BLOCK_SIZE	0x0001E000
+#define ZL_FLASH_PAGE_SIZE	256
+	size_t max_block_size, block_size, sector_size;
+	const void *ptr, *end;
+	int rc;
+
+	/* Get flash sector size */
+	rc = zl3073x_flash_get_sector_size(zldev, &sector_size);
+	if (rc) {
+		ZL_FLASH_ERR_MSG(zldev, extack,
+				 "Failed to get flash sector size");
+		return rc;
+	}
+
+	/* Determine max block size depending on sector size */
+	max_block_size = ALIGN_DOWN(ZL_FLASH_MAX_BLOCK_SIZE, sector_size);
+
+	for (ptr = data, end = data + size; ptr < end; ptr += block_size) {
+		char comp_str[32];
+
+		block_size = min_t(size_t, max_block_size, end - ptr);
+
+		/* Add suffix '-partN' if the requested component size is
+		 * greater than max_block_size.
+		 */
+		if (max_block_size < size)
+			snprintf(comp_str, sizeof(comp_str), "%s-part%zu",
+				 component, (ptr - data) / max_block_size + 1);
+		else
+			strscpy(comp_str, component);
+
+		/* Download block to device memory */
+		rc = zl3073x_flash_download(zldev, comp_str, addr, ptr,
+					    block_size, extack);
+		if (rc)
+			goto finish;
+
+		/* Set address to flash from */
+		rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_START_ADDR, addr);
+		if (rc)
+			goto finish;
+
+		/* Set size of block to flash */
+		rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_SIZE, block_size);
+		if (rc)
+			goto finish;
+
+		/* Set destination page to flash */
+		rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, page);
+		if (rc)
+			goto finish;
+
+		/* Set filling pattern */
+		rc = zl3073x_write_u32(zldev, ZL_REG_FILL_PATTERN, U32_MAX);
+		if (rc)
+			goto finish;
+
+		zl3073x_devlink_flash_notify(zldev, "Flashing image", comp_str,
+					     0, 0);
+
+		dev_dbg(zldev->dev, "Flashing %zu bytes to page %u\n",
+			block_size, page);
+
+		/* Execute sectors flash operation */
+		rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_SECTORS,
+					    extack);
+		if (rc)
+			goto finish;
+
+		/* Move to next page */
+		page += block_size / ZL_FLASH_PAGE_SIZE;
+	}
+
+finish:
+	zl3073x_devlink_flash_notify(zldev,
+				     rc ?  "Flashing failed" : "Flashing done",
+				     component, 0, 0);
+
+	return rc;
+}
+
+/**
+ * zl3073x_flash_page - Flash page
+ * @zldev: zl3073x device structure
+ * @component: component name
+ * @page: destination flash page
+ * @addr: device memory address to load data
+ * @data: pointer to data to be flashed
+ * @size: size of data
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function flashes given @data with size of @size to the internal flash
+ * memory block starting with page @page.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_flash_page(struct zl3073x_dev *zldev, const char *component,
+		       u32 page, u32 addr, const void *data, size_t size,
+		       struct netlink_ext_ack *extack)
+{
+	int rc;
+
+	/* Download component to device memory */
+	rc = zl3073x_flash_download(zldev, component, addr, data, size, extack);
+	if (rc)
+		goto finish;
+
+	/* Set address to flash from */
+	rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_START_ADDR, addr);
+	if (rc)
+		goto finish;
+
+	/* Set size of block to flash */
+	rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_SIZE, size);
+	if (rc)
+		goto finish;
+
+	/* Set destination page to flash */
+	rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, page);
+	if (rc)
+		goto finish;
+
+	/* Set filling pattern */
+	rc = zl3073x_write_u32(zldev, ZL_REG_FILL_PATTERN, U32_MAX);
+	if (rc)
+		goto finish;
+
+	zl3073x_devlink_flash_notify(zldev, "Flashing image", component, 0,
+				     size);
+
+	/* Execute sectors flash operation */
+	rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_PAGE, extack);
+	if (rc)
+		goto finish;
+
+	zl3073x_devlink_flash_notify(zldev, "Flashing image", component, size,
+				     size);
+
+finish:
+	zl3073x_devlink_flash_notify(zldev,
+				     rc ?  "Flashing failed" : "Flashing done",
+				     component, 0, 0);
+
+	return rc;
+}
+
+/**
+ * zl3073x_flash_page_copy - Copy flash page
+ * @zldev: zl3073x device structure
+ * @component: component name
+ * @src_page: source page to copy
+ * @dst_page: destination page
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function copies one flash page specified by @src_page into the flash
+ * page specified by @dst_page.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_flash_page_copy(struct zl3073x_dev *zldev, const char *component,
+			    u32 src_page, u32 dst_page,
+			    struct netlink_ext_ack *extack)
+{
+	int rc;
+
+	/* Set source page to be copied */
+	rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_READ, src_page);
+	if (rc)
+		return rc;
+
+	/* Set destination page for the copy */
+	rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, dst_page);
+	if (rc)
+		return rc;
+
+	/* Perform copy operation */
+	rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_COPY_PAGE, extack);
+	if (rc)
+		ZL_FLASH_ERR_MSG(zldev, extack,
+				 "Failed to copy page %u to page %u", src_page,
+				 dst_page);
+
+	return rc;
+}
+
+/**
+ * zl3073x_flash_mode_verify - Check flash utility
+ * @zldev: zl3073x device structure
+ *
+ * Return: 0 if the flash utility is ready, <0 on error
+ */
+static int
+zl3073x_flash_mode_verify(struct zl3073x_dev *zldev)
+{
+	u8 family, release;
+	u32 hash;
+	int rc;
+
+	rc = zl3073x_read_u32(zldev, ZL_REG_FLASH_HASH, &hash);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_u8(zldev, ZL_REG_FLASH_FAMILY, &family);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_u8(zldev, ZL_REG_FLASH_RELEASE, &release);
+	if (rc)
+		return rc;
+
+	dev_dbg(zldev->dev,
+		"Flash utility check: hash 0x%08x, fam 0x%02x, rel 0x%02x\n",
+		hash, family, release);
+
+	/* Return success for correct family */
+	return (family == 0x21) ? 0 : -ENODEV;
+}
+
+static int
+zl3073x_flash_host_ctrl_enable(struct zl3073x_dev *zldev)
+{
+	u8 host_ctrl;
+	int rc;
+
+	/* Read host control register */
+	rc = zl3073x_read_u8(zldev, ZL_REG_HOST_CONTROL, &host_ctrl);
+	if (rc)
+		return rc;
+
+	/* Enable host control */
+	host_ctrl &= ~ZL_HOST_CONTROL_ENABLE;
+	host_ctrl |= ZL_HOST_CONTROL_ENABLE;
+
+	/* Update host control register */
+	return zl3073x_write_u8(zldev, ZL_REG_HOST_CONTROL, host_ctrl);
+}
+
+/**
+ * zl3073x_flash_mode_enter - Switch the device to flash mode
+ * @zldev: zl3073x device structure
+ * @util_ptr: buffer with flash utility
+ * @util_size: size of buffer with flash utility
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function prepares and switches the device into flash mode.
+ *
+ * The procedure:
+ * 1) Stop device CPU by specific HW register sequence
+ * 2) Download flash utility to device memory
+ * 3) Resume device CPU by specific HW register sequence
+ * 4) Check communication with flash utility
+ * 5) Enable host control necessary to access flash API
+ * 6) Check for potential error detected by the utility
+ *
+ * The API provided by normal firmware is not available in flash mode
+ * so the caller has to ensure that this API is not used in this mode.
+ *
+ * After performing flash operation the caller should call
+ * @zl3073x_flash_mode_leave to return back to normal operation.
+ *
+ * Return: 0 on success, <0 on error.
+ */
+int zl3073x_flash_mode_enter(struct zl3073x_dev *zldev, const void *util_ptr,
+			     size_t util_size, struct netlink_ext_ack *extack)
+{
+	/* Sequence to be written prior utility download */
+	static const struct zl3073x_hwreg_seq_item pre_seq[] = {
+		HWREG_SEQ_ITEM(0x80000400, 1, BIT(0), 0),
+		HWREG_SEQ_ITEM(0x80206340, 1, BIT(4), 0),
+		HWREG_SEQ_ITEM(0x10000000, 1, BIT(2), 0),
+		HWREG_SEQ_ITEM(0x10000024, 0x00000001, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10000020, 0x00000001, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10000000, 1, BIT(10), 1000),
+	};
+	/* Sequence to be written after utility download */
+	static const struct zl3073x_hwreg_seq_item post_seq[] = {
+		HWREG_SEQ_ITEM(0x10400004, 0x000000C0, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10400008, 0x00000000, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10400010, 0x20000000, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10400014, 0x20000004, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10000000, 1, GENMASK(10, 9), 0),
+		HWREG_SEQ_ITEM(0x10000020, 0x00000000, U32_MAX, 0),
+		HWREG_SEQ_ITEM(0x10000000, 0, BIT(0), 1000),
+	};
+	int rc;
+
+	zl3073x_devlink_flash_notify(zldev, "Prepare flash mode", "utility",
+				     0, 0);
+
+	/* Execure pre-load sequence */
+	rc = zl3073x_write_hwreg_seq(zldev, pre_seq, ARRAY_SIZE(pre_seq));
+	if (rc) {
+		ZL_FLASH_ERR_MSG(zldev, extack,
+				 "cannot execute pre-load sequence");
+		goto error;
+	}
+
+	/* Download utility image to device memory */
+	rc = zl3073x_flash_download(zldev, "utility", 0x20000000, util_ptr,
+				    util_size, extack);
+	if (rc) {
+		ZL_FLASH_ERR_MSG(zldev, extack,
+				 "cannot download flash utility");
+		goto error;
+	}
+
+	/* Execute post-load sequence */
+	rc = zl3073x_write_hwreg_seq(zldev, post_seq, ARRAY_SIZE(post_seq));
+	if (rc) {
+		ZL_FLASH_ERR_MSG(zldev, extack,
+				 "cannot execute post-load sequence");
+		goto error;
+	}
+
+	/* Check that utility identifies itself correctly */
+	rc = zl3073x_flash_mode_verify(zldev);
+	if (rc) {
+		ZL_FLASH_ERR_MSG(zldev, extack, "flash utility check failed");
+		goto error;
+	}
+
+	/* Enable host control */
+	rc = zl3073x_flash_host_ctrl_enable(zldev);
+	if (rc) {
+		ZL_FLASH_ERR_MSG(zldev, extack, "cannot enable host control");
+		goto error;
+	}
+
+	zl3073x_devlink_flash_notify(zldev, "Flash mode enabled", "utility",
+				     0, 0);
+
+	return 0;
+
+error:
+	rc = zl3073x_flash_mode_leave(zldev, extack);
+	if (rc)
+		ZL_FLASH_ERR_MSG(zldev, extack,
+				 "failed to switch back to normal mode");
+
+	return rc;
+}
+
+/**
+ * zl3073x_flash_mode_leave - Leave flash mode
+ * @zldev: zl3073x device structure
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function instructs the device to leave the flash mode and
+ * to return back to normal operation.
+ *
+ * The procedure:
+ * 1) Set reset flag
+ * 2) Reset the device CPU by specific HW register sequence
+ * 3) Wait for the device to be ready
+ * 4) Check the reset flag was cleared
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_flash_mode_leave(struct zl3073x_dev *zldev,
+			     struct netlink_ext_ack *extack)
+{
+	/* Sequence to be written after flash */
+	static const struct zl3073x_hwreg_seq_item fw_reset_seq[] = {
+		HWREG_SEQ_ITEM(0x80000404, 1, BIT(0), 0),
+		HWREG_SEQ_ITEM(0x80000410, 1, BIT(0), 0),
+	};
+	u8 reset_status;
+	int rc;
+
+	zl3073x_devlink_flash_notify(zldev, "Leaving flash mode", "utility",
+				     0, 0);
+
+	/* Read reset status register */
+	rc = zl3073x_read_u8(zldev, ZL_REG_RESET_STATUS, &reset_status);
+	if (rc)
+		return rc;
+
+	/* Set reset bit */
+	reset_status |= ZL_REG_RESET_STATUS_RESET;
+
+	/* Update reset status register */
+	rc = zl3073x_write_u8(zldev, ZL_REG_RESET_STATUS, reset_status);
+	if (rc)
+		return rc;
+
+	/* We do not check the return value here as the sequence resets
+	 * the device CPU and the last write always return an error.
+	 */
+	zl3073x_write_hwreg_seq(zldev, fw_reset_seq, ARRAY_SIZE(fw_reset_seq));
+
+	/* Wait for the device to be ready */
+	msleep(500);
+
+	/* Read again the reset status register */
+	rc = zl3073x_read_u8(zldev, ZL_REG_RESET_STATUS, &reset_status);
+	if (rc)
+		return rc;
+
+	/* Check the reset bit was cleared */
+	if (reset_status & ZL_REG_RESET_STATUS_RESET) {
+		dev_err(zldev->dev,
+			"Reset not confirmed after switch to normal mode\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/dpll/zl3073x/flash.h b/drivers/dpll/zl3073x/flash.h
new file mode 100644
index 0000000000000..effe1b16b3591
--- /dev/null
+++ b/drivers/dpll/zl3073x/flash.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef __ZL3073X_FLASH_H
+#define __ZL3073X_FLASH_H
+
+#include <linux/types.h>
+
+struct netlink_ext_ack;
+struct zl3073x_dev;
+
+int zl3073x_flash_mode_enter(struct zl3073x_dev *zldev, const void *util_ptr,
+			     size_t util_size, struct netlink_ext_ack *extack);
+
+int zl3073x_flash_mode_leave(struct zl3073x_dev *zldev,
+			     struct netlink_ext_ack *extack);
+
+int zl3073x_flash_page(struct zl3073x_dev *zldev, const char *component,
+		       u32 page, u32 addr, const void *data, size_t size,
+		       struct netlink_ext_ack *extack);
+
+int zl3073x_flash_page_copy(struct zl3073x_dev *zldev, const char *component,
+			    u32 src_page, u32 dst_page,
+			    struct netlink_ext_ack *extack);
+
+int zl3073x_flash_sectors(struct zl3073x_dev *zldev, const char *component,
+			  u32 page, u32 addr, const void *data, size_t size,
+			  struct netlink_ext_ack *extack);
+
+#endif /* __ZL3073X_FLASH_H */
diff --git a/drivers/dpll/zl3073x/regs.h b/drivers/dpll/zl3073x/regs.h
index 80922987add34..19a25325bd9c7 100644
--- a/drivers/dpll/zl3073x/regs.h
+++ b/drivers/dpll/zl3073x/regs.h
@@ -72,6 +72,9 @@
 #define ZL_REG_FW_VER				ZL_REG(0, 0x05, 2)
 #define ZL_REG_CUSTOM_CONFIG_VER		ZL_REG(0, 0x07, 4)
 
+#define ZL_REG_RESET_STATUS			ZL_REG(0, 0x18, 1)
+#define ZL_REG_RESET_STATUS_RESET		BIT(0)
+
 /*************************
  * Register Page 2, Status
  *************************/
@@ -272,4 +275,40 @@
 #define ZL_REG_HWREG_WRITE_DATA			ZL_REG(0xff, 0x08, 4)
 #define ZL_REG_HWREG_READ_DATA			ZL_REG(0xff, 0x0c, 4)
 
+/*
+ * Registers available in flash mode
+ */
+#define ZL_REG_FLASH_HASH			ZL_REG(0, 0x78, 4)
+#define ZL_REG_FLASH_FAMILY			ZL_REG(0, 0x7c, 1)
+#define ZL_REG_FLASH_RELEASE			ZL_REG(0, 0x7d, 1)
+
+#define ZL_REG_HOST_CONTROL			ZL_REG(1, 0x02, 1)
+#define ZL_HOST_CONTROL_ENABLE			BIT(0)
+
+#define ZL_REG_IMAGE_START_ADDR			ZL_REG(1, 0x04, 4)
+#define ZL_REG_IMAGE_SIZE			ZL_REG(1, 0x08, 4)
+#define ZL_REG_FLASH_INDEX_READ			ZL_REG(1, 0x0c, 4)
+#define ZL_REG_FLASH_INDEX_WRITE		ZL_REG(1, 0x10, 4)
+#define ZL_REG_FILL_PATTERN			ZL_REG(1, 0x14, 4)
+
+#define ZL_REG_WRITE_FLASH			ZL_REG(1, 0x18, 1)
+#define ZL_WRITE_FLASH_OP			GENMASK(2, 0)
+#define ZL_WRITE_FLASH_OP_DONE			0x0
+#define ZL_WRITE_FLASH_OP_SECTORS		0x2
+#define ZL_WRITE_FLASH_OP_PAGE			0x3
+#define ZL_WRITE_FLASH_OP_COPY_PAGE		0x4
+
+#define ZL_REG_FLASH_INFO			ZL_REG(2, 0x00, 1)
+#define ZL_FLASH_INFO_SECTOR_SIZE		GENMASK(3, 0)
+#define ZL_FLASH_INFO_SECTOR_4K			0
+#define ZL_FLASH_INFO_SECTOR_64K		1
+
+#define ZL_REG_ERROR_COUNT			ZL_REG(2, 0x04, 4)
+#define ZL_REG_ERROR_CAUSE			ZL_REG(2, 0x08, 4)
+
+#define ZL_REG_OP_STATE				ZL_REG(2, 0x14, 1)
+#define ZL_OP_STATE_NO_COMMAND			0
+#define ZL_OP_STATE_PENDING			1
+#define ZL_OP_STATE_DONE			2
+
 #endif /* _ZL3073X_REGS_H */
-- 
2.49.1

[PATCH net-next v2 3/5] dpll: zl3073x: Add firmware loading functionality

[Ivan Vecera]

Add functionality for loading firmware files provided by the vendor
to be flashed into the device's internal flash memory. The firmware
consists of several components, such as the firmware executable itself,
chip-specific customizations, and configuration files.

The firmware file contains at least a flash utility, which is executed
on the device side, and one or more flashable components. Each component
has its own specific properties, such as the address where it should be
loaded during flashing, one or more destination flash pages, and
the flashing method that should be used.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v2:
* added additional includes
* removed empty line
* '*(dst+len)' -> '*(dst + len)'
* 'Santity' -> 'Sanity'
* fixed smatch warning about uninitialized 'rc'
---
 drivers/dpll/zl3073x/Makefile |   2 +-
 drivers/dpll/zl3073x/fw.c     | 498 ++++++++++++++++++++++++++++++++++
 drivers/dpll/zl3073x/fw.h     |  52 ++++
 3 files changed, 551 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dpll/zl3073x/fw.c
 create mode 100644 drivers/dpll/zl3073x/fw.h

diff --git a/drivers/dpll/zl3073x/Makefile b/drivers/dpll/zl3073x/Makefile
index 9894513f67dd3..84e22aae57e5f 100644
--- a/drivers/dpll/zl3073x/Makefile
+++ b/drivers/dpll/zl3073x/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_ZL3073X)		+= zl3073x.o
-zl3073x-objs			:= core.o devlink.o dpll.o flash.o prop.o
+zl3073x-objs			:= core.o devlink.o dpll.o flash.o fw.o prop.o
 
 obj-$(CONFIG_ZL3073X_I2C)	+= zl3073x_i2c.o
 zl3073x_i2c-objs		:= i2c.o
diff --git a/drivers/dpll/zl3073x/fw.c b/drivers/dpll/zl3073x/fw.c
new file mode 100644
index 0000000000000..5599b075bdf83
--- /dev/null
+++ b/drivers/dpll/zl3073x/fw.c
@@ -0,0 +1,498 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/array_size.h>
+#include <linux/build_bug.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/netlink.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include "core.h"
+#include "flash.h"
+#include "fw.h"
+
+#define ZL3073X_FW_ERR_PFX "FW load failed: "
+#define ZL3073X_FW_ERR_MSG(_zldev, _extack, _msg, ...)			\
+	do {								\
+		dev_err((_zldev)->dev, ZL3073X_FW_ERR_PFX _msg "\n",	\
+			## __VA_ARGS__);				\
+		NL_SET_ERR_MSG_FMT_MOD((_extack),			\
+				       ZL3073X_FW_ERR_PFX _msg,		\
+				       ## __VA_ARGS__);			\
+	} while (0)
+
+enum zl3073x_flash_type {
+	ZL3073X_FLASH_TYPE_NONE = 0,
+	ZL3073X_FLASH_TYPE_SECTORS,
+	ZL3073X_FLASH_TYPE_PAGE,
+	ZL3073X_FLASH_TYPE_PAGE_AND_COPY,
+};
+
+struct zl3073x_fw_component_info {
+	const char		*name;
+	size_t			max_size;
+	enum zl3073x_flash_type	flash_type;
+	u32			load_addr;
+	u32			dest_page;
+	u32			copy_page;
+};
+
+static const struct zl3073x_fw_component_info component_info[] = {
+	[ZL_FW_COMPONENT_UTIL] = {
+		.name		= "utility",
+		.max_size	= 0x2300,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_NONE,
+	},
+	[ZL_FW_COMPONENT_FW1] = {
+		.name		= "firmware1",
+		.max_size	= 0x35000,
+		.load_addr	= 0x20002000,
+		.flash_type	= ZL3073X_FLASH_TYPE_SECTORS,
+		.dest_page	= 0x020,
+	},
+	[ZL_FW_COMPONENT_FW2] = {
+		.name		= "firmware2",
+		.max_size	= 0x0040,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE_AND_COPY,
+		.dest_page	= 0x3e0,
+		.copy_page	= 0x000,
+	},
+	[ZL_FW_COMPONENT_FW3] = {
+		.name		= "firmware3",
+		.max_size	= 0x0248,
+		.load_addr	= 0x20000400,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE_AND_COPY,
+		.dest_page	= 0x3e4,
+		.copy_page	= 0x004,
+	},
+	[ZL_FW_COMPONENT_CFG0] = {
+		.name		= "config0",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x3d0,
+	},
+	[ZL_FW_COMPONENT_CFG1] = {
+		.name		= "config1",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x3c0,
+	},
+	[ZL_FW_COMPONENT_CFG2] = {
+		.name		= "config2",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x3b0,
+	},
+	[ZL_FW_COMPONENT_CFG3] = {
+		.name		= "config3",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x3a0,
+	},
+	[ZL_FW_COMPONENT_CFG4] = {
+		.name		= "config4",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x390,
+	},
+	[ZL_FW_COMPONENT_CFG5] = {
+		.name		= "config5",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x380,
+	},
+	[ZL_FW_COMPONENT_CFG6] = {
+		.name		= "config6",
+		.max_size	= 0x1000,
+		.load_addr	= 0x20000000,
+		.flash_type	= ZL3073X_FLASH_TYPE_PAGE,
+		.dest_page	= 0x370,
+	},
+};
+
+/* Sanity check */
+static_assert(ARRAY_SIZE(component_info) == ZL_FW_NUM_COMPONENTS);
+
+/**
+ * zl3073x_fw_readline - Read next line from firmware
+ * @dst: destination buffer
+ * @dst_sz: destination buffer size
+ * @psrc: source buffer
+ * @psrc_sz: source buffer size
+ *
+ * The function read next line from the firmware buffer specified by @psrc
+ * and @psrc_sz and stores it into buffer specified by @dst and @dst_sz.
+ * The pointer @psrc and remaining bytes in @psrc_sz are updated accordingly.
+ *
+ * Return: number of characters read on success, -EINVAL on error
+ */
+static ssize_t
+zl3073x_fw_readline(char *dst, size_t dst_sz, const char **psrc,
+		    size_t *psrc_sz)
+{
+	const char *ptr = *psrc;
+	size_t len;
+
+	/* Skip any existing new-lines at the beginning */
+	ptr = memchr_inv(*psrc, '\n', *psrc_sz);
+	if (ptr) {
+		*psrc_sz -= ptr - *psrc;
+		*psrc = ptr;
+	}
+
+	/* Now look for the next new-line in the source */
+	ptr = memscan((void *)*psrc, '\n', *psrc_sz);
+	len = ptr - *psrc;
+
+	/* Return error if the source line is too long for destination */
+	if (len >= dst_sz)
+		return -EINVAL;
+
+	/* Copy the line from source and append NUL char  */
+	memcpy(dst, *psrc, len);
+	*(dst + len) = '\0';
+
+	*psrc = ptr;
+	*psrc_sz -= len;
+
+	/* Return number of read chars */
+	return len;
+}
+
+/**
+ * zl3073x_fw_component_alloc - Alloc structure to hold firmware component
+ * @size: size of buffer to store data
+ *
+ * Return: pointer to allocated component structure or NULL on error.
+ */
+static struct zl3073x_fw_component *
+zl3073x_fw_component_alloc(size_t size)
+{
+	struct zl3073x_fw_component *comp;
+
+	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
+	if (!comp)
+		return NULL;
+
+	comp->size = size;
+	comp->data = kzalloc(size, GFP_KERNEL);
+	if (!comp->data) {
+		kfree(comp);
+		return NULL;
+	}
+
+	return comp;
+}
+
+/**
+ * zl3073x_fw_component_free - Free allocated component structure
+ * @comp: pointer to allocated component
+ */
+static void
+zl3073x_fw_component_free(struct zl3073x_fw_component *comp)
+{
+	if (comp)
+		kfree(comp->data);
+
+	kfree(comp);
+}
+
+/**
+ * zl3073x_fw_component_id_get - Get ID for firmware component name
+ * @name: input firmware component name
+ *
+ * Return:
+ * - ZL3073X_FW_COMPONENT_* ID for known component name
+ * - ZL3073X_FW_COMPONENT_INVALID if the given name is unknown
+ */
+static enum zl3073x_fw_component_id
+zl3073x_fw_component_id_get(const char *name)
+{
+	enum zl3073x_fw_component_id id;
+
+	for (id = ZL_FW_COMPONENT_UTIL; id < ZL_FW_NUM_COMPONENTS; id++)
+		if (!strcasecmp(name, component_info[id].name))
+			return id;
+
+	return ZL_FW_COMPONENT_INVALID;
+}
+
+/**
+ * zl3073x_fw_component_load - Load component from firmware source
+ * @zldev: zl3073x device structure
+ * @pcomp: pointer to loaded component
+ * @psrc: data pointer to load component from
+ * @psize: remaining bytes in buffer
+ * @extack: netlink extack pointer to report errors
+ *
+ * The function allocates single firmware component and loads the data from
+ * the buffer specified by @psrc and @psize. Pointer to allocated component
+ * is stored in output @pcomp. Source data pointer @psrc and remaining bytes
+ * @psize are updated accordingly.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static ssize_t
+zl3073x_fw_component_load(struct zl3073x_dev *zldev,
+			  struct zl3073x_fw_component **pcomp,
+			  const char **psrc, size_t *psize,
+			  struct netlink_ext_ack *extack)
+{
+	const struct zl3073x_fw_component_info *info;
+	struct zl3073x_fw_component *comp = NULL;
+	struct device *dev = zldev->dev;
+	enum zl3073x_fw_component_id id;
+	ssize_t len, count;
+	u32 comp_size;
+	char line[32];
+	int rc;
+
+	/* Fetch image name from input */
+	len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
+	if (len < 0) {
+		rc = len;
+		goto err_unexpected;
+	} else if (!len) {
+		/* No more data */
+		return 0;
+	}
+
+	dev_dbg(dev, "Firmware component '%s' found\n", line);
+
+	id = zl3073x_fw_component_id_get(line);
+	if (id == ZL_FW_COMPONENT_INVALID) {
+		ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] unknown component type",
+				   line);
+		return -EINVAL;
+	}
+
+	info = &component_info[id];
+
+	/* Fetch image size from input */
+	len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
+	if (len < 0) {
+		rc = len;
+		goto err_unexpected;
+	} else if (!len) {
+		ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing size",
+				   info->name);
+		return -ENODATA;
+	}
+
+	rc = kstrtou32(line, 10, &comp_size);
+	if (rc) {
+		ZL3073X_FW_ERR_MSG(zldev, extack,
+				   "[%s] invalid size value '%s'", info->name,
+				   line);
+		return rc;
+	}
+
+	comp_size *= sizeof(u32); /* convert num of dwords to bytes */
+
+	/* Check image size validity */
+	if (comp_size > component_info[id].max_size) {
+		ZL3073X_FW_ERR_MSG(zldev, extack,
+				   "[%s] component is too big (%u bytes)\n",
+				   info->name, comp_size);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "Indicated component image size: %u bytes\n", comp_size);
+
+	/* Alloc component */
+	comp = zl3073x_fw_component_alloc(comp_size);
+	if (!comp) {
+		ZL3073X_FW_ERR_MSG(zldev, extack, "failed to alloc memory");
+		return -ENOMEM;
+	}
+	comp->id = id;
+
+	/* Load component data from firmware source */
+	for (count = 0; count < comp_size; count += 4) {
+		len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
+		if (len < 0) {
+			rc = len;
+			goto err_unexpected;
+		} else if (!len) {
+			ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing data",
+					   info->name);
+			rc = -ENODATA;
+			goto err;
+		}
+
+		rc = kstrtou32(line, 16, comp->data + count);
+		if (rc) {
+			ZL3073X_FW_ERR_MSG(zldev, extack,
+					   "[%s] invalid data: '%s'",
+					   info->name, line);
+			goto err;
+		}
+	}
+
+	*pcomp = comp;
+
+	return 1;
+
+err_unexpected:
+	ZL3073X_FW_ERR_MSG(zldev, extack, "unexpected input");
+err:
+	zl3073x_fw_component_free(comp);
+
+	return rc;
+}
+
+/**
+ * zl3073x_fw_free - Free allocated firmware
+ * @fw: firmware pointer
+ *
+ * The function frees existing firmware allocated by @zl3073x_fw_load.
+ */
+void zl3073x_fw_free(struct zl3073x_fw *fw)
+{
+	size_t i;
+
+	if (!fw)
+		return;
+
+	for (i = 0; i < ZL_FW_NUM_COMPONENTS; i++)
+		zl3073x_fw_component_free(fw->component[i]);
+
+	kfree(fw);
+}
+
+/**
+ * zl3073x_fw_load - Load all components from source
+ * @zldev: zl3073x device structure
+ * @data: source buffer pointer
+ * @size: size of source buffer
+ * @extack: netlink extack pointer to report errors
+ *
+ * The functions allocate firmware structure and loads all components from
+ * the given buffer specified by @data and @size.
+ *
+ * Return: pointer to firmware on success, error pointer on error
+ */
+struct zl3073x_fw *zl3073x_fw_load(struct zl3073x_dev *zldev, const char *data,
+				   size_t size, struct netlink_ext_ack *extack)
+{
+	struct zl3073x_fw_component *comp;
+	enum zl3073x_fw_component_id id;
+	struct zl3073x_fw *fw;
+	ssize_t rc;
+
+	/* Allocate firmware structure */
+	fw = kzalloc(sizeof(*fw), GFP_KERNEL);
+	if (!fw)
+		return ERR_PTR(-ENOMEM);
+
+	do {
+		/* Load single component */
+		rc = zl3073x_fw_component_load(zldev, &comp, &data, &size,
+					       extack);
+		if (rc <= 0)
+			/* Everything was read or error occurred */
+			break;
+
+		id = comp->id;
+
+		/* Report error if the given component is present twice
+		 * or more.
+		 */
+		if (fw->component[id]) {
+			ZL3073X_FW_ERR_MSG(zldev, extack,
+					   "duplicate component '%s' detected",
+					   component_info[id].name);
+			zl3073x_fw_component_free(comp);
+			rc = -EINVAL;
+			break;
+		}
+
+		fw->component[id] = comp;
+	} while (1);
+
+	if (rc) {
+		/* Free allocated firmware in case of error */
+		zl3073x_fw_free(fw);
+		return ERR_PTR(rc);
+	}
+
+	return fw;
+}
+
+/**
+ * zl3073x_flash_bundle_flash - Flash all components
+ * @zldev: zl3073x device structure
+ * @components: pointer to components array
+ * @extack: netlink extack pointer to report errors
+ *
+ * Returns 0 in case of success or negative number otherwise.
+ */
+static int
+zl3073x_fw_component_flash(struct zl3073x_dev *zldev,
+			   struct zl3073x_fw_component *comp,
+			   struct netlink_ext_ack *extack)
+{
+	const struct zl3073x_fw_component_info *info;
+	int rc;
+
+	info = &component_info[comp->id];
+
+	switch (info->flash_type) {
+	case ZL3073X_FLASH_TYPE_NONE:
+		/* Non-flashable component - used for utility */
+		return 0;
+	case ZL3073X_FLASH_TYPE_SECTORS:
+		rc = zl3073x_flash_sectors(zldev, info->name, info->dest_page,
+					   info->load_addr, comp->data,
+					   comp->size, extack);
+		break;
+	case ZL3073X_FLASH_TYPE_PAGE:
+		rc = zl3073x_flash_page(zldev, info->name, info->dest_page,
+					info->load_addr, comp->data, comp->size,
+					extack);
+		break;
+	case ZL3073X_FLASH_TYPE_PAGE_AND_COPY:
+		rc = zl3073x_flash_page(zldev, info->name, info->dest_page,
+					info->load_addr, comp->data, comp->size,
+					extack);
+		if (!rc)
+			rc = zl3073x_flash_page_copy(zldev, info->name,
+						     info->dest_page,
+						     info->copy_page, extack);
+		break;
+	}
+	if (rc)
+		ZL3073X_FW_ERR_MSG(zldev, extack,
+				   "Failed to flash component '%s'",
+				   info->name);
+
+	return rc;
+}
+
+int zl3073x_fw_flash(struct zl3073x_dev *zldev, struct zl3073x_fw *zlfw,
+		     struct netlink_ext_ack *extack)
+{
+	int i, rc = 0;
+
+	for (i = 0; i < ZL_FW_NUM_COMPONENTS; i++) {
+		if (!zlfw->component[i])
+			continue; /* Component is not present */
+
+		rc = zl3073x_fw_component_flash(zldev, zlfw->component[i],
+						extack);
+		if (rc)
+			break;
+	}
+
+	return rc;
+}
diff --git a/drivers/dpll/zl3073x/fw.h b/drivers/dpll/zl3073x/fw.h
new file mode 100644
index 0000000000000..242d3f9433a18
--- /dev/null
+++ b/drivers/dpll/zl3073x/fw.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _ZL3073X_FW_H
+#define _ZL3073X_FW_H
+
+/*
+ * enum zl3073x_fw_component_id - Identifiers for possible flash components
+ */
+enum zl3073x_fw_component_id {
+	ZL_FW_COMPONENT_INVALID = -1,
+	ZL_FW_COMPONENT_UTIL = 0,
+	ZL_FW_COMPONENT_FW1,
+	ZL_FW_COMPONENT_FW2,
+	ZL_FW_COMPONENT_FW3,
+	ZL_FW_COMPONENT_CFG0,
+	ZL_FW_COMPONENT_CFG1,
+	ZL_FW_COMPONENT_CFG2,
+	ZL_FW_COMPONENT_CFG3,
+	ZL_FW_COMPONENT_CFG4,
+	ZL_FW_COMPONENT_CFG5,
+	ZL_FW_COMPONENT_CFG6,
+	ZL_FW_NUM_COMPONENTS,
+};
+
+/**
+ * struct zl3073x_fw_component - Firmware component
+ * @id: Flash component ID
+ * @size: Size of the buffer
+ * @data: Pointer to buffer with component data
+ */
+struct zl3073x_fw_component {
+	enum zl3073x_fw_component_id	id;
+	size_t				size;
+	void				*data;
+};
+
+/**
+ * struct zl3073x_fw - Firmware bundle
+ * @component: firmware components array
+ */
+struct zl3073x_fw {
+	struct zl3073x_fw_component	*component[ZL_FW_NUM_COMPONENTS];
+};
+
+struct zl3073x_fw *zl3073x_fw_load(struct zl3073x_dev *zldev, const char *data,
+				   size_t size, struct netlink_ext_ack *extack);
+void zl3073x_fw_free(struct zl3073x_fw *fw);
+
+int zl3073x_fw_flash(struct zl3073x_dev *zldev, struct zl3073x_fw *zlfw,
+		     struct netlink_ext_ack *extack);
+
+#endif /* _ZL3073X_FW_H */
-- 
2.49.1

[PATCH net-next v2 4/5] dpll: zl3073x: Refactor DPLL initialization

[Ivan Vecera]

Refactor DPLL initialization and move DPLL (de)registration, monitoring
control, fetching device invariant parameters and phase offset
measurement block setup to separate functions.

Use these new functions during device probe and teardown functions and
during changes to the clock_id devlink parameter.

These functions will also be used in the next patch implementing devlink
flash, where this functionality is likewise required.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v2:
* fixed warning with uninitialized 'mask'
---
 drivers/dpll/zl3073x/core.c    | 207 +++++++++++++++++++++------------
 drivers/dpll/zl3073x/core.h    |   3 +
 drivers/dpll/zl3073x/devlink.c |  18 +--
 3 files changed, 142 insertions(+), 86 deletions(-)

diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index 86c26edc90462..e96095baac657 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -956,21 +956,142 @@ zl3073x_dev_periodic_work(struct kthread_work *work)
 				   msecs_to_jiffies(500));
 }
 
+/**
+ * zl3073x_dev_phase_meas_setup - setup phase offset measurement
+ * @zldev: pointer to zl3073x_dev structure
+ *
+ * Enable phase offset measurement block, set measurement averaging factor
+ * and enable DPLL-to-its-ref phase measurement for all DPLLs.
+ *
+ * Returns: 0 on success, <0 on error
+ */
+static int
+zl3073x_dev_phase_meas_setup(struct zl3073x_dev *zldev)
+{
+	struct zl3073x_dpll *zldpll;
+	u8 dpll_meas_ctrl, mask = 0;
+	int rc;
+
+	/* Read DPLL phase measurement control register */
+	rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, &dpll_meas_ctrl);
+	if (rc)
+		return rc;
+
+	/* Setup phase measurement averaging factor */
+	dpll_meas_ctrl &= ~ZL_DPLL_MEAS_CTRL_AVG_FACTOR;
+	dpll_meas_ctrl |= FIELD_PREP(ZL_DPLL_MEAS_CTRL_AVG_FACTOR, 3);
+
+	/* Enable DPLL measurement block */
+	dpll_meas_ctrl |= ZL_DPLL_MEAS_CTRL_EN;
+
+	/* Update phase measurement control register */
+	rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, dpll_meas_ctrl);
+	if (rc)
+		return rc;
+
+	/* Enable DPLL-to-connected-ref measurement for each channel */
+	list_for_each_entry(zldpll, &zldev->dplls, list)
+		mask |= BIT(zldpll->id);
+
+	return zl3073x_write_u8(zldev, ZL_REG_DPLL_PHASE_ERR_READ_MASK, mask);
+}
+
+/**
+ * zl3073x_dev_start - Start normal operation
+ * @zldev: zl3073x device pointer
+ * @full: perform full initialization
+ *
+ * The function starts normal operation, which means registering all DPLLs and
+ * their pins, and starting monitoring. If full initialization is requested,
+ * the function additionally initializes the phase offset measurement block and
+ * fetches hardware-invariant parameters.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_dev_start(struct zl3073x_dev *zldev, bool full)
+{
+	struct zl3073x_dpll *zldpll;
+	int rc;
+
+	if (full) {
+		/* Fetch device state */
+		rc = zl3073x_dev_state_fetch(zldev);
+		if (rc)
+			return rc;
+
+		/* Setup phase offset measurement block */
+		rc = zl3073x_dev_phase_meas_setup(zldev);
+		if (rc) {
+			dev_err(zldev->dev,
+				"Failed to setup phase measurement\n");
+			return rc;
+		}
+	}
+
+	/* Register all DPLLs */
+	list_for_each_entry(zldpll, &zldev->dplls, list) {
+		rc = zl3073x_dpll_register(zldpll);
+		if (rc) {
+			dev_err_probe(zldev->dev, rc,
+				      "Failed to register DPLL%u\n",
+				      zldpll->id);
+			return rc;
+		}
+	}
+
+	/* Perform initial firmware fine phase correction */
+	rc = zl3073x_dpll_init_fine_phase_adjust(zldev);
+	if (rc) {
+		dev_err_probe(zldev->dev, rc,
+			      "Failed to init fine phase correction\n");
+		return rc;
+	}
+
+	/* Start monitoring */
+	kthread_queue_delayed_work(zldev->kworker, &zldev->work, 0);
+
+	return 0;
+}
+
+/**
+ * zl3073x_dev_stop - Stop normal operation
+ * @zldev: zl3073x device pointer
+ *
+ * The function stops the normal operation that mean deregistration of all
+ * DPLLs and their pins and stop monitoring.
+ *
+ * Return: 0 on success, <0 on error
+ */
+void zl3073x_dev_stop(struct zl3073x_dev *zldev)
+{
+	struct zl3073x_dpll *zldpll;
+
+	/* Stop monitoring */
+	kthread_cancel_delayed_work_sync(&zldev->work);
+
+	/* Unregister all DPLLs */
+	list_for_each_entry(zldpll, &zldev->dplls, list) {
+		if (zldpll->dpll_dev)
+			zl3073x_dpll_unregister(zldpll);
+	}
+}
+
 static void zl3073x_dev_dpll_fini(void *ptr)
 {
 	struct zl3073x_dpll *zldpll, *next;
 	struct zl3073x_dev *zldev = ptr;
 
-	/* Stop monitoring thread */
+	/* Stop monitoring and unregister DPLLs */
+	zl3073x_dev_stop(zldev);
+
+	/* Destroy monitoring thread */
 	if (zldev->kworker) {
-		kthread_cancel_delayed_work_sync(&zldev->work);
 		kthread_destroy_worker(zldev->kworker);
 		zldev->kworker = NULL;
 	}
 
-	/* Release DPLLs */
+	/* Free all DPLLs */
 	list_for_each_entry_safe(zldpll, next, &zldev->dplls, list) {
-		zl3073x_dpll_unregister(zldpll);
 		list_del(&zldpll->list);
 		zl3073x_dpll_free(zldpll);
 	}
@@ -986,7 +1107,7 @@ zl3073x_devm_dpll_init(struct zl3073x_dev *zldev, u8 num_dplls)
 
 	INIT_LIST_HEAD(&zldev->dplls);
 
-	/* Initialize all DPLLs */
+	/* Allocate all DPLLs */
 	for (i = 0; i < num_dplls; i++) {
 		zldpll = zl3073x_dpll_alloc(zldev, i);
 		if (IS_ERR(zldpll)) {
@@ -996,25 +1117,9 @@ zl3073x_devm_dpll_init(struct zl3073x_dev *zldev, u8 num_dplls)
 			goto error;
 		}
 
-		rc = zl3073x_dpll_register(zldpll);
-		if (rc) {
-			dev_err_probe(zldev->dev, rc,
-				      "Failed to register DPLL%u\n", i);
-			zl3073x_dpll_free(zldpll);
-			goto error;
-		}
-
 		list_add_tail(&zldpll->list, &zldev->dplls);
 	}
 
-	/* Perform initial firmware fine phase correction */
-	rc = zl3073x_dpll_init_fine_phase_adjust(zldev);
-	if (rc) {
-		dev_err_probe(zldev->dev, rc,
-			      "Failed to init fine phase correction\n");
-		goto error;
-	}
-
 	/* Initialize monitoring thread */
 	kthread_init_delayed_work(&zldev->work, zl3073x_dev_periodic_work);
 	kworker = kthread_run_worker(0, "zl3073x-%s", dev_name(zldev->dev));
@@ -1022,9 +1127,14 @@ zl3073x_devm_dpll_init(struct zl3073x_dev *zldev, u8 num_dplls)
 		rc = PTR_ERR(kworker);
 		goto error;
 	}
-
 	zldev->kworker = kworker;
-	kthread_queue_delayed_work(zldev->kworker, &zldev->work, 0);
+
+	/* Start normal operation */
+	rc = zl3073x_dev_start(zldev, true);
+	if (rc) {
+		dev_err_probe(zldev->dev, rc, "Failed to start device\n");
+		goto error;
+	}
 
 	/* Add devres action to release DPLL related resources */
 	rc = devm_add_action_or_reset(zldev->dev, zl3073x_dev_dpll_fini, zldev);
@@ -1039,46 +1149,6 @@ zl3073x_devm_dpll_init(struct zl3073x_dev *zldev, u8 num_dplls)
 	return rc;
 }
 
-/**
- * zl3073x_dev_phase_meas_setup - setup phase offset measurement
- * @zldev: pointer to zl3073x_dev structure
- * @num_channels: number of DPLL channels
- *
- * Enable phase offset measurement block, set measurement averaging factor
- * and enable DPLL-to-its-ref phase measurement for all DPLLs.
- *
- * Returns: 0 on success, <0 on error
- */
-static int
-zl3073x_dev_phase_meas_setup(struct zl3073x_dev *zldev, int num_channels)
-{
-	u8 dpll_meas_ctrl, mask;
-	int i, rc;
-
-	/* Read DPLL phase measurement control register */
-	rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, &dpll_meas_ctrl);
-	if (rc)
-		return rc;
-
-	/* Setup phase measurement averaging factor */
-	dpll_meas_ctrl &= ~ZL_DPLL_MEAS_CTRL_AVG_FACTOR;
-	dpll_meas_ctrl |= FIELD_PREP(ZL_DPLL_MEAS_CTRL_AVG_FACTOR, 3);
-
-	/* Enable DPLL measurement block */
-	dpll_meas_ctrl |= ZL_DPLL_MEAS_CTRL_EN;
-
-	/* Update phase measurement control register */
-	rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, dpll_meas_ctrl);
-	if (rc)
-		return rc;
-
-	/* Enable DPLL-to-connected-ref measurement for each channel */
-	for (i = 0, mask = 0; i < num_channels; i++)
-		mask |= BIT(i);
-
-	return zl3073x_write_u8(zldev, ZL_REG_DPLL_PHASE_ERR_READ_MASK, mask);
-}
-
 /**
  * zl3073x_dev_probe - initialize zl3073x device
  * @zldev: pointer to zl3073x device
@@ -1146,17 +1216,6 @@ int zl3073x_dev_probe(struct zl3073x_dev *zldev,
 		return dev_err_probe(zldev->dev, rc,
 				     "Failed to initialize mutex\n");
 
-	/* Fetch device state */
-	rc = zl3073x_dev_state_fetch(zldev);
-	if (rc)
-		return rc;
-
-	/* Setup phase offset measurement block */
-	rc = zl3073x_dev_phase_meas_setup(zldev, chip_info->num_channels);
-	if (rc)
-		return dev_err_probe(zldev->dev, rc,
-				     "Failed to setup phase measurement\n");
-
 	/* Register DPLL channels */
 	rc = zl3073x_devm_dpll_init(zldev, chip_info->num_channels);
 	if (rc)
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index 16e750d77e1dd..128fb899cafc3 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -112,6 +112,9 @@ struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev);
 int zl3073x_dev_probe(struct zl3073x_dev *zldev,
 		      const struct zl3073x_chip_info *chip_info);
 
+int zl3073x_dev_start(struct zl3073x_dev *zldev, bool full);
+void zl3073x_dev_stop(struct zl3073x_dev *zldev);
+
 /**********************
  * Registers operations
  **********************/
diff --git a/drivers/dpll/zl3073x/devlink.c b/drivers/dpll/zl3073x/devlink.c
index f3ca973a4d416..d0f6d9cd4a68e 100644
--- a/drivers/dpll/zl3073x/devlink.c
+++ b/drivers/dpll/zl3073x/devlink.c
@@ -86,14 +86,12 @@ zl3073x_devlink_reload_down(struct devlink *devlink, bool netns_change,
 			    struct netlink_ext_ack *extack)
 {
 	struct zl3073x_dev *zldev = devlink_priv(devlink);
-	struct zl3073x_dpll *zldpll;
 
 	if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT)
 		return -EOPNOTSUPP;
 
-	/* Unregister all DPLLs */
-	list_for_each_entry(zldpll, &zldev->dplls, list)
-		zl3073x_dpll_unregister(zldpll);
+	/* Stop normal operation */
+	zl3073x_dev_stop(zldev);
 
 	return 0;
 }
@@ -107,7 +105,6 @@ zl3073x_devlink_reload_up(struct devlink *devlink,
 {
 	struct zl3073x_dev *zldev = devlink_priv(devlink);
 	union devlink_param_value val;
-	struct zl3073x_dpll *zldpll;
 	int rc;
 
 	if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT)
@@ -125,13 +122,10 @@ zl3073x_devlink_reload_up(struct devlink *devlink,
 		zldev->clock_id = val.vu64;
 	}
 
-	/* Re-register all DPLLs */
-	list_for_each_entry(zldpll, &zldev->dplls, list) {
-		rc = zl3073x_dpll_register(zldpll);
-		if (rc)
-			dev_warn(zldev->dev,
-				 "Failed to re-register DPLL%u\n", zldpll->id);
-	}
+	/* Restart normal operation */
+	rc = zl3073x_dev_start(zldev, false);
+	if (rc)
+		dev_warn(zldev->dev, "Failed to re-start normal operation\n");
 
 	*actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT);
 
-- 
2.49.1

[PATCH net-next v2 5/5] dpll: zl3073x: Implement devlink flash callback

[Ivan Vecera]

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>
---
 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..06962643c9363 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
+ *
+ * Returns 0 in case of success or negative value otherwise
+ */
+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

Re: [PATCH net-next v2 5/5] dpll: zl3073x: Implement devlink flash callback

[Randy Dunlap]

Hi--

On 8/11/25 7:40 AM, Ivan Vecera wrote:
> +/**
> + * zl3073x_flash_update - Devlink flash update callback
> + * @devlink: devlink structure pointer
> + * @params: flashing parameters pointer
> + * @extack: netlink extack pointer to report errors
> + *
> + * Returns 0 in case of success or negative value otherwise

Preferably with a colon:

    * Returns: 0 in case of success or negative value otherwise


> + */
> +static int
> +zl3073x_devlink_flash_update(struct devlink *devlink,
> +			     struct devlink_flash_update_params *params,
> +			     struct netlink_ext_ack *extack)
> +{
Thanks.
-- 
~Randy

Re: [PATCH net-next v2 2/5] dpll: zl3073x: Add low-level flash functions

[Przemek Kitszel]

On 8/11/25 16:40, Ivan Vecera wrote:
> To implement the devlink device flash functionality, the driver needs
> to access both the device memory and the internal flash memory. The flash
> memory is accessed using a device-specific program (called the flash
> utility). This flash utility must be downloaded by the driver into
> the device memory and then executed by the device CPU. Once running,
> the flash utility provides a flash API to access the flash memory itself.
> 
> During this operation, the normal functionality provided by the standard
> firmware is not available. Therefore, the driver must ensure that DPLL
> callbacks and monitoring functions are not executed during the flash
> operation.
> 
> Add all necessary functions for downloading the utility to device memory,
> entering and exiting flash mode, and performing flash operations.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> v2:
> * extended 'comp_str' to 32 chars to avoid warnings related to snprintf
> * added additional includes
> ---
>   drivers/dpll/zl3073x/Makefile  |   2 +-
>   drivers/dpll/zl3073x/devlink.c |   9 +
>   drivers/dpll/zl3073x/devlink.h |   3 +
>   drivers/dpll/zl3073x/flash.c   | 684 +++++++++++++++++++++++++++++++++
>   drivers/dpll/zl3073x/flash.h   |  29 ++
>   drivers/dpll/zl3073x/regs.h    |  39 ++
>   6 files changed, 765 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/dpll/zl3073x/flash.c
>   create mode 100644 drivers/dpll/zl3073x/flash.h


> +static int
> +zl3073x_flash_download(struct zl3073x_dev *zldev, const char *component,
> +		       u32 addr, const void *data, size_t size,
> +		       struct netlink_ext_ack *extack)
> +{
> +#define CHECK_DELAY	5000 /* Check for interrupt each 5 seconds */

nit: please add ZL_ prefix

> +	unsigned long timeout;
> +	const void *ptr, *end;
> +	int rc = 0;
> +
> +	dev_dbg(zldev->dev, "Downloading %zu bytes to device memory at 0x%0x\n",
> +		size, addr);
> +
> +	timeout = jiffies + msecs_to_jiffies(CHECK_DELAY);
> +
> +	for (ptr = data, end = data + size; ptr < end; ptr += 4, addr += 4) {
> +		/* Write current word to HW memory */
> +		rc = zl3073x_write_hwreg(zldev, addr, *(const u32 *)ptr);
> +		if (rc) {
> +			ZL_FLASH_ERR_MSG(zldev, extack,
> +					 "failed to write to memory at 0x%0x",
> +					 addr);
> +			return rc;
> +		}
> +
> +		/* Check for pending interrupt each 5 seconds */

nit: comment seems too trivial (and ~repeats the above one)

> +		if (time_after(jiffies, timeout)) {
> +			if (signal_pending(current)) {
> +				ZL_FLASH_ERR_MSG(zldev, extack,
> +						 "Flashing interrupted");
> +				return -EINTR;
> +			}
> +
> +			timeout = jiffies + msecs_to_jiffies(CHECK_DELAY);
> +		}
> +
> +		/* Report status each 1 kB block */
> +		if ((ptr - data) % 1024 == 0)
> +			zl3073x_devlink_flash_notify(zldev, "Downloading image",
> +						     component, ptr - data,
> +						     size);
> +	}
> +
> +	zl3073x_devlink_flash_notify(zldev, "Downloading image", component,
> +				     ptr - data, size);
> +
> +	dev_dbg(zldev->dev, "%zu bytes downloaded to device memory\n", size);
> +
> +	return rc;
> +}
> +


> +/**
> + * zl3073x_flash_wait_ready - Check or wait for utility to be ready to flash
> + * @zldev: zl3073x device structure
> + * @timeout_ms: timeout for the waiting
> + *
> + * Return: 0 on success, <0 on error
> + */
> +static int
> +zl3073x_flash_wait_ready(struct zl3073x_dev *zldev, unsigned int timeout_ms)
> +{
> +#define ZL_FLASH_POLL_DELAY_MS	100
> +	unsigned long timeout;
> +	int rc, i;
> +
> +	dev_dbg(zldev->dev, "Waiting for flashing to be ready\n");
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);

this is duplicated in the loop init below

> +
> +	for (i = 0, timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	     time_before(jiffies, timeout);
> +	     i++) {
> +		u8 value;
> +
> +		/* Check for interrupt each 1s */
> +		if (i > 9) {
> +			if (signal_pending(current))
> +				return -EINTR;
> +			i = 0;
> +		}
> +
> +		/* Read write_flash register value */
> +		rc = zl3073x_read_u8(zldev, ZL_REG_WRITE_FLASH, &value);
> +		if (rc)
> +			return rc;
> +
> +		value = FIELD_GET(ZL_WRITE_FLASH_OP, value);
> +
> +		/* Check if the current operation was done */
> +		if (value == ZL_WRITE_FLASH_OP_DONE)
> +			return 0; /* Operation was successfully done */
> +
> +		msleep(ZL_FLASH_POLL_DELAY_MS);

nit: needless sleep in the very last iteration step
(a very minor issue with timeouts in range of minutes ;P)

> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +/**
> + * zl3073x_flash_cmd_wait - Perform flash operation and wait for finish
> + * @zldev: zl3073x device structure
> + * @operation: operation to perform
> + * @extack: netlink extack pointer to report errors
> + *
> + * Return: 0 on success, <0 on error
> + */
> +static int
> +zl3073x_flash_cmd_wait(struct zl3073x_dev *zldev, u32 operation,
> +		       struct netlink_ext_ack *extack)
> +{
> +#define FLASH_PHASE1_TIMEOUT_MS 60000	/* up to 1 minute */
> +#define FLASH_PHASE2_TIMEOUT_MS 120000	/* up to 2 minutes */

nit: missing prefixes

> +	u8 value;
> +	int rc;
> +
> +	dev_dbg(zldev->dev, "Sending flash command: 0x%x\n", operation);
> +
> +	/* Wait for access */
> +	rc = zl3073x_flash_wait_ready(zldev, FLASH_PHASE1_TIMEOUT_MS);
> +	if (rc)
> +		return rc;
> +
> +	/* Issue the requested operation */
> +	rc = zl3073x_read_u8(zldev, ZL_REG_WRITE_FLASH, &value);
> +	if (rc)
> +		return rc;
> +
> +	value &= ~ZL_WRITE_FLASH_OP;
> +	value |= FIELD_PREP(ZL_WRITE_FLASH_OP, operation);
> +
> +	rc = zl3073x_write_u8(zldev, ZL_REG_WRITE_FLASH, value);
> +	if (rc)
> +		return rc;
> +
> +	/* Wait for command completion */
> +	rc = zl3073x_flash_wait_ready(zldev, FLASH_PHASE2_TIMEOUT_MS);
> +	if (rc)
> +		return rc;
> +
> +	/* Check for utility errors */
> +	return zl3073x_flash_error_check(zldev, extack);
> +}
> +
> +/**
> + * zl3073x_flash_get_sector_size - Get flash sector size
> + * @zldev: zl3073x device structure
> + * @sector_size: sector size returned by the function
> + *
> + * The function reads the flash sector size detected by flash utility and
> + * stores it into @sector_size.
> + *
> + * Return: 0 on success, <0 on error
> + */
> +static int
> +zl3073x_flash_get_sector_size(struct zl3073x_dev *zldev, size_t *sector_size)
> +{
> +	u8 flash_info;
> +	int rc;
> +
> +	rc = zl3073x_read_u8(zldev, ZL_REG_FLASH_INFO, &flash_info);
> +	if (rc)
> +		return rc;
> +
> +	switch (FIELD_GET(ZL_FLASH_INFO_SECTOR_SIZE, flash_info)) {
> +	case ZL_FLASH_INFO_SECTOR_4K:
> +		*sector_size = 0x1000;
> +		break;
> +	case ZL_FLASH_INFO_SECTOR_64K:
> +		*sector_size = 0x10000;

nit: up to you, but I would like to see SZ_64K instead
(and don't count zeroes), if so, SZ_4K for the above too

> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * zl3073x_flash_sectors - Flash sectors
> + * @zldev: zl3073x device structure
> + * @component: component name
> + * @page: destination flash page
> + * @addr: device memory address to load data
> + * @data: pointer to data to be flashed
> + * @size: size of data
> + * @extack: netlink extack pointer to report errors
> + *
> + * The function flashes given @data with size of @size to the internal flash
> + * memory block starting from page @page. The function uses sector flash
> + * method and has to take into account the flash sector size reported by
> + * flashing utility. Input data are spliced into blocks according this
> + * sector size and each block is flashed separately.
> + *
> + * Return: 0 on success, <0 on error
> + */
> +int zl3073x_flash_sectors(struct zl3073x_dev *zldev, const char *component,
> +			  u32 page, u32 addr, const void *data, size_t size,
> +			  struct netlink_ext_ack *extack)
> +{
> +#define ZL_FLASH_MAX_BLOCK_SIZE	0x0001E000
> +#define ZL_FLASH_PAGE_SIZE	256
> +	size_t max_block_size, block_size, sector_size;
> +	const void *ptr, *end;
> +	int rc;
> +
> +	/* Get flash sector size */
> +	rc = zl3073x_flash_get_sector_size(zldev, &sector_size);
> +	if (rc) {
> +		ZL_FLASH_ERR_MSG(zldev, extack,
> +				 "Failed to get flash sector size");
> +		return rc;
> +	}
> +
> +	/* Determine max block size depending on sector size */
> +	max_block_size = ALIGN_DOWN(ZL_FLASH_MAX_BLOCK_SIZE, sector_size);
> +
> +	for (ptr = data, end = data + size; ptr < end; ptr += block_size) {

block_size is uninitialized on the first loop iteration

> +		char comp_str[32];
> +
> +		block_size = min_t(size_t, max_block_size, end - ptr);
> +
> +		/* Add suffix '-partN' if the requested component size is
> +		 * greater than max_block_size.
> +		 */
> +		if (max_block_size < size)
> +			snprintf(comp_str, sizeof(comp_str), "%s-part%zu",
> +				 component, (ptr - data) / max_block_size + 1);
> +		else
> +			strscpy(comp_str, component);
> +
> +		/* Download block to device memory */
> +		rc = zl3073x_flash_download(zldev, comp_str, addr, ptr,
> +					    block_size, extack);
> +		if (rc)
> +			goto finish;
> +
> +		/* Set address to flash from */
> +		rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_START_ADDR, addr);
> +		if (rc)
> +			goto finish;
> +
> +		/* Set size of block to flash */
> +		rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_SIZE, block_size);
> +		if (rc)
> +			goto finish;
> +
> +		/* Set destination page to flash */
> +		rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, page);
> +		if (rc)
> +			goto finish;
> +
> +		/* Set filling pattern */
> +		rc = zl3073x_write_u32(zldev, ZL_REG_FILL_PATTERN, U32_MAX);
> +		if (rc)
> +			goto finish;
> +
> +		zl3073x_devlink_flash_notify(zldev, "Flashing image", comp_str,
> +					     0, 0);
> +
> +		dev_dbg(zldev->dev, "Flashing %zu bytes to page %u\n",
> +			block_size, page);
> +
> +		/* Execute sectors flash operation */
> +		rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_SECTORS,
> +					    extack);
> +		if (rc)
> +			goto finish;
> +
> +		/* Move to next page */
> +		page += block_size / ZL_FLASH_PAGE_SIZE;
> +	}
> +
> +finish:
> +	zl3073x_devlink_flash_notify(zldev,
> +				     rc ?  "Flashing failed" : "Flashing done",
> +				     component, 0, 0);
> +
> +	return rc;
> +}
> +
> +/**
> + * zl3073x_flash_page - Flash page
> + * @zldev: zl3073x device structure
> + * @component: component name
> + * @page: destination flash page
> + * @addr: device memory address to load data
> + * @data: pointer to data to be flashed
> + * @size: size of data
> + * @extack: netlink extack pointer to report errors
> + *
> + * The function flashes given @data with size of @size to the internal flash
> + * memory block starting with page @page.
> + *
> + * Return: 0 on success, <0 on error
> + */
> +int zl3073x_flash_page(struct zl3073x_dev *zldev, const char *component,
> +		       u32 page, u32 addr, const void *data, size_t size,
> +		       struct netlink_ext_ack *extack)
> +{

looks like a canditate to use zl3073x_flash_sectors(), or make
a higher-level helper that will do heavy-lifting for
zl3073x_flash_sectors() and zl3073x_flash_page()
(especially that you did such great job with low-level helpers)

> +	int rc;
> +
> +	/* Download component to device memory */
> +	rc = zl3073x_flash_download(zldev, component, addr, data, size, extack);
> +	if (rc)
> +		goto finish;
> +
> +	/* Set address to flash from */
> +	rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_START_ADDR, addr);
> +	if (rc)
> +		goto finish;
> +
> +	/* Set size of block to flash */
> +	rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_SIZE, size);
> +	if (rc)
> +		goto finish;
> +
> +	/* Set destination page to flash */
> +	rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, page);
> +	if (rc)
> +		goto finish;
> +
> +	/* Set filling pattern */
> +	rc = zl3073x_write_u32(zldev, ZL_REG_FILL_PATTERN, U32_MAX);
> +	if (rc)
> +		goto finish;
> +
> +	zl3073x_devlink_flash_notify(zldev, "Flashing image", component, 0,
> +				     size);
> +
> +	/* Execute sectors flash operation */
> +	rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_PAGE, extack);
> +	if (rc)
> +		goto finish;
> +
> +	zl3073x_devlink_flash_notify(zldev, "Flashing image", component, size,
> +				     size);
> +
> +finish:
> +	zl3073x_devlink_flash_notify(zldev,
> +				     rc ?  "Flashing failed" : "Flashing done",
> +				     component, 0, 0);
> +
> +	return rc;
> +}


> +
> +static int
> +zl3073x_flash_host_ctrl_enable(struct zl3073x_dev *zldev)
> +{
> +	u8 host_ctrl;
> +	int rc;
> +
> +	/* Read host control register */
> +	rc = zl3073x_read_u8(zldev, ZL_REG_HOST_CONTROL, &host_ctrl);
> +	if (rc)
> +		return rc;
> +
> +	/* Enable host control */
> +	host_ctrl &= ~ZL_HOST_CONTROL_ENABLE;

suspicious, as this line does nothing (in the context of the next one)

> +	host_ctrl |= ZL_HOST_CONTROL_ENABLE;
> +
> +	/* Update host control register */
> +	return zl3073x_write_u8(zldev, ZL_REG_HOST_CONTROL, host_ctrl);
> +}
> +
> +/**
> + * zl3073x_flash_mode_enter - Switch the device to flash mode
> + * @zldev: zl3073x device structure
> + * @util_ptr: buffer with flash utility
> + * @util_size: size of buffer with flash utility
> + * @extack: netlink extack pointer to report errors
> + *
> + * The function prepares and switches the device into flash mode.
> + *
> + * The procedure:
> + * 1) Stop device CPU by specific HW register sequence
> + * 2) Download flash utility to device memory
> + * 3) Resume device CPU by specific HW register sequence
> + * 4) Check communication with flash utility
> + * 5) Enable host control necessary to access flash API
> + * 6) Check for potential error detected by the utility
> + *
> + * The API provided by normal firmware is not available in flash mode
> + * so the caller has to ensure that this API is not used in this mode.
> + *
> + * After performing flash operation the caller should call
> + * @zl3073x_flash_mode_leave to return back to normal operation.
> + *
> + * Return: 0 on success, <0 on error.
> + */
> +int zl3073x_flash_mode_enter(struct zl3073x_dev *zldev, const void *util_ptr,
> +			     size_t util_size, struct netlink_ext_ack *extack)
> +{
> +	/* Sequence to be written prior utility download */
> +	static const struct zl3073x_hwreg_seq_item pre_seq[] = {
> +		HWREG_SEQ_ITEM(0x80000400, 1, BIT(0), 0),
> +		HWREG_SEQ_ITEM(0x80206340, 1, BIT(4), 0),
> +		HWREG_SEQ_ITEM(0x10000000, 1, BIT(2), 0),
> +		HWREG_SEQ_ITEM(0x10000024, 0x00000001, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10000020, 0x00000001, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10000000, 1, BIT(10), 1000),
> +	};
> +	/* Sequence to be written after utility download */
> +	static const struct zl3073x_hwreg_seq_item post_seq[] = {
> +		HWREG_SEQ_ITEM(0x10400004, 0x000000C0, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10400008, 0x00000000, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10400010, 0x20000000, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10400014, 0x20000004, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10000000, 1, GENMASK(10, 9), 0),
> +		HWREG_SEQ_ITEM(0x10000020, 0x00000000, U32_MAX, 0),
> +		HWREG_SEQ_ITEM(0x10000000, 0, BIT(0), 1000),
> +	};
very nice code

Re: [PATCH net-next v2 3/5] dpll: zl3073x: Add firmware loading functionality

[Przemek Kitszel]

On 8/11/25 16:40, Ivan Vecera wrote:
> Add functionality for loading firmware files provided by the vendor
> to be flashed into the device's internal flash memory. The firmware
> consists of several components, such as the firmware executable itself,
> chip-specific customizations, and configuration files.
> 
> The firmware file contains at least a flash utility, which is executed
> on the device side, and one or more flashable components. Each component
> has its own specific properties, such as the address where it should be
> loaded during flashing, one or more destination flash pages, and
> the flashing method that should be used.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> v2:
> * added additional includes
> * removed empty line
> * '*(dst+len)' -> '*(dst + len)'
> * 'Santity' -> 'Sanity'
> * fixed smatch warning about uninitialized 'rc'
> ---
>   drivers/dpll/zl3073x/Makefile |   2 +-
>   drivers/dpll/zl3073x/fw.c     | 498 ++++++++++++++++++++++++++++++++++
>   drivers/dpll/zl3073x/fw.h     |  52 ++++
>   3 files changed, 551 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/dpll/zl3073x/fw.c
>   create mode 100644 drivers/dpll/zl3073x/fw.h
> 

overview:
I don't like zl3073x_fw_readline() and it's usage - sscanf will do IMO

please find other feedback inline

> +/* Sanity check */
> +static_assert(ARRAY_SIZE(component_info) == ZL_FW_NUM_COMPONENTS);
> +
> +/**
> + * zl3073x_fw_readline - Read next line from firmware
> + * @dst: destination buffer
> + * @dst_sz: destination buffer size
> + * @psrc: source buffer
> + * @psrc_sz: source buffer size
> + *
> + * The function read next line from the firmware buffer specified by @psrc
> + * and @psrc_sz and stores it into buffer specified by @dst and @dst_sz.
> + * The pointer @psrc and remaining bytes in @psrc_sz are updated accordingly.
> + *
> + * Return: number of characters read on success, -EINVAL on error
> + */
> +static ssize_t
> +zl3073x_fw_readline(char *dst, size_t dst_sz, const char **psrc,
> +		    size_t *psrc_sz)
> +{
> +	const char *ptr = *psrc;
> +	size_t len;
> +
> +	/* Skip any existing new-lines at the beginning */
> +	ptr = memchr_inv(*psrc, '\n', *psrc_sz);
> +	if (ptr) {
> +		*psrc_sz -= ptr - *psrc;
> +		*psrc = ptr;
> +	}
> +
> +	/* Now look for the next new-line in the source */
> +	ptr = memscan((void *)*psrc, '\n', *psrc_sz);
> +	len = ptr - *psrc;
> +
> +	/* Return error if the source line is too long for destination */
> +	if (len >= dst_sz)
> +		return -EINVAL;
> +
> +	/* Copy the line from source and append NUL char  */
> +	memcpy(dst, *psrc, len);
> +	*(dst + len) = '\0';
> +
> +	*psrc = ptr;
> +	*psrc_sz -= len;
> +
> +	/* Return number of read chars */
> +	return len;
> +}
> +
> +/**
> + * zl3073x_fw_component_alloc - Alloc structure to hold firmware component
> + * @size: size of buffer to store data
> + *
> + * Return: pointer to allocated component structure or NULL on error.
> + */
> +static struct zl3073x_fw_component *
> +zl3073x_fw_component_alloc(size_t size)
> +{
> +	struct zl3073x_fw_component *comp;
> +
> +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
> +	if (!comp)
> +		return NULL;
> +
> +	comp->size = size;
> +	comp->data = kzalloc(size, GFP_KERNEL);
> +	if (!comp->data) {
> +		kfree(comp);
> +		return NULL;
> +	}
> +
> +	return comp;
> +}
> +
> +/**
> + * zl3073x_fw_component_free - Free allocated component structure
> + * @comp: pointer to allocated component
> + */
> +static void
> +zl3073x_fw_component_free(struct zl3073x_fw_component *comp)
> +{
> +	if (comp)
> +		kfree(comp->data);
> +
> +	kfree(comp);
> +}
> +
> +/**
> + * zl3073x_fw_component_id_get - Get ID for firmware component name
> + * @name: input firmware component name
> + *
> + * Return:
> + * - ZL3073X_FW_COMPONENT_* ID for known component name
> + * - ZL3073X_FW_COMPONENT_INVALID if the given name is unknown
> + */
> +static enum zl3073x_fw_component_id
> +zl3073x_fw_component_id_get(const char *name)
> +{
> +	enum zl3073x_fw_component_id id;
> +
> +	for (id = ZL_FW_COMPONENT_UTIL; id < ZL_FW_NUM_COMPONENTS; id++)

I would type the start as "id = 0"
(as you did in other functions, eg zl3073x_fw_free())

> +		if (!strcasecmp(name, component_info[id].name))
> +			return id;
> +
> +	return ZL_FW_COMPONENT_INVALID;
> +}
> +
> +/**
> + * zl3073x_fw_component_load - Load component from firmware source
> + * @zldev: zl3073x device structure
> + * @pcomp: pointer to loaded component
> + * @psrc: data pointer to load component from
> + * @psize: remaining bytes in buffer
> + * @extack: netlink extack pointer to report errors
> + *
> + * The function allocates single firmware component and loads the data from
> + * the buffer specified by @psrc and @psize. Pointer to allocated component
> + * is stored in output @pcomp. Source data pointer @psrc and remaining bytes
> + * @psize are updated accordingly.
> + *
> + * Return: 0 on success, <0 on error

document return of 1

> + */
> +static ssize_t
> +zl3073x_fw_component_load(struct zl3073x_dev *zldev,
> +			  struct zl3073x_fw_component **pcomp,
> +			  const char **psrc, size_t *psize,
> +			  struct netlink_ext_ack *extack)
> +{
> +	const struct zl3073x_fw_component_info *info;
> +	struct zl3073x_fw_component *comp = NULL;
> +	struct device *dev = zldev->dev;
> +	enum zl3073x_fw_component_id id;
> +	ssize_t len, count;
> +	u32 comp_size;
> +	char line[32];
> +	int rc;
> +
> +	/* Fetch image name from input */
> +	len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
> +	if (len < 0) {
> +		rc = len;
> +		goto err_unexpected;
> +	} else if (!len) {
> +		/* No more data */
> +		return 0;
> +	}
> +
> +	dev_dbg(dev, "Firmware component '%s' found\n", line);
> +
> +	id = zl3073x_fw_component_id_get(line);
> +	if (id == ZL_FW_COMPONENT_INVALID) {
> +		ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] unknown component type",
> +				   line);
> +		return -EINVAL;
> +	}
> +
> +	info = &component_info[id];
> +
> +	/* Fetch image size from input */
> +	len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
> +	if (len < 0) {
> +		rc = len;
> +		goto err_unexpected;
> +	} else if (!len) {
> +		ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing size",
> +				   info->name);
> +		return -ENODATA;
> +	}
> +
> +	rc = kstrtou32(line, 10, &comp_size);
> +	if (rc) {
> +		ZL3073X_FW_ERR_MSG(zldev, extack,
> +				   "[%s] invalid size value '%s'", info->name,
> +				   line);
> +		return rc;
> +	}

why not sscanf()? it would greatly simplify the above, and likely you
could entriely remove zl3073x_fw_readline() too

> +
> +	comp_size *= sizeof(u32); /* convert num of dwords to bytes */
> +
> +	/* Check image size validity */
> +	if (comp_size > component_info[id].max_size) {
> +		ZL3073X_FW_ERR_MSG(zldev, extack,
> +				   "[%s] component is too big (%u bytes)\n",
> +				   info->name, comp_size);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "Indicated component image size: %u bytes\n", comp_size);
> +
> +	/* Alloc component */
> +	comp = zl3073x_fw_component_alloc(comp_size);
> +	if (!comp) {
> +		ZL3073X_FW_ERR_MSG(zldev, extack, "failed to alloc memory");
> +		return -ENOMEM;
> +	}
> +	comp->id = id;
> +
> +	/* Load component data from firmware source */
> +	for (count = 0; count < comp_size; count += 4) {
> +		len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
> +		if (len < 0) {
> +			rc = len;
> +			goto err_unexpected;
> +		} else if (!len) {
> +			ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing data",
> +					   info->name);
> +			rc = -ENODATA;
> +			goto err;
> +		}
> +
> +		rc = kstrtou32(line, 16, comp->data + count);
> +		if (rc) {
> +			ZL3073X_FW_ERR_MSG(zldev, extack,
> +					   "[%s] invalid data: '%s'",
> +					   info->name, line);
> +			goto err;
> +		}
> +	}
> +
> +	*pcomp = comp;
> +
> +	return 1;> +
> +err_unexpected:
> +	ZL3073X_FW_ERR_MSG(zldev, extack, "unexpected input");
> +err:
> +	zl3073x_fw_component_free(comp);
> +
> +	return rc;
> +}
> +
> +/**
> + * zl3073x_fw_free - Free allocated firmware
> + * @fw: firmware pointer
> + *
> + * The function frees existing firmware allocated by @zl3073x_fw_load.
> + */
> +void zl3073x_fw_free(struct zl3073x_fw *fw)
> +{
> +	size_t i;
> +
> +	if (!fw)
> +		return;
> +
> +	for (i = 0; i < ZL_FW_NUM_COMPONENTS; i++)
> +		zl3073x_fw_component_free(fw->component[i]);
> +
> +	kfree(fw);
> +}
> +
> +/**
> + * zl3073x_fw_load - Load all components from source
> + * @zldev: zl3073x device structure
> + * @data: source buffer pointer
> + * @size: size of source buffer
> + * @extack: netlink extack pointer to report errors
> + *
> + * The functions allocate firmware structure and loads all components from
> + * the given buffer specified by @data and @size.
> + *
> + * Return: pointer to firmware on success, error pointer on error
> + */
> +struct zl3073x_fw *zl3073x_fw_load(struct zl3073x_dev *zldev, const char *data,
> +				   size_t size, struct netlink_ext_ack *extack)
> +{
> +	struct zl3073x_fw_component *comp;
> +	enum zl3073x_fw_component_id id;
> +	struct zl3073x_fw *fw;
> +	ssize_t rc;
> +
> +	/* Allocate firmware structure */
> +	fw = kzalloc(sizeof(*fw), GFP_KERNEL);
> +	if (!fw)
> +		return ERR_PTR(-ENOMEM);
> +
> +	do {
> +		/* Load single component */
> +		rc = zl3073x_fw_component_load(zldev, &comp, &data, &size,
> +					       extack);
> +		if (rc <= 0)
> +			/* Everything was read or error occurred */
> +			break;
> +
> +		id = comp->id;
> +
> +		/* Report error if the given component is present twice
> +		 * or more.
> +		 */
> +		if (fw->component[id]) {
> +			ZL3073X_FW_ERR_MSG(zldev, extack,
> +					   "duplicate component '%s' detected",
> +					   component_info[id].name);
> +			zl3073x_fw_component_free(comp);
> +			rc = -EINVAL;
> +			break;
> +		}
> +
> +		fw->component[id] = comp;
> +	} while (1);

s/1/true/

> +
> +	if (rc) {
> +		/* Free allocated firmware in case of error */
> +		zl3073x_fw_free(fw);

I found no call to it on success.

> +		return ERR_PTR(rc);
> +	}
> +
> +	return fw;
> +}


> +++ b/drivers/dpll/zl3073x/fw.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _ZL3073X_FW_H
> +#define _ZL3073X_FW_H
> +
> +/*
> + * enum zl3073x_fw_component_id - Identifiers for possible flash components
> + */
> +enum zl3073x_fw_component_id {
> +	ZL_FW_COMPONENT_INVALID = -1,
> +	ZL_FW_COMPONENT_UTIL = 0,
> +	ZL_FW_COMPONENT_FW1,
> +	ZL_FW_COMPONENT_FW2,
> +	ZL_FW_COMPONENT_FW3,
> +	ZL_FW_COMPONENT_CFG0,
> +	ZL_FW_COMPONENT_CFG1,
> +	ZL_FW_COMPONENT_CFG2,
> +	ZL_FW_COMPONENT_CFG3,
> +	ZL_FW_COMPONENT_CFG4,
> +	ZL_FW_COMPONENT_CFG5,
> +	ZL_FW_COMPONENT_CFG6,
> +	ZL_FW_NUM_COMPONENTS,

no comma after enum that will be last forever (guard/size/max/num/cnt)

> +};

Re: [PATCH net-next v2 2/5] dpll: zl3073x: Add low-level flash functions

[Ivan Vecera]

On 12. 08. 25 12:29 dop., Przemek Kitszel wrote:
> On 8/11/25 16:40, Ivan Vecera wrote:
>> To implement the devlink device flash functionality, the driver needs
>> to access both the device memory and the internal flash memory. The flash
>> memory is accessed using a device-specific program (called the flash
>> utility). This flash utility must be downloaded by the driver into
>> the device memory and then executed by the device CPU. Once running,
>> the flash utility provides a flash API to access the flash memory itself.
>>
>> During this operation, the normal functionality provided by the standard
>> firmware is not available. Therefore, the driver must ensure that DPLL
>> callbacks and monitoring functions are not executed during the flash
>> operation.
>>
>> Add all necessary functions for downloading the utility to device memory,
>> entering and exiting flash mode, and performing flash operations.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>> v2:
>> * extended 'comp_str' to 32 chars to avoid warnings related to snprintf
>> * added additional includes
>> ---
>>   drivers/dpll/zl3073x/Makefile  |   2 +-
>>   drivers/dpll/zl3073x/devlink.c |   9 +
>>   drivers/dpll/zl3073x/devlink.h |   3 +
>>   drivers/dpll/zl3073x/flash.c   | 684 +++++++++++++++++++++++++++++++++
>>   drivers/dpll/zl3073x/flash.h   |  29 ++
>>   drivers/dpll/zl3073x/regs.h    |  39 ++
>>   6 files changed, 765 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/dpll/zl3073x/flash.c
>>   create mode 100644 drivers/dpll/zl3073x/flash.h
> 
> 
>> +static int
>> +zl3073x_flash_download(struct zl3073x_dev *zldev, const char *component,
>> +               u32 addr, const void *data, size_t size,
>> +               struct netlink_ext_ack *extack)
>> +{
>> +#define CHECK_DELAY    5000 /* Check for interrupt each 5 seconds */
> 
> nit: please add ZL_ prefix

Ack, will fix.

>> +    unsigned long timeout;
>> +    const void *ptr, *end;
>> +    int rc = 0;
>> +
>> +    dev_dbg(zldev->dev, "Downloading %zu bytes to device memory at 
>> 0x%0x\n",
>> +        size, addr);
>> +
>> +    timeout = jiffies + msecs_to_jiffies(CHECK_DELAY);
>> +
>> +    for (ptr = data, end = data + size; ptr < end; ptr += 4, addr += 
>> 4) {
>> +        /* Write current word to HW memory */
>> +        rc = zl3073x_write_hwreg(zldev, addr, *(const u32 *)ptr);
>> +        if (rc) {
>> +            ZL_FLASH_ERR_MSG(zldev, extack,
>> +                     "failed to write to memory at 0x%0x",
>> +                     addr);
>> +            return rc;
>> +        }
>> +
>> +        /* Check for pending interrupt each 5 seconds */
> 
> nit: comment seems too trivial (and ~repeats the above one)

Ack, will remove

>> +        if (time_after(jiffies, timeout)) {
>> +            if (signal_pending(current)) {
>> +                ZL_FLASH_ERR_MSG(zldev, extack,
>> +                         "Flashing interrupted");
>> +                return -EINTR;
>> +            }
>> +
>> +            timeout = jiffies + msecs_to_jiffies(CHECK_DELAY);
>> +        }
>> +
>> +        /* Report status each 1 kB block */
>> +        if ((ptr - data) % 1024 == 0)
>> +            zl3073x_devlink_flash_notify(zldev, "Downloading image",
>> +                             component, ptr - data,
>> +                             size);
>> +    }
>> +
>> +    zl3073x_devlink_flash_notify(zldev, "Downloading image", component,
>> +                     ptr - data, size);
>> +
>> +    dev_dbg(zldev->dev, "%zu bytes downloaded to device memory\n", 
>> size);
>> +
>> +    return rc;
>> +}
>> +
> 
> 
>> +/**
>> + * zl3073x_flash_wait_ready - Check or wait for utility to be ready 
>> to flash
>> + * @zldev: zl3073x device structure
>> + * @timeout_ms: timeout for the waiting
>> + *
>> + * Return: 0 on success, <0 on error
>> + */
>> +static int
>> +zl3073x_flash_wait_ready(struct zl3073x_dev *zldev, unsigned int 
>> timeout_ms)
>> +{
>> +#define ZL_FLASH_POLL_DELAY_MS    100
>> +    unsigned long timeout;
>> +    int rc, i;
>> +
>> +    dev_dbg(zldev->dev, "Waiting for flashing to be ready\n");
>> +
>> +    timeout = jiffies + msecs_to_jiffies(timeout_ms);
> 
> this is duplicated in the loop init below

Ack, will remove this dup.

>> +
>> +    for (i = 0, timeout = jiffies + msecs_to_jiffies(timeout_ms);
>> +         time_before(jiffies, timeout);
>> +         i++) {
>> +        u8 value;
>> +
>> +        /* Check for interrupt each 1s */
>> +        if (i > 9) {
>> +            if (signal_pending(current))
>> +                return -EINTR;
>> +            i = 0;
>> +        }
>> +
>> +        /* Read write_flash register value */
>> +        rc = zl3073x_read_u8(zldev, ZL_REG_WRITE_FLASH, &value);
>> +        if (rc)
>> +            return rc;
>> +
>> +        value = FIELD_GET(ZL_WRITE_FLASH_OP, value);
>> +
>> +        /* Check if the current operation was done */
>> +        if (value == ZL_WRITE_FLASH_OP_DONE)
>> +            return 0; /* Operation was successfully done */
>> +
>> +        msleep(ZL_FLASH_POLL_DELAY_MS);
> 
> nit: needless sleep in the very last iteration step
> (a very minor issue with timeouts in range of minutes ;P)

Yes, I would not take care of 100ms delay in a minute timeout.

>> +    }
>> +
>> +    return -ETIMEDOUT;
>> +}
>> +
>> +/**
>> + * zl3073x_flash_cmd_wait - Perform flash operation and wait for finish
>> + * @zldev: zl3073x device structure
>> + * @operation: operation to perform
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * Return: 0 on success, <0 on error
>> + */
>> +static int
>> +zl3073x_flash_cmd_wait(struct zl3073x_dev *zldev, u32 operation,
>> +               struct netlink_ext_ack *extack)
>> +{
>> +#define FLASH_PHASE1_TIMEOUT_MS 60000    /* up to 1 minute */
>> +#define FLASH_PHASE2_TIMEOUT_MS 120000    /* up to 2 minutes */
> 
> nit: missing prefixes

Will fix

>> +    u8 value;
>> +    int rc;
>> +
>> +    dev_dbg(zldev->dev, "Sending flash command: 0x%x\n", operation);
>> +
>> +    /* Wait for access */
>> +    rc = zl3073x_flash_wait_ready(zldev, FLASH_PHASE1_TIMEOUT_MS);
>> +    if (rc)
>> +        return rc;
>> +
>> +    /* Issue the requested operation */
>> +    rc = zl3073x_read_u8(zldev, ZL_REG_WRITE_FLASH, &value);
>> +    if (rc)
>> +        return rc;
>> +
>> +    value &= ~ZL_WRITE_FLASH_OP;
>> +    value |= FIELD_PREP(ZL_WRITE_FLASH_OP, operation);
>> +
>> +    rc = zl3073x_write_u8(zldev, ZL_REG_WRITE_FLASH, value);
>> +    if (rc)
>> +        return rc;
>> +
>> +    /* Wait for command completion */
>> +    rc = zl3073x_flash_wait_ready(zldev, FLASH_PHASE2_TIMEOUT_MS);
>> +    if (rc)
>> +        return rc;
>> +
>> +    /* Check for utility errors */
>> +    return zl3073x_flash_error_check(zldev, extack);
>> +}
>> +
>> +/**
>> + * zl3073x_flash_get_sector_size - Get flash sector size
>> + * @zldev: zl3073x device structure
>> + * @sector_size: sector size returned by the function
>> + *
>> + * The function reads the flash sector size detected by flash utility 
>> and
>> + * stores it into @sector_size.
>> + *
>> + * Return: 0 on success, <0 on error
>> + */
>> +static int
>> +zl3073x_flash_get_sector_size(struct zl3073x_dev *zldev, size_t 
>> *sector_size)
>> +{
>> +    u8 flash_info;
>> +    int rc;
>> +
>> +    rc = zl3073x_read_u8(zldev, ZL_REG_FLASH_INFO, &flash_info);
>> +    if (rc)
>> +        return rc;
>> +
>> +    switch (FIELD_GET(ZL_FLASH_INFO_SECTOR_SIZE, flash_info)) {
>> +    case ZL_FLASH_INFO_SECTOR_4K:
>> +        *sector_size = 0x1000;
>> +        break;
>> +    case ZL_FLASH_INFO_SECTOR_64K:
>> +        *sector_size = 0x10000;
> 
> nit: up to you, but I would like to see SZ_64K instead
> (and don't count zeroes), if so, SZ_4K for the above too

Will fix.

>> +        break;
>> +    default:
>> +        rc = -EINVAL;
>> +        break;
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +/**
>> + * zl3073x_flash_sectors - Flash sectors
>> + * @zldev: zl3073x device structure
>> + * @component: component name
>> + * @page: destination flash page
>> + * @addr: device memory address to load data
>> + * @data: pointer to data to be flashed
>> + * @size: size of data
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The function flashes given @data with size of @size to the 
>> internal flash
>> + * memory block starting from page @page. The function uses sector flash
>> + * method and has to take into account the flash sector size reported by
>> + * flashing utility. Input data are spliced into blocks according this
>> + * sector size and each block is flashed separately.
>> + *
>> + * Return: 0 on success, <0 on error
>> + */
>> +int zl3073x_flash_sectors(struct zl3073x_dev *zldev, const char 
>> *component,
>> +              u32 page, u32 addr, const void *data, size_t size,
>> +              struct netlink_ext_ack *extack)
>> +{
>> +#define ZL_FLASH_MAX_BLOCK_SIZE    0x0001E000
>> +#define ZL_FLASH_PAGE_SIZE    256
>> +    size_t max_block_size, block_size, sector_size;
>> +    const void *ptr, *end;
>> +    int rc;
>> +
>> +    /* Get flash sector size */
>> +    rc = zl3073x_flash_get_sector_size(zldev, &sector_size);
>> +    if (rc) {
>> +        ZL_FLASH_ERR_MSG(zldev, extack,
>> +                 "Failed to get flash sector size");
>> +        return rc;
>> +    }
>> +
>> +    /* Determine max block size depending on sector size */
>> +    max_block_size = ALIGN_DOWN(ZL_FLASH_MAX_BLOCK_SIZE, sector_size);
>> +
>> +    for (ptr = data, end = data + size; ptr < end; ptr += block_size) {
> 
> block_size is uninitialized on the first loop iteration

The block_size here is used after 1st loop iteration...
> 
>> +        char comp_str[32];
>> +
>> +        block_size = min_t(size_t, max_block_size, end - ptr);

...and it is initialized here.

>> +
>> +        /* Add suffix '-partN' if the requested component size is
>> +         * greater than max_block_size.
>> +         */
>> +        if (max_block_size < size)
>> +            snprintf(comp_str, sizeof(comp_str), "%s-part%zu",
>> +                 component, (ptr - data) / max_block_size + 1);
>> +        else
>> +            strscpy(comp_str, component);
>> +
>> +        /* Download block to device memory */
>> +        rc = zl3073x_flash_download(zldev, comp_str, addr, ptr,
>> +                        block_size, extack);
>> +        if (rc)
>> +            goto finish;
>> +
>> +        /* Set address to flash from */
>> +        rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_START_ADDR, addr);
>> +        if (rc)
>> +            goto finish;
>> +
>> +        /* Set size of block to flash */
>> +        rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_SIZE, block_size);
>> +        if (rc)
>> +            goto finish;
>> +
>> +        /* Set destination page to flash */
>> +        rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, page);
>> +        if (rc)
>> +            goto finish;
>> +
>> +        /* Set filling pattern */
>> +        rc = zl3073x_write_u32(zldev, ZL_REG_FILL_PATTERN, U32_MAX);
>> +        if (rc)
>> +            goto finish;
>> +
>> +        zl3073x_devlink_flash_notify(zldev, "Flashing image", comp_str,
>> +                         0, 0);
>> +
>> +        dev_dbg(zldev->dev, "Flashing %zu bytes to page %u\n",
>> +            block_size, page);
>> +
>> +        /* Execute sectors flash operation */
>> +        rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_SECTORS,
>> +                        extack);
>> +        if (rc)
>> +            goto finish;
>> +
>> +        /* Move to next page */
>> +        page += block_size / ZL_FLASH_PAGE_SIZE;
>> +    }
>> +
>> +finish:
>> +    zl3073x_devlink_flash_notify(zldev,
>> +                     rc ?  "Flashing failed" : "Flashing done",
>> +                     component, 0, 0);
>> +
>> +    return rc;
>> +}
>> +
>> +/**
>> + * zl3073x_flash_page - Flash page
>> + * @zldev: zl3073x device structure
>> + * @component: component name
>> + * @page: destination flash page
>> + * @addr: device memory address to load data
>> + * @data: pointer to data to be flashed
>> + * @size: size of data
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The function flashes given @data with size of @size to the 
>> internal flash
>> + * memory block starting with page @page.
>> + *
>> + * Return: 0 on success, <0 on error
>> + */
>> +int zl3073x_flash_page(struct zl3073x_dev *zldev, const char *component,
>> +               u32 page, u32 addr, const void *data, size_t size,
>> +               struct netlink_ext_ack *extack)
>> +{
> 
> looks like a canditate to use zl3073x_flash_sectors(), or make
> a higher-level helper that will do heavy-lifting for
> zl3073x_flash_sectors() and zl3073x_flash_page()
> (especially that you did such great job with low-level helpers)

Will refactor the common code to separate function that will be called
by both zl3073x_flash_page() and zl3073x_flash_sectors().

>> +    int rc;
>> +
>> +    /* Download component to device memory */
>> +    rc = zl3073x_flash_download(zldev, component, addr, data, size, 
>> extack);
>> +    if (rc)
>> +        goto finish;
>> +
>> +    /* Set address to flash from */
>> +    rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_START_ADDR, addr);
>> +    if (rc)
>> +        goto finish;
>> +
>> +    /* Set size of block to flash */
>> +    rc = zl3073x_write_u32(zldev, ZL_REG_IMAGE_SIZE, size);
>> +    if (rc)
>> +        goto finish;
>> +
>> +    /* Set destination page to flash */
>> +    rc = zl3073x_write_u32(zldev, ZL_REG_FLASH_INDEX_WRITE, page);
>> +    if (rc)
>> +        goto finish;
>> +
>> +    /* Set filling pattern */
>> +    rc = zl3073x_write_u32(zldev, ZL_REG_FILL_PATTERN, U32_MAX);
>> +    if (rc)
>> +        goto finish;
>> +
>> +    zl3073x_devlink_flash_notify(zldev, "Flashing image", component, 0,
>> +                     size);
>> +
>> +    /* Execute sectors flash operation */
>> +    rc = zl3073x_flash_cmd_wait(zldev, ZL_WRITE_FLASH_OP_PAGE, extack);
>> +    if (rc)
>> +        goto finish;
>> +
>> +    zl3073x_devlink_flash_notify(zldev, "Flashing image", component, 
>> size,
>> +                     size);
>> +
>> +finish:
>> +    zl3073x_devlink_flash_notify(zldev,
>> +                     rc ?  "Flashing failed" : "Flashing done",
>> +                     component, 0, 0);
>> +
>> +    return rc;
>> +}
> 
> 
>> +
>> +static int
>> +zl3073x_flash_host_ctrl_enable(struct zl3073x_dev *zldev)
>> +{
>> +    u8 host_ctrl;
>> +    int rc;
>> +
>> +    /* Read host control register */
>> +    rc = zl3073x_read_u8(zldev, ZL_REG_HOST_CONTROL, &host_ctrl);
>> +    if (rc)
>> +        return rc;
>> +
>> +    /* Enable host control */
>> +    host_ctrl &= ~ZL_HOST_CONTROL_ENABLE;
> 
> suspicious, as this line does nothing (in the context of the next one)

Will remove.

>> +    host_ctrl |= ZL_HOST_CONTROL_ENABLE;
>> +
>> +    /* Update host control register */
>> +    return zl3073x_write_u8(zldev, ZL_REG_HOST_CONTROL, host_ctrl);
>> +}
>> +
>> +/**
>> + * zl3073x_flash_mode_enter - Switch the device to flash mode
>> + * @zldev: zl3073x device structure
>> + * @util_ptr: buffer with flash utility
>> + * @util_size: size of buffer with flash utility
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The function prepares and switches the device into flash mode.
>> + *
>> + * The procedure:
>> + * 1) Stop device CPU by specific HW register sequence
>> + * 2) Download flash utility to device memory
>> + * 3) Resume device CPU by specific HW register sequence
>> + * 4) Check communication with flash utility
>> + * 5) Enable host control necessary to access flash API
>> + * 6) Check for potential error detected by the utility
>> + *
>> + * The API provided by normal firmware is not available in flash mode
>> + * so the caller has to ensure that this API is not used in this mode.
>> + *
>> + * After performing flash operation the caller should call
>> + * @zl3073x_flash_mode_leave to return back to normal operation.
>> + *
>> + * Return: 0 on success, <0 on error.
>> + */
>> +int zl3073x_flash_mode_enter(struct zl3073x_dev *zldev, const void 
>> *util_ptr,
>> +                 size_t util_size, struct netlink_ext_ack *extack)
>> +{
>> +    /* Sequence to be written prior utility download */
>> +    static const struct zl3073x_hwreg_seq_item pre_seq[] = {
>> +        HWREG_SEQ_ITEM(0x80000400, 1, BIT(0), 0),
>> +        HWREG_SEQ_ITEM(0x80206340, 1, BIT(4), 0),
>> +        HWREG_SEQ_ITEM(0x10000000, 1, BIT(2), 0),
>> +        HWREG_SEQ_ITEM(0x10000024, 0x00000001, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10000020, 0x00000001, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10000000, 1, BIT(10), 1000),
>> +    };
>> +    /* Sequence to be written after utility download */
>> +    static const struct zl3073x_hwreg_seq_item post_seq[] = {
>> +        HWREG_SEQ_ITEM(0x10400004, 0x000000C0, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10400008, 0x00000000, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10400010, 0x20000000, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10400014, 0x20000004, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10000000, 1, GENMASK(10, 9), 0),
>> +        HWREG_SEQ_ITEM(0x10000020, 0x00000000, U32_MAX, 0),
>> +        HWREG_SEQ_ITEM(0x10000000, 0, BIT(0), 1000),
>> +    };
> very nice code
> 

Thanks for the review and advices.

Ivan

Re: [PATCH net-next v2 3/5] dpll: zl3073x: Add firmware loading functionality

[Ivan Vecera]

On 12. 08. 25 1:11 dop., Przemek Kitszel wrote:
> On 8/11/25 16:40, Ivan Vecera wrote:
>> Add functionality for loading firmware files provided by the vendor
>> to be flashed into the device's internal flash memory. The firmware
>> consists of several components, such as the firmware executable itself,
>> chip-specific customizations, and configuration files.
>>
>> The firmware file contains at least a flash utility, which is executed
>> on the device side, and one or more flashable components. Each component
>> has its own specific properties, such as the address where it should be
>> loaded during flashing, one or more destination flash pages, and
>> the flashing method that should be used.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>> v2:
>> * added additional includes
>> * removed empty line
>> * '*(dst+len)' -> '*(dst + len)'
>> * 'Santity' -> 'Sanity'
>> * fixed smatch warning about uninitialized 'rc'
>> ---
>>   drivers/dpll/zl3073x/Makefile |   2 +-
>>   drivers/dpll/zl3073x/fw.c     | 498 ++++++++++++++++++++++++++++++++++
>>   drivers/dpll/zl3073x/fw.h     |  52 ++++
>>   3 files changed, 551 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/dpll/zl3073x/fw.c
>>   create mode 100644 drivers/dpll/zl3073x/fw.h
>>
> 
> overview:
> I don't like zl3073x_fw_readline() and it's usage - sscanf will do IMO
> 
> please find other feedback inline
> 
>> +/* Sanity check */
>> +static_assert(ARRAY_SIZE(component_info) == ZL_FW_NUM_COMPONENTS);
>> +
>> +/**
>> + * zl3073x_fw_readline - Read next line from firmware
>> + * @dst: destination buffer
>> + * @dst_sz: destination buffer size
>> + * @psrc: source buffer
>> + * @psrc_sz: source buffer size
>> + *
>> + * The function read next line from the firmware buffer specified by 
>> @psrc
>> + * and @psrc_sz and stores it into buffer specified by @dst and @dst_sz.
>> + * The pointer @psrc and remaining bytes in @psrc_sz are updated 
>> accordingly.
>> + *
>> + * Return: number of characters read on success, -EINVAL on error
>> + */
>> +static ssize_t
>> +zl3073x_fw_readline(char *dst, size_t dst_sz, const char **psrc,
>> +            size_t *psrc_sz)
>> +{
>> +    const char *ptr = *psrc;
>> +    size_t len;
>> +
>> +    /* Skip any existing new-lines at the beginning */
>> +    ptr = memchr_inv(*psrc, '\n', *psrc_sz);
>> +    if (ptr) {
>> +        *psrc_sz -= ptr - *psrc;
>> +        *psrc = ptr;
>> +    }
>> +
>> +    /* Now look for the next new-line in the source */
>> +    ptr = memscan((void *)*psrc, '\n', *psrc_sz);
>> +    len = ptr - *psrc;
>> +
>> +    /* Return error if the source line is too long for destination */
>> +    if (len >= dst_sz)
>> +        return -EINVAL;
>> +
>> +    /* Copy the line from source and append NUL char  */
>> +    memcpy(dst, *psrc, len);
>> +    *(dst + len) = '\0';
>> +
>> +    *psrc = ptr;
>> +    *psrc_sz -= len;
>> +
>> +    /* Return number of read chars */
>> +    return len;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_alloc - Alloc structure to hold firmware 
>> component
>> + * @size: size of buffer to store data
>> + *
>> + * Return: pointer to allocated component structure or NULL on error.
>> + */
>> +static struct zl3073x_fw_component *
>> +zl3073x_fw_component_alloc(size_t size)
>> +{
>> +    struct zl3073x_fw_component *comp;
>> +
>> +    comp = kzalloc(sizeof(*comp), GFP_KERNEL);
>> +    if (!comp)
>> +        return NULL;
>> +
>> +    comp->size = size;
>> +    comp->data = kzalloc(size, GFP_KERNEL);
>> +    if (!comp->data) {
>> +        kfree(comp);
>> +        return NULL;
>> +    }
>> +
>> +    return comp;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_free - Free allocated component structure
>> + * @comp: pointer to allocated component
>> + */
>> +static void
>> +zl3073x_fw_component_free(struct zl3073x_fw_component *comp)
>> +{
>> +    if (comp)
>> +        kfree(comp->data);
>> +
>> +    kfree(comp);
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_id_get - Get ID for firmware component name
>> + * @name: input firmware component name
>> + *
>> + * Return:
>> + * - ZL3073X_FW_COMPONENT_* ID for known component name
>> + * - ZL3073X_FW_COMPONENT_INVALID if the given name is unknown
>> + */
>> +static enum zl3073x_fw_component_id
>> +zl3073x_fw_component_id_get(const char *name)
>> +{
>> +    enum zl3073x_fw_component_id id;
>> +
>> +    for (id = ZL_FW_COMPONENT_UTIL; id < ZL_FW_NUM_COMPONENTS; id++)
> 
> I would type the start as "id = 0"
> (as you did in other functions, eg zl3073x_fw_free())

Will change.

>> +        if (!strcasecmp(name, component_info[id].name))
>> +            return id;
>> +
>> +    return ZL_FW_COMPONENT_INVALID;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_component_load - Load component from firmware source
>> + * @zldev: zl3073x device structure
>> + * @pcomp: pointer to loaded component
>> + * @psrc: data pointer to load component from
>> + * @psize: remaining bytes in buffer
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The function allocates single firmware component and loads the 
>> data from
>> + * the buffer specified by @psrc and @psize. Pointer to allocated 
>> component
>> + * is stored in output @pcomp. Source data pointer @psrc and 
>> remaining bytes
>> + * @psize are updated accordingly.
>> + *
>> + * Return: 0 on success, <0 on error
> 
> document return of 1

Will fix.

>> + */
>> +static ssize_t
>> +zl3073x_fw_component_load(struct zl3073x_dev *zldev,
>> +              struct zl3073x_fw_component **pcomp,
>> +              const char **psrc, size_t *psize,
>> +              struct netlink_ext_ack *extack)
>> +{
>> +    const struct zl3073x_fw_component_info *info;
>> +    struct zl3073x_fw_component *comp = NULL;
>> +    struct device *dev = zldev->dev;
>> +    enum zl3073x_fw_component_id id;
>> +    ssize_t len, count;
>> +    u32 comp_size;
>> +    char line[32];
>> +    int rc;
>> +
>> +    /* Fetch image name from input */
>> +    len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
>> +    if (len < 0) {
>> +        rc = len;
>> +        goto err_unexpected;
>> +    } else if (!len) {
>> +        /* No more data */
>> +        return 0;
>> +    }
>> +
>> +    dev_dbg(dev, "Firmware component '%s' found\n", line);
>> +
>> +    id = zl3073x_fw_component_id_get(line);
>> +    if (id == ZL_FW_COMPONENT_INVALID) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] unknown component type",
>> +                   line);
>> +        return -EINVAL;
>> +    }
>> +
>> +    info = &component_info[id];
>> +
>> +    /* Fetch image size from input */
>> +    len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
>> +    if (len < 0) {
>> +        rc = len;
>> +        goto err_unexpected;
>> +    } else if (!len) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing size",
>> +                   info->name);
>> +        return -ENODATA;
>> +    }
>> +
>> +    rc = kstrtou32(line, 10, &comp_size);
>> +    if (rc) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                   "[%s] invalid size value '%s'", info->name,
>> +                   line);
>> +        return rc;
>> +    }
> 
> why not sscanf()? it would greatly simplify the above, and likely you
> could entriely remove zl3073x_fw_readline() too

I cannot use sscanf here because the input buffer is passed by devlink
as 'struct firmware' buffer and this buffer is not null-terminated.
In this case the driver would have to make a copy of the input buffer
and place null char at the end. The firmware can have up to 256kB and
IMO it is better to process original firmware buffer instead of extra
copy.

>> +
>> +    comp_size *= sizeof(u32); /* convert num of dwords to bytes */
>> +
>> +    /* Check image size validity */
>> +    if (comp_size > component_info[id].max_size) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                   "[%s] component is too big (%u bytes)\n",
>> +                   info->name, comp_size);
>> +        return -EINVAL;
>> +    }
>> +
>> +    dev_dbg(dev, "Indicated component image size: %u bytes\n", 
>> comp_size);
>> +
>> +    /* Alloc component */
>> +    comp = zl3073x_fw_component_alloc(comp_size);
>> +    if (!comp) {
>> +        ZL3073X_FW_ERR_MSG(zldev, extack, "failed to alloc memory");
>> +        return -ENOMEM;
>> +    }
>> +    comp->id = id;
>> +
>> +    /* Load component data from firmware source */
>> +    for (count = 0; count < comp_size; count += 4) {
>> +        len = zl3073x_fw_readline(line, sizeof(line), psrc, psize);
>> +        if (len < 0) {
>> +            rc = len;
>> +            goto err_unexpected;
>> +        } else if (!len) {
>> +            ZL3073X_FW_ERR_MSG(zldev, extack, "[%s] missing data",
>> +                       info->name);
>> +            rc = -ENODATA;
>> +            goto err;
>> +        }
>> +
>> +        rc = kstrtou32(line, 16, comp->data + count);
>> +        if (rc) {
>> +            ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                       "[%s] invalid data: '%s'",
>> +                       info->name, line);
>> +            goto err;
>> +        }
>> +    }
>> +
>> +    *pcomp = comp;
>> +
>> +    return 1;> +
>> +err_unexpected:
>> +    ZL3073X_FW_ERR_MSG(zldev, extack, "unexpected input");
>> +err:
>> +    zl3073x_fw_component_free(comp);
>> +
>> +    return rc;
>> +}
>> +
>> +/**
>> + * zl3073x_fw_free - Free allocated firmware
>> + * @fw: firmware pointer
>> + *
>> + * The function frees existing firmware allocated by @zl3073x_fw_load.
>> + */
>> +void zl3073x_fw_free(struct zl3073x_fw *fw)
>> +{
>> +    size_t i;
>> +
>> +    if (!fw)
>> +        return;
>> +
>> +    for (i = 0; i < ZL_FW_NUM_COMPONENTS; i++)
>> +        zl3073x_fw_component_free(fw->component[i]);
>> +
>> +    kfree(fw);
>> +}
>> +
>> +/**
>> + * zl3073x_fw_load - Load all components from source
>> + * @zldev: zl3073x device structure
>> + * @data: source buffer pointer
>> + * @size: size of source buffer
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * The functions allocate firmware structure and loads all components 
>> from
>> + * the given buffer specified by @data and @size.
>> + *
>> + * Return: pointer to firmware on success, error pointer on error
>> + */
>> +struct zl3073x_fw *zl3073x_fw_load(struct zl3073x_dev *zldev, const 
>> char *data,
>> +                   size_t size, struct netlink_ext_ack *extack)
>> +{
>> +    struct zl3073x_fw_component *comp;
>> +    enum zl3073x_fw_component_id id;
>> +    struct zl3073x_fw *fw;
>> +    ssize_t rc;
>> +
>> +    /* Allocate firmware structure */
>> +    fw = kzalloc(sizeof(*fw), GFP_KERNEL);
>> +    if (!fw)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    do {
>> +        /* Load single component */
>> +        rc = zl3073x_fw_component_load(zldev, &comp, &data, &size,
>> +                           extack);
>> +        if (rc <= 0)
>> +            /* Everything was read or error occurred */
>> +            break;
>> +
>> +        id = comp->id;
>> +
>> +        /* Report error if the given component is present twice
>> +         * or more.
>> +         */
>> +        if (fw->component[id]) {
>> +            ZL3073X_FW_ERR_MSG(zldev, extack,
>> +                       "duplicate component '%s' detected",
>> +                       component_info[id].name);
>> +            zl3073x_fw_component_free(comp);
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        fw->component[id] = comp;
>> +    } while (1);
> 
> s/1/true/

Will fix.

>> +
>> +    if (rc) {
>> +        /* Free allocated firmware in case of error */
>> +        zl3073x_fw_free(fw);
> 
> I found no call to it on success.

The caller will deallocate it using zl3073x_fw_free()

> 
>> +        return ERR_PTR(rc);
>> +    }
>> +
>> +    return fw;
>> +}
> 
> 
>> +++ b/drivers/dpll/zl3073x/fw.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _ZL3073X_FW_H
>> +#define _ZL3073X_FW_H
>> +
>> +/*
>> + * enum zl3073x_fw_component_id - Identifiers for possible flash 
>> components
>> + */
>> +enum zl3073x_fw_component_id {
>> +    ZL_FW_COMPONENT_INVALID = -1,
>> +    ZL_FW_COMPONENT_UTIL = 0,
>> +    ZL_FW_COMPONENT_FW1,
>> +    ZL_FW_COMPONENT_FW2,
>> +    ZL_FW_COMPONENT_FW3,
>> +    ZL_FW_COMPONENT_CFG0,
>> +    ZL_FW_COMPONENT_CFG1,
>> +    ZL_FW_COMPONENT_CFG2,
>> +    ZL_FW_COMPONENT_CFG3,
>> +    ZL_FW_COMPONENT_CFG4,
>> +    ZL_FW_COMPONENT_CFG5,
>> +    ZL_FW_COMPONENT_CFG6,
>> +    ZL_FW_NUM_COMPONENTS,
> 
> no comma after enum that will be last forever (guard/size/max/num/cnt)

Will fix.

>> +};
> 

Thanks,
Ivan