2018-01-16 12:46:26

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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.

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 | 37 +-
drivers/pwm/pwm-stm32.c | 407 ++++++++++++++++++++-
include/linux/mfd/stm32-timers.h | 31 ++
5 files changed, 492 insertions(+), 6 deletions(-)

--
1.9.1


2018-01-16 12:45:27

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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, one for duty-cycle.
When there's only one channel available, only period can be captured.
It's zero'ed in such a case.
This requires exclusive access (e.g. no pwm output running at the same
time, to protect common prescaler).
Timer DMA burst mode is being used, to take two snapshots of capture
registers (upon each period rising edge).
Falling edge captures duty cycle to capture registers (only).

Signed-off-by: Fabrice Gasnier <[email protected]>
---
drivers/pwm/pwm-stm32.c | 301 ++++++++++++++++++++++++++++++++++++++-
include/linux/mfd/stm32-timers.h | 16 +++
2 files changed, 314 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 3ac55df..b9c7e878 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/dma-mapping.h>
#include <linux/mfd/stm32-timers.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -21,11 +22,17 @@

struct stm32_pwm {
struct pwm_chip chip;
+ struct completion completion;
struct mutex lock; /* protect pwm config/enable */
struct clk *clk;
struct regmap *regmap;
+ phys_addr_t phys_base;
u32 max_arr;
bool have_complementary_output;
+ u32 capture[2];
+ struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
+ dma_addr_t dma_buf_phys; /* dma buffer bus address (phys) */
+ u32 *dma_buf; /* dma buffer cpu address */
};

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

