Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

2 messages, 2 authors, 2014-09-16 · open the first message on its own page

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

From: Maxime Ripard <hidden>
Date: 2014-09-16 15:48:49

On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
[off-list ref] wrote:
quoted
On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
quoted
The DMA controller requires AHB1 bus clock to be clocked from PLL6.

Signed-off-by: Chen-Yu Tsai <redacted>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
 1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 8eb2c6d..1117989 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -317,6 +317,11 @@
                      clocks = <&ahb1_gates 6>;
                      resets = <&ahb1_rst 6>;
                      #dma-cells = <1>;
+
+                     /* DMA controller requires AHB1 clocked from PLL6 */
+                     assigned-clocks = <&ahb1>;
+                     assigned-clock-parents = <&pll6>;
+                     assigned-clock-rates = <200000000>;
Where did you get that from?

The user manual says that it should be clocked at 600MHz, and I'm not
sure it should be enforced there either.
The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
not "pll6 should be 200 MHz". I assume you were misled by them.

Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
vendor BSP default:

On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
pll6, which sets pre-divider to /3 and divider to /1.

Hope this clears it up. :)
It does, thanks :)

But still, I find it the wrong place to enforce such a limit. This
should go into the clock driver itself. The DMA controller requires
such a parenting, but it doesn't require any specific rate, this is
more something of the global system policy.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140916/25a15db7/attachment-0001.sig>

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

From: Chen-Yu Tsai <hidden>
Date: 2014-09-16 16:01:46

On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
[off-list ref] wrote:
On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
quoted
On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
[off-list ref] wrote:
quoted
On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
quoted
The DMA controller requires AHB1 bus clock to be clocked from PLL6.

Signed-off-by: Chen-Yu Tsai <redacted>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
 1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 8eb2c6d..1117989 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -317,6 +317,11 @@
                      clocks = <&ahb1_gates 6>;
                      resets = <&ahb1_rst 6>;
                      #dma-cells = <1>;
+
+                     /* DMA controller requires AHB1 clocked from PLL6 */
+                     assigned-clocks = <&ahb1>;
+                     assigned-clock-parents = <&pll6>;
+                     assigned-clock-rates = <200000000>;
Where did you get that from?

The user manual says that it should be clocked at 600MHz, and I'm not
sure it should be enforced there either.
The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
not "pll6 should be 200 MHz". I assume you were misled by them.

Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
vendor BSP default:

On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
pll6, which sets pre-divider to /3 and divider to /1.

Hope this clears it up. :)
It does, thanks :)

But still, I find it the wrong place to enforce such a limit. This
should go into the clock driver itself. The DMA controller requires
such a parenting, but it doesn't require any specific rate, this is
more something of the global system policy.
But I am guessing the AHB bus clock cannot be clock at any arbitrary
rate. Just muxing without setting a specific clock rate might result
in a hang. We could address this with per clock limitations, like the
patch you posted earlier. Not sure if there's anything in the kernel
like that at the moment, or how the clock framework handles clock rates
after re-parenting.

Or we could just specify in the clock driver, or in the DT via clock
supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.

The latter seems easier to do.

Also, I tested the DMA controller using dmatest on my A31 Hummingbird
without this re-parenting patch. It worked fine. How did your tests fail?


Cheers
ChenYu
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help