2018-02-14 10:08:59

by Fabrice Gasnier

Subject: [RESEND PATCH v2 0/8] Add support for PWM input capture on STM32

This series adds support for capture to stm32-pwm driver.
Capture is based on DMAs.
- First two patches are precursor patches
- Subsequent two patches add support for requesting DMAs to MFD core
- Next three patches add support for capture to stm32-pwm driver
- This has been tested on stm32429i-eval board.

---
Resend v2:
- Add collected Acks

Changes in v2:
- Abstract DMA handling from child driver: move it to MFD core
- Rework pwm capture routines to adopt this change
- Comment on optional dma support, beautify DMAs probe

Fabrice Gasnier (8):
pwm: stm32: fix, remove unused struct device
pwm: stm32: protect common prescaler for all channels
dt-bindings: mfd: stm32-timers: add support for dmas
mfd: stm32-timers: add support for dmas
pwm: stm32: add capture support
pwm: stm32: improve capture by tuning counter prescaler
pwm: stm32: use input prescaler to improve period capture
ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval

.../devicetree/bindings/mfd/stm32-timers.txt | 20 ++
arch/arm/boot/dts/stm32429i-eval.dts | 3 +
drivers/mfd/stm32-timers.c | 215 +++++++++++++++-
drivers/pwm/pwm-stm32.c | 276 ++++++++++++++++++++-
include/linux/mfd/stm32-timers.h | 39 +++
5 files changed, 547 insertions(+), 6 deletions(-)

--
1.9.1

2018-02-14 10:06:50

by Fabrice Gasnier

Subject: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
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 */
+ 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
--
1.9.1

2018-02-14 10:07:15

by Fabrice Gasnier

Subject: [RESEND PATCH v2 6/8] pwm: stm32: improve capture by tuning counter prescaler

Currently, capture is based on timeout window to configure prescaler.
PWM capture framework provides 1s window at the time of writing.

There's place for improvement, after input signal has been captured once:
- Finer tune counter clock prescaler, by using 1st capture result (with
arbitrary margin).
- Do a 2nd capture, with scaled capture window.
This increases accuracy, especially at high rates.

Signed-off-by: Fabrice Gasnier <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
Changes in v2:
- Adopt DMA read from MFD core.
---
drivers/pwm/pwm-stm32.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index f3a92fc..5dfb296 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -169,7 +169,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
unsigned long long prd, div, dty;
unsigned long rate;
- unsigned int psc = 0;
+ unsigned int psc = 0, scale;
u32 raw_prd, raw_dty;
int ret = 0;

@@ -220,6 +220,28 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
if (ret)
goto stop;

+ /*
+ * Got a capture. Try to improve accuracy at high rates:
+ * - decrease counter clock prescaler, scale up to max rate.
+ */
+ if (raw_prd) {
+ u32 max_arr = priv->max_arr - 0x1000; /* arbitrary margin */
+
+ scale = max_arr / min(max_arr, raw_prd);
+ } else {
+ scale = priv->max_arr; /* bellow resolution, use max scale */
+ }
+
+ if (psc && scale > 1) {
+ /* 2nd measure with new scale */
+ psc /= scale;
+ regmap_write(priv->regmap, TIM_PSC, psc);
+ ret = stm32_pwm_raw_capture(priv, pwm, tmo_ms, &raw_prd,
+ &raw_dty);
+ if (ret)
+ goto stop;
+ }
+
prd = (unsigned long long)raw_prd * (psc + 1) * NSEC_PER_SEC;
result->period = DIV_ROUND_UP_ULL(prd, rate);
dty = (unsigned long long)raw_dty * (psc + 1) * NSEC_PER_SEC;
--
1.9.1

2018-02-14 10:07:22

by Fabrice Gasnier

Subject: [RESEND PATCH v2 1/8] pwm: stm32: fix, remove unused struct device

dev is never assigned nor used. remove it.

Fixes: 7edf7369205b ("pwm: Add driver for STM32 plaftorm")

Signed-off-by: Fabrice Gasnier <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
drivers/pwm/pwm-stm32.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 6139512..59524f9 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -21,7 +21,6 @@

