Re: [PATCH 3/3] spi: cadence-xspi: add ACMD mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi: cadence-xspi: restore work mode and ACMD config in cdns_xspi_resume()
- [Medium] spi: cadence-xspi: xfer_error check in cdns_xspi_acmd_read() bypasses status check
- [Medium] spi: cadence-xspi: missing STIG mode in cdns_xspi_transfer_one_message_b0()
--

commit db673b09285a06730d352ef002e9477417c0c965
Author: Nuno Sá <nuno.sa@analog.com>

spi: cadence-xspi: add ACMD mode support

This commit adds auto command (ACMD) mode support using the internal DMA for
SPI memory read operations of at least 2048 bytes that include an address
phase. Smaller reads and other operations continue to use STIG mode.

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 905292fa50a25..b2c1ba86fa149 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c

[ ... ]

> @@ -378,13 +435,19 @@ static void cdns_xspi_trigger_command(struct cdns_xspi_dev *cdns_xspi,
>  	writel(cmd_regs[0], cdns_xspi->iobase + CDNS_XSPI_CMD_REG_0);
>  }
>  
> -static int cdns_xspi_check_command_status(struct cdns_xspi_dev *cdns_xspi)
> +static int cdns_xspi_check_command_status(struct cdns_xspi_dev *cdns_xspi,
> +					  bool acmd)
>  {
>  	int ret = 0;
>  	u32 cmd_status = readl(cdns_xspi->iobase + CDNS_XSPI_CMD_STATUS_REG);
>  
>  	if (cmd_status & CDNS_XSPI_CMD_STATUS_COMPLETED) {
>  		if ((cmd_status & CDNS_XSPI_CMD_STATUS_FAILED) != 0) {
> +			if (acmd && cmd_status & CDNS_XSPI_CMD_STATUS_DEVICE_ERROR) {
> +				dev_err(cdns_xspi->dev,
> +					"Device reported an error\n");
> +				ret = -EPROTO;
> +			}
>  			if (cmd_status & CDNS_XSPI_CMD_STATUS_DQS_ERROR) {
>  				dev_err(cdns_xspi->dev,
>  					"Incorrect DQS pulses detected\n");

[ ... ]

> @@ -447,6 +543,9 @@ static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi)
>  	cdns_xspi->dma_data_width = (ctrl_features & CDNS_XSPI_DMA_DATA_WIDTH) ? 8 : 4;
>  	cdns_xspi->set_interrupts_handler(cdns_xspi, false);
>  
> +	if (cdns_xspi->has_acmd)
> +		cdns_xspi_acmd_init(cdns_xspi);
> +
>  	return 0;
>  }
>  
> @@ -527,6 +626,24 @@ static int cdns_xspi_sdma_handle(struct cdns_xspi_dev *cdns_xspi)
>  	return 0;
>  }
>  
> +static int cdns_xspi_set_work_mode(struct cdns_xspi_dev *cdns_xspi, int mode)
> +{
> +	int ret;
> +
> +	if (cdns_xspi->cur_work_mode == mode)
> +		return 0;
> +
> +	ret = cdns_xspi_wait_for_controller_idle(cdns_xspi);
> +	if (ret < 0)
> +		return ret;
> +
> +	writel(FIELD_PREP(CDNS_XSPI_CTRL_WORK_MODE, mode),
> +	       cdns_xspi->iobase + CDNS_XSPI_CTRL_CONFIG_REG);
> +	cdns_xspi->cur_work_mode = mode;
> +
> +	return 0;
> +}

[Severity: High]
What happens across a system suspend and resume cycle when
cdns_xspi->cur_work_mode has been cached as CDNS_XSPI_WORK_MODE_STIG or
CDNS_XSPI_WORK_MODE_ACMD?

Previously, cdns_xspi_send_stig_command() wrote CDNS_XSPI_CTRL_CONFIG_REG
unconditionally on every command. With cdns_xspi_set_work_mode() skipping the
register write when cdns_xspi->cur_work_mode == mode, and cdns_xspi_acmd_init()
only running in cdns_xspi_controller_init() during probe, a controller reset
during system suspend will leave CDNS_XSPI_CTRL_CONFIG_REG in
CDNS_XSPI_WORK_MODE_DIRECT (0) and clear CDNS_XSPI_SEQ_GCTL0_REG and
CDNS_XSPI_READ_SEQ_CFG1_REG.

Looking at cdns_xspi_resume() in drivers/spi/spi-cadence-xspi.c:

static int cdns_xspi_resume(struct device *dev)
{
	struct cdns_xspi_dev *cdns_xspi = dev_get_drvdata(dev);

#ifdef CONFIG_64BIT
	if (cdns_xspi->driver_data->mrvl_hw_overlay) {
		cdns_mrvl_xspi_setup_clock(cdns_xspi, MRVL_DEFAULT_CLK);
		cdns_xspi_configure_phy(cdns_xspi);
	}
#endif

	cdns_xspi->set_interrupts_handler(cdns_xspi, false);

	return spi_controller_resume(cdns_xspi->host);
}

Should cdns_xspi_resume() reset cdns_xspi->cur_work_mode to
CDNS_XSPI_WORK_MODE_DIRECT and call cdns_xspi_acmd_init() when
cdns_xspi->has_acmd is set so that post-resume SPI operations do not skip
reprogramming CDNS_XSPI_CTRL_CONFIG_REG and the ACMD sequence registers?

> +
>  static int cdns_xspi_send_stig_command(struct cdns_xspi_dev *cdns_xspi,
>  				       const struct spi_mem_op *op,
>  				       bool data_phase)
> @@ -536,12 +653,9 @@ static int cdns_xspi_send_stig_command(struct cdns_xspi_dev *cdns_xspi,
>  	int ret;
>  	int dummybytes = op->dummy.nbytes;
>  
> -	ret = cdns_xspi_wait_for_controller_idle(cdns_xspi);
> -	if (ret < 0)
> -		return -EIO;
> -
> -	writel(FIELD_PREP(CDNS_XSPI_CTRL_WORK_MODE, CDNS_XSPI_WORK_MODE_STIG),
> -	       cdns_xspi->iobase + CDNS_XSPI_CTRL_CONFIG_REG);
> +	ret = cdns_xspi_set_work_mode(cdns_xspi, CDNS_XSPI_WORK_MODE_STIG);
> +	if (ret)
> +		return ret;
>  
>  	cdns_xspi->set_interrupts_handler(cdns_xspi, true);
>  	cdns_xspi->xfer_error = false;
> @@ -589,13 +703,112 @@ static int cdns_xspi_send_stig_command(struct cdns_xspi_dev *cdns_xspi,
>  	wait_for_completion(&cdns_xspi->cmd_complete);
>  	cdns_xspi->set_interrupts_handler(cdns_xspi, false);
>  
> -	cmd_status = cdns_xspi_check_command_status(cdns_xspi);
> +	cmd_status = cdns_xspi_check_command_status(cdns_xspi, false);
>  	if (cmd_status)
>  		return -EPROTO;
>  
>  	return 0;
>  }
>  
> +static void cdns_xspi_acmd_setup_read_seq(struct cdns_xspi_dev *cdns_xspi,
> +					  const struct spi_mem_op *op)
> +{
> +	u32 cfg0;
> +
> +	cfg0 = FIELD_PREP(CDNS_XSPI_READ_SEQ_CFG0_CMD_VALUE, op->cmd.opcode) |
> +	       FIELD_PREP(CDNS_XSPI_READ_SEQ_CFG0_CMD_IOS,
> +			  ilog2(op->cmd.buswidth)) |
> +	       FIELD_PREP(CDNS_XSPI_READ_SEQ_CFG0_ADDR_CNT, op->addr.nbytes) |
> +	       FIELD_PREP(CDNS_XSPI_READ_SEQ_CFG0_ADDR_IOS,
> +			  ilog2(op->addr.buswidth)) |
> +	       FIELD_PREP(CDNS_XSPI_READ_SEQ_CFG0_DAT_IOS,
> +			  ilog2(op->data.buswidth));
> +
> +	if (op->dummy.nbytes && op->dummy.buswidth)
> +		cfg0 |= FIELD_PREP(CDNS_XSPI_READ_SEQ_CFG0_DMY_CNT,
> +				   (op->dummy.nbytes * 8) / op->dummy.buswidth);
> +
> +	writel(cfg0, cdns_xspi->iobase + CDNS_XSPI_READ_SEQ_CFG0_REG);
> +}
> +
> +static int cdns_xspi_acmd_check_status(struct cdns_xspi_dev *cdns_xspi)
> +{
> +	writel(FIELD_PREP(CDNS_XSPI_CMD_STATUS_PTR_SEL, CDNS_XSPI_ACMD_THRD_0),
> +	       cdns_xspi->iobase + CDNS_XSPI_CMD_STATUS_PTR_REG);
> +
> +	return cdns_xspi_check_command_status(cdns_xspi, true);
> +}
> +
> +static int cdns_xspi_acmd_read(struct cdns_xspi_dev *cdns_xspi,
> +			       const struct spi_mem_op *op)
> +{
> +	dma_addr_t dma_addr;
> +	u32 cmd_regs[6];
> +	int ret;
> +
> +	ret = cdns_xspi_set_work_mode(cdns_xspi, CDNS_XSPI_WORK_MODE_ACMD);
> +	if (ret)
> +		return ret;
> +
> +	ret = cdns_xspi_wait_for_acmd_idle(cdns_xspi);
> +	if (ret)
> +		return ret;
> +
> +	cdns_xspi_acmd_setup_read_seq(cdns_xspi, op);
> +
> +	dma_addr = dma_map_single(cdns_xspi->dev, op->data.buf.in,
> +				  op->data.nbytes, DMA_FROM_DEVICE);
> +	if (dma_mapping_error(cdns_xspi->dev, dma_addr))
> +		return -ENOMEM;
> +
> +	cdns_xspi->xfer_error = false;
> +	writel(CDNS_XSPI_ACMD_THRD_0_MASK,
> +	       cdns_xspi->iobase + CDNS_XSPI_TRD_COMP_INTR_STATUS);
> +	writel(CDNS_XSPI_ACMD_THRD_0_MASK,
> +	       cdns_xspi->iobase + CDNS_XSPI_TRD_ERR_INTR_STATUS);
> +	cdns_xspi_set_thread_error_interrupt(cdns_xspi, true);
> +	cdns_xspi->set_interrupts_handler(cdns_xspi, true);
> +
> +	cmd_regs[1] = lower_32_bits(op->addr.val);
> +	cmd_regs[5] = upper_32_bits(op->addr.val);
> +	cmd_regs[2] = lower_32_bits(dma_addr);
> +	cmd_regs[3] = upper_32_bits(dma_addr);
> +	cmd_regs[4] = op->data.nbytes - 1;
> +	cmd_regs[0] = FIELD_PREP(CDNS_XSPI_ACMD_MODE,
> +				 CDNS_XSPI_ACMD_MODE_PIO) |
> +		      FIELD_PREP(CDNS_XSPI_ACMD_TRD_NUM, CDNS_XSPI_ACMD_THRD_0) |
> +		      FIELD_PREP(CDNS_XSPI_ACMD_BANK, cdns_xspi->cur_cs) |
> +		      CDNS_XSPI_ACMD_DMA_SEL | CDNS_XSPI_ACMD_INT |
> +		      FIELD_PREP(CDNS_XSPI_ACMD_CMD_TYPE,
> +				 CDNS_XSPI_ACMD_TYPE_READ);
> +
> +	reinit_completion(&cdns_xspi->auto_cmd_complete);
> +	cdns_xspi_trigger_command(cdns_xspi, cmd_regs);
> +	wait_for_completion(&cdns_xspi->auto_cmd_complete);
> +
> +	cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> +	cdns_xspi_set_thread_error_interrupt(cdns_xspi, false);
> +	dma_unmap_single(cdns_xspi->dev, dma_addr, op->data.nbytes,
> +			 DMA_FROM_DEVICE);
> +
> +	if (cdns_xspi->xfer_error)
> +		return -EIO;
> +
> +	return cdns_xspi_acmd_check_status(cdns_xspi);
> +}

[Severity: Medium]
Does checking cdns_xspi->xfer_error and returning -EIO here prevent
cdns_xspi_acmd_check_status() from ever running when an ACMD command fails?

When thread 0 encounters an ACMD command error (setting
CDNS_XSPI_CMD_STATUS_FAILED in CDNS_XSPI_CMD_STATUS_REG due to
CDNS_XSPI_CMD_STATUS_DEVICE_ERROR, CDNS_XSPI_CMD_STATUS_DQS_ERROR,
CDNS_XSPI_CMD_STATUS_CRC_ERROR, CDNS_XSPI_CMD_STATUS_BUS_ERROR, or
CDNS_XSPI_CMD_STATUS_INV_SEQ_ERROR), the controller raises the thread 0 error
interrupt in CDNS_XSPI_TRD_ERR_INTR_STATUS.

In cdns_xspi_irq_handler(), that interrupt sets cdns_xspi->xfer_error = true
and completes cdns_xspi->auto_cmd_complete:

	irq_status = readl(cdns_xspi->iobase + CDNS_XSPI_TRD_ERR_INTR_STATUS);
	if (irq_status) {
		writel(irq_status,
		       cdns_xspi->iobase + CDNS_XSPI_TRD_ERR_INTR_STATUS);

		if (irq_status & CDNS_XSPI_ACMD_THRD_0_MASK) {
			cdns_xspi->xfer_error = true;
			acmd_done = true;
		}

		result = IRQ_HANDLED;
	}

Because cdns_xspi_acmd_read() returns -EIO as soon as cdns_xspi->xfer_error is
true, cdns_xspi_acmd_check_status() -> cdns_xspi_check_command_status() is
bypassed on thread error interrupts, skipping all error logging (including the
new CDNS_XSPI_CMD_STATUS_DEVICE_ERROR check) and returning -EIO instead of
-EPROTO.

> +
> +static bool cdns_xspi_acmd_can_read(const struct cdns_xspi_dev *cdns_xspi,
> +				    const struct spi_mem_op *op)
> +{
> +	if (!cdns_xspi->has_acmd)
> +		return false;
> +
> +	if (op->data.dir != SPI_MEM_DATA_IN || !op->addr.nbytes)
> +		return false;
> +
> +	return op->data.nbytes >= CDNS_XSPI_ACMD_MIN_BYTES;
> +}
> +
>  static int cdns_xspi_mem_op(struct cdns_xspi_dev *cdns_xspi,
>  			    struct spi_mem *mem,
>  			    const struct spi_mem_op *op)
> @@ -605,6 +818,9 @@ static int cdns_xspi_mem_op(struct cdns_xspi_dev *cdns_xspi,
>  	if (cdns_xspi->cur_cs != spi_get_chipselect(mem->spi, 0))
>  		cdns_xspi->cur_cs = spi_get_chipselect(mem->spi, 0);
>  
> +	if (cdns_xspi_acmd_can_read(cdns_xspi, op))
> +		return cdns_xspi_acmd_read(cdns_xspi, op);
> +
>  	return cdns_xspi_send_stig_command(cdns_xspi, op,
>  					   (dir != SPI_MEM_NO_DATA));
>  }

[Severity: Medium]
Can cdns_xspi_transfer_one_message_b0() issue STIG commands while the
controller is still left in CDNS_XSPI_WORK_MODE_ACMD?

When mrvl_hw_overlay is enabled, marvell_xspi_mem_ops.exec_op calls
marvell_xspi_mem_op_execute() -> cdns_xspi_mem_op() -> cdns_xspi_acmd_read(),
which switches CDNS_XSPI_CTRL_CONFIG_REG to CDNS_XSPI_WORK_MODE_ACMD.

However, cdns_xspi_transfer_one_message_b0() in
drivers/spi/spi-cadence-xspi.c triggers STIG generic commands directly without
calling cdns_xspi_set_work_mode(cdns_xspi, CDNS_XSPI_WORK_MODE_STIG):

			if (current_transfer_len < 10) {
				cdns_xspi_prepare_generic(cs, txd, current_transfer_len,
							  false, cmd_regs);
				cdns_xspi_trigger_command(cdns_xspi, cmd_regs);
				if (!cdns_xspi_is_stig_ready(cdns_xspi, true))
					return -EIO;
			} else {
				cdns_xspi_prepare_generic(cs, txd, 1, true, cmd_regs);
				cdns_xspi_trigger_command(cdns_xspi, cmd_regs);
				cdns_xspi_prepare_transfer(cs, 1, current_transfer_len - 1,
							   cmd_regs);
				cdns_xspi_trigger_command(cdns_xspi, cmd_regs);

If a controller with mrvl_hw_overlay uses ACMD mode and then runs a regular SPI
transfer via cdns_xspi_transfer_one_message_b0(), won't writing STIG command
words to CDNS_XSPI_CMD_REG_0..5 while in ACMD mode be misinterpreted by the
hardware as an ACMD command?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-xspi-cadence-add-acmd-v1-0-53cd44690e04@analog.com?part=3