[PATCH v2 0/6] Add clock support for Armada 37xx SoCs

[PATCH v2 0/6] Add clock support for Armada 37xx SoCs

[Gregory CLEMENT]

Hi,

eventually this is the second version of the series adding clock
support for the Armada 37xx SoCs.

The design of the drivers is as close as possible as the hardware is,
with some clocks made of several layers: muxing, divider and gating.

The device tree binding was written in a way that even if we discover
some change inside the clocks, the binding should not be
affected. Especially, there are some holes in the clocks, but we
should be able to add them seamless.

Since the firs version the main change is the use of clk_hw instead of
clk for the registration. I also removed the dt related patches as they
were already applied.

However the full series is available on the branch
Armada-3700-Clocks-v2 at
git@github.com:MISL-EBU-System-SW/mainline-public.git

Thanks,

Changelog:

v1 -> v2

- Move to clk_hw based registration for the 3 clock driver as
  requested by Stephen Boyd
- Fixed typo noticed by Stephen Boyd
- Added const and static when they were missing: suggested by Stephen
  Boyd
- Allocated the driver variable during probe instead of using a global
  one as requested by Stephen Boyd
- Managed the failure of the of_clk_add_hw_provider call
- Added the Acked-by from Rob Herring on the dt binding patchs

Gregory CLEMENT (6):
  dt-bindings: clock: add DT binding for the Xtal clock on Armada 3700
  clk: mvebu: Add the xtal clock for Armada 3700 SoC
  dt-bindings: clock: add DT binding for the TBG clocks on Armada 3700
  clk: mvebu Add the time base generator clocks for Armada 3700
  dt-bindings: clock: add DT binding for the peripheral clocks on Armada
    3700
  clk: mvebu: Add the peripheral clock driver for Armada 3700

 .../bindings/clock/armada3700-periph-clock.txt     |  70 ++++
 .../bindings/clock/armada3700-tbg-clock.txt        |  27 ++
 .../bindings/clock/armada3700-xtal-clock.txt       |  28 ++
 drivers/clk/mvebu/Kconfig                          |   3 +
 drivers/clk/mvebu/Makefile                         |   3 +
 drivers/clk/mvebu/armada-37xx-periph.c             | 457 +++++++++++++++++++++
 drivers/clk/mvebu/armada-37xx-tbg.c                | 165 ++++++++
 drivers/clk/mvebu/armada-37xx-xtal.c               |  98 +++++
 8 files changed, 851 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-tbg-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt
 create mode 100644 drivers/clk/mvebu/armada-37xx-periph.c
 create mode 100644 drivers/clk/mvebu/armada-37xx-tbg.c
 create mode 100644 drivers/clk/mvebu/armada-37xx-xtal.c

-- 
2.5.0

[PATCH v2 1/6] dt-bindings: clock: add DT binding for the Xtal clock on Armada 3700

[Gregory CLEMENT]

This commit adds the DT binding documentation for the the Xtal clock on
Armada 3700 used in the Marvell Armada 3700 SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/armada3700-xtal-clock.txt       | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt
new file mode 100644
index 000000000000..176c4fe6f0e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt
@@ -0,0 +1,28 @@
+* Xtal Clock bindings for Marvell Armada 37xx SoCs
+
+Marvell Armada 37xx SoCs allow to determine the xtal clock frequencies by
+reading the gpio latch register.
+
+This node must be a subnode of the node exposing the register address
+of the GPIO block where the gpio latch is located.
+
+Required properties:
+- compatible : shall be one of the following:
+	"marvell,armada-3700-xtal-clock"
+- #clock-cells : from common clock binding; shall be set to 0
+
+Optional properties:
+- clock-output-names : from common clock binding; allows overwrite default clock
+	output names ("xtal")
+
+Example:
+gpio1: gpio@13800 {
+	compatible = "marvell,mvebu-gpio-3700",	"syscon", "simple-mfd";
+	reg = <0x13800 0x1000>;
+
+	xtalclk: xtal-clk {
+		compatible = "marvell,armada-3700-xtal-clock";
+		clock-output-names = "xtal";
+		#clock-cells = <0>;
+	};
+};
-- 
2.5.0

[PATCH v2 2/6] clk: mvebu: Add the xtal clock for Armada 3700 SoC

[Gregory CLEMENT]

This clock is the parent of all the Armada 3700 clocks. It is a fixed
rate clock which depends on the gpio configuration read when resetting
the SoC.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/mvebu/Kconfig            |  3 ++
 drivers/clk/mvebu/Makefile           |  1 +
 drivers/clk/mvebu/armada-37xx-xtal.c | 98 ++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 drivers/clk/mvebu/armada-37xx-xtal.c

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index 3165da77d525..fddc8ac5faff 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -24,6 +24,9 @@ config ARMADA_39X_CLK
 	bool
 	select MVEBU_CLK_COMMON
 
+config ARMADA_37XX_CLK
+       bool
+
 config ARMADA_XP_CLK
 	bool
 	select MVEBU_CLK_COMMON
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 7172ef65693d..4257a36d0219 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_ARMADA_370_CLK)	+= armada-370.o
 obj-$(CONFIG_ARMADA_375_CLK)	+= armada-375.o
 obj-$(CONFIG_ARMADA_38X_CLK)	+= armada-38x.o
 obj-$(CONFIG_ARMADA_39X_CLK)	+= armada-39x.o
+obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-xtal.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
 obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
 obj-$(CONFIG_ARMADA_CP110_SYSCON) += cp110-system-controller.o
