| Message ID | 1522404084-24903-1-git-send-email-fabrice.gasnier@st.com |
|---|---|
| Headers | show |
| Series | Add support for PWM input capture on STM32 | expand |
On Fri, 30 Mar 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 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 | 219 ++++++++++++++++++++++++++++++++++++++- > include/linux/mfd/stm32-timers.h | 32 ++++++ > 2 files changed, 249 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c > index 1d347e5..98191ec 100644 > --- a/drivers/mfd/stm32-timers.c > +++ b/drivers/mfd/stm32-timers.c > @@ -4,16 +4,165 @@ > * 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 { > + struct completion completion; > + phys_addr_t phys_base; > + struct mutex lock; /* protect dma access */ Nit: I like comments to use good grammar i.e. capital letters to start a sentence and 's/dma/DMA/'. Or better still, drop the comment, we know what the lock is for. > + struct dma_chan *chan; > + struct dma_chan *chans[STM32_TIMERS_MAX_DMAS]; This requires explanation. Maybe a kerneldoc header would be good here. [...] > +/** > + * 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 */ Proper grammar in all comments please. "Sanity check" [...] > + /* select dma channel in use */ Here too. Etc, etc, etc ... > + 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 */ This is good. [...] [...] > diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h > index 2aadab6..585a4de 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> [...] > +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; Why don't you move the declaration into here? Then you don't need to forward declare. > struct stm32_timers { > struct clk *clk; > struct regmap *regmap; > u32 max_arr; > + struct stm32_timers_dma *dma; > }; > + > +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 Fri, 30 Mar 2018, Fabrice Gasnier wrote: > Add support for PMW input mode on pwm-stm32. STM32 timers support > period and duty cycle capture as long as they have at least two PWM > channels. One capture channel is used for period (rising-edge), one > for duty-cycle (falling-edge). > When there's only one channel available, only period can be captured. > Duty-cycle is simply zero'ed in such a case. > > Capture requires exclusive access (e.g. no pwm output running at the > same time, to protect common prescaler). > Timer DMA burst mode (from MFD core) is being used, to take two > snapshots of capture registers (upon each period rising edge). > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Acked-by: Thierry Reding <thierry.reding@gmail.com> > --- > Changes in v3: > - update stm32_timers_dma_burst_read() call: don't pass ddata structure, > use MFD parent device structure instead since MFD core update. > > Changes in v2: > - DMA handling has been moved to MFD core. Rework capture routines to > use it. > --- > drivers/pwm/pwm-stm32.c | 176 +++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/stm32-timers.h | 11 +++ For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> > 2 files changed, 187 insertions(+)
On Fri, 30 Mar 2018, Fabrice Gasnier wrote: > Using input prescaler, capture unit will trigger DMA once every > configurable /2, /4 or /8 events (rising edge). This helps improve > period (only) capture accuracy at high rates. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Acked-by: Thierry Reding <thierry.reding@gmail.com> > --- > Changes in v2: > - Adopt DMA read from MFD core. > --- > drivers/pwm/pwm-stm32.c | 63 ++++++++++++++++++++++++++++++++++++++-- > include/linux/mfd/stm32-timers.h | 1 + For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> > 2 files changed, 62 insertions(+), 2 deletions(-)
On 04/16/2018 02:22 PM, Lee Jones wrote: > On Fri, 30 Mar 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 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 | 219 ++++++++++++++++++++++++++++++++++++++- >> include/linux/mfd/stm32-timers.h | 32 ++++++ >> 2 files changed, 249 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >> index 1d347e5..98191ec 100644 >> --- a/drivers/mfd/stm32-timers.c >> +++ b/drivers/mfd/stm32-timers.c >> @@ -4,16 +4,165 @@ >> * 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 { >> + struct completion completion; >> + phys_addr_t phys_base; >> + struct mutex lock; /* protect dma access */ > > Nit: I like comments to use good grammar i.e. capital letters to > start a sentence and 's/dma/DMA/'. Or better still, drop the comment, > we know what the lock is for. > >> + struct dma_chan *chan; >> + struct dma_chan *chans[STM32_TIMERS_MAX_DMAS]; > > This requires explanation. Maybe a kerneldoc header would be good here. Hi Lee, I'll add kerneldoc in next version. > > [...] > >> +/** >> + * 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 */ > > Proper grammar in all comments please. > > "Sanity check" > > [...] > >> + /* select dma channel in use */ > > Here too. > > Etc, etc, etc ... Okay, will take care of this. > >> + 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 */ > > This is good. > [...] > > [...] > >> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h >> index 2aadab6..585a4de 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> > > [...] > >> +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; > > Why don't you move the declaration into here? To follow previous discussions we had in V1 and V2, this is to avoid sharing all the information with child drivers, e.g. passing physical address of parent MFD into child devices. I should probably add a comment there ? Something like: /* STM32 timers MFD parent internal struct to handle DMA transfers */ struct stm32_timers_dma; Do you agree with this ? Thanks for reviewing, BR, Fabrice > > Then you don't need to forward declare. > >> struct stm32_timers { >> struct clk *clk; >> struct regmap *regmap; >> u32 max_arr; >> + struct stm32_timers_dma *dma; >> }; >> + >> +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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 16 Apr 2018, Fabrice Gasnier wrote: > On 04/16/2018 02:22 PM, Lee Jones wrote: > > On Fri, 30 Mar 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 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 | 219 ++++++++++++++++++++++++++++++++++++++- > >> include/linux/mfd/stm32-timers.h | 32 ++++++ > >> 2 files changed, 249 insertions(+), 2 deletions(-) [...] > >> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h > >> index 2aadab6..585a4de 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> > > > > [...] > > > >> +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; > > > > Why don't you move the declaration into here? > > To follow previous discussions we had in V1 and V2, this is to avoid > sharing all the information with child drivers, e.g. passing physical > address of parent MFD into child devices. > > I should probably add a comment there ? Something like: > > /* STM32 timers MFD parent internal struct to handle DMA transfers */ > struct stm32_timers_dma; > > Do you agree with this ? > > Thanks for reviewing, > BR, > Fabrice > > > > > Then you don't need to forward declare. Yes, I remember our previous conversation. Perhaps you could always put a comment like: > >> struct stm32_timers { > >> struct clk *clk; > >> struct regmap *regmap; > >> u32 max_arr; > >> + struct stm32_timers_dma *dma; /* Only to be used by the parent */ > >> }; If this is not acceptable, then the current solution will do.
On 04/16/2018 04:47 PM, Lee Jones wrote: > On Mon, 16 Apr 2018, Fabrice Gasnier wrote: > >> On 04/16/2018 02:22 PM, Lee Jones wrote: >>> On Fri, 30 Mar 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 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 | 219 ++++++++++++++++++++++++++++++++++++++- >>>> include/linux/mfd/stm32-timers.h | 32 ++++++ >>>> 2 files changed, 249 insertions(+), 2 deletions(-) > > [...] > >>>> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h >>>> index 2aadab6..585a4de 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> >>> >>> [...] >>> >>>> +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; >>> >>> Why don't you move the declaration into here? >> >> To follow previous discussions we had in V1 and V2, this is to avoid >> sharing all the information with child drivers, e.g. passing physical >> address of parent MFD into child devices. >> >> I should probably add a comment there ? Something like: >> >> /* STM32 timers MFD parent internal struct to handle DMA transfers */ >> struct stm32_timers_dma; >> >> Do you agree with this ? >> >> Thanks for reviewing, >> BR, >> Fabrice >> >>> >>> Then you don't need to forward declare. > > Yes, I remember our previous conversation. > > Perhaps you could always put a comment like: > >>>> struct stm32_timers { >>>> struct clk *clk; >>>> struct regmap *regmap; >>>> u32 max_arr; >>>> + struct stm32_timers_dma *dma; /* Only to be used by the parent */ >>>> }; > > If this is not acceptable, then the current solution will do. Hi Lee, I'll update it with your proposal, Many thanks, Fabrice > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html