Re: [PATCH v2 05/11] spi: dw: update NDF while using enhanced spi mode

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `cfg.ndf` (Number of Data Frames) field is left uninitialized (0) for Transmit Only operations in `dw_spi_exec_mem_op()`, causing the hardware to be erroneously programmed to transmit 0 frames.
- [Medium] The condition to update the NDF register for Transmit Only mode incorrectly checks the static controller capability instead of the actual transfer format.
--

--- Patch [5]: [PATCH v2 05/11] spi: dw: update NDF while using enhanced spi mode ---
commit 5891fe2f8ceff4e681ce6ee14b10868e4b3ce4bc
Author: Sudip Mukherjee <[email protected]>

spi: dw: update NDF while using enhanced spi mode

This commit updates the driver to configure the Number of Data Frames (NDF)
register during Transmit Only mode on enhanced SPI controllers. It modifies
dw_spi_update_config() to write cfg->ndf to DW_SPI_CTRLR1 when the transfer
mode is DW_SPI_CTRLR0_TMOD_TO and the controller supports enhanced SPI.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index f1b7328fed772..d654cb748646a 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)
> +		dw_writel(dws, DW_SPI_CTRLR1, cfg->ndf);

[Severity: Medium]
Will this write a 0 to DW_SPI_CTRLR1 for outbound memory operations?

Looking at dw_spi_exec_mem_op(), cfg is initialized to zero, and cfg.ndf is
only assigned for SPI_MEM_DATA_IN:

drivers/spi/spi-dw-core.c:dw_spi_exec_mem_op() {
	struct dw_spi_cfg cfg = {0};
	...
	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;
	}
	...
}

For transmit operations (SPI_MEM_DATA_OUT), cfg.ndf remains 0 when it's passed
into dw_spi_update_config(), causing the hardware to be programmed to transmit
0 frames.

[Severity: Medium]
Does this condition correctly identify an Enhanced SPI transfer?

Checking dws->caps & DW_SPI_CAP_EMODE verifies that the controller supports
Enhanced SPI, but it does not check if the current transfer is actually
using it.

Could this cause standard 1-bit SPI memory writes on EMODE-capable controllers
to inappropriately update DW_SPI_CTRLR1 with cfg->ndf?

>  	/* 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/[email protected]?part=5