| Message ID | 20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com |
|---|---|
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
Hi David, On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > There is a recap at the end of this cover letter for those not familiar > with the previous discussions. For those that are, we'll get right to > the changes since the last version. > > In RFC v2, most of the discussion was around the DT bindings, so that > is what has mostly changed since then. I think we mostly settled on > what properties are needed and where they should go. There are probably > still some details to work out (see PATCH 5/9 for more discussion) but > I think we have the big-picture stuff figured out. > > Here is the actual devicetree used for testing to show how it all > comes together: > > trigger_clk: adc-trigger-clock { > compatible = "pwm-clock"; > #clock-cells = <0>; > #trigger-source-cells = <0>; > pwms = <&adc_trigger 0 10000>; > }; > > ... > > axi_spi_engine_0: spi@44a00000 { > compatible = "adi,axi-spi-engine-1.00.a"; > reg = <0x44a00000 0x1000>; > interrupt-parent = <&intc>; > interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clkc 15>, <&spi_clk>; > clock-names = "s_axi_aclk", "spi_clk"; > > /* offload-specific properties */ > #spi-offload-cells = <1>; > dmas = <&rx_dma 0>; > dma-names = "offload0-rx"; > trigger-sources = <&trigger_clk>; > > #address-cells = <1>; > #size-cells = <0>; > > ad7986: adc@0 { > compatible = "adi,ad7986"; > reg = <0>; > spi-max-frequency = <111111111>; /* 9 ns period */ > adi,spi-mode = "single"; > avdd-supply = <&eval_u12>; > dvdd-supply = <&eval_u12>; > vio-supply = <&eval_u3>; > bvdd-supply = <&eval_u10>; > ref-supply = <&eval_u5>; > turbo-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>; > > spi-offloads = <&axi_spi_engine_0 0>; > }; > }; > > A working branch complete with extra hacks can be found at [1]. > > Also, I took a detour looking into what it would take to get Martin > Sperl's Raspberry Pi DMA offload proof-of-concept [2] updated to work > with this. This way we could have a second user to help guide the > design process. Given all of the SPI hardware quirks on that platform > and the unsolved technical issues, like how to get accurate time delays > and how to work around the 32-bit DMA word limitation, it would be more > work than I have time for (at least without someone sponsoring the work). > > [1]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v3 > [2]: > https://github.com/msperl/spi-bcm2835/blob/refactor_dmachain_for_prepared_messages/spi-bcm2835dma.c > > --- > Changes in v3: > - See individual patches for more detailed changes. > - Reworked DT bindings to have things physically connected to the SPI > controller be properties of the SPI controller and use more > conventional provider/consumer properties. > - Added more SPI APIs for peripheral drivers to use to get auxillary > offload resources, like triggers. > - Link to v2: > https://lore.kernel.org/r/20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com > > --- > > As a recap, here is the background and end goal of this series: > > The AXI SPI Engine is a SPI controller that has the ability to record a > series of SPI transactions and then play them back using a hardware > trigger. This allows operations to be performed, repeating many times, > without any CPU intervention. This is needed for achieving high data > rates (millions of samples per second) from ADCs and DACs that are > connected via a SPI bus. > > The offload hardware interface consists of a trigger input and a data > output for the RX data. These are connected to other hardware external > to the SPI controller. > > To record one or more transactions, commands and TX data are written > to memories in the controller (RX buffer is not used since RX data gets > streamed to an external sink). This sequence of transactions can then be > played back when the trigger input is asserted. > > This series includes core SPI support along with the first SPI > controller (AXI SPI Engine) and SPI peripheral (AD7944 ADC) that use > them. This enables capturing analog data at 2 million samples per > second. > > The hardware setup looks like this: > > +-------------------------------+ +------------------+ > > | | | > > SOC/FPGA | | AD7944 ADC | > > +---------------------+ | | | > > | AXI SPI Engine | | | | > > | SPI Bus ============ SPI Bus | > > | | | | | > > | +---------------+ | | | | > > | | Offload 0 | | | +------------------+ > > | | RX DATA OUT > > > > | > > | | TRIGGER IN < < < v | > > | +---------------+ | ^ v | > > +---------------------+ ^ v | > > | AXI PWM | ^ v | > > | CH0 > ^ v | > > +---------------------+ v | > > | AXI DMA | v | > > | CH0 < < < | > > +---------------------+ | > > | > +-------------------------------+ > > To: Mark Brown <broonie@kernel.org> > To: Jonathan Cameron <jic23@kernel.org> > To: Rob Herring <robh@kernel.org> > To: Krzysztof Kozlowski <krzk+dt@kernel.org> > To: Conor Dooley <conor+dt@kernel.org> > To: Nuno Sá <nuno.sa@analog.com> > Cc: Michael Hennerich <Michael.Hennerich@analog.com> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: David Jander <david@protonic.nl> > Cc: Martin Sperl <kernel@martin.sperl.org> > Cc: <linux-spi@vger.kernel.org> > Cc: <devicetree@vger.kernel.org> > Cc: <linux-kernel@vger.kernel.org> > Cc: <linux-iio@vger.kernel.org> > > --- > I think there are things that we need to better figure but things are improving IMO :) I'm only doing a very superficial review since I need to better look at the patches... But one thing that I do want to mention is a scenario (another funny one...) that I've discussing and that might be a reality. Something like: +-------------------------------+ +------------------+ | | | | | SOC/FPGA | | ADC | | | | | | +---------------+ | | | | | SPI PS Zynq |============== SPI Bus | | +---------------+ | | | | | | | | +---------------------+ | | | | | AXI SPI Engine | | | | | | v================ DATA Bus | | | v | | | | | | +---------------+ | | | | | | | Offload 0 | | | +------------------+ | | | RX DATA OUT | | | | | | TRIGGER IN | | | | | +---------------+ | | | | +-------------------------------+ From the above, the spi controller for typical register access/configuration is not the spi_enigine and the offload core is pretty much only used for streaming data. So I think your current approach would not work with this usecase. In your first RFC you had something overly complicated (IMHO) but you already had a concept that maybe it's worth looking at again. I mean having a spi_offload object that could describe it and more importantly have a provider/consumer logic where a spi consumer (or maybe even something else?) can get()/put() an offload object to stream data. I know, I did said that I did not liked for spi consumers to have to explicitly call something like spi_offload_get() but I guess I have been proved wrong :). We can also try to be smart about it as an explicit get is only needed (likely) in the above scenario (or maybe we can even do it directly in the spi core during spi_probe()). Or maybe it's not worth it to play smart and just let consumers do it (that's the typical pattern anyways). Having said the above, I still think your current proposal for triggers and getting DMA streams is valid for the above usecase. FWIW, I'm also trying to understand with the HW guys why the hell can't we just use the spi_engine controller for everything. But the whole discussion is already showing us that we may need more flexibility. Thanks! - Nuno Sá
On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > SPI offloading is a feature that allows the SPI controller to perform > transfers without any CPU intervention. This is useful, e.g. for > high-speed data acquisition. > > This patch adds the basic infrastructure to support SPI offloading. It > introduces new callbacks that are to be implemented by controllers with > offload capabilities. > > On SPI device probe, the standard spi-offloads devicetree property is > parsed and passed to the controller driver to reserve the resources > requested by the peripheral via the map_channel() callback. > > The peripheral driver can then use spi_offload_prepare() to load a SPI > message into the offload hardware. > > If the controller supports it, this message can then be passed to the > SPI message queue as if it was a normal message. Future patches will > will also implement a way to use a hardware trigger to start the message > transfers rather than going through the message queue. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v3 changes: > * Minor changes to doc comments. > * Changed to use phandle array for spi-offloads. > * Changed id to string to make use of spi-offload-names. > > v2 changes: > > This is a rework of "spi: add core support for controllers with offload > capabilities" from v1. > > The spi_offload_get() function that Nuno didn't like is gone. Instead, > there is now a mapping callback that uses the new generic devicetree > binding to request resources automatically when a SPI device is probed. > Given my reply to the cover you can start calling me names already :). But even with that function back I think we need a more explicit provider/consumer logic. > The spi_offload_enable/disable() functions for dealing with hardware > triggers are deferred to a separate patch. > > This leaves adding spi_offload_prepare/unprepare() which have been > reworked to be a bit more robust. > > In the previous review, Mark suggested that these functions should not > be separate from the spi_[un]optimize() functions. I understand the > reasoning behind that. However, it seems like there are two different > kinds of things going on here. Currently, spi_optimize() only performs > operations on the message data structures and doesn't poke any hardware. > This makes it free to be use by any peripheral without worrying about > tying up any hardware resources while the message is "optimized". On the > other hand, spi_offload_prepare() is poking hardware, so we need to be > more careful about how it is used. And in these cases, we need a way to > specify exactly which hardware resources it should use, which it is > currently doing with the extra ID parameter. > --- > drivers/spi/spi.c | 123 > ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi.h | 57 ++++++++++++++++++++++ > 2 files changed, 180 insertions(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index d4da5464dbd0..d01b2e5c8c44 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -2477,6 +2477,51 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, > struct spi_device *spi, > of_spi_parse_dt_cs_delay(nc, &spi->cs_hold, "spi-cs-hold-delay-ns"); > of_spi_parse_dt_cs_delay(nc, &spi->cs_inactive, "spi-cs-inactive- > delay-ns"); > > + /* Offloads */ > + rc = of_count_phandle_with_args(nc, "spi-offloads", "#spi-offload- > cells"); > + if (rc > 0) { > + int num_offload = rc; > + > + if (!ctlr->offload_ops) { > + dev_err(&ctlr->dev, "SPI controller doesn't support > offloading\n"); > + return -EINVAL; > + } > + > + for (idx = 0; idx < num_offload; idx++) { > + struct of_phandle_args args; > + const char *offload_name = NULL; > + > + rc = of_parse_phandle_with_args(nc, "spi-offloads", > + "#spi-offload-cells", > + idx, &args); > + if (rc) { > + dev_err(&spi->dev, "Failed to parse offload > phandle %d: %d\n", > + idx, rc); > + return rc; > + } > + > + if (args.np != ctlr->dev.of_node) { > + dev_err(&spi->dev, "Offload phandle %d is not > for this SPI controller\n", > + idx); > + of_node_put(args.np); > + return -EINVAL; > + } > + > + of_property_read_string_index(nc, "spi-offload- > names", > + idx, &offload_name); > + > + rc = ctlr->offload_ops->map_channel(spi, > offload_name, > + args.args, > + args.args_count); In here, I would expect for the mapping to return something the core could then directly pass into the other operations. And hence saving controllers to always have to do a lookup in all the operations. It seems we may need a struct spi_offload * object that can be attached to a given spi_device and that can be directly passed and used by the specific offload operations. - Nuno Sá
On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > This extends the SPI framework to support hardware triggered offloading. > This allows an arbitrary hardware trigger to be used to start a SPI > transfer that was previously set up with spi_offload_prepare(). > > Since the hardware trigger can happen at any time, this means the SPI > bus must be reserved for exclusive use as long as the hardware trigger > is enabled. Since a hardware trigger could be enabled indefinitely, > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, > otherwise this could cause deadlocks. So we introduce a new flag so that > any attempt to lock or use the bus will fail with -EBUSY as long as the > hardware trigger is enabled. > > Peripheral drivers may need to control the trigger source as well. For > this, we introduce a new spi_offload_hw_trigger_get_clk() function that > can be used to get a clock trigger source. This is intended for used > by ADC drivers that will use the clock to control the sample rate. > Additional functions to get other types of trigger sources could be > added in the future. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers > check the return value. All callers will need to be updated first before > this can be merged. > > v3 changes: > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > * added spi_offload_hw_trigger_get_clk() function > * fixed missing EXPORT_SYMBOL_GPL > > v2 changes: > > This is split out from "spi: add core support for controllers with > offload capabilities". > > Mark suggested that the standard SPI APIs should be aware that the > hardware trigger is enabled. So I've added some locking for this. Nuno > suggested that this might be overly strict though, and that we should > let each individual controller driver decide what to do. For our use > case though, I think we generally are going to have a single peripheral > on the SPI bus, so this seems like a reasonable starting place anyway. > --- How explicitly do we want to be about returning errors? It seems that if the trigger is enabled we can't anything else on the controller/offload_engine so we could very well just hold the controller lock when enabling the trigger and release it when disabling it. Pretty much the same behavior as spi_bus_lock()... ... > > + > +/** > + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger > + * @spi: SPI device > + * @id: Function ID if SPI device uses more than one offload or NULL. > + * > + * The caller is responsible for calling clk_put() on the returned clock. > + * > + * Return: The clock for the offload trigger, or negative error code > + */ > +static inline > +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char > *id) > +{ > + struct spi_controller *ctlr = spi->controller; > + > + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) > + return ERR_PTR(-EOPNOTSUPP); > + > + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); > +} > It would be nice if we could have some kind of spi abstraction... - Nuno Sá
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@baylibre.com> > --- > ... 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()? > + > ... > +} > + > +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... - Nuno Sá
On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > This adds support for SPI offload to the ad7944 driver. This allows > reading data at the max sample rate of 2.5 MSPS. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > ... > +static void ad7944_put_clk_trigger(void *p) > +{ > + clk_put(p); > +} > + I think this means we may still need to improve the API a bit. This asymmetric handling is (to me) and indicator that something is not very well from a design perspective. What I mean is that if you get the clock through spi I would also expect to put() it through SPI. Now that I think about it that's also true for the DMA channel handling but in there things are a bit more complicated. I mean, at least you're making this explicit in the docs so maybe it's acceptable. But it stills feels strange to me that the place where the resources are requested and bound too is not the same one responsible for releasing them. If we go with the provider/consumer approach and having a properly refcounted spi_offload object I think we may be able to do it from the offload object context. Maybe not worth it though... Not sure tbh. - Nuno Sá
On Mon, Jul 22, 2024 at 04:57:07PM -0500, David Lechner wrote: > There is a recap at the end of this cover letter for those not familiar > with the previous discussions. For those that are, we'll get right to > the changes since the last version. > > In RFC v2, most of the discussion was around the DT bindings, so that > is what has mostly changed since then. I think we mostly settled on > what properties are needed and where they should go. There are probably > still some details to work out (see PATCH 5/9 for more discussion) but > I think we have the big-picture stuff figured out. Thanks for the updates. I'm on holiday until rc2, so it'll not be until then that I can really take a look at this. Figured I'd let you know rather than just ignore you...
On 7/23/24 2:35 AM, Nuno Sá wrote: > Hi David, > > > I think there are things that we need to better figure but things are improving > IMO :) > > I'm only doing a very superficial review since I need to better look at the > patches... > > But one thing that I do want to mention is a scenario (another funny one...) > that I've discussing and that might be a reality. Something like: > > +-------------------------------+ +------------------+ > | | | | > | SOC/FPGA | | ADC | > | | | | > | +---------------+ | | | > | | SPI PS Zynq |============== SPI Bus | > | +---------------+ | | | > | | | | > | +---------------------+ | | | > | | AXI SPI Engine | | | | > | | v================ DATA Bus | > | | v | | | | > | | +---------------+ | | | | > | | | Offload 0 | | | +------------------+ > | | | RX DATA OUT | | | > | | | TRIGGER IN | | | > | | +---------------+ | | > | | > +-------------------------------+ > > From the above, the spi controller for typical register access/configuration is > not the spi_enigine and the offload core is pretty much only used for streaming > data. So I think your current approach would not work with this usecase. In your > first RFC you had something overly complicated (IMHO) but you already had a > concept that maybe it's worth looking at again. I mean having a spi_offload > object that could describe it and more importantly have a provider/consumer > logic where a spi consumer (or maybe even something else?) can get()/put() an > offload object to stream data. Although it isn't currently implemented this way in the core SPI code, I think the DT bindings proposed in this version of the series would allow for this. The offload provider is just the one with the #spi-offload-cells and doesn't necessarily have to be the parent of the SPI peripheral. > > I know, I did said that I did not liked for spi consumers to have to explicitly > call something like spi_offload_get() but I guess I have been proved wrong :). > We can also try to be smart about it as an explicit get is only needed (likely) > in the above scenario (or maybe we can even do it directly in the spi core > during spi_probe()). Or maybe it's not worth it to play smart and just let > consumers do it (that's the typical pattern anyways). > > Having said the above, I still think your current proposal for triggers and > getting DMA streams is valid for the above usecase. > > FWIW, I'm also trying to understand with the HW guys why the hell can't we just > use the spi_engine controller for everything. But the whole discussion is > already showing us that we may need more flexibility. > > Thanks! > - Nuno Sá
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@baylibre.com> >> --- >> > > ... > > > 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.
On 7/23/24 2:53 AM, Nuno Sá wrote: > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: >> This extends the SPI framework to support hardware triggered offloading. >> This allows an arbitrary hardware trigger to be used to start a SPI >> transfer that was previously set up with spi_offload_prepare(). >> >> Since the hardware trigger can happen at any time, this means the SPI >> bus must be reserved for exclusive use as long as the hardware trigger >> is enabled. Since a hardware trigger could be enabled indefinitely, >> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, >> otherwise this could cause deadlocks. So we introduce a new flag so that >> any attempt to lock or use the bus will fail with -EBUSY as long as the >> hardware trigger is enabled. >> >> Peripheral drivers may need to control the trigger source as well. For >> this, we introduce a new spi_offload_hw_trigger_get_clk() function that >> can be used to get a clock trigger source. This is intended for used >> by ADC drivers that will use the clock to control the sample rate. >> Additional functions to get other types of trigger sources could be >> added in the future. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> >> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers >> check the return value. All callers will need to be updated first before >> this can be merged. >> >> v3 changes: >> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... >> * added spi_offload_hw_trigger_get_clk() function >> * fixed missing EXPORT_SYMBOL_GPL >> >> v2 changes: >> >> This is split out from "spi: add core support for controllers with >> offload capabilities". >> >> Mark suggested that the standard SPI APIs should be aware that the >> hardware trigger is enabled. So I've added some locking for this. Nuno >> suggested that this might be overly strict though, and that we should >> let each individual controller driver decide what to do. For our use >> case though, I think we generally are going to have a single peripheral >> on the SPI bus, so this seems like a reasonable starting place anyway. >> --- > > How explicitly do we want to be about returning errors? It seems that if the > trigger is enabled we can't anything else on the controller/offload_engine so we > could very well just hold the controller lock when enabling the trigger and > release it when disabling it. Pretty much the same behavior as spi_bus_lock()... The problem I see with using spi_bus_lock() in it's current form is that SPI offload triggers could be enabled indefinitely. So any other devices on the bus that tried to use the bus and take the lock would essentially deadlock waiting for the offload user to release the lock. This is why I added the -BUSY return, to avoid this deadlock. > > ... > >> >> + >> +/** >> + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger >> + * @spi: SPI device >> + * @id: Function ID if SPI device uses more than one offload or NULL. >> + * >> + * The caller is responsible for calling clk_put() on the returned clock. >> + * >> + * Return: The clock for the offload trigger, or negative error code >> + */ >> +static inline >> +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char >> *id) >> +{ >> + struct spi_controller *ctlr = spi->controller; >> + >> + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); >> +} >> > > It would be nice if we could have some kind of spi abstraction... After reading your other replies, I think I understand what you mean here. Are you thinking some kind of `struct spi_offload_trigger` that could be any kind of trigger (clk, gpio, etc.)?
On Tue, 2024-07-23 at 09:30 -0500, David Lechner wrote: > On 7/23/24 2:53 AM, Nuno Sá wrote: > > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > > > This extends the SPI framework to support hardware triggered offloading. > > > This allows an arbitrary hardware trigger to be used to start a SPI > > > transfer that was previously set up with spi_offload_prepare(). > > > > > > Since the hardware trigger can happen at any time, this means the SPI > > > bus must be reserved for exclusive use as long as the hardware trigger > > > is enabled. Since a hardware trigger could be enabled indefinitely, > > > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, > > > otherwise this could cause deadlocks. So we introduce a new flag so that > > > any attempt to lock or use the bus will fail with -EBUSY as long as the > > > hardware trigger is enabled. > > > > > > Peripheral drivers may need to control the trigger source as well. For > > > this, we introduce a new spi_offload_hw_trigger_get_clk() function that > > > can be used to get a clock trigger source. This is intended for used > > > by ADC drivers that will use the clock to control the sample rate. > > > Additional functions to get other types of trigger sources could be > > > added in the future. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > --- > > > > > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers > > > check the return value. All callers will need to be updated first before > > > this can be merged. > > > > > > v3 changes: > > > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > > > * added spi_offload_hw_trigger_get_clk() function > > > * fixed missing EXPORT_SYMBOL_GPL > > > > > > v2 changes: > > > > > > This is split out from "spi: add core support for controllers with > > > offload capabilities". > > > > > > Mark suggested that the standard SPI APIs should be aware that the > > > hardware trigger is enabled. So I've added some locking for this. Nuno > > > suggested that this might be overly strict though, and that we should > > > let each individual controller driver decide what to do. For our use > > > case though, I think we generally are going to have a single peripheral > > > on the SPI bus, so this seems like a reasonable starting place anyway. > > > --- > > > > How explicitly do we want to be about returning errors? It seems that if the > > trigger is enabled we can't anything else on the controller/offload_engine so we > > could very well just hold the controller lock when enabling the trigger and > > release it when disabling it. Pretty much the same behavior as spi_bus_lock()... > > The problem I see with using spi_bus_lock() in it's current form is that > SPI offload triggers could be enabled indefinitely. So any other devices > on the bus that tried to use the bus and take the lock would essentially > deadlock waiting for the offload user to release the lock. This is why > I added the -BUSY return, to avoid this deadlock. > If someone does not disable the trigger at some point that's a bug. If I understood things correctly, even if someone tries to access the bus will get -EBUSY. But yeah, arguably it's better to get a valid error rather than blocking/sleeping > > > > ... > > > > > > > > + > > > +/** > > > + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger > > > + * @spi: SPI device > > > + * @id: Function ID if SPI device uses more than one offload or NULL. > > > + * > > > + * The caller is responsible for calling clk_put() on the returned clock. > > > + * > > > + * Return: The clock for the offload trigger, or negative error code > > > + */ > > > +static inline > > > +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char > > > *id) > > > +{ > > > + struct spi_controller *ctlr = spi->controller; > > > + > > > + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) > > > + return ERR_PTR(-EOPNOTSUPP); > > > + > > > + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); > > > +} > > > > > > > It would be nice if we could have some kind of spi abstraction... > > After reading your other replies, I think I understand what you mean here. > > Are you thinking some kind of `struct spi_offload_trigger` that could be > any kind of trigger (clk, gpio, etc.)? > Yeah, something in that direction... - Nuno Sá
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@baylibre.com> > > > --- > > > > > > > ... > > > > > > 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á
On Tue, 2024-07-23 at 08:48 -0500, David Lechner wrote: > On 7/23/24 2:35 AM, Nuno Sá wrote: > > Hi David, > > > > > > I think there are things that we need to better figure but things are improving > > IMO :) > > > > I'm only doing a very superficial review since I need to better look at the > > patches... > > > > But one thing that I do want to mention is a scenario (another funny one...) > > that I've discussing and that might be a reality. Something like: > > > > +-------------------------------+ +------------------+ > > > | | | > > > SOC/FPGA | | ADC | > > > | | | > > > +---------------+ | | | > > > | SPI PS Zynq |============== SPI Bus | > > > +---------------+ | | | > > > | | | > > > +---------------------+ | | | > > > | AXI SPI Engine | | | | > > > | v================ DATA Bus | > > > | v | | | | > > > | +---------------+ | | | | > > > | | Offload 0 | | | +------------------+ > > > | | RX DATA OUT | | | > > > | | TRIGGER IN | | | > > > | +---------------+ | | > > > | > > +-------------------------------+ > > > > From the above, the spi controller for typical register access/configuration is > > not the spi_enigine and the offload core is pretty much only used for streaming > > data. So I think your current approach would not work with this usecase. In your > > first RFC you had something overly complicated (IMHO) but you already had a > > concept that maybe it's worth looking at again. I mean having a spi_offload > > object that could describe it and more importantly have a provider/consumer > > logic where a spi consumer (or maybe even something else?) can get()/put() an > > offload object to stream data. > > Although it isn't currently implemented this way in the core SPI code, I think > the DT bindings proposed in this version of the series would allow for this. > The offload provider is just the one with the #spi-offload-cells and doesn't > necessarily have to be the parent of the SPI peripheral. > I get that but the thing is that when the spi device consuming an offload service is under the same controller providing that service we have guarantees regarding lifetimes. In case the offload is another controller, those guarantees are not true anymore and we need to make sure to handle things correctly (like the controller going away under our feet). That's why a proper provider/consumer interface is needed and I think that's a significant shift from what we have now? Out of my head (the minimal interface): spi_controller_offload_register() - we may need a list of register offloads in the controller struct. (devm_)spi_offload_get() - and this one could return a nice struct spi_offload abstraction to pass to the other APIs spi_offload_put() Or for starters we may skip the registering in the core and have it per driver but if we assume more controlles will have this we might just make it common code already. Just some ideas... - Nuno Sá
On Mon, 22 Jul 2024 16:57:09 -0500 David Lechner <dlechner@baylibre.com> wrote: > SPI offloading is a feature that allows the SPI controller to perform > transfers without any CPU intervention. This is useful, e.g. for > high-speed data acquisition. > > This patch adds the basic infrastructure to support SPI offloading. It > introduces new callbacks that are to be implemented by controllers with > offload capabilities. > > On SPI device probe, the standard spi-offloads devicetree property is > parsed and passed to the controller driver to reserve the resources > requested by the peripheral via the map_channel() callback. > > The peripheral driver can then use spi_offload_prepare() to load a SPI > message into the offload hardware. > > If the controller supports it, this message can then be passed to the > SPI message queue as if it was a normal message. Future patches will > will also implement a way to use a hardware trigger to start the message > transfers rather than going through the message queue. > > Signed-off-by: David Lechner <dlechner@baylibre.com> A few trivial comments inline. J > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index d4da5464dbd0..d01b2e5c8c44 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -2477,6 +2477,51 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > of_spi_parse_dt_cs_delay(nc, &spi->cs_hold, "spi-cs-hold-delay-ns"); > of_spi_parse_dt_cs_delay(nc, &spi->cs_inactive, "spi-cs-inactive-delay-ns"); > > + /* Offloads */ Might be good to factor this out as a little utility function. > + rc = of_count_phandle_with_args(nc, "spi-offloads", "#spi-offload-cells"); > + if (rc > 0) { > + int num_offload = rc; > + > + if (!ctlr->offload_ops) { > + dev_err(&ctlr->dev, "SPI controller doesn't support offloading\n"); > + return -EINVAL; > + } > + > + for (idx = 0; idx < num_offload; idx++) { > + struct of_phandle_args args; > + const char *offload_name = NULL; > + > + rc = of_parse_phandle_with_args(nc, "spi-offloads", > + "#spi-offload-cells", > + idx, &args); > + if (rc) { > + dev_err(&spi->dev, "Failed to parse offload phandle %d: %d\n", > + idx, rc); > + return rc; > + } > + > + if (args.np != ctlr->dev.of_node) { > + dev_err(&spi->dev, "Offload phandle %d is not for this SPI controller\n", > + idx); > + of_node_put(args.np); > + return -EINVAL; > + } > + > + of_property_read_string_index(nc, "spi-offload-names", > + idx, &offload_name); Check for errors? If not, perhaps a comment on why not. > + > + rc = ctlr->offload_ops->map_channel(spi, offload_name, > + args.args, > + args.args_count); > + of_node_put(args.np); > + if (rc) { > + dev_err(&spi->dev, "Failed to map offload channel %d: %d\n", > + value, rc); > + return rc; > + } > + } > + } > + > return 0; > } ... > +/** > + * spi_offload_prepare - prepare offload hardware for a transfer > + * @spi: The spi device to use for the transfers. > + * @id: Function ID if SPI device uses more than one offload or NULL. > + * @msg: The SPI message to use for the offload operation. > + * > + * Requests an offload instance with the specified ID and programs it with the > + * provided message. > + * > + * The message must not be pre-optimized (do not call spi_optimize_message() on > + * the message). > + * > + * Calls must be balanced with spi_offload_unprepare(). > + * > + * Return: 0 on success, else a negative error code. > + */ > +int spi_offload_prepare(struct spi_device *spi, const char *id, > + struct spi_message *msg) > +{ > + struct spi_controller *ctlr = spi->controller; > + int ret; > + > + if (!ctlr->offload_ops) > + return -EOPNOTSUPP; > + > + msg->offload = true; I'd set this later perhaps as... > + > + ret = spi_optimize_message(spi, msg); > + if (ret) It otherwise needs clearing here so it doesn't have side effects if an error occurs. > + return ret; > + > + mutex_lock(&ctlr->io_mutex); > + ret = ctlr->offload_ops->prepare(spi, id, msg); > + mutex_unlock(&ctlr->io_mutex); > + > + if (ret) { > + spi_unoptimize_message(msg); > + msg->offload = false; > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(spi_offload_prepare); ... > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index d7a16e0adf44..4998b48ea7fd 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -31,6 +31,7 @@ struct spi_transfer; ... > @@ -1122,6 +1127,7 @@ struct spi_transfer { > * @pre_optimized: peripheral driver pre-optimized the message > * @optimized: the message is in the optimized state > * @prepared: spi_prepare_message was called for the this message > + * @offload: message is to be used with offload hardware > * @status: zero for success, else negative errno > * @complete: called to report transaction completions > * @context: the argument to complete() when it's called > @@ -1131,6 +1137,7 @@ struct spi_transfer { > * @queue: for use by whichever driver currently owns the message > * @state: for use by whichever driver currently owns the message > * @opt_state: for use by whichever driver currently owns the message > + * @offload_state: for use by whichever driver currently owns the message > * @resources: for resource management when the SPI message is processed > * > * A @spi_message is used to execute an atomic sequence of data transfers, > @@ -1159,6 +1166,8 @@ struct spi_message { > > /* spi_prepare_message() was called for this message */ > bool prepared; > + /* spi_offload_prepare() was called on this message */ > + bool offload; offloaded? To match with prepared. > > /* > * REVISIT: we might want a flag affecting the behavior of the > @@ -1191,6 +1200,11 @@ struct spi_message { > * __spi_optimize_message() and __spi_unoptimize_message(). > */ > void *opt_state; > + /* > + * Optional state for use by controller driver between calls to > + * offload_ops->prepare() and offload_ops->unprepare(). > + */ > + void *offload_state; > > /* List of spi_res resources when the SPI message is processed */ > struct list_head resources; > @@ -1556,6 +1570,49 @@ static inline ssize_t spi_w8r16be(struct spi_device *spi, u8 cmd) > > /*---------------------------------------------------------------------------*/ > > +/* > + * Offloading support. > + * > + * Some SPI controllers support offloading of SPI transfers. Essentially, > + * this allows the SPI controller to record SPI transfers and then play them > + * back later in one go via a single trigger. > + */ > + > +/** > + * struct spi_controller_offload_ops - callbacks for offload support > + * > + * Drivers for hardware with offload support need to implement all of these > + * callbacks. > + */ > +struct spi_controller_offload_ops { > + /** > + * @map_channel: Required callback to reserve an offload instance for > + * the given SPI device. If a SPI device requires more than one instance, > + * then @id is used to differentiate between them. Channels must be > + * unmapped in the struct spi_controller::cleanup() callback. Probably a good idea to talk about possible return values as well. > + */ > + int (*map_channel)(struct spi_device *spi, const char *id, > + const unsigned int *args, unsigned int num_args);
On Mon, 22 Jul 2024 16:57:10 -0500 David Lechner <dlechner@baylibre.com> wrote: > This extends the SPI framework to support hardware triggered offloading. > This allows an arbitrary hardware trigger to be used to start a SPI > transfer that was previously set up with spi_offload_prepare(). > > Since the hardware trigger can happen at any time, this means the SPI > bus must be reserved for exclusive use as long as the hardware trigger > is enabled. Since a hardware trigger could be enabled indefinitely, > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, > otherwise this could cause deadlocks. So we introduce a new flag so that > any attempt to lock or use the bus will fail with -EBUSY as long as the > hardware trigger is enabled. > > Peripheral drivers may need to control the trigger source as well. For > this, we introduce a new spi_offload_hw_trigger_get_clk() function that > can be used to get a clock trigger source. This is intended for used > by ADC drivers that will use the clock to control the sample rate. > Additional functions to get other types of trigger sources could be > added in the future. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers > check the return value. All callers will need to be updated first before > this can be merged. If it's going to fail sometimes, probably needs a name that indicates that. I'm not sure spi_bus_try_lock() is appropriate though. > > v3 changes: > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > * added spi_offload_hw_trigger_get_clk() function > * fixed missing EXPORT_SYMBOL_GPL > > v2 changes: > > This is split out from "spi: add core support for controllers with > offload capabilities". > > Mark suggested that the standard SPI APIs should be aware that the > hardware trigger is enabled. So I've added some locking for this. Nuno > suggested that this might be overly strict though, and that we should > let each individual controller driver decide what to do. For our use > case though, I think we generally are going to have a single peripheral > on the SPI bus, so this seems like a reasonable starting place anyway. ... > +int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id) > +{ > + struct spi_controller *ctlr = spi->controller; > + unsigned long flags; > + int ret; > + > + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_enable) > + return -EOPNOTSUPP; > + > + mutex_lock(&ctlr->bus_lock_mutex); > + > + if (ctlr->offload_hw_trigger_mode_enabled) { > + mutex_unlock(&ctlr->bus_lock_mutex); > + return -EBUSY; > + } > + > + spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags); > + ctlr->offload_hw_trigger_mode_enabled = true; Why do you need to take the spinlock when setting this to true, but not when setting it to fast (in the error path below)? > + spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags); > + > + /* TODO: how to wait for empty message queue? */ > + > + mutex_lock(&ctlr->io_mutex); > + ret = ctlr->offload_ops->hw_trigger_mode_enable(spi, id); > + mutex_unlock(&ctlr->io_mutex); > + > + if (ret) { > + ctlr->offload_hw_trigger_mode_enabled = false; > + mutex_unlock(&ctlr->bus_lock_mutex); > + return ret; > + } > + > + mutex_unlock(&ctlr->bus_lock_mutex); > + > + return 0; > +} >
On Mon, 22 Jul 2024 16:57:14 -0500 David Lechner <dlechner@baylibre.com> wrote: > This patch generalizes the iio_dmaengine_buffer_setup_ext() functions > by passing the pointer to the DMA channel as an argument rather than > the channel name. This will allow future callers of the function to > use other methods to get the DMA channel pointer. > > This aims to keep it as easy to use as possible by stealing ownership > of the dma_chan pointer from the caller. This way, dma_request_chan() > can be called inline in the function call without any extra error > handling. That's odd enough to be a likely source of future bugs. Doesn't seem necessary to me. Just have the extra handling in the few places it's needed. Or add a wrapper for this case where you just provide the channel name as was done before this patch. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > v3 changes: > * This is a new patch in v3. ... > @@ -277,22 +282,26 @@ static void __devm_iio_dmaengine_buffer_free(void *buffer) > * devm_iio_dmaengine_buffer_setup_ext() - Setup a DMA buffer for an IIO device > * @dev: Parent device for the buffer > * @indio_dev: IIO device to which to attach this buffer. > - * @channel: DMA channel name, typically "rx". > + * @chan: DMA channel. > * @dir: Direction of buffer (in or out) > * > * This allocates a new IIO buffer with devm_iio_dmaengine_buffer_alloc() > * and attaches it to an IIO device with iio_device_attach_buffer(). > * It also appends the INDIO_BUFFER_HARDWARE mode to the supported modes of the > * IIO device. > + * > + * This "steals" the @chan pointer, so the caller must not call > + * dma_release_channel() on it. @chan is also checked for error, so callers > + * can pass the result of dma_request_chan() directly. > */ > int devm_iio_dmaengine_buffer_setup_ext(struct device *dev, > struct iio_dev *indio_dev, > - const char *channel, > + struct dma_chan *chan, > enum iio_buffer_direction dir) > { > struct iio_buffer *buffer; > > - buffer = iio_dmaengine_buffer_setup_ext(dev, indio_dev, channel, dir); > + buffer = iio_dmaengine_buffer_setup_ext(dev, indio_dev, chan, dir); > if (IS_ERR(buffer)) > return PTR_ERR(buffer); >
On Mon, 22 Jul 2024 16:57:16 -0500 David Lechner <dlechner@baylibre.com> wrote: > This adds support for SPI offload to the ad7944 driver. This allows > reading data at the max sample rate of 2.5 MSPS. > > Signed-off-by: David Lechner <dlechner@baylibre.com> I think you can easily make the sampling frequence attribute disappear for cases where we can't provide a real value back for a read. > --- > > Note: in v2 we discussed if we should make the SPI offload use buffer1 > instead of buffer0 as to not break userspace. I'm still on the fence > about if we should do that or not. Mainly because many userspace tools > aren't aware of multiple buffers yet, so would make it harder to > use the driver. And technically, the way it is implemented right now > is not going to change anything for existing users since they won't > be using the offload feature. So the argument could be made that this > isn't really breaking userspace after all. > > v3 changes: > * Finished TODOs. > * Adapted to changes in other patches. > > v2 changes: > > In the previous version, there was a new separate driver for the PWM > trigger and DMA hardware buffer. This was deemed too complex so they > are moved into the ad7944 driver. > > It has also been reworked to accommodate for the changes described in > the other patches. > --- > drivers/iio/adc/ad7944.c | 173 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 166 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c > index 0f36138a7144..43674ff439d2 100644 > --- a/drivers/iio/adc/ad7944.c > +++ b/drivers/iio/adc/ad7944.c > @@ -9,6 +9,7 @@ > #include <linux/align.h> > #include <linux/bitfield.h> > #include <linux/bitops.h> > +#include <linux/clk.h> > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/err.h> > @@ -21,6 +22,7 @@ > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > +#include <linux/iio/buffer-dmaengine.h> > #include <linux/iio/trigger_consumer.h> > #include <linux/iio/triggered_buffer.h> > > @@ -54,6 +56,8 @@ struct ad7944_adc { > enum ad7944_spi_mode spi_mode; > struct spi_transfer xfers[3]; > struct spi_message msg; > + struct spi_transfer offload_xfers[3]; > + struct spi_message offload_msg; > void *chain_mode_buf; > /* Chip-specific timing specifications. */ > const struct ad7944_timing_spec *timing_spec; > @@ -65,6 +69,8 @@ struct ad7944_adc { > bool always_turbo; > /* Reference voltage (millivolts). */ > unsigned int ref_mv; > + /* Clock that triggers SPI offload. */ > + struct clk *trigger_clk; > > /* > * DMA (thus cache coherency maintenance) requires the > @@ -81,6 +87,8 @@ struct ad7944_adc { > > /* quite time before CNV rising edge */ > #define T_QUIET_NS 20 I'd prefer these prefixed as AD7944_T_QUIET_NS etc > +/* minimum CNV high time to trigger conversion */ > +#define T_CNVH_NS 20 > > static const struct ad7944_timing_spec ad7944_timing_spec = { > .conv_ns = 420, > @@ -123,6 +131,7 @@ static const struct ad7944_chip_info _name##_chip_info = { \ > .scan_type.endianness = IIO_CPU, \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \ > | BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\ So this gets registered for all cases, but may return -EIOPNOTSUPPORTED? That's not ideal. If we can we should hide this file if we aren't going to be able to read it. Whilst it may seem overkill that's a separate iio_chan_spec array etc that is picked based on whether we are using spi offloading or not. > }, \ > IIO_CHAN_SOFT_TIMESTAMP(1), \ > }, \ > @@ -236,6 +245,54 @@ static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc > return devm_spi_optimize_message(dev, adc->spi, &adc->msg); > } > > +static void ad7944_offload_unprepare(void *p) > +{ > + struct ad7944_adc *adc = p; > + > + spi_offload_unprepare(adc->spi, NULL, &adc->offload_msg); > +} > static int ad7944_probe(struct spi_device *spi) > { > const struct ad7944_chip_info *chip_info; > @@ -554,13 +682,11 @@ static int ad7944_probe(struct spi_device *spi) > ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]); > if (ret) > return ret; > - > break; > case AD7944_SPI_MODE_SINGLE: > ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]); > if (ret) > return ret; > - Don't edit unrelated white space in a patch that is doing more important things. Just noise... > break;
On 7/27/24 8:15 AM, Jonathan Cameron wrote: > On Mon, 22 Jul 2024 16:57:09 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> +/** >> + * spi_offload_prepare - prepare offload hardware for a transfer >> + * @spi: The spi device to use for the transfers. >> + * @id: Function ID if SPI device uses more than one offload or NULL. >> + * @msg: The SPI message to use for the offload operation. >> + * >> + * Requests an offload instance with the specified ID and programs it with the >> + * provided message. >> + * >> + * The message must not be pre-optimized (do not call spi_optimize_message() on >> + * the message). >> + * >> + * Calls must be balanced with spi_offload_unprepare(). >> + * >> + * Return: 0 on success, else a negative error code. >> + */ >> +int spi_offload_prepare(struct spi_device *spi, const char *id, >> + struct spi_message *msg) >> +{ >> + struct spi_controller *ctlr = spi->controller; >> + int ret; >> + >> + if (!ctlr->offload_ops) >> + return -EOPNOTSUPP; >> + >> + msg->offload = true; > I'd set this later perhaps as... If we move it, then we would have to create a new function to call instead of spi_optimize_message() so that the controller driver can know that this is an offload message and not a regular message since they will need to be handled differently during the optimization phase. >> + >> + ret = spi_optimize_message(spi, msg); >> + if (ret) > > It otherwise needs clearing here so it doesn't have side > effects if an error occurs. > >> + return ret; >> + >> + mutex_lock(&ctlr->io_mutex); >> + ret = ctlr->offload_ops->prepare(spi, id, msg); >> + mutex_unlock(&ctlr->io_mutex); >> + >> + if (ret) { >> + spi_unoptimize_message(msg); >> + msg->offload = false; >> + return ret; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(spi_offload_prepare); >
On Tue, 30 Jul 2024 14:35:09 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 7/27/24 8:15 AM, Jonathan Cameron wrote: > > On Mon, 22 Jul 2024 16:57:09 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > >> +/** > >> + * spi_offload_prepare - prepare offload hardware for a transfer > >> + * @spi: The spi device to use for the transfers. > >> + * @id: Function ID if SPI device uses more than one offload or NULL. > >> + * @msg: The SPI message to use for the offload operation. > >> + * > >> + * Requests an offload instance with the specified ID and programs it with the > >> + * provided message. > >> + * > >> + * The message must not be pre-optimized (do not call spi_optimize_message() on > >> + * the message). > >> + * > >> + * Calls must be balanced with spi_offload_unprepare(). > >> + * > >> + * Return: 0 on success, else a negative error code. > >> + */ > >> +int spi_offload_prepare(struct spi_device *spi, const char *id, > >> + struct spi_message *msg) > >> +{ > >> + struct spi_controller *ctlr = spi->controller; > >> + int ret; > >> + > >> + if (!ctlr->offload_ops) > >> + return -EOPNOTSUPP; > >> + > >> + msg->offload = true; > > I'd set this later perhaps as... > > If we move it, then we would have to create a new function > to call instead of spi_optimize_message() so that the controller > driver can know that this is an offload message and not a > regular message since they will need to be handled differently > during the optimization phase. Ah. I'd missed that. > > >> + > >> + ret = spi_optimize_message(spi, msg); > >> + if (ret) > > > > It otherwise needs clearing here so it doesn't have side > > effects if an error occurs. Then it needs clearing here I think. > > > >> + return ret; > >> + > >> + mutex_lock(&ctlr->io_mutex); > >> + ret = ctlr->offload_ops->prepare(spi, id, msg); > >> + mutex_unlock(&ctlr->io_mutex); > >> + > >> + if (ret) { > >> + spi_unoptimize_message(msg); > >> + msg->offload = false; > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> +EXPORT_SYMBOL_GPL(spi_offload_prepare); > >
On Tue, Jul 23, 2024 at 09:58:30AM +0100, Conor Dooley wrote: > On Mon, Jul 22, 2024 at 04:57:07PM -0500, David Lechner wrote: > > There is a recap at the end of this cover letter for those not familiar > > with the previous discussions. For those that are, we'll get right to > > the changes since the last version. > > > > In RFC v2, most of the discussion was around the DT bindings, so that > > is what has mostly changed since then. I think we mostly settled on > > what properties are needed and where they should go. There are probably > > still some details to work out (see PATCH 5/9 for more discussion) but > > I think we have the big-picture stuff figured out. > > Thanks for the updates. I'm on holiday until rc2, so it'll not be until > then that I can really take a look at this. Figured I'd let you know > rather than just ignore you... Finally got around to actually looking at the binding patches, but since Rob got there before me I didn't have all that much to say. Thanks for looking into the graph idea, and I think I agree that it is worth excluding that until you're actually going to use the crc checker etc.
On Mon, Jul 22, 2024 at 04:57:07PM -0500, David Lechner wrote: > There is a recap at the end of this cover letter for those not familiar > with the previous discussions. For those that are, we'll get right to > the changes since the last version. I didn't reply on this mainly because I don't have anything super substantial to say that wasn't already covered.
There is a recap at the end of this cover letter for those not familiar with the previous discussions. For those that are, we'll get right to the changes since the last version. In RFC v2, most of the discussion was around the DT bindings, so that is what has mostly changed since then. I think we mostly settled on what properties are needed and where they should go. There are probably still some details to work out (see PATCH 5/9 for more discussion) but I think we have the big-picture stuff figured out. Here is the actual devicetree used for testing to show how it all comes together: trigger_clk: adc-trigger-clock { compatible = "pwm-clock"; #clock-cells = <0>; #trigger-source-cells = <0>; pwms = <&adc_trigger 0 10000>; }; ... axi_spi_engine_0: spi@44a00000 { compatible = "adi,axi-spi-engine-1.00.a"; reg = <0x44a00000 0x1000>; interrupt-parent = <&intc>; interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clkc 15>, <&spi_clk>; clock-names = "s_axi_aclk", "spi_clk"; /* offload-specific properties */ #spi-offload-cells = <1>; dmas = <&rx_dma 0>; dma-names = "offload0-rx"; trigger-sources = <&trigger_clk>; #address-cells = <1>; #size-cells = <0>; ad7986: adc@0 { compatible = "adi,ad7986"; reg = <0>; spi-max-frequency = <111111111>; /* 9 ns period */ adi,spi-mode = "single"; avdd-supply = <&eval_u12>; dvdd-supply = <&eval_u12>; vio-supply = <&eval_u3>; bvdd-supply = <&eval_u10>; ref-supply = <&eval_u5>; turbo-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>; spi-offloads = <&axi_spi_engine_0 0>; }; }; A working branch complete with extra hacks can be found at [1]. Also, I took a detour looking into what it would take to get Martin Sperl's Raspberry Pi DMA offload proof-of-concept [2] updated to work with this. This way we could have a second user to help guide the design process. Given all of the SPI hardware quirks on that platform and the unsolved technical issues, like how to get accurate time delays and how to work around the 32-bit DMA word limitation, it would be more work than I have time for (at least without someone sponsoring the work). [1]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v3 [2]: https://github.com/msperl/spi-bcm2835/blob/refactor_dmachain_for_prepared_messages/spi-bcm2835dma.c --- Changes in v3: - See individual patches for more detailed changes. - Reworked DT bindings to have things physically connected to the SPI controller be properties of the SPI controller and use more conventional provider/consumer properties. - Added more SPI APIs for peripheral drivers to use to get auxillary offload resources, like triggers. - Link to v2: https://lore.kernel.org/r/20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com --- As a recap, here is the background and end goal of this series: The AXI SPI Engine is a SPI controller that has the ability to record a series of SPI transactions and then play them back using a hardware trigger. This allows operations to be performed, repeating many times, without any CPU intervention. This is needed for achieving high data rates (millions of samples per second) from ADCs and DACs that are connected via a SPI bus. The offload hardware interface consists of a trigger input and a data output for the RX data. These are connected to other hardware external to the SPI controller. To record one or more transactions, commands and TX data are written to memories in the controller (RX buffer is not used since RX data gets streamed to an external sink). This sequence of transactions can then be played back when the trigger input is asserted. This series includes core SPI support along with the first SPI controller (AXI SPI Engine) and SPI peripheral (AD7944 ADC) that use them. This enables capturing analog data at 2 million samples per second. The hardware setup looks like this: +-------------------------------+ +------------------+ | | | | | SOC/FPGA | | AD7944 ADC | | +---------------------+ | | | | | AXI SPI Engine | | | | | | SPI Bus ============ SPI Bus | | | | | | | | | +---------------+ | | | | | | | Offload 0 | | | +------------------+ | | | RX DATA OUT > > > > | | | | TRIGGER IN < < < v | | | +---------------+ | ^ v | | +---------------------+ ^ v | | | AXI PWM | ^ v | | | CH0 > ^ v | | +---------------------+ v | | | AXI DMA | v | | | CH0 < < < | | +---------------------+ | | | +-------------------------------+ To: Mark Brown <broonie@kernel.org> To: Jonathan Cameron <jic23@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzk+dt@kernel.org> To: Conor Dooley <conor+dt@kernel.org> To: Nuno Sá <nuno.sa@analog.com> Cc: Michael Hennerich <Michael.Hennerich@analog.com> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: David Jander <david@protonic.nl> Cc: Martin Sperl <kernel@martin.sperl.org> Cc: <linux-spi@vger.kernel.org> Cc: <devicetree@vger.kernel.org> Cc: <linux-kernel@vger.kernel.org> Cc: <linux-iio@vger.kernel.org> --- David Lechner (9): spi: dt-bindings: add spi-offload properties spi: add basic support for SPI offloading spi: add support for hardware triggered offload spi: add offload TX/RX streaming APIs spi: dt-bindings: axi-spi-engine: document spi-offloads spi: axi-spi-engine: implement offload support iio: buffer-dmaengine: generalize requesting DMA channel dt-bindings: iio: adc: adi,ad7944: add SPI offload properties iio: adc: ad7944: add support for SPI offload .../devicetree/bindings/iio/adc/adi,ad7944.yaml | 3 + .../bindings/spi/adi,axi-spi-engine.yaml | 41 +++ .../devicetree/bindings/spi/spi-controller.yaml | 5 + .../bindings/spi/spi-peripheral-props.yaml | 11 + drivers/iio/adc/ad7944.c | 173 ++++++++++- drivers/iio/buffer/industrialio-buffer-dmaengine.c | 39 ++- drivers/iio/dac/adi-axi-dac.c | 3 +- drivers/spi/spi-axi-spi-engine.c | 341 ++++++++++++++++++++- drivers/spi/spi.c | 226 +++++++++++++- include/linux/iio/buffer-dmaengine.h | 11 +- include/linux/spi/spi.h | 169 ++++++++++ 11 files changed, 989 insertions(+), 33 deletions(-) --- base-commit: 7a891f6a5000f7658274b554cf993dd56aa5adbc change-id: 20240510-dlech-mainline-spi-engine-offload-2-afce3790b5ab