diff --git a/drivers/clk/mvebu/armada-37xx-xtal.c b/drivers/clk/mvebu/armada-37xx-xtal.c
new file mode 100644
index 000000000000..f832c219420f
--- /dev/null
+++ b/drivers/clk/mvebu/armada-37xx-xtal.c
@@ -0,0 +1,98 @@
+/*
+ * Marvell Armada 37xx SoC xtal clocks
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@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/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define NB_GPIO1_LATCH	0xC
+#define XTAL_MODE	    BIT(31)
+
+static int armada_3700_xtal_clock_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const char *xtal_name = "xtal";
+	struct device_node *parent;
+	struct regmap *regmap;
+	struct clk_hw *xtal_hw;
+	unsigned int rate;
+	u32 reg;
+	int ret;
+
+	xtal_hw = devm_kzalloc(&pdev->dev, sizeof(*xtal_hw), GFP_KERNEL);
+	if (!xtal_hw)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, xtal_hw);
+
+	parent = np->parent;
+	if (!parent) {
+		dev_err(&pdev->dev, "no parent\n");
+		return -ENODEV;
+	}
+
+	regmap = syscon_node_to_regmap(parent);
+	if (IS_ERR(regmap)) {
+		dev_err(&pdev->dev, "cannot get regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	ret = regmap_read(regmap, NB_GPIO1_LATCH, &reg);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot read from regmap\n");
+		return ret;
+	}
+
+	if (reg & XTAL_MODE)
+		rate = 40000000;
+	else
+		rate = 25000000;
+
+	of_property_read_string_index(np, "clock-output-names", 0, &xtal_name);
+	xtal_hw = clk_hw_register_fixed_rate(NULL, xtal_name, NULL, 0, rate);
+	if (IS_ERR(xtal_hw))
+		return PTR_ERR(xtal_hw);
+	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, xtal_hw);
+
+	return ret;
+}
+
+static int armada_3700_xtal_clock_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+
+	return 0;
+}
+
+static const struct of_device_id armada_3700_xtal_clock_of_match[] = {
+	{ .compatible = "marvell,armada-3700-xtal-clock", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, armada_3700_xtal_clock_of_match);
+
+static struct platform_driver armada_3700_xtal_clock_driver = {
+	.probe = armada_3700_xtal_clock_probe,
+	.remove = armada_3700_xtal_clock_remove,
+	.driver		= {
+		.name	= "marvell-armada-3700-xtal-clock",
+		.of_match_table = armada_3700_xtal_clock_of_match,
+	},
+};
+
+module_platform_driver(armada_3700_xtal_clock_driver);
+
+MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Armada 37xx SoC xtal clocks driver");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

[PATCH v2 3/6] dt-bindings: clock: add DT binding for the TBG clocks on Armada 3700

[Gregory CLEMENT]

This commit adds the DT binding documentation for the Time Base Generator
clock used in the Marvell Armada 3700 SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/armada3700-tbg-clock.txt        | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-tbg-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/armada3700-tbg-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-tbg-clock.txt
new file mode 100644
index 000000000000..0ba1d83ff363
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/armada3700-tbg-clock.txt
@@ -0,0 +1,27 @@
+* Time Base Generator Clock bindings for Marvell Armada 37xx SoCs
+
+Marvell Armada 37xx SoCs provde Time Base Generator clocks which are
+used as parent clocks for the peripheral clocks.
+
+The TBG clock consumer should 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 on Armada 3700:
+ 0 = TBG A P
+ 1 = TBG B P
+ 2 = TBG A S
+ 3 = TBG B S
+
+Required properties:
+- compatible : shall be "marvell,armada-3700-tbg-clock"
+- reg : must be the register address of North Bridge PLL register
+- #clock-cells : from common clock binding; shall be set to 1
+
+Example:
+
+tbg: tbg@13200 {
+	compatible = "marvell,armada-3700-tbg-clock";
+	reg = <0x13200 0x1000>;
+	clocks = <&xtalclk>;
+	#clock-cells = <1>;
+};
-- 
2.5.0

[PATCH v2 4/6] clk: mvebu Add the time base generator clocks for Armada 3700

[Gregory CLEMENT]

These clocks are children of the xtal clock and each one can be selected
as a source for the peripheral clocks.

According to the datasheet it should be possible to modify their rate,
but currently it is not supported.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/mvebu/Makefile          |   1 +
 drivers/clk/mvebu/armada-37xx-tbg.c | 165 ++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 drivers/clk/mvebu/armada-37xx-tbg.c

diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 4257a36d0219..72e3512a9d4a 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ARMADA_375_CLK)	+= armada-375.o
 obj-$(CONFIG_ARMADA_38X_CLK)	+= armada-38x.o
 obj-$(CONFIG_ARMADA_39X_CLK)	+= armada-39x.o
 obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-xtal.o
+obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-tbg.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
 obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
 obj-$(CONFIG_ARMADA_CP110_SYSCON) += cp110-system-controller.o
diff --git a/drivers/clk/mvebu/armada-37xx-tbg.c b/drivers/clk/mvebu/armada-37xx-tbg.c
new file mode 100644
index 000000000000..f4af12593afa
--- /dev/null
+++ b/drivers/clk/mvebu/armada-37xx-tbg.c
@@ -0,0 +1,165 @@
+/*
+ * Marvell Armada 37xx SoC Time Base Generator clocks
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@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/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define NUM_TBG	    4
+
+#define TBG_CTRL0		0x4
+#define TBG_CTRL1		0x8
+#define TBG_CTRL7		0x20
+#define TBG_CTRL8		0x30
+
+#define TBG_DIV_MASK		0x1FF
+
+#define TBG_A_REFDIV		0
+#define TBG_B_REFDIV		16
+
+#define TBG_A_FBDIV		2
+#define TBG_B_FBDIV		18
+
+#define TBG_A_VCODIV_SE		0
+#define TBG_B_VCODIV_SE		16
+
+#define TBG_A_VCODIV_DIFF	1
+#define TBG_B_VCODIV_DIFF	17
+
+struct tbg_def {
+	char *name;
+	u32 refdiv_offset;
+	u32 fbdiv_offset;
+	u32 vcodiv_reg;
+	u32 vcodiv_offset;
+};
+
+const struct tbg_def tbg[NUM_TBG] = {
+	{"TBG-A-P", TBG_A_REFDIV, TBG_A_FBDIV, TBG_CTRL8, TBG_A_VCODIV_DIFF},
+	{"TBG-B-P", TBG_B_REFDIV, TBG_B_FBDIV, TBG_CTRL8, TBG_B_VCODIV_DIFF},
+	{"TBG-A-S", TBG_A_REFDIV, TBG_A_FBDIV, TBG_CTRL1, TBG_A_VCODIV_SE},
+	{"TBG-B-S", TBG_B_REFDIV, TBG_B_FBDIV, TBG_CTRL1, TBG_B_VCODIV_SE},
+};
+
+unsigned int tbg_get_mult(void __iomem *reg, const struct tbg_def *ptbg)
+{
+	u32 val;
+
+	val = readl(reg + TBG_CTRL0);
+
+	return ((val >> ptbg->fbdiv_offset) & TBG_DIV_MASK) << 2;
+}
+
+unsigned int tbg_get_div(void __iomem *reg, const struct tbg_def *ptbg)
+{
+	u32 val;
+	unsigned int div;
+
+	val = readl(reg + TBG_CTRL7);
+
+	div = (val >> ptbg->refdiv_offset) & TBG_DIV_MASK;
+	if (div == 0)
+		div = 1;
+	val = readl(reg + ptbg->vcodiv_reg);
+
+	div *= 1 << ((val >>  ptbg->vcodiv_offset) & TBG_DIV_MASK);
+
+	return div;
+}
+
+
+static int armada_3700_tbg_clock_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk_hw_onecell_data *hw_tbg_data;
+	struct device *dev = &pdev->dev;
+	const char *parent_name;
+	struct resource *res;
+	struct clk *parent;
+	void __iomem *reg;
+	int i, ret;
+
+	hw_tbg_data = devm_kzalloc(&pdev->dev, sizeof(*hw_tbg_data)
+				   + sizeof(*hw_tbg_data->hws) * NUM_TBG,
+				   GFP_KERNEL);
+	if (!hw_tbg_data)
+		return -ENOMEM;
+	hw_tbg_data->num = NUM_TBG;
+	platform_set_drvdata(pdev, hw_tbg_data);
+
+	parent = devm_clk_get(dev, 0);
+	if (IS_ERR(parent)) {
+		dev_err(dev, "Could get the clock parent\n");
+		return -EINVAL;
+	}
+	parent_name = __clk_get_name(parent);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	for (i = 0; i < NUM_TBG; i++) {
+		const char *name;
+		unsigned int mult, div;
+
+		name = tbg[i].name;
+		mult = tbg_get_mult(reg, &tbg[i]);
+		div = tbg_get_div(reg, &tbg[i]);
+		hw_tbg_data->hws[i] = clk_hw_register_fixed_factor(NULL, name,
+						parent_name, 0, mult, div);
+		if (IS_ERR(hw_tbg_data->hws[i]))
+			dev_err(dev, "Can't register TBG clock %s\n", name);
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_tbg_data);
+
+	return ret;
+}
+
+static int armada_3700_tbg_clock_remove(struct platform_device *pdev)
+{
+	int i;
+	struct clk_hw_onecell_data *hw_tbg_data = platform_get_drvdata(pdev);
+
+	of_clk_del_provider(pdev->dev.of_node);
+	for (i = 0; i < hw_tbg_data->num; i++)
+		clk_hw_unregister_fixed_factor(hw_tbg_data->hws[i]);
+
+	return 0;
+}
+
+static const struct of_device_id armada_3700_tbg_clock_of_match[] = {
+	{ .compatible = "marvell,armada-3700-tbg-clock", },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, armada_3700_tbg_clock_of_match);
+
+static struct platform_driver armada_3700_tbg_clock_driver = {
+	.probe = armada_3700_tbg_clock_probe,
+	.remove = armada_3700_tbg_clock_remove,
+	.driver		= {
+		.name	= "marvell-armada-3700-tbg-clock",
+		.of_match_table = armada_3700_tbg_clock_of_match,
+	},
+};
+
+module_platform_driver(armada_3700_tbg_clock_driver);
+
+MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Armada 37xx SoC Time Base Generator driver");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

[PATCH v2 5/6] dt-bindings: clock: add DT binding for the peripheral clocks on Armada 3700

[Gregory CLEMENT]

This commit adds the DT binding documentation for the peripheral clocks
used in the Marvell Armada 3700 SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/armada3700-periph-clock.txt     | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
new file mode 100644
index 000000000000..1e3370ba189f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
@@ -0,0 +1,70 @@
+* Peripheral Clock bindings for Marvell Armada 37xx SoCs
+
+Marvell Armada 37xx SoCs provide peripheral clocks which are
+used as clock source for the peripheral of the SoC.
+
+There are two different blocks associated to north bridge and south
+bridge.
+
+The peripheral clock consumer should specify the desired clock by
+having the clock ID in its "clocks" phandle cell.
+
+The following is a list of provided IDs for Armada 370 North bridge clocks:
+ID	Clock name	Description
+-----------------------------------
+0	mmc		MMC controller
+1	sata_host	Sata Host
+2	sec_at		Security AT
+3	sac_dap		Security DAP
+4	tsecm		Security Engine
+5	setm_tmx	Serial Embedded Trace Module
+6	avs		Adaptive Voltage Scaling
+7	sqf		SPI
+8	pwm		PWM
+9	i2c_2		I2C 2
+10	i2c_1		I2C 1
+11	ddr_phy		DDR PHY
+12	ddr_fclk	DDR F clock
+13	trace		Trace
+14	counter		Counter
+15	eip97		EIP 97
+16	cpu		CPU
+
+The following is a list of provided IDs for Armada 370 South bridge clocks:
+ID	Clock name	Description
+-----------------------------------
+0	gbe-50		50 MHz parent clock for Gigabit Ethernet
+1	gbe-core	parent clock for Gigabit Ethernet core
+2	gbe-125		125 MHz parent clock for Gigabit Ethernet
+3	gbe1-50		50 MHz clock for Gigabit Ethernet port 1
+4	gbe0-50		50 MHz clock for Gigabit Ethernet port 0
+5	gbe1-125	125 MHz clock for Gigabit Ethernet port 1
+6	gbe0-125	125 MHz clock for Gigabit Ethernet port 0
+7	gbe1-core	Gigabit Ethernet core port 1
+8	gbe0-core	Gigabit Ethernet core port 0
+9	gbe-bm		Gigabit Ethernet Buffer Manager
+10	sdio		SDIO
+11	usb32-sub2-sys	USB 2 clock
+12	usb32-ss-sys	USB 3 clock
+
+Required properties:
+
+- compatible : shall be "marvell,armada-3700-periph-clock-nb" for the
+  north bridge block, or
+  "marvell,armada-3700-periph-clock-sb" for the south bridge block
+- reg : must be the register address of North/South Bridge Clock register
+- #clock-cells : from common clock binding; shall be set to 1
+
+- clocks : list of the parent clock phandle in the following order:
+  TBG-A P, TBG-B P, TBG-A S, TBG-B S and finally the xtal clock.
+
+
+Example:
+
+nb_perih_clk: nb-periph-clk@13000{
+	compatible = "marvell,armada-3700-periph-clock-nb";
+	reg = <0x13000 0x1000>;
+	clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>,
+	<&tbg 3>, <&xtalclk>;
+	#clock-cells = <1>;
+};
-- 
2.5.0

[PATCH v2 6/6] clk: mvebu: Add the peripheral clock driver for Armada 3700

[Gregory CLEMENT]

These clocks are the ones which will be used as source for the
peripherals of the Armada 3700 SoC. On this SoC there is two blocks of
clocks: the North bridge one and the South bridge one.

Most of them are gatable. Most of the time their rate are their parent
rated divided by a ratio depending of two registers. Their parent can be
choose between the TBG clocks for most of them.

However, some of them can't choose their parent or directly depend of the
xtal clocks. Other ones do not use exactly the same pattern to find the
ratio between their parent rate and their rate.

For these reason each clock is a composite clock and the operations they
use are different depending of the clock.

According to the datasheet it would be possible to select the parent
clock and the ratio, however currently the driver does not support it.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/mvebu/Makefile             |   1 +
 drivers/clk/mvebu/armada-37xx-periph.c | 457 +++++++++++++++++++++++++++++++++
 2 files changed, 458 insertions(+)
 create mode 100644 drivers/clk/mvebu/armada-37xx-periph.c

diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 72e3512a9d4a..d9ae97fb43c4 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARMADA_38X_CLK)	+= armada-38x.o
 obj-$(CONFIG_ARMADA_39X_CLK)	+= armada-39x.o
 obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-xtal.o
 obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-tbg.o
+obj-$(CONFIG_ARMADA_37XX_CLK)	+= armada-37xx-periph.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
 obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
 obj-$(CONFIG_ARMADA_CP110_SYSCON) += cp110-system-controller.o
diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
new file mode 100644
index 000000000000..d8c2330fd685
--- /dev/null
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -0,0 +1,457 @@
+/*
+ * Marvell Armada 37xx SoC Peripheral clocks
+ *
+ * Copyright (C) 2016 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@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.
+ *
+ * Most of the peripheral clocks can be modelled like this:
+ *             _____    _______    _______
+ * TBG-A-P  --|     |  |       |  |       |   ______
+ * TBG-B-P  --| Mux |--| /div1 |--| /div2 |--| Gate |--> perip_clk
+ * TBG-A-S  --|     |  |       |  |       |  |______|
+ * TBG-B-S  --|_____|  |_______|  |_______|
+ *
+ * However some clocks may use only one or two block or and use the
+ * xtal clock as parent.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define PARENT_NUM	5
+
+#define TBG_SEL		0x0
+#define DIV_SEL0	0x4
+#define DIV_SEL1	0x8
+#define DIV_SEL2	0xC
+#define CLK_SEL		0x10
+#define CLK_DIS		0x14
+
+
+#define UNUSED	0xFFFF
+#define XTAL_CHILD	0x1 /* Xtal is the only parent of the clock */
+#define TBGA_S_CHILD	0x2 /* TBG A S is the only parent of the clock */
+#define GBE_CORE_CHILD	0x3 /* GBE core is the only parent of the clock */
+#define GBE_50_CHILD	0x4 /* GBE 50 is the only parent of the clock */
+#define GBE_125_CHILD	0x5 /* GBE 125 is the only parent of the clock */
+
+struct clk_double_div {
+	struct clk_hw hw;
+	void __iomem *reg1;
+	int shift1;
+	void __iomem *reg2;
+	int shift2;
+};
+
+#define to_clk_double_div(_hw) container_of(_hw, struct clk_double_div, hw)
+
+struct clk_periph_data {
+	char *name;
+	int gate_shift;
+	int mux_shift;
+	u32 div_reg1;
+	int div_shift1;
+	u32 div_reg2;
+	int div_shift2;
+	const struct clk_div_table *table;
+	int flags;
+};
+
+static const struct clk_div_table clk_table6[] = {
+	{ .val = 1, .div = 1, },
+	{ .val = 2, .div = 2, },
+	{ .val = 3, .div = 3, },
+	{ .val = 4, .div = 4, },
+	{ .val = 5, .div = 5, },
+	{ .val = 6, .div = 6, },
+	{ .val = 0, .div = 0, }, /* last entry */
+};
+
+static const struct clk_div_table clk_table1[] = {
+	{ .val = 0, .div = 1, },
+	{ .val = 1, .div = 2, },
+	{ .val = 0, .div = 0, }, /* last entry */
+};
+
+static const struct clk_div_table clk_table2[] = {
+	{ .val = 0, .div = 2, },
+	{ .val = 1, .div = 4, },
+	{ .val = 0, .div = 0, }, /* last entry */
+};
+
+static const struct clk_periph_data data_nb[] = {
+	{ .name = "mmc",	.gate_shift =  2, .mux_shift = 0,
+	  .div_reg1 = DIV_SEL2, .div_shift1 = 16, .div_reg2 = DIV_SEL2,
+	  .div_shift2 = 13, .table = NULL, .flags = 0 },
+	{ .name = "sata_host",	.gate_shift =  3, .mux_shift = 2,
+	  .div_reg1 = DIV_SEL2, .div_shift1 = 10, .div_reg2 = DIV_SEL2,
+	  .div_shift2 = 7, .table = NULL, .flags = 0 },
+	{ .name = "sec at",	.gate_shift =  6, .mux_shift = 4,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 3, .div_reg2 = DIV_SEL1,
+	  .div_shift2 = 0, .table = NULL, .flags = 0 },
+	{ .name = "sec_dap",	.gate_shift =  7, .mux_shift = 6,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 9, .div_reg2 = DIV_SEL1,
+	  .div_shift2 = 6, .table = NULL, .flags = 0 },
+	{ .name = "tsecm",	.gate_shift =  8, .mux_shift = 8,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 15, .div_reg2 = DIV_SEL1,
+	  .div_shift2 = 12, .table = NULL, .flags = 0 },
+	{ .name = "setm_tmx",	.gate_shift =  10, .mux_shift = 10,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 18, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table6, .flags = 0 },
+	{ .name = "avs",	.gate_shift =  11, .mux_shift = UNUSED,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = XTAL_CHILD},
+	{ .name = "sqf",	.gate_shift =  12, .mux_shift = 12,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 27, .div_reg2 = DIV_SEL1,
+	  .div_shift2 = 24, .table = NULL, .flags = 0 },
+	{ .name = "pwm",	.gate_shift =  13, .mux_shift = 14,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 3, .div_reg2 = DIV_SEL0,
+	  .div_shift2 = 0, .table = NULL, .flags = 0 },
+	{ .name = "i2c_2",	.gate_shift =  16, .mux_shift = UNUSED,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = XTAL_CHILD },
+	{ .name = "i2c_1",	.gate_shift =  17, .mux_shift = UNUSED,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = XTAL_CHILD },
+	{ .name = "ddr_phy",	.gate_shift =  19, .mux_shift = UNUSED,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 18, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table2, .flags = TBGA_S_CHILD },
+	{ .name = "ddr_fclk",	.gate_shift =  21, .mux_shift = 16,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 15, .div_reg2 = DIV_SEL0,
+	  .div_shift2 = 12, .table = NULL, .flags = 0 },
+	{ .name = "trace",	.gate_shift =  22, .mux_shift = 18,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 20, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table6, .flags = 0 },
+	{ .name = "counter",	.gate_shift =  23, .mux_shift = 20,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 23, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table6, .flags = 0 },
+	{ .name = "eip97",	.gate_shift =  24, .mux_shift = 24,
+	  .div_reg1 = DIV_SEL2, .div_shift1 = 22, .div_reg2 = DIV_SEL2,
+	  .div_shift2 = 19, .table = NULL, .flags = 0 },
+	{ .name = "cpu",	.gate_shift =  UNUSED, .mux_shift = 22,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 28, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table6, .flags = 0 },
+	{ },
+};
+
+static const struct clk_periph_data data_sb[] = {
+	{ .name = "gbe-50",	.gate_shift =  UNUSED, .mux_shift = 6,
+	  .div_reg1 = DIV_SEL2, .div_shift1 = 6, .div_reg2 = DIV_SEL2,
+	  .div_shift2 = 9, .table = NULL, .flags = 0 },
+	{ .name = "gbe-core",	.gate_shift =  UNUSED, .mux_shift = 8,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 18, .div_reg2 = DIV_SEL1,
+	  .div_shift2 = 21, .table = NULL, .flags = 0 },
+	{ .name = "gbe-125",	.gate_shift =  UNUSED, .mux_shift = 10,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 6, .div_reg2 = DIV_SEL1,
+	  .div_shift2 = 9, .table = NULL, .flags = 0 },
+	{ .name = "gbe1-50",	.gate_shift =  0, .mux_shift = 0,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = GBE_50_CHILD },
+	{ .name = "gbe0-50",	.gate_shift =  1, .mux_shift = 2,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = GBE_50_CHILD },
+	{ .name = "gbe1-125",	.gate_shift =  2, .mux_shift = 4,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = GBE_125_CHILD },
+	{ .name = "gbe0-125",	.gate_shift =  3, .mux_shift = 6,
+	  .div_reg1 = UNUSED, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = NULL, .flags = GBE_125_CHILD },
+	{ .name = "gbe1-core",	.gate_shift =  4, .mux_shift = 8,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 13, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table1, .flags = GBE_CORE_CHILD },
+	{ .name = "gbe0-core",	.gate_shift =  5, .mux_shift = 10,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 14, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table1, .flags = GBE_CORE_CHILD },
+	{ .name = "gbe-bm",	.gate_shift =  12, .mux_shift = UNUSED,
+	  .div_reg1 = DIV_SEL1, .div_shift1 = 0, .div_reg2 = UNUSED,
+	  .div_shift2 = 0, .table = clk_table1, .flags = GBE_CORE_CHILD },
+	{ .name = "sdio",	.gate_shift =  11, .mux_shift = 14,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 3, .div_reg2 = DIV_SEL0,
+	  .div_shift2 = 6, .table = NULL, .flags = 0 },
+	{ .name = "usb32-usb2-sys",	.gate_shift =  16, .mux_shift = 16,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 9, .div_reg2 = DIV_SEL0,
+	  .div_shift2 = 12, .table = NULL, .flags = 0 },
+	{ .name = "usb32-ss-sys",	.gate_shift =  17, .mux_shift = 18,
+	  .div_reg1 = DIV_SEL0, .div_shift1 = 15, .div_reg2 = DIV_SEL0,
+	  .div_shift2 = 18, .table = NULL, .flags = 0 },
+	{  },
+};
+
+static const char * const gbe_name[] = {
+	"gbe-50", "gbe-core", "gbe-125",
+};
+
+struct clk_periph_driver_data {
+	struct clk_hw_onecell_data *hw_data;
+	spinlock_t lock;
+};
+
+static unsigned int get_div(void __iomem *reg, int shift)
+{
+	u32 val;
+
+	val = (readl(reg) >> shift) & 0x7;
+	if (val > 6)
+		return 0;
+	return val;
+}
+
+static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	struct clk_double_div *double_div = to_clk_double_div(hw);
+	unsigned int div;
+
+	div = get_div(double_div->reg1, double_div->shift1);
+	div *= get_div(double_div->reg2, double_div->shift2);
+
+	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
+}
+
+static const struct clk_ops clk_double_div_ops = {
+	.recalc_rate = clk_double_div_recalc_rate,
+};
+
+static int armada_3700_add_composite_clk(const struct clk_periph_data *data,
+					 const char * const *parent_name,
+					 void __iomem *reg, spinlock_t *lock,
+					 struct device *dev, struct clk_hw *hw)
+{
+	const struct clk_ops *mux_ops = NULL, *gate_ops = NULL,
+		*div_ops = NULL;
+	struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *div_hw = NULL;
+	const char * const *names;
+	struct clk_mux *mux = NULL;
+	struct clk_gate *gate = NULL;
+	struct clk_divider *div = NULL;
+	struct clk_double_div *double_div = NULL;
+	int num_parent;
+	int ret = 0;
+
+	if (data->gate_shift != UNUSED) {
+		gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
+
+		if (!gate)
+			return -ENOMEM;
+
+		gate->reg = reg + CLK_DIS;
+		gate->bit_idx = data->gate_shift;
+		gate->lock = lock;
+		gate_ops = &clk_gate_ops;
+		gate_hw = &gate->hw;
+	}
+
+	if (data->mux_shift != UNUSED) {
+		mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+
+		if (!mux) {
+			ret = -ENOMEM;
+			goto free_gate;
+		}
+
+		mux->reg = reg + TBG_SEL;
+		mux->shift = data->mux_shift;
+		mux->mask = 0x3;
+		mux->lock = lock;
+		mux_ops = &clk_mux_ro_ops;
+		mux_hw = &mux->hw;
+	}
+
+	if (data->div_reg1 != UNUSED) {
+		if (data->div_reg2 == UNUSED) {
+			const struct clk_div_table *clkt;
+			int table_size = 0;
+
+			div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+			if (!div) {
+				ret = -ENOMEM;
+				goto free_mux;
+			}
+
+			div->reg = reg + data->div_reg1;
+			div->table = data->table;
+			for (clkt = div->table; clkt->div; clkt++)
+				table_size++;
+			div->width = order_base_2(table_size);
+			div->lock = lock;
+			div_ops = &clk_divider_ro_ops;
+			div_hw = &div->hw;
+		} else {
+			double_div = devm_kzalloc(dev, sizeof(*double_div),
+						  GFP_KERNEL);
+			if (!double_div) {
+				ret = -ENOMEM;
+				goto free_mux;
+			}
+
+			double_div->reg1 = reg + data->div_reg1;
+			double_div->shift1 = data->div_shift1;
+			double_div->reg2 = reg + data->div_reg1;
+			double_div->shift2 = data->div_shift2;
+			div_ops = &clk_double_div_ops;
+			div_hw = &double_div->hw;
+		}
+	}
+
+	switch (data->flags) {
+	case XTAL_CHILD:
+		/* the xtal clock is the 5th clock */
+		names = &parent_name[4];
+		num_parent = 1;
+		break;
+	case TBGA_S_CHILD:
+		/* the TBG A S clock is the 3rd clock */
+		names = &parent_name[2];
+		num_parent = 1;
+		break;
+	case GBE_CORE_CHILD:
+		names = &gbe_name[1];
+		num_parent = 1;
+		break;
+	case  GBE_50_CHILD:
+		names = &gbe_name[0];
+		num_parent = 1;
+		break;
+	case  GBE_125_CHILD:
+		names = &gbe_name[2];
+		num_parent = 1;
+		break;
+	default:
+		names = parent_name;
+		num_parent = 4;
+	}
+	hw = clk_hw_register_composite(dev, data->name,
+				     names, num_parent,
+				     mux_hw, mux_ops,
+				     div_hw, div_ops,
+				     gate_hw, gate_ops,
+				     CLK_IGNORE_UNUSED);
+	if (IS_ERR(hw)) {
+		ret = PTR_ERR(hw);
+		goto free_div;
+	}
+
+	return 0;
+free_div:
+	devm_kfree(dev, div);
+	devm_kfree(dev, double_div);
+free_mux:
+	devm_kfree(dev, mux);
+free_gate:
+	devm_kfree(dev, gate);
+	return ret;
+}
+
+static const struct of_device_id armada_3700_periph_clock_of_match[] = {
+	{ .compatible = "marvell,armada-3700-periph-clock-nb",
+	  .data = data_nb, },
+	{ .compatible = "marvell,armada-3700-periph-clock-sb",
+	  .data = data_sb, },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, armada_3700_periph_clock_of_match);
+
+static int armada_3700_periph_clock_probe(struct platform_device *pdev)
+{
+	struct clk_periph_driver_data *driver_data;
+	struct device_node *np = pdev->dev.of_node;
+	const char *parent_name[PARENT_NUM];
+	const struct clk_periph_data *data;
+	struct device *dev = &pdev->dev;
+	int num_periph = 0, i, ret;
+	struct resource *res;
+	void __iomem *reg;
+
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	while (data[num_periph].name)
+		num_periph++;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg)) {
+		dev_err(dev, "Could not map the periph clock registers\n");
+		return PTR_ERR(reg);
+	}
+
+	ret = of_clk_parent_fill(np, parent_name, PARENT_NUM);
+	if (ret != PARENT_NUM) {
+		dev_err(dev, "Could not retrieve the parents\n");
+		return -EINVAL;
+	}
+
+	driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
+	if (!driver_data)
+		return -ENOMEM;
+
+	driver_data->hw_data = devm_kzalloc(dev, sizeof(*driver_data->hw_data) +
+			    sizeof(*driver_data->hw_data->hws) * num_periph,
+			    GFP_KERNEL);
+	if (!driver_data->hw_data)
+		return -ENOMEM;
+	driver_data->hw_data->num = num_periph;
+
+	spin_lock_init(&driver_data->lock);
+
+	for (i = 0; i < num_periph; i++) {
+		struct clk_hw *hw = driver_data->hw_data->hws[i];
+
+		if (armada_3700_add_composite_clk(&data[i], parent_name, reg,
+						  &driver_data->lock, dev, hw))
+			dev_err(dev, "Can't register periph clock %s\n",
+			       data[i].name);
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+				  driver_data->hw_data);
+	if (ret) {
+		for (i = 0; i < num_periph; i++)
+			clk_hw_unregister(driver_data->hw_data->hws[i]);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, driver_data);
+	return 0;
+}
+
+static int armada_3700_periph_clock_remove(struct platform_device *pdev)
+{
+	struct clk_periph_driver_data *data = platform_get_drvdata(pdev);
+	struct clk_hw_onecell_data *hw_data = data->hw_data;
+	int i;
+
+	of_clk_del_provider(pdev->dev.of_node);
+
+	for (i = 0; i < hw_data->num; i++)
+		clk_hw_unregister(hw_data->hws[i]);
+
+	return 0;
+}
+
+static struct platform_driver armada_3700_periph_clock_driver = {
+	.probe = armada_3700_periph_clock_probe,
+	.remove = armada_3700_periph_clock_remove,
+	.driver		= {
+		.name	= "marvell-armada-3700-periph-clock",
+		.of_match_table = armada_3700_periph_clock_of_match,
+	},
+};
+
+module_platform_driver(armada_3700_periph_clock_driver);
+
+MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Armada 37xx SoC Peripheral clocks driver");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [PATCH v2 1/6] dt-bindings: clock: add DT binding for the Xtal clock on Armada 3700

