Message-ID: <168315c3-48c2-40ca-be70-8967f65f1343@intel.com>
Date: Tue, 12 Aug 2025 00:29:35 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Ivan Vecera <ivecera@...hat.com>
CC: Jiri Pirko <jiri@...nulli.us>, <netdev@...r.kernel.org>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
	<horms@...nel.org>, Jonathan Corbet <corbet@....net>, Prathosh Satish
	<Prathosh.Satish@...rochip.com>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Michal Schmidt <mschmidt@...hat.com>, "Petr
 Oros" <poros@...hat.com>
Subject: Re: [PATCH net-next v2 2/5] dpll: zl3073x: Add low-level flash
 functions

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@...hat.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

Powered by blists - more mailing lists