| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <01850482-7648-4a20-bb02-f4956f3ae6d2@quicinc.com>
Date: Wed, 11 Jun 2025 17:14:15 +0530
From: Viken Dadhaniya <quic_vdadhani@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <andi.shyti@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <gregkh@...uxfoundation.org>,
<jirislaby@...nel.org>, <broonie@...nel.or>, <konradybcio@...nel.org>,
<johan+linaro@...nel.org>, <dianders@...omium.org>,
<agross@...nel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-serial@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <quic_msavaliy@...cinc.com>,
<quic_anupkulk@...cinc.com>
Subject: Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE
Firmware via Linux subsystem
On 4/22/2025 12:02 PM, Viken Dadhaniya wrote:
>
>
> On 3/6/2025 4:57 AM, Bjorn Andersson wrote:
>> 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@...cinc.com>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
>>> ---
>>> 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@...cinc.com/
>>>
>>> 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@...cinc.com/
>>> ---
>>> ---
>>> 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.
>
> This structure is related to firmware written in an ELF file. I didn't
> understand your concern. Could you please clarify what is required here?
>
If I understand your comments correctly, the current struct name does
not align with its functionality. shall i update it to
se_fw_config_hdr. Hope that matches the ELF as per functionality.
>>
>>> + 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
>>>
>
Powered by blists - more mailing lists