struct stm32_pwm {
struct pwm_chip chip;
- struct device *dev;
struct clk *clk;
struct regmap *regmap;
u32 max_arr;
--
1.9.1

2018-02-14 10:07:24

by Fabrice Gasnier

Subject: [RESEND PATCH v2 2/8] pwm: stm32: protect common prescaler for all channels

There may be a race, when configuring two pwm channels, with different
prescaler values, when there's no active channel yet.
Add mutex lock to avoid concurrent access on pwm apply state.
This is also precursor patch for pwm capture support.

Signed-off-by: Fabrice Gasnier <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
drivers/pwm/pwm-stm32.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 59524f9..3ac55df 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -21,6 +21,7 @@

struct stm32_pwm {
struct pwm_chip chip;
+ struct mutex lock; /* protect pwm config/enable */
struct clk *clk;
struct regmap *regmap;
u32 max_arr;
@@ -213,9 +214,23 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return ret;
}

+static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+ int ret;
+
+ /* protect common prescaler for all active channels */
+ mutex_lock(&priv->lock);
+ ret = stm32_pwm_apply(chip, pwm, state);
+ mutex_unlock(&priv->lock);
+
+ return ret;
+}
+
static const struct pwm_ops stm32pwm_ops = {
.owner = THIS_MODULE,
- .apply = stm32_pwm_apply,
+ .apply = stm32_pwm_apply_locked,
};

static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
@@ -335,6 +350,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;

+ mutex_init(&priv->lock);
priv->regmap = ddata->regmap;
priv->clk = ddata->clk;
priv->max_arr = ddata->max_arr;
--
1.9.1

2018-02-14 10:07:50

by Fabrice Gasnier

Subject: [RESEND PATCH v2 7/8] pwm: stm32: use input prescaler to improve period capture

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 <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
Changes in v2:
- Adopt DMA read from MFD core.
---
drivers/pwm/pwm-stm32.c | 63 ++++++++++++++++++++++++++++++++++++++--
include/linux/mfd/stm32-timers.h | 1 +
2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 5dfb296..67964f5 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -9,6 +9,7 @@
* pwm-atmel.c from Bo Shen
*/

+#include <linux/bitfield.h>
#include <linux/mfd/stm32-timers.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -169,7 +170,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
unsigned long long prd, div, dty;
unsigned long rate;
- unsigned int psc = 0, scale;
+ unsigned int psc = 0, icpsc, scale;
u32 raw_prd, raw_dty;
int ret = 0;

@@ -223,6 +224,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
/*
* Got a capture. Try to improve accuracy at high rates:
* - decrease counter clock prescaler, scale up to max rate.
+ * - use input prescaler, capture once every /2 /4 or /8 edges.
*/
if (raw_prd) {
u32 max_arr = priv->max_arr - 0x1000; /* arbitrary margin */
@@ -242,8 +244,65 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
goto stop;
}

+ /* Compute intermediate period not to exceed timeout at low rates */
prd = (unsigned long long)raw_prd * (psc + 1) * NSEC_PER_SEC;
- result->period = DIV_ROUND_UP_ULL(prd, rate);
+ do_div(prd, rate);
+
+ for (icpsc = 0; icpsc < MAX_TIM_ICPSC ; icpsc++) {
+ /* input prescaler: also keep arbitrary margin */
+ if (raw_prd >= (priv->max_arr - 0x1000) >> (icpsc + 1))
+ break;
+ if (prd >= (tmo_ms * NSEC_PER_MSEC) >> (icpsc + 2))
+ break;
+ }
+
+ if (!icpsc)
+ goto done;
+
+ /* Last chance to improve period accuracy, using input prescaler */
+ regmap_update_bits(priv->regmap,
+ pwm->hwpwm < 2 ? TIM_CCMR1 : TIM_CCMR2,
+ TIM_CCMR_IC1PSC | TIM_CCMR_IC2PSC,
+ FIELD_PREP(TIM_CCMR_IC1PSC, icpsc) |
+ FIELD_PREP(TIM_CCMR_IC2PSC, icpsc));
+
+ ret = stm32_pwm_raw_capture(priv, pwm, tmo_ms, &raw_prd, &raw_dty);
+ if (ret)
+ goto stop;
+
+ if (raw_dty >= (raw_prd >> icpsc)) {
+ /*
+ * We may fall here using input prescaler, when input
+ * capture starts on high side (before falling edge).
+ * Example with icpsc to capture on each 4 events:
+ *
+ * start 1st capture 2nd capture
+ * v v v
+ * ___ _____ _____ _____ _____ ____
+ * TI1..4 |__| |__| |__| |__| |__|
+ * v v . . . . . v v
+ * icpsc1/3: . 0 . 1 . 2 . 3 . 0
+ * icpsc2/4: 0 1 2 3 0
+ * v v v v
+ * CCR1/3 ......t0..............................t2
+ * CCR2/4 ..t1..............................t1'...
+ * . . .
+ * Capture0: .<----------------------------->.
+ * Capture1: .<-------------------------->. .
+ * . . .
+ * Period: .<------> . .
+ * Low side: .<>.
+ *
+ * Result:
+ * - Period = Capture0 / icpsc
+ * - Duty = Period - Low side = Period - (Capture0 - Capture1)
+ */
+ raw_dty = (raw_prd >> icpsc) - (raw_prd - raw_dty);
+ }
+
+done:
+ prd = (unsigned long long)raw_prd * (psc + 1) * NSEC_PER_SEC;
+ result->period = DIV_ROUND_UP_ULL(prd, rate << icpsc);
dty = (unsigned long long)raw_dty * (psc + 1) * NSEC_PER_SEC;
result->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
stop:
diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index 3abfa04..734cbca 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -82,6 +82,7 @@
#define TIM_DCR_DBL GENMASK(12, 8) /* DMA burst len */

#define MAX_TIM_PSC 0xFFFF
+#define MAX_TIM_ICPSC 0x3
#define TIM_CR2_MMS_SHIFT 4
#define TIM_CR2_MMS2_SHIFT 20
#define TIM_SMCR_TS_SHIFT 4
--
1.9.1

2018-02-14 10:08:19

by Fabrice Gasnier

Subject: [RESEND PATCH v2 3/8] dt-bindings: mfd: stm32-timers: add support for dmas

Add support for DMAs to STM32 timers. STM32 Timers can support up to 7
dma requests: up to 4 channels, update, compare and trigger.
DMAs may be used to transfer data from pwm capture for instance.
DMA support is made optional, PWM capture support is also an option.
This is much more wise system-wide to avoid shortage on DMA request
lines as there's significant amount of timer instances that can
request up to 7 channels.

Signed-off-by: Fabrice Gasnier <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
.../devicetree/bindings/mfd/stm32-timers.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/stm32-timers.txt b/Documentation/devicetree/bindings/mfd/stm32-timers.txt
index 1db6e00..0e900b5 100644
--- a/Documentation/devicetree/bindings/mfd/stm32-timers.txt
+++ b/Documentation/devicetree/bindings/mfd/stm32-timers.txt
@@ -19,6 +19,11 @@ Required parameters:
Optional parameters:
- resets: Phandle to the parent reset controller.
See ../reset/st,stm32-rcc.txt
+- dmas: List of phandle to dma channels that can be used for
+ this timer instance. There may be up to 7 dma channels.
+- dma-names: List of dma names. Must match 'dmas' property. Valid
+ names are: "ch1", "ch2", "ch3", "ch4", "up", "trig",
+ "com".

Optional subnodes:
- pwm: See ../pwm/pwm-stm32.txt
@@ -44,3 +49,18 @@ Example:
reg = <0>;
};
};
+
+Example with all dmas:
+ timer@40010000 {
+ ...
+ dmas = <&dmamux1 11 0x400 0x0>,
+ <&dmamux1 12 0x400 0x0>,
+ <&dmamux1 13 0x400 0x0>,
+ <&dmamux1 14 0x400 0x0>,
+ <&dmamux1 15 0x400 0x0>,
+ <&dmamux1 16 0x400 0x0>,
+ <&dmamux1 17 0x400 0x0>;
+ dma-names = "ch1", "ch2", "ch3", "ch4", "up", "trig", "com";
+ ...
+ child nodes...
+ };
--
1.9.1

2018-02-14 10:09:26

by Fabrice Gasnier

Subject: [RESEND PATCH v2 5/8] pwm: stm32: add capture support

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 <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
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 +++
2 files changed, 187 insertions(+)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 3ac55df..f3a92fc 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -26,6 +26,7 @@ struct stm32_pwm {
struct regmap *regmap;
u32 max_arr;
bool have_complementary_output;
+ u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
};

struct stm32_breakinput {
@@ -63,6 +64,178 @@ static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
return -EINVAL;
}

