Message-ID: <106ff518-4973-5d4a-b5bf-dd58cbd8439a@st.com>
Date:   Wed, 28 Mar 2018 18:41:11 +0200
From:   Fabrice Gasnier <fabrice.gasnier@...com>
To:     Lee Jones <lee.jones@...aro.org>
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: [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 <fabrice.gasnier@...com>
>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.org>
>> ---
>> Changes in v2:
>> - Abstract DMA handling from child driver: move it to MFD core
>> - Add comments on optional dma support
>> ---
>>  drivers/mfd/stm32-timers.c       | 215 ++++++++++++++++++++++++++++++++++++++-
>>  include/linux/mfd/stm32-timers.h |  27 +++++
>>  2 files changed, 238 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>> index a6675a4..2cdad2c 100644
>> --- a/drivers/mfd/stm32-timers.c
>> +++ b/drivers/mfd/stm32-timers.c
>> @@ -6,16 +6,166 @@
>>   * License terms:  GNU General Public License (GPL), version 2
>>   */
>>  
>> +#include <linux/bitfield.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/dma-mapping.h>
>>  #include <linux/mfd/stm32-timers.h>
>>  #include <linux/module.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/reset.h>
>>  
>> +#define STM32_TIMERS_MAX_REGISTERS	0x3fc
>> +
>> +struct stm32_timers_priv {
>> +	struct device *dev;
>> +	struct completion completion;
>> +	phys_addr_t phys_base;		/* timers physical addr for dma */
>> +	struct mutex lock;		/* protect dma access */
>> +	struct dma_chan *dma_chan;	/* dma channel in use */
> 
> I think kerneldoc would be better than inline comments.

Hi Lee,

Okay, I can update it with kerneldoc instead.

> 
>> +	struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>> +	struct stm32_timers ddata;
> 
> This looks odd to me.  Why can't you expand the current ddata
> structure?  Wouldn't it be better to create a stm32_timers_dma
> structure to place all this information in (except *dev, that should
> live in the ddata struct), then place a reference in the existing
> stm32_timers struct?

Maybe I miss-understand you here, from what we discussed in V1:
https://lkml.org/lkml/2018/1/23/574
>... "passing in the physical address of the parent MFD into
> a child device doesn't quite sit right with me"
I introduced this private struct in MFD parent, and completely hide it
from the child.

So, do you suggest to add struct definition here ? But make it part of
struct stm32_timers *ddata?

And only put declaration in include/linux/mfd/stm32-timers.h:
+ struct stm32_timers_dma;

struct stm32_timers {
	struct clk *clk;
	struct regmap *regmap;
	u32 max_arr;
+	struct stm32_timers_dma;
};

I can probably spare the *dev then... use dev->parent in child driver.

Can you just confirm this please?

> 
>> +};
>> +
>> +static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d)
>> +{
>> +	return container_of(d, struct stm32_timers_priv, ddata);
>> +}
> 
> If you take my other suggestions, you can remove this function.
> 
>> +/* DIER register DMA enable bits */
>> +static const u32 stm32_timers_dier_dmaen[STM32_TIMERS_MAX_DMAS] = {
>> +	TIM_DIER_CC1DE, TIM_DIER_CC2DE, TIM_DIER_CC3DE, TIM_DIER_CC4DE,
>> +	TIM_DIER_UIE, TIM_DIER_TDE, TIM_DIER_COMDE
>> +};
> 
> Maybe one per line would be kinder on the eye?

Ok, I'll update this.

> 
>> +static void stm32_timers_dma_done(void *p)
>> +{
>> +	struct stm32_timers_priv *priv = p;
>> +	struct dma_chan *dma_chan = priv->dma_chan;
>> +	struct dma_tx_state state;
>> +	enum dma_status status;
>> +
>> +	status = dmaengine_tx_status(dma_chan, dma_chan->cookie, &state);
>> +	if (status == DMA_COMPLETE)
>> +		complete(&priv->completion);
>> +}
>> +
>> +/**
>> + * stm32_timers_dma_burst_read - Read from timers registers using DMA.
>> + *
>> + * Read from STM32 timers registers using DMA on a single event.
>> + * @ddata: reference to stm32_timers
> 
> It's odd to pass device data back like this.
> 
> Better to pass a pointer to 'struct device', then use the normal
> helpers.

You're right, I'll update this.

> 
>> + * @buf: dma'able destination buffer
>> + * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
>> + * @reg: registers start offset for DMA to read from (like CCRx for capture)
>> + * @num_reg: number of registers to read upon each dma request, starting @reg.
>> + * @bursts: number of bursts to read (e.g. like two for pwm period capture)
>> + * @tmo_ms: timeout (milliseconds)
>> + */
>> +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf,
>> +				enum stm32_timers_dmas id, u32 reg,
>> +				unsigned int num_reg, unsigned int bursts,
>> +				unsigned long tmo_ms)
>> +{
>> +	struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>> +	unsigned long timeout = msecs_to_jiffies(tmo_ms);
>> +	struct regmap *regmap = priv->ddata.regmap;
>> +	size_t len = num_reg * bursts * sizeof(u32);
>> +	struct dma_async_tx_descriptor *desc;
>> +	struct dma_slave_config config;
>> +	dma_cookie_t cookie;
>> +	dma_addr_t dma_buf;
>> +	u32 dbl, dba;
>> +	long err;
>> +	int ret;
>> +
>> +	/* sanity check */
>> +	if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
>> +		return -EINVAL;
>> +
>> +	if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
>> +	    (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
>> +		return -EINVAL;
>> +
>> +	if (!priv->dmas[id])
>> +		return -ENODEV;
>> +	mutex_lock(&priv->lock);
>> +	priv->dma_chan = priv->dmas[id];
>> +
>> +	dma_buf = dma_map_single(priv->dev, buf, len, DMA_FROM_DEVICE);
>> +	ret = dma_mapping_error(priv->dev, dma_buf);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	/* Prepare DMA read from timer registers, using DMA burst mode */
>> +	memset(&config, 0, sizeof(config));
>> +	config.src_addr = (dma_addr_t)priv->phys_base + TIM_DMAR;
>> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +	ret = dmaengine_slave_config(priv->dma_chan, &config);
>> +	if (ret)
>> +		goto unmap;
>> +
>> +	desc = dmaengine_prep_slave_single(priv->dma_chan, dma_buf, len,
>> +					   DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
>> +	if (!desc) {
>> +		ret = -EBUSY;
>> +		goto unmap;
>> +	}
>> +
>> +	desc->callback = stm32_timers_dma_done;
>> +	desc->callback_param = priv;
>> +	cookie = dmaengine_submit(desc);
>> +	ret = dma_submit_error(cookie);
>> +	if (ret)
>> +		goto dma_term;
>> +
>> +	reinit_completion(&priv->completion);
>> +	dma_async_issue_pending(priv->dma_chan);
>> +
>> +	/* Setup and enable timer DMA burst mode */
>> +	dbl = FIELD_PREP(TIM_DCR_DBL, bursts - 1);
>> +	dba = FIELD_PREP(TIM_DCR_DBA, reg >> 2);
>> +	ret = regmap_write(regmap, TIM_DCR, dbl | dba);
>> +	if (ret)
>> +		goto dma_term;
>> +
>> +	/* Clear pending flags before enabling DMA request */
>> +	ret = regmap_write(regmap, TIM_SR, 0);
>> +	if (ret)
>> +		goto dcr_clr;
>> +
>> +	ret = regmap_update_bits(regmap, TIM_DIER, stm32_timers_dier_dmaen[id],
>> +				 stm32_timers_dier_dmaen[id]);
>> +	if (ret)
>> +		goto dcr_clr;
>> +
>> +	err = wait_for_completion_interruptible_timeout(&priv->completion,
>> +							timeout);
>> +	if (err == 0)
>> +		ret = -ETIMEDOUT;
>> +	else if (err < 0)
>> +		ret = err;
>> +
>> +	regmap_update_bits(regmap, TIM_DIER, stm32_timers_dier_dmaen[id], 0);
>> +	regmap_write(regmap, TIM_SR, 0);
>> +dcr_clr:
>> +	regmap_write(regmap, TIM_DCR, 0);
>> +dma_term:
>> +	dmaengine_terminate_all(priv->dma_chan);
>> +unmap:
>> +	dma_unmap_single(priv->dev, dma_buf, len, DMA_FROM_DEVICE);
>> +unlock:
>> +	priv->dma_chan = NULL;
>> +	mutex_unlock(&priv->lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(stm32_timers_dma_burst_read);
>> +
>>  static const struct regmap_config stm32_timers_regmap_cfg = {
>>  	.reg_bits = 32,
>>  	.val_bits = 32,
>>  	.reg_stride = sizeof(u32),
>> -	.max_register = 0x3fc,
>> +	.max_register = STM32_TIMERS_MAX_REGISTERS,
>>  };
>>  
>>  static void stm32_timers_get_arr_size(struct stm32_timers *ddata)
>> @@ -29,21 +179,55 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata)
>>  	regmap_write(ddata->regmap, TIM_ARR, 0x0);
>>  }
>>  
>> +static void stm32_timers_dma_probe(struct device *dev,
>> +				   struct stm32_timers_priv *priv)
>> +{
>> +	int i;
>> +	char name[4];
>> +	struct dma_chan **dmas = priv->dmas;
>> +
>> +	/* Optional DMA support: get valid dma channel(s) or NULL */
>> +	for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) {
>> +		snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1);
>> +		dmas[i] = dma_request_slave_channel(dev, name);
>> +	}
>> +	dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up");
>> +	dmas[STM32_TIMERS_DMA_TRIG] = dma_request_slave_channel(dev, "trig");
>> +	dmas[STM32_TIMERS_DMA_COM] = dma_request_slave_channel(dev, "com");
>> +}
>> +
>> +static void stm32_timers_dma_remove(struct device *dev,
>> +				    struct stm32_timers_priv *priv)
>> +{
>> +	int i;
>> +
>> +	for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++)
>> +		if (priv->dmas[i])
>> +			dma_release_channel(priv->dmas[i]);
>> +}
>> +
>>  static int stm32_timers_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> +	struct stm32_timers_priv *priv;
>>  	struct stm32_timers *ddata;
>>  	struct resource *res;
>>  	void __iomem *mmio;
>> +	int ret;
>>  
>> -	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
>> -	if (!ddata)
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>>  		return -ENOMEM;
>> +	ddata = &priv->ddata;
>> +	init_completion(&priv->completion);
>> +	mutex_init(&priv->lock);
>> +	priv->dev = dev;
>>  
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	mmio = devm_ioremap_resource(dev, res);
>>  	if (IS_ERR(mmio))
>>  		return PTR_ERR(mmio);
>> +	priv->phys_base = res->start;
>>  
>>  	ddata->regmap = devm_regmap_init_mmio_clk(dev, "int", mmio,
>>  						  &stm32_timers_regmap_cfg);
>> @@ -56,9 +240,31 @@ static int stm32_timers_probe(struct platform_device *pdev)
>>  
>>  	stm32_timers_get_arr_size(ddata);
>>  
>> +	stm32_timers_dma_probe(dev, priv);
>> +
>>  	platform_set_drvdata(pdev, ddata);
>>  
>> -	return devm_of_platform_populate(&pdev->dev);
>> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>> +	if (ret)
>> +		goto dma_remove;
>> +
>> +	return 0;
>> +
>> +dma_remove:
>> +	stm32_timers_dma_remove(dev, priv);
> 
> You can easily remove this label and the goto.

