| 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: <ac2f3d9a-1dd5-48c0-abc3-b541720d244c@quicinc.com>
Date: Tue, 22 Apr 2025 12:02:23 +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 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@quicinc.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@quicinc.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?
>
>> + 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