Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

From: Bjorn Andersson
Date: Wed Mar 05 2025 - 18:28:06 EST

Next message: Namhyung Kim: "[PATCH] perf report: Do not process non-JIT BPF ksymbol events"
Previous message: Jasjiv Singh: "Re: [PATCH v3] ipe: add errno field to IPE policy load auditing"
In reply to: kernel test robot: "Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH v3 0/9] Add support to load QUP SE firmware from"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Mar 03, 2025 at 06:13:46PM +0530, Viken Dadhaniya wrote:
> Load the firmware to QUP SE based on the 'firmware-name' property specified

Please start your commit message with a description of "the problem"
you're trying to solve, explain that QUP firmware is typically loaded by
the "bootloader" and that for <reason> you're adding support for loading
firmware are load time. Please read and follow:
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> in devicetree. Populate Serial engine and base address details in the probe
> function of the protocol driver and pass to firmware load routine.

Don't describe the code flow, describe how this fits into the bigger
picture. E.g. that SE-functional drivers will load this at bootup.

>
> Skip the firmware loading if the firmware is already loaded in Serial
> Engine's firmware memory area.
>

Above description should be clear enough that this is obvious.

> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
> ---
> v2 -> v3:
>
> - Remove code related to the 'qcom,xfer-mode' property.
> - Add logic to read the boolean property 'qcom,gsi-dma-allowed' and select the transfer mode.
> - Hardcode FIFO mode for the serial driver as GSI mode is currently not supported.
> - Update function descriptions as suggested.
> - Enhance error handling and remove redundant if conditions.
> - Drop the ternary operator.
>
> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-6-quic_vdadhani@xxxxxxxxxxx/
>
> v1 -> v2:
>
> - Remove the fixed firmware path and add logic to read the path from the device tree.
> - Remove code related to the 'qcom,load-firmware' property.
> - Resolve kernel test robot warnings.
> - Update the commit message.
> - Update Copyright year.
>
> v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-5-quic_vdadhani@xxxxxxxxxxx/
> ---
> ---
> drivers/soc/qcom/qcom-geni-se.c | 423 +++++++++++++++++++++++++++
> include/linux/soc/qcom/geni-se.h | 18 ++
> include/linux/soc/qcom/qup-fw-load.h | 179 ++++++++++++
> 3 files changed, 620 insertions(+)
> create mode 100644 include/linux/soc/qcom/qup-fw-load.h
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 4cb959106efa..345181ad6fcc 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> +// Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.

Odd, this should be a multiline comment.

>
> /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
> #define __DISABLE_TRACE_MMIO__
> @@ -15,6 +16,7 @@
> #include <linux/pinctrl/consumer.h>
> #include <linux/platform_device.h>
> #include <linux/soc/qcom/geni-se.h>
> +#include <linux/soc/qcom/qup-fw-load.h>
>
> /**
> * DOC: Overview
> @@ -110,6 +112,9 @@ struct geni_se_desc {
> static const char * const icc_path_names[] = {"qup-core", "qup-config",
> "qup-memory"};
>
> +static const char * const protocol_name[] = { "None", "SPI", "UART",
> + "I2C", "I3C", "SPI SLAVE"};
> +
> #define QUP_HW_VER_REG 0x4
>
> /* Common SE registers */
> @@ -891,6 +896,424 @@ int geni_icc_disable(struct geni_se *se)
> }
> EXPORT_SYMBOL_GPL(geni_icc_disable);
>
> +/**
> + * elf_phdr_valid() - Validate an ELF header.
> + * @phdr: Pointer to the ELF header.
> + *
> + * Validate the ELF header by comparing the fields stored in p_flags and the payload type.

The interesting piece of information here would be what the definition
of "valid" is - because you're not checking that the program header
entry is valid, you're checking that it's a specific valid.

> + *
> + * Return: true if the validation is successful, false otherwise.
> + */

That said, I think you should just inline these checks in "read_elf()"
below, preferably with a comment about what you're looking for.

> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
> +{
> + if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
> + return false;
> +
> + if (MI_PBT_PAGE_MODE_VALUE(phdr->p_flags) == MI_PBT_NON_PAGED_SEGMENT &&
> + MI_PBT_SEGMENT_TYPE_VALUE(phdr->p_flags) != MI_PBT_HASH_SEGMENT &&
> + MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) != MI_PBT_NOTUSED_SEGMENT &&
> + MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) != MI_PBT_SHARED_SEGMENT)
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * valid_seg_size() - Validate the segment size.
> + * @pelfseg: Pointer to the ELF header.
> + * @p_filesz: Pointer to the file size.
> + *
> + * Validate the ELF segment size by comparing the file size.
> + *
> + * Return: true if the segment is valid, false if the segment is invalid.
> + */
> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
> +{
> + if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
> + p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
> + p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
> + return true;
> + return false;
> +}
> +
> +/**
> + * read_elf() - Read an ELF file.
> + * @rsc: Pointer to the SE resources structure.
> + * @fw: Pointer to the firmware buffer.
> + * @pelfseg: Pointer to the SE-specific ELF header.
> + * @phdr: Pointer to one of the valid headers from the list in the firmware buffer.
> + *
> + * Read the ELF file and output a pointer to the header data, which
> + * contains the firmware data and any other details.
> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,

I think this name is too generic, please prefix it with "geni_" to make
it easier to search for.

> + struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
> +{
> + const struct elf32_hdr *ehdr = (const struct elf32_hdr *)fw->data;
> + struct elf32_phdr *phdrs = (struct elf32_phdr *)(ehdr + 1);
> + const u8 *addr;
> + int i;
> +
> + ehdr = (struct elf32_hdr *)fw->data;

Please validate that fw->size is sufficient before accessing - here and
in the loop.

> +
> + if (ehdr->e_phnum < 2)
> + return -EINVAL;
> +
> + for (i = 0; i < ehdr->e_phnum; i++) {
> + *phdr = &phdrs[i];
> + if (!elf_phdr_valid(*phdr))
> + continue;
> +
> + if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
> + addr = fw->data + (*phdr)->p_offset;
> + *pelfseg = (struct elf_se_hdr *)addr;

Don't assign *pelfseg until you've found a match.

> +
> + if ((*pelfseg)->magic == MAGIC_NUM_SE &&
> + (*pelfseg)->version == 1 &&
> + valid_seg_size(*pelfseg, (*phdr)->p_filesz) &&
> + (*pelfseg)->serial_protocol == rsc->protocol &&
> + (*pelfseg)->serial_protocol != GENI_SE_NONE)

There's so much going on in this one conditional. Turn this the way
around:

if (phdr->p_filesz < sizeof(struct elf_se_hdr))
continue;

if (se->magic != MAGIC_NUM_SE)
continue;

...

Makes is super easy to read and each one encapsulates one disqualifying
factor.

> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
> +
> +/**
> + * geni_config_common_control() - Configure common CGC and disable high priority interrupt.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Configure the common CGC and disable high priority interrupts until the current low priority
> + * interrupts are handled.

What do you mean with "low priority interrupt", this is called from the
firmware loading operation - that doesn't sound like an interrupt at
all.

It leaves me with the question about why interrupts should be disabled
and why they should be disabled in a function called "config common
control".

Unless it makes the code easier to read, you should wrap it at 80
characters. I don't think this sentence is easier to read in it's
unwrapped form.

> + *
> + * Return: None.

Drop this for void functions.

> + */
> +static void geni_config_common_control(struct qup_se_rsc *rsc)
> +{
> + /*
> + * Disable high priority interrupt until current
> + * low priority interrupts are handled.
> + */
> + setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CFG,

I find that the qup_se_rsc structure makes these functions overflow with
pointer dereferences, and it makes it impossible to know if a function
operates on 1, 2, or 3 of its members.

Please just pass the arguments around where necessary.

> + FAST_SWITCH_TO_HIGH_DISABLE_BMASK);
> +
> + /*
> + * Set AHB_M_CLK_CGC_ON to indicate hardware controls
> + * se-wrapper cgc clock.
> + */
> + setbits32(rsc->se->wrapper->base + QUPV3_SE_AHB_M_CFG,
> + AHB_M_CLK_CGC_ON_BMASK);
> +
> + /* Let hardware to control common cgc. */
> + setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CGC_CTRL,
> + COMMON_CSR_SLV_CLK_CGC_ON_BMASK);
> +}
> +
> +/**
> + * geni_configure_xfer_mode() - Set the transfer mode.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Set the transfer mode to either FIFO or DMA according to the mode specified by the protocol
> + * driver.
> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int geni_configure_xfer_mode(struct qup_se_rsc *rsc)
> +{
> + /* Configure SE FIFO, DMA or GSI mode. */
> + switch (rsc->mode) {
> + case GENI_GPI_DMA:
> + setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> + GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> + writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
> + writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
> + break;
> +
> + case GENI_SE_FIFO:
> + clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> + GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> + writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
> + writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
> + break;
> +
> + case GENI_SE_DMA:
> + setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> + GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> + writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
> + writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
> + break;
> +
> + default:
> + dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);

