clk: mvebu: initial support for AP806 clocks

The drivers/clk/mvebu directory is only being built when CONFIG_PLAT_ORION=y. As we are going to support additional mvebu platforms in drivers/clk/mvebu, which don't have CONFIG_PLAT_ORION=y, we need to recurse into this directory regardless of the value of CONFIG_PLAT_ORION.

Since all files in drivers/clk/mvebu/ are already conditionally compiled depending on various Kconfig options, we can recurse unconditionally into drivers/clk/mvebu without any other change.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/clk/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/clk/Makefile +0 -0

index b038e36..e08bb0e 100644

Hi Thomas,

2016-02-15 8:34 GMT+01:00 Thomas Petazzoni
<[email protected]>:

Quoted 15 lines

The drivers/clk/mvebu directory is only being built when CONFIG_PLAT_ORION=y. As we are going to support additional mvebu platforms in drivers/clk/mvebu, which don't have CONFIG_PLAT_ORION=y, we need to recurse into this directory regardless of the value of CONFIG_PLAT_ORION.

Since all files in drivers/clk/mvebu/ are already conditionally compiled depending on various Kconfig options, we can recurse unconditionally into drivers/clk/mvebu without any other change.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/clk/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/clk/Makefile obj-$(CONFIG_ARCH_MEDIATEK) += mediatek/ · line 64

Quoted 10 lines

drivers/clk/Makefile +1 -1

index b038e36..e08bb0e 100644
@@ -61,7 +61,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)           += mediatek/
 ifeq ($(CONFIG_COMMON_CLK), y)
 obj-$(CONFIG_ARCH_MMP)                 += mmp/
 endif
-obj-$(CONFIG_PLAT_ORION)               += mvebu/
+obj-y                                  += mvebu/

I know it works with obj-y, but how about using obj-$(CONFIG_ARCH_MVEBU)? A person editing this Kconfig in future wouldn't have to run git blame in order to ensure with your commit log, that he doesn't compile anything more than planned.

Best regards,
Marcin

This commit adds a new driver to handle the core clocks found in the AP806 HW block, which is the core block of all Armada 7K and 8K Marvell 64-bits processors. This core clock driver reads the Sample-At-Reset register to determine the frequencies of several core
clocks: DDR, Ring and CPU clocks.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
 drivers/clk/mvebu/Kconfig                          |   3 +
 drivers/clk/mvebu/Makefile                         |   2 +-
 drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
 4 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-core.c

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +0 -0

new file mode 100644
index 0000000..b2131bb

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +4 -0

+
+Clock consumers must specify the desired clock by having the clock ID
+in its "clocks" phandle cell.
+

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +3 -0

+		#clock-cells = <1>;
+		clock-output-names = "ddr", "ring", "cpu";
+	};

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt line 7

Quoted 5 lines

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +5 -0

+The Marvell MVEBU Armada 7K/8K SoCs contain a block called AP806,
+hosting the CPU and other core components of the CPU. This Device Tree
+binding allows to describe the core clocks of the AP806, whose
+frequencies are determined by reading the Sample-At-Reset (SAR)
+register.

What else is in the AP806?

Quoted 9 lines

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +9 -0

+Clock consumers must specify the desired clock by having the clock ID
+in its "clocks" phandle cell.
+
+The following is a list of provided IDs and clock names for the core
+Armada AP806 clocks:
+
+ 0 = DDR
+ 1 = Ring
+ 2 = CPU

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt line 21

Quoted 4 lines

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +4 -0

+
+Required properties:
+- compatible: must be be one of the following:
+	"marvell,armada-ap806-core-clock"

I'd expect this to be a sub-node of a syscon block or just a single clock provider for the full block. Hard to tell reviewing this without context of what the full clock tree looks like.

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt line 28

Quoted 7 lines

+- reg: must be the register address of the Sample-At-Reset (SAR) register
+- #clock-cells: from common clock binding; shall be set to 1
+- clock-output-names: name of the output clocks

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +4 -0

