| Message ID | 20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com (mailing list archive) |
|---|---|
| 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, 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 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 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