Please make this error more descriptive so that it's easier for someone
finding it in the kernel log to understand what the problem is.

> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/**
> + * geni_enable_interrupts() Enable interrupts.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Enable the required interrupts during the firmware load process.
> + *
> + * Return: None.
> + */
> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
> +{
> + u32 reg_value;
> +
> + /* Enable required interrupts. */
> + writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
> +
> + reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
> + S_CMD_CANCEL_EN | S_CMD_ABORT_EN |

Please confirm that the indentation is correct here.

> + S_GP_IRQ_0_EN | S_GP_IRQ_1_EN |
> + S_GP_IRQ_2_EN | S_GP_IRQ_3_EN |
> + S_RX_FIFO_WR_ERR_EN | S_RX_FIFO_RD_ERR_EN;
> + writel_relaxed(reg_value, rsc->se->base + GENI_S_IRQ_ENABLE);
> +
> + /* DMA mode configuration. */
> + reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
> + DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
> + DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
> + writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
> + reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
> + DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
> + DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
> + DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
> + writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
> +}
> +
> +/**
> + * geni_flash_fw_revision() - Flash the firmware revision.

It's not really "flashing", it's more just "writing"...right?

> + * @rsc: Pointer to a structure representing SE-related resources.
> + * @hdr: Pointer to the ELF header of the Serial Engine.

That's not the ELF header, it's the serial engine firmware header. That
said, you only consume the serial_protocol and fw_version, so a function
called "geni_write_fw_revision()" could perhaps just take those two
values as parameters?

> + *
> + * Flash the firmware revision and protocol into the respective register.

Again, no flashing, just writing.

> + *
> + * Return: None.
> + */
> +static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct elf_se_hdr *hdr)
> +{
> + u32 reg_value;
> +
> + /* Flash firmware revision register. */
> + reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
> + (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
> + writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
> +
> + reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
> + (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
> +
> + writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
> +}
> +
> +/**
> + * geni_load_se_fw() - Load Serial Engine specific firmware.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + * @fw: Pointer to the firmware structure.
> + *
> + * Load the protocol firmware into the IRAM of the Serial Engine.
> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int geni_load_se_fw(struct qup_se_rsc *rsc, const struct firmware *fw)
> +{
> + const u32 *fw_val_arr, *cfg_val_arr;
> + const u8 *cfg_idx_arr;
> + u32 i, reg_value, mask, ramn_cnt;
> + int ret;
> + struct elf_se_hdr *hdr;
> + struct elf32_phdr *phdr;
> +
> + ret = geni_icc_set_bw(rsc->se);
> + if (ret) {
> + dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);

Drop the __func__ from all your printouts.

> + return ret;
> + }
> +
> + ret = geni_icc_enable(rsc->se);
> + if (ret) {
> + dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = geni_se_resources_on(rsc->se);

Why do you turn on the buses and clocks before you're parsing the
firmware file?

> + if (ret) {
> + dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
> + goto err;
> + }
> +
> + ret = read_elf(rsc, fw, &hdr, &phdr);

Why is phdr returned here? Makes read_elf() harder to read...

> + if (ret) {
> + dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
> + goto err;
> + }
> +
> + fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);

Why is this a "firmware value array", isn't it the "firmware data"? Why
is it chunks of u32? Does it also need to be written in words?

> + cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
> + cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
> +
> + geni_config_common_control(rsc);
> +
> + /* Allows to drive corresponding data according to hardware value. */
> + writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
> +
> + /* Set SCLK and HCLK to program RAM */
> + setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> + GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> + writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
> + clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> + GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> +
> + /* Enable required clocks for DMA CSR, TX and RX. */
> + reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
> + DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
> + DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
> + DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
> +
> + setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
> +
> + /* Let hardware control CGC by default. */
> + writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
> +
> + /* Set version of the configuration register part of firmware. */
> + writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
> + writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_S_INIT_CFG_REVISION);
> +
> + /* Configure GENI primitive table. */
> + for (i = 0; i < hdr->cfg_size_in_items; i++)
> + writel_relaxed(cfg_val_arr[i],
> + rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));

