[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

This patchset adds support for a set of clocks available in Armada 370/XP
known as "Core Divider" clocks.

These are a set of gatable clocks, derived from a fixed PLL at
a configurable ratio. Although this patchset adds support for the more
general case, we only support one of the clocks, namely the NAND ECC clock.

With this infrastructure in place, the addition of the rest of the clocks
would be fairly easy.

In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
to model accurately the SoC clock tree.

A pictorical representation of the clock tree would be:

    PLL
    |
    |
    ---------------------
    |			|
    |			|
    NAND ECC clock	...
    |
    |
    NAND clock

This patchset is based on v3.12-rc2. Any opinions are highly appreciated.

Thanks!

Ezequiel Garcia (5):
  clk: mvebu: Add core-divider clock
  ARM: mvebu: Add Core Divider clock device-tree binding
  ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
  ARM: mvebu: Add the core-divider clock to Armada 370/XP
  ARM: mvebu: Add NAND clock to Armada 370/XP

 .../bindings/clock/mvebu-corediv-clock.txt         |  19 ++
 arch/arm/boot/dts/armada-370-xp.dtsi               |  25 +++
 drivers/clk/mvebu/Kconfig                          |   5 +
 drivers/clk/mvebu/Makefile                         |   1 +
 drivers/clk/mvebu/clk-corediv.c                    | 209 +++++++++++++++++++++
 5 files changed, 259 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
 create mode 100644 drivers/clk/mvebu/clk-corediv.c

-- 
1.8.1.5

[PATCH 1/5] clk: mvebu: Add core-divider clock

[Ezequiel Garcia]

This commit introduces a new group of clocks present in Armada 370/XP
SoCs (called "Core Divider" clocks) and add a provider for them.
The only clock supported for now is the NAND ECC clock, but the
infrastructure to add the rest is already set.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/clk/mvebu/Kconfig       |   5 +
 drivers/clk/mvebu/Makefile      |   1 +
 drivers/clk/mvebu/clk-corediv.c | 209 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/clk/mvebu/clk-corediv.c

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index 0b0f3e7..c339b82 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -4,15 +4,20 @@ config MVEBU_CLK_COMMON
 config MVEBU_CLK_CPU
 	bool
 
+config MVEBU_CLK_COREDIV
+	bool
+
 config ARMADA_370_CLK
 	bool
 	select MVEBU_CLK_COMMON
 	select MVEBU_CLK_CPU
+	select MVEBU_CLK_COREDIV
 
 config ARMADA_XP_CLK
 	bool
 	select MVEBU_CLK_COMMON
 	select MVEBU_CLK_CPU
+	select MVEBU_CLK_COREDIV
 
 config DOVE_CLK
 	bool
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 1c7e70c..21bbfb4 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_MVEBU_CLK_COMMON)	+= common.o
 obj-$(CONFIG_MVEBU_CLK_CPU) 	+= clk-cpu.o
+obj-$(CONFIG_MVEBU_CLK_COREDIV)	+= clk-corediv.o
 
 obj-$(CONFIG_ARMADA_370_CLK)	+= armada-370.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
