[PATCH] dmaengine: pl300: enable the clock to PL330 dma

[PATCH] dmaengine: pl300: enable the clock to PL330 dma

[dinguyen]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Turn on the clock to the PL330 DMA if there is a clock node provided.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 drivers/dma/pl330.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 0e1f567..82eb641 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2894,6 +2894,10 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	adev->dev.dma_parms = &pl330->dma_parms;
 
+	adev->pclk = devm_clk_get(&adev->dev, "apb_pclk");
+	if (adev->pclk)
+		clk_prepare_enable(adev->pclk);
+
 	/*
 	 * This is the limit for transfers with a buswidth of 1, larger
 	 * buswidths will have larger limits.
-- 
2.2.1

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Krzysztof Kozlowski]

2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>
> Turn on the clock to the PL330 DMA if there is a clock node provided.

Why? There is no explanation in the patch for this important question - why?

Amba bus already does this and provide a wrapper function.
Additionally that would mess up with runtime PM and clock
enable/disable.

Best regards,
Krzysztof

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

Hi Krzystof,

On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Turn on the clock to the PL330 DMA if there is a clock node provided.
> 
> Why? There is no explanation in the patch for this important question - why?
> 
> Amba bus already does this and provide a wrapper function.
> Additionally that would mess up with runtime PM and clock
> enable/disable.

I don't see the clock for the DMA getting turned on at all, which is why
after the kernel has booted, the filesystem tries to open up a serial
port using DMA and the system hangs. The failure is seen here:

http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html

This only happens with the multi_v7_defconfig, because the PL330 DMA is
getting built into the kernel, while the socfpga_defconfig does not
enable the PL330.

The DTS for the socfpga platform looks like this:

pdma: pdma@ffe01000 {
	compatible = "arm,pl330", "arm,primecell";
	reg = <0xffe01000 0x1000>;
	interrupts = <0 104 4>,
		    <0 105 4>,
		...
		#dma-cells = <1>;
		#dma-channels = <8>;
		#dma-requests = <32>;
		clocks = <&l4_main_clk>;
		clock-names = "apb_pclk";
};

Perhaps I have the wrong designation for clock-names and the amba bus is
not able to pick up the correct clock?

Dinh

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Krzysztof Kozlowski]

2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
> Hi Krzystof,
>
> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>
>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>
>> Why? There is no explanation in the patch for this important question - why?
>>
>> Amba bus already does this and provide a wrapper function.
>> Additionally that would mess up with runtime PM and clock
>> enable/disable.
>
> I don't see the clock for the DMA getting turned on at all, which is why
> after the kernel has booted, the filesystem tries to open up a serial
> port using DMA and the system hangs. The failure is seen here:
>
> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html

Thanks!

The amba bus and pl330 should enable the clock and then disable it
after probing:
static int amba_probe(struct device *dev)
{
...
        ret = amba_get_enable_pclk(pcdev);
...

I wonder why do you think it is not enabled at all?

>
> This only happens with the multi_v7_defconfig, because the PL330 DMA is
> getting built into the kernel, while the socfpga_defconfig does not
> enable the PL330.

It makes sense. If pl330 driver is not enabled then necessary clocks
are turned on by bootloader. Probing pl330 effectively disables the
clock (if DMA is not used).

> The DTS for the socfpga platform looks like this:
>
> pdma: pdma@ffe01000 {
>         compatible = "arm,pl330", "arm,primecell";
>         reg = <0xffe01000 0x1000>;
>         interrupts = <0 104 4>,
>                     <0 105 4>,
>                 ...
>                 #dma-cells = <1>;
>                 #dma-channels = <8>;
>                 #dma-requests = <32>;
>                 clocks = <&l4_main_clk>;
>                 clock-names = "apb_pclk";
> };
>
> Perhaps I have the wrong designation for clock-names and the amba bus is
> not able to pick up the correct clock?

I have two ideas:
1. Is this really the clock for the DMA? If DMA is not used then
disabling it should be OK.
2. Disabling the clock may effectively disable its parent or
grandparent if there are not more users. Maybe some other driver needs
these parents to be enabled? This was the issue for at least one
similar error (on Exynos boards).

Best regards,
Krzysztof

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

+CC Olof

On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>> Hi Krzystof,
>>
>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>
>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>
>>> Why? There is no explanation in the patch for this important question - why?
>>>
>>> Amba bus already does this and provide a wrapper function.
>>> Additionally that would mess up with runtime PM and clock
>>> enable/disable.
>>
>> I don't see the clock for the DMA getting turned on at all, which is why
>> after the kernel has booted, the filesystem tries to open up a serial
>> port using DMA and the system hangs. The failure is seen here:
>>
>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
> 
> Thanks!
> 
> The amba bus and pl330 should enable the clock and then disable it
> after probing:
> static int amba_probe(struct device *dev)
> {
> ...
>         ret = amba_get_enable_pclk(pcdev);
> ...
> 
> I wonder why do you think it is not enabled at all?

I've checked it down to the register level that the gate for this clock
does not get set.

> 
>>
>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>> getting built into the kernel, while the socfpga_defconfig does not
>> enable the PL330.
> 
> It makes sense. If pl330 driver is not enabled then necessary clocks
> are turned on by bootloader. Probing pl330 effectively disables the
> clock (if DMA is not used).
> 
>> The DTS for the socfpga platform looks like this:
>>
>> pdma: pdma@ffe01000 {
>>         compatible = "arm,pl330", "arm,primecell";
>>         reg = <0xffe01000 0x1000>;
>>         interrupts = <0 104 4>,
>>                     <0 105 4>,
>>                 ...
>>                 #dma-cells = <1>;
>>                 #dma-channels = <8>;
>>                 #dma-requests = <32>;
>>                 clocks = <&l4_main_clk>;
>>                 clock-names = "apb_pclk";
>> };
>>
>> Perhaps I have the wrong designation for clock-names and the amba bus is
>> not able to pick up the correct clock?
> 
> I have two ideas:
> 1. Is this really the clock for the DMA? If DMA is not used then
> disabling it should be OK.

Yes, this is the clock for the DMA. Yeah, leaving this clock off is
fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
DMA channel.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8

> 2. Disabling the clock may effectively disable its parent or
> grandparent if there are not more users. Maybe some other driver needs
> these parents to be enabled? This was the issue for at least one
> similar error (on Exynos boards).
> 

I'll check up on these issues. When I was debugging this issue, the
l4_main_clk is only used by the DMA, so it was not getting turned on by
an other drivers.

Thanks,
Dinh

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
> +CC Olof
> 
> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>> Hi Krzystof,
>>>
>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>
>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>
>>>> Why? There is no explanation in the patch for this important question - why?
>>>>
>>>> Amba bus already does this and provide a wrapper function.
>>>> Additionally that would mess up with runtime PM and clock
>>>> enable/disable.
>>>
>>> I don't see the clock for the DMA getting turned on at all, which is why
>>> after the kernel has booted, the filesystem tries to open up a serial
>>> port using DMA and the system hangs. The failure is seen here:
>>>
>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>
>> Thanks!
>>
>> The amba bus and pl330 should enable the clock and then disable it
>> after probing:
>> static int amba_probe(struct device *dev)
>> {
>> ...
>>         ret = amba_get_enable_pclk(pcdev);
>> ...
>>
>> I wonder why do you think it is not enabled at all?
> 
> I've checked it down to the register level that the gate for this clock
> does not get set.
> 
>>
>>>
>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>> getting built into the kernel, while the socfpga_defconfig does not
>>> enable the PL330.
>>
>> It makes sense. If pl330 driver is not enabled then necessary clocks
>> are turned on by bootloader. Probing pl330 effectively disables the
>> clock (if DMA is not used).
>>
>>> The DTS for the socfpga platform looks like this:
>>>
>>> pdma: pdma@ffe01000 {
>>>         compatible = "arm,pl330", "arm,primecell";
>>>         reg = <0xffe01000 0x1000>;
>>>         interrupts = <0 104 4>,
>>>                     <0 105 4>,
>>>                 ...
>>>                 #dma-cells = <1>;
>>>                 #dma-channels = <8>;
>>>                 #dma-requests = <32>;
>>>                 clocks = <&l4_main_clk>;
>>>                 clock-names = "apb_pclk";
>>> };
>>>
>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>> not able to pick up the correct clock?
>>
>> I have two ideas:
>> 1. Is this really the clock for the DMA? If DMA is not used then
>> disabling it should be OK.
> 
> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
> DMA channel.
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
> 
>> 2. Disabling the clock may effectively disable its parent or
>> grandparent if there are not more users. Maybe some other driver needs
>> these parents to be enabled? This was the issue for at least one
>> similar error (on Exynos boards).
>>
> 
> I'll check up on these issues. When I was debugging this issue, the
> l4_main_clk is only used by the DMA, so it was not getting turned on by
> an other drivers.
> 

Ah, it looks like perhaps there's a problem with the serial driver and
suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
fine with the debug uart. It appears the DMA is getting suspended and
doesn't get resumed.

Dinh

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Krzysztof Kozlowski]

2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>> +CC Olof
>>
>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>> Hi Krzystof,
>>>>
>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>
>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>
>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>
>>>>> Amba bus already does this and provide a wrapper function.
>>>>> Additionally that would mess up with runtime PM and clock
>>>>> enable/disable.
>>>>
>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>> port using DMA and the system hangs. The failure is seen here:
>>>>
>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>
>>> Thanks!
>>>
>>> The amba bus and pl330 should enable the clock and then disable it
>>> after probing:
>>> static int amba_probe(struct device *dev)
>>> {
>>> ...
>>>         ret = amba_get_enable_pclk(pcdev);
>>> ...
>>>
>>> I wonder why do you think it is not enabled at all?
>>
>> I've checked it down to the register level that the gate for this clock
>> does not get set.
>>
>>>
>>>>
>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>> enable the PL330.
>>>
>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>> are turned on by bootloader. Probing pl330 effectively disables the
>>> clock (if DMA is not used).
>>>
>>>> The DTS for the socfpga platform looks like this:
>>>>
>>>> pdma: pdma@ffe01000 {
>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>         reg = <0xffe01000 0x1000>;
>>>>         interrupts = <0 104 4>,
>>>>                     <0 105 4>,
>>>>                 ...
>>>>                 #dma-cells = <1>;
>>>>                 #dma-channels = <8>;
>>>>                 #dma-requests = <32>;
>>>>                 clocks = <&l4_main_clk>;
>>>>                 clock-names = "apb_pclk";
>>>> };
>>>>
>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>> not able to pick up the correct clock?
>>>
>>> I have two ideas:
>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>> disabling it should be OK.
>>
>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>> DMA channel.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>
>>> 2. Disabling the clock may effectively disable its parent or
>>> grandparent if there are not more users. Maybe some other driver needs
>>> these parents to be enabled? This was the issue for at least one
>>> similar error (on Exynos boards).
>>>
>>
>> I'll check up on these issues. When I was debugging this issue, the
>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>> an other drivers.
>>
>
> Ah, it looks like perhaps there's a problem with the serial driver and
> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
> fine with the debug uart. It appears the DMA is getting suspended and
> doesn't get resumed.
>

You mean runtime PM suspend and resume or system sleep? During boot
only the first one should happen.

Could you test the DMA with dmatest? Disable the DMA in UART and
compile with CONFIG_DMATEST. Syntax for testing is here:
Documentation/dmaengine/dmatest.txt

Best regards,
Krzysztof

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

On 05/04/2015 10:55 PM, Krzysztof Kozlowski wrote:
> 2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>>> +CC Olof
>>>
>>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>> Hi Krzystof,
>>>>>
>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>>
>>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>>
>>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>>
>>>>>> Amba bus already does this and provide a wrapper function.
>>>>>> Additionally that would mess up with runtime PM and clock
>>>>>> enable/disable.
>>>>>
>>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>>> port using DMA and the system hangs. The failure is seen here:
>>>>>
>>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>>
>>>> Thanks!
>>>>
>>>> The amba bus and pl330 should enable the clock and then disable it
>>>> after probing:
>>>> static int amba_probe(struct device *dev)
>>>> {
>>>> ...
>>>>         ret = amba_get_enable_pclk(pcdev);
>>>> ...
>>>>
>>>> I wonder why do you think it is not enabled at all?
>>>
>>> I've checked it down to the register level that the gate for this clock
>>> does not get set.
>>>
>>>>
>>>>>
>>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>>> enable the PL330.
>>>>
>>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>>> are turned on by bootloader. Probing pl330 effectively disables the
>>>> clock (if DMA is not used).
>>>>
>>>>> The DTS for the socfpga platform looks like this:
>>>>>
>>>>> pdma: pdma@ffe01000 {
>>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>>         reg = <0xffe01000 0x1000>;
>>>>>         interrupts = <0 104 4>,
>>>>>                     <0 105 4>,
>>>>>                 ...
>>>>>                 #dma-cells = <1>;
>>>>>                 #dma-channels = <8>;
>>>>>                 #dma-requests = <32>;
>>>>>                 clocks = <&l4_main_clk>;
>>>>>                 clock-names = "apb_pclk";
>>>>> };
>>>>>
>>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>>> not able to pick up the correct clock?
>>>>
>>>> I have two ideas:
>>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>>> disabling it should be OK.
>>>
>>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>>> DMA channel.
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>>
>>>> 2. Disabling the clock may effectively disable its parent or
>>>> grandparent if there are not more users. Maybe some other driver needs
>>>> these parents to be enabled? This was the issue for at least one
>>>> similar error (on Exynos boards).
>>>>
>>>
>>> I'll check up on these issues. When I was debugging this issue, the
>>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>>> an other drivers.
>>>
>>
>> Ah, it looks like perhaps there's a problem with the serial driver and
>> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
>> fine with the debug uart. It appears the DMA is getting suspended and
>> doesn't get resumed.
>>
> 
> You mean runtime PM suspend and resume or system sleep? During boot
> only the first one should happen.

It's runtime PM suspend/resume.

> 
> Could you test the DMA with dmatest? Disable the DMA in UART and
> compile with CONFIG_DMATEST. Syntax for testing is here:
> Documentation/dmaengine/dmatest.txt
> 

# echo Y > /sys/module/dmatest/parameters/run
[   93.143775] dmatest: Started 1 threads using dma0chan0
[   93.149227] pm_generic_runtime_resume
[   93.153334] dmatest: Started 1 threads using dma0chan1
[   93.159380] dmatest: Started 1 threads using dma0chan2
[   93.165041] dmatest: Started 1 threads using dma0chan3
[   93.170280] dmatest: Started 1 threads using dma0chan4
[   93.175996] dmatest: Started 1 threads using dma0chan5
[   93.181642] dmatest: Started 1 threads using dma0chan6
[   93.188754] dmatest: dma0chan1-copy0: summary 10 tests, 0 failures
282 iops 2008 KB/s (0)
[   93.197091] dmatest: Started 1 threads using dma0chan7
[   93.199353] dmatest: dma0chan3-copy0: summary 10 tests, 0 failures
297 iops 2260 KB/s (0)
[   93.205407] dmatest: dma0chan0-copy0: summary 10 tests, 0 failures
177 iops 1364 KB/s (0)
[   93.215599] dmatest: dma0chan2-copy0: summary 10 tests, 0 failures
196 iops 1450 KB/s (0)
[   93.219994] dmatest: dma0chan4-copy0: summary 10 tests, 0 failures
225 iops 1554 KB/s (0)
[   93.224322] dmatest: dma0chan5-copy0: summary 10 tests, 0 failures
231 iops 1948 KB/s (0)
[   93.230065] dmatest: dma0chan6-copy0: summary 10 tests, 0 failures
231 iops 1759 KB/s (0)
[   93.231251] dmatest: dma0chan7-copy0: summary 10 tests, 0 failures
298 iops 2331 KB/s (0)
[   93.243523] pm_generic_runtime_suspend
root@socfpga_cyclone5:~#

Dinh

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

On 05/05/2015 09:56 AM, Dinh Nguyen wrote:
> On 05/04/2015 10:55 PM, Krzysztof Kozlowski wrote:
>> 2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>>>> +CC Olof
>>>>
>>>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>> Hi Krzystof,
>>>>>>
>>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>>>
>>>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>>>
>>>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>>>
>>>>>>> Amba bus already does this and provide a wrapper function.
>>>>>>> Additionally that would mess up with runtime PM and clock
>>>>>>> enable/disable.
>>>>>>
>>>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>>>> port using DMA and the system hangs. The failure is seen here:
>>>>>>
>>>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>>>
>>>>> Thanks!
>>>>>
>>>>> The amba bus and pl330 should enable the clock and then disable it
>>>>> after probing:
>>>>> static int amba_probe(struct device *dev)
>>>>> {
>>>>> ...
>>>>>         ret = amba_get_enable_pclk(pcdev);
>>>>> ...
>>>>>
>>>>> I wonder why do you think it is not enabled at all?
>>>>
>>>> I've checked it down to the register level that the gate for this clock
>>>> does not get set.
>>>>
>>>>>
>>>>>>
>>>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>>>> enable the PL330.
>>>>>
>>>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>>>> are turned on by bootloader. Probing pl330 effectively disables the
>>>>> clock (if DMA is not used).
>>>>>
>>>>>> The DTS for the socfpga platform looks like this:
>>>>>>
>>>>>> pdma: pdma@ffe01000 {
>>>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>>>         reg = <0xffe01000 0x1000>;
>>>>>>         interrupts = <0 104 4>,
>>>>>>                     <0 105 4>,
>>>>>>                 ...
>>>>>>                 #dma-cells = <1>;
>>>>>>                 #dma-channels = <8>;
>>>>>>                 #dma-requests = <32>;
>>>>>>                 clocks = <&l4_main_clk>;
>>>>>>                 clock-names = "apb_pclk";
>>>>>> };
>>>>>>
>>>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>>>> not able to pick up the correct clock?
>>>>>
>>>>> I have two ideas:
>>>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>>>> disabling it should be OK.
>>>>
>>>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>>>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>>>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>>>> DMA channel.
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>>>
>>>>> 2. Disabling the clock may effectively disable its parent or
>>>>> grandparent if there are not more users. Maybe some other driver needs
>>>>> these parents to be enabled? This was the issue for at least one
>>>>> similar error (on Exynos boards).
>>>>>
>>>>
>>>> I'll check up on these issues. When I was debugging this issue, the
>>>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>>>> an other drivers.
>>>>
>>>
>>> Ah, it looks like perhaps there's a problem with the serial driver and
>>> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
>>> fine with the debug uart. It appears the DMA is getting suspended and
>>> doesn't get resumed.
>>>
>>
>> You mean runtime PM suspend and resume or system sleep? During boot
>> only the first one should happen.
> 
> It's runtime PM suspend/resume.
> 
>>
>> Could you test the DMA with dmatest? Disable the DMA in UART and
>> compile with CONFIG_DMATEST. Syntax for testing is here:
>> Documentation/dmaengine/dmatest.txt
>>
> 
> # echo Y > /sys/module/dmatest/parameters/run
> [   93.143775] dmatest: Started 1 threads using dma0chan0
> [   93.149227] pm_generic_runtime_resume
> [   93.153334] dmatest: Started 1 threads using dma0chan1
> [   93.159380] dmatest: Started 1 threads using dma0chan2
> [   93.165041] dmatest: Started 1 threads using dma0chan3
> [   93.170280] dmatest: Started 1 threads using dma0chan4
> [   93.175996] dmatest: Started 1 threads using dma0chan5
> [   93.181642] dmatest: Started 1 threads using dma0chan6
> [   93.188754] dmatest: dma0chan1-copy0: summary 10 tests, 0 failures
> 282 iops 2008 KB/s (0)
> [   93.197091] dmatest: Started 1 threads using dma0chan7
> [   93.199353] dmatest: dma0chan3-copy0: summary 10 tests, 0 failures
> 297 iops 2260 KB/s (0)
> [   93.205407] dmatest: dma0chan0-copy0: summary 10 tests, 0 failures
> 177 iops 1364 KB/s (0)
> [   93.215599] dmatest: dma0chan2-copy0: summary 10 tests, 0 failures
> 196 iops 1450 KB/s (0)
> [   93.219994] dmatest: dma0chan4-copy0: summary 10 tests, 0 failures
> 225 iops 1554 KB/s (0)
> [   93.224322] dmatest: dma0chan5-copy0: summary 10 tests, 0 failures
> 231 iops 1948 KB/s (0)
> [   93.230065] dmatest: dma0chan6-copy0: summary 10 tests, 0 failures
> 231 iops 1759 KB/s (0)
> [   93.231251] dmatest: dma0chan7-copy0: summary 10 tests, 0 failures
> 298 iops 2331 KB/s (0)
> [   93.243523] pm_generic_runtime_suspend
> root@socfpga_cyclone5:~#
> 

If I run dmatest the 2nd time it fails. It does not look like
amba_pm_runtime_resume() is getting called to turn on the clocks on the
subsequent tries.

Dinh

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

Hi Krzysztof,

On 05/05/2015 02:22 PM, Dinh Nguyen wrote:
> On 05/05/2015 09:56 AM, Dinh Nguyen wrote:
>> On 05/04/2015 10:55 PM, Krzysztof Kozlowski wrote:
>>> 2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>>>>> +CC Olof
>>>>>
>>>>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>>>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>>> Hi Krzystof,
>>>>>>>
>>>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>>>>
>>>>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>>>>
>>>>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>>>>
>>>>>>>> Amba bus already does this and provide a wrapper function.
>>>>>>>> Additionally that would mess up with runtime PM and clock
>>>>>>>> enable/disable.
>>>>>>>
>>>>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>>>>> port using DMA and the system hangs. The failure is seen here:
>>>>>>>
>>>>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> The amba bus and pl330 should enable the clock and then disable it
>>>>>> after probing:
>>>>>> static int amba_probe(struct device *dev)
>>>>>> {
>>>>>> ...
>>>>>>         ret = amba_get_enable_pclk(pcdev);
>>>>>> ...
>>>>>>
>>>>>> I wonder why do you think it is not enabled at all?
>>>>>
>>>>> I've checked it down to the register level that the gate for this clock
>>>>> does not get set.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>>>>> enable the PL330.
>>>>>>
>>>>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>>>>> are turned on by bootloader. Probing pl330 effectively disables the
>>>>>> clock (if DMA is not used).
>>>>>>
>>>>>>> The DTS for the socfpga platform looks like this:
>>>>>>>
>>>>>>> pdma: pdma@ffe01000 {
>>>>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>>>>         reg = <0xffe01000 0x1000>;
>>>>>>>         interrupts = <0 104 4>,
>>>>>>>                     <0 105 4>,
>>>>>>>                 ...
>>>>>>>                 #dma-cells = <1>;
>>>>>>>                 #dma-channels = <8>;
>>>>>>>                 #dma-requests = <32>;
>>>>>>>                 clocks = <&l4_main_clk>;
>>>>>>>                 clock-names = "apb_pclk";
>>>>>>> };
>>>>>>>
>>>>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>>>>> not able to pick up the correct clock?
>>>>>>
>>>>>> I have two ideas:
>>>>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>>>>> disabling it should be OK.
>>>>>
>>>>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>>>>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>>>>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>>>>> DMA channel.
>>>>>
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>>>>
>>>>>> 2. Disabling the clock may effectively disable its parent or
>>>>>> grandparent if there are not more users. Maybe some other driver needs
>>>>>> these parents to be enabled? This was the issue for at least one
>>>>>> similar error (on Exynos boards).
>>>>>>
>>>>>
>>>>> I'll check up on these issues. When I was debugging this issue, the
>>>>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>>>>> an other drivers.
>>>>>
>>>>
>>>> Ah, it looks like perhaps there's a problem with the serial driver and
>>>> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
>>>> fine with the debug uart. It appears the DMA is getting suspended and
>>>> doesn't get resumed.
>>>>
>>>
>>> You mean runtime PM suspend and resume or system sleep? During boot
>>> only the first one should happen.
>>
>> It's runtime PM suspend/resume.
>>
>>>
>>> Could you test the DMA with dmatest? Disable the DMA in UART and
>>> compile with CONFIG_DMATEST. Syntax for testing is here:
>>> Documentation/dmaengine/dmatest.txt
>>>
>>
>> # echo Y > /sys/module/dmatest/parameters/run
>> [   93.143775] dmatest: Started 1 threads using dma0chan0
>> [   93.149227] pm_generic_runtime_resume
>> [   93.153334] dmatest: Started 1 threads using dma0chan1
>> [   93.159380] dmatest: Started 1 threads using dma0chan2
>> [   93.165041] dmatest: Started 1 threads using dma0chan3
>> [   93.170280] dmatest: Started 1 threads using dma0chan4
>> [   93.175996] dmatest: Started 1 threads using dma0chan5
>> [   93.181642] dmatest: Started 1 threads using dma0chan6
>> [   93.188754] dmatest: dma0chan1-copy0: summary 10 tests, 0 failures
>> 282 iops 2008 KB/s (0)
>> [   93.197091] dmatest: Started 1 threads using dma0chan7
>> [   93.199353] dmatest: dma0chan3-copy0: summary 10 tests, 0 failures
>> 297 iops 2260 KB/s (0)
>> [   93.205407] dmatest: dma0chan0-copy0: summary 10 tests, 0 failures
>> 177 iops 1364 KB/s (0)
>> [   93.215599] dmatest: dma0chan2-copy0: summary 10 tests, 0 failures
>> 196 iops 1450 KB/s (0)
>> [   93.219994] dmatest: dma0chan4-copy0: summary 10 tests, 0 failures
>> 225 iops 1554 KB/s (0)
>> [   93.224322] dmatest: dma0chan5-copy0: summary 10 tests, 0 failures
>> 231 iops 1948 KB/s (0)
>> [   93.230065] dmatest: dma0chan6-copy0: summary 10 tests, 0 failures
>> 231 iops 1759 KB/s (0)
>> [   93.231251] dmatest: dma0chan7-copy0: summary 10 tests, 0 failures
>> 298 iops 2331 KB/s (0)
>> [   93.243523] pm_generic_runtime_suspend
>> root@socfpga_cyclone5:~#
>>
> 
> If I run dmatest the 2nd time it fails. It does not look like
> amba_pm_runtime_resume() is getting called to turn on the clocks on the
> subsequent tries.
> 


I managed to track this down the call dmaengine_terminate_all(), which
then calls into pl330_terminate_all(). So in pl330_terminate_all(), it
call _stop, which hits a infinite loop, UNTIL. But since the
amba_pm_runtime_resume() has not been called yet, the clock is turned
off. Thus, we're stuck in an infinite loop.

I'm not sure what would be right approach to fix this?

Thanks,

Dinh

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Krzysztof Kozlowski]

On 21.05.2015 05:30, Dinh Nguyen wrote:
> Hi Krzysztof,
> 
> On 05/05/2015 02:22 PM, Dinh Nguyen wrote:
>> On 05/05/2015 09:56 AM, Dinh Nguyen wrote:
>>> On 05/04/2015 10:55 PM, Krzysztof Kozlowski wrote:
>>>> 2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>>>>>> +CC Olof
>>>>>>
>>>>>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>>>>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>>>> Hi Krzystof,
>>>>>>>>
>>>>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>>>>>
>>>>>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>>>>>
>>>>>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>>>>>
>>>>>>>>> Amba bus already does this and provide a wrapper function.
>>>>>>>>> Additionally that would mess up with runtime PM and clock
>>>>>>>>> enable/disable.
>>>>>>>>
>>>>>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>>>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>>>>>> port using DMA and the system hangs. The failure is seen here:
>>>>>>>>
>>>>>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> The amba bus and pl330 should enable the clock and then disable it
>>>>>>> after probing:
>>>>>>> static int amba_probe(struct device *dev)
>>>>>>> {
>>>>>>> ...
>>>>>>>         ret = amba_get_enable_pclk(pcdev);
>>>>>>> ...
>>>>>>>
>>>>>>> I wonder why do you think it is not enabled at all?
>>>>>>
>>>>>> I've checked it down to the register level that the gate for this clock
>>>>>> does not get set.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>>>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>>>>>> enable the PL330.
>>>>>>>
>>>>>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>>>>>> are turned on by bootloader. Probing pl330 effectively disables the
>>>>>>> clock (if DMA is not used).
>>>>>>>
>>>>>>>> The DTS for the socfpga platform looks like this:
>>>>>>>>
>>>>>>>> pdma: pdma@ffe01000 {
>>>>>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>>>>>         reg = <0xffe01000 0x1000>;
>>>>>>>>         interrupts = <0 104 4>,
>>>>>>>>                     <0 105 4>,
>>>>>>>>                 ...
>>>>>>>>                 #dma-cells = <1>;
>>>>>>>>                 #dma-channels = <8>;
>>>>>>>>                 #dma-requests = <32>;
>>>>>>>>                 clocks = <&l4_main_clk>;
>>>>>>>>                 clock-names = "apb_pclk";
>>>>>>>> };
>>>>>>>>
>>>>>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>>>>>> not able to pick up the correct clock?
>>>>>>>
>>>>>>> I have two ideas:
>>>>>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>>>>>> disabling it should be OK.
>>>>>>
>>>>>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>>>>>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>>>>>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>>>>>> DMA channel.
>>>>>>
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>>>>>
>>>>>>> 2. Disabling the clock may effectively disable its parent or
>>>>>>> grandparent if there are not more users. Maybe some other driver needs
>>>>>>> these parents to be enabled? This was the issue for at least one
>>>>>>> similar error (on Exynos boards).
>>>>>>>
>>>>>>
>>>>>> I'll check up on these issues. When I was debugging this issue, the
>>>>>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>>>>>> an other drivers.
>>>>>>
>>>>>
>>>>> Ah, it looks like perhaps there's a problem with the serial driver and
>>>>> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
>>>>> fine with the debug uart. It appears the DMA is getting suspended and
>>>>> doesn't get resumed.
>>>>>
>>>>
>>>> You mean runtime PM suspend and resume or system sleep? During boot
>>>> only the first one should happen.
>>>
>>> It's runtime PM suspend/resume.
>>>
>>>>
>>>> Could you test the DMA with dmatest? Disable the DMA in UART and
>>>> compile with CONFIG_DMATEST. Syntax for testing is here:
>>>> Documentation/dmaengine/dmatest.txt
>>>>
>>>
>>> # echo Y > /sys/module/dmatest/parameters/run
>>> [   93.143775] dmatest: Started 1 threads using dma0chan0
>>> [   93.149227] pm_generic_runtime_resume
>>> [   93.153334] dmatest: Started 1 threads using dma0chan1
>>> [   93.159380] dmatest: Started 1 threads using dma0chan2
>>> [   93.165041] dmatest: Started 1 threads using dma0chan3
>>> [   93.170280] dmatest: Started 1 threads using dma0chan4
>>> [   93.175996] dmatest: Started 1 threads using dma0chan5
>>> [   93.181642] dmatest: Started 1 threads using dma0chan6
>>> [   93.188754] dmatest: dma0chan1-copy0: summary 10 tests, 0 failures
>>> 282 iops 2008 KB/s (0)
>>> [   93.197091] dmatest: Started 1 threads using dma0chan7
>>> [   93.199353] dmatest: dma0chan3-copy0: summary 10 tests, 0 failures
>>> 297 iops 2260 KB/s (0)
>>> [   93.205407] dmatest: dma0chan0-copy0: summary 10 tests, 0 failures
>>> 177 iops 1364 KB/s (0)
>>> [   93.215599] dmatest: dma0chan2-copy0: summary 10 tests, 0 failures
>>> 196 iops 1450 KB/s (0)
>>> [   93.219994] dmatest: dma0chan4-copy0: summary 10 tests, 0 failures
>>> 225 iops 1554 KB/s (0)
>>> [   93.224322] dmatest: dma0chan5-copy0: summary 10 tests, 0 failures
>>> 231 iops 1948 KB/s (0)
>>> [   93.230065] dmatest: dma0chan6-copy0: summary 10 tests, 0 failures
>>> 231 iops 1759 KB/s (0)
>>> [   93.231251] dmatest: dma0chan7-copy0: summary 10 tests, 0 failures
>>> 298 iops 2331 KB/s (0)
>>> [   93.243523] pm_generic_runtime_suspend
>>> root@socfpga_cyclone5:~#
>>>
>>
>> If I run dmatest the 2nd time it fails. It does not look like
>> amba_pm_runtime_resume() is getting called to turn on the clocks on the
>> subsequent tries.
>>
> 
> 
> I managed to track this down the call dmaengine_terminate_all(), which
> then calls into pl330_terminate_all(). So in pl330_terminate_all(), it
> call _stop, which hits a infinite loop, UNTIL. But since the
> amba_pm_runtime_resume() has not been called yet, the clock is turned
> off. Thus, we're stuck in an infinite loop.
> 
> I'm not sure what would be right approach to fix this?

Good catch. I confirmed that device is not runtime resumed. I wonder why
it works in my case (pl330 on Exynos4412)...

Anyway I have an idea to fix it. I'll send a patch.

Best regards,
Krzysztof

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Krzysztof Kozlowski]

On 21.05.2015 09:16, Krzysztof Kozlowski wrote:
> On 21.05.2015 05:30, Dinh Nguyen wrote:
>> Hi Krzysztof,
>>
>> On 05/05/2015 02:22 PM, Dinh Nguyen wrote:
>>> On 05/05/2015 09:56 AM, Dinh Nguyen wrote:
>>>> On 05/04/2015 10:55 PM, Krzysztof Kozlowski wrote:
>>>>> 2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>>>>>>> +CC Olof
>>>>>>>
>>>>>>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>>>>>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>>>>> Hi Krzystof,
>>>>>>>>>
>>>>>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>>>>>>
>>>>>>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>>>>>>
>>>>>>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>>>>>>
>>>>>>>>>> Amba bus already does this and provide a wrapper function.
>>>>>>>>>> Additionally that would mess up with runtime PM and clock
>>>>>>>>>> enable/disable.
>>>>>>>>>
>>>>>>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>>>>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>>>>>>> port using DMA and the system hangs. The failure is seen here:
>>>>>>>>>
>>>>>>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> The amba bus and pl330 should enable the clock and then disable it
>>>>>>>> after probing:
>>>>>>>> static int amba_probe(struct device *dev)
>>>>>>>> {
>>>>>>>> ...
>>>>>>>>         ret = amba_get_enable_pclk(pcdev);
>>>>>>>> ...
>>>>>>>>
>>>>>>>> I wonder why do you think it is not enabled at all?
>>>>>>>
>>>>>>> I've checked it down to the register level that the gate for this clock
>>>>>>> does not get set.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>>>>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>>>>>>> enable the PL330.
>>>>>>>>
>>>>>>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>>>>>>> are turned on by bootloader. Probing pl330 effectively disables the
>>>>>>>> clock (if DMA is not used).
>>>>>>>>
>>>>>>>>> The DTS for the socfpga platform looks like this:
>>>>>>>>>
>>>>>>>>> pdma: pdma@ffe01000 {
>>>>>>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>>>>>>         reg = <0xffe01000 0x1000>;
>>>>>>>>>         interrupts = <0 104 4>,
>>>>>>>>>                     <0 105 4>,
>>>>>>>>>                 ...
>>>>>>>>>                 #dma-cells = <1>;
>>>>>>>>>                 #dma-channels = <8>;
>>>>>>>>>                 #dma-requests = <32>;
>>>>>>>>>                 clocks = <&l4_main_clk>;
>>>>>>>>>                 clock-names = "apb_pclk";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>>>>>>> not able to pick up the correct clock?
>>>>>>>>
>>>>>>>> I have two ideas:
>>>>>>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>>>>>>> disabling it should be OK.
>>>>>>>
>>>>>>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>>>>>>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>>>>>>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>>>>>>> DMA channel.
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>>>>>>
>>>>>>>> 2. Disabling the clock may effectively disable its parent or
>>>>>>>> grandparent if there are not more users. Maybe some other driver needs
>>>>>>>> these parents to be enabled? This was the issue for at least one
>>>>>>>> similar error (on Exynos boards).
>>>>>>>>
>>>>>>>
>>>>>>> I'll check up on these issues. When I was debugging this issue, the
>>>>>>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>>>>>>> an other drivers.
>>>>>>>
>>>>>>
>>>>>> Ah, it looks like perhaps there's a problem with the serial driver and
>>>>>> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
>>>>>> fine with the debug uart. It appears the DMA is getting suspended and
>>>>>> doesn't get resumed.
>>>>>>
>>>>>
>>>>> You mean runtime PM suspend and resume or system sleep? During boot
>>>>> only the first one should happen.
>>>>
>>>> It's runtime PM suspend/resume.
>>>>
>>>>>
>>>>> Could you test the DMA with dmatest? Disable the DMA in UART and
>>>>> compile with CONFIG_DMATEST. Syntax for testing is here:
>>>>> Documentation/dmaengine/dmatest.txt
>>>>>
>>>>
>>>> # echo Y > /sys/module/dmatest/parameters/run
>>>> [   93.143775] dmatest: Started 1 threads using dma0chan0
>>>> [   93.149227] pm_generic_runtime_resume
>>>> [   93.153334] dmatest: Started 1 threads using dma0chan1
>>>> [   93.159380] dmatest: Started 1 threads using dma0chan2
>>>> [   93.165041] dmatest: Started 1 threads using dma0chan3
>>>> [   93.170280] dmatest: Started 1 threads using dma0chan4
>>>> [   93.175996] dmatest: Started 1 threads using dma0chan5
>>>> [   93.181642] dmatest: Started 1 threads using dma0chan6
>>>> [   93.188754] dmatest: dma0chan1-copy0: summary 10 tests, 0 failures
>>>> 282 iops 2008 KB/s (0)
>>>> [   93.197091] dmatest: Started 1 threads using dma0chan7
>>>> [   93.199353] dmatest: dma0chan3-copy0: summary 10 tests, 0 failures
>>>> 297 iops 2260 KB/s (0)
>>>> [   93.205407] dmatest: dma0chan0-copy0: summary 10 tests, 0 failures
>>>> 177 iops 1364 KB/s (0)
>>>> [   93.215599] dmatest: dma0chan2-copy0: summary 10 tests, 0 failures
>>>> 196 iops 1450 KB/s (0)
>>>> [   93.219994] dmatest: dma0chan4-copy0: summary 10 tests, 0 failures
>>>> 225 iops 1554 KB/s (0)
>>>> [   93.224322] dmatest: dma0chan5-copy0: summary 10 tests, 0 failures
>>>> 231 iops 1948 KB/s (0)
>>>> [   93.230065] dmatest: dma0chan6-copy0: summary 10 tests, 0 failures
>>>> 231 iops 1759 KB/s (0)
>>>> [   93.231251] dmatest: dma0chan7-copy0: summary 10 tests, 0 failures
>>>> 298 iops 2331 KB/s (0)
>>>> [   93.243523] pm_generic_runtime_suspend
>>>> root@socfpga_cyclone5:~#
>>>>
>>>
>>> If I run dmatest the 2nd time it fails. It does not look like
>>> amba_pm_runtime_resume() is getting called to turn on the clocks on the
>>> subsequent tries.
>>>
>>
>>
>> I managed to track this down the call dmaengine_terminate_all(), which
>> then calls into pl330_terminate_all(). So in pl330_terminate_all(), it
>> call _stop, which hits a infinite loop, UNTIL. But since the
>> amba_pm_runtime_resume() has not been called yet, the clock is turned
>> off. Thus, we're stuck in an infinite loop.
>>
>> I'm not sure what would be right approach to fix this?
> 
> Good catch. I confirmed that device is not runtime resumed. I wonder why
> it works in my case (pl330 on Exynos4412)...
> 
> Anyway I have an idea to fix it. I'll send a patch.

I sent a patch. Could you test on your board and confirm that this fixes
the issue?

Best regards,
Krzysztof

Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

[Dinh Nguyen]

On 05/20/2015 07:35 PM, Krzysztof Kozlowski wrote:
> On 21.05.2015 09:16, Krzysztof Kozlowski wrote:
>> On 21.05.2015 05:30, Dinh Nguyen wrote:
>>> Hi Krzysztof,
>>>
>>> On 05/05/2015 02:22 PM, Dinh Nguyen wrote:
>>>> On 05/05/2015 09:56 AM, Dinh Nguyen wrote:
>>>>> On 05/04/2015 10:55 PM, Krzysztof Kozlowski wrote:
>>>>>> 2015-05-05 4:52 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>>> On 05/04/2015 09:06 AM, Dinh Nguyen wrote:
>>>>>>>> +CC Olof
>>>>>>>>
>>>>>>>> On 5/4/15 8:50 AM, Krzysztof Kozlowski wrote:
>>>>>>>>> 2015-05-04 22:28 GMT+09:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
>>>>>>>>>> Hi Krzystof,
>>>>>>>>>>
>>>>>>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@opensource.altera.com>:
>>>>>>>>>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Turn on the clock to the PL330 DMA if there is a clock node provided.
>>>>>>>>>>>
>>>>>>>>>>> Why? There is no explanation in the patch for this important question - why?
>>>>>>>>>>>
>>>>>>>>>>> Amba bus already does this and provide a wrapper function.
>>>>>>>>>>> Additionally that would mess up with runtime PM and clock
>>>>>>>>>>> enable/disable.
>>>>>>>>>>
>>>>>>>>>> I don't see the clock for the DMA getting turned on at all, which is why
>>>>>>>>>> after the kernel has booted, the filesystem tries to open up a serial
>>>>>>>>>> port using DMA and the system hangs. The failure is seen here:
>>>>>>>>>>
>>>>>>>>>> http://arm-soc.lixom.net/bootlogs/next/next-20150504/socfpga-arm-multi_v7_defconfig.html
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>> The amba bus and pl330 should enable the clock and then disable it
>>>>>>>>> after probing:
>>>>>>>>> static int amba_probe(struct device *dev)
>>>>>>>>> {
>>>>>>>>> ...
>>>>>>>>>         ret = amba_get_enable_pclk(pcdev);
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> I wonder why do you think it is not enabled at all?
>>>>>>>>
>>>>>>>> I've checked it down to the register level that the gate for this clock
>>>>>>>> does not get set.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This only happens with the multi_v7_defconfig, because the PL330 DMA is
>>>>>>>>>> getting built into the kernel, while the socfpga_defconfig does not
>>>>>>>>>> enable the PL330.
>>>>>>>>>
>>>>>>>>> It makes sense. If pl330 driver is not enabled then necessary clocks
>>>>>>>>> are turned on by bootloader. Probing pl330 effectively disables the
>>>>>>>>> clock (if DMA is not used).
>>>>>>>>>
>>>>>>>>>> The DTS for the socfpga platform looks like this:
>>>>>>>>>>
>>>>>>>>>> pdma: pdma@ffe01000 {
>>>>>>>>>>         compatible = "arm,pl330", "arm,primecell";
>>>>>>>>>>         reg = <0xffe01000 0x1000>;
>>>>>>>>>>         interrupts = <0 104 4>,
>>>>>>>>>>                     <0 105 4>,
>>>>>>>>>>                 ...
>>>>>>>>>>                 #dma-cells = <1>;
>>>>>>>>>>                 #dma-channels = <8>;
>>>>>>>>>>                 #dma-requests = <32>;
>>>>>>>>>>                 clocks = <&l4_main_clk>;
>>>>>>>>>>                 clock-names = "apb_pclk";
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> Perhaps I have the wrong designation for clock-names and the amba bus is
>>>>>>>>>> not able to pick up the correct clock?
>>>>>>>>>
>>>>>>>>> I have two ideas:
>>>>>>>>> 1. Is this really the clock for the DMA? If DMA is not used then
>>>>>>>>> disabling it should be OK.
>>>>>>>>
>>>>>>>> Yes, this is the clock for the DMA. Yeah, leaving this clock off is
>>>>>>>> fine, until the DMA gets used. Up until v4.0, SoCFPGA was not using the
>>>>>>>> DMA at all, but in v4.0, there was a patch to assign the UARTs to it's
>>>>>>>> DMA channel.
>>>>>>>>
>>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/socfpga.dtsi?id=78c03c7af89721bd8a4428408a8cc7b53972e4b8
>>>>>>>>
>>>>>>>>> 2. Disabling the clock may effectively disable its parent or
>>>>>>>>> grandparent if there are not more users. Maybe some other driver needs
>>>>>>>>> these parents to be enabled? This was the issue for at least one
>>>>>>>>> similar error (on Exynos boards).
>>>>>>>>>
>>>>>>>>
>>>>>>>> I'll check up on these issues. When I was debugging this issue, the
>>>>>>>> l4_main_clk is only used by the DMA, so it was not getting turned on by
>>>>>>>> an other drivers.
>>>>>>>>
>>>>>>>
>>>>>>> Ah, it looks like perhaps there's a problem with the serial driver and
>>>>>>> suspend/resume? If disable CONFIG_PM, then the DMA seems to be working
>>>>>>> fine with the debug uart. It appears the DMA is getting suspended and
>>>>>>> doesn't get resumed.
>>>>>>>
>>>>>>
>>>>>> You mean runtime PM suspend and resume or system sleep? During boot
>>>>>> only the first one should happen.
>>>>>
>>>>> It's runtime PM suspend/resume.
>>>>>
>>>>>>
>>>>>> Could you test the DMA with dmatest? Disable the DMA in UART and
>>>>>> compile with CONFIG_DMATEST. Syntax for testing is here:
>>>>>> Documentation/dmaengine/dmatest.txt
>>>>>>
>>>>>
>>>>> # echo Y > /sys/module/dmatest/parameters/run
>>>>> [   93.143775] dmatest: Started 1 threads using dma0chan0
>>>>> [   93.149227] pm_generic_runtime_resume
>>>>> [   93.153334] dmatest: Started 1 threads using dma0chan1
>>>>> [   93.159380] dmatest: Started 1 threads using dma0chan2
>>>>> [   93.165041] dmatest: Started 1 threads using dma0chan3
>>>>> [   93.170280] dmatest: Started 1 threads using dma0chan4
>>>>> [   93.175996] dmatest: Started 1 threads using dma0chan5
>>>>> [   93.181642] dmatest: Started 1 threads using dma0chan6
>>>>> [   93.188754] dmatest: dma0chan1-copy0: summary 10 tests, 0 failures
>>>>> 282 iops 2008 KB/s (0)
>>>>> [   93.197091] dmatest: Started 1 threads using dma0chan7
>>>>> [   93.199353] dmatest: dma0chan3-copy0: summary 10 tests, 0 failures
>>>>> 297 iops 2260 KB/s (0)
>>>>> [   93.205407] dmatest: dma0chan0-copy0: summary 10 tests, 0 failures
>>>>> 177 iops 1364 KB/s (0)
>>>>> [   93.215599] dmatest: dma0chan2-copy0: summary 10 tests, 0 failures
>>>>> 196 iops 1450 KB/s (0)
>>>>> [   93.219994] dmatest: dma0chan4-copy0: summary 10 tests, 0 failures
>>>>> 225 iops 1554 KB/s (0)
>>>>> [   93.224322] dmatest: dma0chan5-copy0: summary 10 tests, 0 failures
>>>>> 231 iops 1948 KB/s (0)
>>>>> [   93.230065] dmatest: dma0chan6-copy0: summary 10 tests, 0 failures
>>>>> 231 iops 1759 KB/s (0)
>>>>> [   93.231251] dmatest: dma0chan7-copy0: summary 10 tests, 0 failures
>>>>> 298 iops 2331 KB/s (0)
>>>>> [   93.243523] pm_generic_runtime_suspend
>>>>> root@socfpga_cyclone5:~#
>>>>>
>>>>
>>>> If I run dmatest the 2nd time it fails. It does not look like
>>>> amba_pm_runtime_resume() is getting called to turn on the clocks on the
>>>> subsequent tries.
>>>>
>>>
>>>
>>> I managed to track this down the call dmaengine_terminate_all(), which
>>> then calls into pl330_terminate_all(). So in pl330_terminate_all(), it
>>> call _stop, which hits a infinite loop, UNTIL. But since the
>>> amba_pm_runtime_resume() has not been called yet, the clock is turned
>>> off. Thus, we're stuck in an infinite loop.
>>>
>>> I'm not sure what would be right approach to fix this?
>>
>> Good catch. I confirmed that device is not runtime resumed. I wonder why
>> it works in my case (pl330 on Exynos4412)...
>>
>> Anyway I have an idea to fix it. I'll send a patch.
> 
> I sent a patch. Could you test on your board and confirm that this fixes
> the issue?
> 

Thanks alot for the patch! It does indeed fixes the issue for me.

Dinh