| Message ID | 20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com |
|---|---|
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Fri, 10 May 2024 19:44:31 -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> > --- > > 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. > > RFC: This isn't very polished yet, just FYI. A few things to sort out: > > Rather than making the buffer either triggered buffer or hardware buffer, > I'm considering allowing both, e.g. buffer0 will always be the triggered > buffer and buffer1 will will be the hardware buffer if connected to a SPI > controller with offload support, otherwise buffer1 is absent. But since > multiple buffers haven't been used much so far, more investigation is > needed to see how that would work in practice. If we do that though, then > we would always have the sampling_frequency attribute though even though > it only applies to one buffer. Why would someone who has this nice IP in the path want the conventional triggered buffer? I'm not against the two buffer option, but I'd like to know the reasoning not to just provide the hardware buffer if this SPI offload is available. I can conjecture reasons but would like you to write them out for me :) This feels like if someone has paid for the expensive hardware they probably only want the best performance. Jonathan > --- > drivers/iio/adc/ad7944.c | 147 +++++++++++++++++++++++++++++++++++------------ > 1 file changed, 111 insertions(+), 36 deletions(-) > > diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c > index 4602ab5ed2a6..6724d6c92778 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> > > @@ -65,6 +67,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 > @@ -123,6 +127,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),\ > }, \ > IIO_CHAN_SOFT_TIMESTAMP(1), \ > }, \ > @@ -134,18 +139,12 @@ AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 0); > /* fully differential */ > AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 1); > > -static void ad7944_unoptimize_msg(void *msg) > -{ > - spi_unoptimize_message(msg); > -} > - > -static int ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *adc, > - const struct iio_chan_spec *chan) > +static void ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *adc, > + const struct iio_chan_spec *chan) > { > unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns > : adc->timing_spec->conv_ns; > struct spi_transfer *xfers = adc->xfers; > - int ret; > > /* > * NB: can get better performance from some SPI controllers if we use > @@ -174,21 +173,14 @@ static int ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc * > xfers[2].bits_per_word = chan->scan_type.realbits; > > spi_message_init_with_transfers(&adc->msg, xfers, 3); > - > - ret = spi_optimize_message(adc->spi, &adc->msg); > - if (ret) > - return ret; > - > - return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg); > } > > -static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc, > - const struct iio_chan_spec *chan) > +static void ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc, > + const struct iio_chan_spec *chan) > { > unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns > : adc->timing_spec->conv_ns; > struct spi_transfer *xfers = adc->xfers; > - int ret; > > /* > * NB: can get better performance from some SPI controllers if we use > @@ -208,12 +200,6 @@ static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc > xfers[1].bits_per_word = chan->scan_type.realbits; > > spi_message_init_with_transfers(&adc->msg, xfers, 2); > - > - ret = spi_optimize_message(adc->spi, &adc->msg); > - if (ret) > - return ret; > - > - return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg); > } > > static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc, > @@ -345,6 +331,30 @@ static int ad7944_read_raw(struct iio_dev *indio_dev, > return -EINVAL; > } > > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (!adc->trigger_clk) > + return -EOPNOTSUPP; > + > + *val = clk_get_rate(adc->trigger_clk); > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > +} > + > +static int ad7944_write_raw(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + int val, int val2, long info) > +{ > + struct ad7944_adc *adc = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (!adc->trigger_clk) > + return -EOPNOTSUPP; > + > + return clk_set_rate(adc->trigger_clk, val); > default: > return -EINVAL; > } > @@ -352,6 +362,28 @@ static int ad7944_read_raw(struct iio_dev *indio_dev, > > static const struct iio_info ad7944_iio_info = { > .read_raw = &ad7944_read_raw, > + .write_raw = &ad7944_write_raw, > +}; > + > +static int ad7944_offload_ex_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad7944_adc *adc = iio_priv(indio_dev); > + > + return spi_offload_hw_trigger_enable(adc->spi, 0); > +} > + > +static int ad7944_offload_ex_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct ad7944_adc *adc = iio_priv(indio_dev); > + > + spi_offload_hw_trigger_disable(adc->spi, 0); > + > + return 0; > +} > + > +static const struct iio_buffer_setup_ops ad7944_offload_ex_buffer_setup_ops = { > + .postenable = &ad7944_offload_ex_buffer_postenable, > + .predisable = &ad7944_offload_ex_buffer_predisable, > }; > > static irqreturn_t ad7944_trigger_handler(int irq, void *p) > @@ -471,6 +503,18 @@ static void ad7944_ref_disable(void *ref) > regulator_disable(ref); > } > > +static void ad7944_offload_unprepare(void *p) > +{ > + struct ad7944_adc *adc = p; > + > + spi_offload_unprepare(adc->spi, 0, &adc->msg); > +} > + > +static void ad7944_unoptimize_msg(void *msg) > +{ > + spi_unoptimize_message(msg); > +} > + > static int ad7944_probe(struct spi_device *spi) > { > const struct ad7944_chip_info *chip_info; > @@ -603,16 +647,10 @@ static int ad7944_probe(struct spi_device *spi) > > switch (adc->spi_mode) { > case AD7944_SPI_MODE_DEFAULT: > - ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]); > - if (ret) > - return ret; > - > + ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]); > break; > case AD7944_SPI_MODE_SINGLE: > - ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]); > - if (ret) > - return ret; > - > + ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]); > break; > case AD7944_SPI_MODE_CHAIN: > ret = device_property_read_u32(dev, "#daisy-chained-devices", > @@ -649,11 +687,48 @@ static int ad7944_probe(struct spi_device *spi) > indio_dev->num_channels = ARRAY_SIZE(chip_info->channels); > } > > - ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > - iio_pollfunc_store_time, > - ad7944_trigger_handler, NULL); > - if (ret) > - return ret; > + if (device_property_present(dev, "spi-offloads")) { > + /* TODO: make this a parameter to ad7944_3wire_cs_mode_init_msg() */ > + /* FIXME: wrong index for 4-wire mode */ > + adc->xfers[2].rx_buf = NULL; > + adc->xfers[2].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM; > + > + ret = spi_offload_prepare(adc->spi, 0, &adc->msg); > + if (ret) > + return dev_err_probe(dev, ret, "failed to prepare offload\n"); > + > + ret = devm_add_action_or_reset(dev, ad7944_offload_unprepare, adc); > + if (ret) > + return ret; > + > + adc->trigger_clk = devm_clk_get_enabled(dev, "trigger"); > + if (IS_ERR(adc->trigger_clk)) > + return dev_err_probe(dev, PTR_ERR(adc->trigger_clk), > + "failed to get trigger clk\n"); > + > + ret = devm_iio_dmaengine_buffer_setup(dev, indio_dev, "rx"); > + if (ret) > + return ret; > + > + indio_dev->setup_ops = &ad7944_offload_ex_buffer_setup_ops; > + /* offload can't have soft timestamp */ > + indio_dev->num_channels--; > + } else { > + ret = spi_optimize_message(adc->spi, &adc->msg); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg); > + if (ret) > + return ret; > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + iio_pollfunc_store_time, > + ad7944_trigger_handler, > + NULL); > + if (ret) > + return ret; > + } > > return devm_iio_device_register(dev, indio_dev); > } >
On Sat, 11 May 2024 13:41:09 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Sat, May 11, 2024 at 11:58 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 10 May 2024 19:44:31 -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> > > > --- > > > > > > 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. > > > > > > RFC: This isn't very polished yet, just FYI. A few things to sort out: > > > > > > Rather than making the buffer either triggered buffer or hardware buffer, > > > I'm considering allowing both, e.g. buffer0 will always be the triggered > > > buffer and buffer1 will will be the hardware buffer if connected to a SPI > > > controller with offload support, otherwise buffer1 is absent. But since > > > multiple buffers haven't been used much so far, more investigation is > > > needed to see how that would work in practice. If we do that though, then > > > we would always have the sampling_frequency attribute though even though > > > it only applies to one buffer. > > > > Why would someone who has this nice IP in the path want the conventional > > triggered buffer? I'm not against the two buffer option, but I'd like to know > > the reasoning not to just provide the hardware buffer if this SPI offload > > is available. > > > > I can conjecture reasons but would like you to write them out for me :) > > This feels like if someone has paid for the expensive hardware they probably > > only want the best performance. > > > > For me, it was more of a question of if we need to keep the userspace > interface consistent between both with or without offload support. But > if you are happy with it this way where we have only one or the other, > it is less work for me. :-) So inconsistency in userspace interfaces can occur for many reasons like whether the interrupt is wired or not, but in this particularly case I guess we have ABI stability issue because there are boards out there today and people using the driver without this offload functionality. I'd not really thought that bit through, so I think you are correct that we need to maintain the triggered buffer interface and 'add' the new ABI for the offloaded case. The multibuffer approach should work for this. Will be interesting if any problem surface from having two very different types of buffer on the same device. Jonathan
On Sun, May 12, 2024 at 6:52 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 11 May 2024 13:41:09 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Sat, May 11, 2024 at 11:58 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Fri, 10 May 2024 19:44:31 -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> > > > > --- > > > > > > > > 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. > > > > > > > > RFC: This isn't very polished yet, just FYI. A few things to sort out: > > > > > > > > Rather than making the buffer either triggered buffer or hardware buffer, > > > > I'm considering allowing both, e.g. buffer0 will always be the triggered > > > > buffer and buffer1 will will be the hardware buffer if connected to a SPI > > > > controller with offload support, otherwise buffer1 is absent. But since > > > > multiple buffers haven't been used much so far, more investigation is > > > > needed to see how that would work in practice. If we do that though, then > > > > we would always have the sampling_frequency attribute though even though > > > > it only applies to one buffer. > > > > > > Why would someone who has this nice IP in the path want the conventional > > > triggered buffer? I'm not against the two buffer option, but I'd like to know > > > the reasoning not to just provide the hardware buffer if this SPI offload > > > is available. > > > > > > I can conjecture reasons but would like you to write them out for me :) > > > This feels like if someone has paid for the expensive hardware they probably > > > only want the best performance. > > > > > > > For me, it was more of a question of if we need to keep the userspace > > interface consistent between both with or without offload support. But > > if you are happy with it this way where we have only one or the other, > > it is less work for me. :-) > > So inconsistency in userspace interfaces can occur for many reasons like > whether the interrupt is wired or not, but in this particularly > case I guess we have ABI stability issue because there are boards out there > today and people using the driver without this offload functionality. FWIW, the ad7944 driver will be landing in 6.10, so no users to speak of yet. > I'd not really thought that bit through, so I think you are correct that > we need to maintain the triggered buffer interface and 'add' the new > ABI for the offloaded case. The multibuffer approach should work for this. > Will be interesting if any problem surface from having two very different > types of buffer on the same device. > In this particular case, I think the issues will be: 1. The hardware buffer can't allow the soft timestamp. Otherwise, the buffer layout is the same (on other chips, we won't always be so lucky). 2. The hardware trigger (sampling_frequency attribute) will only apply if there is the SPI offload support.
On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote: > This adds a new property to the spi-peripheral-props binding for use > with peripherals connected to controllers that support offloading. > > Here, offloading means that the controller has the ability to perform > complex SPI transactions without CPU intervention in some shape or form. > > This property will be used to assign controller offload resources to > each peripheral that needs them. What these resources are will be > defined by each specific controller binding. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v2 changes: > > In v1, instead of generic SPI bindings, there were only controller- > specific bindings, so this is a new patch. > > In the previous version I also had an offloads object node that described > what the offload capabilities were but it was suggested that this was > not necessary/overcomplicated. So I've gone to the other extreme and > made it perhaps over-simplified now by requiring all information about > how each offload is used to be encoded in a single u32. The property is a u32-array, so I guess, not a single u32? > We could of course consider using #spi-offload-cells instead for > allowing encoding multiple parameters for each offload instance if that > would be preferable. A -cells property was my gut reaction to what you'd written here and seems especially appropriate if there's any likelihood of some future device using some external resources for spi-offloading. However, -cells properties go in providers, not consumers, so it wouldn't end up in spi-periph-props.yaml, but rather in the controller binding, and instead there'd be a cell array type property in here. I think you know that though and I'm interpreting what's been written rather than what you meant. > I also considered adding spi-offload-names that could be used as sort > of a compatible string (more of an interface name really) in case some > peripherals may want to support more than 1 specialized type of offload. > --- > .../devicetree/bindings/spi/spi-peripheral-props.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > index 15938f81fdce..32991a2d2264 100644 > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > @@ -113,6 +113,16 @@ properties: > minItems: 2 > maxItems: 4 > > + spi-offloads: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: > + Array of controller offload instances that are reserved for use by the > + peripheral device. The semantic meaning of the values of the array > + elements is defined by the controller. For example, it could be a simple > + 0-based index of the offload instance, or it could be a bitfield where > + a few bits represent the assigned hardware trigger, a few bits represent > + the assigned RX stream, etc. > + > st,spi-midi-ns: > description: | > Only for STM32H7, (Master Inter-Data Idleness) minimum time > > -- > 2.43.2 >
On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote: > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote: > > This adds a new property to the spi-peripheral-props binding for use > > with peripherals connected to controllers that support offloading. > > > > Here, offloading means that the controller has the ability to perform > > complex SPI transactions without CPU intervention in some shape or form. > > > > This property will be used to assign controller offload resources to > > each peripheral that needs them. What these resources are will be > > defined by each specific controller binding. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > > > v2 changes: > > > > In v1, instead of generic SPI bindings, there were only controller- > > specific bindings, so this is a new patch. > > > > In the previous version I also had an offloads object node that described > > what the offload capabilities were but it was suggested that this was > > not necessary/overcomplicated. So I've gone to the other extreme and > > made it perhaps over-simplified now by requiring all information about > > how each offload is used to be encoded in a single u32. > > The property is a u32-array, so I guess, not a single u32? It is an array to handle cases where a peripheral might need more than one offload. But the idea was it put everything about each individual offload in a single u32. e.g. 0x0101 could be offload 1 with hardware trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it needed to select between both triggers at runtime. > > > We could of course consider using #spi-offload-cells instead for > > allowing encoding multiple parameters for each offload instance if that > > would be preferable. > > A -cells property was my gut reaction to what you'd written here and > seems especially appropriate if there's any likelihood of some future > device using some external resources for spi-offloading. > However, -cells properties go in providers, not consumers, so it wouldn't > end up in spi-periph-props.yaml, but rather in the controller binding, > and instead there'd be a cell array type property in here. I think you > know that though and I'm interpreting what's been written rather than > what you meant. Indeed you guess correctly. So the next question is if it should be the kind of #-cells that implies a phandle like most providers or without phandles like #address-cells? Asking because I got pushback on v1 for using a phandle with offloads (although in that case, the phandle was for the offload instance itself instead for the SPI controller, so maybe this is different in this case?). > > > I also considered adding spi-offload-names that could be used as sort > > of a compatible string (more of an interface name really) in case some > > peripherals may want to support more than 1 specialized type of offload. > > --- > > .../devicetree/bindings/spi/spi-peripheral-props.yaml | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > index 15938f81fdce..32991a2d2264 100644 > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > @@ -113,6 +113,16 @@ properties: > > minItems: 2 > > maxItems: 4 > > > > + spi-offloads: > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > + description: > > + Array of controller offload instances that are reserved for use by the > > + peripheral device. The semantic meaning of the values of the array > > + elements is defined by the controller. For example, it could be a simple > > + 0-based index of the offload instance, or it could be a bitfield where > > + a few bits represent the assigned hardware trigger, a few bits represent > > + the assigned RX stream, etc. > > + > > st,spi-midi-ns: > > description: | > > Only for STM32H7, (Master Inter-Data Idleness) minimum time > > > > -- > > 2.43.2 > >
On Tue, May 14, 2024 at 1:46 PM Conor Dooley <conor@kernel.org> wrote: > > On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote: > > On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote: > > > > This adds a new property to the spi-peripheral-props binding for use > > > > with peripherals connected to controllers that support offloading. > > > > > > > > Here, offloading means that the controller has the ability to perform > > > > complex SPI transactions without CPU intervention in some shape or form. > > > > > > > > This property will be used to assign controller offload resources to > > > > each peripheral that needs them. What these resources are will be > > > > defined by each specific controller binding. > > > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > --- > > > > > > > > v2 changes: > > > > > > > > In v1, instead of generic SPI bindings, there were only controller- > > > > specific bindings, so this is a new patch. > > > > > > > > In the previous version I also had an offloads object node that described > > > > what the offload capabilities were but it was suggested that this was > > > > not necessary/overcomplicated. So I've gone to the other extreme and > > > > made it perhaps over-simplified now by requiring all information about > > > > how each offload is used to be encoded in a single u32. > > > > > > The property is a u32-array, so I guess, not a single u32? > > > > It is an array to handle cases where a peripheral might need more than > > one offload. But the idea was it put everything about each individual > > offload in a single u32. e.g. 0x0101 could be offload 1 with hardware > > trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then > > a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it > > needed to select between both triggers at runtime. > > > > > > > > > We could of course consider using #spi-offload-cells instead for > > > > allowing encoding multiple parameters for each offload instance if that > > > > would be preferable. > > > > > > A -cells property was my gut reaction to what you'd written here and > > > seems especially appropriate if there's any likelihood of some future > > > device using some external resources for spi-offloading. > > > However, -cells properties go in providers, not consumers, so it wouldn't > > > end up in spi-periph-props.yaml, but rather in the controller binding, > > > and instead there'd be a cell array type property in here. I think you > > > know that though and I'm interpreting what's been written rather than > > > what you meant. > > > > Indeed you guess correctly. So the next question is if it should be > > the kind of #-cells that implies a phandle like most providers or > > without phandles like #address-cells? > > I'm trying to understand if the offload could ever refer to something > beyond the controller that you'd need the phandle for. I think it would > be really helpful to see an example dt of a non-trivial example for how > this would work. The example in the ad7944 patch has a stub controller > node & the clocks/dmas in the peripheral node so it is difficult to > reason about the spi-offloads property there. The fully implemented and tested version of the .dts corresponding to the hardware pictured in the cover letter can be found at [1]. [1]: https://github.com/dlech/linux/blob/axi-spi-engine-offload-v2/arch/arm/boot/dts/xilinx/zynq-zed-adv7511-ad7986.dts To be clear though, the idea that I am proposing here is that if there is something beyond the SPI controller directly connected to the offload, then we would add those things in the peripheral node along with the spi-offloads property that specifies the offload those other things are connected to. Tangent on phandle vs. no phandle: If we add #spi-offload-cells, I would expect that it would always be in the SPI controller node. And the consumers would always be SPI peripheral nodes. So having a phandle seems redundant since it would always point to the controller which is the parent of the peripheral. example_spi: spi { ... #spi-offload-cells = <1>; adc@0 { ... /* fine, but not sure phandle is needed since it always the parent */ spi-offloads = <&example_spi 0>; }; }; spi { ... #spi-offload-cells = <1>; adc@0 { ... /* simpler is better? */ spi-offloads = <0>; }; }; Back to "something beyond the SPI controller": Here are some examples of how I envision this would work. Let's suppose we have a SPI controller that has some sort of offload capability with a configurable trigger source. The trigger can either be an internal software trigger (i.e. writing a register of the SPI controller) or and external trigger (i.e. a input signal from a pin on the SoC). The SPI controller has a lookup table with 8 slots where it can store a series of SPI commands that can be played back by asserting the trigger (this is what provides the "offloading"). So this SPI controller would have #spi-offload-cells = <2>; where the first cell would be the index in the lookup table 0 to 7 and the second cell would be the trigger source 0 for software or 1 for hardware. Application 1: a network controller This could use two offloads, one for TX and one for RX. For TX, we use the first slot with a software trigger because the data is coming from Linux. For RX we use the second slot with a hardware trigger since data is coming from the network controller (i.e. a data ready signal that would normally be wired to a gpio for interrupt but wired to the SPI offload trigger input pin instead). So the peripheral bindings would be: #define SOFTWARE_TRIGGER 0 #define HARDWARE_TRIGGER 1 can@0 { ... spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>; /* maybe we need names too? */ spi-offload-names = "tx", "rx"; }; In this case, there is nothing extra beyond the SPI controller and the network controller, so no extra bindings beyond this are needed. Application 2: an advanced ADC + FPGA This is basically the same as the ad7944 case seen already with one extra feature. In this case, the sample data also contains a CRC byte for error checking. So instead of SPI RX data going directly to DMA, the FPGA removes the CRC byte from the data stream an only the sample data goes to the DMA buffer. The CRC is checked and if bad, an interrupt is asserted. Since this is an FPGA, most everything is hardwired rather than having any kind of mux selection so #spi-offload-cells = <1>; for this controller. By adding spi-offloads to the peripheral node, it also extends the peripheral binding to include the additional properties needed for the extra features provided by the FPGA. In other words, we are saying this DT node now represents the ADC chip plus everything connected to the offload instance used by the ADC chip. adc@0 { ... spi-offloads = <0>; dmas = <&dma 0>; /* channel receiving split out sample data */ dma-names = "rx"; interrupts = <&intc 99>; /* interrupt for bad CRC */ interrupt-names = "crc"; }; > > > Asking because I got pushback on > > v1 for using a phandle with offloads (although in that case, the > > phandle was for the offload instance itself instead for the SPI > > controller, so maybe this is different in this case?). > > Do you have a link to this v1 pushback? Hmm... maybe that was from some internal review before v1 that I was remembering and confusing with the resistance of different aspects you mention below. > I had looked at the v1's binding > comments and didn't see that type of property being resisted - although > I did see some resistance to the spi peripheral node containing any of > the information about the offloads it had been assigned and instead > doing that mapping in the controller so that the cs was sufficient. I > don't think that'd work with the scenario you describe above though > where there could be two different triggers per device tho. I think most of the objection was to having an offloads object node with offload@ subnodes in the SPI controller node along side the peripheral nodes.
Yo, Sorry for the delay, long reply deserved some time to sit and think about it. On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > On Tue, May 14, 2024 at 1:46 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote: > > > On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote: > > > > > This adds a new property to the spi-peripheral-props binding for use > > > > > with peripherals connected to controllers that support offloading. > > > > > > > > > > Here, offloading means that the controller has the ability to perform > > > > > complex SPI transactions without CPU intervention in some shape or form. > > > > > > > > > > This property will be used to assign controller offload resources to > > > > > each peripheral that needs them. What these resources are will be > > > > > defined by each specific controller binding. > > > > > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > > --- > > > > > > > > > > v2 changes: > > > > > > > > > > In v1, instead of generic SPI bindings, there were only controller- > > > > > specific bindings, so this is a new patch. > > > > > > > > > > In the previous version I also had an offloads object node that described > > > > > what the offload capabilities were but it was suggested that this was > > > > > not necessary/overcomplicated. So I've gone to the other extreme and > > > > > made it perhaps over-simplified now by requiring all information about > > > > > how each offload is used to be encoded in a single u32. > > > > > > > > The property is a u32-array, so I guess, not a single u32? > > > > > > It is an array to handle cases where a peripheral might need more than > > > one offload. But the idea was it put everything about each individual > > > offload in a single u32. e.g. 0x0101 could be offload 1 with hardware > > > trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then > > > a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it > > > needed to select between both triggers at runtime. > > > > > > > > > > > > We could of course consider using #spi-offload-cells instead for > > > > > allowing encoding multiple parameters for each offload instance if that > > > > > would be preferable. > > > > > > > > A -cells property was my gut reaction to what you'd written here and > > > > seems especially appropriate if there's any likelihood of some future > > > > device using some external resources for spi-offloading. > > > > However, -cells properties go in providers, not consumers, so it wouldn't > > > > end up in spi-periph-props.yaml, but rather in the controller binding, > > > > and instead there'd be a cell array type property in here. I think you > > > > know that though and I'm interpreting what's been written rather than > > > > what you meant. > > > > > > Indeed you guess correctly. So the next question is if it should be > > > the kind of #-cells that implies a phandle like most providers or > > > without phandles like #address-cells? > > > > I'm trying to understand if the offload could ever refer to something > > beyond the controller that you'd need the phandle for. I think it would > > be really helpful to see an example dt of a non-trivial example for how > > this would work. The example in the ad7944 patch has a stub controller > > node & the clocks/dmas in the peripheral node so it is difficult to > > reason about the spi-offloads property there. > > The fully implemented and tested version of the .dts corresponding to > the hardware pictured in the cover letter can be found at [1]. > > [1]: https://github.com/dlech/linux/blob/axi-spi-engine-offload-v2/arch/arm/boot/dts/xilinx/zynq-zed-adv7511-ad7986.dts Unfortunately this is a trivial example, so there's not much to be gained in new information from the example in the bindings :/ Your examples below are good though, which makes up for that and more. > To be clear though, the idea that I am proposing here is that if there > is something beyond the SPI controller directly connected to the > offload, then we would add those things in the peripheral node along > with the spi-offloads property that specifies the offload those other > things are connected to. > > Tangent on phandle vs. no phandle: Yeah, I think not having a phandle makes sense based on what you've said. > Back to "something beyond the SPI controller": > > Here are some examples of how I envision this would work. > > Let's suppose we have a SPI controller that has some sort of offload > capability with a configurable trigger source. The trigger can either > be an internal software trigger (i.e. writing a register of the SPI > controller) or and external trigger (i.e. a input signal from a pin on > the SoC). The SPI controller has a lookup table with 8 slots where it > can store a series of SPI commands that can be played back by > asserting the trigger (this is what provides the "offloading"). > > So this SPI controller would have #spi-offload-cells = <2>; where the > first cell would be the index in the lookup table 0 to 7 and the > second cell would be the trigger source 0 for software or 1 for > hardware. > > Application 1: a network controller > > This could use two offloads, one for TX and one for RX. For TX, we use > the first slot with a software trigger because the data is coming from > Linux. For RX we use the second slot with a hardware trigger since > data is coming from the network controller (i.e. a data ready signal > that would normally be wired to a gpio for interrupt but wired to the > SPI offload trigger input pin instead). So the peripheral bindings > would be: > > #define SOFTWARE_TRIGGER 0 > #define HARDWARE_TRIGGER 1 > > can@0 { > ... > spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>; > /* maybe we need names too? */ > spi-offload-names = "tx", "rx"; > }; > > In this case, there is nothing extra beyond the SPI controller and the > network controller, so no extra bindings beyond this are needed. > > Application 2: an advanced ADC + FPGA > > This is basically the same as the ad7944 case seen already with one > extra feature. In this case, the sample data also contains a CRC byte > for error checking. So instead of SPI RX data going directly to DMA, > the FPGA removes the CRC byte from the data stream an only the sample > data goes to the DMA buffer. The CRC is checked and if bad, an > interrupt is asserted. > > Since this is an FPGA, most everything is hardwired rather than having > any kind of mux selection so #spi-offload-cells = <1>; for this > controller. > > By adding spi-offloads to the peripheral node, it also extends the > peripheral binding to include the additional properties needed for the > extra features provided by the FPGA. In other words, we are saying > this DT node now represents the ADC chip plus everything connected to > the offload instance used by the ADC chip. It seems very strange to me that the dmas and the clock triggers are going into the spi device nodes. The description is | + dmas: | + maxItems: 1 | + description: RX DMA Channel for receiving a samples from the SPI offload. But as far as I can tell this device is in a package of its own and not some IP provided by Analog that an engine on the FPGA can actually do DMA to, and the actual connection of the device is "just" SPI. The dmas and clock triggers etc appear to be resources belonging to the controller that can "assigned" to a particular spi device. If the adc gets disconnected from the system, the dmas and clock triggers are still connected to the spi controller/offload engine, they don't end up n/c, right? (Well maybe they would in the case of a fancy SPI device that provides it's own sampling clock or w/e, but then it'd be a clock provider of sorts). I'd be expecting the spi-offloads property to be responsible for selecting which of the various resources belonging to the controller are to be used by a device. Maybe it overcomplicates the shit out of things and Rob or Mark are gonna start screaming at me but w/e, looking at it from the point of view of how the hardware is laid out (or at least how it is described in your FPGA case above) the dma/clock properties looks like they're misplaced. IOW, I don't think that adding the spi-offloads property should convert a node from representing an ADC in a qfn-20 or w/e to "the ADC chip plus everything connected to the offload instance used by the ADC chip". > adc@0 { > ... > spi-offloads = <0>; > dmas = <&dma 0>; /* channel receiving split out sample data */ > dma-names = "rx"; > interrupts = <&intc 99>; /* interrupt for bad CRC */ > interrupt-names = "crc"; > }; > > > > > > Asking because I got pushback on > > > v1 for using a phandle with offloads (although in that case, the > > > phandle was for the offload instance itself instead for the SPI > > > controller, so maybe this is different in this case?). > > > > Do you have a link to this v1 pushback? > > Hmm... maybe that was from some internal review before v1 that I was > remembering and confusing with the resistance of different aspects you > mention below. > > > I had looked at the v1's binding > > comments and didn't see that type of property being resisted - although > > I did see some resistance to the spi peripheral node containing any of > > the information about the offloads it had been assigned and instead > > doing that mapping in the controller so that the cs was sufficient. I > > don't think that'd work with the scenario you describe above though > > where there could be two different triggers per device tho. > > I think most of the objection was to having an offloads object node > with offload@ subnodes in the SPI controller node along side the > peripheral nodes. I dunno, that was my reading of Rob's comments at least. I know he had more than one objection though, so maybe we're just looking at different portions of it - I did note that you removed the offload@ though. Cheers, Conor.
On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > > > > > Back to "something beyond the SPI controller": > > > > > > > > Here are some examples of how I envision this would work. > > > > > > > > Let's suppose we have a SPI controller that has some sort of offload > > > > capability with a configurable trigger source. The trigger can either > > > > be an internal software trigger (i.e. writing a register of the SPI > > > > controller) or and external trigger (i.e. a input signal from a pin on > > > > the SoC). The SPI controller has a lookup table with 8 slots where it > > > > can store a series of SPI commands that can be played back by > > > > asserting the trigger (this is what provides the "offloading"). > > > > > > > > So this SPI controller would have #spi-offload-cells = <2>; where the > > > > first cell would be the index in the lookup table 0 to 7 and the > > > > second cell would be the trigger source 0 for software or 1 for > > > > hardware. > > > > > > > > Application 1: a network controller > > > > > > > > This could use two offloads, one for TX and one for RX. For TX, we use > > > > the first slot with a software trigger because the data is coming from > > > > Linux. For RX we use the second slot with a hardware trigger since > > > > data is coming from the network controller (i.e. a data ready signal > > > > that would normally be wired to a gpio for interrupt but wired to the > > > > SPI offload trigger input pin instead). So the peripheral bindings > > > > would be: > > > > > > > > #define SOFTWARE_TRIGGER 0 > > > > #define HARDWARE_TRIGGER 1 > > > > > > > > can@0 { > > > > ... > > > > spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>; > > > > /* maybe we need names too? */ > > > > spi-offload-names = "tx", "rx"; > > > > }; > > > > > > > > In this case, there is nothing extra beyond the SPI controller and the > > > > network controller, so no extra bindings beyond this are needed. > > > > > > > > Application 2: an advanced ADC + FPGA > > > > > > > > This is basically the same as the ad7944 case seen already with one > > > > extra feature. In this case, the sample data also contains a CRC byte > > > > for error checking. So instead of SPI RX data going directly to DMA, > > > > the FPGA removes the CRC byte from the data stream an only the sample > > > > data goes to the DMA buffer. The CRC is checked and if bad, an > > > > interrupt is asserted. > > > > > > > > Since this is an FPGA, most everything is hardwired rather than having > > > > any kind of mux selection so #spi-offload-cells = <1>; for this > > > > controller. > > > > > > > > By adding spi-offloads to the peripheral node, it also extends the > > > > peripheral binding to include the additional properties needed for the > > > > extra features provided by the FPGA. In other words, we are saying > > > > this DT node now represents the ADC chip plus everything connected to > > > > the offload instance used by the ADC chip. > > > > > > It seems very strange to me that the dmas and the clock triggers are > > > going into the spi device nodes. The description is > > > | + dmas: > > > | + maxItems: 1 > > > | + description: RX DMA Channel for receiving a samples from the SPI offload. > > > But as far as I can tell this device is in a package of its own and not > > > some IP provided by Analog that an engine on the FPGA can actually do > > > DMA to, and the actual connection of the device is "just" SPI. > > > The dmas and clock triggers etc appear to be resources belonging to the > > > controller that can "assigned" to a particular spi device. If the adc > > > gets disconnected from the system, the dmas and clock triggers are still > > > connected to the spi controller/offload engine, they don't end up n/c, > > > right? (Well maybe they would in the case of a fancy SPI device that > > > provides it's own sampling clock or w/e, but then it'd be a clock > > > provider of sorts). I'd be expecting the spi-offloads property to be > > > responsible for selecting which of the various resources belonging to > > > the controller are to be used by a device. > > > Maybe it overcomplicates the shit out of things and Rob or Mark are > > > gonna start screaming at me but w/e, looking at it from the point of > > > view of how the hardware is laid out (or at least how it is described > > > in your FPGA case above) the dma/clock properties looks like they're > > > misplaced. IOW, I don't think that adding the spi-offloads property > > > should convert a node from representing an ADC in a qfn-20 or w/e > > > to "the ADC chip plus everything connected to the offload instance > > > used by the ADC chip". > > > > This is the same reasoning that led me to the binding proposed in v1. > > Rob suggested that these extras (dmas/clocks) should just be > > properties directly of the SPI controller. > > > But the issue I have with > > that is that since this is an FPGA, these properties are not fixed. > > I don't think whether or not we're talking about an FPGA or an ASIC > matters at all here to be honest. In my view the main thing that FPGA > impact in terms of bindings is the number of properties required to > represent the configurability of a particular IP. Sure, you're gonna > have to change the dts around in a way you'll never have to with an > ASIC, but the description of individual devices or relations between > them doesn't change. > > > Maybe there are more clocks or no clocks or interrupts or something we > > didn't think of yet. > > This could happen with a new generation of an ASIC and is not specific > to an FPGA IP core. Even with FPGA IP, while there may be lots of > different configuration parameters, they are known & limited - you can > look at the IP's documentation (or failing that, the HDL) to figure out > what the points of variability are. It's not particularly difficult to > account for there being a configurable number of clocks or interrupts. > For "something we didn't think of yet" to be a factor, someone has to > think of it and add it to the IP core, and which point we can absolutely > change the bindings and software to account for the revised IP. > > > So it doesn't really seem possible to write a > > binding for the SPI controller node to cover all of these cases. > > I dunno, I don't think your concerns about numbers of clocks (or any > other such property) are unique to this particular use-case. > > > These > > extras are included in the FPGA bitstream only for a specific type of > > peripheral, not for general use of the SPI controller with any type of > > peripheral. > > I accept that, but what I was trying to get across was that you could > configure the FPGA with a bitstream that contains these extra resources > and then connect a peripheral device that does not make use of them at > all. Sure, in this case the peripheral has no spi-offloads property and the SPI controller acts as a typical SPI controller. > Or you could connect a range of different peripherals that do use > them. OK, you've got me thinking about something I hadn't really thought about yet. > Whether or not the resources are there and how they are connected > in the FPGA bitstream is not a property of the device connected to the > SPI controller, only whether or not you use them is. > Even when those things are connected directly to a specific peripheral device? Or not connected to the offload? Here is another potential ADC trigger case to consider. +-------------------------------+ +------------------+ | | | | | SOC/FPGA | | ADC | | +---------------------+ | | | | | AXI SPI Engine | | | | | | SPI Bus ============ SPI Bus | | | | | | | | | +---------------+ | < < < < < BUSY | | | | Offload 0 | | v | | | | | | | | v > > > > CNV | | | | TRIGGER IN < < < ^ | | | | | +---------------+ | ^ | +------------------+ | +---------------------+ ^ | | | AXI PWM | ^ | | | CH0 > > ^ | | +---------------------+ | | | +-------------------------------+ This time, the periodic trigger (PWM) is connected to the pin on the ADC that triggers a sample conversion (CNV). The ADC has a BUSY output that will go high at the start of the conversion and go low at the end of the conversion. The BUSY output of the ADC is wired as the hardware trigger input of the offload. In this case would we still consider the PWM as part of the SPI controller/offload since it can only be used in conjunction with the SPI offload? It isn't connected to it at all though. Another case could be a self-clocked ADC. Here, the ADC has it's own periodic sample trigger on the chip and the RDY output goes high whenever a sample is ready to read. +-------------------------------+ +------------------+ | | | | | SOC/FPGA | | ADC | | +---------------------+ | | | | | AXI SPI Engine | | | | | | SPI Bus ============ SPI Bus | | | | | | | | | +---------------+ | < < < < < RDY | | | | Offload 0 | | v | | | | | | | | v | | | | | | TRIGGER IN < < < | | | | | +---------------+ | | +------------------+ | +---------------------+ | | | +-------------------------------+ If both of these are valid wiring configurations for the same ADC, wouldn't it make more sense to describe this in the peripheral node rather than having to query the controller to see how the peripheral is wired up? > In fact, looking at the documentation for the "spi engine" some more, I > am even less convinced that the right place for describing the offload is > the peripheral as I (finally?) noticed that the registers for the offload > engine are mapped directly into the "spi engine"'s memory map, rather than > it being a entirely separate IP in the FPGA fabric. True, but we don't use these registers, e.g., to configure the sampling frequency of a trigger (if it can even be done). That is done in a completely separate IP block with it's own registers. > > Further, what resources are available depends on what the SPI offload > engine IP is. If my employer decides to start shipping some SPI offload > IP in our catalogue that can work with ADI's ADCs, the set of offload > related properties or their names may well differ. If all of these resources were fairly generic, like the periodic trigger we've been discussing, then I could see the case for trying to accommodate this the same way we do for other common features of SPI controllers. But for cases like "Application 2" that I described previously, these resources can get very specific to a peripheral (like the example given of having a data processing unit that knows about the exact data format and CRC algorithm used by a specific ADC). These seems like too specific of a thing to say that a SPI controller "supports". But, OK, let's go with the idea of putting everything related to the offloads in the SPI controller node an see where it takes us... spi@1000 { compatible = "adi,axi-spi-engine"; #spi-offload-cells = <1>; /* PWM is connected to offload hardware trigger. DMA for streaming sample * data can only handle 16-bit words. IIO hardware buffer will be CPU- * endian because data is streamed one word at a time. */ spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma"; /* pwm properties are only allowed because spi-offload-0-capabilities * contains "pwm-trigger" */ pwms = <&pwm 0>; pwm-names = "offload-0-pwm-trigger"; /* dma properties are only allowed because spi-offload-0-capabilities * contains "16-bit-rx-dma" */ dmas = <&dma 0>; dma-names = "offload-0-16-bit-rx"; ... adc@0 { compatible = "adi,ad7944"; spi-offloads = <0>; ... }; }; spi@2000 { compatible = "not-adi,other-spi-engine"; #spi-offload-cells = <1>; /* Clock is connected to offload hardware trigger. DMA for streaming sample * data can only handle one byte at a time. IIO hardware buffer will be big- * endian because data is streamed one byte at a time. */ spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma"; /* Clock properties are extended because spi-offload-0-capabilities contains * "clock-trigger" and SPI controller itself has a clock */ clocks = <&cpu_clock 0>, <&extra_clock 0>; clock-names = "sclk", "offload-0-pwm-trigger"; /* DMA properties are extended because spi-offload-0-capabilities contains * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */ dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>; dma-names = "tx", "rx", "offload-0-16-bit-rx"; ... adc@0 { compatible = "adi,ad7944"; spi-offloads = <0>; ... }; }; spi@3000 { compatible = "adi,axi-spi-engine"; #spi-offload-cells = <1>; /* Sample ready signal (~BSY) from ADC is connected to offload hardware * trigger. DMA for streaming sample data can only handle 16-bit words. */ spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma"; /* Do we need a property to say that the sample trigger is connected to * adc@0 so that if adc@1 tries to use it, it will fail? */ /* dma properties are only allowed because spi-offload-0-capabilities * contains "16-bit-rx-dma" */ dmas = <&dma 0>; dma-names = "offload-0-16-bit-rx"; ... adc@0 { compatible = "adi,ad7944"; spi-offloads = <0>; ... /* PWM connected to the conversion pin (CNV). This only makes sense * when offload is used with BSY signal, otherwise we would have CNV * connected to SPI CS. */ pwms = <&pwm 0>; pwm-names = "cnv"; }; }; spi@4000 { compatible = "adi,axi-spi-engine"; #spi-offload-cells = <1>; /* Sample ready signal (~BSY) from ADC is connected to offload hardware * trigger. DMA for streaming sample data can only handle 32-bit words. * This also has the CRC validation that strips off the CRC byte of the * raw data before passing the sample to DMA. */ spi-offload-0-capabilities = "sample-trigger", "32-bit-rx-dma-with-ad4630-crc-check"; /* dma properties are only allowed because spi-offload-0-capabilities * contains "16-bit-rx-dma" */ dmas = <&dma 0>; dma-names = "offload-0-16-bit-rx"; interrupt-parent = <&intc>; /* First interrupt is for the SPI controller (always present), second * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check" * of offload 0. */ interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "controller", "offload-0-crc-error"; ... adc@0 { compatible = "adi,ad4630"; spi-offloads = <0>; ... /* PWM connected to the conversion pin (CNV). Without offload, we would * have cnv-gpios instead. */ pwms = <&pwm 0>; pwm-names = "cnv"; }; }; So this is what I came up with of how things could look (combining suggestions from Rob in v1 and Conor's suggestions here). I can see how we can make this work. But the thing I don't like about it is that the peripheral drivers still need to know all of the information about the offload capabilities and need to interact with the pwms/clocks/interrupts/dmas/etc. So this is just adding layers of indirection where all of this stuff has to go through the SPI controller driver. > > > Another idea I had was to perhaps use the recently added IIO backend > > framework for the "extras". The idea there is that we are creating a > > "composite" IIO device that consists of the ADC chip (frontend) plus > > these extra hardware trigger and hardware buffer functions provided by > > the FPGA (backend). > > > > offload_backend: adc0-backend { > > /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */ > > compatible = "adi,pulsar-adc-offload"; > > #io-backend-cells = <0>; > > dmas = <&dma 0>; > > dma-names = "rx"; > > clocks = <&trigger_clock>; > > }; > > > > spi { > > ... > > adc@0 { > > ... > > spi-offloads = <0>; > > io-backends = <&offload_backend>; > > }; > > }; > > > > While this could be a solution for IIO devices, this wouldn't solve > > the issue in general though for SPI offloads used with non-IIO > > peripherals. > > Yeah, I agree that using something specific to IIO isn't really a good > solution. > > Cheers, > Conor. > > > So I don't think it is the right thing to do here. But, I > > think this idea of a "composite" device helps explain why we are > > pushing for putting the "extras" with the peripheral node rather than > > the controller node, at least for the specific case of the AXI SPI > > Engine controller. >
On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > > > > > > > Back to "something beyond the SPI controller": > > > > > > > > > > Here are some examples of how I envision this would work. > > > > > > > > > > Let's suppose we have a SPI controller that has some sort of offload > > > > > capability with a configurable trigger source. The trigger can either > > > > > be an internal software trigger (i.e. writing a register of the SPI > > > > > controller) or and external trigger (i.e. a input signal from a pin on > > > > > the SoC). The SPI controller has a lookup table with 8 slots where it > > > > > can store a series of SPI commands that can be played back by > > > > > asserting the trigger (this is what provides the "offloading"). > > > > > > > > > > So this SPI controller would have #spi-offload-cells = <2>; where the > > > > > first cell would be the index in the lookup table 0 to 7 and the > > > > > second cell would be the trigger source 0 for software or 1 for > > > > > hardware. > > > > > > > > > > Application 1: a network controller > > > > > > > > > > This could use two offloads, one for TX and one for RX. For TX, we use > > > > > the first slot with a software trigger because the data is coming from > > > > > Linux. For RX we use the second slot with a hardware trigger since > > > > > data is coming from the network controller (i.e. a data ready signal > > > > > that would normally be wired to a gpio for interrupt but wired to the > > > > > SPI offload trigger input pin instead). So the peripheral bindings > > > > > would be: > > > > > > > > > > #define SOFTWARE_TRIGGER 0 > > > > > #define HARDWARE_TRIGGER 1 > > > > > > > > > > can@0 { > > > > > ... > > > > > spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>; > > > > > /* maybe we need names too? */ > > > > > spi-offload-names = "tx", "rx"; > > > > > }; > > > > > > > > > > In this case, there is nothing extra beyond the SPI controller and the > > > > > network controller, so no extra bindings beyond this are needed. > > > > > > > > > > Application 2: an advanced ADC + FPGA > > > > > > > > > > This is basically the same as the ad7944 case seen already with one > > > > > extra feature. In this case, the sample data also contains a CRC byte > > > > > for error checking. So instead of SPI RX data going directly to DMA, > > > > > the FPGA removes the CRC byte from the data stream an only the sample > > > > > data goes to the DMA buffer. The CRC is checked and if bad, an > > > > > interrupt is asserted. > > > > > > > > > > Since this is an FPGA, most everything is hardwired rather than having > > > > > any kind of mux selection so #spi-offload-cells = <1>; for this > > > > > controller. > > > > > > > > > > By adding spi-offloads to the peripheral node, it also extends the > > > > > peripheral binding to include the additional properties needed for the > > > > > extra features provided by the FPGA. In other words, we are saying > > > > > this DT node now represents the ADC chip plus everything connected to > > > > > the offload instance used by the ADC chip. > > > > > > > > It seems very strange to me that the dmas and the clock triggers are > > > > going into the spi device nodes. The description is > > > > | + dmas: > > > > | + maxItems: 1 > > > > | + description: RX DMA Channel for receiving a samples from the SPI offload. > > > > But as far as I can tell this device is in a package of its own and not > > > > some IP provided by Analog that an engine on the FPGA can actually do > > > > DMA to, and the actual connection of the device is "just" SPI. > > > > The dmas and clock triggers etc appear to be resources belonging to the > > > > controller that can "assigned" to a particular spi device. If the adc > > > > gets disconnected from the system, the dmas and clock triggers are still > > > > connected to the spi controller/offload engine, they don't end up n/c, > > > > right? (Well maybe they would in the case of a fancy SPI device that > > > > provides it's own sampling clock or w/e, but then it'd be a clock > > > > provider of sorts). I'd be expecting the spi-offloads property to be > > > > responsible for selecting which of the various resources belonging to > > > > the controller are to be used by a device. > > > > Maybe it overcomplicates the shit out of things and Rob or Mark are > > > > gonna start screaming at me but w/e, looking at it from the point of > > > > view of how the hardware is laid out (or at least how it is described > > > > in your FPGA case above) the dma/clock properties looks like they're > > > > misplaced. IOW, I don't think that adding the spi-offloads property > > > > should convert a node from representing an ADC in a qfn-20 or w/e > > > > to "the ADC chip plus everything connected to the offload instance > > > > used by the ADC chip". > > > > > > This is the same reasoning that led me to the binding proposed in v1. > > > Rob suggested that these extras (dmas/clocks) should just be > > > properties directly of the SPI controller. > > > > > But the issue I have with > > > that is that since this is an FPGA, these properties are not fixed. > > > > I don't think whether or not we're talking about an FPGA or an ASIC > > matters at all here to be honest. In my view the main thing that FPGA > > impact in terms of bindings is the number of properties required to > > represent the configurability of a particular IP. Sure, you're gonna > > have to change the dts around in a way you'll never have to with an > > ASIC, but the description of individual devices or relations between > > them doesn't change. > > > > > Maybe there are more clocks or no clocks or interrupts or something we > > > didn't think of yet. > > > > This could happen with a new generation of an ASIC and is not specific > > to an FPGA IP core. Even with FPGA IP, while there may be lots of > > different configuration parameters, they are known & limited - you can > > look at the IP's documentation (or failing that, the HDL) to figure out > > what the points of variability are. It's not particularly difficult to > > account for there being a configurable number of clocks or interrupts. > > For "something we didn't think of yet" to be a factor, someone has to > > think of it and add it to the IP core, and which point we can absolutely > > change the bindings and software to account for the revised IP. > > > > > So it doesn't really seem possible to write a > > > binding for the SPI controller node to cover all of these cases. > > > > I dunno, I don't think your concerns about numbers of clocks (or any > > other such property) are unique to this particular use-case. > > > > > These > > > extras are included in the FPGA bitstream only for a specific type of > > > peripheral, not for general use of the SPI controller with any type of > > > peripheral. > > > > I accept that, but what I was trying to get across was that you could > > configure the FPGA with a bitstream that contains these extra resources > > and then connect a peripheral device that does not make use of them at > > all. > > Sure, in this case the peripheral has no spi-offloads property and the > SPI controller acts as a typical SPI controller. > > > Or you could connect a range of different peripherals that do use > > them. > > OK, you've got me thinking about something I hadn't really thought about yet. > > > Whether or not the resources are there and how they are connected > > in the FPGA bitstream is not a property of the device connected to the > > SPI controller, only whether or not you use them is. > > > > Even when those things are connected directly to a specific peripheral > device? Or not connected to the offload? > > Here is another potential ADC trigger case to consider. > > +-------------------------------+ +------------------+ > | | | | > | SOC/FPGA | | ADC | > | +---------------------+ | | | > | | AXI SPI Engine | | | | > | | SPI Bus ============ SPI Bus | > | | | | | | > | | +---------------+ | < < < < < BUSY | > | | | Offload 0 | | v | | | > | | | | | v > > > > CNV | > | | | TRIGGER IN < < < ^ | | | > | | +---------------+ | ^ | +------------------+ > | +---------------------+ ^ | > | | AXI PWM | ^ | > | | CH0 > > ^ | > | +---------------------+ | > | | > +-------------------------------+ > > This time, the periodic trigger (PWM) is connected to the pin on the > ADC that triggers a sample conversion (CNV). The ADC has a BUSY output > that will go high at the start of the conversion and go low at the end > of the conversion. The BUSY output of the ADC is wired as the hardware > trigger input of the offload. > > In this case would we still consider the PWM as part of the SPI > controller/offload since it can only be used in conjunction with the > SPI offload? It isn't connected to it at all though. No, in this case the ADC is a PWM consumer and the offload engine is not. The ADC is a "trigger" provider and the SPI offload engine is a trigger consumer. > Another case could be a self-clocked ADC. Here, the ADC has it's own > periodic sample trigger on the chip and the RDY output goes high > whenever a sample is ready to read. > > +-------------------------------+ +------------------+ > | | | | > | SOC/FPGA | | ADC | > | +---------------------+ | | | > | | AXI SPI Engine | | | | > | | SPI Bus ============ SPI Bus | > | | | | | | > | | +---------------+ | < < < < < RDY | > | | | Offload 0 | | v | | | > | | | | | v | | | > | | | TRIGGER IN < < < | | | > | | +---------------+ | | +------------------+ > | +---------------------+ | > | | > +-------------------------------+ > > If both of these are valid wiring configurations for the same ADC, > wouldn't it make more sense to describe this in the peripheral node > rather than having to query the controller to see how the peripheral > is wired up? In both of these cases, the offload works in the same way, it gets a trigger from the ADC and acts upon it. I would expect that in this case the ADC driver would look for an optional pwm property in the devicetree and if it is present configure the ADC to use that and if absent do then do whatever configuration is required for self clocking. I would expect that both cases would be handled identically by the SPI [offload] engine side of things, other than inverting the polarity of the trigger. (If I understand correctly, you want to trigger the offload engine on a falling edge of BUSY but a rising edge of RDY). > > In fact, looking at the documentation for the "spi engine" some more, I > > am even less convinced that the right place for describing the offload is > > the peripheral as I (finally?) noticed that the registers for the offload > > engine are mapped directly into the "spi engine"'s memory map, rather than > > it being a entirely separate IP in the FPGA fabric. > > True, but we don't use these registers, e.g., to configure the > sampling frequency of a trigger (if it can even be done). That is done > in a completely separate IP block with it's own registers. Where is the binding for that IP block? I think describing that is pretty key. goto continuation; > > Further, what resources are available depends on what the SPI offload > > engine IP is. If my employer decides to start shipping some SPI offload > > IP in our catalogue that can work with ADI's ADCs, the set of offload > > related properties or their names may well differ. > > If all of these resources were fairly generic, like the periodic > trigger we've been discussing, then I could see the case for trying to > accommodate this the same way we do for other common features of SPI > controllers. But for cases like "Application 2" that I described > previously, these resources can get very specific to a peripheral > (like the example given of having a data processing unit that knows > about the exact data format and CRC algorithm used by a specific ADC). > These seems like too specific of a thing to say that a SPI controller > "supports". To remind myself, "Application 2" featured an offload engine designed specifically to work with a particular data format that would strip a CRC byte and check the validity of the data stream. I think you're right something like that is a stretch to say that that is a feature of the SPI controller - but I still don't believe that modelling it as part of the ADC is correct. I don't fully understand the io-backends and how they work yet, but the features you describe there seem like something that should/could be modelled as one, with its own node and compatible etc. Describing custom RTL stuff ain't always strightforward, but the stuff from Analog is versioned and documented etc so it shouldn't be quite that hard. continuation: If offload engines have their own register region in the memory map, their own resources (the RTL is gonna need at the very least a clock) and potentially also provide other services (like acting as an io-backend type device that pre-processes data) it doesn't sound like either the controller or peripheral nodes are the right place for these properties. And uh, spi-offloads gets a phandle once more... FWIW, I did read these examples but didn't feel it was worth commenting on them given the above. I'll comment on them if they stay "accurate". Cheers, Conor. > But, OK, let's go with the idea of putting everything related to the > offloads in the SPI controller node an see where it takes us... > > spi@1000 { > compatible = "adi,axi-spi-engine"; > #spi-offload-cells = <1>; > /* PWM is connected to offload hardware trigger. DMA for streaming sample > * data can only handle 16-bit words. IIO hardware buffer will be CPU- > * endian because data is streamed one word at a time. */ > spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma"; > > /* pwm properties are only allowed because spi-offload-0-capabilities > * contains "pwm-trigger" */ > pwms = <&pwm 0>; > pwm-names = "offload-0-pwm-trigger"; > > /* dma properties are only allowed because spi-offload-0-capabilities > * contains "16-bit-rx-dma" */ > dmas = <&dma 0>; > dma-names = "offload-0-16-bit-rx"; > > ... > > adc@0 { > compatible = "adi,ad7944"; > spi-offloads = <0>; > ... > }; > }; > > spi@2000 { > compatible = "not-adi,other-spi-engine"; > #spi-offload-cells = <1>; > /* Clock is connected to offload hardware trigger. DMA for streaming sample > * data can only handle one byte at a time. IIO hardware buffer will be big- > * endian because data is streamed one byte at a time. */ > spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma"; > > /* Clock properties are extended because spi-offload-0-capabilities contains > * "clock-trigger" and SPI controller itself has a clock */ > clocks = <&cpu_clock 0>, <&extra_clock 0>; > clock-names = "sclk", "offload-0-pwm-trigger"; > > /* DMA properties are extended because spi-offload-0-capabilities contains > * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */ > dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>; > dma-names = "tx", "rx", "offload-0-16-bit-rx"; > > ... > > adc@0 { > compatible = "adi,ad7944"; > spi-offloads = <0>; > ... > }; > }; > > spi@3000 { > compatible = "adi,axi-spi-engine"; > #spi-offload-cells = <1>; > /* Sample ready signal (~BSY) from ADC is connected to offload hardware > * trigger. DMA for streaming sample data can only handle 16-bit words. */ > spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma"; > > /* Do we need a property to say that the sample trigger is connected to > * adc@0 so that if adc@1 tries to use it, it will fail? */ > > /* dma properties are only allowed because spi-offload-0-capabilities > * contains "16-bit-rx-dma" */ > dmas = <&dma 0>; > dma-names = "offload-0-16-bit-rx"; > > ... > > adc@0 { > compatible = "adi,ad7944"; > spi-offloads = <0>; > ... > > /* PWM connected to the conversion pin (CNV). This only makes sense > * when offload is used with BSY signal, otherwise we would have CNV > * connected to SPI CS. */ > pwms = <&pwm 0>; > pwm-names = "cnv"; > }; > }; > > spi@4000 { > compatible = "adi,axi-spi-engine"; > #spi-offload-cells = <1>; > /* Sample ready signal (~BSY) from ADC is connected to offload hardware > * trigger. DMA for streaming sample data can only handle 32-bit words. > * This also has the CRC validation that strips off the CRC byte of the > * raw data before passing the sample to DMA. */ > spi-offload-0-capabilities = "sample-trigger", > "32-bit-rx-dma-with-ad4630-crc-check"; > > /* dma properties are only allowed because spi-offload-0-capabilities > * contains "16-bit-rx-dma" */ > dmas = <&dma 0>; > dma-names = "offload-0-16-bit-rx"; > > interrupt-parent = <&intc>; > /* First interrupt is for the SPI controller (always present), second > * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check" > * of offload 0. */ > interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "controller", "offload-0-crc-error"; > > ... > > adc@0 { > compatible = "adi,ad4630"; > spi-offloads = <0>; > ... > > /* PWM connected to the conversion pin (CNV). Without offload, we would > * have cnv-gpios instead. */ > pwms = <&pwm 0>; > pwm-names = "cnv"; > }; > }; > > So this is what I came up with of how things could look (combining > suggestions from Rob in v1 and Conor's suggestions here). I can see > how we can make this work. But the thing I don't like about it is that > the peripheral drivers still need to know all of the information about > the offload capabilities and need to interact with the > pwms/clocks/interrupts/dmas/etc. So this is just adding layers of > indirection where all of this stuff has to go through the SPI > controller driver. > > > > > > > Another idea I had was to perhaps use the recently added IIO backend > > > framework for the "extras". The idea there is that we are creating a > > > "composite" IIO device that consists of the ADC chip (frontend) plus > > > these extra hardware trigger and hardware buffer functions provided by > > > the FPGA (backend). > > > > > > offload_backend: adc0-backend { > > > /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */ > > > compatible = "adi,pulsar-adc-offload"; > > > #io-backend-cells = <0>; > > > dmas = <&dma 0>; > > > dma-names = "rx"; > > > clocks = <&trigger_clock>; > > > }; > > > > > > spi { > > > ... > > > adc@0 { > > > ... > > > spi-offloads = <0>; > > > io-backends = <&offload_backend>; > > > }; > > > }; > > > > > > While this could be a solution for IIO devices, this wouldn't solve > > > the issue in general though for SPI offloads used with non-IIO > > > peripherals. > > > > Yeah, I agree that using something specific to IIO isn't really a good > > solution. > > > > Cheers, > > Conor. > > > > > So I don't think it is the right thing to do here. But, I > > > think this idea of a "composite" device helps explain why we are > > > pushing for putting the "extras" with the peripheral node rather than > > > the controller node, at least for the specific case of the AXI SPI > > > Engine controller. > >
On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > > > > > > > > > Back to "something beyond the SPI controller": > > > > > > > > > > > > Here are some examples of how I envision this would work. > > > > > > > > > > > > Let's suppose we have a SPI controller that has some sort of offload > > > > > > capability with a configurable trigger source. The trigger can either > > > > > > be an internal software trigger (i.e. writing a register of the SPI > > > > > > controller) or and external trigger (i.e. a input signal from a pin on > > > > > > the SoC). The SPI controller has a lookup table with 8 slots where it > > > > > > can store a series of SPI commands that can be played back by > > > > > > asserting the trigger (this is what provides the "offloading"). > > > > > > > > > > > > So this SPI controller would have #spi-offload-cells = <2>; where the > > > > > > first cell would be the index in the lookup table 0 to 7 and the > > > > > > second cell would be the trigger source 0 for software or 1 for > > > > > > hardware. > > > > > > > > > > > > Application 1: a network controller > > > > > > > > > > > > This could use two offloads, one for TX and one for RX. For TX, we use > > > > > > the first slot with a software trigger because the data is coming from > > > > > > Linux. For RX we use the second slot with a hardware trigger since > > > > > > data is coming from the network controller (i.e. a data ready signal > > > > > > that would normally be wired to a gpio for interrupt but wired to the > > > > > > SPI offload trigger input pin instead). So the peripheral bindings > > > > > > would be: > > > > > > > > > > > > #define SOFTWARE_TRIGGER 0 > > > > > > #define HARDWARE_TRIGGER 1 > > > > > > > > > > > > can@0 { > > > > > > ... > > > > > > spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>; > > > > > > /* maybe we need names too? */ > > > > > > spi-offload-names = "tx", "rx"; > > > > > > }; > > > > > > > > > > > > In this case, there is nothing extra beyond the SPI controller and the > > > > > > network controller, so no extra bindings beyond this are needed. > > > > > > > > > > > > Application 2: an advanced ADC + FPGA > > > > > > > > > > > > This is basically the same as the ad7944 case seen already with one > > > > > > extra feature. In this case, the sample data also contains a CRC byte > > > > > > for error checking. So instead of SPI RX data going directly to DMA, > > > > > > the FPGA removes the CRC byte from the data stream an only the sample > > > > > > data goes to the DMA buffer. The CRC is checked and if bad, an > > > > > > interrupt is asserted. > > > > > > > > > > > > Since this is an FPGA, most everything is hardwired rather than having > > > > > > any kind of mux selection so #spi-offload-cells = <1>; for this > > > > > > controller. > > > > > > > > > > > > By adding spi-offloads to the peripheral node, it also extends the > > > > > > peripheral binding to include the additional properties needed for the > > > > > > extra features provided by the FPGA. In other words, we are saying > > > > > > this DT node now represents the ADC chip plus everything connected to > > > > > > the offload instance used by the ADC chip. > > > > > > > > > > It seems very strange to me that the dmas and the clock triggers are > > > > > going into the spi device nodes. The description is > > > > > > + dmas: > > > > > > + maxItems: 1 > > > > > > + description: RX DMA Channel for receiving a samples from the SPI > > > > > > offload. > > > > > But as far as I can tell this device is in a package of its own and not > > > > > some IP provided by Analog that an engine on the FPGA can actually do > > > > > DMA to, and the actual connection of the device is "just" SPI. > > > > > The dmas and clock triggers etc appear to be resources belonging to the > > > > > controller that can "assigned" to a particular spi device. If the adc > > > > > gets disconnected from the system, the dmas and clock triggers are still > > > > > connected to the spi controller/offload engine, they don't end up n/c, > > > > > right? (Well maybe they would in the case of a fancy SPI device that > > > > > provides it's own sampling clock or w/e, but then it'd be a clock > > > > > provider of sorts). I'd be expecting the spi-offloads property to be > > > > > responsible for selecting which of the various resources belonging to > > > > > the controller are to be used by a device. > > > > > Maybe it overcomplicates the shit out of things and Rob or Mark are > > > > > gonna start screaming at me but w/e, looking at it from the point of > > > > > view of how the hardware is laid out (or at least how it is described > > > > > in your FPGA case above) the dma/clock properties looks like they're > > > > > misplaced. IOW, I don't think that adding the spi-offloads property > > > > > should convert a node from representing an ADC in a qfn-20 or w/e > > > > > to "the ADC chip plus everything connected to the offload instance > > > > > used by the ADC chip". > > > > > > > > This is the same reasoning that led me to the binding proposed in v1. > > > > Rob suggested that these extras (dmas/clocks) should just be > > > > properties directly of the SPI controller. > > > > > > > But the issue I have with > > > > that is that since this is an FPGA, these properties are not fixed. > > > > > > I don't think whether or not we're talking about an FPGA or an ASIC > > > matters at all here to be honest. In my view the main thing that FPGA > > > impact in terms of bindings is the number of properties required to > > > represent the configurability of a particular IP. Sure, you're gonna > > > have to change the dts around in a way you'll never have to with an > > > ASIC, but the description of individual devices or relations between > > > them doesn't change. > > > > > > > Maybe there are more clocks or no clocks or interrupts or something we > > > > didn't think of yet. > > > > > > This could happen with a new generation of an ASIC and is not specific > > > to an FPGA IP core. Even with FPGA IP, while there may be lots of > > > different configuration parameters, they are known & limited - you can > > > look at the IP's documentation (or failing that, the HDL) to figure out > > > what the points of variability are. It's not particularly difficult to > > > account for there being a configurable number of clocks or interrupts. > > > For "something we didn't think of yet" to be a factor, someone has to > > > think of it and add it to the IP core, and which point we can absolutely > > > change the bindings and software to account for the revised IP. > > > > > > > So it doesn't really seem possible to write a > > > > binding for the SPI controller node to cover all of these cases. > > > > > > I dunno, I don't think your concerns about numbers of clocks (or any > > > other such property) are unique to this particular use-case. > > > > > > > These > > > > extras are included in the FPGA bitstream only for a specific type of > > > > peripheral, not for general use of the SPI controller with any type of > > > > peripheral. > > > > > > I accept that, but what I was trying to get across was that you could > > > configure the FPGA with a bitstream that contains these extra resources > > > and then connect a peripheral device that does not make use of them at > > > all. > > > > Sure, in this case the peripheral has no spi-offloads property and the > > SPI controller acts as a typical SPI controller. > > > > > Or you could connect a range of different peripherals that do use > > > them. > > > > OK, you've got me thinking about something I hadn't really thought about yet. > > > > > Whether or not the resources are there and how they are connected > > > in the FPGA bitstream is not a property of the device connected to the > > > SPI controller, only whether or not you use them is. > > > > > > > Even when those things are connected directly to a specific peripheral > > device? Or not connected to the offload? > > > > Here is another potential ADC trigger case to consider. > > > > +-------------------------------+ +------------------+ > > > | | | > > > SOC/FPGA | | ADC | > > > +---------------------+ | | | > > > | AXI SPI Engine | | | | > > > | SPI Bus ============ SPI Bus | > > > | | | | | > > > | +---------------+ | < < < < < BUSY | > > > | | Offload 0 | | v | | | > > > | | | | v > > > > CNV | > > > | | TRIGGER IN < < < ^ | | | > > > | +---------------+ | ^ | +------------------+ > > > +---------------------+ ^ | > > > | AXI PWM | ^ | > > > | CH0 > > ^ | > > > +---------------------+ | > > > | > > +-------------------------------+ > > > > This time, the periodic trigger (PWM) is connected to the pin on the > > ADC that triggers a sample conversion (CNV). The ADC has a BUSY output > > that will go high at the start of the conversion and go low at the end > > of the conversion. The BUSY output of the ADC is wired as the hardware > > trigger input of the offload. > > > > In this case would we still consider the PWM as part of the SPI > > controller/offload since it can only be used in conjunction with the > > SPI offload? It isn't connected to it at all though. > > No, in this case the ADC is a PWM consumer and the offload engine is > not. The ADC is a "trigger" provider and the SPI offload engine is a > trigger consumer. > > > Another case could be a self-clocked ADC. Here, the ADC has it's own > > periodic sample trigger on the chip and the RDY output goes high > > whenever a sample is ready to read. > > > > +-------------------------------+ +------------------+ > > > | | | > > > SOC/FPGA | | ADC | > > > +---------------------+ | | | > > > | AXI SPI Engine | | | | > > > | SPI Bus ============ SPI Bus | > > > | | | | | > > > | +---------------+ | < < < < < RDY | > > > | | Offload 0 | | v | | | > > > | | | | v | | | > > > | | TRIGGER IN < < < | | | > > > | +---------------+ | | +------------------+ > > > +---------------------+ | > > > | > > +-------------------------------+ > > > > If both of these are valid wiring configurations for the same ADC, > > wouldn't it make more sense to describe this in the peripheral node > > rather than having to query the controller to see how the peripheral > > is wired up? > > In both of these cases, the offload works in the same way, it gets a > trigger from the ADC and acts upon it. I would expect that in this case > the ADC driver would look for an optional pwm property in the devicetree > and if it is present configure the ADC to use that and if absent do then > do whatever configuration is required for self clocking. I would expect > that both cases would be handled identically by the SPI [offload] engine > side of things, other than inverting the polarity of the trigger. (If I > understand correctly, you want to trigger the offload engine on a > falling edge of BUSY but a rising edge of RDY). > > > > > In fact, looking at the documentation for the "spi engine" some more, I > > > am even less convinced that the right place for describing the offload is > > > the peripheral as I (finally?) noticed that the registers for the offload > > > engine are mapped directly into the "spi engine"'s memory map, rather than > > > it being a entirely separate IP in the FPGA fabric. > > > > True, but we don't use these registers, e.g., to configure the > > sampling frequency of a trigger (if it can even be done). That is done > > in a completely separate IP block with it's own registers. > > Where is the binding for that IP block? I think describing that is > pretty key. goto continuation; > > > > Further, what resources are available depends on what the SPI offload > > > engine IP is. If my employer decides to start shipping some SPI offload > > > IP in our catalogue that can work with ADI's ADCs, the set of offload > > > related properties or their names may well differ. > > > > If all of these resources were fairly generic, like the periodic > > trigger we've been discussing, then I could see the case for trying to > > accommodate this the same way we do for other common features of SPI > > controllers. But for cases like "Application 2" that I described > > previously, these resources can get very specific to a peripheral > > (like the example given of having a data processing unit that knows > > about the exact data format and CRC algorithm used by a specific ADC). > > These seems like too specific of a thing to say that a SPI controller > > "supports". > > To remind myself, "Application 2" featured an offload engine designed > specifically to work with a particular data format that would strip a > CRC byte and check the validity of the data stream. > I think the data manipulation is not really a property of the engine. Typically data going out of the offload engine goes into another "data reorder" block that is pure HW. > I think you're right something like that is a stretch to say that that > is a feature of the SPI controller - but I still don't believe that > modelling it as part of the ADC is correct. I don't fully understand the > io-backends and how they work yet, but the features you describe there > seem like something that should/could be modelled as one, with its own > node and compatible etc. Describing custom RTL stuff ain't always > strightforward, but the stuff from Analog is versioned and documented > etc so it shouldn't be quite that hard. > Putting this in io-backends is likely a stretch but one thing to add is that the peripheral is always (I think) kind of the consumer of the resources. Taking the trigger (PWM) as an example and even when it is directly connected with the offload block, the peripheral still needs to know about it. Think of sampling frequency... The period of the trigger signal is strictly connected with the sampling frequency of the peripheral for example. So I see 2 things: 1) Enabling/Disabling the trigger could be easily done from the peripheral even with the resource in the spi engine. I think David already has some code in the series that would make this trivial and so having the property in the spi controller brings no added complexity. 2) Controlling things like the trigger period/sample_rate. This could be harder to do over SPI (or making it generic enough) so we would still need to have the same property on the peripheral (even if not directly connected to it). I kind of agree with David that having the property both in the peripheral and controller is a bit weird. And the DMA block is a complete different story. Sharing that data back with the peripheral driver (in this case, the IIO subsystem) would be very interesting at the very least. Note that the DMA block is not really something that is part of the controller nor the offload block. It is an external block that gets the data coming out of the offload engine (or the data reorder block). In IIO, we already have a DMA buffer interface so users of the peripheral can get the data without any intervention of the driver (on the data). We "just" enable buffering and then everything happens on HW and userspace can start requesting data. If we are going to attach the DMA in the controller, I have no idea how we can handle it. Moreover, the offload it's really just a way of replaying the same spi transfer over and over and that happens in HW so I'm not sure how we could "integrate" that with dmaengine. But maybe I'm overlooking things... And thinking more in how this can be done in SW rather than what makes sense from an HW perspective. > continuation: > If offload engines have their own register region in the memory map, Don't think it has it's own register region... David? > their own resources (the RTL is gonna need at the very least a clock) > and potentially also provide other services (like acting as an > io-backend type device that pre-processes data) it doesn't sound like > either the controller or peripheral nodes are the right place for these > properties. And uh, spi-offloads gets a phandle once more... > But then it would be valid for both peripheral and controller to reference that phandle right (and get the resources of interest)? FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful to you: https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl - Nuno Sá
On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote: > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material.
On 5/22/24 1:24 PM, Conor Dooley wrote: > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: >> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: >>> >>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: >>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: >>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: >>> ... >> This time, the periodic trigger (PWM) is connected to the pin on the >> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output >> that will go high at the start of the conversion and go low at the end >> of the conversion. The BUSY output of the ADC is wired as the hardware >> trigger input of the offload. >> >> In this case would we still consider the PWM as part of the SPI >> controller/offload since it can only be used in conjunction with the >> SPI offload? It isn't connected to it at all though. > > No, in this case the ADC is a PWM consumer and the offload engine is > not. The ADC is a "trigger" provider and the SPI offload engine is a > trigger consumer. Makes sense. ... > >>> In fact, looking at the documentation for the "spi engine" some more, I >>> am even less convinced that the right place for describing the offload is >>> the peripheral as I (finally?) noticed that the registers for the offload >>> engine are mapped directly into the "spi engine"'s memory map, rather than >>> it being a entirely separate IP in the FPGA fabric. >> >> True, but we don't use these registers, e.g., to configure the >> sampling frequency of a trigger (if it can even be done). That is done >> in a completely separate IP block with it's own registers. > > Where is the binding for that IP block? I think describing that is > pretty key. goto continuation; For the real-world case I used to test this series, it is an AXI PWMGEN [1] that is providing the trigger event source. It has a typical PWM provider binding with #pwm-cells [2]. Calling this a "trigger" provider to the SPI offload instance just like the case above where the ADC is directly connected as the offload trigger makes sense to me. What I was going for in this patch (slightly revised to use #cells) is that this trigger provider, whatever it is, is selected by one of the cells of #spi-offload-cells. It doesn't seem like there should be a special case for if the trigger provider is a clock or PWM where the SPI controller node becomes a consumer of the clock or PWM provider rather than just describing the trigger relationship. For example, supposing we had an FPGA/HDL that could handle all 3 wiring configurations we have discussed so far. A) PWM connected directly to SPI offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected to SPI offload as trigger. So the DT would have: controller: #spi-offload-cells = <2>: /* 1st cell = offload instance * 2nd cell = trigger provider */ peripheral (choose one based on actual wiring): spi-offloads = <0 0>; /* case A */ spi-offloads = <0 1>; /* case B */ spi-offloads = <0 2>; /* case C */ As to what is the actual consumer of the PWM provider in each of these cases... * C is easy. There isn't a PWM provider since the ADC is self-clocked. * B, as discussed elsewhere is fairly straight forward. The ADC node is the consumer since the PWM is connected directly to the ADC. * A is the one we need to figure out. I'm proposing that the PWM consumer should be whatever kind of composite device node we come up with that also solves the issue described below about where does the CRC checker (or whatever) go. I think we are in agreement here at least on the point that it doesn't belong in the SPI controller node? [1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html [2]: https://lore.kernel.org/linux-pwm/20240424125850.4189116-2-tgamblin@baylibre.com/ > > I think you're right something like that is a stretch to say that that > is a feature of the SPI controller - but I still don't believe that > modelling it as part of the ADC is correct. I don't fully understand the > io-backends and how they work yet, but the features you describe there > seem like something that should/could be modelled as one, with its own > node and compatible etc. Describing custom RTL stuff ain't always > strightforward, but the stuff from Analog is versioned and documented > etc so it shouldn't be quite that hard. > > continuation: > If offload engines have their own register region in the memory map, > their own resources (the RTL is gonna need at the very least a clock) > and potentially also provide other services (like acting as an > io-backend type device that pre-processes data) it doesn't sound like > either the controller or peripheral nodes are the right place for these > properties. And uh, spi-offloads gets a phandle once more... >
On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote: > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > > > > > > I think you're right something like that is a stretch to say that that > > is a feature of the SPI controller - but I still don't believe that > > modelling it as part of the ADC is correct. I don't fully understand the > > io-backends and how they work yet, but the features you describe there > > seem like something that should/could be modelled as one, with its own > > node and compatible etc. Describing custom RTL stuff ain't always > > strightforward, but the stuff from Analog is versioned and documented > > etc so it shouldn't be quite that hard. > > > > Putting this in io-backends is likely a stretch but one thing to add is that the > peripheral is always (I think) kind of the consumer of the resources. Taking the > trigger (PWM) as an example and even when it is directly connected with the offload > block, the peripheral still needs to know about it. Think of sampling frequency... > The period of the trigger signal is strictly connected with the sampling frequency of > the peripheral for example. So I see 2 things: Cherry picking this one thing to reply to cos I'm not sure if it was understood as I intended. When I talked about io-backends I was not suggesting that we drop the spi-offload idea, I was suggesting that if something has a dedicated register region & resources that handles both offloading and some usecase specific data processing that it should be a device of its own, acting as a provider of both spi-offloads and io-backends. I'll look at the rest of the mail soonTM.
On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote: > On 5/22/24 1:24 PM, Conor Dooley wrote: > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > > > > > ... > > controller: > #spi-offload-cells = <2>: /* 1st cell = offload instance > * 2nd cell = trigger provider */ > What about things like DMA? I'm mentioning it a lot because it's way more complex having it on the controller (from a SW perspective). But from an HW point of view, it's always very similar (if not the same) as your case A. - Nuno Sá >
On 5/23/24 7:15 AM, Nuno Sá wrote: > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: >> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: >>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: >>>> >>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: >>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: >>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: >>>> ... >> To remind myself, "Application 2" featured an offload engine designed >> specifically to work with a particular data format that would strip a >> CRC byte and check the validity of the data stream. >> > > I think the data manipulation is not really a property of the engine. Typically data > going out of the offload engine goes into another "data reorder" block that is pure > HW. > >> I think you're right something like that is a stretch to say that that >> is a feature of the SPI controller - but I still don't believe that >> modelling it as part of the ADC is correct. I don't fully understand the >> io-backends and how they work yet, but the features you describe there >> seem like something that should/could be modelled as one, with its own >> node and compatible etc. Describing custom RTL stuff ain't always >> strightforward, but the stuff from Analog is versioned and documented >> etc so it shouldn't be quite that hard. >> > > Putting this in io-backends is likely a stretch but one thing to add is that the > peripheral is always (I think) kind of the consumer of the resources. Taking the > trigger (PWM) as an example and even when it is directly connected with the offload > block, the peripheral still needs to know about it. Think of sampling frequency... > The period of the trigger signal is strictly connected with the sampling frequency of > the peripheral for example. So I see 2 things: > > 1) Enabling/Disabling the trigger could be easily done from the peripheral even with > the resource in the spi engine. I think David already has some code in the series > that would make this trivial and so having the property in the spi controller brings > no added complexity. > > 2) Controlling things like the trigger period/sample_rate. This could be harder to do > over SPI (or making it generic enough) so we would still need to have the same > property on the peripheral (even if not directly connected to it). I kind of agree > with David that having the property both in the peripheral and controller is a bit > weird. > > And the DMA block is a complete different story. Sharing that data back with the > peripheral driver (in this case, the IIO subsystem) would be very interesting at the > very least. Note that the DMA block is not really something that is part of the > controller nor the offload block. It is an external block that gets the data coming > out of the offload engine (or the data reorder block). In IIO, we already have a DMA > buffer interface so users of the peripheral can get the data without any intervention > of the driver (on the data). We "just" enable buffering and then everything happens > on HW and userspace can start requesting data. If we are going to attach the DMA in > the controller, I have no idea how we can handle it. Moreover, the offload it's > really just a way of replaying the same spi transfer over and over and that happens > in HW so I'm not sure how we could "integrate" that with dmaengine. > > But maybe I'm overlooking things... And thinking more in how this can be done in SW > rather than what makes sense from an HW perspective. > > >> continuation: >> If offload engines have their own register region in the memory map, > > > Don't think it has it's own register region... David? I think the question here was if the CRC checker IP block (or descrambler shown in the link below, or whatever) had registers in the offload/SPI controller to control that extra part or if they had their own dedicated registers. So far, these have been fixed-purpose, so have no registers at all. But I could see needing a register, e.g. for turning it on or off. In this case, I think it does become something like an io-backend. Or would we add this on/off switch to the AXI SPI Engine registers? Also, as shown in the link below, the extra bits share a clock domain with the AXI SPI Engine. So, yes, technically I suppose they could/should? be independent consumers of the same clock like Conor suggests below. It does seems kind of goofy if we have to write a driver just to turn on a clock that is already guaranteed to be on though. > >> their own resources (the RTL is gonna need at the very least a clock) >> and potentially also provide other services (like acting as an >> io-backend type device that pre-processes data) it doesn't sound like >> either the controller or peripheral nodes are the right place for these >> properties. And uh, spi-offloads gets a phandle once more... >> > > But then it would be valid for both peripheral and controller to reference that > phandle right (and get the resources of interest)? > > FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful > to you: > > https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl > > - Nuno Sá > >
On 5/23/24 9:57 AM, Nuno Sá wrote: > On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote: >> On 5/22/24 1:24 PM, Conor Dooley wrote: >>> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: >>>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: >>>>> >>>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: >>>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: >>>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: >>>>> >> > > ... > >> >> controller: >> #spi-offload-cells = <2>: /* 1st cell = offload instance >> * 2nd cell = trigger provider */ >> > > What about things like DMA? I'm mentioning it a lot because it's way more complex > having it on the controller (from a SW perspective). But from an HW point of view, > it's always very similar (if not the same) as your case A. > If we had a setup where there was more than one place that, e.g. the RX stream from the offload could be piped, then I would add a 3rd cell to describe that. If the hardware is fixed and the RX stream always goes to a specific DMA channel, then it doesn't seem like we need to describe that in the SPI controller node because the hardware is fixed.
On Thu, 2024-05-23 at 10:09 -0500, David Lechner wrote: > On 5/23/24 9:57 AM, Nuno Sá wrote: > > On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote: > > > On 5/22/24 1:24 PM, Conor Dooley wrote: > > > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > > > > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > > > > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > > > > > > > > > > > > > ... > > > > > > > > controller: > > > #spi-offload-cells = <2>: /* 1st cell = offload instance > > > * 2nd cell = trigger provider */ > > > > > > > What about things like DMA? I'm mentioning it a lot because it's way more complex > > having it on the controller (from a SW perspective). But from an HW point of > > view, > > it's always very similar (if not the same) as your case A. > > > > If we had a setup where there was more than one place that, e.g. the > RX stream from the offload could be piped, then I would add a 3rd > cell to describe that. If the hardware is fixed and the RX stream > always goes to a specific DMA channel, then it doesn't seem like we > need to describe that in the SPI controller node because the hardware > is fixed. > Well, yes, still the DMA channel is connected on the controller and not the peripheral. Same deal as we discussed on the IIO backends stuff. But there, since it's all IIO it was easy to make the DMA a property of the backend device. That said, I can surely see having the property in the peripheral. Another thing that came to mind for the trigger case. What about an additional spi interface for configuring/setting the trigger rate? Sounds generic and then we would not really need the trigger on the peripheral right (did not checked the CRC issue you mentioned so not sure if it's somehow trigger related)? Hmm but sometimes there's other things than rate/period (like offset) to care about so maybe not doable... Bah! - Nuno Sá
On Thu, May 23, 2024 at 10:05:49AM -0500, David Lechner wrote: > On 5/23/24 7:15 AM, Nuno Sá wrote: > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > >> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > >>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > >>>> > >>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote: > >>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote: > >>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote: > >>>> > > ... > > >> To remind myself, "Application 2" featured an offload engine designed > >> specifically to work with a particular data format that would strip a > >> CRC byte and check the validity of the data stream. > >> > > > > I think the data manipulation is not really a property of the engine. Typically data > > going out of the offload engine goes into another "data reorder" block that is pure > > HW. > > > >> I think you're right something like that is a stretch to say that that > >> is a feature of the SPI controller - but I still don't believe that > >> modelling it as part of the ADC is correct. I don't fully understand the > >> io-backends and how they work yet, but the features you describe there > >> seem like something that should/could be modelled as one, with its own > >> node and compatible etc. Describing custom RTL stuff ain't always > >> strightforward, but the stuff from Analog is versioned and documented > >> etc so it shouldn't be quite that hard. > >> > > > > Putting this in io-backends is likely a stretch but one thing to add is that the > > peripheral is always (I think) kind of the consumer of the resources. Taking the > > trigger (PWM) as an example and even when it is directly connected with the offload > > block, the peripheral still needs to know about it. Think of sampling frequency... > > The period of the trigger signal is strictly connected with the sampling frequency of > > the peripheral for example. So I see 2 things: > > > > 1) Enabling/Disabling the trigger could be easily done from the peripheral even with > > the resource in the spi engine. I think David already has some code in the series > > that would make this trivial and so having the property in the spi controller brings > > no added complexity. > > > > 2) Controlling things like the trigger period/sample_rate. This could be harder to do > > over SPI (or making it generic enough) so we would still need to have the same > > property on the peripheral (even if not directly connected to it). I kind of agree > > with David that having the property both in the peripheral and controller is a bit > > weird. > > > > And the DMA block is a complete different story. Sharing that data back with the > > peripheral driver (in this case, the IIO subsystem) would be very interesting at the > > very least. Note that the DMA block is not really something that is part of the > > controller nor the offload block. It is an external block that gets the data coming > > out of the offload engine (or the data reorder block). In IIO, we already have a DMA > > buffer interface so users of the peripheral can get the data without any intervention > > of the driver (on the data). We "just" enable buffering and then everything happens > > on HW and userspace can start requesting data. If we are going to attach the DMA in > > the controller, I have no idea how we can handle it. Moreover, the offload it's > > really just a way of replaying the same spi transfer over and over and that happens > > in HW so I'm not sure how we could "integrate" that with dmaengine. > > > > But maybe I'm overlooking things... And thinking more in how this can be done in SW > > rather than what makes sense from an HW perspective. > > > > > >> continuation: > >> If offload engines have their own register region in the memory map, > > > > > > Don't think it has it's own register region... David? > > I think the question here was if the CRC checker IP block (or descrambler shown > in the link below, or whatever) had registers in the offload/SPI controller > to control that extra part or if they had their own dedicated registers. I don't think there was a question here at all. I was simply stating that if the offload engine was not just a subordinate feature of the SPI controller, but also provided additional data processing features then treating the offload engine as a component of the SPI controller would not be accurate. > So far, > these have been fixed-purpose, so have no registers at all. But I could see > needing a register, e.g. for turning it on or off. In this case, I think it > does become something like an io-backend. Or would we add this on/off switch > to the AXI SPI Engine registers? Seems to be that the CRC checking is a separate piece of IP though, and so is not part of the offload engine at all, so my concern was misplaced. I think whether or not you have registers to control it, it should be represented in DT. How do you know it is there otherwise? > Also, as shown in the link below, the extra bits share a clock domain with the > AXI SPI Engine. So, yes, technically I suppose they could/should? be independent > consumers of the same clock like Conor suggests below. It does seems kind of > goofy if we have to write a driver just to turn on a clock that is already > guaranteed to be on though. You wouldn't necessarily need to write a driver for it, you could reach out and turn it on from the backend consumer for example. And, obviously there may be other ways that the FPGA design is configured where the clock is not shared with the SPI controller or the offload engine.
On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote: > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > > > > To remind myself, "Application 2" featured an offload engine designed > > specifically to work with a particular data format that would strip a > > CRC byte and check the validity of the data stream. > > > > I think the data manipulation is not really a property of the engine. Typically data > going out of the offload engine goes into another "data reorder" block that is pure > HW. > > > I think you're right something like that is a stretch to say that that > > is a feature of the SPI controller - but I still don't believe that > > modelling it as part of the ADC is correct. I don't fully understand the > > io-backends and how they work yet, but the features you describe there > > seem like something that should/could be modelled as one, with its own > > node and compatible etc. Describing custom RTL stuff ain't always > > strightforward, but the stuff from Analog is versioned and documented > > etc so it shouldn't be quite that hard. > > > > Putting this in io-backends is likely a stretch but one thing to add is that the > peripheral is always (I think) kind of the consumer of the resources. Could you explain you think why making some additional processing done to the data an io-backend is a stretch? Where else can this RTL be represented? hint: it's not part of the ADC, just like how if you have some custom RTL that does video processing that is not part of the camera! > Taking the > trigger (PWM) as an example and even when it is directly connected with the offload > block, the peripheral still needs to know about it. Think of sampling frequency... > The period of the trigger signal is strictly connected with the sampling frequency of > the peripheral for example. So I see 2 things: > > 1) Enabling/Disabling the trigger could be easily done from the peripheral even with > the resource in the spi engine. I think David already has some code in the series > that would make this trivial and so having the property in the spi controller brings > no added complexity. > > 2) Controlling things like the trigger period/sample_rate. This could be harder to do > over SPI (or making it generic enough) so we would still need to have the same > property on the peripheral (even if not directly connected to it). I kind of agree > with David that having the property both in the peripheral and controller is a bit > weird. Can you explain what you mean by "same property on the peripheral"? I would expect a peripheral to state its trigger period (just like how it states the max frequency) and for the trigger period not to appear in the controller. I think a way that this could be modelled to reduce some software complexity is considering that the periodic trigger is a clock, not a PWM, provided you are only interested in the period. That'd give you an interface that was less concerned about what the provider of the periodic trigger is. After all, I doubt the ADC cares how you decide to generate the trigger, as long as the periodicity is correct. With the examples provided, you'd get something like: pwm { } pclk { compatible = pwm-clock pwms = <&pwm 0 x> } spi { compatible = spi-engine clocks = <&clks SPI>, <&pwm> clock-names = "bus", "offload" } The pwm-clock driver (clk-pwm.c) doesn't implement .set_rate though, but maybe you don't need that or maybe it could be added if needed. > And the DMA block is a complete different story. Sharing that data back with the > peripheral driver (in this case, the IIO subsystem) would be very interesting at the > very least. Note that the DMA block is not really something that is part of the > controller nor the offload block. It is an external block that gets the data coming > out of the offload engine (or the data reorder block). In IIO, we already have a DMA > buffer interface so users of the peripheral can get the data without any intervention > of the driver (on the data). We "just" enable buffering and then everything happens > on HW and userspace can start requesting data. If we are going to attach the DMA in > the controller, I have no idea how we can handle it. Moreover, the offload it's > really just a way of replaying the same spi transfer over and over and that happens > in HW so I'm not sure how we could "integrate" that with dmaengine. > > But maybe I'm overlooking things... And thinking more in how this can be done in SW > rather than what makes sense from an HW perspective. I don't think you're overlooking things at all, I'm intentionally being a bit difficult and ignoring what may be convenient for the adc driver. This is being presented as a solution to a generic problem (and I think you're right to do that), but it feels as if the one implementation is all that's being considered here and I'm trying to ensure that what we end up with doesn't make limiting assumptions. Part of me says "sure, hook the DMAs up to the devices, as that's what happens for other IIO devices" but at the same time I recognise that the DMA isn't actually hooked up like that and the other IIO devices I see like that are all actually on the SoC, rather than connected over SPI. It might be easy to do it this way right now, but be problematic for a future device or if someone wants to chuck away the ADI provided RTL and do their own thing for this device. Really it just makes me wonder if what's needed to describe more complex data pipelines uses an of_graph, just like how video pipelines are handled, rather than the implementation of io-backends that don't really seem to model the flow of data. > > continuation: > > If offload engines have their own register region in the memory map, > > > Don't think it has it's own register region... David? > > > their own resources (the RTL is gonna need at the very least a clock) > > and potentially also provide other services (like acting as an > > io-backend type device that pre-processes data) it doesn't sound like > > either the controller or peripheral nodes are the right place for these > > properties. And uh, spi-offloads gets a phandle once more... > > > > But then it would be valid for both peripheral and controller to reference that > phandle right (and get the resources of interest)? Sure. But to assume that, for example, the provider of data processing services also provided the periodic polling trigger would be incorrect even if in the only currently existing case it does. Nothing is stopping someone splitting those into different blocks, after all this is "random" RTL in an FPGA :) > FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful > to you: > > https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl Yeah, they're a good reference, thanks. I had to go AFK in the middle of this, I have a nagging feeling that I forgot to say something but I cannot recall what. Cheers, Conor.
On 5/26/24 10:45 AM, Conor Dooley wrote: > On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote: >> * A is the one we need to figure out. I'm proposing that the PWM consumer >> should be whatever kind of composite device node we come up with that >> also solves the issue described below about where does the CRC checker >> (or whatever) go. I think we are in agreement here at least on the point >> that it doesn't belong in the SPI controller node? > > To be clear, you're saying that we agree that the CRC checker doesnt > belong in the SPI controller node, right? Yes.
On Wed, May 29, 2024 at 03:10:54PM -0500, David Lechner wrote: > On 5/26/24 10:45 AM, Conor Dooley wrote: > > On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote: > > > >> * A is the one we need to figure out. I'm proposing that the PWM consumer > >> should be whatever kind of composite device node we come up with that > >> also solves the issue described below about where does the CRC checker > >> (or whatever) go. I think we are in agreement here at least on the point > >> that it doesn't belong in the SPI controller node? > > > > To be clear, you're saying that we agree that the CRC checker doesnt > > belong in the SPI controller node, right? > > Yes. Okay, ye. We're on the same page then about that part.
On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote: > On 5/22/24 1:24 PM, Conor Dooley wrote: > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote: > >> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote: > >>> In fact, looking at the documentation for the "spi engine" some more, I > >>> am even less convinced that the right place for describing the offload is > >>> the peripheral as I (finally?) noticed that the registers for the offload > >>> engine are mapped directly into the "spi engine"'s memory map, rather than > >>> it being a entirely separate IP in the FPGA fabric. > >> > >> True, but we don't use these registers, e.g., to configure the > >> sampling frequency of a trigger (if it can even be done). That is done > >> in a completely separate IP block with it's own registers. > > > > Where is the binding for that IP block? I think describing that is > > pretty key. goto continuation; > > For the real-world case I used to test this series, it is an AXI PWMGEN > [1] that is providing the trigger event source. It has a typical PWM > provider binding with #pwm-cells [2]. > > Calling this a "trigger" provider to the SPI offload instance just like the > case above where the ADC is directly connected as the offload trigger makes > sense to me. > > What I was going for in this patch (slightly revised to use #cells) is that > this trigger provider, whatever it is, is selected by one of the cells of > #spi-offload-cells. It doesn't seem like there should be a special case for > if the trigger provider is a clock or PWM where the SPI controller node > becomes a consumer of the clock or PWM provider rather than just describing > the trigger relationship. > > For example, supposing we had an FPGA/HDL that could handle all 3 wiring > configurations we have discussed so far. A) PWM connected directly to SPI > offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected > to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected > to SPI offload as trigger. So the DT would have: > > controller: > #spi-offload-cells = <2>: /* 1st cell = offload instance > * 2nd cell = trigger provider */ > > peripheral (choose one based on actual wiring): > spi-offloads = <0 0>; /* case A */ > spi-offloads = <0 1>; /* case B */ > spi-offloads = <0 2>; /* case C */ > > > As to what is the actual consumer of the PWM provider in each of these > cases... > > * C is easy. There isn't a PWM provider since the ADC is self-clocked. > * B, as discussed elsewhere is fairly straight forward. The ADC node is > the consumer since the PWM is connected directly to the ADC. > * A is the one we need to figure out. I'm proposing that the PWM consumer > should be whatever kind of composite device node we come up with that > also solves the issue described below about where does the CRC checker > (or whatever) go. I think we are in agreement here at least on the point > that it doesn't belong in the SPI controller node? I think C and B are correct here. The term "composite device" scares me, but yeah the PWM gets connected to the device that encompasses the offload engine. In case B and C, the trigger signals are connected to the offload engine - what property are we using to describe that? If this were not a "passive" process and the OS was actually involved in kicking off an action when the ADC asserted the !BSY signal, interrupts would make sense, but I'm not sure here. Maybe interrupts is the right thing to do, because that would also make sense in the case that we used the busy signal without an offload and plumbed it into a regular interrupt controller? gpio0: gpio@20120000 { compatible = "microchip,mpfs-gpio"; reg = <0x0 0x20120000 0x0 0x1000>; interrupt-parent = <&plic>; interrupt-controller; #interrupt-cells = <1>; clocks = <&clkcfg CLK_GPIO0>; gpio-controller; #gpio-cells = <2>; }; adc { interrupt-parent = <&gpio0>; interrupts = <0 HIGH>; }; I suppose then the offload needs to be an interrupt provider too, even if no irqchip is ever actually created for it. I dunno how tf that's gonna work out on the software side though? If you have spi-offloads you interpret the interrupts property differently (IOW you ignore it)? Or were you thinking of not really hooking that up at all, but instead just saying checking for whatever x is in spi-offloads = <0 x> and defining 2 to mean "active low" and 1 to mean "active high" etc? Once again, had to take a break to make food in the middle of responding here, so I hope I didn't write something incomprehensible. Thanks, Conor. > > [1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html > [2]: https://lore.kernel.org/linux-pwm/20240424125850.4189116-2-tgamblin@baylibre.com/
On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote: > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote: > > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote: > > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > > > Taking the > > > trigger (PWM) as an example and even when it is directly connected with the > > > offload > > > block, the peripheral still needs to know about it. Think of sampling > > > frequency... > > > The period of the trigger signal is strictly connected with the sampling > > > frequency of > > > the peripheral for example. So I see 2 things: > > > > > > 1) Enabling/Disabling the trigger could be easily done from the peripheral even > > > with > > > the resource in the spi engine. I think David already has some code in the series > > > that would make this trivial and so having the property in the spi controller > > > brings > > > no added complexity. > > > > > > 2) Controlling things like the trigger period/sample_rate. This could be harder > > > to do > > > over SPI (or making it generic enough) so we would still need to have the same > > > property on the peripheral (even if not directly connected to it). I kind of > > > agree > > > with David that having the property both in the peripheral and controller is a > > > bit > > > weird. > > > > Can you explain what you mean by "same property on the peripheral"? I > > would expect a peripheral to state its trigger period (just like how it > > states the max frequency) and for the trigger period not to appear in > > the controller. > > > > Just have the same 'pwms' property on both the controller and peripheral... Yeah, no... Opinion unchanged since my last message. > > I think a way that this could be modelled to reduce some software > > complexity is considering that the periodic trigger is a clock, not > > a PWM, provided you are only interested in the period. That'd give you > > an interface that was less concerned about what the provider of the > > periodic trigger is. After all, I doubt the ADC cares how you decide to > > generate the trigger, as long as the periodicity is correct. > > With the examples provided, you'd get something like: > > > > Unfortunately that's not the case. Ahh, that sucks. Oh well. > For instance, in the design on the link I gave you > on the last reply we do have an averaging mode where we actually need an offset > (effort for supporting that in PWM is already ongoing) between the offload trigger > and the peripheral conversion signal (so assuming we only care about period will fail > pretty soon :)). > > > And the DMA block is a complete different story. Sharing that data back with the > > > peripheral driver (in this case, the IIO subsystem) would be very interesting at > > > the > > > very least. Note that the DMA block is not really something that is part of the > > > controller nor the offload block. It is an external block that gets the data > > > coming > > > out of the offload engine (or the data reorder block). In IIO, we already have a > > > DMA > > > buffer interface so users of the peripheral can get the data without any > > > intervention > > > of the driver (on the data). We "just" enable buffering and then everything > > > happens > > > on HW and userspace can start requesting data. If we are going to attach the DMA > > > in > > > the controller, I have no idea how we can handle it. Moreover, the offload it's > > > really just a way of replaying the same spi transfer over and over and that > > > happens > > > in HW so I'm not sure how we could "integrate" that with dmaengine. > > > > > > But maybe I'm overlooking things... And thinking more in how this can be done in > > > SW > > > rather than what makes sense from an HW perspective. > > > > I don't think you're overlooking things at all, I'm intentionally being > > a bit difficult and ignoring what may be convenient for the adc driver. > > This is being presented as a solution to a generic problem (and I think > > you're right to do that), but it feels as if the one implementation is > > all that's being considered here and I'm trying to ensure that what we > > end up with doesn't make limiting assumptions. > > Yeah, I know and I do agree we need something generic enough and not something that > only fits a couple usecases. If only we had another user... I suppose you lads are the market leader in these kinds of devices. If I did happen to know if Microchip was working on anything similar (which I don't, I work on FPGAs not these kinds of devices) I couldn't even tell you. I suppose I could ask around and see. Do you know if TI is doing anything along these lines? > > Part of me says "sure, hook the DMAs up to the devices, as that's what > > happens for other IIO devices" but at the same time I recognise that the > > DMA isn't actually hooked up like that and the other IIO devices I see > > like that are all actually on the SoC, rather than connected over SPI. > > Yeah, I know... But note (but again, only for ADI designs) that the DMA role is > solely for carrying the peripheral data. It is done like this so everything works in > HW and there's no need for SW to deal with the samples at all. I mean, only the > userspace app touches the samples. > > TBH, the DMA is the bit that worries me the most as it may be overly complex to share > buffers (using dma-buf or something else) from the spi controller back to consumers > of it (IIO in this case). And I mean sharing in a way that there's no need to touch > the buffers. <snip> > Maybe having an offload dedicated API (through spi) to get/share a DMA handle would > be acceptable. Then we could add support to "import" it in the IIO core. Then it > would be up to the controller to accept or not to share the handle (in some cases the > controller could really want to have the control of the DMA transfers). Yeah, that is about what I was thinking. I wasn't expecting the spi code to grow handing for dmabuf or anything like that, just a way for the offload consumer to say "yo, can you tell me what dma buffer I can use?". Unless (until?) there's some controller that wants to manage it, I think that'd be sufficient?
On 5/29/24 3:07 AM, Nuno Sá wrote: > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote: >> It might be easy to do it this way right now, but be problematic for a >> future device or if someone wants to chuck away the ADI provided RTL and >> do their own thing for this device. Really it just makes me wonder if >> what's needed to describe more complex data pipelines uses an of_graph, >> just like how video pipelines are handled, rather than the implementation >> of io-backends that don't really seem to model the flow of data. >> > > Yeah, backends is more for devices/soft-cores that extend the functionality of the > device they are connected too. Like having DACs/ADCs hdl cores for connecting to high > speed controllers. Note that in some cases they also manipulate or even create data > but since they fit in IIO, having things like the DMA property in the hdl binding was > fairly straight. > > Maybe having an offload dedicated API (through spi) to get/share a DMA handle would > be acceptable. Then we could add support to "import" it in the IIO core. Then it > would be up to the controller to accept or not to share the handle (in some cases the > controller could really want to have the control of the DMA transfers). I could see this working for some SPI controllers, but for the AXI SPI Engine + DMA currently, the DMA has a fixed word size, so can't be used as a generic DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit word size, then even if we are reading 16-bit sample data, the DMA is going to put it in a 32-bit slot. So one could argue that this is still doing some data manipulation similar to the CRC checker example. > > Not familiar enough with of_graph so can't argue about it but likely is something > worth looking at. > > - Nuno Sá >>> I did try implementing something using graph bindings when I first started working on this, but it didn't seem to really give us any extra useful information. It was just describing connections (endpoints) that I thought we could just implicitly assume. After this discussion though, maybe worth a second look. I'll have to think about it more.
On 5/30/24 2:18 PM, Conor Dooley wrote: > > If only we had another user... I suppose you lads are the market leader > in these kinds of devices. If I did happen to know if Microchip was > working on anything similar (which I don't, I work on FPGAs not these > kinds of devices) I couldn't even tell you. I suppose I could ask around > and see. Do you know if TI is doing anything along these lines? > I think the most popular use case for the performance improvements made possible with a SPI offload unrelated to ADCs/DACs is for CAN controllers (e.g. the discussion from David Jander a few years ago). I think one of my colleagues was working on one in the last year that we might still have lying around. But I don't know what we could use as the SPI controller that would have offload support. I suppose we could make something using e.g. the PRU in a BeagleBone, but that would take lots of engineering resources and we could design it to fit whatever interface we want, so I'm not sure that really helps much. If we could find an off-the-shelf SPI controller with offload capabilities that would be helpful, but I don't know of any.
On Thu, 2024-05-30 at 20:18 +0100, Conor Dooley wrote: > On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote: > > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote: > > > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote: > > > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > > > > > Taking the > > > > trigger (PWM) as an example and even when it is directly connected with the > > > > offload > > > > block, the peripheral still needs to know about it. Think of sampling > > > > frequency... > > > > The period of the trigger signal is strictly connected with the sampling > > > > frequency of > > > > the peripheral for example. So I see 2 things: > > > > > > > > 1) Enabling/Disabling the trigger could be easily done from the peripheral > > > > even > > > > with > > > > the resource in the spi engine. I think David already has some code in the > > > > series > > > > that would make this trivial and so having the property in the spi controller > > > > brings > > > > no added complexity. > > > > > > > > 2) Controlling things like the trigger period/sample_rate. This could be > > > > harder > > > > to do > > > > over SPI (or making it generic enough) so we would still need to have the > > > > same > > > > property on the peripheral (even if not directly connected to it). I kind of > > > > agree > > > > with David that having the property both in the peripheral and controller is > > > > a > > > > bit > > > > weird. > > > > > > Can you explain what you mean by "same property on the peripheral"? I > > > would expect a peripheral to state its trigger period (just like how it > > > states the max frequency) and for the trigger period not to appear in > > > the controller. > > > > > > > Just have the same 'pwms' property on both the controller and peripheral... > > Yeah, no... Opinion unchanged since my last message. > ... > > > > If only we had another user... I suppose you lads are the market leader > in these kinds of devices. If I did happen to know if Microchip was > working on anything similar (which I don't, I work on FPGAs not these > kinds of devices) I couldn't even tell you. I suppose I could ask around > and see. Do you know if TI is doing anything along these lines? > Unfortunately, no idea. > > > Part of me says "sure, hook the DMAs up to the devices, as that's what > > > happens for other IIO devices" but at the same time I recognise that the > > > DMA isn't actually hooked up like that and the other IIO devices I see > > > like that are all actually on the SoC, rather than connected over SPI. > > > > Yeah, I know... But note (but again, only for ADI designs) that the DMA role is > > solely for carrying the peripheral data. It is done like this so everything works > > in > > HW and there's no need for SW to deal with the samples at all. I mean, only the > > userspace app touches the samples. > > > > TBH, the DMA is the bit that worries me the most as it may be overly complex to > > share > > buffers (using dma-buf or something else) from the spi controller back to > > consumers > > of it (IIO in this case). And I mean sharing in a way that there's no need to > > touch > > the buffers. > > <snip> > > > Maybe having an offload dedicated API (through spi) to get/share a DMA handle > > would > > be acceptable. Then we could add support to "import" it in the IIO core. Then it > > would be up to the controller to accept or not to share the handle (in some cases > > the > > controller could really want to have the control of the DMA transfers). > > Yeah, that is about what I was thinking. I wasn't expecting the spi code > to grow handing for dmabuf or anything like that, just a way for the > offload consumer to say "yo, can you tell me what dma buffer I can > use?". Unless (until?) there's some controller that wants to manage it, > I think that'd be sufficient? Yeah, I could see some kind of submit_request() API with some kind of completion handler for this. But on the IIO side the DMA code is not that straight (even getting more complex with dma-buf's) so I can't really tell how the whole thing would look like. But may be something to look at. - Nuno Sá
On 6/4/24 2:33 PM, Conor Dooley wrote: > On Thu, May 30, 2024 at 02:24:17PM -0500, David Lechner wrote: >> On 5/29/24 3:07 AM, Nuno Sá wrote: >>> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote: >> >> >>>> It might be easy to do it this way right now, but be problematic for a >>>> future device or if someone wants to chuck away the ADI provided RTL and >>>> do their own thing for this device. Really it just makes me wonder if >>>> what's needed to describe more complex data pipelines uses an of_graph, >>>> just like how video pipelines are handled, rather than the implementation >>>> of io-backends that don't really seem to model the flow of data. >>>> >>> >>> Yeah, backends is more for devices/soft-cores that extend the functionality of the >>> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high >>> speed controllers. Note that in some cases they also manipulate or even create data >>> but since they fit in IIO, having things like the DMA property in the hdl binding was >>> fairly straight. >>> >>> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would >>> be acceptable. Then we could add support to "import" it in the IIO core. Then it >>> would be up to the controller to accept or not to share the handle (in some cases the >>> controller could really want to have the control of the DMA transfers). >> >> I could see this working for some SPI controllers, but for the AXI SPI Engine >> + DMA currently, the DMA has a fixed word size, so can't be used as a generic >> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit >> word size, then even if we are reading 16-bit sample data, the DMA is going to >> put it in a 32-bit slot. So one could argue that this is still doing some data >> manipulation similar to the CRC checker example. >> >>> >>> Not familiar enough with of_graph so can't argue about it but likely is something >>> worth looking at. >> >> I did try implementing something using graph bindings when I first started >> working on this, but it didn't seem to really give us any extra useful >> information. It was just describing connections (endpoints) that I thought >> we could just implicitly assume. After this discussion though, maybe worth >> a second look. I'll have to think about it more. > > Could you elaborate on why you think you can assume the connections? What > happens when you have multiple stages of data processing and/or multiple > ADCs in your system? As I've previously said, I work on FPGA stuff, and > everyone here seems to fawn over having <insert custom DSP IP here> in > their data pipelines. I can't imagine it being any different for ADC data, > and an io-backend property that doesn't describe how the data flows is > gonna become lacklustre I think. I was more ignorant back then. :-) That is is why I said "thought" instead of "think". I am more enlightened now.
On 6/4/24 2:42 PM, Conor Dooley wrote: > On Tue, Jun 04, 2024 at 02:39:18PM -0500, David Lechner wrote: >> On 6/4/24 2:33 PM, Conor Dooley wrote: >>> On Thu, May 30, 2024 at 02:24:17PM -0500, David Lechner wrote: >>>> On 5/29/24 3:07 AM, Nuno Sá wrote: >>>>> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote: >>>> >>>> >>>>>> It might be easy to do it this way right now, but be problematic for a >>>>>> future device or if someone wants to chuck away the ADI provided RTL and >>>>>> do their own thing for this device. Really it just makes me wonder if >>>>>> what's needed to describe more complex data pipelines uses an of_graph, >>>>>> just like how video pipelines are handled, rather than the implementation >>>>>> of io-backends that don't really seem to model the flow of data. >>>>>> >>>>> >>>>> Yeah, backends is more for devices/soft-cores that extend the functionality of the >>>>> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high >>>>> speed controllers. Note that in some cases they also manipulate or even create data >>>>> but since they fit in IIO, having things like the DMA property in the hdl binding was >>>>> fairly straight. >>>>> >>>>> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would >>>>> be acceptable. Then we could add support to "import" it in the IIO core. Then it >>>>> would be up to the controller to accept or not to share the handle (in some cases the >>>>> controller could really want to have the control of the DMA transfers). >>>> >>>> I could see this working for some SPI controllers, but for the AXI SPI Engine >>>> + DMA currently, the DMA has a fixed word size, so can't be used as a generic >>>> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit >>>> word size, then even if we are reading 16-bit sample data, the DMA is going to >>>> put it in a 32-bit slot. So one could argue that this is still doing some data >>>> manipulation similar to the CRC checker example. >>>> >>>>> >>>>> Not familiar enough with of_graph so can't argue about it but likely is something >>>>> worth looking at. >>>> >>>> I did try implementing something using graph bindings when I first started >>>> working on this, but it didn't seem to really give us any extra useful >>>> information. It was just describing connections (endpoints) that I thought >>>> we could just implicitly assume. After this discussion though, maybe worth >>>> a second look. I'll have to think about it more. >>> >>> Could you elaborate on why you think you can assume the connections? What >>> happens when you have multiple stages of data processing and/or multiple >>> ADCs in your system? As I've previously said, I work on FPGA stuff, and >>> everyone here seems to fawn over having <insert custom DSP IP here> in >>> their data pipelines. I can't imagine it being any different for ADC data, >>> and an io-backend property that doesn't describe how the data flows is >>> gonna become lacklustre I think. >> >> I was more ignorant back then. :-) >> >> That is is why I said "thought" instead of "think". I am more enlightened now. > > Heh, I didn't mean it in a bad way. I just wanted to flesh out why you > thought that way. > Back then, we were going on the assumption that no one would want to make their own custom IP and only use the IP blocks provided by ADI for ADI chips. So given chip X + SPI offload, we could assume HDL project Y was being used.
Continuing the discussion started a few months ago [1]... It was suggested that we were trying to do too much in one series. So we split out part of the series into a separate series that adds generic support for pre-preparing SPI messages [2]. That work has been merged. [1]: https://lore.kernel.org/linux-spi/20240109-axi-spi-engine-series-3-v1-0-e42c6a986580@baylibre.com/ [2]: https://lore.kernel.org/linux-spi/20240219-mainline-spi-precook-message-v2-0-4a762c6701b9@baylibre.com/ I was hoping to also break down the remaining work into smaller parts by first only considering more generic offload support that could be used with the regular SPI message queue and leaving out the hardware trigger bits. But the hardware I have at hand is not capable of doing that so this is the best I can do for now. (Happy to take suggestions if someone knows some other hardware that could be used for this.) There have been significant changes since the last version so I'll discuss what changed in each patch instead of having a very lengthy cover letter. A working branch complete with extra hacks can be found at [3]. [3]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v2 --- 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 piped 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 (8): spi: dt-bindings: spi-peripheral-props: add spi-offloads property spi: add basic support for SPI offloading spi: add support for hardware triggered offload spi: add offload xfer flags spi: dt-bindings: axi-spi-engine: document spi-offloads spi: axi-spi-engine: add offload support 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 | 58 +++++ .../bindings/spi/adi,axi-spi-engine.yaml | 14 ++ .../bindings/spi/spi-peripheral-props.yaml | 10 + drivers/iio/adc/ad7944.c | 147 +++++++++--- drivers/spi/spi-axi-spi-engine.c | 267 ++++++++++++++++++++- drivers/spi/spi.c | 202 +++++++++++++++- include/linux/spi/spi.h | 84 +++++++ 7 files changed, 741 insertions(+), 41 deletions(-) --- base-commit: 14fde009028126f91c2bd72c404e425cf4f8aec3 change-id: 20240510-dlech-mainline-spi-engine-offload-2-afce3790b5ab Best regards,