Re: [PATCH RFC v3 6/9] spi: axi-spi-engine: implement offload support

[David Lechner <dlechner@xxxxxxxxxxxx>]

On 7/23/24 3:01 AM, Nuno Sá wrote:
> On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
>> This implements SPI offload support for the AXI SPI Engine. Currently,
>> the hardware only supports triggering offload transfers with a hardware
>> trigger so attempting to use an offload message in the regular SPI
>> message queue will fail. Also, only allows streaming rx data to an
>> external sink, so attempts to use a rx_buf in the offload message will
>> fail.
>>
>> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
>> ---
>>
> 
> ...
> 
> 
> I'm likely missing something but you already have:
> 
> priv = &spi_engine->offload_priv[args[0]];
> 
> which seems that from FW you already got the offload index you need. Can't we
> just save that index in struct spi_device and use that directly in the other
> operations? Saving the trouble to save the id string and having to always call 
> spi_engine_get_offload()?

Saving the index in the struct spi_device would assume 1. that all SPI
peripherals can only use one SPI offload instance and 2. that all SPI
offload providers have #spi-offload-cells = <1> where the cell is the
index. I don't think either of these are safe assumptions.

> 
>> +
>>
> 
> ...
> 
>> +}
>> +
>> +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
>> *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv)) {
>> +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
>> +		return;
>> +	}
>> +
>> +	writel_relaxed(1, spi_engine->base +
>> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
>> +	writel_relaxed(0, spi_engine->base +
>> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
>> +
>> +	priv->prepared = false;
>> +}
>> +
>> +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
>> +					     const char *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num, reg;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv))
>> +		return PTR_ERR(priv);
>> +
>> +	reg = readl_relaxed(spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>> +	writel_relaxed(reg, spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +
>> +	return 0;
>> +}
>> +
>> +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
>> +					       const char *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num, reg;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv)) {
>> +		dev_warn(&spi->dev, "failed match offload in disable\n");
>> +		return;
>> +	}
>> +
>> +	reg = readl_relaxed(spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>> +	writel_relaxed(reg, spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +}
>> +
> 
> I would expect for the enable/disable() operations to act on the trigger. In
> this case to enable/disable the clock...

I'm not opposed to doing that, but things would get more complicated if we
ever added more trigger types. Because then we would need to add some kind
of trigger device abstraction to wrap the enable and disable functions of
the various triggers.

It seems simpler to me to have the peripheral driver do it since it already
needs to get the clock device for other reasons anyway.

But I also got some internal feedback that it might make more sense to add
a trigger abstraction layer, so maybe that is something we should look into
more.