+/*
+ * 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 void stm32_pwm_dma_done(void *p)
+{
+ struct pwm_device *pwm = p;
+ struct stm32_pwm *priv = to_stm32_pwm_dev(pwm->chip);
+ /* Use cc1 / cc3 DMA resp for PWM input channels 1 & 2 / 3 & 4 */
+ struct dma_chan *dma_chan = priv->dmas[pwm->hwpwm < 2 ? 0 : 2];
+ struct dma_tx_state state;
+ enum dma_status status;
+
+ status = dmaengine_tx_status(dma_chan, dma_chan->cookie, &state);
+ if (status == DMA_COMPLETE) {
+ /* Period: t2 - t0 (take care of counter overflow) */
+ if (priv->dma_buf[0] <= priv->dma_buf[2])
+ priv->capture[0] = priv->dma_buf[2] - priv->dma_buf[0];
+ else
+ priv->capture[0] = priv->max_arr - priv->dma_buf[0] +
+ priv->dma_buf[2];
+
+ /* Duty cycle capture requires at least two capture units */
+ if (pwm->chip->npwm < 2)
+ priv->capture[1] = 0;
+ else if (priv->dma_buf[0] <= priv->dma_buf[3])
+ priv->capture[1] = priv->dma_buf[3] - priv->dma_buf[0];
+ else
+ priv->capture[1] = priv->max_arr - priv->dma_buf[0] +
+ priv->dma_buf[3];
+
+ if (priv->capture[1] > priv->capture[0]) {
+ /*
+ * 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.
+ */
+ priv->capture[1] -= priv->capture[0];
+ }
+ complete(&priv->completion);
+ }
+}
+
+#define TIM_DCR_DBL_2_TRANSFERS BIT(8)
+
+static int stm32_pwm_dma_start(struct stm32_pwm *priv, struct pwm_device *pwm)
+{
+ struct dma_chan *dma_chan = priv->dmas[pwm->hwpwm < 2 ? 0 : 2];
+ struct dma_async_tx_descriptor *desc;
+ struct dma_slave_config config;
+ dma_cookie_t cookie;
+ int ret;
+
+ if (!dma_chan || !priv->dma_buf)
+ return -ENODEV;
+
+ 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(dma_chan, &config);
+ if (ret)
+ return ret;
+
+ /* Prepare DMA transaction (4 32-bits words, twice bellow transfer) */
+ desc = dmaengine_prep_slave_single(dma_chan, priv->dma_buf_phys,
+ 4 * sizeof(u32), DMA_DEV_TO_MEM,
+ DMA_PREP_INTERRUPT);
+ if (!desc)
+ return -EBUSY;
+
+ desc->callback = stm32_pwm_dma_done;
+ desc->callback_param = pwm;
+ cookie = dmaengine_submit(desc);
+ ret = dma_submit_error(cookie);
+ if (ret) {
+ dmaengine_terminate_all(dma_chan);
+ return ret;
+ }
+ dma_async_issue_pending(dma_chan);
+
+ /*
+ * Timer DMA burst mode. Request 2 transfers to get both CCR1 & CCR2
+ * (or CCR3 & CCR4) on each capture event from DMAR register:
+ * - DBL (transfer len): 2
+ * - DBA (start offset): CCR1 or CCR3 (offset / 4)
+ * As 4 32-bits words has been requested above, we'll get two
+ * snapchots in 'dma_buf': { CCR1, CCR2 }, { CCR1, CCR2 }
+ * or { CCR3, CCR4 }, { CCR3, CCR4 }
+ */
+ regmap_write(priv->regmap, TIM_DCR, TIM_DCR_DBL_2_TRANSFERS |
+ (pwm->hwpwm < 2 ? (TIM_CCR1 / 4) : (TIM_CCR3 / 4)));
+ regmap_update_bits(priv->regmap, TIM_DIER, TIM_DIER_XDE,
+ pwm->hwpwm < 2 ? TIM_DIER_CC1DE : TIM_DIER_CC3DE);
+
+ return 0;
+}
+
+static void stm32_pwm_dma_stop(struct stm32_pwm *priv, struct pwm_device *pwm)
+{
+ struct dma_chan *dma_chan = priv->dmas[pwm->hwpwm < 2 ? 0 : 2];
+
+ regmap_update_bits(priv->regmap, TIM_DIER, TIM_DIER_XDE, 0);
+ regmap_write(priv->regmap, TIM_DCR, 0);
+ dmaengine_terminate_all(dma_chan);
+}
+
+#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)
+
+static int stm32_pwm_do_capture(struct stm32_pwm *priv, struct pwm_device *pwm,
+ unsigned long tmo_ms)
+{
+ unsigned long timeout = msecs_to_jiffies(tmo_ms);
+ long err;
+ int ret;
+
+ reinit_completion(&priv->completion);
+
+ /* Ensure registers have been updated */
+ regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
+
+ /* Clear pending flags, start counter, enable DMA, then capture */
+ regmap_write(priv->regmap, TIM_SR, 0);
+ regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
+ ret = stm32_pwm_dma_start(priv, pwm);
+ if (ret)
+ goto stop;
+ regmap_update_bits(priv->regmap, TIM_CCER, pwm->hwpwm < 2 ?
+ TIM_CCER_CC12E : TIM_CCER_CC34E,
+ TIM_CCER_CC12E | TIM_CCER_CC34E);
+
+ err = wait_for_completion_interruptible_timeout(&priv->completion,
+ timeout);
+
+ regmap_update_bits(priv->regmap, TIM_CCER, pwm->hwpwm < 2 ?
+ TIM_CCER_CC12E : TIM_CCER_CC34E, 0);
+ stm32_pwm_dma_stop(priv, pwm);
+ if (err == 0)
+ ret = -ETIMEDOUT;
+ else if (err < 0)
+ ret = err;
+
+stop:
+ regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
+ regmap_write(priv->regmap, TIM_SR, 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_do_capture(priv, pwm, tmo_ms);
+ if (ret)
+ goto stop;
+
+ raw_prd = priv->capture[0];
+ raw_dty = priv->capture[1];
+
+ 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 +495,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,
@@ -344,23 +611,40 @@ static int stm32_pwm_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
struct stm32_pwm *priv;
- int ret;
+ bool dma = false;
+ int i, ret;

priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;

mutex_init(&priv->lock);
+ init_completion(&priv->completion);
priv->regmap = ddata->regmap;
+ priv->phys_base = ddata->phys_base;
priv->clk = ddata->clk;
priv->max_arr = ddata->max_arr;

if (!priv->regmap || !priv->clk)
return -EINVAL;

+ for (i = 0; i < STM32_TIMERS_MAX_DMAS; i++) {
+ priv->dmas[i] = ddata->dmas[i];
+ if (priv->dmas[i])
+ dma = true;
+ }
+
+ if (dma) {
+ priv->dma_buf = dma_alloc_coherent(dev->parent, PAGE_SIZE,
+ &priv->dma_buf_phys,
+ GFP_KERNEL);
+ if (!priv->dma_buf)
+ dev_dbg(dev, "can't allocate DMA buffer\n");
+ }
+
ret = stm32_pwm_apply_breakinputs(priv, np);
if (ret)
- return ret;
+ goto dma_free;

stm32_pwm_detect_complementary(priv);

@@ -371,11 +655,18 @@ static int stm32_pwm_probe(struct platform_device *pdev)

ret = pwmchip_add(&priv->chip);
if (ret < 0)
- return ret;
+ goto dma_free;

platform_set_drvdata(pdev, priv);

return 0;
+
+dma_free:
+ if (priv->dma_buf)
+ dma_free_coherent(priv->chip.dev, PAGE_SIZE,
+ priv->dma_buf, priv->dma_buf_phys);
+
+ return ret;
}

static int stm32_pwm_remove(struct platform_device *pdev)
@@ -388,6 +679,10 @@ static int stm32_pwm_remove(struct platform_device *pdev)

pwmchip_remove(&priv->chip);

+ if (priv->dma_buf)
+ dma_free_coherent(priv->chip.dev, PAGE_SIZE,
+ priv->dma_buf, priv->dma_buf_phys);
+
return 0;
}

diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index 2b4ffb9..219eccc 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -30,6 +30,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 */
@@ -39,17 +41,31 @@
#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_CC1DE BIT(9) /* CC1 DMA request Enable */
+#define TIM_DIER_CC3DE BIT(11) /* CC3 DMA request Enable */
+#define TIM_DIER_XDE GENMASK(14, 8)
#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 */
#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-01-16 12:45:45

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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]>
---
.../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-01-16 12:45:24

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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]>
---
drivers/pwm/pwm-stm32.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index b9c7e878..c890404 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -257,7 +257,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;