+#define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P)
+#define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E)
+#define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P)
+#define TIM_CCER_CC34E (TIM_CCER_CC3E | TIM_CCER_CC4E)
+
+/*
+ * Capture using PWM input mode:
+ * ___ ___
+ * TI[1, 2, 3 or 4]: ........._| |________|
+ * ^0 ^1 ^2
+ * . . .
+ * . . XXXXX
+ * . . XXXXX |
+ * . XXXXX . |
+ * XXXXX . . |
+ * COUNTER: ______XXXXX . . . |_XXX
+ * start^ . . . ^stop
+ * . . . .
+ * v v . v
+ * v
+ * CCR1/CCR3: tx..........t0...........t2
+ * CCR2/CCR4: tx..............t1.........
+ *
+ * DMA burst transfer: | |
+ * v v
+ * DMA buffer: { t0, tx } { t2, t1 }
+ * DMA done: ^
+ *
+ * 0: IC1/3 snapchot on rising edge: counter value -> CCR1/CCR3
+ * + DMA transfer CCR[1/3] & CCR[2/4] values (t0, tx: doesn't care)
+ * 1: IC2/4 snapchot on falling edge: counter value -> CCR2/CCR4
+ * 2: IC1/3 snapchot on rising edge: counter value -> CCR1/CCR3
+ * + DMA transfer CCR[1/3] & CCR[2/4] values (t2, t1)
+ *
+ * DMA done, compute:
+ * - Period = t2 - t0
+ * - Duty cycle = t1 - t0
+ */
+static int stm32_pwm_raw_capture(struct stm32_pwm *priv, struct pwm_device *pwm,
+ unsigned long tmo_ms, u32 *raw_prd,
+ u32 *raw_dty)
+{
+ struct stm32_timers *ddata = dev_get_drvdata(priv->chip.dev->parent);
+ enum stm32_timers_dmas dma_id;
+ u32 ccen, ccr;
+ int ret;
+
+ /* Ensure registers have been updated, enable counter and capture */
+ regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
+ regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
+
+ /* Use cc1 or cc3 DMA resp for PWM input channels 1 & 2 or 3 & 4 */
+ dma_id = pwm->hwpwm < 2 ? STM32_TIMERS_DMA_CH1 : STM32_TIMERS_DMA_CH3;
+ ccen = pwm->hwpwm < 2 ? TIM_CCER_CC12E : TIM_CCER_CC34E;
+ ccr = pwm->hwpwm < 2 ? TIM_CCR1 : TIM_CCR3;
+ regmap_update_bits(priv->regmap, TIM_CCER, ccen, ccen);
+
+ /*
+ * Timer DMA burst mode. Request 2 registers, 2 bursts, to get both
+ * CCR1 & CCR2 (or CCR3 & CCR4) on each capture event.
+ * We'll get two capture snapchots: { CCR1, CCR2 }, { CCR1, CCR2 }
+ * or { CCR3, CCR4 }, { CCR3, CCR4 }
+ */
+ ret = stm32_timers_dma_burst_read(ddata, priv->capture, dma_id, ccr, 2,
+ 2, tmo_ms);
+ if (ret)
+ goto stop;
+
+ /* Period: t2 - t0 (take care of counter overflow) */
+ if (priv->capture[0] <= priv->capture[2])
+ *raw_prd = priv->capture[2] - priv->capture[0];
+ else
+ *raw_prd = priv->max_arr - priv->capture[0] + priv->capture[2];
+
+ /* Duty cycle capture requires at least two capture units */
+ if (pwm->chip->npwm < 2)
+ *raw_dty = 0;
+ else if (priv->capture[0] <= priv->capture[3])
+ *raw_dty = priv->capture[3] - priv->capture[0];
+ else
+ *raw_dty = priv->max_arr - priv->capture[0] + priv->capture[3];
+
+ if (*raw_dty > *raw_prd) {
+ /*
+ * Race beetween PWM input and DMA: it may happen
+ * falling edge triggers new capture on TI2/4 before DMA
+ * had a chance to read CCR2/4. It means capture[1]
+ * contains period + duty_cycle. So, subtract period.
+ */
+ *raw_dty -= *raw_prd;
+ }
+
+stop:
+ regmap_update_bits(priv->regmap, TIM_CCER, ccen, 0);
+ regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
+
+ return ret;
+}
+
+static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_capture *result, unsigned long tmo_ms)
+{
+ struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+ unsigned long long prd, div, dty;
+ unsigned long rate;
+ unsigned int psc = 0;
+ u32 raw_prd, raw_dty;
+ int ret = 0;
+
+ mutex_lock(&priv->lock);
+
+ if (active_channels(priv)) {
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ ret = clk_enable(priv->clk);
+ if (ret) {
+ dev_err(priv->chip.dev, "failed to enable counter clock\n");
+ goto unlock;
+ }
+
+ rate = clk_get_rate(priv->clk);
+ if (!rate) {
+ ret = -EINVAL;
+ goto clk_dis;
+ }
+
+ /* prescaler: fit timeout window provided by upper layer */
+ div = (unsigned long long)rate * (unsigned long long)tmo_ms;
+ do_div(div, MSEC_PER_SEC);
+ prd = div;
+ while ((div > priv->max_arr) && (psc < MAX_TIM_PSC)) {
+ psc++;
+ div = prd;
+ do_div(div, psc + 1);
+ }
+ regmap_write(priv->regmap, TIM_ARR, priv->max_arr);
+ regmap_write(priv->regmap, TIM_PSC, psc);
+
+ /* Map TI1 or TI2 PWM input to IC1 & IC2 (or TI3/4 to IC3 & IC4) */
+ regmap_update_bits(priv->regmap,
+ pwm->hwpwm < 2 ? TIM_CCMR1 : TIM_CCMR2,
+ TIM_CCMR_CC1S | TIM_CCMR_CC2S, pwm->hwpwm & 0x1 ?
+ TIM_CCMR_CC1S_TI2 | TIM_CCMR_CC2S_TI2 :
+ TIM_CCMR_CC1S_TI1 | TIM_CCMR_CC2S_TI1);
+
+ /* Capture period on IC1/3 rising edge, duty cycle on IC2/4 falling. */
+ regmap_update_bits(priv->regmap, TIM_CCER, pwm->hwpwm < 2 ?
+ TIM_CCER_CC12P : TIM_CCER_CC34P, pwm->hwpwm < 2 ?
+ TIM_CCER_CC2P : TIM_CCER_CC4P);
+
+ ret = stm32_pwm_raw_capture(priv, pwm, tmo_ms, &raw_prd, &raw_dty);
+ if (ret)
+ goto stop;
+
+ prd = (unsigned long long)raw_prd * (psc + 1) * NSEC_PER_SEC;
+ result->period = DIV_ROUND_UP_ULL(prd, rate);
+ dty = (unsigned long long)raw_dty * (psc + 1) * NSEC_PER_SEC;
+ result->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
+stop:
+ regmap_write(priv->regmap, TIM_CCER, 0);
+ regmap_write(priv->regmap, pwm->hwpwm < 2 ? TIM_CCMR1 : TIM_CCMR2, 0);
+ regmap_write(priv->regmap, TIM_PSC, 0);
+clk_dis:
+ clk_disable(priv->clk);
+unlock:
+ mutex_unlock(&priv->lock);
+
+ return ret;
+}
+
static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
int duty_ns, int period_ns)
{
@@ -231,6 +404,9 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
static const struct pwm_ops stm32pwm_ops = {
.owner = THIS_MODULE,
.apply = stm32_pwm_apply_locked,
+#if IS_ENABLED(CONFIG_DMA_ENGINE)
+ .capture = stm32_pwm_capture,
+#endif
};

static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index 5fd2d6b..3abfa04 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -51,13 +51,24 @@
#define TIM_EGR_UG BIT(0) /* Update Generation */
#define TIM_CCMR_PE BIT(3) /* Channel Preload Enable */
#define TIM_CCMR_M1 (BIT(6) | BIT(5)) /* Channel PWM Mode 1 */
+#define TIM_CCMR_CC1S (BIT(0) | BIT(1)) /* Capture/compare 1 sel */
+#define TIM_CCMR_IC1PSC GENMASK(3, 2) /* Input capture 1 prescaler */
+#define TIM_CCMR_CC2S (BIT(8) | BIT(9)) /* Capture/compare 2 sel */
+#define TIM_CCMR_IC2PSC GENMASK(11, 10) /* Input capture 2 prescaler */
+#define TIM_CCMR_CC1S_TI1 BIT(0) /* IC1/IC3 selects TI1/TI3 */
+#define TIM_CCMR_CC1S_TI2 BIT(1) /* IC1/IC3 selects TI2/TI4 */
+#define TIM_CCMR_CC2S_TI2 BIT(8) /* IC2/IC4 selects TI2/TI4 */
+#define TIM_CCMR_CC2S_TI1 BIT(9) /* IC2/IC4 selects TI1/TI3 */
#define TIM_CCER_CC1E BIT(0) /* Capt/Comp 1 out Ena */
#define TIM_CCER_CC1P BIT(1) /* Capt/Comp 1 Polarity */
#define TIM_CCER_CC1NE BIT(2) /* Capt/Comp 1N out Ena */
#define TIM_CCER_CC1NP BIT(3) /* Capt/Comp 1N Polarity */
#define TIM_CCER_CC2E BIT(4) /* Capt/Comp 2 out Ena */
+#define TIM_CCER_CC2P BIT(5) /* Capt/Comp 2 Polarity */
#define TIM_CCER_CC3E BIT(8) /* Capt/Comp 3 out Ena */
+#define TIM_CCER_CC3P BIT(9) /* Capt/Comp 3 Polarity */
#define TIM_CCER_CC4E BIT(12) /* Capt/Comp 4 out Ena */
+#define TIM_CCER_CC4P BIT(13) /* Capt/Comp 4 Polarity */
#define TIM_CCER_CCXE (BIT(0) | BIT(4) | BIT(8) | BIT(12))
#define TIM_BDTR_BKE BIT(12) /* Break input enable */
#define TIM_BDTR_BKP BIT(13) /* Break input polarity */
--
1.9.1

2018-02-14 10:09:39

by Fabrice Gasnier

Subject: [RESEND PATCH v2 8/8] ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval

Enable pwm3 input capture on stm32f429i-eval, by using DMA.

Signed-off-by: Fabrice Gasnier <[email protected]>
Reviewed-by: Benjamin Gaignard <[email protected]>
---
arch/arm/boot/dts/stm32429i-eval.dts | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
index 293ecb9..d5498dd 100644
--- a/arch/arm/boot/dts/stm32429i-eval.dts
+++ b/arch/arm/boot/dts/stm32429i-eval.dts
@@ -271,6 +271,9 @@
&timers3 {
status = "okay";

+ /* Enable PWM input capture by using dma */
+ dmas = <&dma1 4 5 0x400 0x0>;
+ dma-names = "ch1";
pwm {
pinctrl-0 = <&pwm3_pins>;
pinctrl-names = "default";
--
1.9.1

2018-03-23 15:28:46

by Fabrice Gasnier

Subject: Re: [RESEND PATCH v2 0/8] Add support for PWM input capture on STM32

On 02/14/2018 11:04 AM, Fabrice Gasnier wrote:
> This series adds support for capture to stm32-pwm driver.
> Capture is based on DMAs.
> - First two patches are precursor patches
> - Subsequent two patches add support for requesting DMAs to MFD core
> - Next three patches add support for capture to stm32-pwm driver
> - This has been tested on stm32429i-eval board.
>

Hi all,

Gentle ping to review this series, since DT Bindings has been reviewed
by Rob and the series by Benjamin.

Many thanks in advance,
Regards,
Fabrice

> ---
> Resend v2:
> - Add collected Acks
>
> Changes in v2:
> - Abstract DMA handling from child driver: move it to MFD core
> - Rework pwm capture routines to adopt this change
> - Comment on optional dma support, beautify DMAs probe
>
> Fabrice Gasnier (8):
> pwm: stm32: fix, remove unused struct device
> pwm: stm32: protect common prescaler for all channels
> dt-bindings: mfd: stm32-timers: add support for dmas
> mfd: stm32-timers: add support for dmas
> pwm: stm32: add capture support
> pwm: stm32: improve capture by tuning counter prescaler
> pwm: stm32: use input prescaler to improve period capture
> ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval
>
> .../devicetree/bindings/mfd/stm32-timers.txt | 20 ++
> arch/arm/boot/dts/stm32429i-eval.dts | 3 +
> drivers/mfd/stm32-timers.c | 215 +++++++++++++++-
> drivers/pwm/pwm-stm32.c | 276 ++++++++++++++++++++-
> include/linux/mfd/stm32-timers.h | 39 +++
> 5 files changed, 547 insertions(+), 6 deletions(-)
>

2018-03-27 22:52:57

by Thierry Reding

Subject: Re: [RESEND PATCH v2 0/8] Add support for PWM input capture on STM32

On Wed, Feb 14, 2018 at 11:04:31AM +0100, Fabrice Gasnier wrote:
> This series adds support for capture to stm32-pwm driver.
> Capture is based on DMAs.
> - First two patches are precursor patches
> - Subsequent two patches add support for requesting DMAs to MFD core
> - Next three patches add support for capture to stm32-pwm driver
> - This has been tested on stm32429i-eval board.
>
> ---
> Resend v2:
> - Add collected Acks
>
> Changes in v2:
> - Abstract DMA handling from child driver: move it to MFD core
> - Rework pwm capture routines to adopt this change
> - Comment on optional dma support, beautify DMAs probe
>
> Fabrice Gasnier (8):
> pwm: stm32: fix, remove unused struct device
> pwm: stm32: protect common prescaler for all channels
> dt-bindings: mfd: stm32-timers: add support for dmas
> mfd: stm32-timers: add support for dmas
> pwm: stm32: add capture support
> pwm: stm32: improve capture by tuning counter prescaler
> pwm: stm32: use input prescaler to improve period capture
> ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval
>
> .../devicetree/bindings/mfd/stm32-timers.txt | 20 ++
> arch/arm/boot/dts/stm32429i-eval.dts | 3 +
> drivers/mfd/stm32-timers.c | 215 +++++++++++++++-
> drivers/pwm/pwm-stm32.c | 276 ++++++++++++++++++++-
> include/linux/mfd/stm32-timers.h | 39 +++
> 5 files changed, 547 insertions(+), 6 deletions(-)

I can't apply patches 1-2 and 5-6 since they depend on patches 3-4 for
which I'd need an Acked-by: from Lee if I'm to pick them up. Same goes
for patch 7.

By the looks of it there are minor conflicts with the MFD tree, but no
major ones. Perhaps it'd be better for Lee to pick up 3-4 for v4.17-rc1
and ack patch 7, then I can take the rest after v4.17-rc1?

Thierry

Attachments:

2018-03-27 23:00:39

by Thierry Reding

Subject: Re: [RESEND PATCH v2 0/8] Add support for PWM input capture on STM32

On Wed, Mar 28, 2018 at 12:51:02AM +0200, Thierry Reding wrote:
> On Wed, Feb 14, 2018 at 11:04:31AM +0100, Fabrice Gasnier wrote:
> > This series adds support for capture to stm32-pwm driver.
> > Capture is based on DMAs.
> > - First two patches are precursor patches
> > - Subsequent two patches add support for requesting DMAs to MFD core
> > - Next three patches add support for capture to stm32-pwm driver
> > - This has been tested on stm32429i-eval board.
> >
> > ---
> > Resend v2:
> > - Add collected Acks
> >
> > Changes in v2:
> > - Abstract DMA handling from child driver: move it to MFD core
> > - Rework pwm capture routines to adopt this change
> > - Comment on optional dma support, beautify DMAs probe
> >
> > Fabrice Gasnier (8):
> > pwm: stm32: fix, remove unused struct device
> > pwm: stm32: protect common prescaler for all channels
> > dt-bindings: mfd: stm32-timers: add support for dmas
> > mfd: stm32-timers: add support for dmas
> > pwm: stm32: add capture support
> > pwm: stm32: improve capture by tuning counter prescaler
> > pwm: stm32: use input prescaler to improve period capture
> > ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval
> >
> > .../devicetree/bindings/mfd/stm32-timers.txt | 20 ++
> > arch/arm/boot/dts/stm32429i-eval.dts | 3 +
> > drivers/mfd/stm32-timers.c | 215 +++++++++++++++-
> > drivers/pwm/pwm-stm32.c | 276 ++++++++++++++++++++-
> > include/linux/mfd/stm32-timers.h | 39 +++
> > 5 files changed, 547 insertions(+), 6 deletions(-)
>
> I can't apply patches 1-2 and 5-6 since they depend on patches 3-4 for
> which I'd need an Acked-by: from Lee if I'm to pick them up. Same goes
> for patch 7.
>
> By the looks of it there are minor conflicts with the MFD tree, but no
> major ones. Perhaps it'd be better for Lee to pick up 3-4 for v4.17-rc1
> and ack patch 7, then I can take the rest after v4.17-rc1?

I can pick up 1-2 which are separate from the capture changes and have
no dependencies.

Thierry

Attachments:

2018-03-28 10:55:11

by Lee Jones

Subject: Re: [RESEND PATCH v2 0/8] Add support for PWM input capture on STM32

On Wed, 28 Mar 2018, Thierry Reding wrote:

> On Wed, Feb 14, 2018 at 11:04:31AM +0100, Fabrice Gasnier wrote:
> > This series adds support for capture to stm32-pwm driver.
> > Capture is based on DMAs.
> > - First two patches are precursor patches
> > - Subsequent two patches add support for requesting DMAs to MFD core
> > - Next three patches add support for capture to stm32-pwm driver
> > - This has been tested on stm32429i-eval board.
> >
> > ---
> > Resend v2:
> > - Add collected Acks
> >
> > Changes in v2:
> > - Abstract DMA handling from child driver: move it to MFD core
> > - Rework pwm capture routines to adopt this change
> > - Comment on optional dma support, beautify DMAs probe
> >
> > Fabrice Gasnier (8):
> > pwm: stm32: fix, remove unused struct device
> > pwm: stm32: protect common prescaler for all channels
> > dt-bindings: mfd: stm32-timers: add support for dmas
> > mfd: stm32-timers: add support for dmas
> > pwm: stm32: add capture support
> > pwm: stm32: improve capture by tuning counter prescaler
> > pwm: stm32: use input prescaler to improve period capture
> > ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval
> >
> > .../devicetree/bindings/mfd/stm32-timers.txt | 20 ++
> > arch/arm/boot/dts/stm32429i-eval.dts | 3 +
> > drivers/mfd/stm32-timers.c | 215 +++++++++++++++-
> > drivers/pwm/pwm-stm32.c | 276 ++++++++++++++++++++-
> > include/linux/mfd/stm32-timers.h | 39 +++
> > 5 files changed, 547 insertions(+), 6 deletions(-)
>
> I can't apply patches 1-2 and 5-6 since they depend on patches 3-4 for
> which I'd need an Acked-by: from Lee if I'm to pick them up. Same goes
> for patch 7.
>
> By the looks of it there are minor conflicts with the MFD tree, but no
> major ones. Perhaps it'd be better for Lee to pick up 3-4 for v4.17-rc1
> and ack patch 7, then I can take the rest after v4.17-rc1?

Sounds like a faff.

Why don't I just pick them all up (except the ARM patch)?

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2018-03-28 11:18:02

by Lee Jones

Subject: Re: [RESEND PATCH v2 3/8] dt-bindings: mfd: stm32-timers: add support for dmas

On Wed, 14 Feb 2018, Fabrice Gasnier wrote:

> Add support for DMAs to STM32 timers. STM32 Timers can support up to 7
> dma requests: up to 4 channels, update, compare and trigger.
> DMAs may be used to transfer data from pwm capture for instance.
> DMA support is made optional, PWM capture support is also an option.
> This is much more wise system-wide to avoid shortage on DMA request
> lines as there's significant amount of timer instances that can
> request up to 7 channels.
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Benjamin Gaignard <[email protected]>
> ---
> .../devicetree/bindings/mfd/stm32-timers.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)

For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2018-03-28 15:25:34

by Lee Jones

Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
> Reviewed-by: Benjamin Gaignard <[email protected]>
> ---
> 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

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2018-03-28 16:43:02

by Fabrice Gasnier

Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
>> Reviewed-by: Benjamin Gaignard <[email protected]>
>> ---
>> 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
>

2018-03-29 08:59:55

by Thierry Reding

Subject: Re: [RESEND PATCH v2 0/8] Add support for PWM input capture on STM32

On Wed, Mar 28, 2018 at 11:03:09AM +0100, Lee Jones wrote:
> On Wed, 28 Mar 2018, Thierry Reding wrote:
>
> > On Wed, Feb 14, 2018 at 11:04:31AM +0100, Fabrice Gasnier wrote:
> > > This series adds support for capture to stm32-pwm driver.
> > > Capture is based on DMAs.
> > > - First two patches are precursor patches
> > > - Subsequent two patches add support for requesting DMAs to MFD core
> > > - Next three patches add support for capture to stm32-pwm driver
> > > - This has been tested on stm32429i-eval board.
> > >
> > > ---
> > > Resend v2:
> > > - Add collected Acks
> > >
> > > Changes in v2:
> > > - Abstract DMA handling from child driver: move it to MFD core
> > > - Rework pwm capture routines to adopt this change
> > > - Comment on optional dma support, beautify DMAs probe
> > >
> > > Fabrice Gasnier (8):
> > > pwm: stm32: fix, remove unused struct device
> > > pwm: stm32: protect common prescaler for all channels
> > > dt-bindings: mfd: stm32-timers: add support for dmas
> > > mfd: stm32-timers: add support for dmas
> > > pwm: stm32: add capture support
> > > pwm: stm32: improve capture by tuning counter prescaler
> > > pwm: stm32: use input prescaler to improve period capture
> > > ARM: dts: stm32: Enable pwm3 input capture on stm32f429i-eval
> > >
> > > .../devicetree/bindings/mfd/stm32-timers.txt | 20 ++
> > > arch/arm/boot/dts/stm32429i-eval.dts | 3 +
> > > drivers/mfd/stm32-timers.c | 215 +++++++++++++++-
> > > drivers/pwm/pwm-stm32.c | 276 ++++++++++++++++++++-
> > > include/linux/mfd/stm32-timers.h | 39 +++
> > > 5 files changed, 547 insertions(+), 6 deletions(-)
> >
> > I can't apply patches 1-2 and 5-6 since they depend on patches 3-4 for
> > which I'd need an Acked-by: from Lee if I'm to pick them up. Same goes
> > for patch 7.
> >
> > By the looks of it there are minor conflicts with the MFD tree, but no
> > major ones. Perhaps it'd be better for Lee to pick up 3-4 for v4.17-rc1
> > and ack patch 7, then I can take the rest after v4.17-rc1?
>
> Sounds like a faff.
>
> Why don't I just pick them all up (except the ARM patch)?

Fine with me. For all the PWM patches in this series:

Acked-by: Thierry Reding <[email protected]>

Attachments:

2018-03-29 13:00:44

by Lee Jones

Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
> >> Reviewed-by: Benjamin Gaignard <[email protected]>
> >> ---
> >> 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.

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2018-03-29 13:43:16

by Fabrice Gasnier

Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
>>>> Reviewed-by: Benjamin Gaignard <[email protected]>
>>>> ---
>>>> 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

>

2018-03-29 14:33:32

by Lee Jones

Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
> >>>> Reviewed-by: Benjamin Gaignard <[email protected]>
> >>>> ---
> >>>> 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_*.

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2018-03-29 15:32:11

by Fabrice Gasnier

Subject: Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

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 <[email protected]>
>>>>>> Reviewed-by: Benjamin Gaignard <[email protected]>
>>>>>> ---
>>>>>> 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_*.
>