[4/8] mfd: stm32-timers: add support for dmas

Message ID 1516106631-18722-5-git-send-email-fabrice.gasnier@st.com
State Superseded
Headers show
Series
  • Add support for PWM input capture on STM32
Related show

Commit Message

Fabrice Gasnier Jan. 16, 2018, 12:43 p.m.
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 <fabrice.gasnier@st.com>
---
 drivers/mfd/stm32-timers.c       | 37 ++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/stm32-timers.h | 14 ++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

Comments

Lee Jones Jan. 23, 2018, 1:32 p.m. | #1
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 <fabrice.gasnier@st.com>
> ---
>  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
Fabrice Gasnier Jan. 23, 2018, 1:57 p.m. | #2
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 <fabrice.gasnier@st.com>
>> ---
>>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Jan. 23, 2018, 3:30 p.m. | #3
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 <fabrice.gasnier@st.com>
> >> ---
> >>  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?
Fabrice Gasnier Jan. 23, 2018, 3:52 p.m. | #4
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 <fabrice.gasnier@st.com>
>>>> ---
>>>>  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


--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Jan. 23, 2018, 4:41 p.m. | #5
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 <fabrice.gasnier@st.com>
> >>>> ---
> >>>>  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?
Fabrice Gasnier Jan. 24, 2018, 8:40 a.m. | #6
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 <fabrice.gasnier@st.com>
>>>>>> ---
>>>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Jan. 24, 2018, 2:56 p.m. | #7
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 <fabrice.gasnier@st.com>
> >>>>>> ---
> >>>>>>  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.
Fabrice Gasnier Jan. 24, 2018, 3:30 p.m. | #8
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 <fabrice.gasnier@st.com>
>>>>>>>> ---
>>>>>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
cas Jan. 24, 2018, 3:43 p.m. | #9
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.

Patch

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