diff --git a/drivers/clk/mvebu/clk-corediv.c b/drivers/clk/mvebu/clk-corediv.c
new file mode 100644
index 0000000..2472952
--- /dev/null
+++ b/drivers/clk/mvebu/clk-corediv.c
@@ -0,0 +1,209 @@
+/*
+ * MVEBU Core divider clock
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include "common.h"
+
+#define CORE_CLOCK_DIVIDER_RATIO_MASK		0xff
+#define CORE_CLOCK_DIVIDER_RATIO_RELOAD		BIT(8)
+#define CORE_CLOCK_DIVIDER_ENABLE_OFFSET	24
+#define CORE_CLOCK_DIVIDER_RATIO_OFFSET		0x8
+
+struct clk_corediv_desc {
+	unsigned int mask;
+	unsigned int offset;
+	unsigned int fieldbit;
+};
+
+struct clk_corediv {
+	struct clk_hw hw;
+	void __iomem *reg;
+	struct clk_corediv_desc desc;
+};
+
+static struct clk_onecell_data clk_data;
+
+static const struct clk_corediv_desc __initconst mvebu_corediv_desc[] = {
+	{ .mask = 0x3f, .offset = 8, .fieldbit = 1 }, /* NAND ECC clock */
+};
+
+#define to_corediv_clk(p) container_of(p, struct clk_corediv, hw)
+
+static int clk_corediv_is_enabled(struct clk_hw *hwclk)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 enable_mask = BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET;
+
+	u32 reg = readl(corediv->reg);
+	return !!(readl(corediv->reg) & enable_mask);
+}
+
+static int clk_corediv_enable(struct clk_hw *hwclk)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 reg;
+
+	reg = readl(corediv->reg);
+	reg |= (BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
+	writel(reg, corediv->reg);
+	return 0;
+}
+
+static void clk_corediv_disable(struct clk_hw *hwclk)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 reg;
+
+	reg = readl(corediv->reg);
+	reg &= ~(BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
+	writel(reg, corediv->reg);
+}
+
+static unsigned long clk_corediv_recalc_rate(struct clk_hw *hwclk,
+					 unsigned long parent_rate)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 reg, div;
+
+	reg = readl(corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);
+	div = (reg >> desc->offset) & desc->mask;
+	return (parent_rate / div);
+}
+
+static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
+	u32 div;
+
+	div = *parent_rate / rate;
+	if (div <= 4)
+		div = 4;
+	else if (div <= 5)
+		div = 5;
+	else if (div <= 6)
+		div = 6;
+	else
+		div = 8;
+
+	return *parent_rate / div;
+}
+
+static int clk_corediv_set_rate(struct clk_hw *hwclk, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 reg, div;
+
+	div = parent_rate / rate;
+
+	/* Write new divider to the divider ratio register */
+	reg = readl(corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);
+	reg &= ~(desc->mask << desc->offset);
+	reg |= (div & desc->mask) << desc->offset;
+	writel(reg, corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);
+
+	/* Set reload-force for this clock */
+	reg = readl(corediv->reg) | BIT(desc->fieldbit);
+	writel(reg, corediv->reg);
+
+	/* Now trigger the clock update */
+	reg = readl(corediv->reg) | CORE_CLOCK_DIVIDER_RATIO_RELOAD;
+	writel(reg, corediv->reg);
+
+	/*
+	 * Wait for clocks to settle down, and then clear all the
+	 * ratios request and the reload request.
+	 */
+	udelay(1000);
+	reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
+	writel(reg, corediv->reg);
+	udelay(1000);
+
+	return 0;
+}
+
+static const struct clk_ops corediv_ops = {
+	.enable = clk_corediv_enable,
+	.disable = clk_corediv_disable,
+	.is_enabled = clk_corediv_is_enabled,
+	.recalc_rate = clk_corediv_recalc_rate,
+	.round_rate = clk_corediv_round_rate,
+	.set_rate = clk_corediv_set_rate,
+};
+
+static void __init mvebu_corediv_clk_init(struct device_node *node)
+{
+	struct clk_init_data init;
+	struct clk_corediv *corediv;
+	struct clk **clks;
+	void __iomem *base;
+	const char *parent_name;
+	const char *clk_name;
+	int i;
+
+	base = of_iomap(node, 0);
+	if (WARN_ON(!base))
+		return;
+
+	parent_name = of_clk_get_parent_name(node, 0);
+
+	clk_data.clk_num = ARRAY_SIZE(mvebu_corediv_desc);
+
+	/* clks holds the clock array */
+	clks = kcalloc(clk_data.clk_num, sizeof(struct clk *),
+				GFP_KERNEL);
+	if (WARN_ON(!clks))
+		goto err_unmap;
+	/* corediv holds the clock specific array */
+	corediv = kcalloc(clk_data.clk_num, sizeof(struct clk_corediv),
+				GFP_KERNEL);
+	if (WARN_ON(!corediv))
+		goto err_free_clks;
+
+	for (i = 0; i < clk_data.clk_num; i++) {
+		of_property_read_string_index(node, "clock-output-names",
+					      i, &clk_name);
+		init.num_parents = 1;
+		init.parent_names = &parent_name;
+		init.name = clk_name;
+		init.ops = &corediv_ops;
+		init.flags = 0;
+
+		corediv[i].desc = mvebu_corediv_desc[i];
+		corediv[i].reg = base;
+		corediv[i].hw.init = &init;
+
+		clks[i] = clk_register(NULL, &corediv[i].hw);
+		WARN_ON(IS_ERR(clks[i]));
+	}
+
+	clk_data.clks = clks;
+	of_clk_add_provider(node, of_clk_src_onecell_get, &clk_data);
+	return;
+
+err_free_clks:
+	kfree(clks);
+err_unmap:
+	iounmap(base);
+}
+CLK_OF_DECLARE(mvebu_corediv_clk, "marvell,armada-370-corediv-clock",
+	       mvebu_corediv_clk_init);
-- 
1.8.1.5

