[PATCH v1 1/4] spi: dw: Add DDR (Dual Data-Rate) support for enhanced SPI mode
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Thu Oct 22 03:48:28 PDT 2026
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] spi: dw: unconditional DTR capability advertisement in dw_spi_add_controller()
- [High] spi: dw: missing DTR phase validation in dw_spi_supports_enh_mem_op()
- [High] spi: dw: broken no-data 8D-0-0 and 8D-8D-0 ops in dw_spi_supports_enh_mem_op()
- [Medium] spi: dw: odd dummy half-cycle truncation in dw_spi_enh_wait_cycles()
--
commit b43a1a39eac428f70be5daaca07fad4710978bc3
Author: Changhuang Liang <changhuang.liang at starfivetech.com>
spi: dw: Add DDR (Dual Data-Rate) support for enhanced SPI mode
This commit adds Dual Data-Rate (DDR) transfer support for the enhanced SPI
mode of the DesignWare SPI controller by configuring the instruction and SPI
DDR enable bits in SPI_CTRLR0, halving the dummy wait cycles for DTR phases,
and programming the DDR_DRIVE_EDGE register. It also advertises DTR capability
to spi-mem when DW_SPI_CAP_EMODE is present.
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 47434fa78e4b6..6ff142c8a6bd9 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
[ ... ]
> @@ -468,11 +471,26 @@ static void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>
> if (enh_cfg) {
> cr0 = DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN;
> + if (enh_cfg->inst_ddr)
> + cr0 |= DW_SPI_ENH_CTRLR0_INST_DDR_EN;
> + if (enh_cfg->spi_ddr)
> + cr0 |= DW_SPI_ENH_CTRLR0_SPI_DDR_EN;
> cr0 |= FIELD_PREP(DW_SPI_ENH_CTRLR0_WAIT_CYCLE_MASK, enh_cfg->wait_c);
> cr0 |= FIELD_PREP(DW_SPI_ENH_CTRLR0_INST_L_MASK, enh_cfg->inst_l);
> cr0 |= FIELD_PREP(DW_SPI_ENH_CTRLR0_ADDR_L_MASK, enh_cfg->addr_l);
> cr0 |= FIELD_PREP(DW_SPI_ENH_CTRLR0_TRANS_TYPE_MASK, enh_cfg->trans_t);
> dw_writel(dws, DW_SPI_SPI_CTRLR0, cr0);
> +
> + /*
> + * The recommended value of TDE is clk_div / 4, and it must not exceed
> + * SCKDV / 2 - 1
> + */
> + if (enh_cfg->spi_ddr)
> + tde = min3(clk_div / 4U, clk_div / 2U - 1,
> + FIELD_MAX(DW_SPI_DDR_DRIVE_EDGE_TDE_MASK));
> +
> + dw_writel(dws, DW_SPI_DDR_DRIVE_EDGE,
> + FIELD_PREP(DW_SPI_DDR_DRIVE_EDGE_TDE_MASK, tde));
> }
> }
>
> @@ -656,6 +674,21 @@ static int dw_spi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> return 0;
> }
>
> +static u32 dw_spi_enh_wait_cycles(const struct spi_mem_op *op)
> +{
> + u32 wait_c;
> +
> + if (!op->dummy.nbytes || !op->dummy.buswidth)
> + return 0;
> +
> + wait_c = op->dummy.nbytes * BITS_PER_BYTE / op->dummy.buswidth;
> + /* DTR transfers two bits per clock on each line. */
> + if (op->dummy.dtr)
> + wait_c >>= 1;
> +
> + return wait_c;
> +}
[Severity: Medium]
Can wait_c >>= 1 in dw_spi_enh_wait_cycles() silently truncate odd dummy
half-cycles instead of rejecting unsupported operations in
dw_spi_supports_enh_mem_op()?
The WAIT_CYCLES field in DW_SPI_SPI_CTRLR0 counts whole clock cycles, where
each DTR clock cycle transfers two bits per line. When op->dummy.dtr is true
and op->dummy.nbytes * BITS_PER_BYTE / op->dummy.buswidth is odd (for
example, a 1S-8D-8D operation with op->dummy.buswidth == 8 and an odd
op->dummy.nbytes, which spi_mem_default_supports_op() does not reject
because op->cmd.dtr is false), shifting wait_c right by 1 discards the
remainder half-cycle (turning 1 dummy byte on 8 lines into 0 wait cycles).
Should dw_spi_supports_enh_mem_op() verify that the dummy phase represents
an exact integer number of clock cycles before accepting the operation?
> +
> static bool dw_spi_supports_enh_mem_op(struct spi_mem *mem,
> const struct spi_mem_op *op)
> {
> @@ -678,10 +711,7 @@ static bool dw_spi_supports_enh_mem_op(struct spi_mem *mem,
> if (op->dummy.nbytes != 0 && op->data.dir == SPI_MEM_DATA_OUT)
> return false;
>
> - /* WAIT_CYCLES is a 5-bit field in SPI_CTRLR0 */
> - if (op->dummy.nbytes != 0 &&
> - op->dummy.nbytes * BITS_PER_BYTE / op->dummy.buswidth >
> - FIELD_MAX(DW_SPI_ENH_CTRLR0_WAIT_CYCLE_MASK))
> + if (dw_spi_enh_wait_cycles(op) > FIELD_MAX(DW_SPI_ENH_CTRLR0_WAIT_CYCLE_MASK))
> return false;
>
> if ((dws->quirk_flags & DW_SPI_QUIRK_JHB100) && op->addr.nbytes &&
[Severity: High]
Does dw_spi_supports_enh_mem_op() accept DTR phase combinations that
dw_spi_exec_enh_mem_op() and the DW SSI hardware cannot execute?
When ctlr->mem_caps->dtr is set, spi_mem_supports_op() calls
dw_spi_supports_enh_mem_op(), which delegates to
spi_mem_default_supports_op() without checking DTR buswidths, transfer type
constraints, or instruction lengths:
drivers/spi/spi-dw-core.c:dw_spi_exec_enh_mem_op() {
...
switch (op->data.buswidth) {
case 0:
case 1:
cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_STD_SPI;
break;
...
if (op->data.buswidth == op->addr.buswidth &&
op->data.buswidth == op->cmd.buswidth)
enh_cfg.trans_t = DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT2;
else if (op->data.buswidth == op->addr.buswidth)
enh_cfg.trans_t = DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT1;
else
enh_cfg.trans_t = DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT0;
...
enh_cfg.inst_ddr = op->cmd.dtr;
enh_cfg.spi_ddr = op->addr.dtr || op->dummy.dtr || op->data.dtr;
}
This allows several unsupported DTR patterns through validation during SPI
NOR and SPI NAND probing:
1. For 1-wire DTR operations (op->data.buswidth == 1, such as
SNOR_PROTO_1_1_1_DTR or SPINAND_PAGE_READ_FROM_CACHE_1S_1D_1D_OP),
dw_spi_exec_enh_mem_op() selects DW_SPI_CTRLR0_SPI_FRF_STD_SPI, where
DW_SPI_SPI_CTRLR0 and DW_SPI_ENH_CTRLR0_SPI_DDR_EN do not operate.
2. For 1-bit DTR address with multi-bit DTR data (op->addr.buswidth == 1 &&
op->data.buswidth > 1, such as SPINAND_PAGE_READ_FROM_CACHE_1S_1D_8D_OP,
1S_1D_4D_OP, and 1S_1D_2D_OP in drivers/mtd/nand/spi/winbond.c),
dw_spi_exec_enh_mem_op() selects DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT0, which
transmits both the instruction and address phases in 1-wire SDR mode rather
than DDR.
3. For 1-bit DTR instruction with multi-bit DTR address/data
(op->cmd.buswidth == 1 && op->cmd.dtr), dw_spi_exec_enh_mem_op() selects
TT0 or TT1 with DW_SPI_ENH_CTRLR0_INST_DDR_EN set, whereas INST_DDR_EN is
only valid in DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT2 when
DW_SPI_ENH_CTRLR0_SPI_DDR_EN is also enabled.
4. Because op->addr.dtr || op->dummy.dtr || op->data.dtr is collapsed into a
single DW_SPI_ENH_CTRLR0_SPI_DDR_EN bit controlling both address and data
phases simultaneously, mixed SDR/DDR address and data phases cannot be
configured independently.
5. In spi_mem_default_supports_op(), the op->cmd.nbytes != 1 check is
skipped whenever any phase has dtr set, and op->cmd.nbytes == 2 is only
enforced when op->cmd.dtr && op->cmd.buswidth == 8. Without an explicit
op->cmd.nbytes <= 2 check in dw_spi_supports_enh_mem_op(), non-8D DTR ops
with op->cmd.nbytes > 2 are accepted and programmed with
DW_SPI_ENH_CTRLR0_INST_L_INST_L0.
Should dw_spi_supports_enh_mem_op() reject these unsupported DTR phase and
instruction length combinations?
> @@ -1074,8 +1104,10 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
> else
> enh_cfg.inst_l = DW_SPI_ENH_CTRLR0_INST_L_INST_L0;
>
> - if (op->dummy.buswidth)
> - enh_cfg.wait_c = op->dummy.nbytes * BITS_PER_BYTE / op->dummy.buswidth;
> + enh_cfg.wait_c = dw_spi_enh_wait_cycles(op);
> +
> + enh_cfg.inst_ddr = op->cmd.dtr;
> + enh_cfg.spi_ddr = op->addr.dtr || op->dummy.dtr || op->data.dtr;
>
> dw_spi_enable_chip(dws, 0);
>
[Severity: High]
How are non-data Octal DTR operations (8D-0-0 and 8D-8D-0) handled when
op->data.nbytes == 0 and op->data.buswidth == 0?
Once an Octal DTR flash enters stateful 8D-8D-8D mode (for example via
spi_nor_init() setting SNOR_PROTO_8_8_8_DTR after 8D-8D-8D read and page
program pass dw_spi_supports_enh_mem_op()), command-only operations such as
Write Enable or Soft Reset (8D-0-0) and address-only operations such as
Sector or Block Erase (8D-8D-0) have op->data.nbytes == 0 and
op->data.buswidth == 0.
In dw_spi_supports_enh_mem_op(), the buswidth checks compare
op->addr.buswidth and op->cmd.buswidth directly against op->data.buswidth:
drivers/spi/spi-dw-core.c:dw_spi_supports_enh_mem_op() {
if (op->addr.nbytes != 0 && op->addr.buswidth != 1 &&
op->addr.buswidth != op->data.buswidth)
return false;
...
if (op->cmd.buswidth != 1 && op->cmd.buswidth != op->addr.buswidth &&
op->cmd.buswidth != op->data.buswidth)
return false;
}
When op->data.buswidth is 0, both 8D-8D-0 and 8D-0-0 operations fail these
checks and return false.
Additionally, in dw_spi_exec_enh_mem_op(), cfg.spi_frf and enh_cfg.trans_t
are derived from op->data.buswidth (falling back to
DW_SPI_CTRLR0_SPI_FRF_STD_SPI and DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT0 when
op->data.buswidth is 0), and for 8D-0-0 operations where only op->cmd.dtr is
set, enh_cfg.spi_ddr evaluates to false, leaving
DW_SPI_ENH_CTRLR0_SPI_DDR_EN unset and tde = 0 in dw_spi_update_config().
Could dw_spi_supports_enh_mem_op() and dw_spi_exec_enh_mem_op() derive the
transfer buswidth, frame format, and DDR enable state from the active phases
when no data phase is present?
> @@ -1383,6 +1415,11 @@ static const struct spi_controller_mem_caps dw_spi_mem_caps = {
> .per_op_freq = true,
> };
>
> +static const struct spi_controller_mem_caps dw_spi_enh_mem_caps = {
> + .per_op_freq = true,
> + .dtr = true,
> +};
> +
> int dw_spi_add_controller(struct device *dev, struct dw_spi *dws)
> {
> struct spi_controller *ctlr;
> @@ -1439,7 +1476,8 @@ int dw_spi_add_controller(struct device *dev, struct dw_spi *dws)
> ctlr->set_cs = dw_spi_set_cs;
> if (dws->mem_ops.exec_op) {
> ctlr->mem_ops = &dws->mem_ops;
> - ctlr->mem_caps = &dw_spi_mem_caps;
> + ctlr->mem_caps = (dws->caps & DW_SPI_CAP_EMODE) ?
> + &dw_spi_enh_mem_caps : &dw_spi_mem_caps;
> }
> ctlr->max_speed_hz = dws->max_freq;
> ctlr->flags = SPI_CONTROLLER_GPIO_SS;
[Severity: High]
Is it safe to advertise dw_spi_enh_mem_caps (.dtr = true) for all
controllers with DW_SPI_CAP_EMODE without probing whether the hardware
actually synthesizes DDR support?
During initialization in dw_spi_add_controller() -> dw_spi_hw_init() ->
detect_enh_mode(), the driver sets DW_SPI_CAP_EMODE after testing whether
Dual/Quad/Octal frame formats in DW_SPI_CTRLR0 and
DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN in DW_SPI_SPI_CTRLR0 are writable:
drivers/spi/spi-dw-core.c:detect_enh_mode() {
...
/* test clock stretching */
dw_writel(dws, DW_SPI_SPI_CTRLR0, DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN);
if ((DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN & dw_readl(dws, DW_SPI_SPI_CTRLR0)) !=
DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN) {
mode = 0;
goto disable_enh;
}
dws->caps |= DW_SPI_CAP_EMODE;
}
On Synopsys DW_apb_ssi (PSSI) and DWC_ssi (HSSI) controllers, enhanced SPI
mode, clock stretching, and DDR support (SSI_HAS_DDR, introduced in
DW_apb_ssi v4.02a) are independent synthesis parameters. When SSI_HAS_DDR is
not synthesized, DW_SPI_ENH_CTRLR0_SPI_DDR_EN and
DW_SPI_ENH_CTRLR0_INST_DDR_EN are read-only zero bits, and the
DW_SPI_DDR_DRIVE_EDGE register at offset 0xf8 is not implemented.
If a DTR-capable flash is attached to a controller that has DW_SPI_CAP_EMODE
but lacks hardware DDR support, advertising dw_spi_enh_mem_caps causes
spi-mem to issue DTR operations where DW_SPI_ENH_CTRLR0_SPI_DDR_EN is
ignored by hardware while dw_spi_enh_wait_cycles() halves the dummy cycles,
breaking flash transfers. In addition, dw_spi_update_config() will
unconditionally write to the unimplemented DW_SPI_DDR_DRIVE_EDGE register on
every enhanced SPI transfer.
Should detect_enh_mode() probe whether DW_SPI_ENH_CTRLR0_SPI_DDR_EN is
writable in DW_SPI_SPI_CTRLR0 (similar to the clock stretching check) and
record a dedicated capability flag before advertising DTR support or writing
DW_SPI_DDR_DRIVE_EDGE?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022102614.75696-1-changhuang.liang@starfivetech.com?part=1
More information about the linux-mtd
mailing list