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

[Nuno Sá]

On Mon, Sep 21, 2026 at 04:51:59PM +0200, Mark Brown wrote:
> 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?

So the controller can act as a DMA controller and issue DMA transfers
directly to the above mapping. However what I'm seeing is that the above
is pretty much a bounce buffer and needs details from nand chip. Plus,
as you put it, we can have two different flashes attached and then what?

So what I have (and I do not see any other way tbh) is an on demand
dma_mapping for the buffers we get from the spi_mem (which should be
dma_safe yes) core and so, no memcpy() at all. Yeah, we have the
underlying cache maintenance these mappings require but as said, don't
see any other way.

- Nuno Sá

> 
> > +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?