I'll update this

> 
>> +	return ret;
>> +}
>> +
>> +static int stm32_timers_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32_timers *ddata = platform_get_drvdata(pdev);
>> +	struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>> +
>> +	of_platform_depopulate(&pdev->dev);
> 
> Why can't you continue using devm_*?

I can use devm_of_platform_depopulate() here if you prefer, and keep
devm_of_platform_populate() in probe.

Please let me know

Thanks for reviewing,
Best Regards,
Fabrice
> 
>> +	stm32_timers_dma_remove(&pdev->dev, priv);
>> +
>> +	return 0;
>>  }
>>  
>>  static const struct of_device_id stm32_timers_of_match[] = {
>> @@ -69,6 +275,7 @@ static int stm32_timers_probe(struct platform_device *pdev)
>>  
>>  static struct platform_driver stm32_timers_driver = {
>>  	.probe = stm32_timers_probe,
>> +	.remove = stm32_timers_remove,
>>  	.driver	= {
>>  		.name = "stm32-timers",
>>  		.of_match_table = stm32_timers_of_match,
>> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
>> index ce7346e..5fd2d6b 100644
>> --- a/include/linux/mfd/stm32-timers.h
>> +++ b/include/linux/mfd/stm32-timers.h
>> @@ -29,6 +29,8 @@
>>  #define TIM_CCR3	0x3C	/* Capt/Comp Register 3    */
>>  #define TIM_CCR4	0x40	/* Capt/Comp Register 4    */
>>  #define TIM_BDTR	0x44	/* Break and Dead-Time Reg */
>> +#define TIM_DCR		0x48	/* DMA control register    */
>> +#define TIM_DMAR	0x4C	/* DMA register for transfer */
>>  
>>  #define TIM_CR1_CEN	BIT(0)	/* Counter Enable	   */
>>  #define TIM_CR1_DIR	BIT(4)  /* Counter Direction	   */
>> @@ -38,6 +40,13 @@
>>  #define TIM_SMCR_SMS	(BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */
>>  #define TIM_SMCR_TS	(BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */
>>  #define TIM_DIER_UIE	BIT(0)	/* Update interrupt	   */
>> +#define TIM_DIER_UDE	BIT(8)  /* Update DMA request Enable */
>> +#define TIM_DIER_CC1DE	BIT(9)  /* CC1 DMA request Enable  */
>> +#define TIM_DIER_CC2DE	BIT(10) /* CC2 DMA request Enable  */
>> +#define TIM_DIER_CC3DE	BIT(11) /* CC3 DMA request Enable  */
>> +#define TIM_DIER_CC4DE	BIT(12) /* CC4 DMA request Enable  */
>> +#define TIM_DIER_COMDE	BIT(13) /* COM DMA request Enable  */
>> +#define TIM_DIER_TDE	BIT(14) /* Trigger DMA request Enable */
>>  #define TIM_SR_UIF	BIT(0)	/* Update interrupt flag   */
>>  #define TIM_EGR_UG	BIT(0)	/* Update Generation       */
>>  #define TIM_CCMR_PE	BIT(3)	/* Channel Preload Enable  */
>> @@ -58,6 +67,8 @@
>>  #define TIM_BDTR_BK2F	(BIT(20) | BIT(21) | BIT(22) | BIT(23))
>>  #define TIM_BDTR_BK2E	BIT(24) /* Break 2 input enable	   */
>>  #define TIM_BDTR_BK2P	BIT(25) /* Break 2 input polarity  */
>> +#define TIM_DCR_DBA	GENMASK(4, 0)	/* DMA base addr */
>> +#define TIM_DCR_DBL	GENMASK(12, 8)	/* DMA burst len */
>>  
>>  #define MAX_TIM_PSC		0xFFFF
>>  #define TIM_CR2_MMS_SHIFT	4
>> @@ -67,9 +78,25 @@
>>  #define TIM_BDTR_BKF_SHIFT	16
>>  #define TIM_BDTR_BK2F_SHIFT	20
>>  
>> +enum stm32_timers_dmas {
>> +	STM32_TIMERS_DMA_CH1,
>> +	STM32_TIMERS_DMA_CH2,
>> +	STM32_TIMERS_DMA_CH3,
>> +	STM32_TIMERS_DMA_CH4,
>> +	STM32_TIMERS_DMA_UP,
>> +	STM32_TIMERS_DMA_TRIG,
>> +	STM32_TIMERS_DMA_COM,
>> +	STM32_TIMERS_MAX_DMAS,
>> +};
>> +
>>  struct stm32_timers {
>>  	struct clk *clk;
>>  	struct regmap *regmap;
>>  	u32 max_arr;
>>  };
>> +
>> +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf,
>> +				enum stm32_timers_dmas id, u32 reg,
>> +				unsigned int num_reg, unsigned int bursts,
>> +				unsigned long tmo_ms);
>>  #endif
> 

Powered by blists - more mailing lists