@@ -311,6 +311,30 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
raw_prd = priv->capture[0];
raw_dty = priv->capture[1];

+ /*
+ * 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 */
+ regmap_write(priv->regmap, TIM_PSC, psc / scale);
+ ret = stm32_pwm_do_capture(priv, pwm, tmo_ms);
+ if (ret)
+ goto stop;
+
+ psc /= scale;
+ raw_prd = priv->capture[0];
+ raw_dty = priv->capture[1];
+ }
+
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-01-16 12:46:03

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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]>
---
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-01-16 12:46:42

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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, keep reference of device's bus address to allow child drivers to
transfer data from/to device by using dma.

Signed-off-by: Fabrice Gasnier <[email protected]>
---
drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
index a6675a4..372b51e 100644
--- a/drivers/mfd/stm32-timers.c
+++ b/drivers/mfd/stm32-timers.c
@@ -29,6 +29,23 @@ 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 *ddata)
+{
+ int i;
+ char name[4];
+
+ for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
+ snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
+ ddata->dmas[i] = dma_request_slave_channel(dev, name);
+ }
+ ddata->dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up");
+ ddata->dmas[STM32_TIMERS_DMA_TRIG] =
+ dma_request_slave_channel(dev, "trig");
+ ddata->dmas[STM32_TIMERS_DMA_COM] =
+ dma_request_slave_channel(dev, "com");
+}
+
static int stm32_timers_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
mmio = devm_ioremap_resource(dev, res);
if (IS_ERR(mmio))
return PTR_ERR(mmio);
+ ddata->phys_base = res->start;

ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio,
&stm32_timers_regmap_cfg);
@@ -56,9 +74,25 @@ static int stm32_timers_probe(struct platform_device *pdev)

stm32_timers_get_arr_size(ddata);

+ stm32_timers_dma_probe(dev, ddata);
+
platform_set_drvdata(pdev, ddata);

- return devm_of_platform_populate(&pdev->dev);
+ return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static int stm32_timers_remove(struct platform_device *pdev)
+{
+ struct stm32_timers *ddata = platform_get_drvdata(pdev);
+ int i;
+
+ of_platform_depopulate(&pdev->dev);
+
+ for (i = 0; i < STM32_TIMERS_MAX_DMAS; i++)
+ if (ddata->dmas[i])
+ dma_release_channel(ddata->dmas[i]);
+
+ return 0;
}

static const struct of_device_id stm32_timers_of_match[] = {
@@ -69,6 +103,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..2b4ffb9 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -10,6 +10,7 @@
#define _LINUX_STM32_GPTIMER_H_

#include <linux/clk.h>
+#include <linux/dmaengine.h>
#include <linux/regmap.h>

#define TIM_CR1 0x00 /* Control Register 1 */
@@ -67,9 +68,22 @@
#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;
+ phys_addr_t phys_base;
u32 max_arr;
+ struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
};
#endif
--
1.9.1

2018-01-16 12:47:00

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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 can help improve
period (only) capture accuracy.

Signed-off-by: Fabrice Gasnier <[email protected]>
---
drivers/pwm/pwm-stm32.c | 67 ++++++++++++++++++++++++++++++++++++++--
include/linux/mfd/stm32-timers.h | 1 +
2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index c890404..6b99d92 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/dma-mapping.h>
#include <linux/mfd/stm32-timers.h>
#include <linux/module.h>
@@ -257,7 +258,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;

@@ -314,6 +315,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 */
@@ -335,8 +337,69 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
raw_dty = priv->capture[1];
}

+ /* 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_do_capture(priv, pwm, tmo_ms);
+ if (ret)
+ goto stop;
+
+ raw_prd = priv->capture[0];
+ raw_dty = priv->capture[1];
+
+ 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 = (t2 - t0) / icpsc
+ * - Duty = Period - Low side = Period - (Capture0 - Capture1)
+ */
+ raw_dty = priv->capture[0] - priv->capture[1];
+ raw_dty = (raw_prd >> icpsc) - 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 219eccc..b67cb28 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -77,6 +77,7 @@
#define TIM_BDTR_BK2P BIT(25) /* Break 2 input polarity */

#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-01-16 12:46:58

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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]>
---
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-01-16 12:47:28

by Fabrice Gasnier

[permalink] [raw]
Subject: [PATCH 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]>
---
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-01-23 13:33:52

by Lee Jones

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

On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
> transfer data from/to device by using dma.
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
> ---
> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
> 2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> index a6675a4..372b51e 100644
> --- a/drivers/mfd/stm32-timers.c
> +++ b/drivers/mfd/stm32-timers.c
> @@ -29,6 +29,23 @@ 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 *ddata)
> +{
> + int i;
> + char name[4];
> +
> + for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
> + snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
> + ddata->dmas[i] = dma_request_slave_channel(dev, name);

And if any of them fail?

> + }
> + ddata->dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up");
> + ddata->dmas[STM32_TIMERS_DMA_TRIG] =
> + dma_request_slave_channel(dev, "trig");
> + ddata->dmas[STM32_TIMERS_DMA_COM] =
> + dma_request_slave_channel(dev, "com");