What goes into this configuration?

> +
> + /* Configure condition for assertion of RX_RFR_WATERMARK condition. */
> + reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
> + mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;

Based on the WIDTH_MASK, those 6 bits doesn't sound like a "BIT"... And
is the RX_FIFO_WIDTH really a "mask"?

> + writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);

mask - 2?

Use FIELD_GET() and name things with the intention of making it easy for
others to understand what the code does.

> +
> + /* Let hardware control CGC */
> + setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
> +
> + ret = geni_configure_xfer_mode(rsc);
> + if (ret)
> + goto err_resource;
> +
> + geni_enable_interrupts(rsc);
> +
> + geni_flash_fw_revision(rsc, hdr);
> +
> + ramn_cnt = hdr->fw_size_in_items;
> + if (hdr->fw_size_in_items % 2 != 0)
> + ramn_cnt++;
> +
> + if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
> + goto err_resource;

As far as I can tell this error path is based entirely on the firmware
being read from the file system, so you could have checked it before you
turned on any resources - making a cleaner exit on failure.

> +
> + /* Program RAM address space. */
> + memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));

I presume this typecast to void * is to hide the fact that se->base is
flagged as __iomem? Use memcpy_toio() intead.

> +
> + /* Put default values on GENI's output pads. */
> + writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
> +
> + /* High to low SCLK and HCLK to finish RAM. */
> + setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> + GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> + setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
> + clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> + GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> +
> + /* Serial engine DMA interface is enabled. */
> + setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
> +
> + /* Enable or disable FIFO interface of the serial engine. */
> + if (rsc->mode == GENI_SE_FIFO)
> + clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
> + else
> + setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
> +
> +err_resource:
> + geni_se_resources_off(rsc->se);
> +err:
> + geni_icc_disable(rsc->se);
> + return ret;
> +}
> +
> +/**
> + * qup_fw_load() - Initiate firmware load.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Load the firmware into a specific SE. Read the associated ELF file,
> + * copy the data into a buffer in kernel space using the request_firmware API, write the
> + * data into the SE's IRAM register, and then free the buffers. Handle firmware loading
> + * and parsing for a specific protocol.
> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int qup_fw_load(struct qup_se_rsc *rsc, const char *fw_name)
> +{
> + int ret;
> + const struct firmware *fw;
> + struct device *dev = rsc->se->dev;
> +
> + ret = request_firmware(&fw, fw_name, dev);
> + if (ret) {
> + dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
> + return ret;
> + }
> +
> + ret = geni_load_se_fw(rsc, fw);
> +
> + release_firmware(fw);
> +
> + return ret;
> +}
> +
> +/**
> + * geni_load_se_firmware() - Initiate firmware loading.
> + * @se: Serial engine details.
> + * @protocol: Protocol (SPI, I2C, or UART) for which the firmware is to be loaded.
> + *
> + * If the device tree properties are configured to load QUP firmware and the firmware
> + * is not already loaded, start the firmware loading process. If the device tree properties
> + * are not defined, skip loading the firmware, assuming it is already loaded by TZ.
> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +int geni_load_se_firmware(struct geni_se *se,
> + enum geni_se_protocol_type protocol)

This line is 82 characters long if you don't line break it, but it's
cleaner to read...so please unbreak it.

> +{
> + struct qup_se_rsc rsc;
> + const char *fw_name;
> + int ret;
> +
> + ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
> + if (ret)
> + return -EINVAL;
> +
> + rsc.se = se;
> + rsc.protocol = protocol;
> + /* Set default xfer mode to FIFO */

This isn't the "default" mode, this is the else statement in the
conditional below.

