Message-ID: <26fa02f7-c2e3-4890-887d-90aa2040d461@redhat.com>
Date: Wed, 13 Aug 2025 14:17:11 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.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 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@...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

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

Powered by blists - more mailing lists