[Thomas Petazzoni]

Hello,

On Fri,  8 Jul 2016 00:37:46 +0200, Gregory CLEMENT wrote:

> +gpio1: gpio@13800 {
> +	compatible = "marvell,mvebu-gpio-3700",	"syscon", "simple-mfd";

I find this compatible string not very consistent with what we do for
other drivers, it should have been:

	marvell,armada-3700-gpio

or something like that.


> +	xtalclk: xtal-clk {
> +		compatible = "marvell,armada-3700-xtal-clock";

See here for example.

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

Re: [PATCH v2 2/6] clk: mvebu: Add the xtal clock for Armada 3700 SoC

[Michael Turquette]

Quoting Gregory CLEMENT (2016-07-07 15:37:47)
> This clock is the parent of all the Armada 3700 clocks. It is a fixed
> rate clock which depends on the gpio configuration read when resetting
> the SoC.
> =

> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/clk/mvebu/Kconfig            |  3 ++
>  drivers/clk/mvebu/Makefile           |  1 +
>  drivers/clk/mvebu/armada-37xx-xtal.c | 98 ++++++++++++++++++++++++++++++=
++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/clk/mvebu/armada-37xx-xtal.c
> =

> diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
> index 3165da77d525..fddc8ac5faff 100644
> --- a/drivers/clk/mvebu/Kconfig
> +++ b/drivers/clk/mvebu/Kconfig
> @@ -24,6 +24,9 @@ config ARMADA_39X_CLK
>         bool
>         select MVEBU_CLK_COMMON
>  =

> +config ARMADA_37XX_CLK
> +       bool
> +
>  config ARMADA_XP_CLK
>         bool
>         select MVEBU_CLK_COMMON
> diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
> index 7172ef65693d..4257a36d0219 100644
> --- a/drivers/clk/mvebu/Makefile
> +++ b/drivers/clk/mvebu/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARMADA_370_CLK)    +=3D armada-370.o
>  obj-$(CONFIG_ARMADA_375_CLK)   +=3D armada-375.o
>  obj-$(CONFIG_ARMADA_38X_CLK)   +=3D armada-38x.o
>  obj-$(CONFIG_ARMADA_39X_CLK)   +=3D armada-39x.o
> +obj-$(CONFIG_ARMADA_37XX_CLK)  +=3D armada-37xx-xtal.o
>  obj-$(CONFIG_ARMADA_XP_CLK)    +=3D armada-xp.o
>  obj-$(CONFIG_ARMADA_AP806_SYSCON) +=3D ap806-system-controller.o
>  obj-$(CONFIG_ARMADA_CP110_SYSCON) +=3D cp110-system-controller.o
> diff --git a/drivers/clk/mvebu/armada-37xx-xtal.c b/drivers/clk/mvebu/arm=
ada-37xx-xtal.c
> new file mode 100644
> index 000000000000..f832c219420f
> --- /dev/null
> +++ b/drivers/clk/mvebu/armada-37xx-xtal.c
> @@ -0,0 +1,98 @@
> +/*
> + * Marvell Armada 37xx SoC xtal clocks
> + *
> + * Copyright (C) 2016 Marvell
> + *
> + * Gregory CLEMENT <gregory.clement@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/clk.h>

Is clk.h necessary? Is this driver also a clock consumer?

Regards,
Mike

> +#include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define NB_GPIO1_LATCH 0xC
> +#define XTAL_MODE          BIT(31)
> +
> +static int armada_3700_xtal_clock_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np =3D pdev->dev.of_node;
> +       const char *xtal_name =3D "xtal";
> +       struct device_node *parent;
> +       struct regmap *regmap;
> +       struct clk_hw *xtal_hw;
> +       unsigned int rate;
> +       u32 reg;
> +       int ret;
> +
> +       xtal_hw =3D devm_kzalloc(&pdev->dev, sizeof(*xtal_hw), GFP_KERNEL=
);
> +       if (!xtal_hw)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, xtal_hw);
> +
> +       parent =3D np->parent;
> +       if (!parent) {
> +               dev_err(&pdev->dev, "no parent\n");
> +               return -ENODEV;
> +       }
> +
> +       regmap =3D syscon_node_to_regmap(parent);
> +       if (IS_ERR(regmap)) {
> +               dev_err(&pdev->dev, "cannot get regmap\n");
> +               return PTR_ERR(regmap);
> +       }
> +
> +       ret =3D regmap_read(regmap, NB_GPIO1_LATCH, &reg);
> +       if (ret) {
> +               dev_err(&pdev->dev, "cannot read from regmap\n");
> +               return ret;
> +       }
> +
> +       if (reg & XTAL_MODE)
> +               rate =3D 40000000;
> +       else
> +               rate =3D 25000000;
> +
> +       of_property_read_string_index(np, "clock-output-names", 0, &xtal_=
name);
> +       xtal_hw =3D clk_hw_register_fixed_rate(NULL, xtal_name, NULL, 0, =
rate);
> +       if (IS_ERR(xtal_hw))
> +               return PTR_ERR(xtal_hw);
> +       ret =3D of_clk_add_hw_provider(np, of_clk_hw_simple_get, xtal_hw);
> +
> +       return ret;
> +}
> +
> +static int armada_3700_xtal_clock_remove(struct platform_device *pdev)
> +{
> +       of_clk_del_provider(pdev->dev.of_node);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id armada_3700_xtal_clock_of_match[] =3D {
> +       { .compatible =3D "marvell,armada-3700-xtal-clock", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, armada_3700_xtal_clock_of_match);
> +
> +static struct platform_driver armada_3700_xtal_clock_driver =3D {
> +       .probe =3D armada_3700_xtal_clock_probe,
> +       .remove =3D armada_3700_xtal_clock_remove,
> +       .driver         =3D {
> +               .name   =3D "marvell-armada-3700-xtal-clock",
> +               .of_match_table =3D armada_3700_xtal_clock_of_match,
> +       },
> +};
> +
> +module_platform_driver(armada_3700_xtal_clock_driver);
> +
> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
> +MODULE_DESCRIPTION("Marvell Armada 37xx SoC xtal clocks driver");
> +MODULE_LICENSE("GPL v2");
> -- =

> 2.5.0
>=20

Re: [PATCH v2 6/6] clk: mvebu: Add the peripheral clock driver for Armada 3700

[Michael Turquette]

Quoting Gregory CLEMENT (2016-07-07 15:37:51)
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>

Same question as my previous email. Is clk.h necessary? Is this driver
also a clk consumer?

> +static int armada_3700_add_composite_clk(const struct clk_periph_data *d=
ata,
> +                                        const char * const *parent_name,
> +                                        void __iomem *reg, spinlock_t *l=
ock,
> +                                        struct device *dev, struct clk_h=
w *hw)
> +{
> +       const struct clk_ops *mux_ops =3D NULL, *gate_ops =3D NULL,
> +               *div_ops =3D NULL;
> +       struct clk_hw *mux_hw =3D NULL, *gate_hw =3D NULL, *div_hw =3D NU=
LL;
> +       const char * const *names;
> +       struct clk_mux *mux =3D NULL;
> +       struct clk_gate *gate =3D NULL;
> +       struct clk_divider *div =3D NULL;
> +       struct clk_double_div *double_div =3D NULL;
> +       int num_parent;
> +       int ret =3D 0;
> +
> +       if (data->gate_shift !=3D UNUSED) {
> +               gate =3D devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> +
> +               if (!gate)
> +                       return -ENOMEM;
> +
> +               gate->reg =3D reg + CLK_DIS;
> +               gate->bit_idx =3D data->gate_shift;
> +               gate->lock =3D lock;
> +               gate_ops =3D &clk_gate_ops;
> +               gate_hw =3D &gate->hw;
> +       }
> +
> +       if (data->mux_shift !=3D UNUSED) {
> +               mux =3D devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +
> +               if (!mux) {
> +                       ret =3D -ENOMEM;
> +                       goto free_gate;
> +               }
> +
> +               mux->reg =3D reg + TBG_SEL;
> +               mux->shift =3D data->mux_shift;
> +               mux->mask =3D 0x3;
> +               mux->lock =3D lock;
> +               mux_ops =3D &clk_mux_ro_ops;
> +               mux_hw =3D &mux->hw;
> +       }
> +
> +       if (data->div_reg1 !=3D UNUSED) {
> +               if (data->div_reg2 =3D=3D UNUSED) {
> +                       const struct clk_div_table *clkt;
> +                       int table_size =3D 0;
> +
> +                       div =3D devm_kzalloc(dev, sizeof(*div), GFP_KERNE=
L);
> +                       if (!div) {
> +                               ret =3D -ENOMEM;
> +                               goto free_mux;
> +                       }
> +
> +                       div->reg =3D reg + data->div_reg1;
> +                       div->table =3D data->table;
> +                       for (clkt =3D div->table; clkt->div; clkt++)
> +                               table_size++;
> +                       div->width =3D order_base_2(table_size);
> +                       div->lock =3D lock;
> +                       div_ops =3D &clk_divider_ro_ops;
> +                       div_hw =3D &div->hw;
> +               } else {
> +                       double_div =3D devm_kzalloc(dev, sizeof(*double_d=
iv),
> +                                                 GFP_KERNEL);
> +                       if (!double_div) {
> +                               ret =3D -ENOMEM;
> +                               goto free_mux;
> +                       }
> +
> +                       double_div->reg1 =3D reg + data->div_reg1;
> +                       double_div->shift1 =3D data->div_shift1;
> +                       double_div->reg2 =3D reg + data->div_reg1;
> +                       double_div->shift2 =3D data->div_shift2;
> +                       div_ops =3D &clk_double_div_ops;
> +                       div_hw =3D &double_div->hw;
> +               }
> +       }
> +
> +       switch (data->flags) {
> +       case XTAL_CHILD:
> +               /* the xtal clock is the 5th clock */
> +               names =3D &parent_name[4];
> +               num_parent =3D 1;
> +               break;
> +       case TBGA_S_CHILD:
> +               /* the TBG A S clock is the 3rd clock */
> +               names =3D &parent_name[2];
> +               num_parent =3D 1;
> +               break;
> +       case GBE_CORE_CHILD:
> +               names =3D &gbe_name[1];
> +               num_parent =3D 1;
> +               break;
> +       case  GBE_50_CHILD:
> +               names =3D &gbe_name[0];
> +               num_parent =3D 1;
> +               break;
> +       case  GBE_125_CHILD:
> +               names =3D &gbe_name[2];
> +               num_parent =3D 1;
> +               break;
> +       default:
> +               names =3D parent_name;
> +               num_parent =3D 4;
> +       }
> +       hw =3D clk_hw_register_composite(dev, data->name,
> +                                    names, num_parent,
> +                                    mux_hw, mux_ops,
> +                                    div_hw, div_ops,
> +                                    gate_hw, gate_ops,
> +                                    CLK_IGNORE_UNUSED);
> +       if (IS_ERR(hw)) {
> +               ret =3D PTR_ERR(hw);
> +               goto free_div;
> +       }
> +
> +       return 0;
> +free_div:
> +       devm_kfree(dev, div);
> +       devm_kfree(dev, double_div);
> +free_mux:
> +       devm_kfree(dev, mux);
> +free_gate:
> +       devm_kfree(dev, gate);
> +       return ret;
> +}

