* [PATCH v5 13/17] spi: cadence-quadspi: reprogram CS timing on every chip-select switch
From: Santhosh Kumar K @ 2026-07-17 18:51 UTC (permalink / raw)
To: broonie, robh, krzk+dt, conor+dt, miquel.raynal, richard,
vigneshr, pratyush, mwalle, takahiro.kuwano
Cc: linux-spi, devicetree, linux-kernel, linux-mtd, praneeth,
u-kumar1, a-dutta, s-k6
In-Reply-To: <20260717185116.2065505-1-s-k6@ti.com>
cqspi_configure() only reprogrammed per-device CS timing (CQSPI_REG_DELAY)
and the non-PHY read-capture register when the clock frequency changed, not
when the chip-select changed. In a multi-device setup where both devices
operate at the same frequency, a CS switch leaves the previous device's
timing parameters in hardware.
Split the update condition: baud rate divisor is updated on clock change
only (it is frequency-derived), but the per-device delay and read-capture
registers are now updated on any CS or clock switch.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
drivers/spi/spi-cadence-quadspi.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 542ad331ff16..8bf3a5a8bf43 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1570,10 +1570,19 @@ static void cqspi_configure(struct cqspi_flash_pdata *f_pdata,
cqspi_chipselect(f_pdata);
}
- /* Setup baudrate divisor and delays */
+ /* Update baudrate only on clock change. */
if (switch_ck) {
cqspi->sclk = sclk;
cqspi_config_baudrate_div(cqspi);
+ }
+
+ /*
+ * Reprogram per-device CS timing and non-PHY read capture on any
+ * chip-select or clock switch. Without this, two devices at the same
+ * frequency would each see the other's timing parameters after a CS
+ * switch, since the lazy clock-only check would never trigger.
+ */
+ if (switch_cs || switch_ck) {
cqspi_delay(f_pdata);
cqspi_readdata_capture(cqspi, !cqspi->rclk_en, false,
f_pdata->read_delay);
--
2.34.1
^ permalink raw reply related
* [PATCH v5 12/17] spi: cadence-quadspi: enable PHY for indirect writes
From: Santhosh Kumar K @ 2026-07-17 18:51 UTC (permalink / raw)
To: broonie, robh, krzk+dt, conor+dt, miquel.raynal, richard,
vigneshr, pratyush, mwalle, takahiro.kuwano
Cc: linux-spi, devicetree, linux-kernel, linux-mtd, praneeth,
u-kumar1, a-dutta, s-k6
In-Reply-To: <20260717185116.2065505-1-s-k6@ti.com>
Enable PHY for indirect writes of at least CQSPI_PHY_MIN_INDIRECT_WRITE_LEN
bytes. PHY is activated only when tuning completed successfully and the
write op runs at the calibrated post-config frequency, matching the same
frequency guard used by the read path.
Thread post_config_max_speed_hz from cqspi_mem_process() through
cqspi_write() into cqspi_indirect_write_execute() for the frequency check.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
drivers/spi/spi-cadence-quadspi.c | 32 +++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 35a400073944..542ad331ff16 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -153,6 +153,8 @@ struct cqspi_driver_platdata {
#define CQSPI_DLL_TIMEOUT_US 300
/* Minimum transfer length to use DMA for direct reads */
#define CQSPI_PHY_MIN_DIRECT_READ_LEN 17
+/* Minimum indirect write length to amortize PHY enable/disable overhead */
+#define CQSPI_PHY_MIN_INDIRECT_WRITE_LEN SZ_1K
/* Runtime_pm autosuspend delay */
#define CQSPI_AUTOSUSPEND_TIMEOUT 2000
@@ -1335,13 +1337,15 @@ static int cqspi_write_setup(struct cqspi_flash_pdata *f_pdata,
static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
loff_t to_addr, const u8 *txbuf,
- const size_t n_tx)
+ const size_t n_tx,
+ u32 post_config_max_speed_hz)
{
struct cqspi_st *cqspi = f_pdata->cqspi;
struct device *dev = &cqspi->pdev->dev;
void __iomem *reg_base = cqspi->iobase;
unsigned int remaining = n_tx;
unsigned int write_bytes;
+ bool use_tuned_phy_write;
int ret;
if (!refcount_read(&cqspi->refcount))
@@ -1377,6 +1381,18 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
if (cqspi->apb_ahb_hazard)
readl(reg_base + CQSPI_REG_INDIRECTWR);
+ /* Use PHY only for large writes at the calibrated rate */
+ use_tuned_phy_write = n_tx >= CQSPI_PHY_MIN_INDIRECT_WRITE_LEN &&
+ f_pdata->use_tuned_phy &&
+ f_pdata->phy_write_op.max_freq ==
+ post_config_max_speed_hz;
+
+ if (use_tuned_phy_write) {
+ ret = cqspi_tune_phy(f_pdata, true);
+ if (ret)
+ goto failwr;
+ }
+
while (remaining > 0) {
size_t write_words, mod_bytes;
@@ -1425,9 +1441,15 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
cqspi_wait_idle(cqspi);
+ if (use_tuned_phy_write)
+ cqspi_tune_phy(f_pdata, false);
+
return 0;
failwr:
+ if (use_tuned_phy_write)
+ cqspi_tune_phy(f_pdata, false);
+
/* Disable interrupt. */
writel(0, reg_base + CQSPI_REG_IRQMASK);
@@ -1562,7 +1584,8 @@ static void cqspi_configure(struct cqspi_flash_pdata *f_pdata,
}
static ssize_t cqspi_write(struct cqspi_flash_pdata *f_pdata,
- const struct spi_mem_op *op)
+ const struct spi_mem_op *op,
+ u32 post_config_max_speed_hz)
{
struct cqspi_st *cqspi = f_pdata->cqspi;
loff_t to = op->addr.val;
@@ -1589,7 +1612,8 @@ static ssize_t cqspi_write(struct cqspi_flash_pdata *f_pdata,
return cqspi_wait_idle(cqspi);
}
- return cqspi_indirect_write_execute(f_pdata, to, buf, len);
+ return cqspi_indirect_write_execute(f_pdata, to, buf, len,
+ post_config_max_speed_hz);
}
static bool cqspi_use_tuned_phy(struct cqspi_flash_pdata *f_pdata,
@@ -1805,7 +1829,7 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
if (!op->addr.nbytes || !op->data.buf.out)
return cqspi_command_write(f_pdata, op);
- return cqspi_write(f_pdata, op);
+ return cqspi_write(f_pdata, op, mem->spi->post_config_max_speed_hz);
}
static int cqspi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
--
2.34.1
^ permalink raw reply related
* [PATCH v5 15/17] mtd: spinand: negotiate optimal controller operating point before dirmap creation
From: Santhosh Kumar K @ 2026-07-17 18:51 UTC (permalink / raw)
To: broonie, robh, krzk+dt, conor+dt, miquel.raynal, richard,
vigneshr, pratyush, mwalle, takahiro.kuwano
Cc: linux-spi, devicetree, linux-kernel, linux-mtd, praneeth,
u-kumar1, a-dutta, s-k6
In-Reply-To: <20260717185116.2065505-1-s-k6@ti.com>
Dirmap descriptors encode the op template including the operating
frequency at creation time, so controller optimization must complete
before dirmaps are created to ensure the validated frequency is
embedded in the descriptors from the start.
Move dirmap creation from spinand_init() to spinand_probe(), after a
new spinand_optimize_controller() call that negotiates the best
available operating point. spinand_optimize_controller() tries the
pre-selected variant first. If the controller signals that optimization
is not applicable for that op, spinand_try_ranked_variant() iterates
remaining variants in performance order — ODTR variants first, then
SSDR variants after switching the bus interface if needed. On full
failure the device falls back to the best available non-optimized mode.
Add spinand_reset_max_freq_ops() to copy op templates with max_freq
zeroed before each execute_tuning call, enforcing the invariant that a
non-zero max_freq only results from a successful operation.
Optimization failure is never fatal; the device operates at the
conservative base rate.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
drivers/mtd/nand/spi/core.c | 206 ++++++++++++++++++++++++++++++++++--
include/linux/mtd/spinand.h | 11 ++
2 files changed, 208 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 7c3341f1fca0..b79d06d9ddaf 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1284,6 +1284,7 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
info.length = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
info.primary_op_tmpl = *spinand->op_templates->update_cache;
info.primary_op_tmpl.data.ecc = enable_ecc;
+ info.primary_op_tmpl.max_freq = spinand->max_write_op.max_freq;
desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
spinand->spimem, &info);
if (IS_ERR(desc))
@@ -1294,9 +1295,11 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
/* Read descriptor */
info.primary_op_tmpl = *spinand->op_templates->read_cache;
info.primary_op_tmpl.data.ecc = enable_ecc;
+ info.primary_op_tmpl.max_freq = spinand->max_read_op.max_freq;
if (secondary_op) {
info.secondary_op_tmpl = *spinand->op_templates->cont_read_cache;
info.secondary_op_tmpl.data.ecc = enable_ecc;
+ info.secondary_op_tmpl.max_freq = spinand->max_read_op.max_freq;
}
desc = spinand_create_rdesc(spinand, &info);
if (IS_ERR(desc))
@@ -1745,6 +1748,17 @@ int spinand_match_and_init(struct spinand_device *spinand,
spinand->cont_read_possible = false;
}
+ /*
+ * Save the full read variant list (ODTR and SSDR ops) for
+ * ranked controller optimization. Only saved when all ODTR
+ * templates are valid; spinand_optimize_controller() uses this
+ * to fall back to the next-best variant when needed.
+ */
+ if (spinand->odtr_op_templates.read_cache &&
+ spinand->odtr_op_templates.write_cache &&
+ spinand->odtr_op_templates.update_cache)
+ spinand->all_read_variants = info->op_variants.read_cache;
+
return 0;
}
@@ -1923,7 +1937,6 @@ static int spinand_mtd_suspend(struct mtd_info *mtd)
static int spinand_init(struct spinand_device *spinand)
{
- struct device *dev = &spinand->spimem->spi->dev;
struct mtd_info *mtd = spinand_to_mtd(spinand);
struct nand_device *nand = mtd_to_nanddev(mtd);
int ret;
@@ -2015,14 +2028,6 @@ static int spinand_init(struct spinand_device *spinand)
mtd->ecc_step_size = nanddev_get_ecc_conf(nand)->step_size;
mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3, 4);
- ret = spinand_create_dirmaps(spinand);
- if (ret) {
- dev_err(dev,
- "Failed to create direct mappings for read/write operations (err = %d)\n",
- ret);
- goto err_cleanup_ecc_engine;
- }
-
return 0;
err_cleanup_ecc_engine:
@@ -2051,6 +2056,175 @@ static void spinand_cleanup(struct spinand_device *spinand)
kfree(spinand->scratchbuf);
}
+/*
+ * spinand_try_ranked_variant() - Try controller optimization on variants in
+ * performance order.
+ * @spinand: SPI NAND device
+ * @mem: SPI memory device
+ * @iface: bus interface to iterate (ODTR or SSDR)
+ * @tried_mask: bitmask of already-tried variant indices; updated on each try
+ *
+ * Iterates the full read variant list in descending performance order,
+ * skipping variants in @tried_mask, and calls execute_tuning on each until
+ * one succeeds. Ranked iteration finds the best available variant without
+ * re-trying already-attempted ones.
+ *
+ * On success, sets spinand->max_read_op and updates the matching
+ * odtr_op_templates.read_cache or ssdr_op_templates.read_cache.
+ */
+static bool spinand_try_ranked_variant(struct spinand_device *spinand,
+ struct spi_mem *mem,
+ enum spinand_bus_interface iface,
+ u32 *tried_mask)
+{
+ const struct spinand_op_variants *variants = spinand->all_read_variants;
+ const struct spi_mem_op *best;
+ int ret;
+
+ if (!variants)
+ return false;
+
+ while ((best = spinand_op_find_best_variant(spinand, variants, iface,
+ *tried_mask))) {
+ *tried_mask |= BIT(best - variants->ops);
+ spinand->max_read_op = *best;
+ spinand->max_read_op.max_freq = 0;
+ ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
+ &spinand->max_write_op);
+ if (ret && ret != -EOPNOTSUPP)
+ dev_dbg(&mem->spi->dev, "%s optimization failed: %d\n",
+ iface == ODTR ? "ODTR" : "SSDR", ret);
+ if (!ret && spinand->max_read_op.max_freq) {
+ if (iface == ODTR)
+ spinand->odtr_op_templates.read_cache = best;
+ else
+ spinand->ssdr_op_templates.read_cache = best;
+ return true;
+ }
+ }
+ return false;
+}
+
+/*
+ * spinand_reset_max_freq_ops() - Copy op templates and zero max_freq on both.
+ * @spinand: SPI NAND device
+ * @templates: op template set to copy from
+ *
+ * Called before execute_tuning so max_freq starts at zero; execute_tuning sets
+ * it to the validated clock rate only on success. A non-zero max_freq means
+ * controller-optimized; zero means the base rate applies.
+ */
+static void spinand_reset_max_freq_ops(struct spinand_device *spinand,
+ struct spinand_mem_ops *templates)
+{
+ spinand->max_read_op = *templates->read_cache;
+ spinand->max_read_op.max_freq = 0;
+ spinand->max_write_op = *templates->write_cache;
+ spinand->max_write_op.max_freq = 0;
+}
+
+/*
+ * spinand_optimize_controller() - Negotiate the optimal controller operating
+ * point for the SPI NAND device.
+ * @spinand: SPI NAND device
+ * @mem: SPI memory device
+ *
+ * Tries the pre-selected variant first. If the controller signals that
+ * optimization is not applicable for that specific op, iterates all remaining
+ * variants in performance order. For devices that support both DTR and SDR
+ * interfaces, DTR variants are tried first; if all fail the device is
+ * switched to SDR mode and SDR variants are tried. On full failure the
+ * device falls back to the best available non-optimized mode. Devices that
+ * support only SDR skip the DTR ranked pass entirely.
+ *
+ * Optimization failure is never fatal.
+ *
+ * Note: tried_mask is u32, supporting up to 32 variants total across both
+ * ODTR and SSDR. Flash devices with more than 32 read variants are not
+ * supported.
+ */
+static void spinand_optimize_controller(struct spinand_device *spinand,
+ struct spi_mem *mem)
+{
+ u32 tried_mask;
+ int ret;
+
+ /* Skip entirely when no post-config target is configured. */
+ if (!mem->spi->post_config_max_speed_hz)
+ return;
+
+ spinand_reset_max_freq_ops(spinand, spinand->op_templates);
+
+ ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
+ &spinand->max_write_op);
+ if (ret && ret != -EOPNOTSUPP)
+ dev_dbg(&mem->spi->dev, "Controller optimization failed: %d\n",
+ ret);
+
+ /*
+ * Any non-zero return or a set max_freq means we are done (error,
+ * unsupported, or success). Fallback only for the op-specific "skip"
+ * signal: ret == 0 with max_freq still 0.
+ */
+ if (ret || spinand->max_read_op.max_freq)
+ return;
+
+ /* SSDR-only devices have no ranked ODTR fallback available. */
+ if (spinand->bus_iface == SSDR || !spinand->all_read_variants)
+ return;
+
+ if (WARN_ON(spinand->all_read_variants->nops > 32))
+ return;
+
+ /* Mark the pre-selected ODTR variant as already tried. */
+ tried_mask = BIT(spinand->odtr_op_templates.read_cache -
+ spinand->all_read_variants->ops);
+
+ dev_dbg(&mem->spi->dev,
+ "Optimization skipped for current op; searching for best variant\n");
+
+ /* Pass 1: try all remaining ODTR variants in performance order. */
+ if (spinand_try_ranked_variant(spinand, mem, ODTR, &tried_mask))
+ return;
+
+ /*
+ * Pass 2: switch to SSDR and try all SSDR variants in performance
+ * order. configure_chip is guaranteed non-NULL here: reaching ODTR
+ * mode requires it.
+ */
+ if (WARN_ON(!spinand->configure_chip))
+ goto use_odtr_fallback;
+
+ if (spinand->configure_chip(spinand, SSDR))
+ goto use_odtr_fallback;
+
+ spinand->op_templates = &spinand->ssdr_op_templates;
+ spinand->bus_iface = SSDR;
+ spinand->max_write_op = *spinand->ssdr_op_templates.write_cache;
+ spinand->max_write_op.max_freq = 0;
+
+ /* Only ODTR variants were candidates in Pass 1; SSDR bits are clear. */
+ if (spinand_try_ranked_variant(spinand, mem, SSDR, &tried_mask))
+ return;
+
+ /*
+ * All attempts exhausted. Revert to ODTR for non-optimized DTR
+ * operation. If revert fails, stay in SSDR — a mode mismatch
+ * (ODTR op templates on SSDR-mode device) would corrupt data.
+ */
+ if (spinand->configure_chip(spinand, ODTR)) {
+ dev_warn(&mem->spi->dev,
+ "Failed to revert to ODTR, staying in SSDR\n");
+ spinand_reset_max_freq_ops(spinand, &spinand->ssdr_op_templates);
+ return;
+ }
+
+use_odtr_fallback:
+ spinand->op_templates = &spinand->odtr_op_templates;
+ spinand->bus_iface = ODTR;
+ spinand_reset_max_freq_ops(spinand, &spinand->odtr_op_templates);
+}
+
static int spinand_probe(struct spi_mem *mem)
{
struct spinand_device *spinand;
@@ -2073,6 +2247,20 @@ static int spinand_probe(struct spi_mem *mem)
if (ret)
return ret;
+ /*
+ * Negotiate the best controller operating point before creating dirmaps
+ * so the validated frequency is available at dirmap construction time.
+ */
+ spinand_optimize_controller(spinand, mem);
+
+ ret = spinand_create_dirmaps(spinand);
+ if (ret) {
+ dev_err(&mem->spi->dev,
+ "Failed to create direct mappings for read/write operations (err = %d)\n",
+ ret);
+ goto err_spinand_cleanup;
+ }
+
ret = mtd_device_register(mtd, NULL, 0);
if (ret)
goto err_spinand_cleanup;
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 5f4c00ae72a7..7eacda949719 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -792,8 +792,19 @@ struct spinand_device {
struct spinand_mem_ops *op_templates;
enum spinand_bus_interface bus_iface;
+ /*
+ * Full read variant list (ODTR and SSDR ops together), saved when ODTR
+ * templates are valid. Used by spinand_optimize_controller() for ranked
+ * fallback when the pre-selected variant cannot be controller-optimized.
+ */
+ const struct spinand_op_variants *all_read_variants;
+
struct spinand_dirmap *dirmaps;
+ /* Persistent op templates updated by execute_tuning with validated speed. */
+ struct spi_mem_op max_read_op;
+ struct spi_mem_op max_write_op;
+
int (*select_target)(struct spinand_device *spinand,
unsigned int target);
unsigned int cur_target;
--
2.34.1
^ permalink raw reply related
* [PATCH v5 14/17] mtd: spinand: extract variant ranking logic into spinand_op_find_best_variant()
From: Santhosh Kumar K @ 2026-07-17 18:51 UTC (permalink / raw)
To: broonie, robh, krzk+dt, conor+dt, miquel.raynal, richard,
vigneshr, pratyush, mwalle, takahiro.kuwano
Cc: linux-spi, devicetree, linux-kernel, linux-mtd, praneeth,
u-kumar1, a-dutta, s-k6
In-Reply-To: <20260717185116.2065505-1-s-k6@ti.com>
Refactor spinand_select_op_variant() to extract the inner loop into a
shared helper spinand_op_find_best_variant(). The helper takes a
skip_mask bitmask of already-tried variant indices, enabling callers to
iterate variants in ranked performance order while skipping those
already attempted. spinand_select_op_variant() becomes a one-liner
wrapper. No functional change.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
drivers/mtd/nand/spi/core.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 8bf9301f25e7..7c3341f1fca0 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1542,9 +1542,22 @@ static int spinand_init_odtr_instruction_set(struct spinand_device *spinand)
return 0;
}
+/*
+ * spinand_op_find_best_variant() - Find the fastest eligible op variant.
+ * @spinand: SPI NAND device
+ * @variants: full variant list to search
+ * @iface: bus interface to consider (ODTR or SSDR)
+ * @skip_mask: bitmask of variant indices to skip (already tried)
+ *
+ * Iterates @variants, evaluates transfer duration for each eligible op, and
+ * returns a pointer to the fastest one not in @skip_mask. Returns NULL when
+ * no eligible variant remains. Used by both variant selection at init time
+ * (skip_mask == 0) and ranked PHY tuning iteration.
+ */
static const struct spi_mem_op *
-spinand_select_op_variant(struct spinand_device *spinand, enum spinand_bus_interface iface,
- const struct spinand_op_variants *variants)
+spinand_op_find_best_variant(struct spinand_device *spinand,
+ const struct spinand_op_variants *variants,
+ enum spinand_bus_interface iface, u32 skip_mask)
{
struct nand_device *nand = spinand_to_nand(spinand);
const struct spi_mem_op *best_variant = NULL;
@@ -1552,15 +1565,16 @@ spinand_select_op_variant(struct spinand_device *spinand, enum spinand_bus_inter
unsigned int i;
for (i = 0; i < variants->nops; i++) {
- struct spi_mem_op op = variants->ops[i];
+ struct spi_mem_op op;
u64 op_duration_ns = 0;
unsigned int nbytes;
int ret;
- if ((iface == SSDR && spinand_op_is_odtr(&op)) ||
- (iface == ODTR && !spinand_op_is_odtr(&op)))
+ if ((skip_mask & BIT(i)) ||
+ spinand_op_is_odtr(&variants->ops[i]) != (iface == ODTR))
continue;
+ op = variants->ops[i];
nbytes = nanddev_per_page_oobsize(nand) +
nanddev_page_size(nand);
@@ -1589,6 +1603,14 @@ spinand_select_op_variant(struct spinand_device *spinand, enum spinand_bus_inter
return best_variant;
}
+static const struct spi_mem_op *
+spinand_select_op_variant(struct spinand_device *spinand,
+ enum spinand_bus_interface iface,
+ const struct spinand_op_variants *variants)
+{
+ return spinand_op_find_best_variant(spinand, variants, iface, 0);
+}
+
/**
* spinand_match_and_init() - Try to find a match between a device ID and an
* entry in a spinand_info table
--
2.34.1
^ permalink raw reply related
* [PATCH v5 17/17] mtd: spi-nor: run controller optimization before dirmap creation
From: Santhosh Kumar K @ 2026-07-17 18:51 UTC (permalink / raw)
To: broonie, robh, krzk+dt, conor+dt, miquel.raynal, richard,
vigneshr, pratyush, mwalle, takahiro.kuwano
Cc: linux-spi, devicetree, linux-kernel, linux-mtd, praneeth,
u-kumar1, a-dutta, s-k6
In-Reply-To: <20260717185116.2065505-1-s-k6@ti.com>
Run controller optimization in spi_nor_probe() before creating dirmaps
so the validated frequency is available at dirmap construction time.
Store the configured read op template in nor->max_read_op and pass it
to spi_mem_execute_tuning(). On success the controller sets
max_read_op.max_freq to the calibrated rate.
spi_nor_spimem_get_read_op() propagates nor->max_read_op.max_freq into
every op it returns, so the validated frequency flows automatically into
the dirmap template and into regular read ops.
Optimization failure is non-fatal; the device operates at the
conservative base rate.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
drivers/mtd/spi-nor/core.c | 14 ++++++++++++++
include/linux/mtd/spi-nor.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index b683c077a233..2efcdc7f637f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -216,6 +216,9 @@ static struct spi_mem_op spi_nor_spimem_get_read_op(struct spi_nor *nor)
if (spi_nor_protocol_is_dtr(nor->read_proto))
op.dummy.nbytes *= 2;
+ /* Propagate the validated frequency; zero before tuning. */
+ op.max_freq = nor->max_read_op.max_freq;
+
return op;
}
@@ -3843,6 +3846,17 @@ static int spi_nor_probe(struct spi_mem *spimem)
return -ENOMEM;
}
+ /*
+ * Populate the persistent template and run PHY tuning before dirmap
+ * creation so the validated frequency feeds into the dirmap op.
+ * Tuning failure is non-fatal; the device operates at base speed.
+ */
+ nor->max_read_op = spi_nor_spimem_get_read_op(nor);
+
+ ret = spi_mem_execute_tuning(spimem, &nor->max_read_op, NULL);
+ if (ret && ret != -EOPNOTSUPP)
+ dev_dbg(dev, "Controller optimization failed: %d\n", ret);
+
ret = spi_nor_create_read_dirmap(nor);
if (ret)
return ret;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4b92494827b1..ab498a50f15f 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -422,6 +422,9 @@ struct spi_nor {
struct spi_mem_dirmap_desc *wdesc;
} dirmap;
+ /* Persistent op template updated by execute_tuning with validated speed. */
+ struct spi_mem_op max_read_op;
+
void *priv;
};
--
2.34.1
^ permalink raw reply related
* [PATCH v5 16/17] mtd: spi-nor: extract read op template construction into helper
From: Santhosh Kumar K @ 2026-07-17 18:51 UTC (permalink / raw)
To: broonie, robh, krzk+dt, conor+dt, miquel.raynal, richard,
vigneshr, pratyush, mwalle, takahiro.kuwano
Cc: linux-spi, devicetree, linux-kernel, linux-mtd, praneeth,
u-kumar1, a-dutta, s-k6
In-Reply-To: <20260717185116.2065505-1-s-k6@ti.com>
From: Pratyush Yadav <pratyush@kernel.org>
Extract spi_nor_spimem_get_read_op() from the duplicated op-template
construction in spi_nor_spimem_read_data() and
spi_nor_create_read_dirmap().
Using data.nbytes = 2 as a non-zero placeholder lets
spi_nor_spimem_setup_op() configure the data buswidth without a separate
override; callers replace data.nbytes with the actual transfer length
before use. No functional change.
Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ccf4396cdcd0..b683c077a233 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -188,6 +188,37 @@ static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
return nor->controller_ops->erase(nor, offs);
}
+/**
+ * spi_nor_spimem_get_read_op() - build a configured read op template
+ * @nor: the spi-nor device
+ *
+ * Returns a spi_mem_op with the command, address format, dummy cycles,
+ * and data buswidth configured for @nor. For direct reads, the caller
+ * must fill in addr.val, data.nbytes, and data.buf.in before use.
+ */
+static struct spi_mem_op spi_nor_spimem_get_read_op(struct spi_nor *nor)
+{
+ /*
+ * data.nbytes must be non-zero so spi_nor_spimem_setup_op()
+ * configures the data buswidth; callers replace it with the
+ * actual transfer length.
+ */
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
+ SPI_MEM_OP_ADDR(nor->addr_nbytes, 0, 0),
+ SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
+ SPI_MEM_OP_DATA_IN(2, NULL, 0));
+
+ spi_nor_spimem_setup_op(nor, &op, nor->read_proto);
+
+ /* convert the dummy cycles to the number of bytes */
+ op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
+ if (spi_nor_protocol_is_dtr(nor->read_proto))
+ op.dummy.nbytes *= 2;
+
+ return op;
+}
+
/**
* spi_nor_spimem_read_data() - read data from flash's memory region via
* spi-mem
@@ -201,21 +232,14 @@ static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
size_t len, u8 *buf)
{
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_nbytes, from, 0),
- SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
- SPI_MEM_OP_DATA_IN(len, buf, 0));
+ struct spi_mem_op op = spi_nor_spimem_get_read_op(nor);
bool usebouncebuf;
ssize_t nbytes;
int error;
- spi_nor_spimem_setup_op(nor, &op, nor->read_proto);
-
- /* convert the dummy cycles to the number of bytes */
- op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
- if (spi_nor_protocol_is_dtr(nor->read_proto))
- op.dummy.nbytes *= 2;
+ op.addr.val = from;
+ op.data.nbytes = len;
+ op.data.buf.in = buf;
usebouncebuf = spi_nor_spimem_bounce(nor, &op);
@@ -3712,28 +3736,10 @@ static int spi_nor_create_read_dirmap(struct spi_nor *nor)
{
struct spi_mem_dirmap_info info = {
.op_tmpl = &info.primary_op_tmpl,
- .primary_op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
- SPI_MEM_OP_ADDR(nor->addr_nbytes, 0, 0),
- SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
- SPI_MEM_OP_DATA_IN(0, NULL, 0)),
+ .primary_op_tmpl = spi_nor_spimem_get_read_op(nor),
.offset = 0,
.length = nor->params->size,
};
- struct spi_mem_op *op = info.op_tmpl;
-
- spi_nor_spimem_setup_op(nor, op, nor->read_proto);
-
- /* convert the dummy cycles to the number of bytes */
- op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8;
- if (spi_nor_protocol_is_dtr(nor->read_proto))
- op->dummy.nbytes *= 2;
-
- /*
- * Since spi_nor_spimem_setup_op() only sets buswidth when the number
- * of data bytes is non-zero, the data buswidth won't be set here. So,
- * do it explicitly.
- */
- op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem,
&info);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v5 01/17] spi: dt-bindings: add spi-max-post-config-frequency-hz property
From: sashiko-bot @ 2026-07-17 19:00 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717185116.2065505-2-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Redefining the standard `spi-max-frequency` property to mean a conservative initial rate breaks forward compatibility and changes established DT semantics.
--
--- Patch [1]: [PATCH v5 01/17] spi: dt-bindings: add spi-max-post-config-frequency-hz property ---
commit 93cbfe3b6ec01682a84f7fad631a6f353ca37329
Author: Santhosh Kumar K <s-k6@ti.com>
spi: dt-bindings: add spi-max-post-config-frequency-hz property
This commit introduces a new generic device tree property,
spi-max-post-config-frequency-hz, for SPI peripherals. It redefines the
semantics of spi-max-frequency to represent a conservative pre-configuration
rate when both properties are present.
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 880a9f6245667..12211f8c9f7de 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -45,6 +45,11 @@ properties:
> description:
> Maximum SPI clocking speed of the device in Hz.
>
> + spi-max-post-config-frequency-hz:
> + description:
> + Maximum SPI clock frequency in Hz achievable after controller-side
> + configuration.
[Severity: High]
Could redefining the standard spi-max-frequency property to mean a
conservative initial rate break forward compatibility with older drivers or
alternative operating systems?
If older software parses a device tree using this new pattern, would it clamp
the operational speed to the conservative pre-configuration rate, resulting
in a silent performance regression?
Would it be more robust to retain spi-max-frequency as the absolute maximum
limit and instead introduce a new property (such as spi-initial-frequency-hz)
for the pre-configuration limit? This would ensure older software continues
operating at maximum capability while allowing new software to safely
initialize.
Also, if the semantics of spi-max-frequency are changed, shouldn't its YAML
description be updated to reflect this conditional behavior?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=1
^ permalink raw reply
* Re: [PATCH v4 00/16] spi: cadence-quadspi: add PHY tuning support
From: Santhosh Kumar K @ 2026-07-17 19:00 UTC (permalink / raw)
To: Michal Simek, Mahapatra, Amit Kumar, broonie@kernel.org,
robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
pratyush@kernel.org, mwalle@kernel.org,
takahiro.kuwano@infineon.com
Cc: linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
praneeth@ti.com, u-kumar1@ti.com, a-dutta@ti.com,
git (AMD-Xilinx), Amit Mohapatra, s-k6
In-Reply-To: <979c6ed6-4e75-416a-ae5f-a2a8d3ec1a4c@amd.com>
Hello Michal,
On 16/07/26 19:27, Michal Simek wrote:
>
>
> On 7/16/26 08:04, Santhosh Kumar K wrote:
>> Hello Amit,
>>
>> Apologies for the delayed response.
>>
>> On 22/06/26 10:00, Mahapatra, Amit Kumar wrote:
>>> AMD General
>>>
>>> Hello Santosh,
>>>
>>>> -----Original Message-----
>>>> From: Santhosh Kumar K <s-k6@ti.com>
>>>> Sent: Thursday, June 18, 2026 1:07 PM
>>>> To: broonie@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
>>>> conor+dt@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
>>>> vigneshr@ti.com; pratyush@kernel.org; mwalle@kernel.org;
>>>> takahiro.kuwano@infineon.com
>>>> Cc: linux-spi@vger.kernel.org; devicetree@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; linux-mtd@lists.infradead.org;
>>>> praneeth@ti.com; u-
>>>> kumar1@ti.com; a-dutta@ti.com; s-k6@ti.com
>>>> Subject: [PATCH v4 00/16] spi: cadence-quadspi: add PHY tuning support
>>>>
>>>> This series implements PHY tuning support for the Cadence QSPI
>>>> controller to
>>>> enable reliable high-speed operations. Without PHY tuning,
>>>> controllers use
>>>> conservative timing that limits performance. PHY tuning calibrates
>>>> RX/TX delay lines
>>>> to find optimal data capture timing windows, enabling operation up
>>>> to the controller's
>>>> maximum frequency.
>>>>
>>>> Background:
>>>> High-speed SPI memory controllers require precise timing calibration
>>>> for reliable
>>>> operation. At higher frequencies, board-to-board variations make
>>>> fixed timing
>>>> parameters inadequate. The Cadence QSPI controller includes a PHY
>>>> interface with
>>>> programmable delay lines (0-127 taps) for RX and TX paths, but these
>>>> require
>>>> runtime calibration to find the valid timing window.
>>>>
>>>> Approach:
>>>> Add SDR/DDR PHY tuning algorithms for the Cadence controller:
>>>>
>>>> SDR Mode Tuning (1D search):
>>>> - Searches for two consecutive valid RX delay windows
>>>> - Selects the larger window and uses its midpoint for maximum margin
>>>> - TX delay fixed at maximum (127) as it's less critical in SDR
>>>>
>>>> DDR Mode Tuning (2D search):
>>>> - Finds RX boundaries (rxlow/rxhigh) using TX window sweeps
>>>> - Finds TX boundaries (txlow/txhigh) at fixed RX positions
>>>> - Defines valid region corners and detects gaps via binary search
>>>> - Applies temperature compensation for optimal point selection
>>>> - Handles single or dual passing regions with different strategies
>>>
>>> Thank you for this series. I had a question regarding the Virtual Concat
>>> driver patch series [1]. Now that it has been merged into the kernel and
>>> enables support for multiple flash devices connected in stacked mode-
>>> where
>>> each flash device is probed and configured independently-if both flash
>>> parts are required to operate in DDR mode, each device would need to
>>> perform tuning and store its tuning data separately.
>>>
>>> Given this, should we consider this use case and adapt the tuning
>>> architecture to support it?
>>>
>>> I'd appreciate your thoughts on this.
>>
>> Good point.
>>
>> The current implementation already stores the calibration state per
>> chip select (delay line settings, tuned flags, and operation templates),
>> so the data model supports independent tuning for each device.
>>
>> However, the runtime path that enables PHY mode does not restore the
>> per-CS delay line settings when switching between chip selects. As a
>> result, consecutive PHY accesses to different devices could end up using
>> the wrong delay line settings.
>>
>> This can be addressed with a small change by reading the currently
>> programmed DLL and reprogramming the PHY configuration registers
>> whenever they differ from the calibrated values for the selected chip.
>
> Are you going to include it to your series?
Yes, I've added support for this in the v5 series, which I just
posted. [0]
[0] https://lore.kernel.org/linux-spi/20260717185116.2065505-1-s-k6@ti.com/
Thanks,
Santhosh.
>
> Thanks,
> Michal
^ permalink raw reply
* Re: [PATCH v8 3/3] iio: adc: ti-ads1100: Add ti-ads1110 support to ti-ads1100 driver
From: sashiko-bot @ 2026-07-17 19:02 UTC (permalink / raw)
To: Jakub Szczudlo; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717185033.246580-4-jakubszczudlo40@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] I2C read failures during the data-ready polling loop are mishandled, causing a 7.5-second blocking spin-loop, driver stall, and excessive log spam.
- [Medium] The polling wait interval was incorrectly increased to 5 full conversion periods, injecting severe latency on every configuration change.
--
--- Patch [3]: [PATCH v8 3/3] iio: adc: ti-ads1100: Add ti-ads1110 support to ti-ads1100 driver ---
commit 5c24a653a5a6dc7cd0b451448a77f86bfdf06445
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>
iio: adc: ti-ads1100: Add ti-ads1110 support to ti-ads1100 driver
Add ADS1110 support that have faster datarate than ADS1100, it also uses
internal voltage reference of 2.048V for measurement.
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 363f790ca5d5e..19533667e6cd8 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -139,14 +177,16 @@ static int ads1100_new_data_is_ready(struct ads1100_data *data)
> return ret;
> }
>
> - return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]) ? 0 : 1;
> + return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> }
>
> static int ads1100_poll_data_ready(struct ads1100_data *data)
> {
> - int data_rate_Hz = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> + int data_rate_index = FIELD_GET(ADS1100_DR_MASK, data->config);
> + int data_rate_Hz = data->chip_info->available_data_rate_hz[data_rate_index];
> /* To be sure we wait 5 times more than data rate */
> - unsigned long wait_time_us = DIV_ROUND_CLOSEST(USEC_PER_SEC, 2 * data_rate_Hz);
> + unsigned long period_us = DIV_ROUND_CLOSEST(USEC_PER_SEC, data_rate_Hz);
> + unsigned long wait_time_us = 5UL * period_us;
[Severity: Medium]
Does this introduce a severe delay during configuration changes?
The previous code waited for half of a conversion period. For example, at 8Hz,
that would be ~62.5ms.
By changing this to 5 full conversion periods, the initial sleep inside
readx_poll_timeout() will block for over 600ms every time the data rate or
scale is updated.
> int data_ready;
> u8 buffer[3];
> int ret;
> @@ -159,7 +199,7 @@ static int ads1100_poll_data_ready(struct ads1100_data *data)
> }
>
> ret = readx_poll_timeout(ads1100_new_data_is_ready, data,
> - data_ready, data_ready != 0,
> + data_ready, data_ready == 0,
[Severity: High]
Will this loop forever on I2C errors?
If i2c_master_recv() fails inside ads1100_new_data_is_ready(), it returns a
negative error code. Since data_ready == 0 evaluates to false for negative
values, the polling loop won't exit on failure.
This could cause the driver to hang for the entire 7.5-second timeout,
repeatedly reading and spamming the log with error messages.
Should the condition be data_ready <= 0 instead?
> wait_time_us, ADS1100_MAX_DRDY_TIMEOUT_US);
> if (data_ready < 0)
> return data_ready;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185033.246580-1-jakubszczudlo40@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v5 05/17] spi: spi-mem: add execute_tuning callback and spi_mem_execute_tuning()
From: sashiko-bot @ 2026-07-17 19:03 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260717185116.2065505-6-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `spi_mem_execute_tuning()` invokes the hardware controller's tuning callback without locking the SPI bus or acquiring runtime PM references.
- [Medium] `spi_mem_execute_tuning()` fails to check for GPIO Chip Select usage, leading to hardware tuning without an asserted chip select.
--
--- Patch [5]: [PATCH v5 05/17] spi: spi-mem: add execute_tuning callback and spi_mem_execute_tuning() ---
commit 6a445325da668ad3d841d5bfb6bc2ba5a8bc4d99
Author: Santhosh Kumar K <s-k6@ti.com>
spi: spi-mem: add execute_tuning callback and spi_mem_execute_tuning()
Add an execute_tuning callback to struct spi_controller_mem_ops. The
callback receives a mandatory read op template and an optional write op
template. On success the controller sets op->max_freq in each provided
template to the validated clock rate.
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index e20eca1b82457..ae9e2d7ac6f3c 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
[ ... ]
> @@ -660,6 +660,37 @@ u64 spi_mem_calc_op_duration(struct spi_mem *mem, struct spi_mem_op *op)
> }
> EXPORT_SYMBOL_GPL(spi_mem_calc_op_duration);
>
> +/**
> + * spi_mem_execute_tuning() - Execute controller tuning procedure
> + * @mem: the SPI memory device
> + * @read_op: read operation template (mandatory)
> + * @write_op: write operation template (optional, may be NULL)
> + *
> + * Requests the controller to perform tuning for high-speed operation
> + * using the provided op templates. On success the controller callback
> + * sets @read_op->max_freq (and @write_op->max_freq when non-NULL) to
> + * the validated clock rate.
> + *
> + * Return: 0 on success, -EINVAL if @mem or @read_op is NULL,
> + * -EOPNOTSUPP if the controller doesn't support tuning,
> + * or a negative error code on failure.
> + */
> +int spi_mem_execute_tuning(struct spi_mem *mem, struct spi_mem_op *read_op,
> + struct spi_mem_op *write_op)
> +{
> + struct spi_controller *ctlr;
> +
> + if (!mem || !read_op)
> + return -EINVAL;
> +
> + ctlr = mem->spi->controller;
> + if (!ctlr->mem_ops || !ctlr->mem_ops->execute_tuning)
> + return -EOPNOTSUPP;
[Severity: Medium]
Other spi-mem operations like spi_mem_exec_op() and spi_mem_poll_status()
verify that a GPIO chip select isn't being used by checking
!spi_get_csgpiod(mem->spi, 0) before delegating to hardware mem_ops.
Hardware sequencers typically only manage native chip selects. If a GPIO CS
is used, will this skip the check and attempt hardware tuning while the flash
device's chip select remains deasserted?
> +
> + return ctlr->mem_ops->execute_tuning(mem, read_op, write_op);
[Severity: High]
Does this execute the hardware tuning callback without locking the SPI bus or
acquiring runtime PM references?
Other wrappers that invoke hardware controller callbacks directly (such as
spi_mem_exec_op() and spi_mem_poll_status()) are wrapped in
spi_mem_access_start() and spi_mem_access_end() to power up the hardware
and lock the bus against concurrent transfers.
Should spi_mem_access_start() be called here before execute_tuning()?
> +}
> +EXPORT_SYMBOL_GPL(spi_mem_execute_tuning);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=5
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: mmc: Add post-power-off-delay-ms property
From: Judith Mendez @ 2026-07-17 19:05 UTC (permalink / raw)
To: Ulf Hansson
Cc: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-mmc, devicetree, linux-kernel
In-Reply-To: <305cf4be-2f45-49b9-b933-05ac41092685@ti.com>
Hi Ulf,
On 7/17/26 12:40 PM, Judith Mendez wrote:
> Hi Ulf,
>
> On 7/17/26 5:21 AM, Ulf Hansson wrote:
>> On Fri, Jul 17, 2026 at 1:26 AM Judith Mendez <jm@ti.com> wrote:
>>>
>>> Add post-power-off-delay-ms property to MMC controller common.
>>>
>>> This property shall be used to specify delay if needed after
>>> deasserting power during MMC power cycles.
>>
>> Please clarify with some additional information from the cover-letter,
>> like "specify custom delays after MMC power off to work around
>> hardware issues such as slow RC circuites on MMC VDD rails."
>>
>>>
>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>> ---
>>> .../devicetree/bindings/mmc/mmc-controller-common.yaml | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-
>>> common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-
>>> common.yaml
>>> index 3d7195e9461c3..3ff68d32a308f 100644
>>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> @@ -275,6 +275,14 @@ properties:
>>> not available.
>>> default: 10
>>>
>>> + post-power-off-delay-ms:
>>
>> For mmc-pwrseq-simple we already have "power-off-delay-us", perhaps
>> better to re-use that name instead.
>
> Besides having the same dt property name as mmc-pwrseq-simple what is
> the benefit of switching to power-off-delay-us?
>
> All values will have to be in us instead of ms and thus will look a bit
> less clean in code. If it is just a naming preference, I do prefer
> the current name in this version, please let me know if we can keep:
> post-power-off-delay-ms.
>
Forgot to mention, we are using mmc_delay which takes ms, if we switch
to microsecond dt property, we would have to make conversions or not use
mmc_delay.
~ Judith
^ permalink raw reply
* Re: [PATCH v8 1/3] iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode
From: sashiko-bot @ 2026-07-17 19:06 UTC (permalink / raw)
To: Jakub Szczudlo; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717185033.246580-2-jakubszczudlo40@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] The polling loop intended to wait for a new conversion to finish exits immediately without waiting because the ST/BSY bit is always 0 when the device is in continuous conversion mode.
- [Medium] The `-ETIMEDOUT` error code returned by `readx_poll_timeout()` is ignored in `ads1100_poll_data_ready()`, causing timeouts to be treated as successful executions.
Pre-existing issues:
- [High] Reading the ADC immediately after runtime resume yields stale or invalid data because it does not wait for the first continuous conversion to finish.
- [High] Stack-allocated arrays are used for I2C data transfers, violating DMA safety rules.
- [Medium] The return value of `ads1100_set_config_bits()` is completely ignored inside `ads1100_set_scale()`, masking potential I2C communication failures.
--
--- Patch [1]: [PATCH v8 1/3] iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode ---
commit d5f0f22a48498eb2a1b3f20f5e781562bc3b089e
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>
Subject: iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode
When device is suspended and it is in single mode then changing
datarate doesn't make it actually wait for new measurement, so to
be sure that read after change is correct functions that changes
datarate and gain will wait for a new data.
Fixes: 541880542f2b ("iio: adc: Add TI ADS1100 and ADS1000")
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 9fe8d54cce83c..363f790ca5d5e 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -123,10 +128,50 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
> return 0;
> }
[Severity: High]
This is a pre-existing issue, but does reading the ADC immediately after
runtime resume in ads1100_get_adc_result() yield stale or invalid data?
The resume callback re-enables the VDD regulator and switches the ADC to
continuous conversion mode. Since a conversion takes time, the driver reads
the result register via i2c_master_recv() without any delay to wait for the
first continuous conversion to finish.
Can this return whatever stale data was left in the register from before it
was suspended, or invalid power-on default data?
> +static int ads1100_new_data_is_ready(struct ads1100_data *data)
> +{
> + u8 buffer[3];
> + int ret;
> +
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
[Severity: High]
This isn't a bug introduced by this patch, but do the new functions added
here pass a stack-allocated buffer to the I2C core?
If the underlying I2C host controller driver uses DMA, mapping a stack
address can lead to memory corruption or kernel panics when VMAP_STACK and
CONFIG_DMA_API_DEBUG are enabled.
Should these buffers be dynamically allocated or embedded in a
heap-allocated device structure with proper cacheline alignment instead?
> + if (ret < 0) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return ret;
> + }
> +
> + return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]) ? 0 : 1;
[Severity: High]
Does polling the ST/BSY bit work correctly in this context?
According to the ADS1100 datasheet, the ST/BSY bit always reads as 0 in
continuous conversion mode. Because ads1100_runtime_resume() sets the ADC
to continuous conversion mode (ADS1100_CONTINUOUS), won't this check
immediately evaluate as true on its very first invocation?
This would cause the polling loop to exit instantly without actually
waiting for the conversion to complete, bypassing the intended wait period
added by this patch.
> +}
> +
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
[ ... ]
> + ret = readx_poll_timeout(ads1100_new_data_is_ready, data,
> + data_ready, data_ready != 0,
> + wait_time_us, ADS1100_MAX_DRDY_TIMEOUT_US);
> + if (data_ready < 0)
> + return data_ready;
> +
> + return 0;
[Severity: Medium]
Is it intentional that the error code returned from readx_poll_timeout() is
ignored here?
If the ADC fails to signal readiness within the timeout duration,
readx_poll_timeout() sets ret to -ETIMEDOUT and breaks the loop. Since the
timeout condition implies data_ready == 0, the subsequent check evaluating
data_ready < 0 will be false.
Does this cause the function to return 0 instead of propagating the timeout
error from ret, successfully masking the failure from the caller?
> +}
> +
> static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> {
> int microvolts;
> int gain;
> + int ret;
[ ... ]
> ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>
> - return 0;
> + return ads1100_poll_data_ready(data);
> }
[Severity: Medium]
This is a pre-existing issue, but is it safe to ignore the return value
from ads1100_set_config_bits() here?
If the underlying i2c_master_send() fails and returns a negative error code,
this function does not capture or propagate the result. It then proceeds to
poll the ADC and return the poll's result, which could lead to silent
configuration failures.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185033.246580-1-jakubszczudlo40@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v5 03/17] spi: parse spi-max-post-config-frequency-hz into post_config_max_speed_hz
From: sashiko-bot @ 2026-07-17 19:06 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717185116.2065505-4-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added `post_config_max_speed_hz` property is not copied when cloning `spi_device` structures for ancillary or muxed devices.
- [Medium] `spi->post_config_max_speed_hz` is not clamped to the SPI controller's maximum supported frequency.
--
--- Patch [3]: [PATCH v5 03/17] spi: parse spi-max-post-config-frequency-hz into post_config_max_speed_hz ---
commit 96b305610a39094bde249080c339e160beaa3e60
Author: Santhosh Kumar K <s-k6@ti.com>
spi: parse spi-max-post-config-frequency-hz into post_config_max_speed_hz
Add post_config_max_speed_hz to struct spi_device and parse it from
the spi-max-post-config-frequency-hz DT property in of_spi_parse_dt().
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index d9e6b4b87c899..7e7393d25810d 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2602,6 +2602,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
> /* Device speed */
> if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> spi->max_speed_hz = value;
> + if (!of_property_read_u32(nc, "spi-max-post-config-frequency-hz", &value))
> + spi->post_config_max_speed_hz = value;
[Severity: Medium]
Does this code need to clamp post_config_max_speed_hz to the controller's
maximum supported frequency?
During device setup in __spi_setup(), max_speed_hz is safely clamped:
drivers/spi/spi.c:__spi_setup() {
...
if (spi->controller->max_speed_hz &&
(!spi->max_speed_hz ||
spi->max_speed_hz > spi->controller->max_speed_hz))
spi->max_speed_hz = spi->controller->max_speed_hz;
...
}
Will controller drivers directly reading spi->post_config_max_speed_hz see an
unvalidated value if the DT property exceeds the controller's max speed?
[ ... ]
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4c285d3ede1d8..aa7b1b34a594f 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
[ ... ]
> @@ -193,6 +195,7 @@ struct spi_device {
> struct device dev;
> struct spi_controller *controller;
> u32 max_speed_hz;
> + u32 post_config_max_speed_hz;
[Severity: Medium]
Should this new property be copied when cloning spi_device structures for
ancillary or muxed devices?
In spi_new_ancillary_device(), the parent device's speed and mode constraints
are copied to the ancillary device:
drivers/spi/spi.c:spi_new_ancillary_device() {
...
ancillary->max_speed_hz = spi->max_speed_hz;
ancillary->mode = spi->mode;
...
}
A similar copy happens in spi_mux_select():
drivers/spi/spi-mux.c:spi_mux_select() {
...
priv->spi->max_speed_hz = spi->max_speed_hz;
...
}
Will ancillary or multiplexed SPI devices fail to inherit the
post_config_max_speed_hz property, resulting in a value of 0?
> u8 bits_per_word;
> bool rt;
> #define SPI_NO_TX BIT(31) /* No transmit wire */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=3
^ permalink raw reply
* Re: [PATCH v5 10/17] spi: cadence-quadspi: refactor direct read path for PHY support
From: sashiko-bot @ 2026-07-17 19:07 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717185116.2065505-11-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] In 8D-8D-8D mode, `cqspi_memcpy_fromio()` performs an unaligned 16-bit I/O read (`__raw_readw()`) if the requested SPI read offset (`op->addr.val`) is not 2-byte aligned. This causes a synchronous Alignment Fault (kernel panic) on architectures with strict I/O alignment rules.
- [High] Directly casting the generic destination buffer `to` to a `u16 *` and dereferencing it causes an unaligned memory store if the buffer is not 2-byte aligned.
- [High] Relying on `memcpy_fromio()` to perform 4-byte hardware reads in 8D-8D-8D mode violates hardware access width requirements, as `memcpy_fromio()` does not guarantee 32-bit I/O transactions.
Pre-existing issues:
- [High] When the `CQSPI_NO_INDIRECT_MODE` quirk is set, `cqspi_read()` and `cqspi_write()` bypass the `ahb_size` boundary check, allowing out-of-bounds MMIO accesses.
--
--- Patch [10]: [PATCH v5 10/17] spi: cadence-quadspi: refactor direct read path for PHY support ---
commit 2b5af9ab10a299ef73d9e0a5cb2201de2b8ee054
Author: Santhosh Kumar K <s-k6@ti.com>
Subject: spi: cadence-quadspi: refactor direct read path for PHY support
This patch extracts the DMA transfer code from cqspi_direct_read_execute()
into a new cqspi_direct_read_dma() helper. It adds cqspi_memcpy_fromio() to
handle non-DMA transfers, with 2-byte-aligned I/O accesses for 8D-8D-8D
mode.
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 5070ffc53d5e8..322e95b3bc17d 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1580,8 +1577,61 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
> +static void cqspi_memcpy_fromio(const struct spi_mem_op *op, void *to,
> + const void __iomem *from, size_t count)
> +{
> + if (op->data.buswidth == 8 && op->data.dtr) {
> + unsigned long from_addr = (unsigned long)from;
> +
> + /* Handle unaligned start with 2-byte read */
> + if (count && !IS_ALIGNED(from_addr, 4)) {
> + *(u16 *)to = __raw_readw(from);
[Severity: High]
Could this cause a synchronous alignment fault on architectures with strict
I/O alignment rules? If the requested SPI read offset (from_addr) is an odd
address, !IS_ALIGNED(from_addr, 4) evaluates to true, which results in
__raw_readw() executing on an unaligned I/O address.
Additionally, does casting the generic destination buffer 'to' to a u16
pointer risk unaligned memory stores? If the buffer provided by userspace or
another subsystem is not 2-byte aligned, this might cause undefined behavior.
Would it be safer to use put_unaligned() here?
> + from += 2;
> + to += 2;
> + count -= 2;
> + }
> +
> + /* Use 4-byte reads for aligned bulk (no readq for 32-bit) */
> + if (count >= 4) {
> + size_t len = round_down(count, 4);
> +
> + memcpy_fromio(to, from, len);
[Severity: High]
Can we rely on memcpy_fromio() to guarantee 32-bit hardware transactions on
all architectures? The comment mentions using 4-byte reads for aligned bulk,
but on some platforms memcpy_fromio() falls back to byte-by-byte reads. Should
this use ioread32_rep() or a manual readl() loop to enforce the hardware
access width requirements for 8D-8D-8D mode?
> + from += len;
> + to += len;
> + count -= len;
> + }
> +
> + /* Handle remaining 2 bytes */
> + if (count)
> + *(u16 *)to = __raw_readw(from);
[Severity: High]
Similarly to the unaligned start logic, does this risk unaligned I/O reads or
unaligned memory stores if the trailing bytes involve an odd start address or
an unaligned destination buffer?
> +
> + return;
> + }
> +
> + memcpy_fromio(to, from, count);
> +}
[ ... ]
> @@ -1597,7 +1647,8 @@ static ssize_t cqspi_read(struct cqspi_flash_pdata *f_pdata,
>
> if ((cqspi->use_direct_mode && ((from + len) <= cqspi->ahb_size)) ||
> (cqspi->ddata && cqspi->ddata->quirks & CQSPI_NO_INDIRECT_MODE))
> - return cqspi_direct_read_execute(f_pdata, buf, from, len);
> + return cqspi_direct_read_execute(f_pdata, op,
> + post_config_max_speed_hz);
[Severity: High]
This is a pre-existing issue, but does the CQSPI_NO_INDIRECT_MODE quirk
bypass the ahb_size boundary check here? Because of the logical OR condition,
an SPI read with an offset and length exceeding the controller's memory-mapped
AHB window might proceed and access out-of-bounds MMIO.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=10
^ permalink raw reply
* Re: [PATCH v5 08/17] spi: cadence-quadspi: add PHY tuning support
From: sashiko-bot @ 2026-07-17 19:07 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717185116.2065505-9-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware `read_delay` is neither applied during PHY tuning sweeps nor preserved after tuning succeeds.
- [High] The variable `ret` is returned uninitialized if tuning is skipped due to a missing pattern partition offset.
- [High] The `execute_tuning` callback modifies shared controller hardware registers without holding the SPI bus lock.
- [Medium] Missing clamping in dual-region PHY tuning calculation causes integer wrap-around.
- [Medium] Unconditional loop increment corrupts the successful TX delay value during RX low boundary search.
--
--- Patch [8]: [PATCH v5 08/17] spi: cadence-quadspi: add PHY tuning support ---
commit 60d05f5aa8117400f4e61a7bebf88a390815be00
Author: Santhosh Kumar K <s-k6@ti.com>
spi: cadence-quadspi: add PHY tuning support
This commit introduces delay-line PHY calibration for Cadence QSPI controllers
to enable high-speed DDR/SDR operation. It adds an execute_tuning callback
that sweeps through RX and TX delay parameters and tests them using a known
data pattern.
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 046fa5d69b0f7..22a5c88bbd8dc 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -2048,15 +2048,22 @@ static int cqspi_phy_apply_setting(struct cqspi_flash_pdata *f_pdata,
> struct phy_setting *phy)
> {
> struct cqspi_st *cqspi = f_pdata->cqspi;
> unsigned int reg;
> int ret;
>
> reg = readl(cqspi->iobase + CQSPI_REG_READCAPTURE);
> reg |= BIT(CQSPI_REG_READCAPTURE_EDGE_LSB);
> writel(reg, cqspi->iobase + CQSPI_REG_READCAPTURE);
>
> cqspi_set_dll(cqspi->iobase, phy->rx, phy->tx);
[Severity: High]
Does this execute_tuning process run without the SPI bus lock?
Since cqspi_phy_apply_setting() modifies global hardware registers directly,
and the caller subsequently invokes spi_mem_exec_op() to verify the pattern,
could a concurrent transaction on another chip select interfere and cause
register data races?
>
> ret = cqspi_resync_dll(cqspi);
> if (ret)
> return ret;
>
> f_pdata->phy_setting.rx = phy->rx;
> f_pdata->phy_setting.tx = phy->tx;
> f_pdata->phy_setting.read_delay = phy->read_delay;
> return 0;
> }
[Severity: High]
Is the new read_delay value actually programmed into the hardware during
the tuning sweeps?
While cqspi_phy_apply_setting() updates f_pdata->phy_setting.read_delay,
it doesn't appear to write this value to the CQSPI_REG_READCAPTURE
register, meaning all reads during the sweep might still be using the
static initial read delay.
[ ... ]
> @@ -2314,6 +2314,12 @@ static int cqspi_phy_tuning_ddr(struct cqspi_flash_pdata *f_pdata,
> rxlow.tx = CQSPI_PHY_TX_LOOKUP_LOW_START;
> do {
> dev_dbg(dev, "Searching for Golden Primary rxlow on TX = %d\n",
> rxlow.tx);
> rxlow.read_delay = CQSPI_PHY_INIT_RD;
> ret = cqspi_find_rx_low_ddr(f_pdata, mem, &rxlow);
> rxlow.tx += CQSPI_PHY_DDR_SEARCH_STEP;
> } while (ret && rxlow.tx <= CQSPI_PHY_TX_LOOKUP_LOW_END);
[Severity: Medium]
Will this unconditionally increment rxlow.tx even when the pattern search
succeeds?
If ret == 0, the loop breaks, but rxlow.tx has already been incremented by
CQSPI_PHY_DDR_SEARCH_STEP. This seems to skew the successful TX boundary
by 4 taps and could corrupt the later midpoint calculation.
[ ... ]
> @@ -3037,6 +3037,10 @@ static int cqspi_phy_tuning_ddr(struct cqspi_flash_pdata *f_pdata,
> if ((abs(gaplow.tx - bottomleft.tx) +
> abs(gaplow.rx - bottomleft.rx)) <
> (abs(gaphigh.tx - topright.tx) +
> abs(gaphigh.rx - topright.rx))) {
> /* Topright further: Use Region 2, 16 taps inward */
> searchpoint = topright;
> searchpoint.tx -= 16;
> searchpoint.rx -= (16 * (topright.rx - bottomleft.rx)) /
> (topright.tx - bottomleft.tx);
[Severity: Medium]
Could this calculation cause an integer underflow on the 8-bit unsigned
rx and tx values?
If the resulting coordinate drops below zero, it looks like it would
wrap around and apply a very large, incorrect delay to the PHY. Should
this use clamping similar to the single-region temperature compensation
path above?
[ ... ]
> @@ -3237,6 +3237,10 @@ static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
> dev_warn(dev,
> "failed to write pattern to cache: %d, skipping tuning\n",
> ret);
> goto out;
> }
>
> f_pdata->phy_write_op = *write_op;
> } else {
> if (!f_pdata->phy_pattern_partition_offset) {
> dev_warn(dev, "pattern partition not found, skipping tuning\n");
> goto out;
> }
[Severity: High]
Does this path return an uninitialized stack variable?
The local variable ret is declared but not initialized. If write_op
is NULL and the spi-phy-pattern-partition property is missing, taking
this goto will return whatever garbage is on the stack.
[ ... ]
> @@ -3274,6 +3274,8 @@ static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
> ret = cqspi_phy_tuning_sdr(f_pdata, mem);
> }
>
> if (ret)
> dev_warn(dev, "tuning failed: %d\n", ret);
>
> cqspi_phy_post_config(cqspi, f_pdata->read_delay);
[Severity: High]
Does this discard the tuned read delay?
cqspi_phy_post_config() is called with f_pdata->read_delay (the static
DT value) instead of f_pdata->phy_setting.read_delay. This seems to
overwrite the hardware with the untuned delay, dropping the results of
the calibration.
Also, should cqspi_configure() be updated to check use_tuned_phy and
apply f_pdata->phy_setting instead of unconditionally using
f_pdata->read_delay during context switches? Without this, the DLL
parameters might be lost on a CS switch.
> +
> +out:
> + /*
> + * On success, write back the validated maximum speed into the caller's
> + * op templates so that those specific ops bypass the cap in subsequent
> + * exec_op calls.
> + */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=8
^ permalink raw reply
* Re: [PATCH v5 09/17] spi: cadence-quadspi: skip DDR PHY tuning for 2-byte-address ops (i2383)
From: sashiko-bot @ 2026-07-17 19:09 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717185116.2065505-10-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Potential NULL pointer dereference in `cqspi_mem_op_execute_tuning()` if the controller lacks platform match data.
--
--- Patch [9]: [PATCH v5 09/17] spi: cadence-quadspi: skip DDR PHY tuning for 2-byte-address ops (i2383) ---
commit 7e0e0805103ff0525c32a0dd04ddbcea9570af64
Author: Santhosh Kumar K <s-k6@ti.com>
spi: cadence-quadspi: skip DDR PHY tuning for 2-byte-address ops (i2383)
Erratum i2383 on AM654 locks the address phase in PHY DDR mode when a
2-byte column address is used. DDR PHY tuning must not be attempted for
such operations; non-PHY DDR usage is unaffected. [0]
Add CQSPI_NO_PHY_TUNING_WITH_ODDR_2B_ADDR quirk and check it in
cqspi_am654_ospi_execute_tuning(). When the erratum applies, return 0
with read_op->max_freq cleared — the op-skip signal that tells the
caller to try another op variant.
Link: https://www.ti.com/lit/er/sprz544c/sprz544c.pdf
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 22a5c88bbd8dc..5070ffc53d5e8 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -3183,6 +3184,20 @@ static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
> return -EOPNOTSUPP;
> }
>
> + /*
> + * Erratum i2383: in PHY DDR mode, a 2-byte column address locks up
> + * the address phase. Skip DDR PHY tuning for such operations.
> + */
> + if ((cqspi->ddata->quirks & CQSPI_NO_PHY_TUNING_WITH_ODDR_2B_ADDR) &&
> + read_op->addr.nbytes == 2 &&
> + (read_op->cmd.dtr || read_op->addr.dtr || read_op->dummy.dtr ||
> + read_op->data.dtr)) {
> + dev_dbg(dev,
> + "i2383: skipping DDR PHY tuning (2-byte address)\n");
> + read_op->max_freq = 0;
> + return 0;
> + }
> +
> if (write_op) {
> /*
> * For NAND: write the calibration pattern to the page cache.
[Severity: High]
Can cqspi_mem_op_execute_tuning() experience a NULL pointer dereference if
the controller lacks platform match data?
If the driver is bound to a device (e.g., via sysfs driver_override) whose
of_node provides the necessary flash child nodes to successfully pass probe
but possesses an unmatched compatible string, of_device_get_match_data()
will return NULL, leaving cqspi->ddata as NULL.
While cqspi->ddata is defensively guarded in other execution paths, it
appears to be unconditionally dereferenced here:
drivers/spi/spi-cadence-quadspi.c:cqspi_mem_op_execute_tuning() {
...
if (!cqspi->ddata->execute_tuning)
return -EOPNOTSUPP;
return cqspi->ddata->execute_tuning(mem, read_op, write_op);
}
Could any kernel module calling the newly exported spi_mem_execute_tuning()
API on this controller trigger a crash?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=9
^ permalink raw reply
* Re: [PATCH v5 11/17] spi: cadence-quadspi: enable PHY for direct reads
From: sashiko-bot @ 2026-07-17 19:10 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717185116.2065505-12-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues:
- [Critical] Splitting the direct read into adjacent unaligned DMA mappings violates the DMA API cacheline alignment rules, causing memory corruption.
- [Critical] Direct reads for unaligned buffers, small lengths, or vmalloc addresses bypass PHY tuning at high speeds, causing silent data corruption.
- [High] Integer underflow when decrementing the dummy cycle count for 0-dummy cycle commands, resulting in 31 dummy cycles being programmed.
- [High] The return value of `cqspi_tune_phy(..., false)` is completely ignored, masking hardware timeouts.
- [High] Hardware state modifications are not reverted if `cqspi_wait_idle()` fails in `cqspi_tune_phy(..., true)`, leaving the PHY permanently enabled.
- [High] If `cqspi_resync_dll()` fails, the controller retains the target device's settings in `CQSPI_REG_PHY_CONFIG` but the DLL remains unlocked, causing silent data corruption on subsequent reads.
- [High] The `rx_dma_complete` completion is reinitialized after the DMA transaction is submitted, creating a TOCTOU race condition.
- [High] Modifying PHY configuration and dummy cycle registers while the controller may not be fully idle corrupts the hardware state machine.
Pre-existing issues:
- [High] Inconsistent MMIO accessor semantics between bulk transfers and partial remainder transfers, leading to data corruption on big-endian architectures.
--
--- Patch [11]: [PATCH v5 11/17] spi: cadence-quadspi: enable PHY for direct reads ---
commit c88f8eecc883007f79c6f7679e88e02531e508ae
Author: Santhosh Kumar K <s-k6@ti.com>
spi: cadence-quadspi: enable PHY for direct reads
Add cqspi_tune_phy() to toggle PHY mode. Enabling sets the calibrated
read-capture delay, asserts PHY_EN and PHY_PIPELINE, and decrements the
dummy cycle count by one since the PHY pipeline absorbs that latency.
When multiple chip selects have different calibration results, the DLL
is reprogrammed on each enable to match the active device's settings.
Disabling reverses all three.
For direct reads, split the transfer into an unaligned head, a
16-byte-aligned middle section with PHY active, and an unaligned tail.
PHY is used when tuning completed successfully and the transfer is at
the calibrated frequency.
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 322e95b3bc17d..35a4000739449 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1067,6 +1067,82 @@ static int cqspi_resync_dll(struct cqspi_st *cqspi)
> return ret;
> }
>
> +static int cqspi_tune_phy(struct cqspi_flash_pdata *f_pdata, bool enable)
> +{
> + struct cqspi_st *cqspi = f_pdata->cqspi;
> + void __iomem *reg_base = cqspi->iobase;
> + u32 reg;
> + u8 dummy;
> + int ret;
> +
> + if (enable) {
[ ... ]
> + reg = readl(reg_base + CQSPI_REG_PHY_CONFIG);
> + if (((reg >> CQSPI_REG_PHY_CONFIG_RX_DEL_LSB) &
> + CQSPI_REG_PHY_CONFIG_RX_DEL_MASK) != f_pdata->phy_setting.rx ||
> + ((reg >> CQSPI_REG_PHY_CONFIG_TX_DEL_LSB) &
> + CQSPI_REG_PHY_CONFIG_TX_DEL_MASK) != f_pdata->phy_setting.tx) {
> + cqspi_set_dll(reg_base, f_pdata->phy_setting.rx,
> + f_pdata->phy_setting.tx);
> + ret = cqspi_resync_dll(cqspi);
> + if (ret)
> + return ret;
[Severity: High]
Does returning early here if cqspi_resync_dll() fails leave the controller in
an inconsistent state? cqspi_set_dll() has already updated CQSPI_REG_PHY_CONFIG
with the new settings. If we return here, on the next read, the driver might
skip cqspi_resync_dll() entirely because the settings appear to match, and
proceed to read with the PHY enabled but the DLL unlocked or out-of-sync.
Could this cause silent data corruption?
> + }
> +
> + cqspi_readdata_capture(cqspi, true, f_pdata->use_dqs,
> + f_pdata->phy_setting.read_delay);
> +
> + reg = readl(reg_base + CQSPI_REG_CONFIG);
> + reg |= CQSPI_REG_CONFIG_PHY_EN | CQSPI_REG_CONFIG_PHY_PIPELINE;
> + writel(reg, reg_base + CQSPI_REG_CONFIG);
[Severity: High]
Is it safe to modify CQSPI_REG_CONFIG and CQSPI_REG_RD_INSTR here without
first ensuring the SPI controller is fully idle?
In cqspi_direct_read_execute(), cqspi_tune_phy() is called immediately after a
DMA segment completes. The DMA completion indicates that the AHB memory writes
finished, but not necessarily that the QSPI controller has fully de-asserted CS
and returned to idle. Could modifying these registers without a prior
cqspi_wait_idle() check race with the controller's shutdown sequence and
corrupt the hardware state machine?
> +
> + /*
> + * The PHY data-capture pipeline absorbs one dummy cycle's
> + * worth of latency; reduce the count to avoid over-compensation.
> + */
> + reg = readl(reg_base + CQSPI_REG_RD_INSTR);
> + dummy = FIELD_GET(CQSPI_REG_RD_INSTR_DUMMY_MASK
> + << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> + reg);
> + dummy--;
> + reg &= ~(CQSPI_REG_RD_INSTR_DUMMY_MASK
> + << CQSPI_REG_RD_INSTR_DUMMY_LSB);
> + reg |= FIELD_PREP(CQSPI_REG_RD_INSTR_DUMMY_MASK
> + << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> + dummy);
[Severity: High]
Can this decrement cause an integer underflow if the initial dummy count is 0?
If a read command requires 0 dummy cycles, dummy-- would underflow to 255.
When masked to 5 bits via FIELD_PREP, it would write 31 to the hardware
register instead of 0, which might misalign sampling and corrupt data.
> + writel(reg, reg_base + CQSPI_REG_RD_INSTR);
> + } else {
[ ... ]
> + writel(reg, reg_base + CQSPI_REG_RD_INSTR);
> + }
> +
> + return cqspi_wait_idle(cqspi);
[Severity: High]
If cqspi_wait_idle() times out during the enable path, the function returns an
error, but it does not seem to revert the hardware state modifications
(like PHY_EN and the dummy cycle decrement). Since the caller will abort
without calling cqspi_tune_phy(..., false), does this leave the PHY permanently
enabled and break subsequent operations?
> +}
> +
[ ... ]
> @@ -1617,8 +1701,11 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
> {
> struct cqspi_st *cqspi = f_pdata->cqspi;
> loff_t from = op->addr.val;
> + loff_t from_aligned, to_aligned;
> size_t len = op->data.nbytes;
> + size_t len_aligned;
> u_char *buf = op->data.buf.in;
> + int ret;
>
> if (!cqspi->rx_chan || !virt_addr_valid(buf) ||
> len < CQSPI_PHY_MIN_DIRECT_READ_LEN) {
> @@ -1626,7 +1713,42 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
> return 0;
> }
>
> - return cqspi_direct_read_dma(f_pdata, buf, from, len);
> + if (!cqspi_use_tuned_phy(f_pdata, op, post_config_max_speed_hz))
> + return cqspi_direct_read_dma(f_pdata, buf, from, len);
> +
> + /* Split into unaligned head, aligned middle, unaligned tail */
> + from_aligned = ALIGN(from, 16);
> + to_aligned = ALIGN_DOWN(from + len, 16);
> + len_aligned = to_aligned - from_aligned;
> +
> + if (from != from_aligned) {
> + ret = cqspi_direct_read_dma(f_pdata, buf, from,
> + from_aligned - from);
[Severity: Critical]
By splitting the DMA mappings at 16-byte boundaries instead of the CPU's
cacheline size (typically 64 bytes), do these adjacent sub-buffers end up
sharing cachelines?
When the middle section is mapped for DMA, could the cache maintenance
operations destroy the data just written to memory by the head section's
DMA, or write back stale CPU cache data over it, causing memory corruption
on architectures with non-coherent DMA?
[Severity: High]
Additionally, I noticed a race condition inside cqspi_direct_read_dma() when
submitting the transaction:
cookie = tx->tx_submit(tx);
reinit_completion(&cqspi->rx_dma_complete);
If the DMA engine processes the descriptor immediately after tx_submit, the
callback might execute and call complete() before reinit_completion() clears it,
causing wait_for_completion_timeout() to incorrectly hang.
[Severity: Critical]
For unaligned buffers, short lengths, or vmalloc addresses, it appears we bypass
PHY tuning and read directly via AHB. However, the SPI clock frequency is still
running at post_config_max_speed_hz.
If this high frequency requires PHY tuning for proper data capture, will reading
without the PHY enabled violate setup/hold times and sample garbage data?
> + if (ret)
> + return ret;
> + buf += from_aligned - from;
> + }
> +
> + if (len_aligned) {
> + ret = cqspi_tune_phy(f_pdata, true);
> + if (ret)
> + return ret;
> + ret = cqspi_direct_read_dma(f_pdata, buf, from_aligned,
> + len_aligned);
> + cqspi_tune_phy(f_pdata, false);
[Severity: High]
Does this code ignore the return value of cqspi_tune_phy(..., false)?
If the preceding DMA operation succeeded but disabling the PHY fails
due to a hardware timeout in cqspi_wait_idle(), the return value is lost.
Could this mask hardware timeouts and leave the controller broken while the SPI
subsystem assumes success?
> + if (ret)
> + return ret;
> + buf += len_aligned;
> + }
> +
> + if (to_aligned != (from + len)) {
> + ret = cqspi_direct_read_dma(f_pdata, buf, to_aligned,
> + (from + len) - to_aligned);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> }
I also noticed an issue in the indirect transfer functions that is not
introduced by this patch, but might be worth addressing.
[Severity: High]
This is a pre-existing issue, but does mixing ioread32_rep() with ioread32()
cause data corruption on big-endian architectures?
In cqspi_indirect_read_execute():
ioread32_rep(ahb_base, rxbuf, (bytes_to_read / 4));
} else if (!word_remain && mod_bytes) {
unsigned int temp = ioread32(ahb_base);
The bulk data is read using ioread32_rep() (which preserves FIFO byte order
without byteswapping), while the remainder bytes use ioread32() (which performs
CPU-to-device byteswapping on big-endian systems). The trailing bytes of the
payload will be byte-swapped relative to the bulk data. A similar pattern
exists in cqspi_indirect_write_execute() with iowrite32_rep() and iowrite32().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=11
^ permalink raw reply
* Re: [PATCH v5 04/17] spi: spi-mem: teach spi_mem_adjust_op_freq() about post-config ops
From: sashiko-bot @ 2026-07-17 19:11 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717185116.2065505-5-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing hardware limit clamping for post_config_max_speed_hz
- [High] Exact frequency equality check overloads capability field as a state flag, causing tuning failure and regressions
--
--- Patch [4]: [PATCH v5 04/17] spi: spi-mem: teach spi_mem_adjust_op_freq() about post-config ops ---
commit d5308ce32807c28e1b6f48e6c7412cc09f81f812
Author: Santhosh Kumar K <s-k6@ti.com>
spi: spi-mem: teach spi_mem_adjust_op_freq() about post-config ops
This commit extends spi_mem_adjust_op_freq() with a bypass for ops where
op->max_freq equals post_config_max_speed_hz. All other ops are capped to
max_speed_hz, integrating the policy into the single frequency-adjustment
point.
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index a88b9f0383564..e20eca1b82457 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -591,9 +591,18 @@ EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size);
> * Some chips have per-op frequency limitations and must adapt the maximum
> * speed. This function allows SPI mem drivers to set @op->max_freq to the
> * maximum supported value.
> + *
> + * When @mem->spi->post_config_max_speed_hz is set, ops with @op->max_freq
> + * equal to that value are treated as post-configuration ops (e.g. PHY-tuned)
> + * and are allowed to run at the full post-config rate. All other ops are
> + * capped to @mem->spi->max_speed_hz, the always-reachable base rate.
> */
> void spi_mem_adjust_op_freq(struct spi_mem *mem, struct spi_mem_op *op)
> {
> + if (mem->spi->post_config_max_speed_hz &&
> + op->max_freq == mem->spi->post_config_max_speed_hz)
> + return;
[Severity: High]
Does this early return bypass the hardware capability limit clamping for
post-config operations?
If a device tree provides a post-config frequency that exceeds the
controller's absolute hardware capabilities (spi->controller->max_speed_hz),
could returning early here force the SPI controller to configure unbounded
frequencies?
[Severity: High]
Since op is often an ephemeral local variable and op->max_freq is a static
capability bound initialized from flash driver templates (such as
SPINAND_OP_VARIANTS), will this exact equality check fail to match during
actual operation?
For example, if a flash command supports up to 200MHz, but the board is tuned
to 133MHz, the equality check would fail. In this scenario, does the tuned
operation incorrectly fall through and get clamped to the slower untuned
max_speed_hz?
Could this also cause pre-tuning operations to accidentally bypass the clamp
if their natural maximum frequency happens to match the board's post-config
target frequency?
> +
> if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
> op->max_freq = mem->spi->max_speed_hz;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=4
^ permalink raw reply
* Re: [PATCH v5 07/17] spi: cadence-quadspi: add DQS support to read data capture
From: sashiko-bot @ 2026-07-17 19:13 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260717185116.2065505-8-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch adds a `dqs` parameter to `cqspi_readdata_capture()` but hardcodes it to `false` in its only caller, which unconditionally clears the DQS hardware bit and destroys any preexisting bootloader configuration.
--
--- Patch [7]: [PATCH v5 07/17] spi: cadence-quadspi: add DQS support to read data capture ---
commit 9c04e0d8a632e24ea7f330776f17875445decaac
Author: Santhosh Kumar K <s-k6@ti.com>
spi: cadence-quadspi: add DQS support to read data capture
This commit adds a DQS (Data Strobe) parameter to cqspi_readdata_capture()
to control data capture timing. It updates the register read-modify-write
sequence to configure the DQS bit and modifies the caller to provide this
new parameter.
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index f8e2663723677..046fa5d69b0f7 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -476,6 +477,11 @@ static void cqspi_readdata_capture(struct cqspi_st *cqspi, const bool bypass,
> reg |= (delay & CQSPI_REG_READCAPTURE_DELAY_MASK)
> << CQSPI_REG_READCAPTURE_DELAY_LSB;
>
> + if (dqs)
> + reg |= BIT(CQSPI_REG_READCAPTURE_DQS_LSB);
> + else
> + reg &= ~BIT(CQSPI_REG_READCAPTURE_DQS_LSB);
> +
> writel(reg, reg_base + CQSPI_REG_READCAPTURE);
> }
>
> @@ -1317,7 +1323,7 @@ static void cqspi_configure(struct cqspi_flash_pdata *f_pdata,
> cqspi->sclk = sclk;
> cqspi_config_baudrate_div(cqspi);
> cqspi_delay(f_pdata);
> - cqspi_readdata_capture(cqspi, !cqspi->rclk_en,
> + cqspi_readdata_capture(cqspi, !cqspi->rclk_en, false,
[Severity: Medium]
Does passing false here unconditionally clear the DQS hardware bit that was
previously preserved from the bootloader?
Prior to this change, the read-modify-write sequence in
cqspi_readdata_capture() did not touch bit 8 (CQSPI_REG_READCAPTURE_DQS_LSB).
Now, by hardcoding dqs to false, cqspi_readdata_capture() will actively clear
the DQS bit.
Could this degrade timing margins and cause read data corruption on platforms
that currently rely on the bootloader's DQS configuration?
> f_pdata->read_delay);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=7
^ permalink raw reply
* Re: [PATCH v5 12/17] spi: cadence-quadspi: enable PHY for indirect writes
From: sashiko-bot @ 2026-07-17 19:14 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260717185116.2065505-13-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PHY tuning is incorrectly executed after the indirect write operation has been started, causing hardware hangs and a guaranteed timeout.
- [High] A failure during PHY enablement permanently corrupts the controller's dummy cycle configuration due to asymmetric error handling.
- [High] Restoring PHY state before canceling a pending indirect write causes secondary timeouts in the error handler.
--
--- Patch [12]: [PATCH v5 12/17] spi: cadence-quadspi: enable PHY for indirect writes ---
commit ee250e92bd8277b7a8635c4c209796570036798e
Author: Santhosh Kumar K <s-k6@ti.com>
spi: cadence-quadspi: enable PHY for indirect writes
Enable PHY for indirect writes of at least CQSPI_PHY_MIN_INDIRECT_WRITE_LEN
bytes. PHY is activated only when tuning completed successfully and the
write op runs at the calibrated post-config frequency.
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 35a4000739449..542ad331ff167 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1377,6 +1381,18 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
> if (cqspi->apb_ahb_hazard)
> readl(reg_base + CQSPI_REG_INDIRECTWR);
>
> + /* Use PHY only for large writes at the calibrated rate */
> + use_tuned_phy_write = n_tx >= CQSPI_PHY_MIN_INDIRECT_WRITE_LEN &&
> + f_pdata->use_tuned_phy &&
> + f_pdata->phy_write_op.max_freq ==
> + post_config_max_speed_hz;
> +
> + if (use_tuned_phy_write) {
> + ret = cqspi_tune_phy(f_pdata, true);
> + if (ret)
> + goto failwr;
> + }
> +
[Severity: High]
Does this execute PHY tuning after the indirect write operation has already
started? Modifying global controller configuration while an indirect
operation is active could violate hardware sequencing rules.
Since the controller is actively waiting for FIFO data, would the call chain
cqspi_tune_phy()->cqspi_wait_idle() time out and fail the write?
> while (remaining > 0) {
> size_t write_words, mod_bytes;
[ ... ]
> @@ -1425,9 +1441,15 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
>
> cqspi_wait_idle(cqspi);
>
> + if (use_tuned_phy_write)
> + cqspi_tune_phy(f_pdata, false);
> +
> return 0;
>
> failwr:
> + if (use_tuned_phy_write)
> + cqspi_tune_phy(f_pdata, false);
> +
[Severity: High]
If cqspi_tune_phy(f_pdata, true) fails to enable the PHY and returns an
error, it returns early and avoids decrementing the dummy cycle count.
Would jumping to failwr here and unconditionally calling
cqspi_tune_phy(f_pdata, false) end up incrementing the dummy cycle count
without a prior decrement? This could permanently corrupt the controller's
dummy cycle configuration for subsequent read operations.
[Severity: High]
Is cqspi_tune_phy() called here before the pending indirect write is canceled
in the error handler?
If cqspi_tune_phy() calls cqspi_wait_idle(), could this cause a secondary
timeout in the error handler since the indirect write hasn't been canceled
yet?
> /* Disable interrupt. */
> writel(0, reg_base + CQSPI_REG_IRQMASK);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=12
^ permalink raw reply
* Re: [PATCH v5 15/17] mtd: spinand: negotiate optimal controller operating point before dirmap creation
From: sashiko-bot @ 2026-07-17 19:15 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260717185116.2065505-16-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Ranked fallback optimization fails to downgrade continuous read cache variants, leading to execution of untuned operations at tuned high frequencies.
--
--- Patch [15]: [PATCH v5 15/17] mtd: spinand: negotiate optimal controller operating point before dirmap creation ---
commit 7c43cf072fbe0a771ec2adb617714b7d258f3161
Author: Santhosh Kumar K <s-k6@ti.com>
mtd: spinand: negotiate optimal controller operating point before dirmap creation
This commit moves dirmap creation to the probe function, after controller
optimization is completed. It introduces a ranked fallback mechanism that
iterates through read variants if the pre-selected variant fails controller
tuning, applying the validated PHY frequency to the dirmap descriptors.
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index b6896760b0b87..58bf7f26b49ba 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
[ ... ]
> @@ -2050,6 +2055,175 @@ static void spinand_cleanup(struct spinand_device *spinand)
> kfree(spinand->scratchbuf);
> }
>
> +/*
> + * spinand_try_ranked_variant() - Try controller optimization on variants in
[ ... ]
> +static bool spinand_try_ranked_variant(struct spinand_device *spinand,
> + struct spi_mem *mem,
> + enum spinand_bus_interface iface,
> + u32 *tried_mask)
> +{
> + const struct spinand_op_variants *variants = spinand->all_read_variants;
> + const struct spi_mem_op *best;
> + int ret;
> +
> + if (!variants)
> + return false;
> +
> + while ((best = spinand_op_find_best_variant(spinand, variants, iface,
> + *tried_mask))) {
> + *tried_mask |= BIT(best - variants->ops);
> + spinand->max_read_op = *best;
> + spinand->max_read_op.max_freq = 0;
> + ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> + &spinand->max_write_op);
> + if (ret && ret != -EOPNOTSUPP)
> + dev_dbg(&mem->spi->dev, "%s optimization failed: %d\n",
> + iface == ODTR ? "ODTR" : "SSDR", ret);
> + if (!ret && spinand->max_read_op.max_freq) {
> + if (iface == ODTR)
> + spinand->odtr_op_templates.read_cache = best;
> + else
> + spinand->ssdr_op_templates.read_cache = best;
[Severity: High]
If the controller tuning fails for the best variant but succeeds for a slower
fallback variant, this code updates the read_cache to the fallback variant
but does not update or disable cont_read_cache.
Earlier in spinand_create_dirmap(), the untuned continuous read operation
is assigned the high PHY-validated frequency of this fallback variant:
spinand_create_dirmap() {
...
if (secondary_op) {
info.secondary_op_tmpl = *spinand->op_templates->cont_read_cache;
...
info.secondary_op_tmpl.max_freq = spinand->max_read_op.max_freq;
}
...
}
Could this result in executing rejected, untuned continuous read operations
at the high frequency of the fallback variant, potentially causing data
corruption?
> + return true;
> + }
> + }
> + return false;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=15
^ permalink raw reply
* Re: [PATCH v5 17/17] mtd: spi-nor: run controller optimization before dirmap creation
From: sashiko-bot @ 2026-07-17 19:24 UTC (permalink / raw)
To: Santhosh Kumar K; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717185116.2065505-18-s-k6@ti.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Tuned max_freq is unconditionally applied to all read operations, causing PHY mismatch and data corruption during temporary protocol downgrades like 1-1-1 OTP reads.
- [High] An uninitialized variable in the CQSPI controller's tuning implementation can incorrectly report success when tuning is skipped, causing the SPI clock to run at high frequencies without PHY calibration.
--
--- Patch [17]: [PATCH v5 17/17] mtd: spi-nor: run controller optimization before dirmap creation ---
commit 2bd8b43f8358132165659bf7cc2e06b7b61244b6
Author: Santhosh Kumar K <s-k6@ti.com>
mtd: spi-nor: run controller optimization before dirmap creation
This commit runs controller optimization in spi_nor_probe() before creating
dirmaps so the validated frequency is available at construction time. It
stores the configured read op template in nor->max_read_op and propagates
the validated max_freq into every read op.
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index b683c077a233d..2efcdc7f637fb 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -216,6 +216,9 @@ static struct spi_mem_op spi_nor_spimem_get_read_op(struct spi_nor *nor)
> if (spi_nor_protocol_is_dtr(nor->read_proto))
> op.dummy.nbytes *= 2;
>
> + /* Propagate the validated frequency; zero before tuning. */
> + op.max_freq = nor->max_read_op.max_freq;
> +
[Severity: High]
Could this cause issues when read operations temporarily downgrade the
protocol?
If a read operation temporarily downgrades to 1-1-1, it looks like this will
unconditionally apply the DTR-tuned max_freq.
For example, in spi_nor_otp_read_secr():
nor->read_proto = SNOR_PROTO_1_1_1;
...
ret = spi_nor_read_data(nor, addr, len, buf);
Could propagating the DTR-calibrated frequency into an SDR 1-1-1 read op
cause the SPI controller to erroneously engage a DTR-calibrated PHY and
corrupt the data?
> return op;
> }
>
[ ... ]
> @@ -3843,6 +3846,17 @@ static int spi_nor_probe(struct spi_mem *spimem)
> return -ENOMEM;
> }
>
> + /*
> + * Populate the persistent template and run PHY tuning before dirmap
> + * creation so the validated frequency feeds into the dirmap op.
> + * Tuning failure is non-fatal; the device operates at base speed.
> + */
> + nor->max_read_op = spi_nor_spimem_get_read_op(nor);
> +
> + ret = spi_mem_execute_tuning(spimem, &nor->max_read_op, NULL);
[Severity: High]
Does passing NULL for the write_op here expose an uninitialized return value
in some SPI controllers?
Looking at cqspi_am654_ospi_execute_tuning() in the Cadence QSPI driver,
if write_op is NULL and the pattern partition isn't found, it skips tuning:
drivers/spi/spi-cadence-quadspi.c:cqspi_am654_ospi_execute_tuning() {
int ret;
...
if (!f_pdata->phy_pattern_partition_offset) {
dev_warn(dev, "pattern partition not found, skipping tuning\n");
goto out;
}
...
out:
if (!ret) {
read_op->max_freq = mem->spi->post_config_max_speed_hz;
}
Since ret is uninitialized when the partition is missing, could it happen
to be 0 and falsely report success? If this happens, it looks like it would
enable high-speed operations without a tuned PHY.
> + if (ret && ret != -EOPNOTSUPP)
> + dev_dbg(dev, "Controller optimization failed: %d\n", ret);
> +
> ret = spi_nor_create_read_dirmap(nor);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=17
^ permalink raw reply
* Re: [PATCH 3/5] dt-bindings: vendor-prefixes: Add DCLTEK
From: Rob Herring (Arm) @ 2026-07-17 19:59 UTC (permalink / raw)
To: Marc-Olivier Champagne
Cc: Neil Armstrong, Maarten Lankhorst, Jagan Teki, Thomas Zimmermann,
Simona Vetter, Felix Boucher, David Airlie, Krzysztof Kozlowski,
Quentin Lehoux, linux-kernel, Jerome Oufella, Hugo Villeneuve,
Maxime Ripard, devicetree, Conor Dooley, Jessica Zhang, dri-devel
In-Reply-To: <20260708213500.1867092-4-marc-olivier.champagne@savoirfairelinux.com>
On Wed, 08 Jul 2026 17:34:58 -0400, Marc-Olivier Champagne wrote:
> Add DCL Technologies Inc. (DCLTEK) to the devicetree vendor prefixes
> registry.
>
> Link: https://www.dcltek.com/
>
> Signed-off-by: Marc-Olivier Champagne <marc-olivier.champagne@savoirfairelinux.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 4/5] dt-bindings: display: panel: add compatible for DCLTek 300250 9.35in panel
From: Rob Herring (Arm) @ 2026-07-17 19:59 UTC (permalink / raw)
To: Marc-Olivier Champagne
Cc: Maarten Lankhorst, Jessica Zhang, Hugo Villeneuve, David Airlie,
Maxime Ripard, Felix Boucher, dri-devel, Simona Vetter,
Jagan Teki, Conor Dooley, linux-kernel, Neil Armstrong,
Thomas Zimmermann, Jerome Oufella, Quentin Lehoux,
Krzysztof Kozlowski, devicetree
In-Reply-To: <20260708213500.1867092-5-marc-olivier.champagne@savoirfairelinux.com>
On Wed, 08 Jul 2026 17:34:59 -0400, Marc-Olivier Champagne wrote:
> The DCLTek dt300250 is a panel using the jadard-jd9365da controller.
> Add a new compatible for it.
>
> Suggested-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Signed-off-by: Marc-Olivier Champagne <marc-olivier.champagne@savoirfairelinux.com>
> ---
> .../devicetree/bindings/display/panel/jadard,jd9365da-h3.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 2/4] drm/bridge: display-connector: Fix I2C adapter resource leak
From: Luca Ceresoli @ 2026-07-17 20:06 UTC (permalink / raw)
To: Laurent Pinchart, Luca Ceresoli
Cc: Johan Hovold, dri-devel, devicetree, Thuan Nguyen,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, linux-renesas-soc, stable
In-Reply-To: <20260717182802.GD1889304@killaraus.ideasonboard.com>
Hi Laurent,
On Fri Jul 17, 2026 at 8:28 PM CEST, Laurent Pinchart wrote:
> On Fri, Jul 17, 2026 at 05:21:11PM +0200, Luca Ceresoli wrote:
>> On Fri Jul 17, 2026 at 5:03 PM CEST, Johan Hovold wrote:
>> > On Mon, Jul 06, 2026 at 12:35:40AM +0300, Laurent Pinchart wrote:
>> >> If the probe function returns an error after getting the I2C adapter for
>> >> DDC, the reference to the adapter is never released. Fix it by releasing
>> >> it in the bridge .destroy() handler.
>> >>
>> >> There is no need to test the ddc pointer with !IS_ERR(), as
>> >> of_get_i2c_adapter_by_node() returns NULL on error.
>> >
>> > I stumbled over this this morning as well and posted a fix here (which
>> > releases the adapter on driver unbind as is currently done):
>> >
>> > https://lore.kernel.org/lkml/20260717085716.1619275-1-johan@kernel.org/
>> >
>> >> Fixes: 6de79dd3a920 ("drm/bridge: display-connector: add ddc-en gpio support")
>> >
>> > I believe this issue was first introduced by commit 2e2bf3a5584d
>> > ("drm/bridge: display-connector: add DP support") a few releases
>> > earlier.
>> >
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> >
>> > Reviewed-by: Johan Hovold <johan@kernel.org>
>>
>> While Johan's patch has the benefit of putting the i2c adapter at remove
>> time (as opposed to destroy time, which is potentially a long time later),
>> it is also more complex. So I'd say this patch is fine.
>
> In this case putting the I2C adapter early is probably fine, but in
> general I really prefer reference-counting and releasing references at
> destroy time. This ensures that the resources stay available if they
> need to be accessed between .remove() and destruction (for instance in a
> .release() handler following a close() or munmap() from userspace). I
> think late release should be the default, as it's safer, and early
> release should be carefully reviewed when it's required.
Makes sense indeed.
Thanks for the clarification.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox