| Message ID | 1523895561-4073-3-git-send-email-fabrice.gasnier@st.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for PWM input capture on STM32 | expand |
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
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > >
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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