+
+Example:
+
+	coreclk: clk at 0x6F8204 {

Drop 0x and lowercase hex.

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt line 30

Quoted 2 lines

Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt new file +2 -0

+		compatible = "marvell,armada-ap806-core-clock";
+		reg = <0x6F8204 0x04>;

lowercase hex.

Such information will come later, and we can rework the drivers and DT bindings accordingly. Those DT bindings cannot be stable, as the platform is under heavy development and we'll probably discover some issues down the road.

Quoted 4 lines

Such information will come later, and we can rework the drivers and DT bindings accordingly. Those DT bindings cannot be stable, as the platform is under heavy development and we'll probably discover some issues down the road.

Hi Thomas

Maybe add a big fat warning that the bindings are unstable, and the DT blob must be kept in sync with the kernel?

Andrew

Quoting Andrew Lunn (2016-02-22 06:16:17)

Quoted 5 lines

Quoted 4 lines

Such information will come later, and we can rework the drivers and DT bindings accordingly. Those DT bindings cannot be stable, as the platform is under heavy development and we'll probably discover some issues down the road.

Quoted 4 lines

Hi Thomas

Maybe add a big fat warning that the bindings are unstable, and the DT blob must be kept in sync with the kernel?

+1 and feel free to blame the lack of documentation. No one can expect bindings to be finalized when the chip topology is not fully understood.

Quoted 6 lines · Michael Turquette

Quoting Andrew Lunn (2016-02-22 06:16:17)

Quoted 5 lines

Quoted 4 lines

Such information will come later, and we can rework the drivers and DT bindings accordingly. Those DT bindings cannot be stable, as the platform is under heavy development and we'll probably discover some issues down the road.

Quoted 7 lines

Quoted 4 lines

Hi Thomas

Maybe add a big fat warning that the bindings are unstable, and the DT blob must be kept in sync with the kernel?

+1 and feel free to blame the lack of documentation. No one can expect bindings to be finalized when the chip topology is not fully understood.

I can understand not understanding the full clock tree. I have "full" documentation of a Marvell chip and don't understand the clock tree fully. But I can't believe you don't have some sense of how many clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of a single register each for clocks at roughly the same address. That tells me your binding is too fine grained. If you really don't know what is right, then err on the side of a single clock provider node and don't put the clock details in DT.

Rob

Quoted 5 lines

Quoted 2 lines

Maybe add a big fat warning that the bindings are unstable, and the DT blob must be kept in sync with the kernel?

+1 and feel free to blame the lack of documentation. No one can expect bindings to be finalized when the chip topology is not fully understood.

I can understand not understanding the full clock tree. I have "full" documentation of a Marvell chip and don't understand the clock tree fully. But I can't believe you don't have some sense of how many clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of a single register each for clocks at roughly the same address. That tells me your binding is too fine grained. If you really don't know what is right, then err on the side of a single clock provider node and don't put the clock details in DT.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Maybe add a big fat warning that the bindings are unstable, and the DT blob must be kept in sync with the kernel?

+1 and feel free to blame the lack of documentation. No one can expect bindings to be finalized when the chip topology is not fully understood.

Quoted 8 lines

I can understand not understanding the full clock tree. I have "full" documentation of a Marvell chip and don't understand the clock tree fully. But I can't believe you don't have some sense of how many clocks you have to deal with. 10? 100? 1000? What I see is 2 nodes of a single register each for clocks at roughly the same address. That tells me your binding is too fine grained. If you really don't know what is right, then err on the side of a single clock provider node and don't put the clock details in DT.

I don't quite understand the reasoning behind this conclusion. We know for sure that those two registers control only those core and ring clocks. Maybe there are other registers controlling other clocks, that we don't know. But for sure, those two registers only give details about those core and ring clocks, so I don't see what would be the usefulness of merging that into a single DT node.

We would lose the fact that the relationship between the ring clocks and one of the core clock is represented in the DT. Instead of having a clear <&coreclk X> or <&ringclk Y> reference, we would have a mysterious <&allclk XYZ> reference, which doesn't tell immediately whether it's a core clock or ring clock.

So, while I definitely agree that a syscon is probably in order to cover all the system registers, I don't see the point of having a single node to cover all the clocks.

This commit adds a new driver to handle the ring clocks found in the AP806 HW block, which is the core block of all Armada 7K and 8K Marvell 64-bits processors. Those ring clocks are derived from the core ring clock handled by the AP806 core clock driver. The ring clocks are used by various peripherals inside the AP806.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 .../clock/mvebu-armada-ap806-ring-clock.txt        | 32 +++++++++++

 drivers/clk/mvebu/Kconfig                          |  3 +
 drivers/clk/mvebu/Makefile                         |  1 +
 drivers/clk/mvebu/ap806-ring.c                     | 65 ++++++++++++++++++++++
 4 files changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-ring.c

Quoted 6 lines · Thomas Petazzoni

This commit adds a new driver to handle the ring clocks found in the AP806 HW block, which is the core block of all Armada 7K and 8K Marvell 64-bits processors. Those ring clocks are derived from the core ring clock handled by the AP806 core clock driver. The ring clocks are used by various peripherals inside the AP806.

Quoted 3 lines

Signed-off-by Thomas Petazzoni <[email protected]>

---
 .../clock/mvebu-armada-ap806-ring-clock.txt        | 32 +++++++++++

Combine this with the other patch. Similar comments as the other one.

Hello,

The following set of patches add initial support for the main clocks found in the AP806 hardware block, which is the central block used in the newest Marvell 7K and 8K 64-bits processors.

Two new drivers are added, one for the so-called "core" clocks and one for the "ring" clocks. The latter are child clocks from one of the core clocks. The "core" clocks are fixed rate clocks, but their rate is calculated from values in the Sample At Reset register.

The support for the Marvell 7K/8K platform (Device Tree, etc.) will be submitted as a separate patch series.

Thanks!

Thomas

Thomas Petazzoni (3):
clk: unconditionally recurse into clk/mvebu/
clk: mvebu: add AP806 core clock driver
clk: mvebu: add AP806 ring clock driver

 .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
 .../clock/mvebu-armada-ap806-ring-clock.txt        |  32 ++++++
 drivers/clk/Makefile                               |   2 +-
 drivers/clk/mvebu/Kconfig                          |   6 ++
 drivers/clk/mvebu/Makefile                         |   3 +-
 drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
 drivers/clk/mvebu/ap806-ring.c                     |  65 ++++++++++++
 7 files changed, 251 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-core.c
 create mode 100644 drivers/clk/mvebu/ap806-ring.c

Signature · 1 line

2.6.4

Hi Thomas,

Quoting Thomas Petazzoni (2016-02-14 23:34:55)

Quoted 11 lines

Hello,

The following set of patches add initial support for the main clocks found in the AP806 hardware block, which is the central block used in the newest Marvell 7K and 8K 64-bits processors.

Two new drivers are added, one for the so-called "core" clocks and one for the "ring" clocks. The latter are child clocks from one of the core clocks. The "core" clocks are fixed rate clocks, but their rate is calculated from values in the Sample At Reset register.

Quoted 2 lines

The support for the Marvell 7K/8K platform (Device Tree, etc.) will be submitted as a separate patch series.

Yay, new SoC! Just FYI, we worked out an agreement in December[0] that the clk git tree can take DT bindings and headers, and we'll always host them in an immutable branch by default in case arm-soc (or anyone else) needs them. Stephen and I are doing this now.

It's nice for us to review those bindings at the same time that we review the driver.

[0] http://lkml.kernel.org/r/<[email protected]>

Best regards,
Mike

Quoted 26 lines

Thanks!

Thomas

Thomas Petazzoni (3):
clk: unconditionally recurse into clk/mvebu/
clk: mvebu: add AP806 core clock driver
clk: mvebu: add AP806 ring clock driver

 .../clock/mvebu-armada-ap806-core-clock.txt        |  33 ++++++
 .../clock/mvebu-armada-ap806-ring-clock.txt        |  32 ++++++
 drivers/clk/Makefile                               |   2 +-
 drivers/clk/mvebu/Kconfig                          |   6 ++
 drivers/clk/mvebu/Makefile                         |   3 +-
 drivers/clk/mvebu/ap806-core.c                     | 112 +++++++++++++++++++++
 drivers/clk/mvebu/ap806-ring.c                     |  65 ++++++++++++
 7 files changed, 251 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-core-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-armada-ap806-ring-clock.txt
 create mode 100644 drivers/clk/mvebu/ap806-core.c
 create mode 100644 drivers/clk/mvebu/ap806-ring.c

--
2.6.4

Quoted 4 lines

Yay, new SoC! Just FYI, we worked out an agreement in December[0] that the clk git tree can take DT bindings and headers, and we'll always host them in an immutable branch by default in case arm-soc (or anyone else) needs them. Stephen and I are doing this now.

Oops, I misunderstood your point (and I should have looked at the patches before replying). The patches in this series do indeed have the clk DT binding descriptions.

Pardon the noise, nothing to see here, move along ;-)