Re: [PATCH v2 4/6] spi: axi-spi-engine: support SPI_MULTI_BUS_MODE_STRIPE

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi David,

The updates to spi-engine driver look good.
Only one comment about what happens if we have conflicting bus modes for the
offload case. Just to check I'm getting how this is working.

On 11/07, David Lechner wrote:
> Add support for SPI_MULTI_BUS_MODE_STRIPE to the AXI SPI engine driver.
> 
> The v2.0.0 version of the AXI SPI Engine IP core supports multiple
> buses. This can be used with SPI_MULTI_BUS_MODE_STRIPE to support
> reading from simultaneous sampling ADCs that have a separate SDO line
> for each analog channel. This allows reading all channels at the same
> time to increase throughput.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> v2 changes:
> * Fixed off-by-one in SPI_ENGINE_REG_DATA_WIDTH_NUM_OF_SDIO_MASK GENMASK
> ---
>  drivers/spi/spi-axi-spi-engine.c | 128 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 124 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> index e06f412190fd243161a0b3df992f26157531f6a1..c9d146e978b89abb8273900722ae2cfafdd6325f 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -23,6 +23,9 @@
>  #include <linux/spi/spi.h>
>  #include <trace/events/spi.h>
>  
> +#define SPI_ENGINE_REG_DATA_WIDTH		0x0C
> +#define   SPI_ENGINE_REG_DATA_WIDTH_NUM_OF_SDIO_MASK	GENMASK(23, 16)
> +#define   SPI_ENGINE_REG_DATA_WIDTH_MASK		GENMASK(15, 0)
>  #define SPI_ENGINE_REG_OFFLOAD_MEM_ADDR_WIDTH	0x10
>  #define SPI_ENGINE_REG_RESET			0x40
>  
> @@ -75,6 +78,8 @@
>  #define SPI_ENGINE_CMD_REG_CLK_DIV		0x0
>  #define SPI_ENGINE_CMD_REG_CONFIG		0x1
>  #define SPI_ENGINE_CMD_REG_XFER_BITS		0x2
> +#define SPI_ENGINE_CMD_REG_SDI_MASK		0x3
> +#define SPI_ENGINE_CMD_REG_SDO_MASK		0x4
>  
>  #define SPI_ENGINE_MISC_SYNC			0x0
>  #define SPI_ENGINE_MISC_SLEEP			0x1
> @@ -105,6 +110,10 @@
>  #define SPI_ENGINE_OFFLOAD_CMD_FIFO_SIZE	16
>  #define SPI_ENGINE_OFFLOAD_SDO_FIFO_SIZE	16
>  
> +/* Extending SPI_MULTI_BUS_MODE values for optimizing messages. */
> +#define SPI_ENGINE_MULTI_BUS_MODE_UNKNOWN	-1
> +#define SPI_ENGINE_MULTI_BUS_MODE_CONFLICTING	-2
> +
>  struct spi_engine_program {
>  	unsigned int length;
>  	uint16_t instructions[] __counted_by(length);
> @@ -142,6 +151,9 @@ struct spi_engine_offload {
>  	unsigned long flags;
>  	unsigned int offload_num;
>  	unsigned int spi_mode_config;
> +	unsigned int multi_bus_mode;
> +	u8 primary_bus_mask;
> +	u8 all_bus_mask;
>  	u8 bits_per_word;
>  };
>  
> @@ -165,6 +177,22 @@ struct spi_engine {
>  	bool offload_requires_sync;
>  };
>  
> +static u8 spi_engine_primary_bus_flag(struct spi_device *spi)
> +{
> +	return BIT(spi->data_bus[0]);
> +}
> +
> +static u8 spi_engine_all_bus_flags(struct spi_device *spi)
> +{
> +	u8 flags = 0;
> +	int i;
> +
> +	for (i = 0; i < spi->num_data_bus; i++)
> +		flags |= BIT(spi->data_bus[i]);
> +
> +	return flags;
> +}
> +
>  static void spi_engine_program_add_cmd(struct spi_engine_program *p,
>  	bool dry, uint16_t cmd)
>  {
> @@ -193,7 +221,7 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
>  }
>  
>  static void spi_engine_gen_xfer(struct spi_engine_program *p, bool dry,
> -	struct spi_transfer *xfer)
> +				struct spi_transfer *xfer, u32 num_lanes)
>  {
>  	unsigned int len;
>  
> @@ -204,6 +232,9 @@ static void spi_engine_gen_xfer(struct spi_engine_program *p, bool dry,
>  	else
>  		len = xfer->len / 4;
>  
> +	if (xfer->multi_bus_mode == SPI_MULTI_BUS_MODE_STRIPE)
> +		len /= num_lanes;
> +
>  	while (len) {
>  		unsigned int n = min(len, 256U);
>  		unsigned int flags = 0;
> @@ -269,6 +300,7 @@ static int spi_engine_precompile_message(struct spi_message *msg)
>  {
>  	unsigned int clk_div, max_hz = msg->spi->controller->max_speed_hz;
>  	struct spi_transfer *xfer;
> +	int multi_bus_mode = SPI_ENGINE_MULTI_BUS_MODE_UNKNOWN;
>  	u8 min_bits_per_word = U8_MAX;
>  	u8 max_bits_per_word = 0;
>  
> @@ -284,6 +316,24 @@ static int spi_engine_precompile_message(struct spi_message *msg)
>  			min_bits_per_word = min(min_bits_per_word, xfer->bits_per_word);
>  			max_bits_per_word = max(max_bits_per_word, xfer->bits_per_word);
>  		}
> +
> +		if (xfer->rx_buf || xfer->offload_flags & SPI_OFFLOAD_XFER_RX_STREAM ||
> +		    xfer->tx_buf || xfer->offload_flags & SPI_OFFLOAD_XFER_TX_STREAM) {
> +			switch (xfer->multi_bus_mode) {
> +			case SPI_MULTI_BUS_MODE_SINGLE:
> +			case SPI_MULTI_BUS_MODE_STRIPE:
> +				break;
> +			default:
> +				/* Other modes, like mirror not supported */
> +				return -EINVAL;
> +			}
> +
> +			/* If all xfers have the same multi-bus mode, we can optimize. */
> +			if (multi_bus_mode == SPI_ENGINE_MULTI_BUS_MODE_UNKNOWN)
> +				multi_bus_mode = xfer->multi_bus_mode;
> +			else if (multi_bus_mode != xfer->multi_bus_mode)
> +				multi_bus_mode = SPI_ENGINE_MULTI_BUS_MODE_CONFLICTING;

Here we check all xfers have the same multi-bus mode and keep the mode that has
been set. Otherwise, we set this conflicting mode and the intent is to generate
SDI and SDO mask commands on demand on spi_engine_precompile_message(). OTOH,
if all xfers have the same multi-bus mode, we can add just one pair of SDI/SDO
mask commands in spi_engine_trigger_enable() and one pair latter in
spi_engine_trigger_disable(). I guess this is the optimization mentioned in the
comment.

> +		}
>  	}
>  
>  	/*
> @@ -297,6 +347,10 @@ static int spi_engine_precompile_message(struct spi_message *msg)
>  			priv->bits_per_word = min_bits_per_word;
>  		else
>  			priv->bits_per_word = 0;
> +
> +		priv->multi_bus_mode = multi_bus_mode;
> +		priv->primary_bus_mask = spi_engine_primary_bus_flag(msg->spi);
> +		priv->all_bus_mask = spi_engine_all_bus_flags(msg->spi);
>  	}
>  
>  	return 0;
> @@ -310,6 +364,7 @@ static void spi_engine_compile_message(struct spi_message *msg, bool dry,
>  	struct spi_engine_offload *priv;
>  	struct spi_transfer *xfer;
>  	int clk_div, new_clk_div, inst_ns;
> +	int prev_multi_bus_mode = SPI_MULTI_BUS_MODE_SINGLE;
>  	bool keep_cs = false;
>  	u8 bits_per_word = 0;
>  
> @@ -334,6 +389,7 @@ static void spi_engine_compile_message(struct spi_message *msg, bool dry,
>  		 * in the same way.
>  		 */
>  		bits_per_word = priv->bits_per_word;
> +		prev_multi_bus_mode = priv->multi_bus_mode;
>  	} else {
>  		spi_engine_program_add_cmd(p, dry,
>  			SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_CONFIG,
> @@ -344,6 +400,24 @@ static void spi_engine_compile_message(struct spi_message *msg, bool dry,
>  	spi_engine_gen_cs(p, dry, spi, !xfer->cs_off);
>  
>  	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if (xfer->rx_buf || xfer->offload_flags & SPI_OFFLOAD_XFER_RX_STREAM ||
> +		    xfer->tx_buf || xfer->offload_flags & SPI_OFFLOAD_XFER_TX_STREAM) {
> +			if (xfer->multi_bus_mode != prev_multi_bus_mode) {
> +				u8 bus_flags = spi_engine_primary_bus_flag(spi);
> +
> +				if (xfer->multi_bus_mode == SPI_MULTI_BUS_MODE_STRIPE)
> +					bus_flags = spi_engine_all_bus_flags(spi);
> +
> +				spi_engine_program_add_cmd(p, dry,
> +					SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDI_MASK,
> +							     bus_flags));
> +				spi_engine_program_add_cmd(p, dry,
> +					SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDO_MASK,
> +							     bus_flags));
> +			}
> +			prev_multi_bus_mode = xfer->multi_bus_mode;
> +		}
> +
>  		new_clk_div = host->max_speed_hz / xfer->effective_speed_hz;
>  		if (new_clk_div != clk_div) {
>  			clk_div = new_clk_div;
> @@ -360,7 +434,7 @@ static void spi_engine_compile_message(struct spi_message *msg, bool dry,
>  					bits_per_word));
>  		}
>  
> -		spi_engine_gen_xfer(p, dry, xfer);
> +		spi_engine_gen_xfer(p, dry, xfer, spi->num_data_bus);
>  		spi_engine_gen_sleep(p, dry, spi_delay_to_ns(&xfer->delay, xfer),
>  				     inst_ns, xfer->effective_speed_hz);
>  
> @@ -394,6 +468,17 @@ static void spi_engine_compile_message(struct spi_message *msg, bool dry,
>  	if (clk_div != 1)
>  		spi_engine_program_add_cmd(p, dry,
>  			SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_CLK_DIV, 0));
> +
> +	/* Restore single bus mode unless offload disable will restore it later. */
> +	if (prev_multi_bus_mode == SPI_MULTI_BUS_MODE_STRIPE &&
> +	    (!msg->offload || priv->multi_bus_mode != SPI_MULTI_BUS_MODE_STRIPE)) {
> +		u8 bus_flags = spi_engine_primary_bus_flag(spi);
> +
> +		spi_engine_program_add_cmd(p, dry,
> +			SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDI_MASK, bus_flags));
> +		spi_engine_program_add_cmd(p, dry,
> +			SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDO_MASK, bus_flags));
> +	}
>  }
>  
>  static void spi_engine_xfer_next(struct spi_message *msg,
> @@ -799,6 +884,17 @@ static int spi_engine_setup(struct spi_device *device)
>  	writel_relaxed(SPI_ENGINE_CMD_CS_INV(spi_engine->cs_inv),
>  		       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
>  
> +	if (host->num_data_bus > 1) {
> +		u8 bus_flags = spi_engine_primary_bus_flag(device);
> +
> +		writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDI_MASK,
> +						    bus_flags),
> +			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> +		writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDO_MASK,
> +						    bus_flags),
> +			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> +	}
> +
>  	/*
>  	 * In addition to setting the flags, we have to do a CS assert command
>  	 * to make the new setting actually take effect.
> @@ -902,6 +998,15 @@ static int spi_engine_trigger_enable(struct spi_offload *offload)
>  						    priv->bits_per_word),
>  			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
>  
> +	if (priv->multi_bus_mode == SPI_MULTI_BUS_MODE_STRIPE) {
> +		writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDI_MASK,
> +						    priv->all_bus_mask),
> +			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> +		writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDO_MASK,
> +						    priv->all_bus_mask),
> +			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> +	}
> +
>  	writel_relaxed(SPI_ENGINE_CMD_SYNC(1),
>  		spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
>  
> @@ -929,6 +1034,16 @@ static void spi_engine_trigger_disable(struct spi_offload *offload)
>  	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>  	writel_relaxed(reg, spi_engine->base +
>  			    SPI_ENGINE_REG_OFFLOAD_CTRL(priv->offload_num));
> +
> +	/* Restore single-bus mode. */
> +	if (priv->multi_bus_mode == SPI_MULTI_BUS_MODE_STRIPE) {
> +		writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDI_MASK,
> +						    priv->primary_bus_mask),
> +			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> +		writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_SDO_MASK,
> +						    priv->primary_bus_mask),
> +			       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> +	}
>  }
>  
>  static struct dma_chan
> @@ -973,7 +1088,7 @@ static int spi_engine_probe(struct platform_device *pdev)
>  {
>  	struct spi_engine *spi_engine;
>  	struct spi_controller *host;
> -	unsigned int version;
> +	unsigned int version, data_width_reg_val;
>  	int irq, ret;
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -1042,7 +1157,7 @@ static int spi_engine_probe(struct platform_device *pdev)
>  		return PTR_ERR(spi_engine->base);
>  
>  	version = readl(spi_engine->base + ADI_AXI_REG_VERSION);
> -	if (ADI_AXI_PCORE_VER_MAJOR(version) != 1) {
> +	if (ADI_AXI_PCORE_VER_MAJOR(version) > 2) {
>  		dev_err(&pdev->dev, "Unsupported peripheral version %u.%u.%u\n",
>  			ADI_AXI_PCORE_VER_MAJOR(version),
>  			ADI_AXI_PCORE_VER_MINOR(version),
> @@ -1050,6 +1165,8 @@ static int spi_engine_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	data_width_reg_val = readl(spi_engine->base + SPI_ENGINE_REG_DATA_WIDTH);
> +
>  	if (adi_axi_pcore_ver_gteq(version, 1, 1)) {
>  		unsigned int sizes = readl(spi_engine->base +
>  				SPI_ENGINE_REG_OFFLOAD_MEM_ADDR_WIDTH);
> @@ -1097,6 +1214,9 @@ static int spi_engine_probe(struct platform_device *pdev)
>  	}
>  	if (adi_axi_pcore_ver_gteq(version, 1, 3))
>  		host->mode_bits |= SPI_MOSI_IDLE_LOW | SPI_MOSI_IDLE_HIGH;
> +	if (adi_axi_pcore_ver_gteq(version, 2, 0))
> +		host->num_data_bus = FIELD_GET(SPI_ENGINE_REG_DATA_WIDTH_NUM_OF_SDIO_MASK,
> +					       data_width_reg_val);
>  
>  	if (host->max_speed_hz == 0)
>  		return dev_err_probe(&pdev->dev, -EINVAL, "spi_clk rate is 0");
> 
> -- 
> 2.43.0
> 
>