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

[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.

Dinh