> + rsc.mode = GENI_SE_FIFO;
> +
> + if (of_property_read_bool(se->dev->of_node, "qcom,gsi-dma-allowed"))
> + rsc.mode = GENI_GPI_DMA;

if (of_property_present())
sc.mode = GENI_GPI_DMA;
else
sc.mode = GENI_GPI_FIFO;

/* Comment about UART, because it's actually worth mentioning */
if (serial)

> +
> + /* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
> + if (protocol == GENI_SE_UART)
> + rsc.mode = GENI_SE_FIFO;
> +
> + ret = qup_fw_load(&rsc, fw_name);
> + if (ret)
> + return ret;
> +
> + dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
> + protocol_name[rsc.protocol], rsc.mode);

How do you know that rsc.protocol < ARRAY_SIZE(protocol_name)?

> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(geni_load_se_firmware);
> +
> static int geni_se_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index 2996a3c28ef3..fd8cf5c6791f 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
> */
>
> #ifndef _LINUX_QCOM_GENI_SE
> @@ -36,6 +37,7 @@ enum geni_se_protocol_type {
> GENI_SE_I2C,
> GENI_SE_I3C,
> GENI_SE_SPI_SLAVE,
> + GENI_SE_INVALID_PROTO = 255,

This is unused.

> };
>
> struct geni_wrapper;
> @@ -72,6 +74,19 @@ struct geni_se {
> struct geni_icc_path icc_paths[3];
> };
>
> +/**
> + * struct qup_se_rsc - Structure containing se details protocol and xfer mode
> + *
> + * @mode: transfer mode se fifo, dma or gsi.
> + * @protocol: Protocol spi or i2c or serial.
> + * @se: Pointer to the concerned serial engine.

I'd like you to drop this struct, so it doesn't really matter...but
order of parameters doesn't match the structure.

> + */
> +struct qup_se_rsc {
> + struct geni_se *se;
> + enum geni_se_xfer_mode mode;
> + enum geni_se_protocol_type protocol;
> +};
> +
> /* Common SE registers */
> #define GENI_FORCE_DEFAULT_REG 0x20
> #define GENI_OUTPUT_CTRL 0x24
> @@ -531,5 +546,8 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag);
> int geni_icc_enable(struct geni_se *se);
>
> int geni_icc_disable(struct geni_se *se);
> +
> +int geni_load_se_firmware(struct geni_se *se,
> + enum geni_se_protocol_type protocol);

Unbreak the line.

> #endif
> #endif
> diff --git a/include/linux/soc/qcom/qup-fw-load.h b/include/linux/soc/qcom/qup-fw-load.h
> new file mode 100644
> index 000000000000..f139720987f9
> --- /dev/null
> +++ b/include/linux/soc/qcom/qup-fw-load.h
> @@ -0,0 +1,179 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _LINUX_QCOM_QUP_FW_LOAD
> +#define _LINUX_QCOM_QUP_FW_LOAD
> +
> +#include <linux/device.h>
> +#include <linux/elf.h>
> +#include <linux/firmware.h>

Why are device.h, elf.h and firmware.h included here, they are not used.

> +#include <linux/kernel.h>
> +
> +/*Magic numbers*/
> +#define MAGIC_NUM_SE 0x57464553

Prefix things.

> +
> +/* Common SE registers*/
> +#define GENI_INIT_CFG_REVISION 0x0
> +#define GENI_S_INIT_CFG_REVISION 0x4
> +#define GENI_FORCE_DEFAULT_REG 0x20

A bunch of these registers and bitmasks area already defined in
include/soc/qcom/geni-se.h why are they duplicated here?

> +#define GENI_CGC_CTRL 0x28
> +#define GENI_CFG_REG0 0x100
> +
> +#define QUPV3_SE_HW_PARAM_1 0xE28

Lowercase hex digits please.