Can this "add" function (aka registration function) be replaced with
static data instead? I think that all of the static data exists already,
this function can be removed and your probe can call clk_hw_register
directly.

It might need a macro though, since composite clock structures are
rather messy. This avoids a lot of unnecessary allocations and time
populating data that we already have access to. In general I am trying
to encourage clk drivers to use only clk_hw_register() in their probe
instead of the helper registration functions.

Similarly I am discouraging drivers from populating hw.init at run-time,
since we already have that data for that at compile-time.

Regards,
Mike

Re: [PATCH v2 2/6] clk: mvebu: Add the xtal clock for Armada 3700 SoC

[Paul Gortmaker]

On Thu, Jul 7, 2016 at 6:37 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> This clock is the parent of all the Armada 3700 clocks. It is a fixed
> rate clock which depends on the gpio configuration read when resetting
> the SoC.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/clk/mvebu/Kconfig            |  3 ++
>  drivers/clk/mvebu/Makefile           |  1 +
>  drivers/clk/mvebu/armada-37xx-xtal.c | 98 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/clk/mvebu/armada-37xx-xtal.c
>
> diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
> index 3165da77d525..fddc8ac5faff 100644
> --- a/drivers/clk/mvebu/Kconfig
> +++ b/drivers/clk/mvebu/Kconfig
> @@ -24,6 +24,9 @@ config ARMADA_39X_CLK
>         bool
>         select MVEBU_CLK_COMMON
>
> +config ARMADA_37XX_CLK
> +       bool
> +

Since the driver is not tristate, can you please remove all modular
references from it?   With the author and license etc. at the top you
can just delete the last three lines, the DEVICE_TABLE and register
with builtin_platform_driver, and then no need for module.h either.

Either that, or change it to a tristate, if that use case makes sense.

Thanks,
Paul.
--


>  config ARMADA_XP_CLK
>         bool
>         select MVEBU_CLK_COMMON
> diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
> index 7172ef65693d..4257a36d0219 100644
> --- a/drivers/clk/mvebu/Makefile
> +++ b/drivers/clk/mvebu/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARMADA_370_CLK)    += armada-370.o
>  obj-$(CONFIG_ARMADA_375_CLK)   += armada-375.o
>  obj-$(CONFIG_ARMADA_38X_CLK)   += armada-38x.o
>  obj-$(CONFIG_ARMADA_39X_CLK)   += armada-39x.o
> +obj-$(CONFIG_ARMADA_37XX_CLK)  += armada-37xx-xtal.o
>  obj-$(CONFIG_ARMADA_XP_

[...]

Re: [PATCH v2 1/6] dt-bindings: clock: add DT binding for the Xtal clock on Armada 3700

[Gregory CLEMENT]

Hi Thomas,
 
 On ven., juil. 08 2016, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Hello,
>
> On Fri,  8 Jul 2016 00:37:46 +0200, Gregory CLEMENT wrote:
>
>> +gpio1: gpio@13800 {
>> +	compatible = "marvell,mvebu-gpio-3700",	"syscon", "simple-mfd";
>
> I find this compatible string not very consistent with what we do for
> other drivers, it should have been:
>
> 	marvell,armada-3700-gpio

Thanks for pointing this. We missed it during the last review. I agree
that using marvell,armada-3700-gpio is more appropriate, especially
because the gpio controller on Armada 37xx seems to be different that
the ones used on the other mvebu SoCs.

Gregory

>
> or something like that.
>
>
>> +	xtalclk: xtal-clk {
>> +		compatible = "marvell,armada-3700-xtal-clock";
>
> See here for example.
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

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

Re: [PATCH v2 2/6] clk: mvebu: Add the xtal clock for Armada 3700 SoC

[Gregory CLEMENT]

Hi Michael,
 
 On ven., juil. 08 2016, Michael Turquette <mturquette@baylibre.com> wrote:

> Quoting Gregory CLEMENT (2016-07-07 15:37:47)
>> This clock is the parent of all the Armada 3700 clocks. It is a fixed
>> rate clock which depends on the gpio configuration read when resetting
>> the SoC.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/clk/mvebu/Kconfig            |  3 ++
>>  drivers/clk/mvebu/Makefile           |  1 +
>>  drivers/clk/mvebu/armada-37xx-xtal.c | 98 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 102 insertions(+)
>>  create mode 100644 drivers/clk/mvebu/armada-37xx-xtal.c
>> 
>> diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
>> index 3165da77d525..fddc8ac5faff 100644
>> --- a/drivers/clk/mvebu/Kconfig
>> +++ b/drivers/clk/mvebu/Kconfig
>> @@ -24,6 +24,9 @@ config ARMADA_39X_CLK
>>         bool
>>         select MVEBU_CLK_COMMON
>>  
>> +config ARMADA_37XX_CLK
>> +       bool
>> +
>>  config ARMADA_XP_CLK
>>         bool
>>         select MVEBU_CLK_COMMON
>> diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
>> index 7172ef65693d..4257a36d0219 100644
>> --- a/drivers/clk/mvebu/Makefile
>> +++ b/drivers/clk/mvebu/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARMADA_370_CLK)    += armada-370.o
>>  obj-$(CONFIG_ARMADA_375_CLK)   += armada-375.o
>>  obj-$(CONFIG_ARMADA_38X_CLK)   += armada-38x.o
>>  obj-$(CONFIG_ARMADA_39X_CLK)   += armada-39x.o
>> +obj-$(CONFIG_ARMADA_37XX_CLK)  += armada-37xx-xtal.o
>>  obj-$(CONFIG_ARMADA_XP_CLK)    += armada-xp.o
>>  obj-$(CONFIG_ARMADA_AP806_SYSCON) += ap806-system-controller.o
>>  obj-$(CONFIG_ARMADA_CP110_SYSCON) += cp110-system-controller.o
>> diff --git a/drivers/clk/mvebu/armada-37xx-xtal.c b/drivers/clk/mvebu/armada-37xx-xtal.c
>> new file mode 100644
>> index 000000000000..f832c219420f
>> --- /dev/null
>> +++ b/drivers/clk/mvebu/armada-37xx-xtal.c
>> @@ -0,0 +1,98 @@
>> +/*
>> + * Marvell Armada 37xx SoC xtal clocks
>> + *
>> + * Copyright (C) 2016 Marvell
>> + *
>> + * Gregory CLEMENT <gregory.clement@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/clk.h>
>
> Is clk.h necessary? Is this driver also a clock consumer?

This driver is not a clock consumer so it is not needed.
I will remove it.

Thanks,

Gregory

>
> Regards,
> Mike
>
>> +#include <linux/clk-provider.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#define NB_GPIO1_LATCH 0xC
>> +#define XTAL_MODE          BIT(31)
>> +
>> +static int armada_3700_xtal_clock_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       const char *xtal_name = "xtal";
>> +       struct device_node *parent;
>> +       struct regmap *regmap;
>> +       struct clk_hw *xtal_hw;
>> +       unsigned int rate;
>> +       u32 reg;
>> +       int ret;
>> +
>> +       xtal_hw = devm_kzalloc(&pdev->dev, sizeof(*xtal_hw), GFP_KERNEL);
>> +       if (!xtal_hw)
>> +               return -ENOMEM;
>> +
>> +       platform_set_drvdata(pdev, xtal_hw);
>> +
>> +       parent = np->parent;
>> +       if (!parent) {
>> +               dev_err(&pdev->dev, "no parent\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       regmap = syscon_node_to_regmap(parent);
>> +       if (IS_ERR(regmap)) {
>> +               dev_err(&pdev->dev, "cannot get regmap\n");
>> +               return PTR_ERR(regmap);
>> +       }
>> +
>> +       ret = regmap_read(regmap, NB_GPIO1_LATCH, &reg);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "cannot read from regmap\n");
>> +               return ret;
>> +       }
>> +
>> +       if (reg & XTAL_MODE)
>> +               rate = 40000000;
>> +       else
>> +               rate = 25000000;
>> +
>> +       of_property_read_string_index(np, "clock-output-names", 0, &xtal_name);
>> +       xtal_hw = clk_hw_register_fixed_rate(NULL, xtal_name, NULL, 0, rate);
>> +       if (IS_ERR(xtal_hw))
>> +               return PTR_ERR(xtal_hw);
>> +       ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, xtal_hw);
>> +
>> +       return ret;
>> +}
>> +
>> +static int armada_3700_xtal_clock_remove(struct platform_device *pdev)
>> +{
>> +       of_clk_del_provider(pdev->dev.of_node);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id armada_3700_xtal_clock_of_match[] = {
>> +       { .compatible = "marvell,armada-3700-xtal-clock", },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, armada_3700_xtal_clock_of_match);
>> +
>> +static struct platform_driver armada_3700_xtal_clock_driver = {
>> +       .probe = armada_3700_xtal_clock_probe,
>> +       .remove = armada_3700_xtal_clock_remove,
>> +       .driver         = {
>> +               .name   = "marvell-armada-3700-xtal-clock",
>> +               .of_match_table = armada_3700_xtal_clock_of_match,
>> +       },
>> +};
>> +
>> +module_platform_driver(armada_3700_xtal_clock_driver);
>> +
>> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
>> +MODULE_DESCRIPTION("Marvell Armada 37xx SoC xtal clocks driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.5.0
>> 

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

Re: [PATCH v2 2/6] clk: mvebu: Add the xtal clock for Armada 3700 SoC

[Gregory CLEMENT]

Hi Paul,
 
 On dim., juil. 10 2016, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:

> On Thu, Jul 7, 2016 at 6:37 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> This clock is the parent of all the Armada 3700 clocks. It is a fixed
>> rate clock which depends on the gpio configuration read when resetting
>> the SoC.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/clk/mvebu/Kconfig            |  3 ++
>>  drivers/clk/mvebu/Makefile           |  1 +
>>  drivers/clk/mvebu/armada-37xx-xtal.c | 98 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 102 insertions(+)
>>  create mode 100644 drivers/clk/mvebu/armada-37xx-xtal.c
>>
>> diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
>> index 3165da77d525..fddc8ac5faff 100644
>> --- a/drivers/clk/mvebu/Kconfig
>> +++ b/drivers/clk/mvebu/Kconfig
>> @@ -24,6 +24,9 @@ config ARMADA_39X_CLK
>>         bool
>>         select MVEBU_CLK_COMMON
>>
>> +config ARMADA_37XX_CLK
>> +       bool
>> +
>
> Since the driver is not tristate, can you please remove all modular
> references from it?   With the author and license etc. at the top you
> can just delete the last three lines, the DEVICE_TABLE and register
> with builtin_platform_driver, and then no need for module.h either.
>
> Either that, or change it to a tristate, if that use case makes sense.

Indeed having these clock drivers as module is not very useful. Let's
remove the modular reference.

Thanks,

Gregory


>
> Thanks,
> Paul.
> --
>
>
>>  config ARMADA_XP_CLK
>>         bool
>>         select MVEBU_CLK_COMMON
>> diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
>> index 7172ef65693d..4257a36d0219 100644
>> --- a/drivers/clk/mvebu/Makefile
>> +++ b/drivers/clk/mvebu/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARMADA_370_CLK)    += armada-370.o
>>  obj-$(CONFIG_ARMADA_375_CLK)   += armada-375.o
>>  obj-$(CONFIG_ARMADA_38X_CLK)   += armada-38x.o
>>  obj-$(CONFIG_ARMADA_39X_CLK)   += armada-39x.o
>> +obj-$(CONFIG_ARMADA_37XX_CLK)  += armada-37xx-xtal.o
>>  obj-$(CONFIG_ARMADA_XP_
>
> [...]

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

Re: [PATCH v2 6/6] clk: mvebu: Add the peripheral clock driver for Armada 3700

[Gregory CLEMENT]

Hi Michael,
 
 On ven., juil. 08 2016, Michael Turquette <mturquette@baylibre.com> wrote:

> Quoting Gregory CLEMENT (2016-07-07 15:37:51)
>> +#include <linux/clk-provider.h>
>> +#include <linux/clk.h>
>
> Same question as my previous email. Is clk.h necessary? Is this driver
> also a clk consumer?

I think I can remove it indeed.

>
>> +static int armada_3700_add_composite_clk(const struct clk_periph_data *data,
>> +                                        const char * const *parent_name,
>> +                                        void __iomem *reg, spinlock_t *lock,
>> +                                        struct device *dev, struct clk_hw *hw)
>> +{
>> +       const struct clk_ops *mux_ops = NULL, *gate_ops = NULL,
>> +               *div_ops = NULL;
>> +       struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *div_hw = NULL;
>> +       const char * const *names;
>> +       struct clk_mux *mux = NULL;
>> +       struct clk_gate *gate = NULL;
>> +       struct clk_divider *div = NULL;
>> +       struct clk_double_div *double_div = NULL;
>> +       int num_parent;
>> +       int ret = 0;
>> +
>> +       if (data->gate_shift != UNUSED) {
>> +               gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
>> +
>> +               if (!gate)
>> +                       return -ENOMEM;
>> +
>> +               gate->reg = reg + CLK_DIS;
>> +               gate->bit_idx = data->gate_shift;
>> +               gate->lock = lock;
>> +               gate_ops = &clk_gate_ops;
>> +               gate_hw = &gate->hw;
>> +       }
>> +
>> +       if (data->mux_shift != UNUSED) {
>> +               mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +
>> +               if (!mux) {
>> +                       ret = -ENOMEM;
>> +                       goto free_gate;
>> +               }
>> +
>> +               mux->reg = reg + TBG_SEL;
>> +               mux->shift = data->mux_shift;
>> +               mux->mask = 0x3;
>> +               mux->lock = lock;
>> +               mux_ops = &clk_mux_ro_ops;
>> +               mux_hw = &mux->hw;
>> +       }
>> +
>> +       if (data->div_reg1 != UNUSED) {
>> +               if (data->div_reg2 == UNUSED) {
>> +                       const struct clk_div_table *clkt;
>> +                       int table_size = 0;
>> +
>> +                       div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
>> +                       if (!div) {
>> +                               ret = -ENOMEM;
>> +                               goto free_mux;
>> +                       }
>> +
>> +                       div->reg = reg + data->div_reg1;
>> +                       div->table = data->table;
>> +                       for (clkt = div->table; clkt->div; clkt++)
>> +                               table_size++;
>> +                       div->width = order_base_2(table_size);
>> +                       div->lock = lock;
>> +                       div_ops = &clk_divider_ro_ops;
>> +                       div_hw = &div->hw;
>> +               } else {
>> +                       double_div = devm_kzalloc(dev, sizeof(*double_div),
>> +                                                 GFP_KERNEL);
>> +                       if (!double_div) {
>> +                               ret = -ENOMEM;
>> +                               goto free_mux;
>> +                       }
>> +
>> +                       double_div->reg1 = reg + data->div_reg1;
>> +                       double_div->shift1 = data->div_shift1;
>> +                       double_div->reg2 = reg + data->div_reg1;
>> +                       double_div->shift2 = data->div_shift2;
>> +                       div_ops = &clk_double_div_ops;
>> +                       div_hw = &double_div->hw;
>> +               }
>> +       }
>> +
>> +       switch (data->flags) {
>> +       case XTAL_CHILD:
>> +               /* the xtal clock is the 5th clock */
>> +               names = &parent_name[4];
>> +               num_parent = 1;
>> +               break;
>> +       case TBGA_S_CHILD:
>> +               /* the TBG A S clock is the 3rd clock */
>> +               names = &parent_name[2];
>> +               num_parent = 1;
>> +               break;
>> +       case GBE_CORE_CHILD:
>> +               names = &gbe_name[1];
>> +               num_parent = 1;
>> +               break;
>> +       case  GBE_50_CHILD:
>> +               names = &gbe_name[0];
>> +               num_parent = 1;
>> +               break;
>> +       case  GBE_125_CHILD:
>> +               names = &gbe_name[2];
>> +               num_parent = 1;
>> +               break;
>> +       default:
>> +               names = parent_name;
>> +               num_parent = 4;
>> +       }
>> +       hw = clk_hw_register_composite(dev, data->name,
>> +                                    names, num_parent,
>> +                                    mux_hw, mux_ops,
>> +                                    div_hw, div_ops,
>> +                                    gate_hw, gate_ops,
>> +                                    CLK_IGNORE_UNUSED);
>> +       if (IS_ERR(hw)) {
>> +               ret = PTR_ERR(hw);
>> +               goto free_div;
>> +       }
>> +
>> +       return 0;
>> +free_div:
>> +       devm_kfree(dev, div);
>> +       devm_kfree(dev, double_div);
>> +free_mux:
>> +       devm_kfree(dev, mux);
>> +free_gate:
>> +       devm_kfree(dev, gate);
>> +       return ret;
>> +}
>
> Can this "add" function (aka registration function) be replaced with
> static data instead? I think that all of the static data exists already,
> this function can be removed and your probe can call clk_hw_register
> directly.
>

