Re: [PATCH v1 1/4] spi: dw: Add DDR (Dual Data-Rate) support for enhanced SPI mode

[sashiko-bot@kernel.org]

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@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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/