> +#define RX_FIFO_WIDTH_BIT 24
> +#define RX_FIFO_WIDTH_MASK 0x3F
> +
> +/*Same registers as GENI_DMA_MODE_EN*/
> +#define QUPV3_SE_GENI_DMA_MODE_EN 0x258
> +#define GENI_M_IRQ_ENABLE 0x614
> +#define GENI_S_IRQ_ENABLE 0x644
> +#define GENI_RX_RFR_WATERMARK_REG 0x814
> +#define DMA_TX_IRQ_EN_SET 0xC4C
> +#define DMA_RX_IRQ_EN_SET 0xD4C
> +#define DMA_GENERAL_CFG 0xE30
> +#define SE_GENI_FW_REVISION 0x1000
> +#define SE_S_FW_REVISION 0x1004
> +#define SE_GENI_CFG_RAMN 0x1010
> +#define SE_GENI_CLK_CTRL 0x2000
> +#define SE_DMA_IF_EN 0x2004
> +#define SE_FIFO_IF_DISABLE 0x2008
> +
> +#define MAX_GENI_CFG_RAMn_CNT 455
> +
> +#define MI_PBT_NON_PAGED_SEGMENT 0x0
> +#define MI_PBT_HASH_SEGMENT 0x2
> +#define MI_PBT_NOTUSED_SEGMENT 0x3
> +#define MI_PBT_SHARED_SEGMENT 0x4
> +#define MI_PBT_FLAG_PAGE_MODE_MASK 0x100000
> +#define MI_PBT_FLAG_PAGE_MODE_SHIFT 0x14
> +#define MI_PBT_FLAG_SEGMENT_TYPE_MASK 0x7000000
> +#define MI_PBT_FLAG_SEGMENT_TYPE_SHIFT 0x18
> +#define MI_PBT_FLAG_ACCESS_TYPE_MASK 0xE00000
> +#define MI_PBT_FLAG_ACCESS_TYPE_SHIFT 0x15
> +
> +#define MI_PBT_PAGE_MODE_VALUE(x) \
> + (((x) & MI_PBT_FLAG_PAGE_MODE_MASK) >> \
> + MI_PBT_FLAG_PAGE_MODE_SHIFT)

Please replace masking and shifting with FIELD_GET() from
linux/bitfield.h.

> +
> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) \
> + (((x) & MI_PBT_FLAG_SEGMENT_TYPE_MASK) >> \
> + MI_PBT_FLAG_SEGMENT_TYPE_SHIFT)
> +
> +#define MI_PBT_ACCESS_TYPE_VALUE(x) \
> + (((x) & MI_PBT_FLAG_ACCESS_TYPE_MASK) >> \
> + MI_PBT_FLAG_ACCESS_TYPE_SHIFT)
> +
> +/* GENI_FORCE_DEFAULT_REG fields */
> +#define FORCE_DEFAULT BIT(0)
> +
> +/* FW_REVISION_RO fields */
> +#define FW_REV_PROTOCOL_SHFT 8
> +#define FW_REV_VERSION_SHFT 0
> +
> +#define GENI_FW_REVISION_RO 0x68
> +#define GENI_S_FW_REVISION_RO 0x6C
> +
> +/* SE_GENI_DMA_MODE_EN */
> +#define GENI_DMA_MODE_EN BIT(0)
> +
> +/* GENI_M_IRQ_EN fields */
> +#define M_CMD_DONE_EN BIT(0)
> +#define M_IO_DATA_DEASSERT_EN BIT(22)
> +#define M_IO_DATA_ASSERT_EN BIT(23)
> +#define M_RX_FIFO_RD_ERR_EN BIT(24)
> +#define M_RX_FIFO_WR_ERR_EN BIT(25)
> +#define M_RX_FIFO_WATERMARK_EN BIT(26)
> +#define M_RX_FIFO_LAST_EN BIT(27)
> +#define M_TX_FIFO_RD_ERR_EN BIT(28)
> +#define M_TX_FIFO_WR_ERR_EN BIT(29)
> +#define M_TX_FIFO_WATERMARK_EN BIT(30)
> +#define M_COMMON_GENI_M_IRQ_EN (GENMASK(6, 1) | \
> + M_IO_DATA_DEASSERT_EN | \
> + M_IO_DATA_ASSERT_EN | M_RX_FIFO_RD_ERR_EN | \
> + M_RX_FIFO_WR_ERR_EN | M_TX_FIFO_RD_ERR_EN | \
> + M_TX_FIFO_WR_ERR_EN)
> +
> +/* GENI_S_IRQ_EN fields */
> +#define S_CMD_OVERRUN_EN BIT(1)
> +#define S_ILLEGAL_CMD_EN BIT(2)
> +#define S_CMD_CANCEL_EN BIT(4)
> +#define S_CMD_ABORT_EN BIT(5)
> +#define S_GP_IRQ_0_EN BIT(9)
> +#define S_GP_IRQ_1_EN BIT(10)
> +#define S_GP_IRQ_2_EN BIT(11)
> +#define S_GP_IRQ_3_EN BIT(12)
> +#define S_RX_FIFO_RD_ERR_EN BIT(24)
> +#define S_RX_FIFO_WR_ERR_EN BIT(25)
> +#define S_COMMON_GENI_S_IRQ_EN (GENMASK(5, 1) | GENMASK(13, 9) | \
> + S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN)
> +
> +#define GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK 0x00000200
> +#define GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK 0x00000100
> +
> +#define GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK 0x00000001
> +
> +#define DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK 0x00000008
> +#define DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK 0x00000004
> +#define DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK 0x00000001
> +
> +#define DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK 0x00000010
> +#define DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK 0x00000008
> +#define DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK 0x00000004
> +#define DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK 0x00000001
> +
> +#define DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK 0x00000008
> +#define DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK 0x00000004
> +#define DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK 0x00000002
> +#define DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK 0x00000001
> +
> +#define GENI_CLK_CTRL_SER_CLK_SEL_BMSK 0x00000001
> +#define DMA_IF_EN_DMA_IF_EN_BMSK 0x00000001
> +#define SE_GSI_EVENT_EN_BMSK 0x0000000f
> +#define SE_IRQ_EN_RMSK 0x0000000f
> +
> +#define QUPV3_COMMON_CFG 0x0120
> +#define FAST_SWITCH_TO_HIGH_DISABLE_BMASK 0x00000001
> +
> +#define QUPV3_SE_AHB_M_CFG 0x0118
> +#define AHB_M_CLK_CGC_ON_BMASK 0x00000001
> +
> +#define QUPV3_COMMON_CGC_CTRL 0x021C
> +#define COMMON_CSR_SLV_CLK_CGC_ON_BMASK 0x00000001
> +
> +/* access ports */
> +#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) | (_v))
> +#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))