I see your point and indeed we can remove some allocation. However using
clk_hw_register with composite clock is not straight forward. Indeed the
clk_ops is filled in the clk_hw_register_composite function and none of
these operations are exported outside the clk-composite.c file.

We can't directly point a clk_gate_ops structure or a clk_divider_ops as
you did in the drivers/clk/meson/gxbb.c file for example. For clk
composite it would need modify the framework to either export all the
operation or to create set of operations directly usable and I would
like to avoid doing it when we are close to the merge window.

However I can use static data for the clk_mux, clk_gate and clk_divider
struct.

Is it OK for you?

Thanks,

Gregory

> It might need a macro though, since composite clock structures are
> rather messy. This avoids a lot of unnecessary allocations and time
> populating data that we already have access to. In general I am trying
> to encourage clk drivers to use only clk_hw_register() in their probe
> instead of the helper registration functions.
>
> Similarly I am discouraging drivers from populating hw.init at run-time,
> since we already have that data for that at compile-time.
>
> Regards,
> Mike

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

Re: [PATCH v2 6/6] clk: mvebu: Add the peripheral clock driver for Armada 3700

[Michael Turquette]

Quoting Gregory CLEMENT (2016-07-12 09:30:34)
> Hi Michael,
>  =

>  On ven., juil. 08 2016, Michael Turquette <mturquette@baylibre.com> wrot=
e:
> =

