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

2018-03-29 Thread Fabrice Gasnier 
On 03/29/2018 04:31 PM, Lee Jones wrote:
> On Thu, 29 Mar 2018, Fabrice Gasnier wrote:
> 
>> On 03/29/2018 02:59 PM, Lee Jones wrote:
>>> On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
>>>
 On 03/28/2018 05:22 PM, Lee Jones wrote:
> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
>
>> STM32 Timers can support up to 7 DMA requests:
>> - 4 channels, update, compare and trigger.
>> Optionally request part, or all DMAs from stm32-timers MFD core.
>>
>> Also add routine to implement burst reads using DMA from timer registers.
>> This is exported. So, it can be used by child drivers, PWM capture
>> for instance (but not limited to).
>>
>> Signed-off-by: Fabrice Gasnier 
>> Reviewed-by: Benjamin Gaignard 
>> ---
>> Changes in v2:
>> - Abstract DMA handling from child driver: move it to MFD core
>> - Add comments on optional dma support
>> ---
>>  drivers/mfd/stm32-timers.c   | 215 
>> ++-
>>  include/linux/mfd/stm32-timers.h |  27 +
>>  2 files changed, 238 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>> index a6675a4..2cdad2c 100644
>> --- a/drivers/mfd/stm32-timers.c
>> +++ b/drivers/mfd/stm32-timers.c
>>>
>>> [...]
>>>
>> +struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>> +struct stm32_timers ddata;
>
> This looks odd to me.  Why can't you expand the current ddata
> structure?  Wouldn't it be better to create a stm32_timers_dma
> structure to place all this information in (except *dev, that should
> live in the ddata struct), then place a reference in the existing
> stm32_timers struct?

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

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

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

 struct stm32_timers {
struct clk *clk;
struct regmap *regmap;
u32 max_arr;
 +  struct stm32_timers_dma;
 };
>>>
>>> Yes, that's the basic idea.
>>>
 I can probably spare the *dev then... use dev->parent in child driver.