These names are way too generic.

> +
> +#define out_be32(a, v) writel_relaxed(v, a)
> +#define in_be32(a) readl_relaxed(a)

Don't create aliases.

> +
> +/**
> + * struct elf_se_hdr - firmware configurations
> + *
> + * @magic: set to 'SEFW'
> + * @version: A 32-bit value indicating the structure’s version number
> + * @core_version: QUPV3_HW_VERSION
> + * @serial_protocol: Programmed into GENI_FW_REVISION
> + * @fw_version: Programmed into GENI_FW_REVISION
> + * @cfg_version: Programmed into GENI_INIT_CFG_REVISION
> + * @fw_size_in_items: Number of (uint32_t) GENI_FW_RAM words
> + * @fw_offset: Byte offset of GENI_FW_RAM array
> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
> + * @cfg_idx_offset: Byte offset of GENI_FW_CFG index array
> + * @cfg_val_offset: Byte offset of GENI_FW_CFG values array
> + */
> +struct elf_se_hdr {

This structure doesn't seem to relate to ELF at all, it seems to be the
header for the serial engine configuration.

It happens to be packaged up in a ELF segment, but I don't see a reason
for defining the format of the outer container here.

> + u32 magic;

What endian is that u32? Please be specific.

Regards,
Bjorn

> + u32 version;
> + u32 core_version;
> + u16 serial_protocol;
> + u16 fw_version;
> + u16 cfg_version;
> + u16 fw_size_in_items;
> + u16 fw_offset;
> + u16 cfg_size_in_items;
> + u16 cfg_idx_offset;
> + u16 cfg_val_offset;
> +};
> +#endif /* _LINUX_QCOM_QUP_FW_LOAD */
> --
> 2.34.1
>

Next message: Namhyung Kim: "[PATCH] perf report: Do not process non-JIT BPF ksymbol events"
Previous message: Jasjiv Singh: "Re: [PATCH v3] ipe: add errno field to IPE policy load auditing"
In reply to: kernel test robot: "Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH v3 0/9] Add support to load QUP SE firmware from"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]