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

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Tue, 2024-07-23 at 09:19 -0500, David Lechner wrote:
> 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.
> 

Ok, I see what you mean. I guess I just don't like too much of that *id in all over
the place. But we may anyways have to come up with some kind of offload abstraction.

> > 
> > > +
> > > 
> > 
> > ...
> > 
> > > +}
> > > +
> > > +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.
> 

Yeah, to me is about symmetry... I'm of the opinion that consumers, ideally, would
not have to know about the type of the trigger. Just that we have a trigger and then
have an interface for what can we do with it. The one that needs to know about the
type is the controller driver proving offload capabilities. I guess we can have one
DT cell to specify the type of the trigger.

> 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.

Nice. I admit I did not though too much on an actual implementation so I'm not really
sure how feasible this is without getting overly complicated. But from a conceptual
point of view, it looks the right thing to me.

- Nuno Sá