| Message ID | 20240109-axi-spi-engine-series-3-v1-1-e42c6a986580@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Wed, Jan 10, 2024 at 01:49:42PM -0600, David Lechner wrote: > This adds a feature for specialized SPI controllers that can record > a series of SPI transfers, including tx data, cs assertions, delays, > etc. and then play them back using a hardware trigger without CPU > intervention. > The intended use case for this is with the AXI SPI Engine to capture > data from ADCs at high rates (MSPS) with a stable sample period. > Most of the implementation is controller-specific and will be handled by > drivers that implement the offload_ops callbacks. The API follows a > prepare/enable pattern that should be familiar to users of the clk > subsystem. This is a lot to do in one go, and I think it's a bit too off on the side and unintegrated with the core. There's two very high level bits here, there's the pre-cooking a message for offloading to be executed by a hardware engine and there's the bit where that's triggered by some hardwar event rather than by software. There was a bunch of discussion of the former case with David Jander (CCed) a while back when he was doing all the work he did on optimising the core for uncontended uses, the thinking there was to have a spi_prepare_message() (or similar) API that drivers could call and then reuse the same transfer repeatedly, and even without any interface for client drivers it's likely that we'd be able to take advantage of it in the core for multi-transfer messages. I'd be surprised if there weren't wins when the message goes over the DMA copybreak size. A much wider range of hardware would be able to do this bit, for example David's case was a Raspberry Pi using the DMA controller to write into the SPI controller control registers in order to program it for multiple transfers, bounce chip select and so on. You could also use the microcontroller cores that many embedded SoCs have, and even with zero hardware support for offloading anything there's savings in the message validation and DMA mapping. The bit where messages are initiated by hardware is a step beyond that, I think we need a bit more API for connecting up the triggers and we also need to have something handling what happens with normal operation of the device while these triggers are enabled. I think it could be useful to split this bit out since there's a lot more to work out there in terms of interfaces. > +/** > + * SPI_OFFLOAD_RX - placeholder for indicating read transfers for offloads > + * > + * Assign xfer->rx_buf to this value for any read transfer passed to > + * spi_offload_prepare(). This will act as a flag to indicate to the offload > + * that it should do something with the data read during this transfer. What > + * that something can be is determined by the specific hardware, e.g. it could > + * be piped to DMA or a DSP, etc. > + */ > +#define SPI_OFFLOAD_RX_SENTINEL ((void *)1) This feels like something where there are likely to be multiple options and we need configurability. I'd also expect to see a similar transmit option. > +int spi_offload_prepare(struct spi_offload *offload, struct spi_device *spi, > + struct spi_transfer *xfers, unsigned int num_xfers) I would expect us to just generically prepare a message, then pass a prepared message into the API that enables a trigger. We would need something that handles the difference between potentially offloading for better performance and having a hardware trigger, I think that might be a case of just not exposing the engine's prepare to client drivers and then having the core track if it needs to do that when enabling a hardware trigger. > + /** > + * @enable: Callback to enable the offload. > + */ > + int (*enable)(struct spi_offload *offload); > + /** > + * @disable: Callback to disable the offload. > + */ > + void (*disable)(struct spi_offload *offload); I'm not seeing anything in this API that provides a mechanism for configuring what triggers things to start, even in the case where things are triggered by hardware rather than initiated by software I'd expect to see hardware with runtime configurability. The binding is a bit unclear but it seems to be expecting this to be statically configured in hardware and that there will be a 1:1 mapping between triggers and scripts that can be configured, if nothing else I would expect that there will be hardware with more possible triggers than scripts. I'd also expect some treatement of what happens with the standard SPI API while something is enabled.
Hi David, On Wed, 2024-01-10 at 13:49 -0600, David Lechner wrote: > This adds a feature for specialized SPI controllers that can record > a series of SPI transfers, including tx data, cs assertions, delays, > etc. and then play them back using a hardware trigger without CPU > intervention. > > The intended use case for this is with the AXI SPI Engine to capture > data from ADCs at high rates (MSPS) with a stable sample period. > > Most of the implementation is controller-specific and will be handled by > drivers that implement the offload_ops callbacks. The API follows a > prepare/enable pattern that should be familiar to users of the clk > subsystem. > > Consumers of this API will make calls similar to this: > > /* in probe() */ > offload = spi_offload_get(spi, 0); > ... > On top of what Mark already stated, and as we already discussed offline, I personally don't like this provider - consumer interface for the offload. The first thing is that this is taking into account the possibility of having multiple offload cores. While the FGPA core was designed with that in mind, we don't really have any design using multiple offloads in one spi engine (always one). Hence this is all pretty much untested. If we want to already have this support, my feeling is that we should have a simple integer dt property for the peripheral devices (similar to cs). When a device is being created/added, the spi core would parse this property and get it's offload index. The point is that this would all be transparent for spi devices drivers that would only have to call the SPI API's and the core would make sure the right index is passed to the controller. But honestly, IMO, I would just keep things simple for now and assume one core per engine. I would probably also prefer to see all the new interfaces part of the spi_controller struct directly... - Nuno Sá
On Thu, Jan 11, 2024 at 09:49:08AM +0100, Nuno Sá wrote: > On Wed, 2024-01-10 at 13:49 -0600, David Lechner wrote: > > /* in probe() */ > > offload = spi_offload_get(spi, 0); > On top of what Mark already stated, and as we already discussed offline, I > personally don't like this provider - consumer interface for the offload. The > first thing is that this is taking into account the possibility of having > multiple offload cores. While the FGPA core was designed with that in mind, we > don't really have any design using multiple offloads in one spi engine (always > one). Hence this is all pretty much untested. I tend to agree that we shouldn't be exposing this to SPI device drivers however we will want to keep track of if the unit is busy, and designing it to cope with multiple offloads does seem like sensible future proofing. There's also the possibility that one engine might be able to cope with multiple scripts being active at once (eg, triggering a different action depending on the trigger).
On Thu, 2024-01-11 at 13:33 +0000, Mark Brown wrote: > On Thu, Jan 11, 2024 at 09:49:08AM +0100, Nuno Sá wrote: > > On Wed, 2024-01-10 at 13:49 -0600, David Lechner wrote: > > > > /* in probe() */ > > > offload = spi_offload_get(spi, 0); > > > On top of what Mark already stated, and as we already discussed offline, I > > personally don't like this provider - consumer interface for the offload. > > The > > first thing is that this is taking into account the possibility of having > > multiple offload cores. While the FGPA core was designed with that in mind, > > we > > don't really have any design using multiple offloads in one spi engine > > (always > > one). Hence this is all pretty much untested. > > I tend to agree that we shouldn't be exposing this to SPI device drivers > however we will want to keep track of if the unit is busy, and designing > it to cope with multiple offloads does seem like sensible future > proofing. There's also the possibility that one engine might be able to Fair enough. But wouldn't a simple DT integer property (handled by the spi core) to identify the offload index be easier for SPI device drivers? We could still have dedicated interfaces for checking if the unit is busy or not... The point is that we would not need an explicit get() from SPI drivers. I'm of course assuming that one spi device can only be connected to one engine which seems reasonable to me. - Nuno Sá
On Thu, Jan 11, 2024 at 03:11:32PM +0100, Nuno Sá wrote: > On Thu, 2024-01-11 at 13:33 +0000, Mark Brown wrote: > > I tend to agree that we shouldn't be exposing this to SPI device drivers > > however we will want to keep track of if the unit is busy, and designing > > it to cope with multiple offloads does seem like sensible future > > proofing. There's also the possibility that one engine might be able to > Fair enough. But wouldn't a simple DT integer property (handled by the spi core) > to identify the offload index be easier for SPI device drivers? We could still > have dedicated interfaces for checking if the unit is busy or not... The point > is that we would not need an explicit get() from SPI drivers. It feels like we'd need a get/release operation of some kind for mutual exclusion, it's not just the discovery it's also figuring out if the hardware is in use at a given moment. > I'm of course assuming that one spi device can only be connected to one engine > which seems reasonable to me. I can see someone implementing this with for example the microcontroller cores a lot of SoCs have in which case all bets are off.
On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jan 10, 2024 at 01:49:42PM -0600, David Lechner wrote: > > This adds a feature for specialized SPI controllers that can record > > a series of SPI transfers, including tx data, cs assertions, delays, > > etc. and then play them back using a hardware trigger without CPU > > intervention. > > > The intended use case for this is with the AXI SPI Engine to capture > > data from ADCs at high rates (MSPS) with a stable sample period. > > > Most of the implementation is controller-specific and will be handled by > > drivers that implement the offload_ops callbacks. The API follows a > > prepare/enable pattern that should be familiar to users of the clk > > subsystem. > > This is a lot to do in one go, and I think it's a bit too off on the > side and unintegrated with the core. There's two very high level bits > here, there's the pre-cooking a message for offloading to be executed by > a hardware engine and there's the bit where that's triggered by some > hardwar event rather than by software. > > There was a bunch of discussion of the former case with David Jander I found [1] which appears to be the conversation you are referring to. Is that all or is there more that I missed? [1]: https://lore.kernel.org/linux-spi/20220512163445.6dcca126@erd992/ > (CCed) a while back when he was doing all the work he did on optimising > the core for uncontended uses, the thinking there was to have a > spi_prepare_message() (or similar) API that drivers could call and then > reuse the same transfer repeatedly, and even without any interface for > client drivers it's likely that we'd be able to take advantage of it in > the core for multi-transfer messages. I'd be surprised if there weren't > wins when the message goes over the DMA copybreak size. A much wider > range of hardware would be able to do this bit, for example David's case > was a Raspberry Pi using the DMA controller to write into the SPI > controller control registers in order to program it for multiple > transfers, bounce chip select and so on. You could also use the > microcontroller cores that many embedded SoCs have, and even with zero > hardware support for offloading anything there's savings in the message > validation and DMA mapping. > I can see how such a spi_prepare_message() API could be useful in general and would be a good first step towards what we are wanting to accomplish too. For example, in the IIO subsystem, it is a common pattern when using a triggered buffer to prepare some spi xfer structs in the buffer setup phase that get reused multiple times. So this could, as you said, at least save the overhead of validating/mapping the same xfers over and over. I will look into this first and then we can come back to the second part about hardware triggers once that is done.
On Thu, Jan 11, 2024 at 2:54 PM David Lechner <dlechner@baylibre.com> wrote: > > On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Wed, Jan 10, 2024 at 01:49:42PM -0600, David Lechner wrote: > > > This adds a feature for specialized SPI controllers that can record > > > a series of SPI transfers, including tx data, cs assertions, delays, > > > etc. and then play them back using a hardware trigger without CPU > > > intervention. > > > > > The intended use case for this is with the AXI SPI Engine to capture > > > data from ADCs at high rates (MSPS) with a stable sample period. > > > > > Most of the implementation is controller-specific and will be handled by > > > drivers that implement the offload_ops callbacks. The API follows a > > > prepare/enable pattern that should be familiar to users of the clk > > > subsystem. > > > > This is a lot to do in one go, and I think it's a bit too off on the > > side and unintegrated with the core. There's two very high level bits > > here, there's the pre-cooking a message for offloading to be executed by > > a hardware engine and there's the bit where that's triggered by some > > hardwar event rather than by software. > > > > There was a bunch of discussion of the former case with David Jander > > I found [1] which appears to be the conversation you are referring to. > Is that all or is there more that I missed? > > [1]: https://lore.kernel.org/linux-spi/20220512163445.6dcca126@erd992/ > > > (CCed) a while back when he was doing all the work he did on optimising > > the core for uncontended uses, the thinking there was to have a > > spi_prepare_message() (or similar) API that drivers could call and then > > reuse the same transfer repeatedly, and even without any interface for > > client drivers it's likely that we'd be able to take advantage of it in > > the core for multi-transfer messages. I'd be surprised if there weren't > > wins when the message goes over the DMA copybreak size. A much wider > > range of hardware would be able to do this bit, for example David's case > > was a Raspberry Pi using the DMA controller to write into the SPI For those, following along, it looks like the RPi business was actually a 2013 discussion with Martin Sperl [2]. Both this and [1] discuss proposed spi_prepare_message() APIs. [2]: https://lore.kernel.org/linux-spi/CACRpkdb4mn_Hxg=3tuBu89n6eyJ082EETkwtNbzZDFZYTHbVVg@mail.gmail.com/T/#u
On Thu, Jan 11, 2024 at 03:32:54PM -0600, David Lechner wrote: > On Thu, Jan 11, 2024 at 2:54 PM David Lechner <dlechner@baylibre.com> wrote: > > > (CCed) a while back when he was doing all the work he did on optimising > > > the core for uncontended uses, the thinking there was to have a > > > spi_prepare_message() (or similar) API that drivers could call and then > > > reuse the same transfer repeatedly, and even without any interface for > > > client drivers it's likely that we'd be able to take advantage of it in > > > the core for multi-transfer messages. I'd be surprised if there weren't > > > wins when the message goes over the DMA copybreak size. A much wider > > > range of hardware would be able to do this bit, for example David's case > > > was a Raspberry Pi using the DMA controller to write into the SPI > For those, following along, it looks like the RPi business was > actually a 2013 discussion with Martin Sperl [2]. Both this and [1] > discuss proposed spi_prepare_message() APIs. > [2]: https://lore.kernel.org/linux-spi/CACRpkdb4mn_Hxg=3tuBu89n6eyJ082EETkwtNbzZDFZYTHbVVg@mail.gmail.com/T/#u Oh, yes - sorry, I'd misremembered which optimisation effort it was associated with. Apologies.
On Thu, 2024-01-11 at 15:41 +0000, Mark Brown wrote: > On Thu, Jan 11, 2024 at 03:11:32PM +0100, Nuno Sá wrote: > > On Thu, 2024-01-11 at 13:33 +0000, Mark Brown wrote: > > > > I tend to agree that we shouldn't be exposing this to SPI device drivers > > > however we will want to keep track of if the unit is busy, and designing > > > it to cope with multiple offloads does seem like sensible future > > > proofing. There's also the possibility that one engine might be able to > > > Fair enough. But wouldn't a simple DT integer property (handled by the spi core) > > to identify the offload index be easier for SPI device drivers? We could still > > have dedicated interfaces for checking if the unit is busy or not... The point > > is that we would not need an explicit get() from SPI drivers. > > It feels like we'd need a get/release operation of some kind for mutual > exclusion, it's not just the discovery it's also figuring out if the > hardware is in use at a given moment. > Hmm did not thought about the busy case. Still, I could see this being easily done on the controller driver (at least until we have a clear idea if this is useful or if it will attract more users) or even at the spi core on the prepare + unprepare interfaces. A flag could be enough to return EBUSY if someone is already in the process of preparing + enabling the engine. Bah, anyways, it's just I'm really not thrilled about that kind of interface in here but yeah, as long as you think it's worth it... > - Nuno Sá
Hi Mark, David, Thanks for CC'ing me. Been reading the discussion so far. On Thu, 11 Jan 2024 21:49:53 +0000 Mark Brown <broonie@kernel.org> wrote: > On Thu, Jan 11, 2024 at 03:32:54PM -0600, David Lechner wrote: > > On Thu, Jan 11, 2024 at 2:54 PM David Lechner <dlechner@baylibre.com> wrote: > > > > > (CCed) a while back when he was doing all the work he did on optimising > > > > the core for uncontended uses, the thinking there was to have a > > > > spi_prepare_message() (or similar) API that drivers could call and then > > > > reuse the same transfer repeatedly, and even without any interface for > > > > client drivers it's likely that we'd be able to take advantage of it in > > > > the core for multi-transfer messages. I'd be surprised if there weren't > > > > wins when the message goes over the DMA copybreak size. A much wider > > > > range of hardware would be able to do this bit, for example David's case > > > > was a Raspberry Pi using the DMA controller to write into the SPI > > > For those, following along, it looks like the RPi business was > > actually a 2013 discussion with Martin Sperl [2]. Both this and [1] > > discuss proposed spi_prepare_message() APIs. > > > [2]: https://lore.kernel.org/linux-spi/CACRpkdb4mn_Hxg=3tuBu89n6eyJ082EETkwtNbzZDFZYTHbVVg@mail.gmail.com/T/#u > > Oh, yes - sorry, I'd misremembered which optimisation effort it was > associated with. Apologies. Yes. It was Martin Sperl who proposed this on a Rpi. I mentioned something similar toward the end of my 2nd email reply in that thread [1]. That might have triggered the confusion. As for my interests, I am all for devising ways to make the SPI subsystem more suitable for optimized high-performance use-cases. In that regard, I think re-usable messages (spi_prepare_message()) can be useful. More capable hardware can enable very powerful use-cases for SPI, and it would be cool if the spi subsystem had the needed infrastructure to support those. As for hardware-triggers, I still need to wrap my head around how to have a universally usable API that works nice for the first use-case that comes along and also doesn't screw up the next use-case that might follow. Keep me posted. [1] https://lore.kernel.org/linux-spi/20220513144645.2d16475c@erd992/ Best regards,
On Thu, Jan 11, 2024 at 3:32 PM David Lechner <dlechner@baylibre.com> wrote: > > On Thu, Jan 11, 2024 at 2:54 PM David Lechner <dlechner@baylibre.com> wrote: > > > > On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > On Wed, Jan 10, 2024 at 01:49:42PM -0600, David Lechner wrote: > > > > This adds a feature for specialized SPI controllers that can record > > > > a series of SPI transfers, including tx data, cs assertions, delays, > > > > etc. and then play them back using a hardware trigger without CPU > > > > intervention. > > > > > > > The intended use case for this is with the AXI SPI Engine to capture > > > > data from ADCs at high rates (MSPS) with a stable sample period. > > > > > > > Most of the implementation is controller-specific and will be handled by > > > > drivers that implement the offload_ops callbacks. The API follows a > > > > prepare/enable pattern that should be familiar to users of the clk > > > > subsystem. > > > > > > This is a lot to do in one go, and I think it's a bit too off on the > > > side and unintegrated with the core. There's two very high level bits > > > here, there's the pre-cooking a message for offloading to be executed by > > > a hardware engine and there's the bit where that's triggered by some > > > hardwar event rather than by software. > > > > > > There was a bunch of discussion of the former case with David Jander > > > > I found [1] which appears to be the conversation you are referring to. > > Is that all or is there more that I missed? > > > > [1]: https://lore.kernel.org/linux-spi/20220512163445.6dcca126@erd992/ > > > > > (CCed) a while back when he was doing all the work he did on optimising > > > the core for uncontended uses, the thinking there was to have a > > > spi_prepare_message() (or similar) API that drivers could call and then > > > reuse the same transfer repeatedly, and even without any interface for > > > client drivers it's likely that we'd be able to take advantage of it in > > > the core for multi-transfer messages. I'd be surprised if there weren't > > > wins when the message goes over the DMA copybreak size. A much wider > > > range of hardware would be able to do this bit, for example David's case > > > was a Raspberry Pi using the DMA controller to write into the SPI > > For those, following along, it looks like the RPi business was > actually a 2013 discussion with Martin Sperl [2]. Both this and [1] > discuss proposed spi_prepare_message() APIs. > > [2]: https://lore.kernel.org/linux-spi/CACRpkdb4mn_Hxg=3tuBu89n6eyJ082EETkwtNbzZDFZYTHbVVg@mail.gmail.com/T/#u I found one more. A patch from Martin with the basic proposed API but not much in the way of implementation. It looks like this is where the idea fizzled out. https://lore.kernel.org/linux-spi/0C7D5B1E-E561-4F52-BEA8-572EB0CA26A6@martin.sperl.org/
On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jan 10, 2024 at 01:49:42PM -0600, David Lechner wrote: > > This adds a feature for specialized SPI controllers that can record > > a series of SPI transfers, including tx data, cs assertions, delays, > > etc. and then play them back using a hardware trigger without CPU > > intervention. > > > The intended use case for this is with the AXI SPI Engine to capture > > data from ADCs at high rates (MSPS) with a stable sample period. > > > Most of the implementation is controller-specific and will be handled by > > drivers that implement the offload_ops callbacks. The API follows a > > prepare/enable pattern that should be familiar to users of the clk > > subsystem. > > This is a lot to do in one go, and I think it's a bit too off on the > side and unintegrated with the core. There's two very high level bits > here, there's the pre-cooking a message for offloading to be executed by > a hardware engine and there's the bit where that's triggered by some > hardwar event rather than by software. > ... > > The bit where messages are initiated by hardware is a step beyond that, > I think we need a bit more API for connecting up the triggers and we > also need to have something handling what happens with normal operation > of the device while these triggers are enabled. I think it could be > useful to split this bit out since there's a lot more to work out there > in terms of interfaces. Now that we have addressed the pre-cooking messages bit [1] I'm coming back to the hardware trigger bit. Since the hardware trigger part hasn't been discussed in the past, it's not so clear to me what is being requested here (also see specific questions below). [1]: https://lore.kernel.org/linux-spi/20240219-mainline-spi-precook-message-v2-0-4a762c6701b9@baylibre.com/T/#t > > > +/** > > + * SPI_OFFLOAD_RX - placeholder for indicating read transfers for offloads > > + * > > + * Assign xfer->rx_buf to this value for any read transfer passed to > > + * spi_offload_prepare(). This will act as a flag to indicate to the offload > > + * that it should do something with the data read during this transfer. What > > + * that something can be is determined by the specific hardware, e.g. it could > > + * be piped to DMA or a DSP, etc. > > + */ > > +#define SPI_OFFLOAD_RX_SENTINEL ((void *)1) > > This feels like something where there are likely to be multiple options > and we need configurability. I'd also expect to see a similar transmit > option. Having something similar for TX makes sense. What other sorts of options are you envisioning here? > > > +int spi_offload_prepare(struct spi_offload *offload, struct spi_device *spi, > > + struct spi_transfer *xfers, unsigned int num_xfers) > > I would expect us to just generically prepare a message, then pass a > prepared message into the API that enables a trigger. We would need > something that handles the difference between potentially offloading for > better performance and having a hardware trigger, I think that might be > a case of just not exposing the engine's prepare to client drivers and > then having the core track if it needs to do that when enabling a > hardware trigger. Not exposing the offload prepare to client drivers sounds reasonable. I'm not sure I understand the potential need for an offload without a hardware trigger though. > > > + /** > > + * @enable: Callback to enable the offload. > > + */ > > + int (*enable)(struct spi_offload *offload); > > + /** > > + * @disable: Callback to disable the offload. > > + */ > > + void (*disable)(struct spi_offload *offload); > > I'm not seeing anything in this API that provides a mechanism for > configuring what triggers things to start, even in the case where things > are triggered by hardware rather than initiated by software I'd expect > to see hardware with runtime configurability. The binding is a bit > unclear but it seems to be expecting this to be statically configured in > hardware and that there will be a 1:1 mapping between triggers and > scripts that can be configured, if nothing else I would expect that > there will be hardware with more possible triggers than scripts. For the use case of ADCs/DACs we would want a periodic trigger where the period of the trigger is runtime configurable (via sysfs). Is this the sort of thing you had in mind here? What other sorts of triggers do you have in mind? > > I'd also expect some treatement of what happens with the standard SPI > API while something is enabled. I suppose it makes sense to return -EBUSY from spi_sync()/spi_async()/spi_bus_lock() when a hardware trigger is enabled.
On Mon, Mar 04, 2024 at 05:21:21PM -0600, David Lechner wrote: > On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@kernel.org> wrote: > > The bit where messages are initiated by hardware is a step beyond that, > > I think we need a bit more API for connecting up the triggers and we > > also need to have something handling what happens with normal operation > > of the device while these triggers are enabled. I think it could be > > useful to split this bit out since there's a lot more to work out there > > in terms of interfaces. > > > +/** > > > + * SPI_OFFLOAD_RX - placeholder for indicating read transfers for offloads > > > + * > > > + * Assign xfer->rx_buf to this value for any read transfer passed to > > > + * spi_offload_prepare(). This will act as a flag to indicate to the offload > > > + * that it should do something with the data read during this transfer. What > > > + * that something can be is determined by the specific hardware, e.g. it could > > > + * be piped to DMA or a DSP, etc. > > > + */ > > > +#define SPI_OFFLOAD_RX_SENTINEL ((void *)1) > > This feels like something where there are likely to be multiple options > > and we need configurability. I'd also expect to see a similar transmit > > option. > Having something similar for TX makes sense. What other sorts of > options are you envisioning here? You list two options for something that could be done with the data above - sending it to DMA or a DSP. My concern here is that a given piece of hardware might support more than one option and need to choose between them. > > > +int spi_offload_prepare(struct spi_offload *offload, struct spi_device *spi, > > > + struct spi_transfer *xfers, unsigned int num_xfers) > > I would expect us to just generically prepare a message, then pass a > > prepared message into the API that enables a trigger. We would need > > something that handles the difference between potentially offloading for > > better performance and having a hardware trigger, I think that might be > > a case of just not exposing the engine's prepare to client drivers and > > then having the core track if it needs to do that when enabling a > > hardware trigger. > Not exposing the offload prepare to client drivers sounds reasonable. > I'm not sure I understand the potential need for an offload without a > hardware trigger though. Something like pre-cooking the commands to read the interrupt status registers from a device, send a network transfer, or to download firmware and settings if you power the device off frequently. Basically anything with more than one operation that you might want to run repeatedly and care about the performance of. > > I'm not seeing anything in this API that provides a mechanism for > > configuring what triggers things to start, even in the case where things > > are triggered by hardware rather than initiated by software I'd expect > > to see hardware with runtime configurability. The binding is a bit > > unclear but it seems to be expecting this to be statically configured in > > hardware and that there will be a 1:1 mapping between triggers and > > scripts that can be configured, if nothing else I would expect that > > there will be hardware with more possible triggers than scripts. > For the use case of ADCs/DACs we would want a periodic trigger where > the period of the trigger is runtime configurable (via sysfs). Is this > the sort of thing you had in mind here? What other sorts of triggers > do you have in mind? Well, it could be pretty much any signal - I'd imagine there will be things that can trigger off GPIOs for example. Some sort of timer like you mention does sound plausible too. I think the API needs to be general enough to just cope with a very broad range of things in a possibly system/device specified manner and not have a short, prescriptive list. > > I'd also expect some treatement of what happens with the standard SPI > > API while something is enabled. > I suppose it makes sense to return -EBUSY from > spi_sync()/spi_async()/spi_bus_lock() when a hardware trigger is > enabled. That sounds reasonable. If something is software triggered then I'd expect to integrate with the current queuing mechanism.
On Mon, 4 Mar 2024 17:21:21 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Wed, Jan 10, 2024 at 01:49:42PM -0600, David Lechner wrote: > > > This adds a feature for specialized SPI controllers that can record > > > a series of SPI transfers, including tx data, cs assertions, delays, > > > etc. and then play them back using a hardware trigger without CPU > > > intervention. > > > > > The intended use case for this is with the AXI SPI Engine to capture > > > data from ADCs at high rates (MSPS) with a stable sample period. > > > > > Most of the implementation is controller-specific and will be handled by > > > drivers that implement the offload_ops callbacks. The API follows a > > > prepare/enable pattern that should be familiar to users of the clk > > > subsystem. > > > > This is a lot to do in one go, and I think it's a bit too off on the > > side and unintegrated with the core. There's two very high level bits > > here, there's the pre-cooking a message for offloading to be executed by > > a hardware engine and there's the bit where that's triggered by some > > hardwar event rather than by software. > > > > ... > > > > > The bit where messages are initiated by hardware is a step beyond that, > > I think we need a bit more API for connecting up the triggers and we > > also need to have something handling what happens with normal operation > > of the device while these triggers are enabled. I think it could be > > useful to split this bit out since there's a lot more to work out there > > in terms of interfaces. > > Now that we have addressed the pre-cooking messages bit [1] I'm coming > back to the hardware trigger bit. Since the hardware trigger part > hasn't been discussed in the past, it's not so clear to me what is > being requested here (also see specific questions below). > > [1]: https://lore.kernel.org/linux-spi/20240219-mainline-spi-precook-message-v2-0-4a762c6701b9@baylibre.com/T/#t Mark took the spi patches so we don't need to do anything complex next cycle. Just need the IIO driver with these additions as by the time we hit rc1 all the dependencies will be available. Rest were questions for Mark I think.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index a4b8c07c5951..f1d66b5d5491 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3057,6 +3057,13 @@ static int spi_controller_check_ops(struct spi_controller *ctlr) } } + if (ctlr->offload_ops && !(ctlr->offload_ops->get && + ctlr->offload_ops->prepare && + ctlr->offload_ops->unprepare && + ctlr->offload_ops->enable && + ctlr->offload_ops->disable)) + return -EINVAL; + return 0; } @@ -4448,6 +4455,38 @@ int spi_write_then_read(struct spi_device *spi, } EXPORT_SYMBOL_GPL(spi_write_then_read); +/** + * spi_offload_prepare - prepare offload hardware for a transfer + * @offload: The offload instance. + * @spi: The spi device to use for the transfers. + * @xfers: The transfers to be executed. + * @num_xfers: The number of transfers. + * + * Records a series of transfers to be executed later by the offload hardware + * trigger. + * + * Return: 0 on success, else a negative error code. + */ +int spi_offload_prepare(struct spi_offload *offload, struct spi_device *spi, + struct spi_transfer *xfers, unsigned int num_xfers) +{ + struct spi_controller *ctlr = offload->controller; + struct spi_message msg; + int ret; + + spi_message_init_with_transfers(&msg, xfers, num_xfers); + + ret = __spi_validate(spi, &msg); + if (ret) + return ret; + + msg.spi = spi; + ret = ctlr->offload_ops->prepare(offload, &msg); + + return ret; +} +EXPORT_SYMBOL_GPL(spi_offload_prepare); + /*-------------------------------------------------------------------------*/ #if IS_ENABLED(CONFIG_OF_DYNAMIC) diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 5d65a6273dcf..f116dfc1d52c 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -28,6 +28,8 @@ struct spi_transfer; struct spi_controller_mem_ops; struct spi_controller_mem_caps; struct spi_message; +struct spi_controller_offload_ops; +struct spi_offload; /* * INTERFACES between SPI master-side drivers and SPI slave protocol handlers, @@ -713,6 +715,9 @@ struct spi_controller { const struct spi_controller_mem_ops *mem_ops; const struct spi_controller_mem_caps *mem_caps; + /* Operations for controllers with offload support. */ + const struct spi_controller_offload_ops *offload_ops; + /* GPIO chip select */ struct gpio_desc **cs_gpiods; bool use_gpio_descriptors; @@ -1505,6 +1510,124 @@ 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 via a hardware trigger. + */ + +/** + * SPI_OFFLOAD_RX - placeholder for indicating read transfers for offloads + * + * Assign xfer->rx_buf to this value for any read transfer passed to + * spi_offload_prepare(). This will act as a flag to indicate to the offload + * that it should do something with the data read during this transfer. What + * that something can be is determined by the specific hardware, e.g. it could + * be piped to DMA or a DSP, etc. + */ +#define SPI_OFFLOAD_RX_SENTINEL ((void *)1) + +/** + * 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 { + /** + * @get: Callback to get the offload assigned to the given SPI device. + * Index is an index in the offloads array fwnode property of the device. + * Implementations must return the pointer to the device or a negative + * error code (return -ENODEV rather than NULL if no matching device). + */ + struct spi_offload *(*get)(struct spi_device *spi, unsigned int index); + /** + * @prepare: Callback to prepare the offload for the given SPI message. + * @msg and any of its members (including any xfer->tx_buf) is not + * guaranteed to be valid beyond the lifetime of this call. + */ + int (*prepare)(struct spi_offload *offload, struct spi_message *msg); + /** + * @unprepare: Callback to release any resources used by prepare(). + */ + void (*unprepare)(struct spi_offload *offload); + /** + * @enable: Callback to enable the offload. + */ + int (*enable)(struct spi_offload *offload); + /** + * @disable: Callback to disable the offload. + */ + void (*disable)(struct spi_offload *offload); +}; + +/** struct spi_offload - offload handle */ +struct spi_offload { + /** @controller: The associated SPI controller. */ + struct spi_controller *controller; + /** @dev: The device associated with the offload instance. */ + struct device *dev; + /** @priv: Private instance data used by the SPI controller. */ + void *priv; +}; + +/** + * spi_offload_get - gets an offload assigned to the given SPI device + * @spi: SPI device. + * @index: Index of the offload in the SPI device's fwnode int array. + * + * The lifetime of the returned offload is tied to the struct spi_controller + * instance. Since @spi owns a reference to the controller, most consumers + * should not have to do anything extra. But if the offload is passed somewhere + * outside of the control of the SPI device driver, then an additional reference + * to the controller must be made. + * + * Return: Pointer to the offload handle or negative error code. + */ +static inline struct spi_offload *spi_offload_get(struct spi_device *spi, + unsigned int index) +{ + if (!spi->controller->offload_ops) + return ERR_PTR(-EOPNOTSUPP); + + return spi->controller->offload_ops->get(spi, index); +} + +int spi_offload_prepare(struct spi_offload *offload, struct spi_device *spi, + struct spi_transfer *xfers, unsigned int num_xfers); + +/** + * spi_offload_unprepare - releases any resources used by spi_offload_prepare() + * @offload: The offload instance. + */ +static inline void spi_offload_unprepare(struct spi_offload *offload) +{ + offload->controller->offload_ops->unprepare(offload); +} + +/** + * spi_offload_enable - enables the offload + * @offload: The offload instance. + * Return: 0 on success or negative error code. + */ +static inline int spi_offload_enable(struct spi_offload *offload) +{ + return offload->controller->offload_ops->enable(offload); +} + +/** + * spi_offload_disable - disables the offload + * @offload: The offload instance. + */ +static inline void spi_offload_disable(struct spi_offload *offload) +{ + offload->controller->offload_ops->disable(offload); +} + +/*---------------------------------------------------------------------------*/ + /* * INTERFACE between board init code and SPI infrastructure. *
This adds a feature for specialized SPI controllers that can record a series of SPI transfers, including tx data, cs assertions, delays, etc. and then play them back using a hardware trigger without CPU intervention. The intended use case for this is with the AXI SPI Engine to capture data from ADCs at high rates (MSPS) with a stable sample period. Most of the implementation is controller-specific and will be handled by drivers that implement the offload_ops callbacks. The API follows a prepare/enable pattern that should be familiar to users of the clk subsystem. Consumers of this API will make calls similar to this: /* in probe() */ offload = spi_offload_get(spi, 0); ... /* in some setup function */ ret = spi_offload_prepare(offload, xfers, ARRAY_SIZE(xfers)); ... /* in some enable function */ ret = spi_offload_enable(offload); ... /* in corresponding disable function */ spi_offload_disable(offload); ... /* in corresponding teardown function */ spi_offload_unprepare(offload); ... Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/spi/spi.c | 39 +++++++++++++++ include/linux/spi/spi.h | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+)