Message-ID: <8a6e9e90-5c0c-33e8-e349-456384668903@st.com>
Date:   Mon, 30 Jan 2017 09:57:02 +0100
From:   Fabrice Gasnier <fabrice.gasnier@...com>
To:     Jonathan Cameron <jic23@...nel.org>, <linux@...linux.org.uk>,
        <robh+dt@...nel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     <linux-iio@...r.kernel.org>, <mark.rutland@....com>,
        <mcoquelin.stm32@...il.com>, <alexandre.torgue@...com>,
        <lars@...afoo.de>, <knaack.h@....de>, <pmeerw@...erw.net>,
        <benjamin.gaignard@...aro.org>, <benjamin.gaignard@...com>
Subject: Re: [PATCH v2 5/7] iio: adc: stm32: add optional dma support

On 01/28/2017 07:41 PM, Jonathan Cameron wrote:
> On 26/01/17 14:28, Fabrice Gasnier wrote:
>> Add DMA optional support to STM32 ADC, as there is a limited number DMA
>> channels (request lines) that can be assigned to ADC. This way, driver
>> may fall back using interrupts when all DMA channels are in use for
>> other IPs.
>> Use dma cyclic mode with two periods. Allow to tune period length by
>> using watermark. Coherent memory is used for dma (max buffer size is
>> fixed to PAGE_SIZE).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
> I am happy with this whole series, except this patch gives me:
>
> drivers/iio/adc/stm32-adc.c:478:23: warning: symbol 'stm32_adc_trig_pol' was not declared. Should it be static?
> drivers/iio/adc/stm32-adc.c:621:21: error: incompatible types in comparison expression (different type sizes)
>    CC [M]  drivers/iio/adc/stm32-adc.o
> In file included from ./include/linux/clk.h:16:0,
>                   from drivers/iio/adc/stm32-adc.c:22:
> drivers/iio/adc/stm32-adc.c: In function ‘stm32_adc_set_watermark’:
> ./include/linux/kernel.h:753:16: warning: comparison of distinct pointer types lacks a cast
>    (void) (&min1 == &min2);   \
>                  ^
> ./include/linux/kernel.h:756:2: note: in expansion of macro ‘__min’
>    __min(typeof(x), typeof(y),   \
>    ^~~~~
> drivers/iio/adc/stm32-adc.c:621:14: note: in expansion of macro ‘min’
>    watermark = min(watermark, val * sizeof(u16));
>                ^~~
>
> The static is obviously fine so I've added that.
> The second looks to be because sizeof(u16) is a size_t which is signed IIRC.
> Anyhow, a cast of that to unsigned should I think be harmless and fixes the
> warning.
>
> Please check I did these right.

Hi Jonathan,

I just checked, this is ok for me.

Many thanks!
Best Regards,
Fabrice
>
> They are in the testing branch of iio.git.
>
> Thanks,
>
> Jonathan
>
>> ---
>> Changes in v2:
>> - Use iio_trigger_poll_chained() avoids to bounce back into irq context.
>>    Remove irq_work.
>> - Rework dma buffer allocation and use. Allocation moved to probe time,
>>    fixed to PAGE_SIZE. Use hwfifo_set_watermark() routine to tune dma
>>    cyclic period length.
>> ---
>>   drivers/iio/adc/Kconfig          |   1 +
>>   drivers/iio/adc/stm32-adc-core.c |   1 +
>>   drivers/iio/adc/stm32-adc-core.h |   2 +
>>   drivers/iio/adc/stm32-adc.c      | 227 ++++++++++++++++++++++++++++++++++++---
>>   4 files changed, 218 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 9a7b090..03a73f9 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -444,6 +444,7 @@ config ROCKCHIP_SARADC
>>   config STM32_ADC_CORE
>>   	tristate "STMicroelectronics STM32 adc core"
>>   	depends on ARCH_STM32 || COMPILE_TEST
>> +	depends on HAS_DMA
>>   	depends on OF
>>   	depends on REGULATOR
>>   	select IIO_BUFFER
>> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
>> index 4214b0c..22b7c93 100644
>> --- a/drivers/iio/adc/stm32-adc-core.c
>> +++ b/drivers/iio/adc/stm32-adc-core.c
>> @@ -201,6 +201,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
>>   	priv->common.base = devm_ioremap_resource(&pdev->dev, res);
>>   	if (IS_ERR(priv->common.base))
>>   		return PTR_ERR(priv->common.base);
>> +	priv->common.phys_base = res->start;
>>   
>>   	priv->vref = devm_regulator_get(&pdev->dev, "vref");
>>   	if (IS_ERR(priv->vref)) {
>> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
>> index 081fa5f..2ec7abb 100644
>> --- a/drivers/iio/adc/stm32-adc-core.h
>> +++ b/drivers/iio/adc/stm32-adc-core.h
>> @@ -42,10 +42,12 @@
>>   /**
>>    * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
>>    * @base:		control registers base cpu addr
>> + * @phys_base:		control registers base physical addr
>>    * @vref_mv:		vref voltage (mv)
>>    */
>>   struct stm32_adc_common {
>>   	void __iomem			*base;
>> +	phys_addr_t			phys_base;
>>   	int				vref_mv;
>>   };
>>   
>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>> index 9a38f9a..8a30039 100644
>> --- a/drivers/iio/adc/stm32-adc.c
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -21,6 +21,8 @@
>>   
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/buffer.h>
>>   #include <linux/iio/timer/stm32-timer-trigger.h>
>> @@ -68,12 +70,16 @@
>>   #define STM32F4_EXTSEL_SHIFT		24
>>   #define STM32F4_EXTSEL_MASK		GENMASK(27, 24)
>>   #define STM32F4_EOCS			BIT(10)
>> +#define STM32F4_DDS			BIT(9)
>> +#define STM32F4_DMA			BIT(8)
>>   #define STM32F4_ADON			BIT(0)
>>   
>>   #define STM32_ADC_MAX_SQ		16	/* SQ1..SQ16 */
>>   #define STM32_ADC_TIMEOUT_US		100000
>>   #define STM32_ADC_TIMEOUT	(msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
>>   
>> +#define STM32_DMA_BUFFER_SIZE		PAGE_SIZE
>> +
>>   /* External trigger enable */
>>   enum stm32_adc_exten {
>>   	STM32_EXTEN_SWTRIG,
>> @@ -136,6 +142,10 @@ struct stm32_adc_regs {
>>    * @bufi:		data buffer index
>>    * @num_conv:		expected number of scan conversions
>>    * @trigger_polarity:	external trigger polarity (e.g. exten)
>> + * @dma_chan:		dma channel
>> + * @rx_buf:		dma rx buffer cpu address
>> + * @rx_dma_buf:		dma rx buffer bus address
>> + * @rx_buf_sz:		dma rx buffer size
>>    */
>>   struct stm32_adc {
>>   	struct stm32_adc_common	*common;
>> @@ -148,6 +158,10 @@ struct stm32_adc {
>>   	unsigned int		bufi;
>>   	unsigned int		num_conv;
>>   	u32			trigger_polarity;
>> +	struct dma_chan		*dma_chan;
>> +	u8			*rx_buf;
>> +	dma_addr_t		rx_dma_buf;
>> +	unsigned int		rx_buf_sz;
>>   };
>>   
>>   /**
>> @@ -291,10 +305,21 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>>   /**
>>    * stm32_adc_start_conv() - Start conversions for regular channels.
>>    * @adc: stm32 adc instance
>> + * @dma: use dma to transfer conversion result
>> + *
>> + * Start conversions for regular channels.
>> + * Also take care of normal or DMA mode. Circular DMA may be used for regular
>> + * conversions, in IIO buffer modes. Otherwise, use ADC interrupt with direct
>> + * DR read instead (e.g. read_raw, or triggered buffer mode without DMA).
>>    */
>> -static void stm32_adc_start_conv(struct stm32_adc *adc)
>> +static void stm32_adc_start_conv(struct stm32_adc *adc, bool dma)
>>   {
>>   	stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>> +
>> +	if (dma)
>> +		stm32_adc_set_bits(adc, STM32F4_ADC_CR2,
>> +				   STM32F4_DMA | STM32F4_DDS);
>> +
>>   	stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_EOCS | STM32F4_ADON);
>>   
>>   	/* Wait for Power-up time (tSTAB from datasheet) */
>> @@ -311,7 +336,8 @@ static void stm32_adc_stop_conv(struct stm32_adc *adc)
>>   	stm32_adc_clr_bits(adc, STM32F4_ADC_SR, STM32F4_STRT);
>>   
>>   	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>> -	stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_ADON);
>> +	stm32_adc_clr_bits(adc, STM32F4_ADC_CR2,
>> +			   STM32F4_ADON | STM32F4_DMA | STM32F4_DDS);
>>   }
>>   
>>   /**
>> @@ -494,7 +520,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>>   
>>   	stm32_adc_conv_irq_enable(adc);
>>   
>> -	stm32_adc_start_conv(adc);
>> +	stm32_adc_start_conv(adc, false);
>>   
>>   	timeout = wait_for_completion_interruptible_timeout(
>>   					&adc->completion, STM32_ADC_TIMEOUT);
>> @@ -581,6 +607,23 @@ static int stm32_adc_validate_trigger(struct iio_dev *indio_dev,
>>   	return stm32_adc_get_trig_extsel(trig) < 0 ? -EINVAL : 0;
>>   }
>>   
>> +static int stm32_adc_set_watermark(struct iio_dev *indio_dev, unsigned val)
>> +{
>> +	struct stm32_adc *adc = iio_priv(indio_dev);
>> +	unsigned int watermark = STM32_DMA_BUFFER_SIZE / 2;
>> +
>> +	/*
>> +	 * dma cyclic transfers are used, buffer is split into two periods.
>> +	 * There should be :
>> +	 * - always one buffer (period) dma is working on
>> +	 * - one buffer (period) driver can push with iio_trigger_poll().
>> +	 */
>> +	watermark = min(watermark, val * sizeof(u16));
>> +	adc->rx_buf_sz = watermark * 2;
>> +
>> +	return 0;
>> +}
>> +
>>   static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
>>   				      const unsigned long *scan_mask)
>>   {
>> @@ -635,12 +678,83 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>>   static const struct iio_info stm32_adc_iio_info = {
>>   	.read_raw = stm32_adc_read_raw,
>>   	.validate_trigger = stm32_adc_validate_trigger,
>> +	.hwfifo_set_watermark = stm32_adc_set_watermark,
>>   	.update_scan_mode = stm32_adc_update_scan_mode,
>>   	.debugfs_reg_access = stm32_adc_debugfs_reg_access,
>>   	.of_xlate = stm32_adc_of_xlate,
>>   	.driver_module = THIS_MODULE,
>>   };
>>   
>> +static unsigned int stm32_adc_dma_residue(struct stm32_adc *adc)
>> +{
>> +	struct dma_tx_state state;
>> +	enum dma_status status;
>> +
>> +	status = dmaengine_tx_status(adc->dma_chan,
>> +				     adc->dma_chan->cookie,
>> +				     &state);
>> +	if (status == DMA_IN_PROGRESS) {
>> +		/* Residue is size in bytes from end of buffer */
>> +		unsigned int i = adc->rx_buf_sz - state.residue;
>> +		unsigned int size;
>> +
>> +		/* Return available bytes */
>> +		if (i >= adc->bufi)
>> +			size = i - adc->bufi;
>> +		else
>> +			size = adc->rx_buf_sz + i - adc->bufi;
>> +
>> +		return size;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void stm32_adc_dma_buffer_done(void *data)
>> +{
>> +	struct iio_dev *indio_dev = data;
>> +
>> +	iio_trigger_poll_chained(indio_dev->trig);
>> +}
>> +
>> +static int stm32_adc_dma_start(struct iio_dev *indio_dev)
>> +{
>> +	struct stm32_adc *adc = iio_priv(indio_dev);
>> +	struct dma_async_tx_descriptor *desc;
>> +	dma_cookie_t cookie;
>> +	int ret;
>> +
>> +	if (!adc->dma_chan)
>> +		return 0;
>> +
>> +	dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__,
>> +		adc->rx_buf_sz, adc->rx_buf_sz / 2);
>> +
>> +	/* Prepare a DMA cyclic transaction */
>> +	desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
>> +					 adc->rx_dma_buf,
>> +					 adc->rx_buf_sz, adc->rx_buf_sz / 2,
>> +					 DMA_DEV_TO_MEM,
>> +					 DMA_PREP_INTERRUPT);
>> +	if (!desc)
>> +		return -EBUSY;
>> +
>> +	desc->callback = stm32_adc_dma_buffer_done;
>> +	desc->callback_param = indio_dev;
>> +
>> +	cookie = dmaengine_submit(desc);
>> +	ret = dma_submit_error(cookie);
>> +	if (ret) {
>> +		dmaengine_terminate_all(adc->dma_chan);
>> +		return ret;
>> +	}
>> +
>> +	/* Issue pending DMA requests */
>> +	dma_async_issue_pending(adc->dma_chan);
>> +
>> +	return 0;
>> +}
>> +
>>   static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>>   {
>>   	struct stm32_adc *adc = iio_priv(indio_dev);
>> @@ -652,18 +766,29 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>>   		return ret;
>>   	}
>>   
>> +	ret = stm32_adc_dma_start(indio_dev);
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "Can't start dma\n");
>> +		goto err_clr_trig;
>> +	}
>> +
>>   	ret = iio_triggered_buffer_postenable(indio_dev);
>>   	if (ret < 0)
>> -		goto err_clr_trig;
>> +		goto err_stop_dma;
>>   
>>   	/* Reset adc buffer index */
>>   	adc->bufi = 0;
>>   
>> -	stm32_adc_conv_irq_enable(adc);
>> -	stm32_adc_start_conv(adc);
>> +	if (!adc->dma_chan)
>> +		stm32_adc_conv_irq_enable(adc);
>> +
>> +	stm32_adc_start_conv(adc, !!adc->dma_chan);
>>   
>>   	return 0;
>>   
>> +err_stop_dma:
>> +	if (adc->dma_chan)
>> +		dmaengine_terminate_all(adc->dma_chan);
>>   err_clr_trig:
>>   	stm32_adc_set_trig(indio_dev, NULL);
>>   
>> @@ -676,12 +801,16 @@ static int stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
>>   	int ret;
>>   
>>   	stm32_adc_stop_conv(adc);
>> -	stm32_adc_conv_irq_disable(adc);
>> +	if (!adc->dma_chan)
>> +		stm32_adc_conv_irq_disable(adc);
>>   
>>   	ret = iio_triggered_buffer_predisable(indio_dev);
>>   	if (ret < 0)
>>   		dev_err(&indio_dev->dev, "predisable failed\n");
>>   
>> +	if (adc->dma_chan)
>> +		dmaengine_terminate_all(adc->dma_chan);
>> +
>>   	if (stm32_adc_set_trig(indio_dev, NULL))
>>   		dev_err(&indio_dev->dev, "Can't clear trigger\n");
>>   
>> @@ -701,15 +830,31 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>>   
>>   	dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
>>   
>> -	/* reset buffer index */
>> -	adc->bufi = 0;
>> -	iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
>> -					   pf->timestamp);
>> +	if (!adc->dma_chan) {
>> +		/* reset buffer index */
>> +		adc->bufi = 0;
>> +		iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
>> +						   pf->timestamp);
>> +	} else {
>> +		int residue = stm32_adc_dma_residue(adc);
>> +
>> +		while (residue >= indio_dev->scan_bytes) {
>> +			u16 *buffer = (u16 *)&adc->rx_buf[adc->bufi];
>> +
>> +			iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>> +							   pf->timestamp);
>> +			residue -= indio_dev->scan_bytes;
>> +			adc->bufi += indio_dev->scan_bytes;
>> +			if (adc->bufi >= adc->rx_buf_sz)
>> +				adc->bufi = 0;
>> +		}
>> +	}
>>   
>>   	iio_trigger_notify_done(indio_dev->trig);
>>   
>>   	/* re-enable eoc irq */
>> -	stm32_adc_conv_irq_enable(adc);
>> +	if (!adc->dma_chan)
>> +		stm32_adc_conv_irq_enable(adc);
>>   
>>   	return IRQ_HANDLED;
>>   }
>> @@ -781,6 +926,45 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>>   	return 0;
>>   }
>>   
>> +static int stm32_adc_dma_request(struct iio_dev *indio_dev)
>> +{
>> +	struct stm32_adc *adc = iio_priv(indio_dev);
>> +	struct dma_slave_config config;
>> +	int ret;
>> +
>> +	adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
>> +	if (!adc->dma_chan)
>> +		return 0;
>> +
>> +	adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
>> +					 STM32_DMA_BUFFER_SIZE,
>> +					 &adc->rx_dma_buf, GFP_KERNEL);
>> +	if (!adc->rx_buf) {
>> +		goto err_release;
>> +		ret = -ENOMEM;
>> +	}
>> +
>> +	/* Configure DMA channel to read data register */
>> +	memset(&config, 0, sizeof(config));
>> +	config.src_addr = (dma_addr_t)adc->common->phys_base;
>> +	config.src_addr += adc->offset + STM32F4_ADC_DR;
>> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>> +
>> +	ret = dmaengine_slave_config(adc->dma_chan, &config);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	dma_free_coherent(adc->dma_chan->device->dev, STM32_DMA_BUFFER_SIZE,
>> +			  adc->rx_buf, adc->rx_dma_buf);
>> +err_release:
>> +	dma_release_channel(adc->dma_chan);
>> +
>> +	return ret;
>> +}
>> +
>>   static int stm32_adc_probe(struct platform_device *pdev)
>>   {
>>   	struct iio_dev *indio_dev;
>> @@ -842,13 +1026,17 @@ static int stm32_adc_probe(struct platform_device *pdev)
>>   	if (ret < 0)
>>   		goto err_clk_disable;
>>   
>> +	ret = stm32_adc_dma_request(indio_dev);
>> +	if (ret < 0)
>> +		goto err_clk_disable;
>> +
>>   	ret = iio_triggered_buffer_setup(indio_dev,
>>   					 &iio_pollfunc_store_time,
>>   					 &stm32_adc_trigger_handler,
>>   					 &stm32_adc_buffer_setup_ops);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "buffer setup failed\n");
>> -		goto err_clk_disable;
>> +		goto err_dma_disable;
>>   	}
>>   
>>   	ret = iio_device_register(indio_dev);
>> @@ -862,6 +1050,13 @@ static int stm32_adc_probe(struct platform_device *pdev)
>>   err_buffer_cleanup:
>>   	iio_triggered_buffer_cleanup(indio_dev);
>>   
>> +err_dma_disable:
>> +	if (adc->dma_chan) {
>> +		dma_free_coherent(adc->dma_chan->device->dev,
>> +				  STM32_DMA_BUFFER_SIZE,
>> +				  adc->rx_buf, adc->rx_dma_buf);
>> +		dma_release_channel(adc->dma_chan);
>> +	}
>>   err_clk_disable:
>>   	clk_disable_unprepare(adc->clk);
>>   
>> @@ -875,6 +1070,12 @@ static int stm32_adc_remove(struct platform_device *pdev)
>>   
>>   	iio_device_unregister(indio_dev);
>>   	iio_triggered_buffer_cleanup(indio_dev);
>> +	if (adc->dma_chan) {
>> +		dma_free_coherent(adc->dma_chan->device->dev,
>> +				  STM32_DMA_BUFFER_SIZE,
>> +				  adc->rx_buf, adc->rx_dma_buf);
>> +		dma_release_channel(adc->dma_chan);
>> +	}
>>   	clk_disable_unprepare(adc->clk);
>>   
>>   	return 0;
>>

Powered by blists - more mailing lists