| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180416122217.u7h3wawpdx3kj4wd@dell>
Date: Mon, 16 Apr 2018 13:22:17 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Fabrice Gasnier <fabrice.gasnier@...com>
Cc: thierry.reding@...il.com, alexandre.torgue@...com,
benjamin.gaignard@...aro.org, robh+dt@...nel.org,
mark.rutland@....com, linux@...linux.org.uk,
mcoquelin.stm32@...il.com, benjamin.gaignard@...com,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 2/6] mfd: stm32-timers: add support for dmas
On Fri, 30 Mar 2018, Fabrice Gasnier wrote:
> STM32 Timers can support up to 7 DMA requests:
> - 4 channels, update, compare and trigger.
> Optionally request part, or all DMAs from stm32-timers MFD core.
>
> Also add routine to implement burst reads using DMA from timer registers.
> This is exported. So, it can be used by child drivers, PWM capture
> for instance (but not limited to).
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.org>
> ---
> Changes in v3:
> - Basically Lee's comments:
> - rather create a struct stm32_timers_dma, and place a reference to it
> in existing ddata (instead of adding priv struct).
> - rather use a struct device in exported routine prototype, and use
> standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
> - simplify error handling in probe (remove a goto)
> - comment on devm_of_platform_*populate() usage.
>
> Changes in v2:
> - Abstract DMA handling from child driver: move it to MFD core
> - Add comments on optional dma support
> ---
> drivers/mfd/stm32-timers.c | 219 ++++++++++++++++++++++++++++++++++++++-
> include/linux/mfd/stm32-timers.h | 32 ++++++
> 2 files changed, 249 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> index 1d347e5..98191ec 100644
> --- a/drivers/mfd/stm32-timers.c
> +++ b/drivers/mfd/stm32-timers.c
> @@ -4,16 +4,165 @@
> * Author: Benjamin Gaignard <benjamin.gaignard@...com>
> */
>
> +#include <linux/bitfield.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> #include <linux/mfd/stm32-timers.h>
> #include <linux/module.h>
> #include <linux/of_platform.h>
> #include <linux/reset.h>
>
> +#define STM32_TIMERS_MAX_REGISTERS 0x3fc
> +
> +struct stm32_timers_dma {
> + struct completion completion;
> + phys_addr_t phys_base;
> + struct mutex lock; /* protect dma access */
Nit: I like comments to use good grammar i.e. capital letters to
start a sentence and 's/dma/DMA/'. Or better still, drop the comment,
we know what the lock is for.
> + struct dma_chan *chan;
> + struct dma_chan *chans[STM32_TIMERS_MAX_DMAS];
This requires explanation. Maybe a kerneldoc header would be good here.
[...]
> +/**
> + * stm32_timers_dma_burst_read - Read from timers registers using DMA.
> + *
> + * Read from STM32 timers registers using DMA on a single event.
> + * @dev: reference to stm32_timers MFD device
> + * @buf: dma'able destination buffer
> + * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
> + * @reg: registers start offset for DMA to read from (like CCRx for capture)
> + * @num_reg: number of registers to read upon each dma request, starting @reg.
> + * @bursts: number of bursts to read (e.g. like two for pwm period capture)
> + * @tmo_ms: timeout (milliseconds)
> + */
> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
> + enum stm32_timers_dmas id, u32 reg,
> + unsigned int num_reg, unsigned int bursts,
> + unsigned long tmo_ms)
> +{
> + struct stm32_timers *ddata = dev_get_drvdata(dev);
> + unsigned long timeout = msecs_to_jiffies(tmo_ms);
> + struct regmap *regmap = ddata->regmap;
> + struct stm32_timers_dma *dma = ddata->dma;
> + size_t len = num_reg * bursts * sizeof(u32);
> + struct dma_async_tx_descriptor *desc;
> + struct dma_slave_config config;
> + dma_cookie_t cookie;
> + dma_addr_t dma_buf;
> + u32 dbl, dba;
> + long err;
> + int ret;
> +
> + /* sanity check */
Proper grammar in all comments please.
"Sanity check"
[...]
> + /* select dma channel in use */
Here too.
Etc, etc, etc ...
> + dma->chan = dma->chans[id];
> + dma_buf = dma_map_single(dev, buf, len, DMA_FROM_DEVICE);
> + ret = dma_mapping_error(dev, dma_buf);
> + if (ret)
> + goto unlock;
> +
> + /* Prepare DMA read from timer registers, using DMA burst mode */
This is good.
[...]
[...]
> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> index 2aadab6..585a4de 100644
> --- a/include/linux/mfd/stm32-timers.h
> +++ b/include/linux/mfd/stm32-timers.h
> @@ -8,6 +8,8 @@
> #define _LINUX_STM32_GPTIMER_H_
>
> #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> #include <linux/regmap.h>
[...]
> +enum stm32_timers_dmas {
> + STM32_TIMERS_DMA_CH1,
> + STM32_TIMERS_DMA_CH2,
> + STM32_TIMERS_DMA_CH3,
> + STM32_TIMERS_DMA_CH4,
> + STM32_TIMERS_DMA_UP,
> + STM32_TIMERS_DMA_TRIG,
> + STM32_TIMERS_DMA_COM,
> + STM32_TIMERS_MAX_DMAS,
> +};
> +
> +struct stm32_timers_dma;
Why don't you move the declaration into here?
Then you don't need to forward declare.
> struct stm32_timers {
> struct clk *clk;
> struct regmap *regmap;
> u32 max_arr;
> + struct stm32_timers_dma *dma;
> };
> +
> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
> + enum stm32_timers_dmas id, u32 reg,
> + unsigned int num_reg, unsigned int bursts,
> + unsigned long tmo_ms);
> #endif
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists