diff mbox

[v4,2/6] mfd: stm32-timers: add support for dmas

Message ID 1523895561-4073-3-git-send-email-fabrice.gasnier@st.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fabrice Gasnier April 16, 2018, 4:19 p.m. UTC 
STM32 Timers can support up to 7 DMA requests:
- 4 channels, update, compare and trigger.
Optionally request part, or all DMAs from stm32-timers MFD core.

Also add routine to implement burst reads using DMA from timer registers.
This is exported. So, it can be used by child drivers, PWM capture
for instance (but not limited to).

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
Changes in v4:
- Lee's comments: Add kerneldoc header, better format comments.
Changes in v3:
- Basically Lee's comments:
- rather create a struct stm32_timers_dma, and place a reference to it
  in existing ddata (instead of adding priv struct).
- rather use a struct device in exported routine prototype, and use
  standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
- simplify error handling in probe (remove a goto)
- comment on devm_of_platform_*populate() usage.

Changes in v2:
- Abstract DMA handling from child driver: move it to MFD core
- Add comments on optional dma support
---
 drivers/mfd/stm32-timers.c       | 227 ++++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/stm32-timers.h |  32 ++++++
 2 files changed, 257 insertions(+), 2 deletions(-)

Comments

Lee Jones April 17, 2018, 7:12 a.m. UTC | #1 
On Mon, 16 Apr 2018, Fabrice Gasnier wrote:

> STM32 Timers can support up to 7 DMA requests:
> - 4 channels, update, compare and trigger.
> Optionally request part, or all DMAs from stm32-timers MFD core.
> 
> Also add routine to implement burst reads using DMA from timer registers.
> This is exported. So, it can be used by child drivers, PWM capture
> for instance (but not limited to).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
> Changes in v4:
> - Lee's comments: Add kerneldoc header, better format comments.
> Changes in v3:
> - Basically Lee's comments:
> - rather create a struct stm32_timers_dma, and place a reference to it
>   in existing ddata (instead of adding priv struct).
> - rather use a struct device in exported routine prototype, and use
>   standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
> - simplify error handling in probe (remove a goto)
> - comment on devm_of_platform_*populate() usage.
> 
> Changes in v2:
> - Abstract DMA handling from child driver: move it to MFD core
> - Add comments on optional dma support
> ---
>  drivers/mfd/stm32-timers.c       | 227 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/stm32-timers.h |  32 ++++++
>  2 files changed, 257 insertions(+), 2 deletions(-)

[...]

> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> index 2aadab6..a04d7a1 100644
> --- a/include/linux/mfd/stm32-timers.h
> +++ b/include/linux/mfd/stm32-timers.h
> @@ -8,6 +8,8 @@
>  #define _LINUX_STM32_GPTIMER_H_
>  
>  #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/regmap.h>

[...]

> +struct stm32_timers_dma;
> +
>  struct stm32_timers {
>  	struct clk *clk;
>  	struct regmap *regmap;
>  	u32 max_arr;
> +	struct stm32_timers_dma *dma; /* Only to be used by the parent */

I'm confused.  I thought the point of putting this comment in was so
that you could place the definition of 'stm32_timers_dma' and remove
the forward declaration?

>  };
> +
> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
> +				enum stm32_timers_dmas id, u32 reg,
> +				unsigned int num_reg, unsigned int bursts,
> +				unsigned long tmo_ms);
>  #endif
Fabrice Gasnier April 17, 2018, 7:37 a.m. UTC | #2 
On 04/17/2018 09:12 AM, Lee Jones wrote:
> On Mon, 16 Apr 2018, Fabrice Gasnier wrote:
> 
>> STM32 Timers can support up to 7 DMA requests:
>> - 4 channels, update, compare and trigger.
>> Optionally request part, or all DMAs from stm32-timers MFD core.
>>
>> Also add routine to implement burst reads using DMA from timer registers.
>> This is exported. So, it can be used by child drivers, PWM capture
>> for instance (but not limited to).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> ---
>> Changes in v4:
>> - Lee's comments: Add kerneldoc header, better format comments.
>> Changes in v3:
>> - Basically Lee's comments:
>> - rather create a struct stm32_timers_dma, and place a reference to it
>>   in existing ddata (instead of adding priv struct).
>> - rather use a struct device in exported routine prototype, and use
>>   standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
>> - simplify error handling in probe (remove a goto)
>> - comment on devm_of_platform_*populate() usage.
>>
>> Changes in v2:
>> - Abstract DMA handling from child driver: move it to MFD core
>> - Add comments on optional dma support
>> ---
>>  drivers/mfd/stm32-timers.c       | 227 ++++++++++++++++++++++++++++++++++++++-
>>  include/linux/mfd/stm32-timers.h |  32 ++++++
>>  2 files changed, 257 insertions(+), 2 deletions(-)
> 
> [...]
> 
>> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
>> index 2aadab6..a04d7a1 100644
>> --- a/include/linux/mfd/stm32-timers.h
>> +++ b/include/linux/mfd/stm32-timers.h
>> @@ -8,6 +8,8 @@
>>  #define _LINUX_STM32_GPTIMER_H_
>>  
>>  #include <linux/clk.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/dma-mapping.h>
>>  #include <linux/regmap.h>
> 
> [...]
> 
>> +struct stm32_timers_dma;
>> +
>>  struct stm32_timers {
>>  	struct clk *clk;
>>  	struct regmap *regmap;
>>  	u32 max_arr;
>> +	struct stm32_timers_dma *dma; /* Only to be used by the parent */
> 
> I'm confused.  I thought the point of putting this comment in was so
> that you could place the definition of 'stm32_timers_dma' and remove
> the forward declaration?

Hi Lee,

Sorry, if I miss-understood the point then. So, do you wish I both:
- move the full struct definition in above header ?
- and keep this comment ?

+/**
+ * struct stm32_timers_dma - STM32 timer DMA handling.
+ * @completion:		end of DMA transfer completion
+ * @phys_base:		control registers physical base address
+ * @lock:		protect DMA access
+ * @chan:		DMA channel in use
+ * @chans:		DMA channels available for this timer instance
+ */
+struct stm32_timers_dma {
+	struct completion completion;
+	phys_addr_t phys_base;
+	struct mutex lock;
+	struct dma_chan *chan;
+	struct dma_chan *chans[STM32_TIMERS_MAX_DMAS];
+};

This will basically expose the struct to child drivers. But I'm ok if
you think this is acceptable.

I can send a V5 if you wish...

Please advise,
Best regards,
Fabrice

> 
>>  };
>> +
>> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
>> +				enum stm32_timers_dmas id, u32 reg,
>> +				unsigned int num_reg, unsigned int bursts,
>> +				unsigned long tmo_ms);
>>  #endif
>
Lee Jones April 17, 2018, 10:10 a.m. UTC | #3 
On Tue, 17 Apr 2018, Fabrice Gasnier wrote:

> On 04/17/2018 09:12 AM, Lee Jones wrote:
> > On Mon, 16 Apr 2018, Fabrice Gasnier wrote:
> > 
> >> STM32 Timers can support up to 7 DMA requests:
> >> - 4 channels, update, compare and trigger.
> >> Optionally request part, or all DMAs from stm32-timers MFD core.
> >>
> >> Also add routine to implement burst reads using DMA from timer registers.
> >> This is exported. So, it can be used by child drivers, PWM capture
> >> for instance (but not limited to).
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> >> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >> ---
> >> Changes in v4:
> >> - Lee's comments: Add kerneldoc header, better format comments.
> >> Changes in v3:
> >> - Basically Lee's comments:
> >> - rather create a struct stm32_timers_dma, and place a reference to it
> >>   in existing ddata (instead of adding priv struct).
> >> - rather use a struct device in exported routine prototype, and use
> >>   standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
> >> - simplify error handling in probe (remove a goto)
> >> - comment on devm_of_platform_*populate() usage.
> >>
> >> Changes in v2:
> >> - Abstract DMA handling from child driver: move it to MFD core
> >> - Add comments on optional dma support
> >> ---
> >>  drivers/mfd/stm32-timers.c       | 227 ++++++++++++++++++++++++++++++++++++++-
> >>  include/linux/mfd/stm32-timers.h |  32 ++++++
> >>  2 files changed, 257 insertions(+), 2 deletions(-)
> > 
> > [...]
> > 
> >> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> >> index 2aadab6..a04d7a1 100644
> >> --- a/include/linux/mfd/stm32-timers.h
> >> +++ b/include/linux/mfd/stm32-timers.h
> >> @@ -8,6 +8,8 @@
> >>  #define _LINUX_STM32_GPTIMER_H_
> >>  
> >>  #include <linux/clk.h>
> >> +#include <linux/dmaengine.h>
> >> +#include <linux/dma-mapping.h>
> >>  #include <linux/regmap.h>
> > 
> > [...]
> > 
> >> +struct stm32_timers_dma;
> >> +
> >>  struct stm32_timers {
> >>  	struct clk *clk;
> >>  	struct regmap *regmap;
> >>  	u32 max_arr;
> >> +	struct stm32_timers_dma *dma; /* Only to be used by the parent */
> > 
> > I'm confused.  I thought the point of putting this comment in was so
> > that you could place the definition of 'stm32_timers_dma' and remove
> > the forward declaration?
> 
> Hi Lee,
> 
> Sorry, if I miss-understood the point then. So, do you wish I both:
> - move the full struct definition in above header ?
> - and keep this comment ?

That was what I thought we agreed.

However, I left the final decision to you.  If you do not think this
is a reasonable i.e. the comment alone will not be enough to prevent
people from abusing the API, then leave it as it is.

Bear in mind that I think this introduces a build dependency on the
MFD driver for *each and every* other source file which includes this
header.  If you choose the current solution, you will need to handle
that accordingly.

> +/**
> + * struct stm32_timers_dma - STM32 timer DMA handling.
> + * @completion:		end of DMA transfer completion
> + * @phys_base:		control registers physical base address
> + * @lock:		protect DMA access
> + * @chan:		DMA channel in use
> + * @chans:		DMA channels available for this timer instance
> + */
> +struct stm32_timers_dma {
> +	struct completion completion;
> +	phys_addr_t phys_base;
> +	struct mutex lock;
> +	struct dma_chan *chan;
> +	struct dma_chan *chans[STM32_TIMERS_MAX_DMAS];
> +};
> 
> This will basically expose the struct to child drivers. But I'm ok if
> you think this is acceptable.
> 
> I can send a V5 if you wish...
> 
> Please advise,
> Best regards,
> Fabrice
> 
> > 
> >>  };
> >> +
> >> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
> >> +				enum stm32_timers_dmas id, u32 reg,
> >> +				unsigned int num_reg, unsigned int bursts,
> >> +				unsigned long tmo_ms);
> >>  #endif
> >
Dan Carpenter April 17, 2018, 11:25 a.m. UTC | #4 
Hi Fabrice,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pwm/for-next]
[also build test WARNING on v4.17-rc1 next-20180416]
[cannot apply to ljones-mfd/for-mfd-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fabrice-Gasnier/Add-support-for-PWM-input-capture-on-STM32/20180417-052347
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next

smatch warnings:
drivers/mfd/stm32-timers.c:165 stm32_timers_dma_burst_read() warn: warn: dma_mapping_error() doesn't return an error code

# https://github.com/0day-ci/linux/commit/402362a100e6b02b807fbebdc05b7159b565ffa5
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 402362a100e6b02b807fbebdc05b7159b565ffa5
vim +165 drivers/mfd/stm32-timers.c

402362a1 Fabrice Gasnier 2018-04-16   54  
402362a1 Fabrice Gasnier 2018-04-16   55  /**
402362a1 Fabrice Gasnier 2018-04-16   56   * stm32_timers_dma_burst_read - Read from timers registers using DMA.
402362a1 Fabrice Gasnier 2018-04-16   57   *
402362a1 Fabrice Gasnier 2018-04-16   58   * Read from STM32 timers registers using DMA on a single event.
402362a1 Fabrice Gasnier 2018-04-16   59   * @dev: reference to stm32_timers MFD device
402362a1 Fabrice Gasnier 2018-04-16   60   * @buf: DMA'able destination buffer
402362a1 Fabrice Gasnier 2018-04-16   61   * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
402362a1 Fabrice Gasnier 2018-04-16   62   * @reg: registers start offset for DMA to read from (like CCRx for capture)
402362a1 Fabrice Gasnier 2018-04-16   63   * @num_reg: number of registers to read upon each DMA request, starting @reg.
402362a1 Fabrice Gasnier 2018-04-16   64   * @bursts: number of bursts to read (e.g. like two for pwm period capture)
402362a1 Fabrice Gasnier 2018-04-16   65   * @tmo_ms: timeout (milliseconds)
402362a1 Fabrice Gasnier 2018-04-16   66   */
402362a1 Fabrice Gasnier 2018-04-16   67  int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
402362a1 Fabrice Gasnier 2018-04-16   68  				enum stm32_timers_dmas id, u32 reg,
402362a1 Fabrice Gasnier 2018-04-16   69  				unsigned int num_reg, unsigned int bursts,
402362a1 Fabrice Gasnier 2018-04-16   70  				unsigned long tmo_ms)
402362a1 Fabrice Gasnier 2018-04-16   71  {
402362a1 Fabrice Gasnier 2018-04-16   72  	struct stm32_timers *ddata = dev_get_drvdata(dev);
402362a1 Fabrice Gasnier 2018-04-16   73  	unsigned long timeout = msecs_to_jiffies(tmo_ms);
402362a1 Fabrice Gasnier 2018-04-16   74  	struct regmap *regmap = ddata->regmap;
402362a1 Fabrice Gasnier 2018-04-16   75  	struct stm32_timers_dma *dma = ddata->dma;
402362a1 Fabrice Gasnier 2018-04-16   76  	size_t len = num_reg * bursts * sizeof(u32);
402362a1 Fabrice Gasnier 2018-04-16   77  	struct dma_async_tx_descriptor *desc;
402362a1 Fabrice Gasnier 2018-04-16   78  	struct dma_slave_config config;
402362a1 Fabrice Gasnier 2018-04-16   79  	dma_cookie_t cookie;
402362a1 Fabrice Gasnier 2018-04-16   80  	dma_addr_t dma_buf;
402362a1 Fabrice Gasnier 2018-04-16   81  	u32 dbl, dba;
402362a1 Fabrice Gasnier 2018-04-16   82  	long err;
402362a1 Fabrice Gasnier 2018-04-16   83  	int ret;
402362a1 Fabrice Gasnier 2018-04-16   84  
402362a1 Fabrice Gasnier 2018-04-16   85  	/* Sanity check */
402362a1 Fabrice Gasnier 2018-04-16   86  	if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
402362a1 Fabrice Gasnier 2018-04-16   87  		return -EINVAL;
402362a1 Fabrice Gasnier 2018-04-16   88  
402362a1 Fabrice Gasnier 2018-04-16   89  	if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
402362a1 Fabrice Gasnier 2018-04-16   90  	    (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
402362a1 Fabrice Gasnier 2018-04-16   91  		return -EINVAL;
402362a1 Fabrice Gasnier 2018-04-16   92  
402362a1 Fabrice Gasnier 2018-04-16   93  	if (!dma->chans[id])
402362a1 Fabrice Gasnier 2018-04-16   94  		return -ENODEV;
402362a1 Fabrice Gasnier 2018-04-16   95  	mutex_lock(&dma->lock);
402362a1 Fabrice Gasnier 2018-04-16   96  
402362a1 Fabrice Gasnier 2018-04-16   97  	/* Select DMA channel in use */
402362a1 Fabrice Gasnier 2018-04-16   98  	dma->chan = dma->chans[id];
402362a1 Fabrice Gasnier 2018-04-16   99  	dma_buf = dma_map_single(dev, buf, len, DMA_FROM_DEVICE);
402362a1 Fabrice Gasnier 2018-04-16  100  	ret = dma_mapping_error(dev, dma_buf);
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be:

	if (dma_mapping_error(dev, dma_buf)) {
		ret = -ENOMEM;
		goto unlock;
	}

402362a1 Fabrice Gasnier 2018-04-16  101  	if (ret)
402362a1 Fabrice Gasnier 2018-04-16  102  		goto unlock;
402362a1 Fabrice Gasnier 2018-04-16  103  
402362a1 Fabrice Gasnier 2018-04-16  104  	/* Prepare DMA read from timer registers, using DMA burst mode */
402362a1 Fabrice Gasnier 2018-04-16  105  	memset(&config, 0, sizeof(config));
402362a1 Fabrice Gasnier 2018-04-16  106  	config.src_addr = (dma_addr_t)dma->phys_base + TIM_DMAR;
402362a1 Fabrice Gasnier 2018-04-16  107  	config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
402362a1 Fabrice Gasnier 2018-04-16  108  	ret = dmaengine_slave_config(dma->chan, &config);
402362a1 Fabrice Gasnier 2018-04-16  109  	if (ret)
402362a1 Fabrice Gasnier 2018-04-16  110  		goto unmap;
402362a1 Fabrice Gasnier 2018-04-16  111  
402362a1 Fabrice Gasnier 2018-04-16  112  	desc = dmaengine_prep_slave_single(dma->chan, dma_buf, len,
402362a1 Fabrice Gasnier 2018-04-16  113  					   DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
402362a1 Fabrice Gasnier 2018-04-16  114  	if (!desc) {
402362a1 Fabrice Gasnier 2018-04-16  115  		ret = -EBUSY;
402362a1 Fabrice Gasnier 2018-04-16  116  		goto unmap;
402362a1 Fabrice Gasnier 2018-04-16  117  	}
402362a1 Fabrice Gasnier 2018-04-16  118  
402362a1 Fabrice Gasnier 2018-04-16  119  	desc->callback = stm32_timers_dma_done;
402362a1 Fabrice Gasnier 2018-04-16  120  	desc->callback_param = dma;
402362a1 Fabrice Gasnier 2018-04-16  121  	cookie = dmaengine_submit(desc);
402362a1 Fabrice Gasnier 2018-04-16  122  	ret = dma_submit_error(cookie);
402362a1 Fabrice Gasnier 2018-04-16  123  	if (ret)
402362a1 Fabrice Gasnier 2018-04-16  124  		goto dma_term;
402362a1 Fabrice Gasnier 2018-04-16  125  
402362a1 Fabrice Gasnier 2018-04-16  126  	reinit_completion(&dma->completion);
402362a1 Fabrice Gasnier 2018-04-16  127  	dma_async_issue_pending(dma->chan);
402362a1 Fabrice Gasnier 2018-04-16  128  
402362a1 Fabrice Gasnier 2018-04-16  129  	/* Setup and enable timer DMA burst mode */
402362a1 Fabrice Gasnier 2018-04-16  130  	dbl = FIELD_PREP(TIM_DCR_DBL, bursts - 1);
402362a1 Fabrice Gasnier 2018-04-16  131  	dba = FIELD_PREP(TIM_DCR_DBA, reg >> 2);
402362a1 Fabrice Gasnier 2018-04-16  132  	ret = regmap_write(regmap, TIM_DCR, dbl | dba);
402362a1 Fabrice Gasnier 2018-04-16  133  	if (ret)
402362a1 Fabrice Gasnier 2018-04-16  134  		goto dma_term;
402362a1 Fabrice Gasnier 2018-04-16  135  
402362a1 Fabrice Gasnier 2018-04-16  136  	/* Clear pending flags before enabling DMA request */
402362a1 Fabrice Gasnier 2018-04-16  137  	ret = regmap_write(regmap, TIM_SR, 0);
402362a1 Fabrice Gasnier 2018-04-16  138  	if (ret)
402362a1 Fabrice Gasnier 2018-04-16  139  		goto dcr_clr;
402362a1 Fabrice Gasnier 2018-04-16  140  
402362a1 Fabrice Gasnier 2018-04-16  141  	ret = regmap_update_bits(regmap, TIM_DIER, stm32_timers_dier_dmaen[id],
402362a1 Fabrice Gasnier 2018-04-16  142  				 stm32_timers_dier_dmaen[id]);
402362a1 Fabrice Gasnier 2018-04-16  143  	if (ret)
402362a1 Fabrice Gasnier 2018-04-16  144  		goto dcr_clr;
402362a1 Fabrice Gasnier 2018-04-16  145  
402362a1 Fabrice Gasnier 2018-04-16  146  	err = wait_for_completion_interruptible_timeout(&dma->completion,
402362a1 Fabrice Gasnier 2018-04-16  147  							timeout);
402362a1 Fabrice Gasnier 2018-04-16  148  	if (err == 0)
402362a1 Fabrice Gasnier 2018-04-16  149  		ret = -ETIMEDOUT;
402362a1 Fabrice Gasnier 2018-04-16  150  	else if (err < 0)
402362a1 Fabrice Gasnier 2018-04-16  151  		ret = err;
402362a1 Fabrice Gasnier 2018-04-16  152  
402362a1 Fabrice Gasnier 2018-04-16  153  	regmap_update_bits(regmap, TIM_DIER, stm32_timers_dier_dmaen[id], 0);
402362a1 Fabrice Gasnier 2018-04-16  154  	regmap_write(regmap, TIM_SR, 0);
402362a1 Fabrice Gasnier 2018-04-16  155  dcr_clr:
402362a1 Fabrice Gasnier 2018-04-16  156  	regmap_write(regmap, TIM_DCR, 0);
402362a1 Fabrice Gasnier 2018-04-16  157  dma_term:
402362a1 Fabrice Gasnier 2018-04-16  158  	dmaengine_terminate_all(dma->chan);
402362a1 Fabrice Gasnier 2018-04-16  159  unmap:
402362a1 Fabrice Gasnier 2018-04-16  160  	dma_unmap_single(dev, dma_buf, len, DMA_FROM_DEVICE);
402362a1 Fabrice Gasnier 2018-04-16  161  unlock:
402362a1 Fabrice Gasnier 2018-04-16  162  	dma->chan = NULL;
402362a1 Fabrice Gasnier 2018-04-16  163  	mutex_unlock(&dma->lock);
402362a1 Fabrice Gasnier 2018-04-16  164  
402362a1 Fabrice Gasnier 2018-04-16 @165  	return ret;
402362a1 Fabrice Gasnier 2018-04-16  166  }
402362a1 Fabrice Gasnier 2018-04-16  167  EXPORT_SYMBOL_GPL(stm32_timers_dma_burst_read);
402362a1 Fabrice Gasnier 2018-04-16  168  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Fabrice Gasnier April 17, 2018, 11:54 a.m. UTC | #5 
On 04/17/2018 12:10 PM, Lee Jones wrote:
> On Tue, 17 Apr 2018, Fabrice Gasnier wrote:
> 
>> On 04/17/2018 09:12 AM, Lee Jones wrote:
>>> On Mon, 16 Apr 2018, Fabrice Gasnier wrote:
>>>
>>>> STM32 Timers can support up to 7 DMA requests:
>>>> - 4 channels, update, compare and trigger.
>>>> Optionally request part, or all DMAs from stm32-timers MFD core.
>>>>
>>>> Also add routine to implement burst reads using DMA from timer registers.
>>>> This is exported. So, it can be used by child drivers, PWM capture
>>>> for instance (but not limited to).
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>>> ---
>>>> Changes in v4:
>>>> - Lee's comments: Add kerneldoc header, better format comments.
>>>> Changes in v3:
>>>> - Basically Lee's comments:
>>>> - rather create a struct stm32_timers_dma, and place a reference to it
>>>>   in existing ddata (instead of adding priv struct).
>>>> - rather use a struct device in exported routine prototype, and use
>>>>   standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
>>>> - simplify error handling in probe (remove a goto)
>>>> - comment on devm_of_platform_*populate() usage.
>>>>
>>>> Changes in v2:
>>>> - Abstract DMA handling from child driver: move it to MFD core
>>>> - Add comments on optional dma support
>>>> ---
>>>>  drivers/mfd/stm32-timers.c       | 227 ++++++++++++++++++++++++++++++++++++++-
>>>>  include/linux/mfd/stm32-timers.h |  32 ++++++
>>>>  2 files changed, 257 insertions(+), 2 deletions(-)
>>>
>>> [...]
>>>
>>>> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
>>>> index 2aadab6..a04d7a1 100644
>>>> --- a/include/linux/mfd/stm32-timers.h
>>>> +++ b/include/linux/mfd/stm32-timers.h
>>>> @@ -8,6 +8,8 @@
>>>>  #define _LINUX_STM32_GPTIMER_H_
>>>>  
>>>>  #include <linux/clk.h>
>>>> +#include <linux/dmaengine.h>
>>>> +#include <linux/dma-mapping.h>
>>>>  #include <linux/regmap.h>
>>>
>>> [...]
>>>
>>>> +struct stm32_timers_dma;
>>>> +
>>>>  struct stm32_timers {
>>>>  	struct clk *clk;
>>>>  	struct regmap *regmap;
>>>>  	u32 max_arr;
>>>> +	struct stm32_timers_dma *dma; /* Only to be used by the parent */
>>>
>>> I'm confused.  I thought the point of putting this comment in was so
>>> that you could place the definition of 'stm32_timers_dma' and remove
>>> the forward declaration?
>>
>> Hi Lee,
>>
>> Sorry, if I miss-understood the point then. So, do you wish I both:
>> - move the full struct definition in above header ?
>> - and keep this comment ?
> 
> That was what I thought we agreed.

Hi Lee,

Ok, I'll update this in v5.
BTW, I'll fix warning reported by Dan:
> smatch warnings:
drivers/mfd/stm32-timers.c:165 stm32_timers_dma_burst_read() warn: warn:
dma_mapping_error() doesn't return an error code

Thanks,
Fabrice

> However, I left the final decision to you.  If you do not think this
> is a reasonable i.e. the comment alone will not be enough to prevent
> people from abusing the API, then leave it as it is.
> 
> Bear in mind that I think this introduces a build dependency on the
> MFD driver for *each and every* other source file which includes this
> header.  If you choose the current solution, you will need to handle
> that accordingly.
> 
>> +/**
>> + * struct stm32_timers_dma - STM32 timer DMA handling.
>> + * @completion:		end of DMA transfer completion
>> + * @phys_base:		control registers physical base address
>> + * @lock:		protect DMA access
>> + * @chan:		DMA channel in use
>> + * @chans:		DMA channels available for this timer instance
>> + */
>> +struct stm32_timers_dma {
>> +	struct completion completion;
>> +	phys_addr_t phys_base;
>> +	struct mutex lock;
>> +	struct dma_chan *chan;
>> +	struct dma_chan *chans[STM32_TIMERS_MAX_DMAS];
>> +};
>>
>> This will basically expose the struct to child drivers. But I'm ok if
>> you think this is acceptable.
>>
>> I can send a V5 if you wish...
>>
>> Please advise,
>> Best regards,
>> Fabrice
>>
>>>
>>>>  };
>>>> +
>>>> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
>>>> +				enum stm32_timers_dmas id, u32 reg,
>>>> +				unsigned int num_reg, unsigned int bursts,
>>>> +				unsigned long tmo_ms);
>>>>  #endif
>>>
>
diff mbox

Patch

diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
index 1d347e5..497d707 100644
--- a/drivers/mfd/stm32-timers.c
+++ b/drivers/mfd/stm32-timers.c
@@ -4,16 +4,173 @@ 
  * Author: Benjamin Gaignard <benjamin.gaignard@st.com>
  */
 
+#include <linux/bitfield.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/mfd/stm32-timers.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/reset.h>
 
+#define STM32_TIMERS_MAX_REGISTERS	0x3fc
+
+/**
+ * struct stm32_timers_dma - STM32 timer DMA handling.
+ * @completion:		end of DMA transfer completion
+ * @phys_base:		control registers physical base address
+ * @lock:		protect DMA access
+ * @chan:		DMA channel in use
+ * @chans:		DMA channels available for this timer instance
+ */
+struct stm32_timers_dma {
+	struct completion completion;
+	phys_addr_t phys_base;
+	struct mutex lock;
+	struct dma_chan *chan;
+	struct dma_chan *chans[STM32_TIMERS_MAX_DMAS];
+};
+
+/* DIER register DMA enable bits */
+static const u32 stm32_timers_dier_dmaen[STM32_TIMERS_MAX_DMAS] = {
+	TIM_DIER_CC1DE,
+	TIM_DIER_CC2DE,
+	TIM_DIER_CC3DE,
+	TIM_DIER_CC4DE,
+	TIM_DIER_UIE,
+	TIM_DIER_TDE,
+	TIM_DIER_COMDE
+};
+
+static void stm32_timers_dma_done(void *p)
+{
+	struct stm32_timers_dma *dma = p;
+	struct dma_tx_state state;
+	enum dma_status status;
+
+	status = dmaengine_tx_status(dma->chan, dma->chan->cookie, &state);
+	if (status == DMA_COMPLETE)
+		complete(&dma->completion);
+}
+
+/**
+ * stm32_timers_dma_burst_read - Read from timers registers using DMA.
+ *
+ * Read from STM32 timers registers using DMA on a single event.
+ * @dev: reference to stm32_timers MFD device
+ * @buf: DMA'able destination buffer
+ * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
+ * @reg: registers start offset for DMA to read from (like CCRx for capture)
+ * @num_reg: number of registers to read upon each DMA request, starting @reg.
+ * @bursts: number of bursts to read (e.g. like two for pwm period capture)
+ * @tmo_ms: timeout (milliseconds)
+ */
+int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
+				enum stm32_timers_dmas id, u32 reg,
+				unsigned int num_reg, unsigned int bursts,
+				unsigned long tmo_ms)
+{
+	struct stm32_timers *ddata = dev_get_drvdata(dev);
+	unsigned long timeout = msecs_to_jiffies(tmo_ms);
+	struct regmap *regmap = ddata->regmap;
+	struct stm32_timers_dma *dma = ddata->dma;
+	size_t len = num_reg * bursts * sizeof(u32);
+	struct dma_async_tx_descriptor *desc;
+	struct dma_slave_config config;
+	dma_cookie_t cookie;
+	dma_addr_t dma_buf;
+	u32 dbl, dba;
+	long err;
+	int ret;
+
+	/* Sanity check */
+	if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
+		return -EINVAL;
+
+	if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
+	    (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
+		return -EINVAL;
+
+	if (!dma->chans[id])
+		return -ENODEV;
+	mutex_lock(&dma->lock);
+
+	/* Select DMA channel in use */
+	dma->chan = dma->chans[id];
+	dma_buf = dma_map_single(dev, buf, len, DMA_FROM_DEVICE);
+	ret = dma_mapping_error(dev, dma_buf);
+	if (ret)
+		goto unlock;
+
+	/* Prepare DMA read from timer registers, using DMA burst mode */
+	memset(&config, 0, sizeof(config));
+	config.src_addr = (dma_addr_t)dma->phys_base + TIM_DMAR;
+	config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	ret = dmaengine_slave_config(dma->chan, &config);
+	if (ret)
+		goto unmap;
+
+	desc = dmaengine_prep_slave_single(dma->chan, dma_buf, len,
+					   DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+	if (!desc) {
+		ret = -EBUSY;
+		goto unmap;
+	}
+
+	desc->callback = stm32_timers_dma_done;
+	desc->callback_param = dma;
+	cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(cookie);
+	if (ret)
+		goto dma_term;
+
+	reinit_completion(&dma->completion);
+	dma_async_issue_pending(dma->chan);
+
+	/* Setup and enable timer DMA burst mode */
+	dbl = FIELD_PREP(TIM_DCR_DBL, bursts - 1);
+	dba = FIELD_PREP(TIM_DCR_DBA, reg >> 2);
+	ret = regmap_write(regmap, TIM_DCR, dbl | dba);
+	if (ret)
+		goto dma_term;
+
+	/* Clear pending flags before enabling DMA request */
+	ret = regmap_write(regmap, TIM_SR, 0);
+	if (ret)
+		goto dcr_clr;
+
+	ret = regmap_update_bits(regmap, TIM_DIER, stm32_timers_dier_dmaen[id],
+				 stm32_timers_dier_dmaen[id]);
+	if (ret)
+		goto dcr_clr;
+
+	err = wait_for_completion_interruptible_timeout(&dma->completion,
+							timeout);
+	if (err == 0)
+		ret = -ETIMEDOUT;
+	else if (err < 0)
+		ret = err;
+
+	regmap_update_bits(regmap, TIM_DIER, stm32_timers_dier_dmaen[id], 0);
+	regmap_write(regmap, TIM_SR, 0);
+dcr_clr:
+	regmap_write(regmap, TIM_DCR, 0);
+dma_term:
+	dmaengine_terminate_all(dma->chan);
+unmap:
+	dma_unmap_single(dev, dma_buf, len, DMA_FROM_DEVICE);
+unlock:
+	dma->chan = NULL;
+	mutex_unlock(&dma->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(stm32_timers_dma_burst_read);
+
 static const struct regmap_config stm32_timers_regmap_cfg = {
 	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = sizeof(u32),
-	.max_register = 0x3fc,
+	.max_register = STM32_TIMERS_MAX_REGISTERS,
 };
 
 static void stm32_timers_get_arr_size(struct stm32_timers *ddata)
@@ -27,12 +184,53 @@  static void stm32_timers_get_arr_size(struct stm32_timers *ddata)
 	regmap_write(ddata->regmap, TIM_ARR, 0x0);
 }
 
+static int stm32_timers_dma_probe(struct device *dev,
+				  struct stm32_timers *ddata)
+{
+	int i;
+	char name[4];
+	struct stm32_timers_dma *dma;
+
+	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
+	if (!dma)
+		return -ENOMEM;
+
+	init_completion(&dma->completion);
+	mutex_init(&dma->lock);
+
+	/* Optional DMA support: get valid DMA channel(s) or NULL */
+	for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
+		snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
+		dma->chans[i] = dma_request_slave_channel(dev, name);
+	}
+	dma->chans[STM32_TIMERS_DMA_UP] =
+		dma_request_slave_channel(dev, "up");
+	dma->chans[STM32_TIMERS_DMA_TRIG] =
+		dma_request_slave_channel(dev, "trig");
+	dma->chans[STM32_TIMERS_DMA_COM] =
+		dma_request_slave_channel(dev, "com");
+	ddata->dma = dma;
+
+	return 0;
+}
+
+static void stm32_timers_dma_remove(struct device *dev,
+				    struct stm32_timers *ddata)
+{
+	int i;
+
+	for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++)
+		if (ddata->dma->chans[i])
+			dma_release_channel(ddata->dma->chans[i]);
+}
+
 static int stm32_timers_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct stm32_timers *ddata;
 	struct resource *res;
 	void __iomem *mmio;
+	int ret;
 
 	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
 	if (!ddata)
@@ -54,9 +252,33 @@  static int stm32_timers_probe(struct platform_device *pdev)
 
 	stm32_timers_get_arr_size(ddata);
 
+	ret = stm32_timers_dma_probe(dev, ddata);
+	if (ret)
+		return ret;
+	/* Timer physical addr for DMA */
+	ddata->dma->phys_base = res->start;
+
 	platform_set_drvdata(pdev, ddata);
 
-	return devm_of_platform_populate(&pdev->dev);
+	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+	if (ret)
+		stm32_timers_dma_remove(dev, ddata);
+
+	return ret;
+}
+
+static int stm32_timers_remove(struct platform_device *pdev)
+{
+	struct stm32_timers *ddata = platform_get_drvdata(pdev);
+
+	/*
+	 * Don't use devm_ here: enfore of_platform_depopulate() happens before
+	 * DMA are released, to avoid race on DMA.
+	 */
+	of_platform_depopulate(&pdev->dev);
+	stm32_timers_dma_remove(&pdev->dev, ddata);
+
+	return 0;
 }
 
 static const struct of_device_id stm32_timers_of_match[] = {
@@ -67,6 +289,7 @@  static int stm32_timers_probe(struct platform_device *pdev)
 
 static struct platform_driver stm32_timers_driver = {
 	.probe = stm32_timers_probe,
+	.remove = stm32_timers_remove,
 	.driver	= {
 		.name = "stm32-timers",
 		.of_match_table = stm32_timers_of_match,
diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index 2aadab6..a04d7a1 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -8,6 +8,8 @@ 
 #define _LINUX_STM32_GPTIMER_H_
 
 #include <linux/clk.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/regmap.h>
 
 #define TIM_CR1		0x00	/* Control Register 1      */
@@ -27,6 +29,8 @@ 
 #define TIM_CCR3	0x3C	/* Capt/Comp Register 3    */
 #define TIM_CCR4	0x40	/* Capt/Comp Register 4    */
 #define TIM_BDTR	0x44	/* Break and Dead-Time Reg */
+#define TIM_DCR		0x48	/* DMA control register    */
+#define TIM_DMAR	0x4C	/* DMA register for transfer */
 
 #define TIM_CR1_CEN	BIT(0)	/* Counter Enable	   */
 #define TIM_CR1_DIR	BIT(4)  /* Counter Direction	   */
@@ -36,6 +40,13 @@ 
 #define TIM_SMCR_SMS	(BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */
 #define TIM_SMCR_TS	(BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */
 #define TIM_DIER_UIE	BIT(0)	/* Update interrupt	   */
+#define TIM_DIER_UDE	BIT(8)  /* Update DMA request Enable */
+#define TIM_DIER_CC1DE	BIT(9)  /* CC1 DMA request Enable  */
+#define TIM_DIER_CC2DE	BIT(10) /* CC2 DMA request Enable  */
+#define TIM_DIER_CC3DE	BIT(11) /* CC3 DMA request Enable  */
+#define TIM_DIER_CC4DE	BIT(12) /* CC4 DMA request Enable  */
+#define TIM_DIER_COMDE	BIT(13) /* COM DMA request Enable  */
+#define TIM_DIER_TDE	BIT(14) /* Trigger DMA request Enable */
 #define TIM_SR_UIF	BIT(0)	/* Update interrupt flag   */
 #define TIM_EGR_UG	BIT(0)	/* Update Generation       */
 #define TIM_CCMR_PE	BIT(3)	/* Channel Preload Enable  */
@@ -56,6 +67,8 @@ 
 #define TIM_BDTR_BK2F	(BIT(20) | BIT(21) | BIT(22) | BIT(23))
 #define TIM_BDTR_BK2E	BIT(24) /* Break 2 input enable	   */
 #define TIM_BDTR_BK2P	BIT(25) /* Break 2 input polarity  */
+#define TIM_DCR_DBA	GENMASK(4, 0)	/* DMA base addr */
+#define TIM_DCR_DBL	GENMASK(12, 8)	/* DMA burst len */
 
 #define MAX_TIM_PSC		0xFFFF
 #define TIM_CR2_MMS_SHIFT	4
@@ -65,9 +78,28 @@ 
 #define TIM_BDTR_BKF_SHIFT	16
 #define TIM_BDTR_BK2F_SHIFT	20
 
+enum stm32_timers_dmas {
+	STM32_TIMERS_DMA_CH1,
+	STM32_TIMERS_DMA_CH2,
+	STM32_TIMERS_DMA_CH3,
+	STM32_TIMERS_DMA_CH4,
+	STM32_TIMERS_DMA_UP,
+	STM32_TIMERS_DMA_TRIG,
+	STM32_TIMERS_DMA_COM,
+	STM32_TIMERS_MAX_DMAS,
+};
+
+struct stm32_timers_dma;
+
 struct stm32_timers {
 	struct clk *clk;
 	struct regmap *regmap;
 	u32 max_arr;
+	struct stm32_timers_dma *dma; /* Only to be used by the parent */
 };
+
+int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
+				enum stm32_timers_dmas id, u32 reg,
+				unsigned int num_reg, unsigned int bursts,
+				unsigned long tmo_ms);
 #endif