[PATCH 2/5] ARM: mvebu: Add Core Divider clock device-tree binding

[Ezequiel Garcia]

The Armada 370/XP SoCs have a Core Divider clock providing
several clocks. For now, only the NAND clock is supported.

Cc: devicetree at vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/clock/mvebu-corediv-clock.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
new file mode 100644
index 0000000..ea60f4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
@@ -0,0 +1,19 @@
+* Core Divider Clock bindings for Marvell MVEBU SoCs
+
+The following is a list of provided IDs and clock names on Armada 370/XP:
+ 0 = nand_ecc   (NAND ECC clock)
+
+Required properties:
+- compatible : must be "marvell,armada-370-corediv-clock"
+- reg : must be the register address of Core Divider control register
+- #clock-cells : from common clock binding; shall be set to 1
+- #clocks : must be set to the parent's phandle
+
+Example:
+
+corediv_clk: corediv-clocks at 18740 {
+	compatible = "marvell,armada-370-corediv-clock";
+	reg = <0x18740 0x8>;
+	#clock-cells = <1>;
+	clocks = <&pll>;
+};
-- 
1.8.1.5

[PATCH 3/5] ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP

[Ezequiel Garcia]

Armada 370/XP SoCs have a 2 GHz fixed PLL that is used to feed
other clocks. This commit adds a DT representation of this clock
through a fixed-clock compatible node.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 1de2dae..924c721 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -252,4 +252,13 @@
 
 		};
 	};
+
+	clocks {
+		/* 2 GHz fixed main PLL */
+		mainpll: mainpll {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <2000000000>;
+		};
+	};
  };
-- 
1.8.1.5

[PATCH 4/5] ARM: mvebu: Add the core-divider clock to Armada 370/XP

[Ezequiel Garcia]

The Armada 370/XP SoC has a clock provider called "Core Divider",
that is derived from a fixed 2 GHz PLL clock.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 924c721..d0f81cc 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -137,6 +137,14 @@
 				status = "disabled";
 			};
 
+			coredivclk: corediv-clock at 18740 {
+				compatible = "marvell,armada-370-corediv-clock";
+				reg = <0x18740 0x8>;
+				#clock-cells = <1>;
+				clocks = <&mainpll>;
+				clock-output-names = "nandecc";
+			};
+
 			timer at 20300 {
 				reg = <0x20300 0x30>, <0x21040 0x30>;
 				interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
-- 
1.8.1.5

[PATCH 5/5] ARM: mvebu: Add NAND clock to Armada 370/XP

[Ezequiel Garcia]

Add the NAND clock available at Armada 370/XP SoC, which is a
fixed-factor derived from the NAND ECC clock. This is the clock
feeding the NAND controller.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index d0f81cc..8f14092 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -268,5 +268,13 @@
 			#clock-cells = <0>;
 			clock-frequency = <2000000000>;
 		};
+
+		ndclk: nand_clock {
+			compatible = "fixed-factor-clock";
+			#clock-cells = <0>;
+			clocks = <&coredivclk 0>;
+	                clock-div = <2>;
+	                clock-mult = <1>;
+		};
 	};
  };
-- 
1.8.1.5

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

(Ccing forgotten mvebu maintainers)

