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á