Can you format these in the same why. This hurts my eyes.

> +}
> +
> static int stm32_timers_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
> mmio = devm_ioremap_resource(dev, res);
> if (IS_ERR(mmio))
> return PTR_ERR(mmio);
> + ddata->phys_base = res->start;

What do you use this for?

> ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio,
> &stm32_timers_regmap_cfg);
> @@ -56,9 +74,25 @@ static int stm32_timers_probe(struct platform_device *pdev)
>
> stm32_timers_get_arr_size(ddata);
>
> + stm32_timers_dma_probe(dev, ddata);
> +

Surely this can fail?

> platform_set_drvdata(pdev, ddata);
>
> - return devm_of_platform_populate(&pdev->dev);
> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +}
> +
> +static int stm32_timers_remove(struct platform_device *pdev)
> +{
> + struct stm32_timers *ddata = platform_get_drvdata(pdev);
> + int i;
> +
> + of_platform_depopulate(&pdev->dev);
> +
> + for (i = 0; i < STM32_TIMERS_MAX_DMAS; i++)
> + if (ddata->dmas[i])
> + dma_release_channel(ddata->dmas[i]);
> +
> + return 0;
> }
>
> static const struct of_device_id stm32_timers_of_match[] = {
> @@ -69,6 +103,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..2b4ffb9 100644
> --- a/include/linux/mfd/stm32-timers.h
> +++ b/include/linux/mfd/stm32-timers.h
> @@ -10,6 +10,7 @@
> #define _LINUX_STM32_GPTIMER_H_
>
> #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> #include <linux/regmap.h>
>
> #define TIM_CR1 0x00 /* Control Register 1 */
> @@ -67,9 +68,22 @@
> #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;
> + phys_addr_t phys_base;
> u32 max_arr;
> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
> };
> #endif

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

2018-01-23 13:59:12

by Fabrice Gasnier

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

On 01/23/2018 02:32 PM, Lee Jones wrote:
> On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
>> transfer data from/to device by using dma.
>>
>> Signed-off-by: Fabrice Gasnier <[email protected]>
>> ---
>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
>> 2 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>> index a6675a4..372b51e 100644
>> --- a/drivers/mfd/stm32-timers.c
>> +++ b/drivers/mfd/stm32-timers.c
>> @@ -29,6 +29,23 @@ 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 *ddata)
>> +{
>> + int i;
>> + char name[4];
>> +
>> + for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
>> + snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
>> + ddata->dmas[i] = dma_request_slave_channel(dev, name);
>
> And if any of them fail?

Hi Lee,

If some of these fails, reference will be NULL. It is checked in child
driver (pwm for instance) at runtime. Support is being added as an
option: pwm capture will simply be unavailable in this case (fail with
error).

>
>> + }
>> + ddata->dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up");
>> + ddata->dmas[STM32_TIMERS_DMA_TRIG] =
>> + dma_request_slave_channel(dev, "trig");
>> + ddata->dmas[STM32_TIMERS_DMA_COM] =
>> + dma_request_slave_channel(dev, "com");
>
> Can you format these in the same why. This hurts my eyes.

I use enum values and try to match as possible with reference manual for
"up", "trig" & "com" names.
Would have some suggestion to beautify this?

>
>> +}
>> +
>> static int stm32_timers_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
>> mmio = devm_ioremap_resource(dev, res);
>> if (IS_ERR(mmio))
>> return PTR_ERR(mmio);
>> + ddata->phys_base = res->start;
>
> What do you use this for?

This is used in in child driver (pwm) for capture data transfer by dma.

>
>> ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio,
>> &stm32_timers_regmap_cfg);
>> @@ -56,9 +74,25 @@ static int stm32_timers_probe(struct platform_device *pdev)
>>
>> stm32_timers_get_arr_size(ddata);
>>
>> + stm32_timers_dma_probe(dev, ddata);
>> +
>
> Surely this can fail?
Yes. Same as above.

Thanks for reviewing,
Best Regards,
Fabrice

>
>> platform_set_drvdata(pdev, ddata);
>>
>> - return devm_of_platform_populate(&pdev->dev);
>> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>> +}
>> +
>> +static int stm32_timers_remove(struct platform_device *pdev)
>> +{
>> + struct stm32_timers *ddata = platform_get_drvdata(pdev);
>> + int i;
>> +
>> + of_platform_depopulate(&pdev->dev);
>> +
>> + for (i = 0; i < STM32_TIMERS_MAX_DMAS; i++)
>> + if (ddata->dmas[i])
>> + dma_release_channel(ddata->dmas[i]);
>> +
>> + return 0;
>> }
>>
>> static const struct of_device_id stm32_timers_of_match[] = {
>> @@ -69,6 +103,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..2b4ffb9 100644
>> --- a/include/linux/mfd/stm32-timers.h
>> +++ b/include/linux/mfd/stm32-timers.h
>> @@ -10,6 +10,7 @@
>> #define _LINUX_STM32_GPTIMER_H_
>>
>> #include <linux/clk.h>
>> +#include <linux/dmaengine.h>
>> #include <linux/regmap.h>
>>
>> #define TIM_CR1 0x00 /* Control Register 1 */
>> @@ -67,9 +68,22 @@
>> #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;
>> + phys_addr_t phys_base;
>> u32 max_arr;
>> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>> };
>> #endif
>