On Wed, Sep 25, 2013 at 06:28:17PM -0300, Ezequiel Garcia wrote:
> This patchset adds support for a set of clocks available in Armada 370/XP
> known as "Core Divider" clocks.
> 
> These are a set of gatable clocks, derived from a fixed PLL at
> a configurable ratio. Although this patchset adds support for the more
> general case, we only support one of the clocks, namely the NAND ECC clock.
> 
> With this infrastructure in place, the addition of the rest of the clocks
> would be fairly easy.
> 
> In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
> to model accurately the SoC clock tree.
> 
> A pictorical representation of the clock tree would be:
> 
>     PLL
>     |
>     |
>     ---------------------
>     |			|
>     |			|
>     NAND ECC clock	...
>     |
>     |
>     NAND clock
> 
> This patchset is based on v3.12-rc2. Any opinions are highly appreciated.
> 
> Thanks!
> 
> Ezequiel Garcia (5):
>   clk: mvebu: Add core-divider clock
>   ARM: mvebu: Add Core Divider clock device-tree binding
>   ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
>   ARM: mvebu: Add the core-divider clock to Armada 370/XP
>   ARM: mvebu: Add NAND clock to Armada 370/XP
> 
>  .../bindings/clock/mvebu-corediv-clock.txt         |  19 ++
>  arch/arm/boot/dts/armada-370-xp.dtsi               |  25 +++
>  drivers/clk/mvebu/Kconfig                          |   5 +
>  drivers/clk/mvebu/Makefile                         |   1 +
>  drivers/clk/mvebu/clk-corediv.c                    | 209 +++++++++++++++++++++
>  5 files changed, 259 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
>  create mode 100644 drivers/clk/mvebu/clk-corediv.c
> 
> -- 
> 1.8.1.5
> 

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Gregory CLEMENT]

Hi Ezequiel,

On 25/09/2013 23:37, Ezequiel Garcia wrote:
> (Ccing forgotten mvebu maintainers)
> 
> On Wed, Sep 25, 2013 at 06:28:17PM -0300, Ezequiel Garcia wrote:
>> This patchset adds support for a set of clocks available in Armada 370/XP
>> known as "Core Divider" clocks.
>>
>> These are a set of gatable clocks, derived from a fixed PLL at
>> a configurable ratio. Although this patchset adds support for the more
>> general case, we only support one of the clocks, namely the NAND ECC clock.
>>
>> With this infrastructure in place, the addition of the rest of the clocks
>> would be fairly easy.
>>
>> In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
>> to model accurately the SoC clock tree.
>>
>> A pictorical representation of the clock tree would be:
>>
>>     PLL
>>     |
>>     |
>>     ---------------------
>>     |			|
>>     |			|
>>     NAND ECC clock	...
>>     |
>>     |
>>     NAND clock
>>
>> This patchset is based on v3.12-rc2. Any opinions are highly appreciated.
>>

For the whole series you can add my
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

I just wonder if you were enable to test the enable and disable for the clock,
because the datasheet is a little fuzzy about it.

Thanks,

Gregory
>> Thanks!
>>
>> Ezequiel Garcia (5):
>>   clk: mvebu: Add core-divider clock
>>   ARM: mvebu: Add Core Divider clock device-tree binding
>>   ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
>>   ARM: mvebu: Add the core-divider clock to Armada 370/XP
>>   ARM: mvebu: Add NAND clock to Armada 370/XP
>>
>>  .../bindings/clock/mvebu-corediv-clock.txt         |  19 ++
>>  arch/arm/boot/dts/armada-370-xp.dtsi               |  25 +++
>>  drivers/clk/mvebu/Kconfig                          |   5 +
>>  drivers/clk/mvebu/Makefile                         |   1 +
>>  drivers/clk/mvebu/clk-corediv.c                    | 209 +++++++++++++++++++++
>>  5 files changed, 259 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
>>  create mode 100644 drivers/clk/mvebu/clk-corediv.c
>>
>> -- 
>> 1.8.1.5
>>
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 09:38:19AM +0200, Gregory CLEMENT wrote:
> Hi Ezequiel,
> 
> On 25/09/2013 23:37, Ezequiel Garcia wrote:
> > (Ccing forgotten mvebu maintainers)
> > 
> > On Wed, Sep 25, 2013 at 06:28:17PM -0300, Ezequiel Garcia wrote:
> >> This patchset adds support for a set of clocks available in Armada 370/XP
> >> known as "Core Divider" clocks.
> >>
> >> These are a set of gatable clocks, derived from a fixed PLL at
> >> a configurable ratio. Although this patchset adds support for the more
> >> general case, we only support one of the clocks, namely the NAND ECC clock.
> >>
> >> With this infrastructure in place, the addition of the rest of the clocks
> >> would be fairly easy.
> >>
> >> In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
> >> to model accurately the SoC clock tree.
> >>
> >> A pictorical representation of the clock tree would be:
> >>
> >>     PLL
> >>     |
> >>     |
> >>     ---------------------
> >>     |			|
> >>     |			|
> >>     NAND ECC clock	...
> >>     |
> >>     |
> >>     NAND clock
> >>
> >> This patchset is based on v3.12-rc2. Any opinions are highly appreciated.
> >>
> 
> For the whole series you can add my
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 

Great! However, I found some issues on this patchset :-(, see below.

> I just wonder if you were enable to test the enable and disable for the clock,
> because the datasheet is a little fuzzy about it.
> 

Enabling and disabling the clock seems to work OK. I must admit I did
just a simple test:

* If NAND DT node does not consume the correct clock, the controller stops
working just as soon as the core clock framework disables the unused clocks.
* If you make the NAND DT node consumer of the "ndclk", the controller
  works fine.

On the other side, I found this approach is incorrect! (silly me!).

Declaring the NAND clock as a fixed-ratio from the NAND ECC clock,
and making the NAND DT consume the NAND clock removes the possibility
of setting the clock rate from the NAND driver.

This happens because the controller's clock is declared as fixed,
whereas the clock that can be adjusted is the NAND ECC (the parent of NAND
clock).

I've been thinking on how to work this out in a clean way, but I'm not
sure which way is cleaner/better. These are my options so far, feel free
to propose any other:

1. The NAND DT node consumes the two clocks: NAND ECC and NAND clock.
It uses the former to set the rate, and the latter to read the rate to
calculate the timings.

This means we have to modify the NAND driver (but not too much).

2. We register the NAND clock in the clk-corediv driver, as a child of
NAND ECC, and implement some set_rate operation to act on NAND ECC.
The NAND DT node can then consume this clock.

This has the advantage that it's transparent to the NAND driver
(which currently consumes one clock).

3. We hack the NAND driver to consume the NAND ECC, but use the rate
as the half of it, and forget about the halved-rate NAND clock.
This seems certainly hacky.

Thoughts?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Thu, 26 Sep 2013 12:26:52 -0300, Ezequiel Garcia wrote:

> 3. We hack the NAND driver to consume the NAND ECC, but use the rate
> as the half of it, and forget about the halved-rate NAND clock.
> This seems certainly hacky.

Is this really hacky? Since we can't change the rate of one without
changing the other, or gating the one without the other, we can also
see those two clocks as being an internal business of the NAND hardware
block. So instead of seeing things as:

                        -------------
 NAND ECC clk    ---->  |           | 
     ||                 |  NAND HW  |
     ||                 |   block   |
     \/                 |           |
 NAND clk        ---->  |           |
                        -------------

You can see things as follows:

                        -------------
                        |           | 
                        |  NAND HW  |
 NAND clk        ---->  |   block   |
                        |           |
                        |           |
                        -------------

and the ECC clock is actually some internal business of the NAND hw
block, and therefore handled internally by the NAND driver, as your
option (3) suggests.

Since the amount of details that we have about the exact hardware
architecture are pretty scarce, I believe this is probably the easiest
solution.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 05:47:55PM +0200, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
> 
> On Thu, 26 Sep 2013 12:26:52 -0300, Ezequiel Garcia wrote:
> 
> > 3. We hack the NAND driver to consume the NAND ECC, but use the rate
> > as the half of it, and forget about the halved-rate NAND clock.
> > This seems certainly hacky.
> 
> Is this really hacky? Since we can't change the rate of one without
> changing the other, or gating the one without the other, we can also
> see those two clocks as being an internal business of the NAND hardware
> block. So instead of seeing things as:
> 
>                         -------------
>  NAND ECC clk    ---->  |           | 
>      ||                 |  NAND HW  |
>      ||                 |   block   |
>      \/                 |           |
>  NAND clk        ---->  |           |
>                         -------------
> 
> You can see things as follows:
> 
>                         -------------
>                         |           | 
>                         |  NAND HW  |
>  NAND clk        ---->  |   block   |
>                         |           |
>                         |           |
>                         -------------
> 
> and the ECC clock is actually some internal business of the NAND hw
> block, and therefore handled internally by the NAND driver, as your
> option (3) suggests.
> 
> Since the amount of details that we have about the exact hardware
> architecture are pretty scarce, I believe this is probably the easiest
> solution.
> 

Hm... could be. Considering the lack of hardware details (as you
point out) maybe this is indeed the best option.

And it has the cool advantage of simplfying the clock tree, which
is unnecessarily complex with the NAND ECC clock -> NAND clock layout.

On the other side, Emilio has just pointed out (in private) that there's
a flag "CLK_SET_RATE_PARENT" that is meant for these cases.

Anyway, I feel inclined to your suggestion and just forget about the NAND
ECC clock for good, which is only confusing.

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Thu, 26 Sep 2013 13:00:11 -0300, Ezequiel Garcia wrote:

> Hm... could be. Considering the lack of hardware details (as you
> point out) maybe this is indeed the best option.
> 
> And it has the cool advantage of simplfying the clock tree, which
> is unnecessarily complex with the NAND ECC clock -> NAND clock layout.
> 
> On the other side, Emilio has just pointed out (in private) that
> there's a flag "CLK_SET_RATE_PARENT" that is meant for these cases.
> 
> Anyway, I feel inclined to your suggestion and just forget about the
> NAND ECC clock for good, which is only confusing.

Yes, I also believe it's the easiest solution.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 1/5] clk: mvebu: Add core-divider clock

[Andrew Lunn]

Hi Ezequiel

> +static int clk_corediv_enable(struct clk_hw *hwclk)
> +{
> +	struct clk_corediv *corediv = to_corediv_clk(hwclk);
> +	struct clk_corediv_desc *desc = &corediv->desc;
> +	u32 reg;
> +
> +	reg = readl(corediv->reg);
> +	reg |= (BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
> +	writel(reg, corediv->reg);
> +	return 0;
> +}

Shouldn't there be spinlocks around these register accesses? At least
the core gate clk driver has a spinlock.

> +static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
> +	     				          unsigned long *parent_rate)
> +{
> +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
> +	u32 div;
> +
> +	div = *parent_rate / rate;
> +	if (div <= 4)
> +	   div = 4;
> +	   else if (div <= 5)
> +	   	div = 5;
> +		else if (div <= 6)
> +		     div = 6;
> +		     else
> +			div = 8;
> +
> +	return *parent_rate / div;
> +}

This looks odd. Is not the following clearer?

	div = *parent_rate / rate;
	if (div < 5)
	   div = 4;
	   else if (div > 6)
		div = 8;

The CodingStyle might require some {} here?

+   /*
+    * Wait for clocks to settle down, and then clear all the
+     * ratios request and the reload request.
+      */
+      udelay(1000);
+      reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
+      writel(reg, corediv->reg);
+      udelay(1000);


Documentation/timers/timers-howto.txt says: 

       SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
       		* Use usleep_range

	Andrew

[PATCH 1/5] clk: mvebu: Add core-divider clock

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 10:24:04AM +0200, Andrew Lunn wrote:
> Hi Ezequiel
> 
> > +static int clk_corediv_enable(struct clk_hw *hwclk)
> > +{
> > +	struct clk_corediv *corediv = to_corediv_clk(hwclk);
> > +	struct clk_corediv_desc *desc = &corediv->desc;
> > +	u32 reg;
> > +
> > +	reg = readl(corediv->reg);
> > +	reg |= (BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
> > +	writel(reg, corediv->reg);
> > +	return 0;
> > +}
> 
> Shouldn't there be spinlocks around these register accesses? At least
> the core gate clk driver has a spinlock.
> 

Indeed.

> > +static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
> > +	     				          unsigned long *parent_rate)
> > +{
> > +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
> > +	u32 div;
> > +
> > +	div = *parent_rate / rate;
> > +	if (div <= 4)
> > +	   div = 4;
> > +	   else if (div <= 5)
> > +	   	div = 5;
> > +		else if (div <= 6)
> > +		     div = 6;
> > +		     else
> > +			div = 8;
> > +
> > +	return *parent_rate / div;
> > +}
> 
> This looks odd. Is not the following clearer?
> 
> 	div = *parent_rate / rate;
> 	if (div < 5)
> 	   div = 4;
> 	   else if (div > 6)
> 		div = 8;
> 
> The CodingStyle might require some {} here?
> 

Mmmm... no, it's not at all clearer to me.
IMHO, the original construction explicitly show the possible ratios:

	/* If it's smaller than or equal to 4, set to 4 */
	if (div <= 4)
		div = 4;

	/* Otherwise, if it's between 4 and 5, set to 5 */
	else if (div <= 5)
	 	div = 5;

	/* Otherwise, if it's between 5 and 6, set to 6 */
	else if (div <= 6)
		div = 6;

	/* Otherwise, if it's bigger than 6, set to 8 */
	else
		div = 8;

(And I don't think we need any braces).

Is this not clear?

> +   /*
> +    * Wait for clocks to settle down, and then clear all the
> +     * ratios request and the reload request.
> +      */
> +      udelay(1000);
> +      reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
> +      writel(reg, corediv->reg);
> +      udelay(1000);
> 
> 
> Documentation/timers/timers-howto.txt says: 
> 
>        SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>        		* Use usleep_range
> 

Right, forgot about that as well...

Thanks for the feedback!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 1/5] clk: mvebu: Add core-divider clock

[Andrew Lunn]

> > > +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
> > > +	u32 div;
> > > +
> > > +	div = *parent_rate / rate;
> > > +	if (div <= 4)
> > > +	   div = 4;
> > > +	   else if (div <= 5)
> > > +	   	div = 5;
> > > +		else if (div <= 6)
> > > +		     div = 6;
> > > +		     else
> > > +			div = 8;
> > > +
> > > +	return *parent_rate / div;
> > > +}
> > 
> > This looks odd. Is not the following clearer?
> > 
> > 	div = *parent_rate / rate;
> > 	if (div < 5)
> > 	   div = 4;
> > 	   else if (div > 6)
> > 		div = 8;
> > 
> 
> Mmmm... no, it's not at all clearer to me.
> IMHO, the original construction explicitly show the possible ratios:
> 
> 	/* If it's smaller than or equal to 4, set to 4 */
> 	if (div <= 4)
> 		div = 4;
> 
> 	/* Otherwise, if it's between 4 and 5, set to 5 */
> 	else if (div <= 5)
> 	 	div = 5;

If div was a float or double, i would probably agree. But its a u32.
It cannot be between 4 and 5. It must be 5. So it becomes

 	if (div <= 4)
 		div = 4;
        else if (div == 5)
	        div = 5;
        else if (div == 6)
                div = 6
        else 
	    div = 8

Those two middle statements look odd to me...

      Andrew

[PATCH 1/5] clk: mvebu: Add core-divider clock

[Ben Dooks]

On 26/09/13 16:12, Ezequiel Garcia wrote:
> On Thu, Sep 26, 2013 at 10:24:04AM +0200, Andrew Lunn wrote:
>> Hi Ezequiel
>>
>>> +static int clk_corediv_enable(struct clk_hw *hwclk)
>>> +{
>>> +	struct clk_corediv *corediv = to_corediv_clk(hwclk);
>>> +	struct clk_corediv_desc *desc =&corediv->desc;
>>> +	u32 reg;
>>> +
>>> +	reg = readl(corediv->reg);
>>> +	reg |= (BIT(desc->fieldbit)<<  CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
>>> +	writel(reg, corediv->reg);
>>> +	return 0;
>>> +}
>>
>> Shouldn't there be spinlocks around these register accesses? At least
>> the core gate clk driver has a spinlock.
>>
>
> Indeed.
>
>>> +static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
>>> +	     				          unsigned long *parent_rate)
>>> +{
>>> +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
>>> +	u32 div;
>>> +
>>> +	div = *parent_rate / rate;
>>> +	if (div<= 4)
>>> +	   div = 4;
>>> +	   else if (div<= 5)
>>> +	   	div = 5;
>>> +		else if (div<= 6)
>>> +		     div = 6;
>>> +		     else
>>> +			div = 8;
>>> +
>>> +	return *parent_rate / div;
>>> +}
>>
>> This looks odd. Is not the following clearer?
>>
>> 	div = *parent_rate / rate;
>> 	if (div<  5)
>> 	   div = 4;
>> 	   else if (div>  6)
>> 		div = 8;
>>
>> The CodingStyle might require some {} here?
>>
>
> Mmmm... no, it's not at all clearer to me.
> IMHO, the original construction explicitly show the possible ratios:
>
> 	/* If it's smaller than or equal to 4, set to 4 */
> 	if (div<= 4)
> 		div = 4;
>
> 	/* Otherwise, if it's between 4 and 5, set to 5 */
> 	else if (div<= 5)
> 	 	div = 5;

