clk: nomadik: implement the Nomadik clocks properly

The Nomadik clock implementation was a stub just using fixed clocks.

This implements the clocks properly instead of relying on them all being on at boot and leaving them all on.

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +19 -0

+               /*
+                * IP AMBA bus clocks, driving the bus side of the
+                * peripheral clocking, clock gates.
+                */
+
+               hclkdma0: hclkdma0 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <0>;
+                       clocks = <&hclk>;
+               };
+               hclksmc: hclksmc at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <1>;
+                       clocks = <&hclk>;
+               };
+               hclksdram: hclksdram at 48M {
+                       #clock-cells = <0>;

Linus,

While hacking on clock support for msm8960 the other day I did the same thing; I figured that providing clocks with descriptive names would be of great benefit while reading the dts.

I had my uart device setup with:
clocks = <&gsbi8_uart_clk>, <&gsbi8_p_clk>;

However, all currently present clock bindings seem to follow the pattern described by Arnd.
Following this pattern my uart would be setup as:
clocks = <&clks 42>, <&clks 44>;

Needless to say there is no way to make sense of this without consulting the specific binding documentation. So, although your way of setting up the clock binding is not the standard way I do prefer it, but it comes with plenty extra nodes in the tree.

Regards,
Bjorn

Quoted 44 lines · Arnd Bergmann

Quoted 19 lines · Linus Walleij

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +19 -0

+               /*
+                * IP AMBA bus clocks, driving the bus side of the
+                * peripheral clocking, clock gates.
+                */
+
+               hclkdma0: hclkdma0 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <0>;
+                       clocks = <&hclk>;
+               };
+               hclksmc: hclksmc at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <1>;
+                       clocks = <&hclk>;
+               };
+               hclksdram: hclksdram at 48M {
+                       #clock-cells = <0>;

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi line 301

Quoted 10 lines

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +10 -0

+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <2>;
+                       clocks = <&hclk>;
+               };
+               hclkdma1: hclkdma1 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <3>;
+                       clocks = <&hclk>;
+               };

Sorry if I'm being slow to understand how the clock bindings work, but if you have 63 identical clocks that only differ in ther clock-id, can't you just have a single DT node for them instead with #clock-cells=1 to pass the
number from the device using it?

Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

While hacking on clock support for msm8960 the other day I did the same thing; I figured that providing clocks with descriptive names would be of great benefit while reading the dts.

I had my uart device setup with:
clocks = <&gsbi8_uart_clk>, <&gsbi8_p_clk>;

However, all currently present clock bindings follow the pattern described by Arnd.
Following this pattern my uart would be setup as:
clocks = <&clks 42>, <&clks 44>;

Needless to say there is no way to make sense of this without consulting the specific binding documentation. So, although your way of setting up the clock binding is not the standard way I do prefer it, but it comes with plenty extra nodes in the tree.

Quoted 19 lines · Linus Walleij

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +19 -0

+               /*
+                * IP AMBA bus clocks, driving the bus side of the
+                * peripheral clocking, clock gates.
+                */
+
+               hclkdma0: hclkdma0 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <0>;
+                       clocks = <&hclk>;
+               };
+               hclksmc: hclksmc at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <1>;
+                       clocks = <&hclk>;
+               };
+               hclksdram: hclksdram at 48M {
+                       #clock-cells = <0>;

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi line 301

Quoted 10 lines

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +10 -0

+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <2>;
+                       clocks = <&hclk>;
+               };
+               hclkdma1: hclkdma1 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <3>;
+                       clocks = <&hclk>;
+               };

Sorry if I'm being slow to understand how the clock bindings work, but if you have 63 identical clocks that only differ in ther clock-id, can't you just have a single DT node for them instead with #clock-cells=1 to pass the number from the device using it?

Quoted 31 lines · Arnd Bergmann

Quoted 19 lines · Linus Walleij

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +19 -0

