Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1578515 > unrolled thread
| Started by | <jiada_wang@mentor.com> |
|---|---|
| First post | 2017-02-10 15:50 +0100 |
| Last post | 2017-02-13 11:30 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped <jiada_wang@mentor.com> - 2017-02-10 15:50 +0100
Re: [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped Vinod Koul <vinod.koul@intel.com> - 2017-02-13 03:10 +0100
Re: [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped Jiada Wang <jiada_wang@mentor.com> - 2017-02-13 07:40 +0100
Re: [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped Vinod Koul <vinod.koul@intel.com> - 2017-02-13 11:30 +0100
| From | <jiada_wang@mentor.com> |
|---|---|
| Date | 2017-02-10 15:50 +0100 |
| Subject | [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped |
| Message-ID | <t9kGl-4F0-1@gated-at.bofh.it> |
From: Jiada Wang <jiada_wang@mentor.com>
sdma_disable_channel() cannot ensure dma is stopped to access
module's FIFOs. Maybe SDMA core is running and accessing BD when
disable of corresponding channel, this may cause sometimes even
after call of .sdma_disable_channel(), SDMA core still be running
and accessing module's FIFOs.
We should add delay of one BD SDMA cost time, the maximum is 1ms.
So that SDMA clients by calling .device_terminate_all can
ensure SDMA core has really been stopped.
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/dma/imx-sdma.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index d1651a5..7332c40 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -937,6 +937,14 @@ static int sdma_disable_channel(struct dma_chan *chan)
return 0;
}
+static int sdma_disable_channel_with_delay(struct dma_chan *chan)
+{
+ sdma_disable_channel(chan);
+ mdelay(1);
+
+ return 0;
+}
+
static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
{
struct sdma_engine *sdma = sdmac->sdma;
@@ -1828,7 +1836,7 @@ static int sdma_probe(struct platform_device *pdev)
sdma->dma_device.device_prep_slave_sg = sdma_prep_slave_sg;
sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
sdma->dma_device.device_config = sdma_config;
- sdma->dma_device.device_terminate_all = sdma_disable_channel;
+ sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
--
2.7.4
[toc] | [next] | [standalone]
| From | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| Date | 2017-02-13 03:10 +0100 |
| Subject | Re: [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped |
| Message-ID | <taefw-5FQ-7@gated-at.bofh.it> |
| In reply to | #1578515 |
On Fri, Feb 10, 2017 at 06:46:45AM -0800, jiada_wang@mentor.com wrote:
> From: Jiada Wang <jiada_wang@mentor.com>
>
> sdma_disable_channel() cannot ensure dma is stopped to access
> module's FIFOs. Maybe SDMA core is running and accessing BD when
> disable of corresponding channel, this may cause sometimes even
> after call of .sdma_disable_channel(), SDMA core still be running
> and accessing module's FIFOs.
>
> We should add delay of one BD SDMA cost time, the maximum is 1ms.
> So that SDMA clients by calling .device_terminate_all can
> ensure SDMA core has really been stopped.
>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
> drivers/dma/imx-sdma.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index d1651a5..7332c40 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -937,6 +937,14 @@ static int sdma_disable_channel(struct dma_chan *chan)
> return 0;
> }
>
> +static int sdma_disable_channel_with_delay(struct dma_chan *chan)
> +{
> + sdma_disable_channel(chan);
> + mdelay(1);
what is the gaurantee that 1ms is fine? Shouldn't you poll the bit to see
channel is disabled properly..
> +
> + return 0;
> +}
> +
> static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
> {
> struct sdma_engine *sdma = sdmac->sdma;
> @@ -1828,7 +1836,7 @@ static int sdma_probe(struct platform_device *pdev)
> sdma->dma_device.device_prep_slave_sg = sdma_prep_slave_sg;
> sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
> sdma->dma_device.device_config = sdma_config;
> - sdma->dma_device.device_terminate_all = sdma_disable_channel;
> + sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
> sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> --
> 2.7.4
>
>
--
~Vinod
[toc] | [prev] | [next] | [standalone]
| From | Jiada Wang <jiada_wang@mentor.com> |
|---|---|
| Date | 2017-02-13 07:40 +0100 |
| Subject | Re: [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped |
| Message-ID | <taisN-8sW-5@gated-at.bofh.it> |
| In reply to | #1579355 |
Hello Vinod
On 02/13/2017 11:05 AM, Vinod Koul wrote:
> On Fri, Feb 10, 2017 at 06:46:45AM -0800, jiada_wang@mentor.com wrote:
>> From: Jiada Wang <jiada_wang@mentor.com>
>>
>> sdma_disable_channel() cannot ensure dma is stopped to access
>> module's FIFOs. Maybe SDMA core is running and accessing BD when
>> disable of corresponding channel, this may cause sometimes even
>> after call of .sdma_disable_channel(), SDMA core still be running
>> and accessing module's FIFOs.
>>
>> We should add delay of one BD SDMA cost time, the maximum is 1ms.
>> So that SDMA clients by calling .device_terminate_all can
>> ensure SDMA core has really been stopped.
>>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>> drivers/dma/imx-sdma.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index d1651a5..7332c40 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -937,6 +937,14 @@ static int sdma_disable_channel(struct dma_chan *chan)
>> return 0;
>> }
>>
>> +static int sdma_disable_channel_with_delay(struct dma_chan *chan)
>> +{
>> + sdma_disable_channel(chan);
>> + mdelay(1);
>
> what is the gaurantee that 1ms is fine? Shouldn't you poll the bit to see
> channel is disabled properly..
>
I got the information from NXP (freescale) R&D team,
according to them, by write '1' to SDMA_H_STATSTOP, only disables
the related sdma channel (so poll HE bit will indicates the channel has
been disabled),
but it cannot ensure SDMA core stop to access modules' FIFO,
SDMA core may still is running, this is a bug in HW.
regarding if the '1ms' is enough to ensure SDMA core has stopped,
NXP R&D team mentioned:
"we should add some delay of one BD SDMA cost time after disable the
channel bit, the maximum is 1ms"
so I assume 1ms should work for all cases
Thanks,
Jiada
>> +
>> + return 0;
>> +}
>> +
>> static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
>> {
>> struct sdma_engine *sdma = sdmac->sdma;
>> @@ -1828,7 +1836,7 @@ static int sdma_probe(struct platform_device *pdev)
>> sdma->dma_device.device_prep_slave_sg = sdma_prep_slave_sg;
>> sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
>> sdma->dma_device.device_config = sdma_config;
>> - sdma->dma_device.device_terminate_all = sdma_disable_channel;
>> + sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
>> sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
>> --
>> 2.7.4
>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| Date | 2017-02-13 11:30 +0100 |
| Subject | Re: [PATCH 1/1] dma: imx-sdma: add 1ms delay to ensure SDMA channel is stopped |
| Message-ID | <tam3p-2qv-33@gated-at.bofh.it> |
| In reply to | #1579464 |
On Mon, Feb 13, 2017 at 03:30:19PM +0900, Jiada Wang wrote:
> >>+static int sdma_disable_channel_with_delay(struct dma_chan *chan)
> >>+{
> >>+ sdma_disable_channel(chan);
> >>+ mdelay(1);
> >
> >what is the gaurantee that 1ms is fine? Shouldn't you poll the bit to see
> >channel is disabled properly..
> >
> I got the information from NXP (freescale) R&D team,
> according to them, by write '1' to SDMA_H_STATSTOP, only disables
> the related sdma channel (so poll HE bit will indicates the channel
> has been disabled),
> but it cannot ensure SDMA core stop to access modules' FIFO,
> SDMA core may still is running, this is a bug in HW.
Okay b ut you are not doing the HE bit here..??
>
> regarding if the '1ms' is enough to ensure SDMA core has stopped,
> NXP R&D team mentioned:
> "we should add some delay of one BD SDMA cost time after disable the
> channel bit, the maximum is 1ms"
> so I assume 1ms should work for all cases
At least please document this in changelog and comments in code.
--
~Vinod
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web