> > Quoting Gregory CLEMENT (2016-07-07 15:37:51)
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/clk.h>
> >
> > Same question as my previous email. Is clk.h necessary? Is this driver
> > also a clk consumer?
> =

> I think I can remove it indeed.
> =

> >
> >> +static int armada_3700_add_composite_clk(const struct clk_periph_data=
 *data,
> >> +                                        const char * const *parent_na=
me,
> >> +                                        void __iomem *reg, spinlock_t=
 *lock,
> >> +                                        struct device *dev, struct cl=
k_hw *hw)
> >> +{
> >> +       const struct clk_ops *mux_ops =3D NULL, *gate_ops =3D NULL,
> >> +               *div_ops =3D NULL;
> >> +       struct clk_hw *mux_hw =3D NULL, *gate_hw =3D NULL, *div_hw =3D=
 NULL;
> >> +       const char * const *names;
> >> +       struct clk_mux *mux =3D NULL;
> >> +       struct clk_gate *gate =3D NULL;
> >> +       struct clk_divider *div =3D NULL;
> >> +       struct clk_double_div *double_div =3D NULL;
> >> +       int num_parent;
> >> +       int ret =3D 0;
> >> +
> >> +       if (data->gate_shift !=3D UNUSED) {
> >> +               gate =3D devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> >> +
> >> +               if (!gate)
> >> +                       return -ENOMEM;
> >> +
> >> +               gate->reg =3D reg + CLK_DIS;
> >> +               gate->bit_idx =3D data->gate_shift;
> >> +               gate->lock =3D lock;
> >> +               gate_ops =3D &clk_gate_ops;
> >> +               gate_hw =3D &gate->hw;
> >> +       }
> >> +
> >> +       if (data->mux_shift !=3D UNUSED) {
> >> +               mux =3D devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> >> +
> >> +               if (!mux) {
> >> +                       ret =3D -ENOMEM;
> >> +                       goto free_gate;
> >> +               }
> >> +
> >> +               mux->reg =3D reg + TBG_SEL;
> >> +               mux->shift =3D data->mux_shift;
> >> +               mux->mask =3D 0x3;
> >> +               mux->lock =3D lock;
> >> +               mux_ops =3D &clk_mux_ro_ops;
> >> +               mux_hw =3D &mux->hw;
> >> +       }
> >> +
> >> +       if (data->div_reg1 !=3D UNUSED) {
> >> +               if (data->div_reg2 =3D=3D UNUSED) {
> >> +                       const struct clk_div_table *clkt;
> >> +                       int table_size =3D 0;
> >> +
> >> +                       div =3D devm_kzalloc(dev, sizeof(*div), GFP_KE=
RNEL);
> >> +                       if (!div) {
> >> +                               ret =3D -ENOMEM;
> >> +                               goto free_mux;
> >> +                       }
> >> +
> >> +                       div->reg =3D reg + data->div_reg1;
> >> +                       div->table =3D data->table;
> >> +                       for (clkt =3D div->table; clkt->div; clkt++)
> >> +                               table_size++;
> >> +                       div->width =3D order_base_2(table_size);
> >> +                       div->lock =3D lock;
> >> +                       div_ops =3D &clk_divider_ro_ops;
> >> +                       div_hw =3D &div->hw;
> >> +               } else {
> >> +                       double_div =3D devm_kzalloc(dev, sizeof(*doubl=
e_div),
> >> +                                                 GFP_KERNEL);
> >> +                       if (!double_div) {
> >> +                               ret =3D -ENOMEM;
> >> +                               goto free_mux;
> >> +                       }
> >> +
> >> +                       double_div->reg1 =3D reg + data->div_reg1;
> >> +                       double_div->shift1 =3D data->div_shift1;
> >> +                       double_div->reg2 =3D reg + data->div_reg1;
> >> +                       double_div->shift2 =3D data->div_shift2;
> >> +                       div_ops =3D &clk_double_div_ops;
> >> +                       div_hw =3D &double_div->hw;
> >> +               }
> >> +       }
> >> +
> >> +       switch (data->flags) {
> >> +       case XTAL_CHILD:
> >> +               /* the xtal clock is the 5th clock */
> >> +               names =3D &parent_name[4];
> >> +               num_parent =3D 1;
> >> +               break;
> >> +       case TBGA_S_CHILD:
> >> +               /* the TBG A S clock is the 3rd clock */
> >> +               names =3D &parent_name[2];
> >> +               num_parent =3D 1;
> >> +               break;
> >> +       case GBE_CORE_CHILD:
> >> +               names =3D &gbe_name[1];
> >> +               num_parent =3D 1;
> >> +               break;
> >> +       case  GBE_50_CHILD:
> >> +               names =3D &gbe_name[0];
> >> +               num_parent =3D 1;
> >> +               break;
> >> +       case  GBE_125_CHILD:
> >> +               names =3D &gbe_name[2];
> >> +               num_parent =3D 1;
> >> +               break;
> >> +       default:
> >> +               names =3D parent_name;
> >> +               num_parent =3D 4;
> >> +       }
> >> +       hw =3D clk_hw_register_composite(dev, data->name,
> >> +                                    names, num_parent,
> >> +                                    mux_hw, mux_ops,
> >> +                                    div_hw, div_ops,
> >> +                                    gate_hw, gate_ops,
> >> +                                    CLK_IGNORE_UNUSED);
> >> +       if (IS_ERR(hw)) {
> >> +               ret =3D PTR_ERR(hw);
> >> +               goto free_div;
> >> +       }
> >> +
> >> +       return 0;
> >> +free_div:
> >> +       devm_kfree(dev, div);
> >> +       devm_kfree(dev, double_div);
> >> +free_mux:
> >> +       devm_kfree(dev, mux);
> >> +free_gate:
> >> +       devm_kfree(dev, gate);
> >> +       return ret;
> >> +}
> >
> > Can this "add" function (aka registration function) be replaced with
> > static data instead? I think that all of the static data exists already,
> > this function can be removed and your probe can call clk_hw_register
> > directly.
> >
> =