+               /*
+                * IP AMBA bus clocks, driving the bus side of the
+                * peripheral clocking, clock gates.
+                */
+
+               hclkdma0: hclkdma0 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <0>;
+                       clocks = <&hclk>;
+               };
+               hclksmc: hclksmc at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <1>;
+                       clocks = <&hclk>;
+               };
+               hclksdram: hclksdram at 48M {
+                       #clock-cells = <0>;

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi line 301

Quoted 10 lines

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +10 -0

+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <2>;
+                       clocks = <&hclk>;
+               };
+               hclkdma1: hclkdma1 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <3>;
+                       clocks = <&hclk>;
+               };

Quoted 4 lines

Sorry if I'm being slow to understand how the clock bindings work, but if you have 63 identical clocks that only differ in ther clock-id, can't you just have a single DT node for them instead with #clock-cells=1 to pass the number from the device using it?

I first had exactly that implementation but it didn't work. This is explained in the commit message on patch 1/1:

The gated clocks are implemented with zero cells and given the clock ID as a property of each node, so every gate need to have its own node in the device tree. This is because the gate registers contain both HCLK gates and PCLK gates, where the latter has HCLK as parent. As can be seen from the register layout, this is a complete mixup, which means all these gates need their own node to properly model parent/child relations for PCLKs apart from the HCLKs.

Basically the "shortcoming" in device tree is that modelling this with clock-cells = 1 requires them all to have one and the same parent, but they don't.

Quoted 50 lines · Linus Walleij

Quoted 31 lines · Arnd Bergmann

Quoted 19 lines · Linus Walleij

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +19 -0

+               /*
+                * IP AMBA bus clocks, driving the bus side of the
+                * peripheral clocking, clock gates.
+                */
+
+               hclkdma0: hclkdma0 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <0>;
+                       clocks = <&hclk>;
+               };
+               hclksmc: hclksmc at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <1>;
+                       clocks = <&hclk>;
+               };
+               hclksdram: hclksdram at 48M {
+                       #clock-cells = <0>;

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi line 301

Quoted 10 lines

arch/arm/boot/dts/ste-nomadik-stn8815.dtsi +10 -0

+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <2>;
+                       clocks = <&hclk>;
+               };
+               hclkdma1: hclkdma1 at 48M {
+                       #clock-cells = <0>;
+                       compatible = "st,nomadik-src-clock";
+                       clock-id = <3>;
+                       clocks = <&hclk>;
+               };

Quoted 4 lines

Sorry if I'm being slow to understand how the clock bindings work, but if you have 63 identical clocks that only differ in ther clock-id, can't you just have a single DT node for them instead with #clock-cells=1 to pass the number from the device using it?

I first had exactly that implementation but it didn't work. This is explained in the commit message on patch 1/1:

The gated clocks are implemented with zero cells and given the clock ID as a property of each node, so every gate need to have its own node in the device tree. This is because the gate registers contain both HCLK gates and PCLK gates, where the latter has HCLK as parent. As can be seen from the register layout, this is a complete mixup, which means all these gates need their own node to properly model parent/child relations for PCLKs apart from the HCLKs.

Quoted 3 lines

Basically the "shortcoming" in device tree is that modelling this with clock-cells = 1 requires them all to have one and the same parent, but they don't.

Are you prevented from increasing the number of clock-cells used here?

drivers/clk/clk-nomadik.c +14 -0

+
+       spin_lock(&src_lock);
+       val = readl(src_base + SRC_PLLCR);
+       if (pll->id == 1) {
+               if (val & SRC_PLLCR_PLL1OVER) {
+                       val &= ~SRC_PLLCR_PLL1EN;
+                       writel(val, src_base + SRC_PLLCR);
+               }
+       } else if (pll->id == 2) {
+               val &= ~SRC_PLLCR_PLL2EN;
+               writel(val, src_base + SRC_PLLCR);
+       }
+       spin_unlock(&src_lock);
+}

Quoting Linus Walleij (2013-06-09 03:55:47)

drivers/clk/clk-nomadik.c line 109

Quoted 6 lines

drivers/clk/clk-nomadik.c +6 -0

+static void pll_clk_disable(struct clk_hw *hw)
+{
+       struct clk_pll *pll = to_pll(hw);
+       u32 val;
+
+       return;

Is the early return intentional? Everything else looks good.

Acked-by Mike Turquette <[email protected]>