Date:	Thu, 21 May 2015 09:35:56 +0900
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Dinh Nguyen <dinguyen@...nsource.altera.com>
Cc:	dan.j.williams@...el.com, Dinh Nguyen <dinh.linux@...il.com>,
	vinod.koul@...el.com, dmaengine@...r.kernel.org,
	linux-kernel@...r.kernel.org, olof Johansson <olof@...om.net>
Subject: Re: [PATCH] dmaengine: pl300: enable the clock to PL330 dma

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@...nsource.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@...nsource.altera.com>:
>>>>>>>>> Hi Krzystof,
>>>>>>>>>
>>>>>>>>> On 5/4/15 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>> 2015-05-04 13:28 GMT+09:00  <dinguyen@...nsource.altera.com>:
>>>>>>>>>>> From: Dinh Nguyen <dinguyen@...nsource.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@...01000 {
>>>>>>>>>         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@...fpga_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

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

Powered by blists - more mailing lists