Re: [PATCH v1 02/11] spi: dw: update NDF while using enhanced spi mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] For Transmit Only (`TMOD_TO`) transfers, `cfg.ndf` is never populated and defaults to 0, causing the hardware to be incorrectly programmed with 0 data frames.
- [Medium] Missing clamp of `op->data.nbytes` for `SPI_MEM_DATA_OUT` leads to `DW_SPI_CTRLR1` register overflow for large transmit-only operations.
- [Low] The macro `DW_SPI_CAP_EMODE` is used but never defined anywhere in the kernel tree, resulting in a compilation error.
--

--- Patch [2]: [PATCH v1 02/11] spi: dw: update NDF while using enhanced spi mode ---
commit b0d60b5fc76ca13c62d210b00ff881436c76397e
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

spi: dw: update NDF while using enhanced spi mode

If the transfer of Transmit only mode is using dual/quad/octal SPI then
NDF needs to be updated with the number of data frames. If the Transmit FIFO
goes empty in-between, DWC_ssi masks the serial clock and wait for rest of
the data until the programmed amount of frames are transferred successfully.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 2c52209c90955..8d4ad4035d7c9 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -348,6 +348,9 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>  	if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
>  	    cfg->tmode == DW_SPI_CTRLR0_TMOD_RO)
>  		dw_writel(dws, DW_SPI_CTRLR1, cfg->ndf ? cfg->ndf - 1 : 0);
> +	else if (cfg->tmode == DW_SPI_CTRLR0_TMOD_TO &&
> +		 dws->caps & DW_SPI_CAP_EMODE)

[Severity: Low]
Is DW_SPI_CAP_EMODE defined anywhere in the kernel tree?  It does not appear
to be declared, which results in a compilation error when building this file.

> +		dw_writel(dws, DW_SPI_CTRLR1, cfg->ndf);

[Severity: High]
Does this correctly program the NDF register for outbound transfers?  Since
cfg.ndf is now used here for TMOD_TO, it seems it might inadvertently be zero.

In dw_spi_exec_mem_op(), cfg is zero-initialized and cfg.ndf is only populated
for SPI_MEM_DATA_IN:

drivers/spi/spi-dw-core.c:dw_spi_exec_mem_op() {
    ...
	if (op->data.dir == SPI_MEM_DATA_IN) {
		cfg.tmode = DW_SPI_CTRLR0_TMOD_EPROMREAD;
		cfg.ndf = op->data.nbytes;
	} else {
		cfg.tmode = DW_SPI_CTRLR0_TMOD_TO;
	}
    ...
	dw_spi_update_config(dws, mem->spi, &cfg);
    ...
}

Could this result in dw_writel() writing 0 data frames to DW_SPI_CTRLR1,
breaking the Enhanced SPI write functionality?

[Severity: Medium]
Since NDF is now used to bound write transfers in enhanced mode, should the
size be clamped for SPI_MEM_DATA_OUT to prevent register overflow?

Looking at dw_spi_adjust_mem_op_size(), the size is only clamped for reads:

drivers/spi/spi-dw-core.c:dw_spi_adjust_mem_op_size() {
	if (op->data.dir == SPI_MEM_DATA_IN)
		op->data.nbytes = clamp_val(op->data.nbytes, 0, DW_SPI_NDF_MASK + 1);

	return 0;
}

Could a large transmit operation pass through and overflow the 16-bit NDF
register when it is written to DW_SPI_CTRLR1 here?

>  
>  	/* Note DW APB SSI clock divider doesn't support odd numbers */
>  	clk_div = (DIV_ROUND_UP(dws->max_freq, cfg->freq) + 1) & 0xfffe;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709055204.138168-1-changhuang.liang@starfivetech.com?part=2