how can an integer be between 4 and 5? surely it is 4 or 5.

> 	/* Otherwise, if it's between 5 and 6, set to 6 */
> 	else if (div<= 6)
> 		div = 6;

see above.

> 	/* Otherwise, if it's bigger than 6, set to 8 */
> 	else
> 		div = 8;
>
> (And I don't think we need any braces).
>
> Is this not clear?
>
>> +   /*
>> +    * Wait for clocks to settle down, and then clear all the
>> +     * ratios request and the reload request.
>> +      */
>> +      udelay(1000);
>> +      reg&= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
>> +      writel(reg, corediv->reg);
>> +      udelay(1000);
>>
>>
>> Documentation/timers/timers-howto.txt says:
>>
>>         SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>>         		* Use usleep_range
>>
>
> Right, forgot about that as well...
>
> Thanks for the feedback!


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 1/5] clk: mvebu: Add core-divider clock

[Ezequiel Garcia]

Hey Andrew,

On Thu, Sep 26, 2013 at 10:24:04AM +0200, Andrew Lunn wrote:
> 
> +   /*
> +    * Wait for clocks to settle down, and then clear all the
> +     * ratios request and the reload request.
> +      */
> +      udelay(1000);
> +      reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
> +      writel(reg, corediv->reg);
> +      udelay(1000);
> 
> 
> Documentation/timers/timers-howto.txt says: 
> 
>        SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>        		* Use usleep_range
> 

Given the set_rate should be protected by spinlocks (as you pointed out),
I guess we can't afford using usleep_range (the context is now non-atomic).

Now, according to arch/arm/include/asm/delay.h this is the suggestion
for udelay and friends:

 * Use only for very small delays ( < 2 msec).  Should probably use a
 * lookup table, really, as the multiplications take much too long with
 * short delays.  This is a "reasonable" implementation, though (and
 * the first constant multiplications gets optimized away if the delay is
 * a constant)  

Which means we could just use udelay(1000) without much trouble.

On the other side, since we know the rate the clock is running, we could
use __delay() just as drivers/clk/sunxi/clk-factors.c is doing.
__delay() has a much simpler implementation, which feels correct in
this case of "wait until the clock settles".

Therefore, I'm currently going to try that option.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 2/5] ARM: mvebu: Add Core Divider clock device-tree binding

[Andrew Lunn]

Hi Ezequiel

+#define CORE_CLOCK_DIVIDER_RATIO_OFFSET		0x8
...

reg = readl(corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);

...

+Example:
+
+corediv_clk: corediv-clocks at 18740 {
+	      compatible = "marvell,armada-370-corediv-clock";
+	      reg = <0x18740 0x8>;
+	      #clock-cells = <1>;
+	      clocks = <&pll>;
+};

The reg property seems to be wrong.

    Andrew

[PATCH 2/5] ARM: mvebu: Add Core Divider clock device-tree binding

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 10:36:55AM +0200, Andrew Lunn wrote:
> Hi Ezequiel
> 
> +#define CORE_CLOCK_DIVIDER_RATIO_OFFSET		0x8
> ...
> 
> reg = readl(corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);
> 
> ...
> 
> +Example:
> +
> +corediv_clk: corediv-clocks at 18740 {
> +	      compatible = "marvell,armada-370-corediv-clock";
> +	      reg = <0x18740 0x8>;
> +	      #clock-cells = <1>;
> +	      clocks = <&pll>;
> +};
> 
> The reg property seems to be wrong.
> 

Mm.. good catch!

Thanks for the review,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com