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

[Dinh Nguyen <dinguyen@opensource.altera.com>]

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