Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Vinod Koul 
On Wed, Oct 14, 2015 at 05:41:26PM +0200, M'boumba Cedric Madianga wrote:
> 2015-10-14 17:28 GMT+02:00 Daniel Thompson :
> > On 14/10/15 16:26, M'boumba Cedric Madianga wrote:
> >>
> >> 2015-10-14 16:24 GMT+02:00 Daniel Thompson :
> >>>
> >>>
> >>> Hmnnn...
> >>>
> >>> The dmaengine framework will WARN_ONCE() if an dmaengine is removed
> >>> whilst
> >>> it is active and also works hard to ensure dmaengine modules are not
> >>> removed
> >>> whilst there are active drivers using the framework.
> >>>
> >>> How do we get into this function whilst there is still an active DMA
> >>> channels?
> >>
> >>
> >> For example, when a user try "rmmod stm32-dma" in uart console.
> >> It will enter in stm32_dma_remove while there is potentially still active
> >> DMA.
> >
> >
> > Check dmaengine.c for yourself but I think in this case the dmaengine
> > framework will hold references to the module and prevent the remove from
> > taking place.
> 
> Yes I did it.
> As far I understand, the dmaengine framework will print a warning
> message but doesn't stop removing operation if there are some active
> clients.

we hold a ref, so removal wont work, see dma_chan_get()

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
2015-10-14 17:28 GMT+02:00 Daniel Thompson :
> On 14/10/15 16:26, M'boumba Cedric Madianga wrote:
>>
>> 2015-10-14 16:24 GMT+02:00 Daniel Thompson :
>>>
>>>
>>> Hmnnn...
>>>
>>> The dmaengine framework will WARN_ONCE() if an dmaengine is removed
>>> whilst
>>> it is active and also works hard to ensure dmaengine modules are not
>>> removed
>>> whilst there are active drivers using the framework.
>>>
>>> How do we get into this function whilst there is still an active DMA
>>> channels?
>>
>>
>> For example, when a user try "rmmod stm32-dma" in uart console.
>> It will enter in stm32_dma_remove while there is potentially still active
>> DMA.
>
>
> Check dmaengine.c for yourself but I think in this case the dmaengine
> framework will hold references to the module and prevent the remove from
> taking place.

Yes I did it.
As far I understand, the dmaengine framework will print a warning
message but doesn't stop removing operation if there are some active
clients.

Cedric.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 16:26, M'boumba Cedric Madianga wrote:

2015-10-14 16:24 GMT+02:00 Daniel Thompson :


Hmnnn...

The dmaengine framework will WARN_ONCE() if an dmaengine is removed whilst
it is active and also works hard to ensure dmaengine modules are not removed
whilst there are active drivers using the framework.

How do we get into this function whilst there is still an active DMA
channels?


For example, when a user try "rmmod stm32-dma" in uart console.
It will enter in stm32_dma_remove while there is potentially still active DMA.


Check dmaengine.c for yourself but I think in this case the dmaengine 
framework will hold references to the module and prevent the remove from 
taking place.



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
2015-10-14 16:24 GMT+02:00 Daniel Thompson :
>
> Hmnnn...
>
> The dmaengine framework will WARN_ONCE() if an dmaengine is removed whilst
> it is active and also works hard to ensure dmaengine modules are not removed
> whilst there are active drivers using the framework.
>
> How do we get into this function whilst there is still an active DMA
> channels?

For example, when a user try "rmmod stm32-dma" in uart console.
It will enter in stm32_dma_remove while there is potentially still active DMA.

>
>
>> But even with this improvement, I think I have to disable the clock here.
>
>
> As above, I think the dmaengine framework work to protect you from this sort
> problem. However even if I am wrong about that then unconditionally calling
> clk_disable_unprepare() can not be used to reliably manage the clocks. You
> don't know what the counts are!

Ok got it. Thanks for the clarification.

Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 14:41, M'boumba Cedric Madianga wrote:

2015-10-14 15:29 GMT+02:00 Daniel Thompson :

On 14/10/15 14:17, M'boumba Cedric Madianga wrote:


Hi Daniel,


+
+static int stm32_dma_remove(struct platform_device *pdev)
+{
+   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
+
+   of_dma_controller_free(pdev->dev.of_node);
+
+   dma_async_device_unregister(>ddev);
+
+   clk_disable_unprepare(dmadev->clk);




What is the purpose of disabling/unpreparing the clock here?
stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources()
should
pair up and the clock should already be stopped.



stm32_dma_remove() could be called during an on-going transfer during
module unload.
So in that case, it seems that disabling/unpreparing the clock is needed.



Really?

I think we need to be sure any on-going transfers are stopped.

There are multiple reasons for this, not least the risk of executing a
callback that has been freed, but the one related to my point is that a
single clk_disable_unprepare() will remain broken because if you don't know
that the transfers have stopped then you don't know how many on-going
transfers there are.


In the next version, I am going to stop the DMA, free all interrupts
and kill tasklet in stm32_dma_remove.
In that way, all on-going transfers will be lost as we don't have to
wait the end of remaining transfer in order to execute this function
as quickly as possible.


Hmnnn...

The dmaengine framework will WARN_ONCE() if an dmaengine is removed 
whilst it is active and also works hard to ensure dmaengine modules are 
not removed whilst there are active drivers using the framework.


How do we get into this function whilst there is still an active DMA 
channels?




But even with this improvement, I think I have to disable the clock here.


As above, I think the dmaengine framework work to protect you from this 
sort problem. However even if I am wrong about that then unconditionally 
calling clk_disable_unprepare() can not be used to reliably manage the 
clocks. You don't know what the counts are!



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Vinod Koul 
On Wed, Oct 14, 2015 at 03:07:16PM +0200, M'boumba Cedric Madianga wrote:
> >> +static inline uint32_t stm32_dma_read(struct stm32_dma_device *dmadev, 
> >> u32 reg)
> >
> > this and few other could be made more readable
> 
> Ok, I could replace uint32_t by u32. Is it what you expect ?
> For others, I don't see how I could make it more readable.
> Could you please give me more details to help me ? Thanks in advance

Yes that is one and also aplitting this up so that it fits within 80chars
while not sacrficing readablity...

> >> + } else if ((status & STM32_DMA_FEI) && (sfcr & STM32_DMA_SFCR_FEIE)) 
> >> {
> >> + dev_err(chan2dev(chan), "DMA error: received FEI event\n");
> >> + stm32_dma_irq_clear(chan, STM32_DMA_FEI);
> >> + chan->status = DMA_ERROR;
> >> + spin_unlock(>vchan.lock);
> >> + return IRQ_HANDLED;
> >
> > this is repeat of above apart from err print!!
> 
> Not only for print as we also need that to define which bit to clear
> in the IRQ status.
> In that way we will be sure to handle each interrupt event.

You can print error type based on status, or print the whole status but
handle it same for all three cases

> >> + enum dma_status status;
> >> + unsigned long flags;
> >> + unsigned int residue;
> >> +
> >> + status = dma_cookie_status(c, cookie, state);
> >> + if (status == DMA_COMPLETE)
> >> + return status;
> >> +
> >> + if (!state)
> >> + return chan->status;
> > why channel status and not status from dma_cookie_status()?
> 
> If status is different than DMA_COMPLETE it seems better to use channel 
> status.
> Indeed, in that way, we have the possibility to notify that there is a
> potential error in the bus via DMA_ERROR.
> But if I return status from dma_cookie_status(), I will always notify
> DMA_IN_PROGRESS.

No, you are supposed to return the descriptor status and not channel status!

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
2015-10-14 15:29 GMT+02:00 Daniel Thompson :
> On 14/10/15 14:17, M'boumba Cedric Madianga wrote:
>>
>> Hi Daniel,
>>
> +
> +static int stm32_dma_remove(struct platform_device *pdev)
> +{
> +   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
> +
> +   of_dma_controller_free(pdev->dev.of_node);
> +
> +   dma_async_device_unregister(>ddev);
> +
> +   clk_disable_unprepare(dmadev->clk);



 What is the purpose of disabling/unpreparing the clock here?
 stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources()
 should
 pair up and the clock should already be stopped.
>>
>>
>> stm32_dma_remove() could be called during an on-going transfer during
>> module unload.
>> So in that case, it seems that disabling/unpreparing the clock is needed.
>
>
> Really?
>
> I think we need to be sure any on-going transfers are stopped.
>
> There are multiple reasons for this, not least the risk of executing a
> callback that has been freed, but the one related to my point is that a
> single clk_disable_unprepare() will remain broken because if you don't know
> that the transfers have stopped then you don't know how many on-going
> transfers there are.

In the next version, I am going to stop the DMA, free all interrupts
and kill tasklet in stm32_dma_remove.
In that way, all on-going transfers will be lost as we don't have to
wait the end of remaining transfer in order to execute this function
as quickly as possible.
But even with this improvement, I think I have to disable the clock here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 14:17, M'boumba Cedric Madianga wrote:

Hi Daniel,


+
+static int stm32_dma_remove(struct platform_device *pdev)
+{
+   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
+
+   of_dma_controller_free(pdev->dev.of_node);
+
+   dma_async_device_unregister(>ddev);
+
+   clk_disable_unprepare(dmadev->clk);



What is the purpose of disabling/unpreparing the clock here?
stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources() should
pair up and the clock should already be stopped.


stm32_dma_remove() could be called during an on-going transfer during
module unload.
So in that case, it seems that disabling/unpreparing the clock is needed.


Really?

I think we need to be sure any on-going transfers are stopped.

There are multiple reasons for this, not least the risk of executing a 
callback that has been freed, but the one related to my point is that a 
single clk_disable_unprepare() will remain broken because if you don't 
know that the transfers have stopped then you don't know how many 
on-going transfers there are.



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Daniel,

>>> +
>>> +static int stm32_dma_remove(struct platform_device *pdev)
>>> +{
>>> +   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
>>> +
>>> +   of_dma_controller_free(pdev->dev.of_node);
>>> +
>>> +   dma_async_device_unregister(>ddev);
>>> +
>>> +   clk_disable_unprepare(dmadev->clk);
>>
>>
>> What is the purpose of disabling/unpreparing the clock here?
>> stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources() should
>> pair up and the clock should already be stopped.

stm32_dma_remove() could be called during an on-going transfer during
module unload.
So in that case, it seems that disabling/unpreparing the clock is needed.

BR,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Vinod,


2015-10-14 13:16 GMT+02:00 Vinod Koul :
> On Tue, Oct 13, 2015 at 04:05:25PM +0200, M'boumba Cedric Madianga wrote:
>> +#define STM32_DMA_LISR   0x /* DMA Low Int Status 
>> Reg */
>> +#define STM32_DMA_HISR   0x0004 /* DMA High Int Status 
>> Reg */
>> +#define STM32_DMA_LIFCR  0x0008 /* DMA Low Int Flag 
>> Clear Reg */
>> +#define STM32_DMA_HIFCR  0x000c /* DMA High Int Flag 
>> Clear Reg */
>> +#define STM32_DMA_TCIBIT(5) /* Transfer Complete 
>> Interrupt */
>> +#define STM32_DMA_HTIBIT(4) /* Half Transfer 
>> Interrupt */
>> +#define STM32_DMA_TEIBIT(3) /* Transfer Error 
>> Interrupt */
>> +#define STM32_DMA_DMEI   BIT(2) /* Direct Mode Error 
>> Interrupt */
>> +#define STM32_DMA_FEIBIT(0) /* FIFO Error Interrupt 
>> */
>
> Why not use BIT() for everything here and make it consistent
>
> Also where ever possible stick to 80 char limit like above you can

In fact STM32_DMA_LISR, HISR, LIFCR and HIFCR are offset to access register.
So BIT() could not be used for this purpose.

>
>> +
>> +/* DMA Stream x Configuration Register */
>> +#define STM32_DMA_SCR(x) (0x0010 + 0x18 * (x)) /* x = 0..7 */
>> +#define STM32_DMA_SCR_REQ(n) ((n & 0x7) << 25)
>
> this and below looks ugly and hard to maintain, are you sure spec doesn't
> have a formulae for these?

We don't have any formulae in spec to handle this.
It is clearly described that we have to access registers in that way
to address the good channel.
Please, see below an extract of the spec for more details:
DMA stream x configuration register (DMA_SxCR) (x = 0..7)
This register is used to configure the concerned stream.
Address offset: 0x10 + 0x18 × stream number

>
>> +static inline uint32_t stm32_dma_read(struct stm32_dma_device *dmadev, u32 
>> reg)
>
> this and few other could be made more readable

Ok, I could replace uint32_t by u32. Is it what you expect ?
For others, I don't see how I could make it more readable.
Could you please give me more details to help me ? Thanks in advance

>
>> +static struct stm32_dma_desc *stm32_dma_alloc_desc(unsigned int num_sgs)
>> +{
>> + return kzalloc(sizeof(struct stm32_dma_desc) +
>> +sizeof(struct stm32_dma_sg_req) * num_sgs, GFP_ATOMIC);
>
> Not GFP_NOWAIT ?

You're right. We could use GFP_NOWAIT instead. Thanks.

>
>> +static enum stm32_dma_width stm32_get_dma_width(struct stm32_dma_chan *chan,
>> + enum dma_slave_buswidth width)
>> +{
>> + switch (width) {
>> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> + return STM32_DMA_BYTE;
>> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> + return STM32_DMA_HALF_WORD;
>> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> + return STM32_DMA_WORD;
>> + default:
>> + dev_warn(chan2dev(chan),
>> +  "Dma bus width not supported, using 32bits\n");
>> + return STM32_DMA_WORD;
>
> pls return error here
> Assuming wrong parameter can cause havoc of transfer, so is not advisable

OK, I will fix it in the next version. Thanks for the explanation.

>
>> +static enum stm32_dma_burst_size stm32_get_dma_burst(
>> + struct stm32_dma_chan *chan, u32 maxburst)
>> +{
>> + switch (maxburst) {
>> + case 0:
>> + case 1:
>> + return STM32_DMA_BURST_SINGLE;
>> + case 4:
>> + return STM32_DMA_BURST_INCR4;
>> + case 8:
>> + return STM32_DMA_BURST_INCR8;
>> + case 16:
>> + return STM32_DMA_BURST_INCR16;
>> + default:
>> + dev_warn(chan2dev(chan),
>> +  "Dma burst size not supported, using single\n");
>> + return STM32_DMA_BURST_SINGLE;
>
> here too

Ditto

>
>> + }
>> +}
>> +
>> +static int stm32_dma_slave_config(struct dma_chan *c,
>> +   struct dma_slave_config *config)
>> +{
>> + struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
>> +
>> + if (chan->busy) {
>> + dev_err(chan2dev(chan), "Configuration not allowed\n");
>> + return -EBUSY;
>> + }
>
> That is false condition. This configuration should be used for next
> descriptor prepare

OK. Will be fixed in the next patch version.

>
>> +static int stm32_dma_disable_chan(struct stm32_dma_chan *chan)
>> +{
>> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> + unsigned long timeout = jiffies + msecs_to_jiffies(5000);
>> + u32 dma_scr;
>> +
>> + dma_scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
>> + if (dma_scr & STM32_DMA_SCR_EN) {
>> + dma_scr &= ~STM32_DMA_SCR_EN;
>> + stm32_dma_write(dmadev, STM32_DMA_SCR(chan->id), dma_scr);
>> +
>> + do {
>> + dma_scr = stm32_dma_read(dmadev,

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Vinod Koul 
On Tue, Oct 13, 2015 at 04:05:25PM +0200, M'boumba Cedric Madianga wrote:
> +#define STM32_DMA_LISR   0x /* DMA Low Int Status 
> Reg */
> +#define STM32_DMA_HISR   0x0004 /* DMA High Int Status 
> Reg */
> +#define STM32_DMA_LIFCR  0x0008 /* DMA Low Int Flag 
> Clear Reg */
> +#define STM32_DMA_HIFCR  0x000c /* DMA High Int Flag 
> Clear Reg */
> +#define STM32_DMA_TCIBIT(5) /* Transfer Complete 
> Interrupt */
> +#define STM32_DMA_HTIBIT(4) /* Half Transfer 
> Interrupt */
> +#define STM32_DMA_TEIBIT(3) /* Transfer Error 
> Interrupt */
> +#define STM32_DMA_DMEI   BIT(2) /* Direct Mode Error 
> Interrupt */
> +#define STM32_DMA_FEIBIT(0) /* FIFO Error Interrupt 
> */

Why not use BIT() for everything here and make it consistent

Also where ever possible stick to 80 char limit like above you can

> +
> +/* DMA Stream x Configuration Register */
> +#define STM32_DMA_SCR(x) (0x0010 + 0x18 * (x)) /* x = 0..7 */
> +#define STM32_DMA_SCR_REQ(n) ((n & 0x7) << 25)

this and below looks ugly and hard to maintain, are you sure spec doesn't
have a formulae for these?

> +static inline uint32_t stm32_dma_read(struct stm32_dma_device *dmadev, u32 
> reg)

this and few other could be made more readable

> +static struct stm32_dma_desc *stm32_dma_alloc_desc(unsigned int num_sgs)
> +{
> + return kzalloc(sizeof(struct stm32_dma_desc) +
> +sizeof(struct stm32_dma_sg_req) * num_sgs, GFP_ATOMIC);

Not GFP_NOWAIT ?

> +static enum stm32_dma_width stm32_get_dma_width(struct stm32_dma_chan *chan,
> + enum dma_slave_buswidth width)
> +{
> + switch (width) {
> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
> + return STM32_DMA_BYTE;
> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> + return STM32_DMA_HALF_WORD;
> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> + return STM32_DMA_WORD;
> + default:
> + dev_warn(chan2dev(chan),
> +  "Dma bus width not supported, using 32bits\n");
> + return STM32_DMA_WORD;

pls return error here
Assuming wrong parameter can cause havoc of transfer, so is not advisable

> +static enum stm32_dma_burst_size stm32_get_dma_burst(
> + struct stm32_dma_chan *chan, u32 maxburst)
> +{
> + switch (maxburst) {
> + case 0:
> + case 1:
> + return STM32_DMA_BURST_SINGLE;
> + case 4:
> + return STM32_DMA_BURST_INCR4;
> + case 8:
> + return STM32_DMA_BURST_INCR8;
> + case 16:
> + return STM32_DMA_BURST_INCR16;
> + default:
> + dev_warn(chan2dev(chan),
> +  "Dma burst size not supported, using single\n");
> + return STM32_DMA_BURST_SINGLE;

here too

> + }
> +}
> +
> +static int stm32_dma_slave_config(struct dma_chan *c,
> +   struct dma_slave_config *config)
> +{
> + struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
> +
> + if (chan->busy) {
> + dev_err(chan2dev(chan), "Configuration not allowed\n");
> + return -EBUSY;
> + }

That is false condition. This configuration should be used for next
descriptor prepare

> +static int stm32_dma_disable_chan(struct stm32_dma_chan *chan)
> +{
> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
> + unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> + u32 dma_scr;
> +
> + dma_scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
> + if (dma_scr & STM32_DMA_SCR_EN) {
> + dma_scr &= ~STM32_DMA_SCR_EN;
> + stm32_dma_write(dmadev, STM32_DMA_SCR(chan->id), dma_scr);
> +
> + do {
> + dma_scr = stm32_dma_read(dmadev,
> +  STM32_DMA_SCR(chan->id));
> + dma_scr &= STM32_DMA_SCR_EN;
> + if (!dma_scr)
> + break;

empty line here would improve readability

> +static irqreturn_t stm32_dma_chan_irq(int irq, void *devid)
> +{
> + struct stm32_dma_chan *chan = devid;
> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
> + u32 status, scr, sfcr;
> +
> + spin_lock(>vchan.lock);
> +
> + status = stm32_dma_irq_status(chan);
> + scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
> + sfcr = stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id));
> +
> + if ((status & STM32_DMA_HTI) && (scr & STM32_DMA_SCR_HTIE)) {
> + stm32_dma_irq_clear(chan, STM32_DMA_HTI);
> + vchan_cyclic_callback(>desc->vdesc);
> + spin_unlock(>vchan.lock);
> + return IRQ_HANDLED;

line here please and below

> + } else if ((status & STM32_DMA_TCI) && (scr &

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Daniel,

2015-10-14 10:52 GMT+02:00 Daniel Thompson :
> On 14/10/15 08:54, M'boumba Cedric Madianga wrote:

 +static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
 +{
 +   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
 +   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
 +   int ret;
 +
 +   chan->config_init = false;
 +   ret = clk_prepare_enable(dmadev->clk);
 +   if (ret < 0) {
 +   dev_err(chan2dev(chan), "clk_prepare_enable failed:
 %d\n",
 ret);
 +   return ret;
 +   }
 +
 +   ret = stm32_dma_disable_chan(chan);
 +
 +   return ret;
 +}
>>>
>
>
> If stm32_dma_disable_chan() returns an error then we will not restore the
> original the clock counts causing them to "leak".

Ok got it. Thanks for the clarification.
I will fix it in the next version.

BR,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 08:54, M'boumba Cedric Madianga wrote:

+static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
+{
+   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
+   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
+   int ret;
+
+   chan->config_init = false;
+   ret = clk_prepare_enable(dmadev->clk);
+   if (ret < 0) {
+   dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n",
ret);
+   return ret;
+   }
+
+   ret = stm32_dma_disable_chan(chan);
+
+   return ret;
+}



The error path here looks like it will leak clock references.


Sorry I didn't catch it. What does it mean ?


If stm32_dma_disable_chan() returns an error then we will not restore 
the original the clock counts causing them to "leak".



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Daniel,

2015-10-13 16:34 GMT+02:00 Daniel Thompson :
> On 13/10/15 15:05, M'boumba Cedric Madianga wrote:
>>
>> This patch adds support for the STM32 DMA controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga 
>> ---
>>   drivers/dma/Kconfig |   12 +
>>   drivers/dma/Makefile|1 +
>>   drivers/dma/stm32-dma.c | 1175
>> +++
>>   3 files changed, 1188 insertions(+)
>>   create mode 100644 drivers/dma/stm32-dma.c
>
>
>> diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
>> new file mode 100644
>> index 000..031bab7
>> --- /dev/null
>> +++ b/drivers/dma/stm32-dma.c
>
>
>> +enum stm32_dma_width {
>> +   STM32_DMA_BYTE,
>> +   STM32_DMA_HALF_WORD,
>> +   STM32_DMA_WORD,
>> +};
>> +
>> +enum stm32_dma_burst_size {
>> +   STM32_DMA_BURST_SINGLE,
>> +   STM32_DMA_BURST_INCR4,
>> +   STM32_DMA_BURST_INCR8,
>> +   STM32_DMA_BURST_INCR16,
>> +};
>> +
>> +enum stm32_dma_channel_id {
>> +   STM32_DMA_CHANNEL0,
>> +   STM32_DMA_CHANNEL1,
>> +   STM32_DMA_CHANNEL2,
>> +   STM32_DMA_CHANNEL3,
>> +   STM32_DMA_CHANNEL4,
>> +   STM32_DMA_CHANNEL5,
>> +   STM32_DMA_CHANNEL6,
>> +   STM32_DMA_CHANNEL7,
>> +};
>
>
> Why have (unused) enumerations to map numeric symbols to their own natural
> values? Using normal integers would be better.

Good point. I will remove these in the next version.
Thanks.

>
>
>> +enum stm32_dma_request_id {
>> +   STM32_DMA_REQUEST0,
>> +   STM32_DMA_REQUEST1,
>> +   STM32_DMA_REQUEST2,
>> +   STM32_DMA_REQUEST3,
>> +   STM32_DMA_REQUEST4,
>> +   STM32_DMA_REQUEST5,
>> +   STM32_DMA_REQUEST6,
>> +   STM32_DMA_REQUEST7,
>> +};
>
>
> Ditto.

As above, I will remove it in the next version. Thanks.

>
>
>> +static void stm32_dma_dump_reg(struct stm32_dma_chan *chan)
>> +{
>> +   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> +
>> +   dev_dbg(chan2dev(chan), "SCR:   0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "NDTR:  0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SNDTR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SPAR:  0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SPAR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SM0AR: 0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SM0AR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SM1AR: 0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SM1AR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SFCR:  0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id)));
>> +}
>
>
> Even at dev_dbg() this looks like log spam. This debug info gets issued
> every time we set up a transfer!

Yes, this info will be logged after each transfer.
But it will complete other debug info print when DMADEVICES_DEBUG is set.

>
>
>> +static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
>> +{
>> +   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
>> +   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> +   int ret;
>> +
>> +   chan->config_init = false;
>> +   ret = clk_prepare_enable(dmadev->clk);
>> +   if (ret < 0) {
>> +   dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n",
>> ret);
>> +   return ret;
>> +   }
>> +
>> +   ret = stm32_dma_disable_chan(chan);
>> +
>> +   return ret;
>> +}
>
>
> The error path here looks like it will leak clock references.

Sorry I didn't catch it. What does it mean ?

>
>
>
>> +static int stm32_dma_probe(struct platform_device *pdev)
>> +{
>> +   struct stm32_dma_chan *chan;
>> +   struct stm32_dma_device *dmadev;
>> +   struct dma_device *dd;
>> +   const struct of_device_id *match;
>> +   unsigned int i;
>> +   struct resource *res;
>> +   int ret;
>> +
>> +   match = of_match_device(stm32_dma_of_match, >dev);
>> +   if (!match) {
>> +   dev_err(>dev, "Error: No device match found\n");
>> +   return -ENODEV;
>> +   }
>> +
>> +   dmadev = devm_kzalloc(>dev, sizeof(*dmadev), GFP_KERNEL);
>> +   if (!dmadev)
>> +   return -ENOMEM;
>> +
>> +   dd = >ddev;
>> +
>> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +   dmadev->base = devm_ioremap_resource(>dev, res);
>> +   if (IS_ERR(dmadev->base))
>> +   return PTR_ERR(dmadev->base);
>> +
>> +   dmadev->clk = devm_clk_get(>dev, NULL);
>> +   if (IS_ERR(dmadev->clk)) {
>> +   dev_err(>dev, "Error: Missing controller clock\n");
>> +   return PTR_ERR(dmadev->clk);
>> +   }
>> +
>> +   dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
>> +   "st,mem2mem");
>> +
>> +   dmadev->rst = devm_reset_control_get(>dev, NULL);
>> +   if

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
2015-10-14 17:28 GMT+02:00 Daniel Thompson :
> On 14/10/15 16:26, M'boumba Cedric Madianga wrote:
>>
>> 2015-10-14 16:24 GMT+02:00 Daniel Thompson :
>>>
>>>
>>> Hmnnn...
>>>
>>> The dmaengine framework will WARN_ONCE() if an dmaengine is removed
>>> whilst
>>> it is active and also works hard to ensure dmaengine modules are not
>>> removed
>>> whilst there are active drivers using the framework.
>>>
>>> How do we get into this function whilst there is still an active DMA
>>> channels?
>>
>>
>> For example, when a user try "rmmod stm32-dma" in uart console.
>> It will enter in stm32_dma_remove while there is potentially still active
>> DMA.
>
>
> Check dmaengine.c for yourself but I think in this case the dmaengine
> framework will hold references to the module and prevent the remove from
> taking place.

Yes I did it.
As far I understand, the dmaengine framework will print a warning
message but doesn't stop removing operation if there are some active
clients.

Cedric.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 16:26, M'boumba Cedric Madianga wrote:

2015-10-14 16:24 GMT+02:00 Daniel Thompson :


Hmnnn...

The dmaengine framework will WARN_ONCE() if an dmaengine is removed whilst
it is active and also works hard to ensure dmaengine modules are not removed
whilst there are active drivers using the framework.

How do we get into this function whilst there is still an active DMA
channels?


For example, when a user try "rmmod stm32-dma" in uart console.
It will enter in stm32_dma_remove while there is potentially still active DMA.


Check dmaengine.c for yourself but I think in this case the dmaengine 
framework will hold references to the module and prevent the remove from 
taking place.



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Vinod Koul 
On Wed, Oct 14, 2015 at 03:07:16PM +0200, M'boumba Cedric Madianga wrote:
> >> +static inline uint32_t stm32_dma_read(struct stm32_dma_device *dmadev, 
> >> u32 reg)
> >
> > this and few other could be made more readable
> 
> Ok, I could replace uint32_t by u32. Is it what you expect ?
> For others, I don't see how I could make it more readable.
> Could you please give me more details to help me ? Thanks in advance

Yes that is one and also aplitting this up so that it fits within 80chars
while not sacrficing readablity...

> >> + } else if ((status & STM32_DMA_FEI) && (sfcr & STM32_DMA_SFCR_FEIE)) 
> >> {
> >> + dev_err(chan2dev(chan), "DMA error: received FEI event\n");
> >> + stm32_dma_irq_clear(chan, STM32_DMA_FEI);
> >> + chan->status = DMA_ERROR;
> >> + spin_unlock(>vchan.lock);
> >> + return IRQ_HANDLED;
> >
> > this is repeat of above apart from err print!!
> 
> Not only for print as we also need that to define which bit to clear
> in the IRQ status.
> In that way we will be sure to handle each interrupt event.

You can print error type based on status, or print the whole status but
handle it same for all three cases

> >> + enum dma_status status;
> >> + unsigned long flags;
> >> + unsigned int residue;
> >> +
> >> + status = dma_cookie_status(c, cookie, state);
> >> + if (status == DMA_COMPLETE)
> >> + return status;
> >> +
> >> + if (!state)
> >> + return chan->status;
> > why channel status and not status from dma_cookie_status()?
> 
> If status is different than DMA_COMPLETE it seems better to use channel 
> status.
> Indeed, in that way, we have the possibility to notify that there is a
> potential error in the bus via DMA_ERROR.
> But if I return status from dma_cookie_status(), I will always notify
> DMA_IN_PROGRESS.

No, you are supposed to return the descriptor status and not channel status!

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 14:41, M'boumba Cedric Madianga wrote:

2015-10-14 15:29 GMT+02:00 Daniel Thompson :

On 14/10/15 14:17, M'boumba Cedric Madianga wrote:


Hi Daniel,


+
+static int stm32_dma_remove(struct platform_device *pdev)
+{
+   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
+
+   of_dma_controller_free(pdev->dev.of_node);
+
+   dma_async_device_unregister(>ddev);
+
+   clk_disable_unprepare(dmadev->clk);




What is the purpose of disabling/unpreparing the clock here?
stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources()
should
pair up and the clock should already be stopped.



stm32_dma_remove() could be called during an on-going transfer during
module unload.
So in that case, it seems that disabling/unpreparing the clock is needed.



Really?

I think we need to be sure any on-going transfers are stopped.

There are multiple reasons for this, not least the risk of executing a
callback that has been freed, but the one related to my point is that a
single clk_disable_unprepare() will remain broken because if you don't know
that the transfers have stopped then you don't know how many on-going
transfers there are.


In the next version, I am going to stop the DMA, free all interrupts
and kill tasklet in stm32_dma_remove.
In that way, all on-going transfers will be lost as we don't have to
wait the end of remaining transfer in order to execute this function
as quickly as possible.


Hmnnn...

The dmaengine framework will WARN_ONCE() if an dmaengine is removed 
whilst it is active and also works hard to ensure dmaengine modules are 
not removed whilst there are active drivers using the framework.


How do we get into this function whilst there is still an active DMA 
channels?




But even with this improvement, I think I have to disable the clock here.


As above, I think the dmaengine framework work to protect you from this 
sort problem. However even if I am wrong about that then unconditionally 
calling clk_disable_unprepare() can not be used to reliably manage the 
clocks. You don't know what the counts are!



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
2015-10-14 16:24 GMT+02:00 Daniel Thompson :
>
> Hmnnn...
>
> The dmaengine framework will WARN_ONCE() if an dmaengine is removed whilst
> it is active and also works hard to ensure dmaengine modules are not removed
> whilst there are active drivers using the framework.
>
> How do we get into this function whilst there is still an active DMA
> channels?

For example, when a user try "rmmod stm32-dma" in uart console.
It will enter in stm32_dma_remove while there is potentially still active DMA.

>
>
>> But even with this improvement, I think I have to disable the clock here.
>
>
> As above, I think the dmaengine framework work to protect you from this sort
> problem. However even if I am wrong about that then unconditionally calling
> clk_disable_unprepare() can not be used to reliably manage the clocks. You
> don't know what the counts are!

Ok got it. Thanks for the clarification.

Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Daniel,

2015-10-13 16:34 GMT+02:00 Daniel Thompson :
> On 13/10/15 15:05, M'boumba Cedric Madianga wrote:
>>
>> This patch adds support for the STM32 DMA controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga 
>> ---
>>   drivers/dma/Kconfig |   12 +
>>   drivers/dma/Makefile|1 +
>>   drivers/dma/stm32-dma.c | 1175
>> +++
>>   3 files changed, 1188 insertions(+)
>>   create mode 100644 drivers/dma/stm32-dma.c
>
>
>> diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
>> new file mode 100644
>> index 000..031bab7
>> --- /dev/null
>> +++ b/drivers/dma/stm32-dma.c
>
>
>> +enum stm32_dma_width {
>> +   STM32_DMA_BYTE,
>> +   STM32_DMA_HALF_WORD,
>> +   STM32_DMA_WORD,
>> +};
>> +
>> +enum stm32_dma_burst_size {
>> +   STM32_DMA_BURST_SINGLE,
>> +   STM32_DMA_BURST_INCR4,
>> +   STM32_DMA_BURST_INCR8,
>> +   STM32_DMA_BURST_INCR16,
>> +};
>> +
>> +enum stm32_dma_channel_id {
>> +   STM32_DMA_CHANNEL0,
>> +   STM32_DMA_CHANNEL1,
>> +   STM32_DMA_CHANNEL2,
>> +   STM32_DMA_CHANNEL3,
>> +   STM32_DMA_CHANNEL4,
>> +   STM32_DMA_CHANNEL5,
>> +   STM32_DMA_CHANNEL6,
>> +   STM32_DMA_CHANNEL7,
>> +};
>
>
> Why have (unused) enumerations to map numeric symbols to their own natural
> values? Using normal integers would be better.

Good point. I will remove these in the next version.
Thanks.

>
>
>> +enum stm32_dma_request_id {
>> +   STM32_DMA_REQUEST0,
>> +   STM32_DMA_REQUEST1,
>> +   STM32_DMA_REQUEST2,
>> +   STM32_DMA_REQUEST3,
>> +   STM32_DMA_REQUEST4,
>> +   STM32_DMA_REQUEST5,
>> +   STM32_DMA_REQUEST6,
>> +   STM32_DMA_REQUEST7,
>> +};
>
>
> Ditto.

As above, I will remove it in the next version. Thanks.

>
>
>> +static void stm32_dma_dump_reg(struct stm32_dma_chan *chan)
>> +{
>> +   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> +
>> +   dev_dbg(chan2dev(chan), "SCR:   0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "NDTR:  0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SNDTR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SPAR:  0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SPAR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SM0AR: 0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SM0AR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SM1AR: 0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SM1AR(chan->id)));
>> +   dev_dbg(chan2dev(chan), "SFCR:  0x%08x\n",
>> +   stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id)));
>> +}
>
>
> Even at dev_dbg() this looks like log spam. This debug info gets issued
> every time we set up a transfer!

Yes, this info will be logged after each transfer.
But it will complete other debug info print when DMADEVICES_DEBUG is set.

>
>
>> +static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
>> +{
>> +   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
>> +   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> +   int ret;
>> +
>> +   chan->config_init = false;
>> +   ret = clk_prepare_enable(dmadev->clk);
>> +   if (ret < 0) {
>> +   dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n",
>> ret);
>> +   return ret;
>> +   }
>> +
>> +   ret = stm32_dma_disable_chan(chan);
>> +
>> +   return ret;
>> +}
>
>
> The error path here looks like it will leak clock references.

Sorry I didn't catch it. What does it mean ?

>
>
>
>> +static int stm32_dma_probe(struct platform_device *pdev)
>> +{
>> +   struct stm32_dma_chan *chan;
>> +   struct stm32_dma_device *dmadev;
>> +   struct dma_device *dd;
>> +   const struct of_device_id *match;
>> +   unsigned int i;
>> +   struct resource *res;
>> +   int ret;
>> +
>> +   match = of_match_device(stm32_dma_of_match, >dev);
>> +   if (!match) {
>> +   dev_err(>dev, "Error: No device match found\n");
>> +   return -ENODEV;
>> +   }
>> +
>> +   dmadev = devm_kzalloc(>dev, sizeof(*dmadev), GFP_KERNEL);
>> +   if (!dmadev)
>> +   return -ENOMEM;
>> +
>> +   dd = >ddev;
>> +
>> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +   dmadev->base = devm_ioremap_resource(>dev, res);
>> +   if (IS_ERR(dmadev->base))
>> +   return PTR_ERR(dmadev->base);
>> +
>> +   dmadev->clk = devm_clk_get(>dev, NULL);
>> +   if (IS_ERR(dmadev->clk)) {
>> +   dev_err(>dev, "Error: Missing controller clock\n");
>> +   return PTR_ERR(dmadev->clk);
>> +   }
>> +
>> +   dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
>> +   "st,mem2mem");
>> +
>> +   dmadev->rst =

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 08:54, M'boumba Cedric Madianga wrote:

+static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
+{
+   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
+   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
+   int ret;
+
+   chan->config_init = false;
+   ret = clk_prepare_enable(dmadev->clk);
+   if (ret < 0) {
+   dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n",
ret);
+   return ret;
+   }
+
+   ret = stm32_dma_disable_chan(chan);
+
+   return ret;
+}



The error path here looks like it will leak clock references.


Sorry I didn't catch it. What does it mean ?


If stm32_dma_disable_chan() returns an error then we will not restore 
the original the clock counts causing them to "leak".



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Daniel,

2015-10-14 10:52 GMT+02:00 Daniel Thompson :
> On 14/10/15 08:54, M'boumba Cedric Madianga wrote:

 +static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
 +{
 +   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
 +   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
 +   int ret;
 +
 +   chan->config_init = false;
 +   ret = clk_prepare_enable(dmadev->clk);
 +   if (ret < 0) {
 +   dev_err(chan2dev(chan), "clk_prepare_enable failed:
 %d\n",
 ret);
 +   return ret;
 +   }
 +
 +   ret = stm32_dma_disable_chan(chan);
 +
 +   return ret;
 +}
>>>
>
>
> If stm32_dma_disable_chan() returns an error then we will not restore the
> original the clock counts causing them to "leak".

Ok got it. Thanks for the clarification.
I will fix it in the next version.

BR,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Vinod Koul 
On Tue, Oct 13, 2015 at 04:05:25PM +0200, M'boumba Cedric Madianga wrote:
> +#define STM32_DMA_LISR   0x /* DMA Low Int Status 
> Reg */
> +#define STM32_DMA_HISR   0x0004 /* DMA High Int Status 
> Reg */
> +#define STM32_DMA_LIFCR  0x0008 /* DMA Low Int Flag 
> Clear Reg */
> +#define STM32_DMA_HIFCR  0x000c /* DMA High Int Flag 
> Clear Reg */
> +#define STM32_DMA_TCIBIT(5) /* Transfer Complete 
> Interrupt */
> +#define STM32_DMA_HTIBIT(4) /* Half Transfer 
> Interrupt */
> +#define STM32_DMA_TEIBIT(3) /* Transfer Error 
> Interrupt */
> +#define STM32_DMA_DMEI   BIT(2) /* Direct Mode Error 
> Interrupt */
> +#define STM32_DMA_FEIBIT(0) /* FIFO Error Interrupt 
> */

Why not use BIT() for everything here and make it consistent

Also where ever possible stick to 80 char limit like above you can

> +
> +/* DMA Stream x Configuration Register */
> +#define STM32_DMA_SCR(x) (0x0010 + 0x18 * (x)) /* x = 0..7 */
> +#define STM32_DMA_SCR_REQ(n) ((n & 0x7) << 25)

this and below looks ugly and hard to maintain, are you sure spec doesn't
have a formulae for these?

> +static inline uint32_t stm32_dma_read(struct stm32_dma_device *dmadev, u32 
> reg)

this and few other could be made more readable

> +static struct stm32_dma_desc *stm32_dma_alloc_desc(unsigned int num_sgs)
> +{
> + return kzalloc(sizeof(struct stm32_dma_desc) +
> +sizeof(struct stm32_dma_sg_req) * num_sgs, GFP_ATOMIC);

Not GFP_NOWAIT ?

> +static enum stm32_dma_width stm32_get_dma_width(struct stm32_dma_chan *chan,
> + enum dma_slave_buswidth width)
> +{
> + switch (width) {
> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
> + return STM32_DMA_BYTE;
> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> + return STM32_DMA_HALF_WORD;
> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> + return STM32_DMA_WORD;
> + default:
> + dev_warn(chan2dev(chan),
> +  "Dma bus width not supported, using 32bits\n");
> + return STM32_DMA_WORD;

pls return error here
Assuming wrong parameter can cause havoc of transfer, so is not advisable

> +static enum stm32_dma_burst_size stm32_get_dma_burst(
> + struct stm32_dma_chan *chan, u32 maxburst)
> +{
> + switch (maxburst) {
> + case 0:
> + case 1:
> + return STM32_DMA_BURST_SINGLE;
> + case 4:
> + return STM32_DMA_BURST_INCR4;
> + case 8:
> + return STM32_DMA_BURST_INCR8;
> + case 16:
> + return STM32_DMA_BURST_INCR16;
> + default:
> + dev_warn(chan2dev(chan),
> +  "Dma burst size not supported, using single\n");
> + return STM32_DMA_BURST_SINGLE;

here too

> + }
> +}
> +
> +static int stm32_dma_slave_config(struct dma_chan *c,
> +   struct dma_slave_config *config)
> +{
> + struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
> +
> + if (chan->busy) {
> + dev_err(chan2dev(chan), "Configuration not allowed\n");
> + return -EBUSY;
> + }

That is false condition. This configuration should be used for next
descriptor prepare

> +static int stm32_dma_disable_chan(struct stm32_dma_chan *chan)
> +{
> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
> + unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> + u32 dma_scr;
> +
> + dma_scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
> + if (dma_scr & STM32_DMA_SCR_EN) {
> + dma_scr &= ~STM32_DMA_SCR_EN;
> + stm32_dma_write(dmadev, STM32_DMA_SCR(chan->id), dma_scr);
> +
> + do {
> + dma_scr = stm32_dma_read(dmadev,
> +  STM32_DMA_SCR(chan->id));
> + dma_scr &= STM32_DMA_SCR_EN;
> + if (!dma_scr)
> + break;

empty line here would improve readability

> +static irqreturn_t stm32_dma_chan_irq(int irq, void *devid)
> +{
> + struct stm32_dma_chan *chan = devid;
> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
> + u32 status, scr, sfcr;
> +
> + spin_lock(>vchan.lock);
> +
> + status = stm32_dma_irq_status(chan);
> + scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
> + sfcr = stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id));
> +
> + if ((status & STM32_DMA_HTI) && (scr & STM32_DMA_SCR_HTIE)) {
> + stm32_dma_irq_clear(chan, STM32_DMA_HTI);
> + vchan_cyclic_callback(>desc->vdesc);
> + spin_unlock(>vchan.lock);
> + return IRQ_HANDLED;

line here please and below

> + } else if ((status & STM32_DMA_TCI) && (scr &

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Daniel Thompson 

On 14/10/15 14:17, M'boumba Cedric Madianga wrote:

Hi Daniel,


+
+static int stm32_dma_remove(struct platform_device *pdev)
+{
+   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
+
+   of_dma_controller_free(pdev->dev.of_node);
+
+   dma_async_device_unregister(>ddev);
+
+   clk_disable_unprepare(dmadev->clk);



What is the purpose of disabling/unpreparing the clock here?
stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources() should
pair up and the clock should already be stopped.


stm32_dma_remove() could be called during an on-going transfer during
module unload.
So in that case, it seems that disabling/unpreparing the clock is needed.


Really?

I think we need to be sure any on-going transfers are stopped.

There are multiple reasons for this, not least the risk of executing a 
callback that has been freed, but the one related to my point is that a 
single clk_disable_unprepare() will remain broken because if you don't 
know that the transfers have stopped then you don't know how many 
on-going transfers there are.



Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
2015-10-14 15:29 GMT+02:00 Daniel Thompson :
> On 14/10/15 14:17, M'boumba Cedric Madianga wrote:
>>
>> Hi Daniel,
>>
> +
> +static int stm32_dma_remove(struct platform_device *pdev)
> +{
> +   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
> +
> +   of_dma_controller_free(pdev->dev.of_node);
> +
> +   dma_async_device_unregister(>ddev);
> +
> +   clk_disable_unprepare(dmadev->clk);



 What is the purpose of disabling/unpreparing the clock here?
 stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources()
 should
 pair up and the clock should already be stopped.
>>
>>
>> stm32_dma_remove() could be called during an on-going transfer during
>> module unload.
>> So in that case, it seems that disabling/unpreparing the clock is needed.
>
>
> Really?
>
> I think we need to be sure any on-going transfers are stopped.
>
> There are multiple reasons for this, not least the risk of executing a
> callback that has been freed, but the one related to my point is that a
> single clk_disable_unprepare() will remain broken because if you don't know
> that the transfers have stopped then you don't know how many on-going
> transfers there are.

In the next version, I am going to stop the DMA, free all interrupts
and kill tasklet in stm32_dma_remove.
In that way, all on-going transfers will be lost as we don't have to
wait the end of remaining transfer in order to execute this function
as quickly as possible.
But even with this improvement, I think I have to disable the clock here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread Vinod Koul 
On Wed, Oct 14, 2015 at 05:41:26PM +0200, M'boumba Cedric Madianga wrote:
> 2015-10-14 17:28 GMT+02:00 Daniel Thompson :
> > On 14/10/15 16:26, M'boumba Cedric Madianga wrote:
> >>
> >> 2015-10-14 16:24 GMT+02:00 Daniel Thompson :
> >>>
> >>>
> >>> Hmnnn...
> >>>
> >>> The dmaengine framework will WARN_ONCE() if an dmaengine is removed
> >>> whilst
> >>> it is active and also works hard to ensure dmaengine modules are not
> >>> removed
> >>> whilst there are active drivers using the framework.
> >>>
> >>> How do we get into this function whilst there is still an active DMA
> >>> channels?
> >>
> >>
> >> For example, when a user try "rmmod stm32-dma" in uart console.
> >> It will enter in stm32_dma_remove while there is potentially still active
> >> DMA.
> >
> >
> > Check dmaengine.c for yourself but I think in this case the dmaengine
> > framework will hold references to the module and prevent the remove from
> > taking place.
> 
> Yes I did it.
> As far I understand, the dmaengine framework will print a warning
> message but doesn't stop removing operation if there are some active
> clients.

we hold a ref, so removal wont work, see dma_chan_get()

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Vinod,


2015-10-14 13:16 GMT+02:00 Vinod Koul :
> On Tue, Oct 13, 2015 at 04:05:25PM +0200, M'boumba Cedric Madianga wrote:
>> +#define STM32_DMA_LISR   0x /* DMA Low Int Status 
>> Reg */
>> +#define STM32_DMA_HISR   0x0004 /* DMA High Int Status 
>> Reg */
>> +#define STM32_DMA_LIFCR  0x0008 /* DMA Low Int Flag 
>> Clear Reg */
>> +#define STM32_DMA_HIFCR  0x000c /* DMA High Int Flag 
>> Clear Reg */
>> +#define STM32_DMA_TCIBIT(5) /* Transfer Complete 
>> Interrupt */
>> +#define STM32_DMA_HTIBIT(4) /* Half Transfer 
>> Interrupt */
>> +#define STM32_DMA_TEIBIT(3) /* Transfer Error 
>> Interrupt */
>> +#define STM32_DMA_DMEI   BIT(2) /* Direct Mode Error 
>> Interrupt */
>> +#define STM32_DMA_FEIBIT(0) /* FIFO Error Interrupt 
>> */
>
> Why not use BIT() for everything here and make it consistent
>
> Also where ever possible stick to 80 char limit like above you can

In fact STM32_DMA_LISR, HISR, LIFCR and HIFCR are offset to access register.
So BIT() could not be used for this purpose.

>
>> +
>> +/* DMA Stream x Configuration Register */
>> +#define STM32_DMA_SCR(x) (0x0010 + 0x18 * (x)) /* x = 0..7 */
>> +#define STM32_DMA_SCR_REQ(n) ((n & 0x7) << 25)
>
> this and below looks ugly and hard to maintain, are you sure spec doesn't
> have a formulae for these?

We don't have any formulae in spec to handle this.
It is clearly described that we have to access registers in that way
to address the good channel.
Please, see below an extract of the spec for more details:
DMA stream x configuration register (DMA_SxCR) (x = 0..7)
This register is used to configure the concerned stream.
Address offset: 0x10 + 0x18 × stream number

>
>> +static inline uint32_t stm32_dma_read(struct stm32_dma_device *dmadev, u32 
>> reg)
>
> this and few other could be made more readable

Ok, I could replace uint32_t by u32. Is it what you expect ?
For others, I don't see how I could make it more readable.
Could you please give me more details to help me ? Thanks in advance

>
>> +static struct stm32_dma_desc *stm32_dma_alloc_desc(unsigned int num_sgs)
>> +{
>> + return kzalloc(sizeof(struct stm32_dma_desc) +
>> +sizeof(struct stm32_dma_sg_req) * num_sgs, GFP_ATOMIC);
>
> Not GFP_NOWAIT ?

You're right. We could use GFP_NOWAIT instead. Thanks.

>
>> +static enum stm32_dma_width stm32_get_dma_width(struct stm32_dma_chan *chan,
>> + enum dma_slave_buswidth width)
>> +{
>> + switch (width) {
>> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> + return STM32_DMA_BYTE;
>> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> + return STM32_DMA_HALF_WORD;
>> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> + return STM32_DMA_WORD;
>> + default:
>> + dev_warn(chan2dev(chan),
>> +  "Dma bus width not supported, using 32bits\n");
>> + return STM32_DMA_WORD;
>
> pls return error here
> Assuming wrong parameter can cause havoc of transfer, so is not advisable

OK, I will fix it in the next version. Thanks for the explanation.

>
>> +static enum stm32_dma_burst_size stm32_get_dma_burst(
>> + struct stm32_dma_chan *chan, u32 maxburst)
>> +{
>> + switch (maxburst) {
>> + case 0:
>> + case 1:
>> + return STM32_DMA_BURST_SINGLE;
>> + case 4:
>> + return STM32_DMA_BURST_INCR4;
>> + case 8:
>> + return STM32_DMA_BURST_INCR8;
>> + case 16:
>> + return STM32_DMA_BURST_INCR16;
>> + default:
>> + dev_warn(chan2dev(chan),
>> +  "Dma burst size not supported, using single\n");
>> + return STM32_DMA_BURST_SINGLE;
>
> here too

Ditto

>
>> + }
>> +}
>> +
>> +static int stm32_dma_slave_config(struct dma_chan *c,
>> +   struct dma_slave_config *config)
>> +{
>> + struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
>> +
>> + if (chan->busy) {
>> + dev_err(chan2dev(chan), "Configuration not allowed\n");
>> + return -EBUSY;
>> + }
>
> That is false condition. This configuration should be used for next
> descriptor prepare

OK. Will be fixed in the next patch version.

>
>> +static int stm32_dma_disable_chan(struct stm32_dma_chan *chan)
>> +{
>> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> + unsigned long timeout = jiffies + msecs_to_jiffies(5000);
>> + u32 dma_scr;
>> +
>> + dma_scr = stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id));
>> + if (dma_scr & STM32_DMA_SCR_EN) {
>> + dma_scr &= ~STM32_DMA_SCR_EN;
>> + stm32_dma_write(dmadev, STM32_DMA_SCR(chan->id), dma_scr);
>> +
>> + do {
>> + dma_scr =

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-14 Thread M'boumba Cedric Madianga 
Hi Daniel,

>>> +
>>> +static int stm32_dma_remove(struct platform_device *pdev)
>>> +{
>>> +   struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
>>> +
>>> +   of_dma_controller_free(pdev->dev.of_node);
>>> +
>>> +   dma_async_device_unregister(>ddev);
>>> +
>>> +   clk_disable_unprepare(dmadev->clk);
>>
>>
>> What is the purpose of disabling/unpreparing the clock here?
>> stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources() should
>> pair up and the clock should already be stopped.

stm32_dma_remove() could be called during an on-going transfer during
module unload.
So in that case, it seems that disabling/unpreparing the clock is needed.

BR,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-13 Thread Daniel Thompson 

On 13/10/15 15:05, M'boumba Cedric Madianga wrote:

This patch adds support for the STM32 DMA controller.

Signed-off-by: M'boumba Cedric Madianga 
---
  drivers/dma/Kconfig |   12 +
  drivers/dma/Makefile|1 +
  drivers/dma/stm32-dma.c | 1175 +++
  3 files changed, 1188 insertions(+)
  create mode 100644 drivers/dma/stm32-dma.c



diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
new file mode 100644
index 000..031bab7
--- /dev/null
+++ b/drivers/dma/stm32-dma.c



+enum stm32_dma_width {
+   STM32_DMA_BYTE,
+   STM32_DMA_HALF_WORD,
+   STM32_DMA_WORD,
+};
+
+enum stm32_dma_burst_size {
+   STM32_DMA_BURST_SINGLE,
+   STM32_DMA_BURST_INCR4,
+   STM32_DMA_BURST_INCR8,
+   STM32_DMA_BURST_INCR16,
+};
+
+enum stm32_dma_channel_id {
+   STM32_DMA_CHANNEL0,
+   STM32_DMA_CHANNEL1,
+   STM32_DMA_CHANNEL2,
+   STM32_DMA_CHANNEL3,
+   STM32_DMA_CHANNEL4,
+   STM32_DMA_CHANNEL5,
+   STM32_DMA_CHANNEL6,
+   STM32_DMA_CHANNEL7,
+};


Why have (unused) enumerations to map numeric symbols to their own 
natural values? Using normal integers would be better.




+enum stm32_dma_request_id {
+   STM32_DMA_REQUEST0,
+   STM32_DMA_REQUEST1,
+   STM32_DMA_REQUEST2,
+   STM32_DMA_REQUEST3,
+   STM32_DMA_REQUEST4,
+   STM32_DMA_REQUEST5,
+   STM32_DMA_REQUEST6,
+   STM32_DMA_REQUEST7,
+};


Ditto.



+static void stm32_dma_dump_reg(struct stm32_dma_chan *chan)
+{
+   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
+
+   dev_dbg(chan2dev(chan), "SCR:   0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id)));
+   dev_dbg(chan2dev(chan), "NDTR:  0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SNDTR(chan->id)));
+   dev_dbg(chan2dev(chan), "SPAR:  0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SPAR(chan->id)));
+   dev_dbg(chan2dev(chan), "SM0AR: 0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SM0AR(chan->id)));
+   dev_dbg(chan2dev(chan), "SM1AR: 0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SM1AR(chan->id)));
+   dev_dbg(chan2dev(chan), "SFCR:  0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id)));
+}


Even at dev_dbg() this looks like log spam. This debug info gets issued 
every time we set up a transfer!




+static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
+{
+   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
+   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
+   int ret;
+
+   chan->config_init = false;
+   ret = clk_prepare_enable(dmadev->clk);
+   if (ret < 0) {
+   dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n", ret);
+   return ret;
+   }
+
+   ret = stm32_dma_disable_chan(chan);
+
+   return ret;
+}


The error path here looks like it will leak clock references.



+static int stm32_dma_probe(struct platform_device *pdev)
+{
+   struct stm32_dma_chan *chan;
+   struct stm32_dma_device *dmadev;
+   struct dma_device *dd;
+   const struct of_device_id *match;
+   unsigned int i;
+   struct resource *res;
+   int ret;
+
+   match = of_match_device(stm32_dma_of_match, >dev);
+   if (!match) {
+   dev_err(>dev, "Error: No device match found\n");
+   return -ENODEV;
+   }
+
+   dmadev = devm_kzalloc(>dev, sizeof(*dmadev), GFP_KERNEL);
+   if (!dmadev)
+   return -ENOMEM;
+
+   dd = >ddev;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   dmadev->base = devm_ioremap_resource(>dev, res);
+   if (IS_ERR(dmadev->base))
+   return PTR_ERR(dmadev->base);
+
+   dmadev->clk = devm_clk_get(>dev, NULL);
+   if (IS_ERR(dmadev->clk)) {
+   dev_err(>dev, "Error: Missing controller clock\n");
+   return PTR_ERR(dmadev->clk);
+   }
+
+   dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
+   "st,mem2mem");
+
+   dmadev->rst = devm_reset_control_get(>dev, NULL);
+   if (!IS_ERR(dmadev->rst)) {
+   reset_control_assert(dmadev->rst);
+   udelay(2);
+   reset_control_deassert(dmadev->rst);
+   }
+
+   dma_cap_set(DMA_SLAVE, dd->cap_mask);
+   dma_cap_set(DMA_PRIVATE, dd->cap_mask);
+   dma_cap_set(DMA_CYCLIC, dd->cap_mask);
+   dd->device_alloc_chan_resources = stm32_dma_alloc_chan_resources;
+   dd->device_free_chan_resources = stm32_dma_free_chan_resources;
+   dd->device_tx_status = stm32_dma_tx_status;
+   dd->device_issue_pending = stm32_dma_issue_pending;
+   dd->device_prep_slave_sg = stm32_dma_prep_slave_sg;
+   dd->device_prep_dma_cyclic = stm32_dma_prep_dma_cyclic;
+   dd->device_config = stm32_dma_slave_config;
+

Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

2015-10-13 Thread Daniel Thompson 

On 13/10/15 15:05, M'boumba Cedric Madianga wrote:

This patch adds support for the STM32 DMA controller.

Signed-off-by: M'boumba Cedric Madianga 
---
  drivers/dma/Kconfig |   12 +
  drivers/dma/Makefile|1 +
  drivers/dma/stm32-dma.c | 1175 +++
  3 files changed, 1188 insertions(+)
  create mode 100644 drivers/dma/stm32-dma.c



diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
new file mode 100644
index 000..031bab7
--- /dev/null
+++ b/drivers/dma/stm32-dma.c



+enum stm32_dma_width {
+   STM32_DMA_BYTE,
+   STM32_DMA_HALF_WORD,
+   STM32_DMA_WORD,
+};
+
+enum stm32_dma_burst_size {
+   STM32_DMA_BURST_SINGLE,
+   STM32_DMA_BURST_INCR4,
+   STM32_DMA_BURST_INCR8,
+   STM32_DMA_BURST_INCR16,
+};
+
+enum stm32_dma_channel_id {
+   STM32_DMA_CHANNEL0,
+   STM32_DMA_CHANNEL1,
+   STM32_DMA_CHANNEL2,
+   STM32_DMA_CHANNEL3,
+   STM32_DMA_CHANNEL4,
+   STM32_DMA_CHANNEL5,
+   STM32_DMA_CHANNEL6,
+   STM32_DMA_CHANNEL7,
+};


Why have (unused) enumerations to map numeric symbols to their own 
natural values? Using normal integers would be better.




+enum stm32_dma_request_id {
+   STM32_DMA_REQUEST0,
+   STM32_DMA_REQUEST1,
+   STM32_DMA_REQUEST2,
+   STM32_DMA_REQUEST3,
+   STM32_DMA_REQUEST4,
+   STM32_DMA_REQUEST5,
+   STM32_DMA_REQUEST6,
+   STM32_DMA_REQUEST7,
+};


Ditto.



+static void stm32_dma_dump_reg(struct stm32_dma_chan *chan)
+{
+   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
+
+   dev_dbg(chan2dev(chan), "SCR:   0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id)));
+   dev_dbg(chan2dev(chan), "NDTR:  0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SNDTR(chan->id)));
+   dev_dbg(chan2dev(chan), "SPAR:  0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SPAR(chan->id)));
+   dev_dbg(chan2dev(chan), "SM0AR: 0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SM0AR(chan->id)));
+   dev_dbg(chan2dev(chan), "SM1AR: 0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SM1AR(chan->id)));
+   dev_dbg(chan2dev(chan), "SFCR:  0x%08x\n",
+   stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id)));
+}


Even at dev_dbg() this looks like log spam. This debug info gets issued 
every time we set up a transfer!




+static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
+{
+   struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
+   struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
+   int ret;
+
+   chan->config_init = false;
+   ret = clk_prepare_enable(dmadev->clk);
+   if (ret < 0) {
+   dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n", ret);
+   return ret;
+   }
+
+   ret = stm32_dma_disable_chan(chan);
+
+   return ret;
+}


The error path here looks like it will leak clock references.



+static int stm32_dma_probe(struct platform_device *pdev)
+{
+   struct stm32_dma_chan *chan;
+   struct stm32_dma_device *dmadev;
+   struct dma_device *dd;
+   const struct of_device_id *match;
+   unsigned int i;
+   struct resource *res;
+   int ret;
+
+   match = of_match_device(stm32_dma_of_match, >dev);
+   if (!match) {
+   dev_err(>dev, "Error: No device match found\n");
+   return -ENODEV;
+   }
+
+   dmadev = devm_kzalloc(>dev, sizeof(*dmadev), GFP_KERNEL);
+   if (!dmadev)
+   return -ENOMEM;
+
+   dd = >ddev;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   dmadev->base = devm_ioremap_resource(>dev, res);
+   if (IS_ERR(dmadev->base))
+   return PTR_ERR(dmadev->base);
+
+   dmadev->clk = devm_clk_get(>dev, NULL);
+   if (IS_ERR(dmadev->clk)) {
+   dev_err(>dev, "Error: Missing controller clock\n");
+   return PTR_ERR(dmadev->clk);
+   }
+
+   dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
+   "st,mem2mem");
+
+   dmadev->rst = devm_reset_control_get(>dev, NULL);
+   if (!IS_ERR(dmadev->rst)) {
+   reset_control_assert(dmadev->rst);
+   udelay(2);
+   reset_control_deassert(dmadev->rst);
+   }
+
+   dma_cap_set(DMA_SLAVE, dd->cap_mask);
+   dma_cap_set(DMA_PRIVATE, dd->cap_mask);
+   dma_cap_set(DMA_CYCLIC, dd->cap_mask);
+   dd->device_alloc_chan_resources = stm32_dma_alloc_chan_resources;
+   dd->device_free_chan_resources = stm32_dma_free_chan_resources;
+   dd->device_tx_status = stm32_dma_tx_status;
+   dd->device_issue_pending = stm32_dma_issue_pending;
+   dd->device_prep_slave_sg = stm32_dma_prep_slave_sg;
+   dd->device_prep_dma_cyclic = stm32_dma_prep_dma_cyclic;
+   dd->device_config =