Re: [RFC PATCH 2/4] spi: cadence-xspi: add ACMD support for SPI NAND

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]

On Mon, Sep 21, 2026 at 05:36:59PM +0800, Fei Xie wrote:

> Add PIO master-DMA support for the controller auto-command mode.
> Configure the SPI NAND read, program, erase, status and reset
> sequences, retaining STIG for operations not consumed by ACMD.

> +struct cdns_xspi_acmd_info {
> +	u64 row_addr;
> +	u64 column_addr;
> +	size_t data_nbytes;
> +	bool row_addr_valid;
> +	bool initialized;
> +};
>  struct cdns_xspi_dev {

Missing blank line.

> +static void cdns_xspi_nand_erase_seq_init(struct cdns_xspi_dev *cdns_xspi,
> +					  struct spinand_device *spinand)
> +{
> +	u32 erase_seq_cfg0;
> +	u32 erase_seq_cfg1;
> +
> +	/* SPI-NAND block erase is always D8h with a 3-byte row address. */
> +	erase_seq_cfg0 =
> +		FIELD_PREP(CDNS_XSPI_ERSS_SEQ_P1_CMD_VAL, 0xd8) |

CDNS_XSPI_NAND_OP_BLOCK_ERASE.

> +static int cdns_xspi_nand_init(struct cdns_xspi_dev *cdns_xspi,
> +			       struct spinand_device *spinand)
> +{

How much of this initialisation is parameters based on the specific
flash passed in - what if there are two different flashes attached to
the same controller for some reason?

> +	cdns_xspi->dma_buf_len = spinand->base.memorg.pagesize +
> +				 spinand->base.memorg.oobsize;
> +	cdns_xspi->dma_buf = dmam_alloc_coherent(cdns_xspi->dev,
> +						 cdns_xspi->dma_buf_len,
> +						 &cdns_xspi->dma_addr,
> +						 GFP_KERNEL);
> +	if (!cdns_xspi->dma_buf)
> +		return -ENOMEM;

Why do we need the DMA buffer, I'd expect whatever reaches the driver to
already be DMA safe?

> +static int cdns_xspi_acmd_run(struct cdns_xspi_dev *cdns_xspi, u32 cmd_regs[6],
> +			      u32 thread)
> +{
> +	unsigned long timeout;
> +	int ret;
> +
> +	cdns_xspi_set_mode_acmd(cdns_xspi);
> +	reinit_completion(&cdns_xspi->auto_cmd_complete);
> +	cdns_xspi_set_interrupts(cdns_xspi, true);

We have the set_interrupts() operation.

> +	cdns_xspi_trigger_command(cdns_xspi, cmd_regs);
> +
> +	timeout = msecs_to_jiffies(CDNS_XSPI_ACMD_TIMEOUT_MS);
> +	if (!wait_for_completion_timeout(&cdns_xspi->auto_cmd_complete,
> +					 timeout)) {
> +		dev_err(cdns_xspi->dev, "ACMD command timed out\n");
> +		ret = -ETIMEDOUT;

Don't we need to clean up the hardware if this times out?

> +	} else {
> +		ret = cdns_xspi_acmd_get_thread_status(cdns_xspi, thread);
> +	}

Does this need any updates to cover more error types?

> +static int cdns_xspi_pio_mdma_read(struct cdns_xspi_dev *cdns_xspi,
> +				   struct spinand_device *spinand,
> +				   const struct spi_mem_op *op)
> +{

> +	ret = cdns_xspi_acmd_run(cdns_xspi, cmd_regs,
> +				 CDNS_XSPI_ACMD_DATA_THREAD);
> +	if (ret) {
> +		dev_err(cdns_xspi->dev, "ACMD read failed: %d\n", ret);
> +		goto out_clear_read_state;
> +	}
> +
> +	memcpy(op->data.buf.in, cdns_xspi->dma_buf, op->data.nbytes);
> +
> +out_clear_read_state:
> +	cdns_xspi->acmd_info.row_addr_valid = false;
> +	cdns_xspi->acmd_info.row_addr = 0;
> +	return ret;
> +}

Does this do the right thing for short reads?

> +static int cdns_xspi_send_pio_command(struct cdns_xspi_dev *cdns_xspi,
> +				      struct spi_mem *mem,
> +				      const struct spi_mem_op *op)
> +{
> +	struct spinand_device *spinand;
> +	const struct spi_mem_op *read_cache;
> +	const struct spi_mem_op *write_cache;
> +	const struct spi_mem_op *update_cache;
> +	int ret;
> +
> +	if (cdns_xspi->flash_type != CDNS_XSPI_FLASH_TYPE_NAND)
> +		goto use_stig;
> +
> +	spinand = spi_mem_get_drvdata(mem);

This appears to be peering into the child's driver data without any
checking that the child is what we expect, this could go horribly wrong.

> +	case CDNS_XSPI_NAND_OP_GET_FEATURE:
> +		if (op->addr.val != CDNS_XSPI_NAND_STATUS_REG ||
> +		    !cdns_xspi->acmd_info.row_addr_valid)
> +			break;
> +
> +		if (op->data.dir != SPI_MEM_DATA_IN || !op->data.nbytes ||
> +		    !op->data.buf.in)
> +			return -EINVAL;
> +
> +		memset(op->data.buf.in, 0, op->data.nbytes);
> +		return 0;

Are you sure the zeroing makes sense here, for example with ECC?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]