Re: [PATCH v3 3/4] iio: adc: at91-sama5d2_adc: add support for DMA
On Wed, 15 Nov 2017 14:56:47 +0200
Eugen Hristev wrote:
> Added support for DMA transfers. The implementation uses the user watermark
> to decide whether DMA will be used or not. For watermark 1, DMA will not be
> used. If watermark is bigger, DMA will be used.
> Sysfs attributes are created to indicate whether the DMA is used,
> with hwfifo_enabled, and the current DMA watermark is readable
> in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
> and hwfifo_watermark_max.
>
> Signed-off-by: Eugen Hristev
A tiny nitpick inline but otherwise looks fine to me.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
> ---
> Changes in v3:
> - Remove misleaded dev_info message when DMA was not enabled at probe
> - Rebased patch on top of the
> [PATCH] iio: adc: at91-sama5d2_adc: fix probe error on missing trigger
> property
> Which is already upstreamed in 4.14
> - Fixed the bug introduced in v2, with buffer size
> - added extra check when enabling DMA, to have hw trigger present.
> This is because now, we can have the driver with software trigger only (if no
> hw trigger in device tree, start as software only)
>
> Changes in v2:
> - No longer add last timestamp to all samples. Now, compute an interval
> between samples w.r.t. start and end time of the transfer and number
> of samples. Then distribute them each in the time interval.
> - Add warning for conversion overrun. This helps user identify cases
> when the watermark needs adjustment : the software is too slow in reading
> data from the ADC.
> - Protection around watermark is not needed, changing of the watermark
> cannot be done while the buffer is enabled. When buffer is disabled, all
> DMA resources are freed anyway.
> - Added validation on trigger to be used by own device
> - Best sample rate I could obtain using the low frequency clock was about
> 4k samples/second, with a watermark of 100. To get up to 50k samples/second
> the ADC frequency must be increased to max.
> - Fixed computation of DMA buffer size
> - Addressed other comments from mailing list review. Feedback is appreciated
>
> drivers/iio/adc/Kconfig| 1 +
> drivers/iio/adc/at91-sama5d2_adc.c | 453
> +++--
> 2 files changed, 434 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1d13bf0..1a3a8e3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
> tristate "Atmel AT91 SAMA5D2 ADC"
> depends on ARCH_AT91 || COMPILE_TEST
> depends on HAS_IOMEM
> + depends on HAS_DMA
> select IIO_TRIGGERED_BUFFER
> help
> Say yes here to build support for Atmel SAMA5D2 ADC which is
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c
> b/drivers/iio/adc/at91-sama5d2_adc.c
> index a70ef7f..11d34a8 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -16,6 +16,8 @@
>
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -100,6 +102,8 @@
> #define AT91_SAMA5D2_LCDR0x20
> /* Interrupt Enable Register */
> #define AT91_SAMA5D2_IER 0x24
> +/* Interrupt Enable Register - general overrun error */
> +#define AT91_SAMA5D2_IER_GOVRE BIT(25)
> /* Interrupt Disable Register */
> #define AT91_SAMA5D2_IDR 0x28
> /* Interrupt Mask Register */
> @@ -167,13 +171,19 @@
>
> /*
> * Maximum number of bytes to hold conversion from all channels
> - * plus the timestamp
> + * without the timestamp.
> */
> -#define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +
> \
> - AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
> +#define AT91_BUFFER_MAX_CONVERSION_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT + \
> + AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
> +
> +/* This total must also include the timestamp */
> +#define AT91_BUFFER_MAX_BYTES (AT91_BUFFER_MAX_CONVERSION_BYTES + 8)
>
> #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
>
> +#define AT91_HWFIFO_MAX_SIZE_STR "128"
> +#define AT91_HWFIFO_MAX_SIZE 128
> +
> #define AT91_SAMA5D2_CHAN_SINGLE(num, addr) \
> { \
> .type = IIO_VOLTAGE,\
> @@ -228,6 +238,28 @@ struct at91_adc_trigger {
> boolhw_trig;
> };
>
> +/**
> + * at91_adc_dma - at91-sama5d2 dma information struct
> + * @dma_chan:the dma channel acquired
> + * @rx_buf: dma coherent allocated area
> + * @rx_dma_buf: dma handler for the buffer
> + * @phys_addr: physical address of the ADC base register
> + * @buf_idx: index inside the dma buffer where reading was last done
> + * @rx_b
[PATCH v3 3/4] iio: adc: at91-sama5d2_adc: add support for DMA
Added support for DMA transfers. The implementation uses the user watermark
to decide whether DMA will be used or not. For watermark 1, DMA will not be
used. If watermark is bigger, DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.
Signed-off-by: Eugen Hristev
---
Changes in v3:
- Remove misleaded dev_info message when DMA was not enabled at probe
- Rebased patch on top of the
[PATCH] iio: adc: at91-sama5d2_adc: fix probe error on missing trigger property
Which is already upstreamed in 4.14
- Fixed the bug introduced in v2, with buffer size
- added extra check when enabling DMA, to have hw trigger present.
This is because now, we can have the driver with software trigger only (if no
hw trigger in device tree, start as software only)
Changes in v2:
- No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
- Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
- Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
- Added validation on trigger to be used by own device
- Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
- Fixed computation of DMA buffer size
- Addressed other comments from mailing list review. Feedback is appreciated
drivers/iio/adc/Kconfig| 1 +
drivers/iio/adc/at91-sama5d2_adc.c | 453 +++--
2 files changed, 434 insertions(+), 20 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1d13bf0..1a3a8e3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
tristate "Atmel AT91 SAMA5D2 ADC"
depends on ARCH_AT91 || COMPILE_TEST
depends on HAS_IOMEM
+ depends on HAS_DMA
select IIO_TRIGGERED_BUFFER
help
Say yes here to build support for Atmel SAMA5D2 ADC which is
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c
b/drivers/iio/adc/at91-sama5d2_adc.c
index a70ef7f..11d34a8 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -16,6 +16,8 @@
#include
#include
+#include
+#include
#include
#include
#include
@@ -100,6 +102,8 @@
#define AT91_SAMA5D2_LCDR 0x20
/* Interrupt Enable Register */
#define AT91_SAMA5D2_IER 0x24
+/* Interrupt Enable Register - general overrun error */
+#define AT91_SAMA5D2_IER_GOVRE BIT(25)
/* Interrupt Disable Register */
#define AT91_SAMA5D2_IDR 0x28
/* Interrupt Mask Register */
@@ -167,13 +171,19 @@
/*
* Maximum number of bytes to hold conversion from all channels
- * plus the timestamp
+ * without the timestamp.
*/
-#define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT + \
- AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
+#define AT91_BUFFER_MAX_CONVERSION_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT + \
+AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
+
+/* This total must also include the timestamp */
+#define AT91_BUFFER_MAX_BYTES (AT91_BUFFER_MAX_CONVERSION_BYTES + 8)
#define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
+#define AT91_HWFIFO_MAX_SIZE_STR "128"
+#define AT91_HWFIFO_MAX_SIZE 128
+
#define AT91_SAMA5D2_CHAN_SINGLE(num, addr)\
{ \
.type = IIO_VOLTAGE,\
@@ -228,6 +238,28 @@ struct at91_adc_trigger {
boolhw_trig;
};
+/**
+ * at91_adc_dma - at91-sama5d2 dma information struct
+ * @dma_chan: the dma channel acquired
+ * @rx_buf:dma coherent allocated area
+ * @rx_dma_buf:dma handler for the buffer
+ * @phys_addr: physical address of the ADC base register
+ * @buf_idx: index inside the dma buffer where reading was last done
+ * @rx_buf_sz: size of buffer used by DMA operation
+ * @watermark: number of conversions to copy before DMA triggers irq
+ * @dma_ts:hold the start timestamp of dma operation
+ */
+struct at91_adc_dma {
+ struct dma_chan *dma_chan;
+ u8 *rx_buf;
+ dma_addr_t rx_dma_buf;
+ phys_addr_t phys_addr;
+ int