> I see your point and indeed we can remove some allocation. However using
> clk_hw_register with composite clock is not straight forward. Indeed the
> clk_ops is filled in the clk_hw_register_composite function and none of
> these operations are exported outside the clk-composite.c file.
> =

> We can't directly point a clk_gate_ops structure or a clk_divider_ops as
> you did in the drivers/clk/meson/gxbb.c file for example. For clk
> composite it would need modify the framework to either export all the
> operation or to create set of operations directly usable and I would
> like to avoid doing it when we are close to the merge window.
> =

> However I can use static data for the clk_mux, clk_gate and clk_divider
> struct.
> =

> Is it OK for you?

Yes, that sounds good. Let's convert the ones we can for now. Some day
we might have a better solution for the composite clock stuff.

Regards,
Mike

> =

> Thanks,
> =

> Gregory
> =

> > It might need a macro though, since composite clock structures are
> > rather messy. This avoids a lot of unnecessary allocations and time
> > populating data that we already have access to. In general I am trying
> > to encourage clk drivers to use only clk_hw_register() in their probe
> > instead of the helper registration functions.
> >
> > Similarly I am discouraging drivers from populating hw.init at run-time,
> > since we already have that data for that at compile-time.
> >
> > Regards,
> > Mike
> =

> -- =

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