drivers: qcom: add GENI SPI driver and enable spi config

[VeshalaAnilKumar]

Add a Qualcomm SPI geni driver implementing spi_ops for a GENI Serial Engine in FIFO transfer mode with interrupt-driven. Enabled CFG_QCOM_GENI_SPI in lemans platform, and add the corresponding qup config settings in qup_spi_config[] table.

[Selvam Sathappan (zelvam95)]

unbounded busy-waits on RX FIFO and M_GENI_CMD_ACTIVE; a stuck FIFO or hung SE hangs this call forever

while (io_read32(qs->base + SE_GENI_RX_FIFO_STATUS) & RX_FIFO_WC_MSK)
    io_read32(qs->base + SE_GENI_RX_FIFOn);

while (io_read32(qs->base + SE_GENI_STATUS) &
       GENI_STATUS_M_GENI_CMD_ACTIVE)
    ;

Both loops have no timeout. If the SE is in an error state (SW_ERR
that the driver doesn't yet check, bus fault, clock disabled), this
hangs OP-TEE indefinitely with interrupts disabled by the caller (any
spi_ops.flushfifo caller from SPI framework code). A stuck-forever
loop in secure world is significantly worse than a stuck-forever loop
in normal world.

The rest of the driver already uses the timeout_init_us(...) +
timeout_elapsed(...) idiom (see qup_spi_wait_flag); apply the same
bound here.

Suggest: cap both loops with timeout_init_us(QUP_SPI_M_CMD_TIMEOUT_US)
and EMSG(...) + return early on timeout. A caller can then observe
the failure via the next txrx attempting to error out cleanly.

[VeshalaAnilKumar]

sure, will add the changes

[Selvam Sathappan (zelvam95)]

qup_spi_configure() — returns void; on qcom_clk_get_domain() / geni_spi_resolve_clk() failure the SE runs with stale clock configuration and the next transfer produces wrong SPI SCLK

domain = qcom_clk_get_domain(qs->se_clk);
if (!domain) {
    EMSG("QUP SPI: no clock domain for SE %u", qs->id);
    return;              /* <-- SE not reconfigured */
}
res = geni_spi_resolve_clk(domain, qs->speed_hz, &dfs_idx, &clk_div);
if (res) {
    EMSG("QUP SPI: no DFS source rate for %u Hz: %#" PRIx32, ...);
    return;              /* <-- SE not reconfigured */
}

configure()'s spi_chip::ops signature returns void, so the driver
has no way to propagate the failure back to the caller. But this means
the caller's next .start() / .txrx() will run against whatever
clock state the SE was in before configure was called — potentially
the wrong CS, mode, packing, or bus speed. For a security-relevant SPI
target (e.g. an SPI-attached secure element or fTPM), running at a
speed the target rejects is a subtle failure mode; running at a speed
the target does accept but at the wrong CPHA/CPOL corrupts every
byte on the wire.

The safer contract is to set a qs->configured = true flag only on
full success, and to reject start/txrx when it is false.

Suggest: track configuration success in struct qup_spi_data
(e.g. bool configured), set it at the end of a fully-successful
qup_spi_configure(), clear it on any early-return, and short-circuit
qup_spi_start / qup_spi_txrx with an EMSG + no-op when the flag is
false.

[VeshalaAnilKumar]

agree, valid point, will add the changes in next patch

[Selvam Sathappan (zelvam95)]

AI Agents review comment on this function is as below. Can you please review this feedback and if applicable/plan to handle it?

🟡 core/drivers/spi/qcom/qcom_geni_spi.c:557-605 qup_spi_txrx() — for RX-only transfers (wdat == NULL), SE_SPI_TX_TRANS_LEN is still programmed to num_pkts but the TX FIFO is never filled and the TX watermark is never armed

io_write32(qs->base + SE_SPI_TX_TRANS_LEN, num_pkts & TRANS_LEN_MSK);   /* :568 */
io_write32(qs->base + SE_SPI_RX_TRANS_LEN, num_pkts & TRANS_LEN_MSK);   /* :569 */
...
qs->tx_rem_bytes = wdat ? total_bytes : 0;                              /* :574 */
...
if (qs->tx_rem_bytes) {                                                 /* :580 */
    io_write32(qs->base + SE_GENI_TX_WATERMARK_REG, 1);
    qup_spi_fifo_fill_tx(qs);
}

SPI is full-duplex — every clock cycle shifts a bit out on MOSI. For
an RX-only transfer, the SE must still be given something to
shift out (usually zeros). This driver programs SE_SPI_TX_TRANS_LEN = num_pkts (asserting to the SE that num_pkts TX words will be
provided) but then never fills the TX FIFO, so the SE will either
stall on TX_FIFO_WATERMARK (never rearmed) or drive whatever
uninitialised state the FIFO happened to contain onto MOSI. Either
outcome is a correctness bug for pure-RX transfers.

Two typical fixes: (a) for RX-only, set SE_SPI_TX_TRANS_LEN = 0 and
mask the TX-watermark IRQ; or (b) for RX-only, treat the transfer as
"transmit zeros" — fill the TX FIFO with 0-filled words until
num_pkts are shifted out.

Suggest: confirm the SE behaviour when TX_TRANS_LEN > 0 and no
TX FIFO fill is provided (this is documented in the QUP SE HAS), then
fix qup_spi_txrx to program TX_TRANS_LEN only when wdat != NULL,
or fill the TX FIFO with zeros. Similarly, TX-only transfers should
program RX_TRANS_LEN = 0 and disable the RX-watermark IRQ.

[VeshalaAnilKumar]

it's valid point, Rx only or Tx only can possible, will add recommended change

[Selvam Sathappan (zelvam95)]

qup_spi_start()/qup_spi_end() — clock ungate errors in start are dropped silently; end still deasserts CS and gates the clock

static void qup_spi_start(struct spi_chip *chip)
{
    ...
    if (qup_spi_clk_enable(qs))
        return;    /* <-- start() returns void, error is lost */
    ...
    geni_setup_m_cmd(qs, SPI_CS_ASSERT, 0);
    ...
}

If qup_spi_clk_enable() fails, qup_spi_start returns early — CS
is never asserted, the caller's subsequent .txrx* runs against a
gated SE (the M_CMD write will land against a clock-gated peripheral),
and .end() will try to deassert CS that was never asserted. Same
observation as configure() (item 5): the spi_ops op is void, so
there is no error propagation. A qs->started flag gated on full
success of start() would short-circuit txrx/end when start
failed.

spi_ops.start/.end/.configure all being void is an OP-TEE
framework limitation, but this driver can defend against it with a
single boolean.

[VeshalaAnilKumar]

yes it's valid point, will go with recommended changes

[Selvam Sathappan (zelvam95)]

qup_spi_init() — qs->id used by qup_spi_get_platform_data()'s trace before being assigned

if (!qup_spi_get_platform_data(qup_spi_id, qs))     /* uses qs->id internally? */
    return TEE_ERROR_ITEM_NOT_FOUND;

qs->id = qup_spi_id;                                /* <-- assigned here */

qup_spi_get_platform_data() at line 813 does:

DMSG("QUP SPI %u: base=%#" PRIxVA " irq=%zu se_clk=%s",
     qup_spi_id, qs->base, qs->itr_num, qs->se_clock_name);

Note it uses the parameter qup_spi_id, not qs->id — so this is
actually fine. But qup_spi_clk_setup() (called after
qup_spi_get_platform_data but before qs->id = qup_spi_id) does
EMSG("QUP SPI: no se clock name for SE %u", qs->id) — printing 0
instead of the actual SE id. Consequential only for log messages, but
easy to fix by assigning qs->id = qup_spi_id before
qup_spi_get_platform_data(), or by having get_platform_data do the
assignment itself.

[Selvam Sathappan (zelvam95)]

geni_config_packing() — silent no-op if bpw < 1 or bpw > 32; also int iter = (ceil_bpw * 1) / 8; has a stray * 1

int ceil_bpw = ROUNDUP(bpw, 8);
int iter = (ceil_bpw * 1) / 8;

Two nits:

• (ceil_bpw * 1) / 8 — the * 1 reads like a leftover from a
  templated formula (maybe scaled per byte-vector count?). It reduces
  to ceil_bpw / 8. Either drop * 1 or add a comment explaining why
  it's there.
• The guard if (iter <= 0 || iter > NUM_PACKING_VECTORS) return;
  silently no-ops for invalid bpw. Since configure() already
  assert()s bpw >= MIN_WORD_LEN && bpw <= 32, this can only be
  reached in a release build with the assert compiled out — in that
  case a silent no-op is worse than an EMSG.

---

[Selvam Sathappan (zelvam95)]

geni_config_packing() — SE_GENI_BYTE_GRAN written only when bpw == 32

if (bpw == 32)
    io_write32(qs->base + SE_GENI_BYTE_GRAN, bpw / 16);

The Linux kernel GENI SE driver programs BYTE_GRAN for any
bpw > 8 (see drivers/spi/spi-geni-qcom.c in mainline). Confirm
against the QUP SE hardware spec that bpw > 16 on this SoC does not
require BYTE_GRAN != default; if it does, this branch is missing
cases 17..31.

[Selvam Sathappan (zelvam95)]

header uses both #pragma once and #ifndef guards; and the guard is __DRIVERS_QCOM_GENI_SPI_H (leading-underscore identifier reserved by C standard to the implementation)

#pragma once

#ifndef __DRIVERS_QCOM_GENI_SPI_H
#define __DRIVERS_QCOM_GENI_SPI_H

OP-TEE headers uniformly use plain include guards (e.g.
DRIVERS_QCOM_GENI_SPI_H) without #pragma once. Two of the three
lines here are redundant, and the guard uses a name reserved to the
implementation (__ prefix). Either delete #pragma once and rename
the guard to DRIVERS_QCOM_GENI_SPI_H, or drop the #ifndef pair
and keep only #pragma once — but the repo convention is the former.

---

[Selvam Sathappan (zelvam95)]

— SPDX header is BSD-3-Clause while every other file in the PR is BSD-2-Clause

# SPDX-License-Identifier: BSD-3-Clause

All other new files in this PR (qcom_geni_spi.c, qcom_geni_spi.h,
qcom_geni_spi_config.c, platform/lemans/sub.mk, platform/sub.mk)
are BSD-2-Clause. Sub-make files inherit the surrounding directory's
licence — this appears to be a copy-paste artefact from a different
subsystem. Change to BSD-2-Clause for consistency.

Also, check if sub.mk really requires copyright markings? Check other existing sub.mk files for ref.

---

[Selvam Sathappan (zelvam95)]

-- set it to the real value for your target.

This part of it should we remove from this file? Its in a target specific file, someone else enabling this needs to ensure to define this macro for the respective targets? That can be taken as ref.? Explicit comment needed?

[Copilot]

Pull request overview

Adds initial Qualcomm GENI (QUPv3) SPI support to OP-TEE by introducing a new interrupt-driven FIFO-mode SPI driver, wiring it into the core drivers build, and providing a Lemans platform configuration plus build enablement.

Changes:

• Introduces a new spi_ops implementation for Qualcomm GENI SPI (FIFO + interrupt-driven).
• Adds build system plumbing for a new core/drivers/spi/ subtree and Qualcomm SPI driver selection via CFG_QCOM_GENI_SPI.
• Adds Lemans-specific GENI SPI instance configuration (qup_spi_config[]) and enables the driver in the Lemans target.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
core/include/drivers/qcom_geni_spi.h	Adds public driver API and platform configuration structures for GENI SPI.
core/drivers/sub.mk	Includes the new spi driver subtree in the build.
core/drivers/spi/sub.mk	Adds Qualcomm SPI subdirectory.
core/drivers/spi/qcom/sub.mk	Builds the GENI SPI driver and platform subdir when CFG_QCOM_GENI_SPI is enabled.
core/drivers/spi/qcom/qcom_geni_spi.c	Implements GENI SPI driver (clocking, pinctrl, IRQ-driven FIFO TX/RX).
core/drivers/spi/qcom/platform/sub.mk	Selects platform flavor subdir for SPI config.
core/drivers/spi/qcom/platform/lemans/sub.mk	Builds the Lemans GENI SPI config source when enabled.
core/drivers/spi/qcom/platform/lemans/qcom_geni_spi_config.c	Provides Lemans QUP SPI2 instance configuration (base/irq/clocks/pins).
core/arch/arm/plat-qcom/hoya/lemans/target.mk	Enables CFG_QCOM_GENI_SPI for the Lemans platform.

Suppressed comments (1)

core/drivers/spi/qcom/qcom_geni_spi.c:575

• qup_spi_txrx() programs both TX and RX transfer lengths to num_pkts, but it only sets tx_rem_bytes/rx_rem_bytes when the corresponding buffer pointer is non-NULL. This breaks write-only/read-only transfers: RX FIFO may fill without being drained, and read-only transfers never push dummy bytes, potentially stalling until timeout. Align the internal byte counters with the programmed hardware lengths so the ISR will always service both directions (discarding RX when rx_buf is NULL and sending zeroes when tx_buf is NULL).

	qs->tx_buf = wdat;
	qs->rx_buf = rdat;
	qs->tx_rem_bytes = wdat ? total_bytes : 0;
	qs->rx_rem_bytes = rdat ? total_bytes : 0;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Jorge A. Ramirez-Ortiz (ldts)]

VeshalaAnilKumar please follow up on the review comments (including Copilots)

[VeshalaAnilKumar]

VeshalaAnilKumar please follow up on the review comments (including Copilots)

Sure Jorge