| Message ID | 1518602679-3064-5-git-send-email-fabrice.gasnier@st.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for PWM input capture on STM32 | expand |
On Wed, 14 Feb 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 v2: > - Abstract DMA handling from child driver: move it to MFD core > - Add comments on optional dma support > --- > drivers/mfd/stm32-timers.c | 215 ++++++++++++++++++++++++++++++++++++++- > include/linux/mfd/stm32-timers.h | 27 +++++ > 2 files changed, 238 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c > index a6675a4..2cdad2c 100644 > --- a/drivers/mfd/stm32-timers.c > +++ b/drivers/mfd/stm32-timers.c > @@ -6,16 +6,166 @@ > * License terms: GNU General Public License (GPL), version 2 > */ > > +#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_priv { > + struct device *dev; > + struct completion completion; > + phys_addr_t phys_base; /* timers physical addr for dma */ > + struct mutex lock; /* protect dma access */ > + struct dma_chan *dma_chan; /* dma channel in use */ I think kerneldoc would be better than inline comments. > + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; > + struct stm32_timers ddata; This looks odd to me. Why can't you expand the current ddata structure? Wouldn't it be better to create a stm32_timers_dma structure to place all this information in (except *dev, that should live in the ddata struct), then place a reference in the existing stm32_timers struct? > +}; > + > +static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d) > +{ > + return container_of(d, struct stm32_timers_priv, ddata); > +} If you take my other suggestions, you can remove this function. > +/* 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 > +}; Maybe one per line would be kinder on the eye? > +static void stm32_timers_dma_done(void *p) > +{ > + struct stm32_timers_priv *priv = p; > + struct dma_chan *dma_chan = priv->dma_chan; > + struct dma_tx_state state; > + enum dma_status status; > + > + status = dmaengine_tx_status(dma_chan, dma_chan->cookie, &state); > + if (status == DMA_COMPLETE) > + complete(&priv->completion); > +} > + > +/** > + * stm32_timers_dma_burst_read - Read from timers registers using DMA. > + * > + * Read from STM32 timers registers using DMA on a single event. > + * @ddata: reference to stm32_timers It's odd to pass device data back like this. Better to pass a pointer to 'struct device', then use the normal helpers. > + * @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 stm32_timers *ddata, u32 *buf, > + enum stm32_timers_dmas id, u32 reg, > + unsigned int num_reg, unsigned int bursts, > + unsigned long tmo_ms) > +{ > + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); > + unsigned long timeout = msecs_to_jiffies(tmo_ms); > + struct regmap *regmap = priv->ddata.regmap; > + 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 (!priv->dmas[id]) > + return -ENODEV; > + mutex_lock(&priv->lock); > + priv->dma_chan = priv->dmas[id]; > + > + dma_buf = dma_map_single(priv->dev, buf, len, DMA_FROM_DEVICE); > + ret = dma_mapping_error(priv->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)priv->phys_base + TIM_DMAR; > + config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + ret = dmaengine_slave_config(priv->dma_chan, &config); > + if (ret) > + goto unmap; > + > + desc = dmaengine_prep_slave_single(priv->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 = priv; > + cookie = dmaengine_submit(desc); > + ret = dma_submit_error(cookie); > + if (ret) > + goto dma_term; > + > + reinit_completion(&priv->completion); > + dma_async_issue_pending(priv->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(&priv->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(priv->dma_chan); > +unmap: > + dma_unmap_single(priv->dev, dma_buf, len, DMA_FROM_DEVICE); > +unlock: > + priv->dma_chan = NULL; > + mutex_unlock(&priv->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) > @@ -29,21 +179,55 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) > regmap_write(ddata->regmap, TIM_ARR, 0x0); > } > > +static void stm32_timers_dma_probe(struct device *dev, > + struct stm32_timers_priv *priv) > +{ > + int i; > + char name[4]; > + struct dma_chan **dmas = priv->dmas; > + > + /* 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); > + dmas[i] = dma_request_slave_channel(dev, name); > + } > + dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up"); > + dmas[STM32_TIMERS_DMA_TRIG] = dma_request_slave_channel(dev, "trig"); > + dmas[STM32_TIMERS_DMA_COM] = dma_request_slave_channel(dev, "com"); > +} > + > +static void stm32_timers_dma_remove(struct device *dev, > + struct stm32_timers_priv *priv) > +{ > + int i; > + > + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) > + if (priv->dmas[i]) > + dma_release_channel(priv->dmas[i]); > +} > + > static int stm32_timers_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct stm32_timers_priv *priv; > struct stm32_timers *ddata; > struct resource *res; > void __iomem *mmio; > + int ret; > > - ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); > - if (!ddata) > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > return -ENOMEM; > + ddata = &priv->ddata; > + init_completion(&priv->completion); > + mutex_init(&priv->lock); > + priv->dev = dev; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > mmio = devm_ioremap_resource(dev, res); > if (IS_ERR(mmio)) > return PTR_ERR(mmio); > + priv->phys_base = res->start; > > ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio, > &stm32_timers_regmap_cfg); > @@ -56,9 +240,31 @@ static int stm32_timers_probe(struct platform_device *pdev) > > stm32_timers_get_arr_size(ddata); > > + stm32_timers_dma_probe(dev, priv); > + > 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) > + goto dma_remove; > + > + return 0; > + > +dma_remove: > + stm32_timers_dma_remove(dev, priv); You can easily remove this label and the goto. > + return ret; > +} > + > +static int stm32_timers_remove(struct platform_device *pdev) > +{ > + struct stm32_timers *ddata = platform_get_drvdata(pdev); > + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); > + > + of_platform_depopulate(&pdev->dev); Why can't you continue using devm_*? > + stm32_timers_dma_remove(&pdev->dev, priv); > + > + return 0; > } > > static const struct of_device_id stm32_timers_of_match[] = { > @@ -69,6 +275,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 ce7346e..5fd2d6b 100644 > --- a/include/linux/mfd/stm32-timers.h > +++ b/include/linux/mfd/stm32-timers.h > @@ -29,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 */ > @@ -38,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 */ > @@ -58,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 > @@ -67,9 +78,25 @@ > #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 { > struct clk *clk; > struct regmap *regmap; > u32 max_arr; > }; > + > +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf, > + enum stm32_timers_dmas id, u32 reg, > + unsigned int num_reg, unsigned int bursts, > + unsigned long tmo_ms); > #endif
On 03/28/2018 05:22 PM, Lee Jones wrote: > On Wed, 14 Feb 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 v2: >> - Abstract DMA handling from child driver: move it to MFD core >> - Add comments on optional dma support >> --- >> drivers/mfd/stm32-timers.c | 215 ++++++++++++++++++++++++++++++++++++++- >> include/linux/mfd/stm32-timers.h | 27 +++++ >> 2 files changed, 238 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >> index a6675a4..2cdad2c 100644 >> --- a/drivers/mfd/stm32-timers.c >> +++ b/drivers/mfd/stm32-timers.c >> @@ -6,16 +6,166 @@ >> * License terms: GNU General Public License (GPL), version 2 >> */ >> >> +#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_priv { >> + struct device *dev; >> + struct completion completion; >> + phys_addr_t phys_base; /* timers physical addr for dma */ >> + struct mutex lock; /* protect dma access */ >> + struct dma_chan *dma_chan; /* dma channel in use */ > > I think kerneldoc would be better than inline comments. Hi Lee, Okay, I can update it with kerneldoc instead. > >> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; >> + struct stm32_timers ddata; > > This looks odd to me. Why can't you expand the current ddata > structure? Wouldn't it be better to create a stm32_timers_dma > structure to place all this information in (except *dev, that should > live in the ddata struct), then place a reference in the existing > stm32_timers struct? Maybe I miss-understand you here, from what we discussed in V1: https://lkml.org/lkml/2018/1/23/574 >... "passing in the physical address of the parent MFD into > a child device doesn't quite sit right with me" I introduced this private struct in MFD parent, and completely hide it from the child. So, do you suggest to add struct definition here ? But make it part of struct stm32_timers *ddata? And only put declaration in include/linux/mfd/stm32-timers.h: + struct stm32_timers_dma; struct stm32_timers { struct clk *clk; struct regmap *regmap; u32 max_arr; + struct stm32_timers_dma; }; I can probably spare the *dev then... use dev->parent in child driver. Can you just confirm this please? > >> +}; >> + >> +static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d) >> +{ >> + return container_of(d, struct stm32_timers_priv, ddata); >> +} > > If you take my other suggestions, you can remove this function. > >> +/* 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 >> +}; > > Maybe one per line would be kinder on the eye? Ok, I'll update this. > >> +static void stm32_timers_dma_done(void *p) >> +{ >> + struct stm32_timers_priv *priv = p; >> + struct dma_chan *dma_chan = priv->dma_chan; >> + struct dma_tx_state state; >> + enum dma_status status; >> + >> + status = dmaengine_tx_status(dma_chan, dma_chan->cookie, &state); >> + if (status == DMA_COMPLETE) >> + complete(&priv->completion); >> +} >> + >> +/** >> + * stm32_timers_dma_burst_read - Read from timers registers using DMA. >> + * >> + * Read from STM32 timers registers using DMA on a single event. >> + * @ddata: reference to stm32_timers > > It's odd to pass device data back like this. > > Better to pass a pointer to 'struct device', then use the normal > helpers. You're right, I'll update this. > >> + * @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 stm32_timers *ddata, u32 *buf, >> + enum stm32_timers_dmas id, u32 reg, >> + unsigned int num_reg, unsigned int bursts, >> + unsigned long tmo_ms) >> +{ >> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); >> + unsigned long timeout = msecs_to_jiffies(tmo_ms); >> + struct regmap *regmap = priv->ddata.regmap; >> + 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 (!priv->dmas[id]) >> + return -ENODEV; >> + mutex_lock(&priv->lock); >> + priv->dma_chan = priv->dmas[id]; >> + >> + dma_buf = dma_map_single(priv->dev, buf, len, DMA_FROM_DEVICE); >> + ret = dma_mapping_error(priv->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)priv->phys_base + TIM_DMAR; >> + config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; >> + ret = dmaengine_slave_config(priv->dma_chan, &config); >> + if (ret) >> + goto unmap; >> + >> + desc = dmaengine_prep_slave_single(priv->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 = priv; >> + cookie = dmaengine_submit(desc); >> + ret = dma_submit_error(cookie); >> + if (ret) >> + goto dma_term; >> + >> + reinit_completion(&priv->completion); >> + dma_async_issue_pending(priv->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(&priv->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(priv->dma_chan); >> +unmap: >> + dma_unmap_single(priv->dev, dma_buf, len, DMA_FROM_DEVICE); >> +unlock: >> + priv->dma_chan = NULL; >> + mutex_unlock(&priv->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) >> @@ -29,21 +179,55 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) >> regmap_write(ddata->regmap, TIM_ARR, 0x0); >> } >> >> +static void stm32_timers_dma_probe(struct device *dev, >> + struct stm32_timers_priv *priv) >> +{ >> + int i; >> + char name[4]; >> + struct dma_chan **dmas = priv->dmas; >> + >> + /* 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); >> + dmas[i] = dma_request_slave_channel(dev, name); >> + } >> + dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up"); >> + dmas[STM32_TIMERS_DMA_TRIG] = dma_request_slave_channel(dev, "trig"); >> + dmas[STM32_TIMERS_DMA_COM] = dma_request_slave_channel(dev, "com"); >> +} >> + >> +static void stm32_timers_dma_remove(struct device *dev, >> + struct stm32_timers_priv *priv) >> +{ >> + int i; >> + >> + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) >> + if (priv->dmas[i]) >> + dma_release_channel(priv->dmas[i]); >> +} >> + >> static int stm32_timers_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> + struct stm32_timers_priv *priv; >> struct stm32_timers *ddata; >> struct resource *res; >> void __iomem *mmio; >> + int ret; >> >> - ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); >> - if (!ddata) >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> return -ENOMEM; >> + ddata = &priv->ddata; >> + init_completion(&priv->completion); >> + mutex_init(&priv->lock); >> + priv->dev = dev; >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> mmio = devm_ioremap_resource(dev, res); >> if (IS_ERR(mmio)) >> return PTR_ERR(mmio); >> + priv->phys_base = res->start; >> >> ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio, >> &stm32_timers_regmap_cfg); >> @@ -56,9 +240,31 @@ static int stm32_timers_probe(struct platform_device *pdev) >> >> stm32_timers_get_arr_size(ddata); >> >> + stm32_timers_dma_probe(dev, priv); >> + >> 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) >> + goto dma_remove; >> + >> + return 0; >> + >> +dma_remove: >> + stm32_timers_dma_remove(dev, priv); > > You can easily remove this label and the goto. I'll update this > >> + return ret; >> +} >> + >> +static int stm32_timers_remove(struct platform_device *pdev) >> +{ >> + struct stm32_timers *ddata = platform_get_drvdata(pdev); >> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); >> + >> + of_platform_depopulate(&pdev->dev); > > Why can't you continue using devm_*? I can use devm_of_platform_depopulate() here if you prefer, and keep devm_of_platform_populate() in probe. Please let me know Thanks for reviewing, Best Regards, Fabrice > >> + stm32_timers_dma_remove(&pdev->dev, priv); >> + >> + return 0; >> } >> >> static const struct of_device_id stm32_timers_of_match[] = { >> @@ -69,6 +275,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 ce7346e..5fd2d6b 100644 >> --- a/include/linux/mfd/stm32-timers.h >> +++ b/include/linux/mfd/stm32-timers.h >> @@ -29,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 */ >> @@ -38,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 */ >> @@ -58,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 >> @@ -67,9 +78,25 @@ >> #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 { >> struct clk *clk; >> struct regmap *regmap; >> u32 max_arr; >> }; >> + >> +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, 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 Wed, 28 Mar 2018, Fabrice Gasnier wrote: > On 03/28/2018 05:22 PM, Lee Jones wrote: > > On Wed, 14 Feb 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 v2: > >> - Abstract DMA handling from child driver: move it to MFD core > >> - Add comments on optional dma support > >> --- > >> drivers/mfd/stm32-timers.c | 215 ++++++++++++++++++++++++++++++++++++++- > >> include/linux/mfd/stm32-timers.h | 27 +++++ > >> 2 files changed, 238 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c > >> index a6675a4..2cdad2c 100644 > >> --- a/drivers/mfd/stm32-timers.c > >> +++ b/drivers/mfd/stm32-timers.c [...] > >> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; > >> + struct stm32_timers ddata; > > > > This looks odd to me. Why can't you expand the current ddata > > structure? Wouldn't it be better to create a stm32_timers_dma > > structure to place all this information in (except *dev, that should > > live in the ddata struct), then place a reference in the existing > > stm32_timers struct? > > Maybe I miss-understand you here, from what we discussed in V1: > https://lkml.org/lkml/2018/1/23/574 > >... "passing in the physical address of the parent MFD into > > a child device doesn't quite sit right with me" > I introduced this private struct in MFD parent, and completely hide it > from the child. > > So, do you suggest to add struct definition here ? But make it part of > struct stm32_timers *ddata? > > And only put declaration in include/linux/mfd/stm32-timers.h: > + struct stm32_timers_dma; > > struct stm32_timers { > struct clk *clk; > struct regmap *regmap; > u32 max_arr; > + struct stm32_timers_dma; > }; Yes, that's the basic idea. > I can probably spare the *dev then... use dev->parent in child driver. What would you use dev->parent for? [...] > >> +static int stm32_timers_remove(struct platform_device *pdev) > >> +{ > >> + struct stm32_timers *ddata = platform_get_drvdata(pdev); > >> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); > >> + > >> + of_platform_depopulate(&pdev->dev); > > > > Why can't you continue using devm_*? > > I can use devm_of_platform_depopulate() here if you prefer, and keep > devm_of_platform_populate() in probe. The point of devm_* is that you don't have to call depopulate. It happens automatically once this driver is unbound.
On 03/29/2018 02:59 PM, Lee Jones wrote: > On Wed, 28 Mar 2018, Fabrice Gasnier wrote: > >> On 03/28/2018 05:22 PM, Lee Jones wrote: >>> On Wed, 14 Feb 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 v2: >>>> - Abstract DMA handling from child driver: move it to MFD core >>>> - Add comments on optional dma support >>>> --- >>>> drivers/mfd/stm32-timers.c | 215 ++++++++++++++++++++++++++++++++++++++- >>>> include/linux/mfd/stm32-timers.h | 27 +++++ >>>> 2 files changed, 238 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >>>> index a6675a4..2cdad2c 100644 >>>> --- a/drivers/mfd/stm32-timers.c >>>> +++ b/drivers/mfd/stm32-timers.c > > [...] > >>>> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; >>>> + struct stm32_timers ddata; >>> >>> This looks odd to me. Why can't you expand the current ddata >>> structure? Wouldn't it be better to create a stm32_timers_dma >>> structure to place all this information in (except *dev, that should >>> live in the ddata struct), then place a reference in the existing >>> stm32_timers struct? >> >> Maybe I miss-understand you here, from what we discussed in V1: >> https://lkml.org/lkml/2018/1/23/574 >>> ... "passing in the physical address of the parent MFD into >>> a child device doesn't quite sit right with me" >> I introduced this private struct in MFD parent, and completely hide it >> from the child. >> >> So, do you suggest to add struct definition here ? But make it part of >> struct stm32_timers *ddata? >> >> And only put declaration in include/linux/mfd/stm32-timers.h: >> + struct stm32_timers_dma; >> >> struct stm32_timers { >> struct clk *clk; >> struct regmap *regmap; >> u32 max_arr; >> + struct stm32_timers_dma; >> }; > > Yes, that's the basic idea. > >> I can probably spare the *dev then... use dev->parent in child driver. > > What would you use dev->parent for? Hi Lee, This is to follow your sugestion to use *dev instead of *ddata when calling stm32_timers_dma_burst_read(), the idea is to use it on child side: stm32_timers_dma_burst_read(dev->parent,...) from pwm driver. Then there is no need to keep *dev inside ddata struct. > > [...] > >>>> +static int stm32_timers_remove(struct platform_device *pdev) >>>> +{ >>>> + struct stm32_timers *ddata = platform_get_drvdata(pdev); >>>> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); >>>> + >>>> + of_platform_depopulate(&pdev->dev); >>> >>> Why can't you continue using devm_*? >> >> I can use devm_of_platform_depopulate() here if you prefer, and keep >> devm_of_platform_populate() in probe. > > The point of devm_* is that you don't have to call depopulate. > > It happens automatically once this driver is unbound. Ok, so to clarify, keeping devm_ here may be a bit racy: of_platform_depopulate will happen after dma has been released (there is no devm_ variant to release dma). Only way to prevent race condition here, is to enforce of_platform_depopulate() is called before dma release (e.g. in reverse order compared to probe). Do you wish I add a comment about it ? Best Regards, Fabrice > -- 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 Thu, 29 Mar 2018, Fabrice Gasnier wrote: > On 03/29/2018 02:59 PM, Lee Jones wrote: > > On Wed, 28 Mar 2018, Fabrice Gasnier wrote: > > > >> On 03/28/2018 05:22 PM, Lee Jones wrote: > >>> On Wed, 14 Feb 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 v2: > >>>> - Abstract DMA handling from child driver: move it to MFD core > >>>> - Add comments on optional dma support > >>>> --- > >>>> drivers/mfd/stm32-timers.c | 215 ++++++++++++++++++++++++++++++++++++++- > >>>> include/linux/mfd/stm32-timers.h | 27 +++++ > >>>> 2 files changed, 238 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c > >>>> index a6675a4..2cdad2c 100644 > >>>> --- a/drivers/mfd/stm32-timers.c > >>>> +++ b/drivers/mfd/stm32-timers.c > > > > [...] > > > >>>> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; > >>>> + struct stm32_timers ddata; > >>> > >>> This looks odd to me. Why can't you expand the current ddata > >>> structure? Wouldn't it be better to create a stm32_timers_dma > >>> structure to place all this information in (except *dev, that should > >>> live in the ddata struct), then place a reference in the existing > >>> stm32_timers struct? > >> > >> Maybe I miss-understand you here, from what we discussed in V1: > >> https://lkml.org/lkml/2018/1/23/574 > >>> ... "passing in the physical address of the parent MFD into > >>> a child device doesn't quite sit right with me" > >> I introduced this private struct in MFD parent, and completely hide it > >> from the child. > >> > >> So, do you suggest to add struct definition here ? But make it part of > >> struct stm32_timers *ddata? > >> > >> And only put declaration in include/linux/mfd/stm32-timers.h: > >> + struct stm32_timers_dma; > >> > >> struct stm32_timers { > >> struct clk *clk; > >> struct regmap *regmap; > >> u32 max_arr; > >> + struct stm32_timers_dma; > >> }; > > > > Yes, that's the basic idea. > > > >> I can probably spare the *dev then... use dev->parent in child driver. > > > > What would you use dev->parent for? > > Hi Lee, > > This is to follow your sugestion to use *dev instead of *ddata when > calling stm32_timers_dma_burst_read(), the idea is to use it on child side: > stm32_timers_dma_burst_read(dev->parent,...) from pwm driver. > Then there is no need to keep *dev inside ddata struct. I'm wondering if it would be neater to us the child's *dev, then do the ->parent deference in the parent MFD (with a comment to say what you're doing of course). > > [...] > > > >>>> +static int stm32_timers_remove(struct platform_device *pdev) > >>>> +{ > >>>> + struct stm32_timers *ddata = platform_get_drvdata(pdev); > >>>> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); > >>>> + > >>>> + of_platform_depopulate(&pdev->dev); > >>> > >>> Why can't you continue using devm_*? > >> > >> I can use devm_of_platform_depopulate() here if you prefer, and keep > >> devm_of_platform_populate() in probe. > > > > The point of devm_* is that you don't have to call depopulate. > > > > It happens automatically once this driver is unbound. > > Ok, so to clarify, keeping devm_ here may be a bit racy: > of_platform_depopulate will happen after dma has been released (there is > no devm_ variant to release dma). > Only way to prevent race condition here, is to enforce > of_platform_depopulate() is called before dma release (e.g. in reverse > order compared to probe). > > Do you wish I add a comment about it ? Best thing to do then is keep the non-devm variant and provide a comment as to why is it not possible to use devm_*.
On 03/29/2018 04:31 PM, Lee Jones wrote: > On Thu, 29 Mar 2018, Fabrice Gasnier wrote: > >> On 03/29/2018 02:59 PM, Lee Jones wrote: >>> On Wed, 28 Mar 2018, Fabrice Gasnier wrote: >>> >>>> On 03/28/2018 05:22 PM, Lee Jones wrote: >>>>> On Wed, 14 Feb 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 v2: >>>>>> - Abstract DMA handling from child driver: move it to MFD core >>>>>> - Add comments on optional dma support >>>>>> --- >>>>>> drivers/mfd/stm32-timers.c | 215 ++++++++++++++++++++++++++++++++++++++- >>>>>> include/linux/mfd/stm32-timers.h | 27 +++++ >>>>>> 2 files changed, 238 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >>>>>> index a6675a4..2cdad2c 100644 >>>>>> --- a/drivers/mfd/stm32-timers.c >>>>>> +++ b/drivers/mfd/stm32-timers.c >>> >>> [...] >>> >>>>>> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; >>>>>> + struct stm32_timers ddata; >>>>> >>>>> This looks odd to me. Why can't you expand the current ddata >>>>> structure? Wouldn't it be better to create a stm32_timers_dma >>>>> structure to place all this information in (except *dev, that should >>>>> live in the ddata struct), then place a reference in the existing >>>>> stm32_timers struct? >>>> >>>> Maybe I miss-understand you here, from what we discussed in V1: >>>> https://lkml.org/lkml/2018/1/23/574 >>>>> ... "passing in the physical address of the parent MFD into >>>>> a child device doesn't quite sit right with me" >>>> I introduced this private struct in MFD parent, and completely hide it >>>> from the child. >>>> >>>> So, do you suggest to add struct definition here ? But make it part of >>>> struct stm32_timers *ddata? >>>> >>>> And only put declaration in include/linux/mfd/stm32-timers.h: >>>> + struct stm32_timers_dma; >>>> >>>> struct stm32_timers { >>>> struct clk *clk; >>>> struct regmap *regmap; >>>> u32 max_arr; >>>> + struct stm32_timers_dma; >>>> }; >>> >>> Yes, that's the basic idea. >>> >>>> I can probably spare the *dev then... use dev->parent in child driver. >>> >>> What would you use dev->parent for? >> >> Hi Lee, >> >> This is to follow your sugestion to use *dev instead of *ddata when >> calling stm32_timers_dma_burst_read(), the idea is to use it on child side: >> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver. >> Then there is no need to keep *dev inside ddata struct. > > I'm wondering if it would be neater to us the child's *dev, then do > the ->parent deference in the parent MFD (with a comment to say what > you're doing of course). > There's already dev.parent dereference in child drivers, for same purpose: dev_get_drvdata(pdev->dev.parent). So, I guess same can be done here ? Thanks for you review, I'll update all this and send a v3. Best regards, Fabrice >>> [...] >>> >>>>>> +static int stm32_timers_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct stm32_timers *ddata = platform_get_drvdata(pdev); >>>>>> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); >>>>>> + >>>>>> + of_platform_depopulate(&pdev->dev); >>>>> >>>>> Why can't you continue using devm_*? >>>> >>>> I can use devm_of_platform_depopulate() here if you prefer, and keep >>>> devm_of_platform_populate() in probe. >>> >>> The point of devm_* is that you don't have to call depopulate. >>> >>> It happens automatically once this driver is unbound. >> >> Ok, so to clarify, keeping devm_ here may be a bit racy: >> of_platform_depopulate will happen after dma has been released (there is >> no devm_ variant to release dma). >> Only way to prevent race condition here, is to enforce >> of_platform_depopulate() is called before dma release (e.g. in reverse >> order compared to probe). >> >> Do you wish I add a comment about it ? > > Best thing to do then is keep the non-devm variant and provide a > comment as to why is it not possible to use devm_*. > -- 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 a6675a4..2cdad2c 100644 --- a/drivers/mfd/stm32-timers.c +++ b/drivers/mfd/stm32-timers.c @@ -6,16 +6,166 @@ * License terms: GNU General Public License (GPL), version 2 */ +#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_priv { + struct device *dev; + struct completion completion; + phys_addr_t phys_base; /* timers physical addr for dma */ + struct mutex lock; /* protect dma access */ + struct dma_chan *dma_chan; /* dma channel in use */ + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; + struct stm32_timers ddata; +}; + +static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d) +{ + return container_of(d, struct stm32_timers_priv, ddata); +} + +/* 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_priv *priv = p; + struct dma_chan *dma_chan = priv->dma_chan; + struct dma_tx_state state; + enum dma_status status; + + status = dmaengine_tx_status(dma_chan, dma_chan->cookie, &state); + if (status == DMA_COMPLETE) + complete(&priv->completion); +} + +/** + * stm32_timers_dma_burst_read - Read from timers registers using DMA. + * + * Read from STM32 timers registers using DMA on a single event. + * @ddata: reference to stm32_timers + * @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 stm32_timers *ddata, u32 *buf, + enum stm32_timers_dmas id, u32 reg, + unsigned int num_reg, unsigned int bursts, + unsigned long tmo_ms) +{ + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); + unsigned long timeout = msecs_to_jiffies(tmo_ms); + struct regmap *regmap = priv->ddata.regmap; + 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 (!priv->dmas[id]) + return -ENODEV; + mutex_lock(&priv->lock); + priv->dma_chan = priv->dmas[id]; + + dma_buf = dma_map_single(priv->dev, buf, len, DMA_FROM_DEVICE); + ret = dma_mapping_error(priv->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)priv->phys_base + TIM_DMAR; + config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + ret = dmaengine_slave_config(priv->dma_chan, &config); + if (ret) + goto unmap; + + desc = dmaengine_prep_slave_single(priv->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 = priv; + cookie = dmaengine_submit(desc); + ret = dma_submit_error(cookie); + if (ret) + goto dma_term; + + reinit_completion(&priv->completion); + dma_async_issue_pending(priv->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(&priv->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(priv->dma_chan); +unmap: + dma_unmap_single(priv->dev, dma_buf, len, DMA_FROM_DEVICE); +unlock: + priv->dma_chan = NULL; + mutex_unlock(&priv->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) @@ -29,21 +179,55 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) regmap_write(ddata->regmap, TIM_ARR, 0x0); } +static void stm32_timers_dma_probe(struct device *dev, + struct stm32_timers_priv *priv) +{ + int i; + char name[4]; + struct dma_chan **dmas = priv->dmas; + + /* 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); + dmas[i] = dma_request_slave_channel(dev, name); + } + dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up"); + dmas[STM32_TIMERS_DMA_TRIG] = dma_request_slave_channel(dev, "trig"); + dmas[STM32_TIMERS_DMA_COM] = dma_request_slave_channel(dev, "com"); +} + +static void stm32_timers_dma_remove(struct device *dev, + struct stm32_timers_priv *priv) +{ + int i; + + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) + if (priv->dmas[i]) + dma_release_channel(priv->dmas[i]); +} + static int stm32_timers_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct stm32_timers_priv *priv; struct stm32_timers *ddata; struct resource *res; void __iomem *mmio; + int ret; - ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); - if (!ddata) + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) return -ENOMEM; + ddata = &priv->ddata; + init_completion(&priv->completion); + mutex_init(&priv->lock); + priv->dev = dev; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); mmio = devm_ioremap_resource(dev, res); if (IS_ERR(mmio)) return PTR_ERR(mmio); + priv->phys_base = res->start; ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio, &stm32_timers_regmap_cfg); @@ -56,9 +240,31 @@ static int stm32_timers_probe(struct platform_device *pdev) stm32_timers_get_arr_size(ddata); + stm32_timers_dma_probe(dev, priv); + 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) + goto dma_remove; + + return 0; + +dma_remove: + stm32_timers_dma_remove(dev, priv); + + return ret; +} + +static int stm32_timers_remove(struct platform_device *pdev) +{ + struct stm32_timers *ddata = platform_get_drvdata(pdev); + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); + + of_platform_depopulate(&pdev->dev); + stm32_timers_dma_remove(&pdev->dev, priv); + + return 0; } static const struct of_device_id stm32_timers_of_match[] = { @@ -69,6 +275,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 ce7346e..5fd2d6b 100644 --- a/include/linux/mfd/stm32-timers.h +++ b/include/linux/mfd/stm32-timers.h @@ -29,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 */ @@ -38,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 */ @@ -58,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 @@ -67,9 +78,25 @@ #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 { struct clk *clk; struct regmap *regmap; u32 max_arr; }; + +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf, + enum stm32_timers_dmas id, u32 reg, + unsigned int num_reg, unsigned int bursts, + unsigned long tmo_ms); #endif