2018-01-23 15:31:27

by Lee Jones

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

On Tue, 23 Jan 2018, Fabrice Gasnier wrote:

> On 01/23/2018 02:32 PM, Lee Jones wrote:
> > On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
> >> transfer data from/to device by using dma.
> >>
> >> Signed-off-by: Fabrice Gasnier <[email protected]>
> >> ---
> >> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
> >> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
> >> 2 files changed, 50 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> >> index a6675a4..372b51e 100644
> >> --- a/drivers/mfd/stm32-timers.c
> >> +++ b/drivers/mfd/stm32-timers.c
> >> @@ -29,6 +29,23 @@ 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 *ddata)
> >> +{
> >> + int i;
> >> + char name[4];
> >> +
> >> + for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
> >> + snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
> >> + ddata->dmas[i] = dma_request_slave_channel(dev, name);
> >
> > And if any of them fail?
>
> Hi Lee,
>
> If some of these fails, reference will be NULL. It is checked in child
> driver (pwm for instance) at runtime. Support is being added as an
> option: pwm capture will simply be unavailable in this case (fail with
> error).

Can you place a simple one-line comment in here please. Or else
someone will come along and add error handling for you.

> >> + }
> >> + ddata->dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up");
> >> + ddata->dmas[STM32_TIMERS_DMA_TRIG] =
> >> + dma_request_slave_channel(dev, "trig");
> >> + ddata->dmas[STM32_TIMERS_DMA_COM] =
> >> + dma_request_slave_channel(dev, "com");
> >
> > Can you format these in the same why. This hurts my eyes.
>
> I use enum values and try to match as possible with reference manual for
> "up", "trig" & "com" names.
> Would have some suggestion to beautify this?

I just mean, can you push the first dma_request_slave_channel() call
on to the next line, so each of the calls are formatted in the same
manner please?

> >> +}
> >> +
> >> static int stm32_timers_probe(struct platform_device *pdev)
> >> {
> >> struct device *dev = &pdev->dev;
> >> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
> >> mmio = devm_ioremap_resource(dev, res);
> >> if (IS_ERR(mmio))
> >> return PTR_ERR(mmio);
> >> + ddata->phys_base = res->start;
> >
> > What do you use this for?
>
> This is used in in child driver (pwm) for capture data transfer by dma.

Might be worth being clear about that.

Perhaps pass in 'dma_base' (phys_base + offset) instead?

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

2018-01-23 15:53:57

by Fabrice Gasnier

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

On 01/23/2018 04:30 PM, Lee Jones wrote:
> On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
>
>> On 01/23/2018 02:32 PM, Lee Jones wrote:
>>> On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
>>>> transfer data from/to device by using dma.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <[email protected]>
>>>> ---
>>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
>>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
>>>> 2 files changed, 50 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>>>> index a6675a4..372b51e 100644
>>>> --- a/drivers/mfd/stm32-timers.c
>>>> +++ b/drivers/mfd/stm32-timers.c
>>>> @@ -29,6 +29,23 @@ 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 *ddata)
>>>> +{
>>>> + int i;
>>>> + char name[4];
>>>> +
>>>> + for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
>>>> + snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
>>>> + ddata->dmas[i] = dma_request_slave_channel(dev, name);
>>>
>>> And if any of them fail?
>>
>> Hi Lee,
>>
>> If some of these fails, reference will be NULL. It is checked in child
>> driver (pwm for instance) at runtime. Support is being added as an
>> option: pwm capture will simply be unavailable in this case (fail with
>> error).
>
> Can you place a simple one-line comment in here please. Or else
> someone will come along and add error handling for you.

Hi Lee,

Ok, I'll add a comment in v2.

>
>>>> + }
>>>> + ddata->dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up");
>>>> + ddata->dmas[STM32_TIMERS_DMA_TRIG] =
>>>> + dma_request_slave_channel(dev, "trig");
>>>> + ddata->dmas[STM32_TIMERS_DMA_COM] =
>>>> + dma_request_slave_channel(dev, "com");
>>>
>>> Can you format these in the same why. This hurts my eyes.
>>
>> I use enum values and try to match as possible with reference manual for
>> "up", "trig" & "com" names.
>> Would have some suggestion to beautify this?
>
> I just mean, can you push the first dma_request_slave_channel() call
> on to the next line, so each of the calls are formatted in the same
> manner please?

Ok, got it now. I'll do this in v2.

>
>>>> +}
>>>> +
>>>> static int stm32_timers_probe(struct platform_device *pdev)
>>>> {
>>>> struct device *dev = &pdev->dev;
>>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
>>>> mmio = devm_ioremap_resource(dev, res);
>>>> if (IS_ERR(mmio))
>>>> return PTR_ERR(mmio);
>>>> + ddata->phys_base = res->start;
>>>
>>> What do you use this for?
>>
>> This is used in in child driver (pwm) for capture data transfer by dma.
>
> Might be worth being clear about that.
>
> Perhaps pass in 'dma_base' (phys_base + offset) instead?
>

I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture
support. Are you talking about passing phys_base + TIM_DMAR ?

If this is the case, I'd prefer to keep phys base only if you don't
mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave
config is kept locally at one place.
Or do you mean something else ?

Maybe I can add a comment here about this ?
Something like:
/* phys_base to be used by child driver, e.g. DMA burst mode */

Please advise,
Thanks,
Fabrice



2018-01-23 16:42:41

by Lee Jones

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

On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
> On 01/23/2018 04:30 PM, Lee Jones wrote:
> > On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
> >
> >> On 01/23/2018 02:32 PM, Lee Jones wrote:
> >>> On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
> >>>> transfer data from/to device by using dma.
> >>>>
> >>>> Signed-off-by: Fabrice Gasnier <[email protected]>
> >>>> ---
> >>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
> >>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
> >>>> 2 files changed, 50 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> >>>> static int stm32_timers_probe(struct platform_device *pdev)
> >>>> {
> >>>> struct device *dev = &pdev->dev;
> >>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
> >>>> mmio = devm_ioremap_resource(dev, res);
> >>>> if (IS_ERR(mmio))
> >>>> return PTR_ERR(mmio);
> >>>> + ddata->phys_base = res->start;
> >>>
> >>> What do you use this for?
> >>
> >> This is used in in child driver (pwm) for capture data transfer by dma.
> >
> > Might be worth being clear about that.
> >
> > Perhaps pass in 'dma_base' (phys_base + offset) instead?
>
> I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture
> support. Are you talking about passing phys_base + TIM_DMAR ?

I have and I am.

> If this is the case, I'd prefer to keep phys base only if you don't
> mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave
> config is kept locally at one place.
> Or do you mean something else ?
>
> Maybe I can add a comment here about this ?
> Something like:
> /* phys_base to be used by child driver, e.g. DMA burst mode */

I haven't seen the memory map for this device, so it's not easy for me
to comment, but passing in the physical address of the parent MFD into
a child device doesn't quite sit right with me.

At what level does TIM_DMAR sit? Is it a child (PWM) specific
property, or is it described at parent (Timer) level?

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

2018-01-24 08:42:00

by Fabrice Gasnier

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

On 01/23/2018 05:41 PM, Lee Jones wrote:
> On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
>> On 01/23/2018 04:30 PM, Lee Jones wrote:
>>> On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
>>>
>>>> On 01/23/2018 02:32 PM, Lee Jones wrote:
>>>>> On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
>>>>>> transfer data from/to device by using dma.
>>>>>>
>>>>>> Signed-off-by: Fabrice Gasnier <[email protected]>
>>>>>> ---
>>>>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
>>>>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
>>>>>> 2 files changed, 50 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>>>>>> static int stm32_timers_probe(struct platform_device *pdev)
>>>>>> {
>>>>>> struct device *dev = &pdev->dev;
>>>>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
>>>>>> mmio = devm_ioremap_resource(dev, res);
>>>>>> if (IS_ERR(mmio))
>>>>>> return PTR_ERR(mmio);
>>>>>> + ddata->phys_base = res->start;
>>>>>
>>>>> What do you use this for?
>>>>
>>>> This is used in in child driver (pwm) for capture data transfer by dma.
>>>
>>> Might be worth being clear about that.
>>>
>>> Perhaps pass in 'dma_base' (phys_base + offset) instead?
>>
>> I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture
>> support. Are you talking about passing phys_base + TIM_DMAR ?
>
> I have and I am.
>
>> If this is the case, I'd prefer to keep phys base only if you don't
>> mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave
>> config is kept locally at one place.
>> Or do you mean something else ?
>>
>> Maybe I can add a comment here about this ?
>> Something like:
>> /* phys_base to be used by child driver, e.g. DMA burst mode */
>
> I haven't seen the memory map for this device, so it's not easy for me
> to comment, but passing in the physical address of the parent MFD into
> a child device doesn't quite sit right with me.
>
> At what level does TIM_DMAR sit? Is it a child (PWM) specific
> property, or is it described at parent (Timer) level?
>
Hi Lee,

This isn't child (PWM) specific. TIM_DMAR is described at timer level as
well as all timers DMA requests lines. Current patchset make it useful
for PWM capture. Basically, I think this can be seen as interrupts, as
each (0..7) dma request has an enable bit (in DIER: interrupt enable
register). This is similar as interrupts at timer level.

So, I understand your point regarding passing physical address of the
parent MFD... Speaking of interrupts, I'd probably have looked at
irq_chip. Regarding dma, i'm not sure what is preferred way ?

Another way maybe to export a routine (export symbol) from MFD core, to
handle dma transfer from there?
By looking into drivers/mfd, I found similar approach, e.g.
rtsx_pci_dma_transfer(). Do you think this is better approach ?

Please let me know your opinion.

Best Regards,
Fabrice

2018-01-24 14:58:32

by Lee Jones

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

On Wed, 24 Jan 2018, Fabrice Gasnier wrote:
> On 01/23/2018 05:41 PM, Lee Jones wrote:
> > On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
> >> On 01/23/2018 04:30 PM, Lee Jones wrote:
> >>> On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
> >>>
> >>>> On 01/23/2018 02:32 PM, Lee Jones wrote:
> >>>>> On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
> >>>>>> transfer data from/to device by using dma.
> >>>>>>
> >>>>>> Signed-off-by: Fabrice Gasnier <[email protected]>
> >>>>>> ---
> >>>>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
> >>>>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
> >>>>>> 2 files changed, 50 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> >>>>>> static int stm32_timers_probe(struct platform_device *pdev)
> >>>>>> {
> >>>>>> struct device *dev = &pdev->dev;
> >>>>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
> >>>>>> mmio = devm_ioremap_resource(dev, res);
> >>>>>> if (IS_ERR(mmio))
> >>>>>> return PTR_ERR(mmio);
> >>>>>> + ddata->phys_base = res->start;
> >>>>>
> >>>>> What do you use this for?
> >>>>
> >>>> This is used in in child driver (pwm) for capture data transfer by dma.
> >>>
> >>> Might be worth being clear about that.
> >>>
> >>> Perhaps pass in 'dma_base' (phys_base + offset) instead?
> >>
> >> I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture
> >> support. Are you talking about passing phys_base + TIM_DMAR ?
> >
> > I have and I am.
> >
> >> If this is the case, I'd prefer to keep phys base only if you don't
> >> mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave
> >> config is kept locally at one place.
> >> Or do you mean something else ?
> >>
> >> Maybe I can add a comment here about this ?
> >> Something like:
> >> /* phys_base to be used by child driver, e.g. DMA burst mode */
> >
> > I haven't seen the memory map for this device, so it's not easy for me
> > to comment, but passing in the physical address of the parent MFD into
> > a child device doesn't quite sit right with me.
> >
> > At what level does TIM_DMAR sit? Is it a child (PWM) specific
> > property, or is it described at parent (Timer) level?
> >
> Hi Lee,
>
> This isn't child (PWM) specific. TIM_DMAR is described at timer level as
> well as all timers DMA requests lines. Current patchset make it useful
> for PWM capture. Basically, I think this can be seen as interrupts, as
> each (0..7) dma request has an enable bit (in DIER: interrupt enable
> register). This is similar as interrupts at timer level.
>
> So, I understand your point regarding passing physical address of the
> parent MFD... Speaking of interrupts, I'd probably have looked at
> irq_chip. Regarding dma, i'm not sure what is preferred way ?
>
> Another way maybe to export a routine (export symbol) from MFD core, to
> handle dma transfer from there?
> By looking into drivers/mfd, I found similar approach, e.g.
> rtsx_pci_dma_transfer(). Do you think this is better approach ?
>
> Please let me know your opinion.

It sounds fine in principle. You are in a better position to make
that decision as you know the H/W more intimately than I, however it
does sound like a good idea to abstract the DMA handling from the
device if these aren't device-{level|specific} operations.

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

2018-01-24 15:32:17

by Fabrice Gasnier

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

On 01/24/2018 03:56 PM, Lee Jones wrote:
> On Wed, 24 Jan 2018, Fabrice Gasnier wrote:
>> On 01/23/2018 05:41 PM, Lee Jones wrote:
>>> On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
>>>> On 01/23/2018 04:30 PM, Lee Jones wrote:
>>>>> On Tue, 23 Jan 2018, Fabrice Gasnier wrote:
>>>>>
>>>>>> On 01/23/2018 02:32 PM, Lee Jones wrote:
>>>>>>> On Tue, 16 Jan 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, keep reference of device's bus address to allow child drivers to
>>>>>>>> transfer data from/to device by using dma.
>>>>>>>>
>>>>>>>> Signed-off-by: Fabrice Gasnier <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++-
>>>>>>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
>>>>>>>> 2 files changed, 50 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>>>>>>>> static int stm32_timers_probe(struct platform_device *pdev)
>>>>>>>> {
>>>>>>>> struct device *dev = &pdev->dev;
>>>>>>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
>>>>>>>> mmio = devm_ioremap_resource(dev, res);
>>>>>>>> if (IS_ERR(mmio))
>>>>>>>> return PTR_ERR(mmio);
>>>>>>>> + ddata->phys_base = res->start;
>>>>>>>
>>>>>>> What do you use this for?
>>>>>>
>>>>>> This is used in in child driver (pwm) for capture data transfer by dma.
>>>>>
>>>>> Might be worth being clear about that.
>>>>>
>>>>> Perhaps pass in 'dma_base' (phys_base + offset) instead?
>>>>
>>>> I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture
>>>> support. Are you talking about passing phys_base + TIM_DMAR ?
>>>
>>> I have and I am.
>>>
>>>> If this is the case, I'd prefer to keep phys base only if you don't
>>>> mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave
>>>> config is kept locally at one place.
>>>> Or do you mean something else ?
>>>>
>>>> Maybe I can add a comment here about this ?
>>>> Something like:
>>>> /* phys_base to be used by child driver, e.g. DMA burst mode */
>>>
>>> I haven't seen the memory map for this device, so it's not easy for me
>>> to comment, but passing in the physical address of the parent MFD into
>>> a child device doesn't quite sit right with me.
>>>
>>> At what level does TIM_DMAR sit? Is it a child (PWM) specific
>>> property, or is it described at parent (Timer) level?
>>>
>> Hi Lee,
>>
>> This isn't child (PWM) specific. TIM_DMAR is described at timer level as
>> well as all timers DMA requests lines. Current patchset make it useful
>> for PWM capture. Basically, I think this can be seen as interrupts, as
>> each (0..7) dma request has an enable bit (in DIER: interrupt enable
>> register). This is similar as interrupts at timer level.
>>
>> So, I understand your point regarding passing physical address of the
>> parent MFD... Speaking of interrupts, I'd probably have looked at
>> irq_chip. Regarding dma, i'm not sure what is preferred way ?
>>
>> Another way maybe to export a routine (export symbol) from MFD core, to
>> handle dma transfer from there?
>> By looking into drivers/mfd, I found similar approach, e.g.
>> rtsx_pci_dma_transfer(). Do you think this is better approach ?
>>
>> Please let me know your opinion.
>
> It sounds fine in principle. You are in a better position to make
> that decision as you know the H/W more intimately than I, however it
> does sound like a good idea to abstract the DMA handling from the
> device if these aren't device-{level|specific} operations.
>

Hi Lee,

Thanks, I'll rework this in v2 to follow your advice.

Best Regards,
Fabrice

2018-01-24 16:21:02

by cas

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

Apologies for interrupting any work/messages being undertaken. I am a newbie
here (1st post) and felt this was the only way to get this information to
Linus Torvalds.
____
The following was posted on a forum on the 22nd March 2017:

'I've been working at Intel for the last 15 years as an electrical engineer.
3 years ago I [got] bored with my job and wanted to move to a different
department. Intel is pretty good about letting employees move around within
the company. An opening came up on the Management Engine team, and since I
have experience with micro-controllers I figured I'd give it a shot. The
interview went great and they wanted me on board, but the final step was to
get securuty clearance.

I asked them WTF I need a security clearance for, and they said they
couldn't tell me unitl I was on the team. Thus began a 3 year trip down the
fucking rabbit hole.

I'm coming forward today because of the news that Trump was spied on. I know
exactly how it was done because I spent the last three years adding
backdoors into the ME. For those of you that are unaware, the ME is ona
separate CPU and cannot be disabled, and it exists at a level below the
operating system. Intel has been working with the intelligence community for
years to get backdoors into physical hardware so they don't have to worry
about finding OS exploits. If the hardware itself is compromised then the
whole machine is compromised.

----Next post----

The ME has full access to memory without the parent CPU having any
knowledge, it has full access to the TCP/IP stacl, and full access to every
peripheral connected to the computer. It also runs when the computer is
hibernating. It doesn't matter if you're using Windows, Linux, Mac OS X,
Whonix, Tails, Qubes OS, or Subgraph. If you have an i3, i5, or i7 hen
you're completely owned by the intelligence community.

We added similar finctionality to Samsung's WEEPING ANGEL, except the
intelligence community calls our project ODIN'S EYE. Through the ME we can
activate the mic and webcam even though the computer appears to be off or
sleeping.

Evidence of surveillance on Trump, his family, and key people in his
campaign will come out eventually. I know the surveillance happened for a
fact. Future leaks are coming, watch out for ODIN'S EYE.'

---END POST---

Jim Stone over at

82.221.129.208/.zj3.html

has the image of the text of the above (you need to scroll down a bit). He
also mentioned this stuff, and more, over 7 years ago, though not in the
same exact terms as the above, and states that this has actually occured
since the Centrino processor, which is where the first backdoors started to
be placed.

I am here to pass on the information to all of you, if you do not already
have it (and since I have just joined to explicitly to send this I have no
idea if any of you know this or not), and primarily to get this information
to Linus Torvalds who has been quoted by RT very recently as saying that the
patches announced by intel (for the Spectre and Meltdown bug) are 'garbage'.

I hope that the above information can now be used to understand the real
extent of the problem.

[Note: if one is not able to find the image of the text at the above site
you can find it here:

liferepairspecialist.com/Memes/

look for the image called 'inteladmission' - it is in this folder and not in
any subfolders.]