>>>
>>> What would you use dev->parent for?
>>
>> Hi Lee,
>>
>> This is to follow your sugestion to use *dev instead of *ddata when
>> calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
>> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
>> Then there is no need to keep *dev inside ddata struct.
> 
> I'm wondering if it would be neater to us the child's *dev, then do
> the ->parent deference in the parent MFD (with a comment to say what
> you're doing of course).
> 

There's already dev.parent dereference in child drivers, for same
purpose: dev_get_drvdata(pdev->dev.parent). So, I guess same can be done
here ?

Thanks for you review,
I'll update all this and send a v3.

Best regards,
Fabrice

>>> [...]
>>>
>> +static int stm32_timers_remove(struct platform_device *pdev)
>> +{
>> +struct stm32_timers *ddata = platform_get_drvdata(pdev);
>> +struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>> +
>> +of_platform_depopulate(>dev);
>
> Why can't you continue using devm_*?

 I can use devm_of_platform_depopulate() here if you prefer, and keep
 devm_of_platform_populate() in probe.
>>>
>>> The point of devm_* is that you don't have to call depopulate.
>>>
>>> It happens automatically once this driver is unbound.
>>
>> Ok, so to clarify, keeping devm_ here may be a bit racy:
>> of_platform_depopulate will happen after dma has been released (there is
>> no devm_ variant to release dma).
>> Only way to prevent race condition here, is to enforce
>> of_platform_depopulate() is called before dma release (e.g. in reverse
>> order compared to probe).
>>
>> Do you wish I add a comment about it ?
> 
> Best thing to do then is keep the non-devm variant and provide a
> comment as to why is it not possible to use devm_*.
>

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

2018-03-29 Thread Fabrice Gasnier 
On 03/29/2018 04:31 PM, Lee Jones wrote:
> On Thu, 29 Mar 2018, Fabrice Gasnier wrote:
> 
>> On 03/29/2018 02:59 PM, Lee Jones wrote:
>>> On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
>>>
 On 03/28/2018 05:22 PM, Lee Jones wrote:
> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
>
>> STM32 Timers can support up to 7 DMA requests:
>> - 4 channels, update, compare and trigger.
>> Optionally request part, or all DMAs from stm32-timers MFD core.
>>
>> Also add routine to implement burst reads using DMA from timer registers.
>> This is exported. So, it can be used by child drivers, PWM capture
>> for instance (but not limited to).
>>
>> Signed-off-by: Fabrice Gasnier 
>> Reviewed-by: Benjamin Gaignard 
>> ---
>> Changes in v2:
>> - Abstract DMA handling from child driver: move it to MFD core
>> - Add comments on optional dma support
>> ---
>>  drivers/mfd/stm32-timers.c   | 215 
>> ++-
>>  include/linux/mfd/stm32-timers.h |  27 +
>>  2 files changed, 238 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>> index a6675a4..2cdad2c 100644
>> --- a/drivers/mfd/stm32-timers.c
>> +++ b/drivers/mfd/stm32-timers.c
>>>
>>> [...]
>>>
>> +struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>> +struct stm32_timers ddata;
>
> This looks odd to me.  Why can't you expand the current ddata
> structure?  Wouldn't it be better to create a stm32_timers_dma
> structure to place all this information in (except *dev, that should
> live in the ddata struct), then place a reference in the existing
> stm32_timers struct?

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

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

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

 struct stm32_timers {
struct clk *clk;
struct regmap *regmap;
u32 max_arr;
 +  struct stm32_timers_dma;
 };
>>>
>>> Yes, that's the basic idea.
>>>
 I can probably spare the *dev then... use dev->parent in child driver.
>>>
>>> What would you use dev->parent for?
>>
>> Hi Lee,
>>
>> This is to follow your sugestion to use *dev instead of *ddata when
>> calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
>> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
>> Then there is no need to keep *dev inside ddata struct.
> 
> I'm wondering if it would be neater to us the child's *dev, then do
> the ->parent deference in the parent MFD (with a comment to say what
> you're doing of course).
> 

There's already dev.parent dereference in child drivers, for same
purpose: dev_get_drvdata(pdev->dev.parent). So, I guess same can be done
here ?

Thanks for you review,
I'll update all this and send a v3.

Best regards,
Fabrice

>>> [...]
>>>
>> +static int stm32_timers_remove(struct platform_device *pdev)
>> +{
>> +struct stm32_timers *ddata = platform_get_drvdata(pdev);
>> +struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>> +
>> +of_platform_depopulate(>dev);
>
> Why can't you continue using devm_*?

 I can use devm_of_platform_depopulate() here if you prefer, and keep
 devm_of_platform_populate() in probe.
>>>
>>> The point of devm_* is that you don't have to call depopulate.
>>>
>>> It happens automatically once this driver is unbound.
>>
>> Ok, so to clarify, keeping devm_ here may be a bit racy:
>> of_platform_depopulate will happen after dma has been released (there is
>> no devm_ variant to release dma).
>> Only way to prevent race condition here, is to enforce
>> of_platform_depopulate() is called before dma release (e.g. in reverse
>> order compared to probe).
>>
>> Do you wish I add a comment about it ?
> 
> Best thing to do then is keep the non-devm variant and provide a
> comment as to why is it not possible to use devm_*.
>

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

2018-03-29 Thread Lee Jones 
On Thu, 29 Mar 2018, Fabrice Gasnier wrote:

> On 03/29/2018 02:59 PM, Lee Jones wrote:
> > On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
> > 
> >> On 03/28/2018 05:22 PM, Lee Jones wrote:
> >>> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
> >>>
>  STM32 Timers can support up to 7 DMA requests:
>  - 4 channels, update, compare and trigger.
>  Optionally request part, or all DMAs from stm32-timers MFD core.
> 
>  Also add routine to implement burst reads using DMA from timer registers.
>  This is exported. So, it can be used by child drivers, PWM capture
>  for instance (but not limited to).
> 
>  Signed-off-by: Fabrice Gasnier 
>  Reviewed-by: Benjamin Gaignard 
>  ---
>  Changes in v2:
>  - Abstract DMA handling from child driver: move it to MFD core
>  - Add comments on optional dma support
>  ---
>   drivers/mfd/stm32-timers.c   | 215 
>  ++-
>   include/linux/mfd/stm32-timers.h |  27 +
>   2 files changed, 238 insertions(+), 4 deletions(-)
> 
>  diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>  index a6675a4..2cdad2c 100644
>  --- a/drivers/mfd/stm32-timers.c
>  +++ b/drivers/mfd/stm32-timers.c
> > 
> > [...]
> > 
>  +struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>  +struct stm32_timers ddata;
> >>>
> >>> This looks odd to me.  Why can't you expand the current ddata
> >>> structure?  Wouldn't it be better to create a stm32_timers_dma
> >>> structure to place all this information in (except *dev, that should
> >>> live in the ddata struct), then place a reference in the existing
> >>> stm32_timers struct?
> >>
> >> Maybe I miss-understand you here, from what we discussed in V1:
> >> https://lkml.org/lkml/2018/1/23/574
> >>> ... "passing in the physical address of the parent MFD into
> >>> a child device doesn't quite sit right with me"
> >> I introduced this private struct in MFD parent, and completely hide it
> >> from the child.
> >>
> >> So, do you suggest to add struct definition here ? But make it part of
> >> struct stm32_timers *ddata?
> >>
> >> And only put declaration in include/linux/mfd/stm32-timers.h:
> >> + struct stm32_timers_dma;
> >>
> >> struct stm32_timers {
> >>struct clk *clk;
> >>struct regmap *regmap;
> >>u32 max_arr;
> >> +  struct stm32_timers_dma;
> >> };
> > 
> > Yes, that's the basic idea.
> > 
> >> I can probably spare the *dev then... use dev->parent in child driver.
> > 
> > What would you use dev->parent for?
> 
> Hi Lee,
> 
> This is to follow your sugestion to use *dev instead of *ddata when
> calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
> Then there is no need to keep *dev inside ddata struct.

I'm wondering if it would be neater to us the child's *dev, then do
the ->parent deference in the parent MFD (with a comment to say what
you're doing of course).

> > [...]
> > 
>  +static int stm32_timers_remove(struct platform_device *pdev)
>  +{
>  +struct stm32_timers *ddata = platform_get_drvdata(pdev);
>  +struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>  +
>  +of_platform_depopulate(>dev);
> >>>
> >>> Why can't you continue using devm_*?
> >>
> >> I can use devm_of_platform_depopulate() here if you prefer, and keep
> >> devm_of_platform_populate() in probe.
> > 
> > The point of devm_* is that you don't have to call depopulate.
> > 
> > It happens automatically once this driver is unbound.
> 
> Ok, so to clarify, keeping devm_ here may be a bit racy:
> of_platform_depopulate will happen after dma has been released (there is
> no devm_ variant to release dma).
> Only way to prevent race condition here, is to enforce
> of_platform_depopulate() is called before dma release (e.g. in reverse
> order compared to probe).
> 
> Do you wish I add a comment about it ?

Best thing to do then is keep the non-devm variant and provide a
comment as to why is it not possible to use devm_*.

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

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

2018-03-29 Thread Lee Jones 
On Thu, 29 Mar 2018, Fabrice Gasnier wrote:

> On 03/29/2018 02:59 PM, Lee Jones wrote:
> > On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
> > 
> >> On 03/28/2018 05:22 PM, Lee Jones wrote:
> >>> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
> >>>
>  STM32 Timers can support up to 7 DMA requests:
>  - 4 channels, update, compare and trigger.
>  Optionally request part, or all DMAs from stm32-timers MFD core.
> 
>  Also add routine to implement burst reads using DMA from timer registers.
>  This is exported. So, it can be used by child drivers, PWM capture
>  for instance (but not limited to).
> 
>  Signed-off-by: Fabrice Gasnier 
>  Reviewed-by: Benjamin Gaignard 
>  ---
>  Changes in v2:
>  - Abstract DMA handling from child driver: move it to MFD core
>  - Add comments on optional dma support
>  ---
>   drivers/mfd/stm32-timers.c   | 215 
>  ++-
>   include/linux/mfd/stm32-timers.h |  27 +
>   2 files changed, 238 insertions(+), 4 deletions(-)
> 
>  diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>  index a6675a4..2cdad2c 100644
>  --- a/drivers/mfd/stm32-timers.c
>  +++ b/drivers/mfd/stm32-timers.c
> > 
> > [...]
> > 
>  +struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
>  +struct stm32_timers ddata;
> >>>
> >>> This looks odd to me.  Why can't you expand the current ddata
> >>> structure?  Wouldn't it be better to create a stm32_timers_dma
> >>> structure to place all this information in (except *dev, that should
> >>> live in the ddata struct), then place a reference in the existing
> >>> stm32_timers struct?
> >>
> >> Maybe I miss-understand you here, from what we discussed in V1:
> >> https://lkml.org/lkml/2018/1/23/574
> >>> ... "passing in the physical address of the parent MFD into
> >>> a child device doesn't quite sit right with me"
> >> I introduced this private struct in MFD parent, and completely hide it
> >> from the child.
> >>
> >> So, do you suggest to add struct definition here ? But make it part of
> >> struct stm32_timers *ddata?
> >>
> >> And only put declaration in include/linux/mfd/stm32-timers.h:
> >> + struct stm32_timers_dma;
> >>
> >> struct stm32_timers {
> >>struct clk *clk;
> >>struct regmap *regmap;
> >>u32 max_arr;
> >> +  struct stm32_timers_dma;
> >> };
> > 
> > Yes, that's the basic idea.
> > 
> >> I can probably spare the *dev then... use dev->parent in child driver.
> > 
> > What would you use dev->parent for?
> 
> Hi Lee,
> 
> This is to follow your sugestion to use *dev instead of *ddata when
> calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
> Then there is no need to keep *dev inside ddata struct.

I'm wondering if it would be neater to us the child's *dev, then do
the ->parent deference in the parent MFD (with a comment to say what
you're doing of course).

> > [...]
> > 
>  +static int stm32_timers_remove(struct platform_device *pdev)
>  +{
>  +struct stm32_timers *ddata = platform_get_drvdata(pdev);
>  +struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
>  +
>  +of_platform_depopulate(>dev);
> >>>
> >>> Why can't you continue using devm_*?
> >>
> >> I can use devm_of_platform_depopulate() here if you prefer, and keep
> >> devm_of_platform_populate() in probe.
> > 
> > The point of devm_* is that you don't have to call depopulate.
> > 
> > It happens automatically once this driver is unbound.
> 
> Ok, so to clarify, keeping devm_ here may be a bit racy:
> of_platform_depopulate will happen after dma has been released (there is
> no devm_ variant to release dma).
> Only way to prevent race condition here, is to enforce
> of_platform_depopulate() is called before dma release (e.g. in reverse
> order compared to probe).
> 
> Do you wish I add a comment about it ?

Best thing to do then is keep the non-devm variant and provide a
comment as to why is it not possible to use devm_*.

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

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

2018-03-29 Thread Fabrice Gasnier 
On 03/29/2018 02:59 PM, Lee Jones wrote:
> On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
> 
>> On 03/28/2018 05:22 PM, Lee Jones wrote:
>>> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
>>>
 STM32 Timers can support up to 7 DMA requests:
 - 4 channels, update, compare and trigger.
 Optionally request part, or all DMAs from stm32-timers MFD core.

 Also add routine to implement burst reads using DMA from timer registers.
 This is exported. So, it can be used by child drivers, PWM capture
 for instance (but not limited to).

 Signed-off-by: Fabrice Gasnier 
 Reviewed-by: Benjamin Gaignard 
 ---
 Changes in v2:
 - Abstract DMA handling from child driver: move it to MFD core
 - Add comments on optional dma support
 ---
  drivers/mfd/stm32-timers.c   | 215 
 ++-
  include/linux/mfd/stm32-timers.h |  27 +
  2 files changed, 238 insertions(+), 4 deletions(-)

 diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
 index a6675a4..2cdad2c 100644
 --- a/drivers/mfd/stm32-timers.c
 +++ b/drivers/mfd/stm32-timers.c
> 
> [...]
> 
 +  struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
 +  struct stm32_timers ddata;
>>>
>>> This looks odd to me.  Why can't you expand the current ddata
>>> structure?  Wouldn't it be better to create a stm32_timers_dma
>>> structure to place all this information in (except *dev, that should
>>> live in the ddata struct), then place a reference in the existing
>>> stm32_timers struct?
>>
>> Maybe I miss-understand you here, from what we discussed in V1:
>> https://lkml.org/lkml/2018/1/23/574
>>> ... "passing in the physical address of the parent MFD into
>>> a child device doesn't quite sit right with me"
>> I introduced this private struct in MFD parent, and completely hide it
>> from the child.
>>
>> So, do you suggest to add struct definition here ? But make it part of
>> struct stm32_timers *ddata?
>>
>> And only put declaration in include/linux/mfd/stm32-timers.h:
>> + struct stm32_timers_dma;
>>
>> struct stm32_timers {
>>  struct clk *clk;
>>  struct regmap *regmap;
>>  u32 max_arr;
>> +struct stm32_timers_dma;
>> };
> 
> Yes, that's the basic idea.
> 
>> I can probably spare the *dev then... use dev->parent in child driver.
> 
> What would you use dev->parent for?

Hi Lee,

This is to follow your sugestion to use *dev instead of *ddata when
calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
Then there is no need to keep *dev inside ddata struct.

> 
> [...]
> 
 +static int stm32_timers_remove(struct platform_device *pdev)
 +{
 +  struct stm32_timers *ddata = platform_get_drvdata(pdev);
 +  struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
 +
 +  of_platform_depopulate(>dev);
>>>
>>> Why can't you continue using devm_*?
>>
>> I can use devm_of_platform_depopulate() here if you prefer, and keep
>> devm_of_platform_populate() in probe.
> 
> The point of devm_* is that you don't have to call depopulate.
> 
> It happens automatically once this driver is unbound.

Ok, so to clarify, keeping devm_ here may be a bit racy:
of_platform_depopulate will happen after dma has been released (there is
no devm_ variant to release dma).
Only way to prevent race condition here, is to enforce
of_platform_depopulate() is called before dma release (e.g. in reverse
order compared to probe).

Do you wish I add a comment about it ?

Best Regards,
Fabrice

>

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

2018-03-29 Thread Fabrice Gasnier 
On 03/29/2018 02:59 PM, Lee Jones wrote:
> On Wed, 28 Mar 2018, Fabrice Gasnier wrote:
> 
>> On 03/28/2018 05:22 PM, Lee Jones wrote:
>>> On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
>>>
 STM32 Timers can support up to 7 DMA requests:
 - 4 channels, update, compare and trigger.
 Optionally request part, or all DMAs from stm32-timers MFD core.

 Also add routine to implement burst reads using DMA from timer registers.
 This is exported. So, it can be used by child drivers, PWM capture
 for instance (but not limited to).

 Signed-off-by: Fabrice Gasnier 
 Reviewed-by: Benjamin Gaignard 
 ---
 Changes in v2:
 - Abstract DMA handling from child driver: move it to MFD core
 - Add comments on optional dma support
 ---
  drivers/mfd/stm32-timers.c   | 215 
 ++-
  include/linux/mfd/stm32-timers.h |  27 +
  2 files changed, 238 insertions(+), 4 deletions(-)

 diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
 index a6675a4..2cdad2c 100644
 --- a/drivers/mfd/stm32-timers.c
 +++ b/drivers/mfd/stm32-timers.c
> 
> [...]
> 
 +  struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
 +  struct stm32_timers ddata;
>>>
>>> This looks odd to me.  Why can't you expand the current ddata
>>> structure?  Wouldn't it be better to create a stm32_timers_dma
>>> structure to place all this information in (except *dev, that should
>>> live in the ddata struct), then place a reference in the existing
>>> stm32_timers struct?
>>
>> Maybe I miss-understand you here, from what we discussed in V1:
>> https://lkml.org/lkml/2018/1/23/574
>>> ... "passing in the physical address of the parent MFD into
>>> a child device doesn't quite sit right with me"
>> I introduced this private struct in MFD parent, and completely hide it
>> from the child.
>>
>> So, do you suggest to add struct definition here ? But make it part of
>> struct stm32_timers *ddata?
>>
>> And only put declaration in include/linux/mfd/stm32-timers.h:
>> + struct stm32_timers_dma;
>>
>> struct stm32_timers {
>>  struct clk *clk;
>>  struct regmap *regmap;
>>  u32 max_arr;
>> +struct stm32_timers_dma;
>> };
> 
> Yes, that's the basic idea.
> 
>> I can probably spare the *dev then... use dev->parent in child driver.
> 
> What would you use dev->parent for?

Hi Lee,

This is to follow your sugestion to use *dev instead of *ddata when
calling stm32_timers_dma_burst_read(), the idea is to use it on child side:
stm32_timers_dma_burst_read(dev->parent,...) from pwm driver.
Then there is no need to keep *dev inside ddata struct.

> 
> [...]
> 
 +static int stm32_timers_remove(struct platform_device *pdev)
 +{
 +  struct stm32_timers *ddata = platform_get_drvdata(pdev);
 +  struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
 +
 +  of_platform_depopulate(>dev);
>>>
>>> Why can't you continue using devm_*?
>>
>> I can use devm_of_platform_depopulate() here if you prefer, and keep
>> devm_of_platform_populate() in probe.
> 
> The point of devm_* is that you don't have to call depopulate.
> 
> It happens automatically once this driver is unbound.

Ok, so to clarify, keeping devm_ here may be a bit racy:
of_platform_depopulate will happen after dma has been released (there is
no devm_ variant to release dma).
Only way to prevent race condition here, is to enforce
of_platform_depopulate() is called before dma release (e.g. in reverse
order compared to probe).

Do you wish I add a comment about it ?

Best Regards,
Fabrice

>

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

2018-03-29 Thread Lee Jones 
On Wed, 28 Mar 2018, Fabrice Gasnier wrote:

> On 03/28/2018 05:22 PM, Lee Jones wrote:
> > On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
> > 
> >> STM32 Timers can support up to 7 DMA requests:
> >> - 4 channels, update, compare and trigger.
> >> Optionally request part, or all DMAs from stm32-timers MFD core.
> >>
> >> Also add routine to implement burst reads using DMA from timer registers.
> >> This is exported. So, it can be used by child drivers, PWM capture
> >> for instance (but not limited to).
> >>
> >> Signed-off-by: Fabrice Gasnier 
> >> Reviewed-by: Benjamin Gaignard 
> >> ---
> >> Changes in v2:
> >> - Abstract DMA handling from child driver: move it to MFD core
> >> - Add comments on optional dma support
> >> ---
> >>  drivers/mfd/stm32-timers.c   | 215 
> >> ++-
> >>  include/linux/mfd/stm32-timers.h |  27 +
> >>  2 files changed, 238 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> >> index a6675a4..2cdad2c 100644
> >> --- a/drivers/mfd/stm32-timers.c
> >> +++ b/drivers/mfd/stm32-timers.c

[...]

> >> +  struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
> >> +  struct stm32_timers ddata;
> > 
> > This looks odd to me.  Why can't you expand the current ddata
> > structure?  Wouldn't it be better to create a stm32_timers_dma
> > structure to place all this information in (except *dev, that should
> > live in the ddata struct), then place a reference in the existing
> > stm32_timers struct?
> 
> Maybe I miss-understand you here, from what we discussed in V1:
> https://lkml.org/lkml/2018/1/23/574
> >... "passing in the physical address of the parent MFD into
> > a child device doesn't quite sit right with me"
> I introduced this private struct in MFD parent, and completely hide it
> from the child.
> 
> So, do you suggest to add struct definition here ? But make it part of
> struct stm32_timers *ddata?
> 
> And only put declaration in include/linux/mfd/stm32-timers.h:
> + struct stm32_timers_dma;
> 
> struct stm32_timers {
>   struct clk *clk;
>   struct regmap *regmap;
>   u32 max_arr;
> + struct stm32_timers_dma;
> };

Yes, that's the basic idea.

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

What would you use dev->parent for?

[...]

> >> +static int stm32_timers_remove(struct platform_device *pdev)
> >> +{
> >> +  struct stm32_timers *ddata = platform_get_drvdata(pdev);
> >> +  struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
> >> +
> >> +  of_platform_depopulate(>dev);
> > 
> > Why can't you continue using devm_*?
> 
> I can use devm_of_platform_depopulate() here if you prefer, and keep
> devm_of_platform_populate() in probe.

The point of devm_* is that you don't have to call depopulate.

It happens automatically once this driver is unbound.

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

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

2018-03-29 Thread Lee Jones 
On Wed, 28 Mar 2018, Fabrice Gasnier wrote:

> On 03/28/2018 05:22 PM, Lee Jones wrote:
> > On Wed, 14 Feb 2018, Fabrice Gasnier wrote:
> > 
> >> STM32 Timers can support up to 7 DMA requests:
> >> - 4 channels, update, compare and trigger.
> >> Optionally request part, or all DMAs from stm32-timers MFD core.
> >>
> >> Also add routine to implement burst reads using DMA from timer registers.
> >> This is exported. So, it can be used by child drivers, PWM capture
> >> for instance (but not limited to).
> >>
> >> Signed-off-by: Fabrice Gasnier 
> >> Reviewed-by: Benjamin Gaignard 
> >> ---
> >> Changes in v2:
> >> - Abstract DMA handling from child driver: move it to MFD core
> >> - Add comments on optional dma support
> >> ---
> >>  drivers/mfd/stm32-timers.c   | 215 
> >> ++-
> >>  include/linux/mfd/stm32-timers.h |  27 +
> >>  2 files changed, 238 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
> >> index a6675a4..2cdad2c 100644
> >> --- a/drivers/mfd/stm32-timers.c
> >> +++ b/drivers/mfd/stm32-timers.c

[...]

> >> +  struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
> >> +  struct stm32_timers ddata;
> > 
> > This looks odd to me.  Why can't you expand the current ddata
> > structure?  Wouldn't it be better to create a stm32_timers_dma
> > structure to place all this information in (except *dev, that should
> > live in the ddata struct), then place a reference in the existing
> > stm32_timers struct?
> 
> Maybe I miss-understand you here, from what we discussed in V1:
> https://lkml.org/lkml/2018/1/23/574
> >... "passing in the physical address of the parent MFD into
> > a child device doesn't quite sit right with me"
> I introduced this private struct in MFD parent, and completely hide it
> from the child.
> 
> So, do you suggest to add struct definition here ? But make it part of
> struct stm32_timers *ddata?
> 
> And only put declaration in include/linux/mfd/stm32-timers.h:
> + struct stm32_timers_dma;
> 
> struct stm32_timers {
>   struct clk *clk;
>   struct regmap *regmap;
>   u32 max_arr;
> + struct stm32_timers_dma;
> };

Yes, that's the basic idea.

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

What would you use dev->parent for?

[...]

> >> +static int stm32_timers_remove(struct platform_device *pdev)
> >> +{
> >> +  struct stm32_timers *ddata = platform_get_drvdata(pdev);
> >> +  struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
> >> +
> >> +  of_platform_depopulate(>dev);
> > 
> > Why can't you continue using devm_*?
> 
> I can use devm_of_platform_depopulate() here if you prefer, and keep
> devm_of_platform_populate() in probe.

The point of devm_* is that you don't have to call depopulate.

It happens automatically once this driver is unbound.

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

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

2018-03-28 Thread Fabrice Gasnier 
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 
>> Reviewed-by: Benjamin Gaignard 
>> ---
>> 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 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>>  #include 
>>  
>> +#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, );
>> +if (status == DMA_COMPLETE)
>> +complete(>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 =

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

2018-03-28 Thread Fabrice Gasnier 
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 
>> Reviewed-by: Benjamin Gaignard 
>> ---
>> 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 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>>  #include 
>>  
>> +#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, );
>> +if (status == DMA_COMPLETE)
>> +complete(>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 =

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

2018-03-28 Thread Lee Jones 
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 
> Reviewed-by: Benjamin Gaignard 
> ---
> 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 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  
> +#define STM32_TIMERS_MAX_REGISTERS   0x3fc
> +
> +struct stm32_timers_priv {
> + struct device *dev;
> + struct completion completion;
> + phys_addr_t phys_base;  /* timers physical addr for dma */
> + struct mutex lock;  /* protect dma access */
> + struct dma_chan *dma_chan;  /* dma channel in use */

I think kerneldoc would be better than inline comments.

> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
> + struct stm32_timers ddata;

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

> +};
> +
> +static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d)
> +{
> + return container_of(d, struct stm32_timers_priv, ddata);
> +}

If you take my other suggestions, you can remove this function.

> +/* DIER register DMA enable bits */
> +static const u32 stm32_timers_dier_dmaen[STM32_TIMERS_MAX_DMAS] = {
> + TIM_DIER_CC1DE, TIM_DIER_CC2DE, TIM_DIER_CC3DE, TIM_DIER_CC4DE,
> + TIM_DIER_UIE, TIM_DIER_TDE, TIM_DIER_COMDE
> +};

Maybe one per line would be kinder on the eye?

> +static void stm32_timers_dma_done(void *p)
> +{
> + struct stm32_timers_priv *priv = p;
> + struct dma_chan *dma_chan = priv->dma_chan;
> + struct dma_tx_state state;
> + enum dma_status status;
> +
> + status = dmaengine_tx_status(dma_chan, dma_chan->cookie, );
> + if (status == DMA_COMPLETE)
> + complete(>completion);
> +}
> +
> +/**
> + * stm32_timers_dma_burst_read - Read from timers registers using DMA.
> + *
> + * Read from STM32 timers registers using DMA on a single event.
> + * @ddata: reference to stm32_timers

It's odd to pass device data back like this.

Better to pass a pointer to 'struct device', then use the normal
helpers.

> + * @buf: dma'able destination buffer
> + * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
> + * @reg: registers start offset for DMA to read from (like CCRx for capture)
> + * @num_reg: number of registers to read upon each dma request, starting 
> @reg.
> + * @bursts: number of bursts to read (e.g. like two for pwm period capture)
> + * @tmo_ms: timeout (milliseconds)
> + */
> +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf,
> + enum stm32_timers_dmas id, u32 reg,
> + unsigned int num_reg, unsigned int bursts,
> + unsigned long tmo_ms)
> +{
> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
> + unsigned long timeout = msecs_to_jiffies(tmo_ms);
> + struct regmap *regmap = priv->ddata.regmap;
> + size_t len = num_reg * bursts * sizeof(u32);
> + struct dma_async_tx_descriptor *desc;
> + struct dma_slave_config config;
> + dma_cookie_t cookie;
> + dma_addr_t dma_buf;
> + u32 dbl, dba;
> + long err;
> + int ret;
> +
> + /* sanity check */
> + if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
> + return -EINVAL;
> +
> + if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
> + (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
> + return -EINVAL;
> +
> + if (!priv->dmas[id])
> + return -ENODEV;
> + mutex_lock(>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

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

2018-03-28 Thread Lee Jones 
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 
> Reviewed-by: Benjamin Gaignard 
> ---
> 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 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  
> +#define STM32_TIMERS_MAX_REGISTERS   0x3fc
> +
> +struct stm32_timers_priv {
> + struct device *dev;
> + struct completion completion;
> + phys_addr_t phys_base;  /* timers physical addr for dma */
> + struct mutex lock;  /* protect dma access */
> + struct dma_chan *dma_chan;  /* dma channel in use */

I think kerneldoc would be better than inline comments.

> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
> + struct stm32_timers ddata;

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

> +};
> +
> +static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d)
> +{
> + return container_of(d, struct stm32_timers_priv, ddata);
> +}

If you take my other suggestions, you can remove this function.

> +/* DIER register DMA enable bits */
> +static const u32 stm32_timers_dier_dmaen[STM32_TIMERS_MAX_DMAS] = {
> + TIM_DIER_CC1DE, TIM_DIER_CC2DE, TIM_DIER_CC3DE, TIM_DIER_CC4DE,
> + TIM_DIER_UIE, TIM_DIER_TDE, TIM_DIER_COMDE
> +};

Maybe one per line would be kinder on the eye?

> +static void stm32_timers_dma_done(void *p)
> +{
> + struct stm32_timers_priv *priv = p;
> + struct dma_chan *dma_chan = priv->dma_chan;
> + struct dma_tx_state state;
> + enum dma_status status;
> +
> + status = dmaengine_tx_status(dma_chan, dma_chan->cookie, );
> + if (status == DMA_COMPLETE)
> + complete(>completion);
> +}
> +
> +/**
> + * stm32_timers_dma_burst_read - Read from timers registers using DMA.
> + *
> + * Read from STM32 timers registers using DMA on a single event.
> + * @ddata: reference to stm32_timers

It's odd to pass device data back like this.

Better to pass a pointer to 'struct device', then use the normal
helpers.

> + * @buf: dma'able destination buffer
> + * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
> + * @reg: registers start offset for DMA to read from (like CCRx for capture)
> + * @num_reg: number of registers to read upon each dma request, starting 
> @reg.
> + * @bursts: number of bursts to read (e.g. like two for pwm period capture)
> + * @tmo_ms: timeout (milliseconds)
> + */
> +int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf,
> + enum stm32_timers_dmas id, u32 reg,
> + unsigned int num_reg, unsigned int bursts,
> + unsigned long tmo_ms)
> +{
> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
> + unsigned long timeout = msecs_to_jiffies(tmo_ms);
> + struct regmap *regmap = priv->ddata.regmap;
> + size_t len = num_reg * bursts * sizeof(u32);
> + struct dma_async_tx_descriptor *desc;
> + struct dma_slave_config config;
> + dma_cookie_t cookie;
> + dma_addr_t dma_buf;
> + u32 dbl, dba;
> + long err;
> + int ret;
> +
> + /* sanity check */
> + if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
> + return -EINVAL;
> +
> + if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
> + (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
> + return -EINVAL;
> +
> + if (!priv->dmas[id])
> + return -ENODEV;
> + mutex_lock(>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(, 0, sizeof(config));
> +

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

2018-02-14 Thread Fabrice Gasnier 
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 
Reviewed-by: Benjamin Gaignard 
---
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 
+#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
+#define STM32_TIMERS_MAX_REGISTERS 0x3fc
+
+struct stm32_timers_priv {
+   struct device *dev;
+   struct completion completion;
+   phys_addr_t phys_base;  /* timers physical addr for dma */
+   struct mutex lock;  /* protect dma access */
+   struct dma_chan *dma_chan;  /* dma channel in use */
+   struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
+   struct stm32_timers ddata;
+};
+
+static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d)
+{
+   return container_of(d, struct stm32_timers_priv, ddata);
+}
+
+/* DIER register DMA enable bits */
+static const u32 stm32_timers_dier_dmaen[STM32_TIMERS_MAX_DMAS] = {
+   TIM_DIER_CC1DE, TIM_DIER_CC2DE, TIM_DIER_CC3DE, TIM_DIER_CC4DE,
+   TIM_DIER_UIE, TIM_DIER_TDE, TIM_DIER_COMDE
+};
+
+static void stm32_timers_dma_done(void *p)
+{
+   struct stm32_timers_priv *priv = p;
+   struct dma_chan *dma_chan = priv->dma_chan;
+   struct dma_tx_state state;
+   enum dma_status status;
+
+   status = dmaengine_tx_status(dma_chan, dma_chan->cookie, );
+   if (status == DMA_COMPLETE)
+   complete(>completion);
+}
+
+/**
+ * stm32_timers_dma_burst_read - Read from timers registers using DMA.
+ *
+ * Read from STM32 timers registers using DMA on a single event.
+ * @ddata: reference to stm32_timers
+ * @buf: dma'able destination buffer
+ * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
+ * @reg: registers start offset for DMA to read from (like CCRx for capture)
+ * @num_reg: number of registers to read upon each dma request, starting @reg.
+ * @bursts: number of bursts to read (e.g. like two for pwm period capture)
+ * @tmo_ms: timeout (milliseconds)
+ */
+int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf,
+   enum stm32_timers_dmas id, u32 reg,
+   unsigned int num_reg, unsigned int bursts,
+   unsigned long tmo_ms)
+{
+   struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
+   unsigned long timeout = msecs_to_jiffies(tmo_ms);
+   struct regmap *regmap = priv->ddata.regmap;
+   size_t len = num_reg * bursts * sizeof(u32);
+   struct dma_async_tx_descriptor *desc;
+   struct dma_slave_config config;
+   dma_cookie_t cookie;
+   dma_addr_t dma_buf;
+   u32 dbl, dba;
+   long err;
+   int ret;
+
+   /* sanity check */
+   if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
+   return -EINVAL;
+
+   if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
+   (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
+   return -EINVAL;
+
+   if (!priv->dmas[id])
+   return -ENODEV;
+   mutex_lock(>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(, 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, );
+   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;
+
+

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

2018-02-14 Thread Fabrice Gasnier 
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 
Reviewed-by: Benjamin Gaignard 
---
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 
+#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
+#define STM32_TIMERS_MAX_REGISTERS 0x3fc
+
+struct stm32_timers_priv {
+   struct device *dev;
+   struct completion completion;
+   phys_addr_t phys_base;  /* timers physical addr for dma */
+   struct mutex lock;  /* protect dma access */
+   struct dma_chan *dma_chan;  /* dma channel in use */
+   struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS];
+   struct stm32_timers ddata;
+};
+
+static struct stm32_timers_priv *to_stm32_timers_priv(struct stm32_timers *d)
+{
+   return container_of(d, struct stm32_timers_priv, ddata);
+}
+
+/* DIER register DMA enable bits */
+static const u32 stm32_timers_dier_dmaen[STM32_TIMERS_MAX_DMAS] = {
+   TIM_DIER_CC1DE, TIM_DIER_CC2DE, TIM_DIER_CC3DE, TIM_DIER_CC4DE,
+   TIM_DIER_UIE, TIM_DIER_TDE, TIM_DIER_COMDE
+};
+
+static void stm32_timers_dma_done(void *p)
+{
+   struct stm32_timers_priv *priv = p;
+   struct dma_chan *dma_chan = priv->dma_chan;
+   struct dma_tx_state state;
+   enum dma_status status;
+
+   status = dmaengine_tx_status(dma_chan, dma_chan->cookie, );
+   if (status == DMA_COMPLETE)
+   complete(>completion);
+}
+
+/**
+ * stm32_timers_dma_burst_read - Read from timers registers using DMA.
+ *
+ * Read from STM32 timers registers using DMA on a single event.
+ * @ddata: reference to stm32_timers
+ * @buf: dma'able destination buffer
+ * @id: stm32_timers_dmas event identifier (ch[1..4], up, trig or com)
+ * @reg: registers start offset for DMA to read from (like CCRx for capture)
+ * @num_reg: number of registers to read upon each dma request, starting @reg.
+ * @bursts: number of bursts to read (e.g. like two for pwm period capture)
+ * @tmo_ms: timeout (milliseconds)
+ */
+int stm32_timers_dma_burst_read(struct stm32_timers *ddata, u32 *buf,
+   enum stm32_timers_dmas id, u32 reg,
+   unsigned int num_reg, unsigned int bursts,
+   unsigned long tmo_ms)
+{
+   struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata);
+   unsigned long timeout = msecs_to_jiffies(tmo_ms);
+   struct regmap *regmap = priv->ddata.regmap;
+   size_t len = num_reg * bursts * sizeof(u32);
+   struct dma_async_tx_descriptor *desc;
+   struct dma_slave_config config;
+   dma_cookie_t cookie;
+   dma_addr_t dma_buf;
+   u32 dbl, dba;
+   long err;
+   int ret;
+
+   /* sanity check */
+   if (id < STM32_TIMERS_DMA_CH1 || id >= STM32_TIMERS_MAX_DMAS)
+   return -EINVAL;
+
+   if (!num_reg || !bursts || reg > STM32_TIMERS_MAX_REGISTERS ||
+   (reg + num_reg * sizeof(u32)) > STM32_TIMERS_MAX_REGISTERS)
+   return -EINVAL;
+
+   if (!priv->dmas[id])
+   return -ENODEV;
+   mutex_lock(>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(, 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, );
+   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(>completion);
+